linux-kernel - Re: [PATCH v6 2/3] i915/gvt: Save the initial HW state snapshot in i915

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 9 Feb 2022 19:50:09 +0000
From:   "Wang, Zhi A" <zhi.a.wang@...el.com>
To:     Christoph Hellwig <hch@....de>, Zhi Wang <zhi.wang.linux@...il.com>
CC:     "intel-gfx@...ts.freedesktop.org" <intel-gfx@...ts.freedesktop.org>,
        Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "jani.nikula@...ux.intel.com" <jani.nikula@...ux.intel.com>,
        Zhenyu Wang <zhenyuw@...ux.intel.com>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "jgg@...dia.com" <jgg@...dia.com>,
        "Vivi, Rodrigo" <rodrigo.vivi@...el.com>,
        "intel-gvt-dev@...ts.freedesktop.org" 
        <intel-gvt-dev@...ts.freedesktop.org>
Subject: Re: [PATCH v6 2/3] i915/gvt: Save the initial HW state snapshot in
 i915

On 2/9/22 7:32 AM, Christoph Hellwig wrote:
> On Tue, Feb 08, 2022 at 06:11:50AM -0500, Zhi Wang wrote:
>> +	struct drm_i915_private *dev_priv = iter->i915;
>> +	u32 *mmio, i;
>> +
>> +	for (i = offset; i < offset + size; i += 4) {
>> +		mmio = iter->data + i;
>> +		*mmio = intel_uncore_read_notrace(to_gt(dev_priv)->uncore,
>> +						  _MMIO(i));
> 
> This reads much stranger than:
> 
> 	u32 *mmio = iter->data;
> 
> 	for (i = offset; i < offset + size; i += 4) {
> 		mmio[i] = intel_uncore_read_notrace(to_gt(dev_priv)->uncore,
> 						    _MMIO(i));
> 	}
> 
I am not sure the suggestion is correct. That's the reason why I didn't take the comments in the previous review.

if mmio is u32 *, the step of mmio[0] -> mmio[1] will be 4, and i will be increased by 4 in each loop.
I guess the correct one would be mmio[i/4] = xxxxx; would that looks better? if yes, I will do that in the next version.

>> +static int handle_mmio(struct intel_gvt_mmio_table_iter *iter,
>> +		       u32 offset, u32 device, u32 size)
>> +{
>> +	if (WARN_ON(!IS_ALIGNED(offset, 4)))
>> +		return -EINVAL;
> 
> Shouldn't this be in the caller of the method?
> 
>> +	save_mmio(iter, offset, size);
>> +	return 0;
> 
Yes. You are right. It's because I get rid of the mmio_block in intel_gvt.c

> Now that the block callback is gone save_mmio and handle_mmio
> can be merged.
> 
>> +	mem = vzalloc(2 * SZ_1M);
> 
> Don't we want a driver-wide constant for this instead of a magic number?
> 

We actually have one in i915, but it's not exported. Should we export that one?

Thanks,
Zhi.