RE: [PATCH] drm/i915/gvt: Cast u64 array to u32 array

[Zhenyu Wang <zhenyuw.linux@gmail.com>]

"Cavitt, Jonathan" <jonathan.cavitt@intel.com> writes:

> -----Original Message-----
> From: Zhenyu Wang <zhenyuw.linux@gmail.com> 
> Sent: Friday, February 20, 2026 6:41 AM
> To: Cavitt, Jonathan <jonathan.cavitt@intel.com>; intel-gfx@lists.freedesktop.org
> Cc: Gupta, Saurabhg <saurabhg.gupta@intel.com>; Zuo, Alex <alex.zuo@intel.com>; Cavitt, Jonathan <jonathan.cavitt@intel.com>
> Subject: RE: [PATCH] drm/i915/gvt: Cast u64 array to u32 array
>> 
>> "Cavitt, Jonathan" <jonathan.cavitt@intel.com> writes:
>> 
>> > -----Original Message-----
>> > From: Zhenyu Wang <zhenyuw.linux@gmail.com> 
>> > Sent: Friday, February 13, 2026 2:42 AM
>> > To: Cavitt, Jonathan <jonathan.cavitt@intel.com>; intel-gfx@lists.freedesktop.org
>> > Cc: Gupta, Saurabhg <saurabhg.gupta@intel.com>; Zuo, Alex <alex.zuo@intel.com>; Cavitt, Jonathan <jonathan.cavitt@intel.com>
>> > Subject: Re: [PATCH] drm/i915/gvt: Cast u64 array to u32 array
>> >> 
>> >> Jonathan Cavitt <jonathan.cavitt@intel.com> writes:
>> >> 
>> >> > Static analysis issue:
>> >> >
>> >> > The u64 array workload->shadow_mm->ppgtt_mm.shadow_pdps is cast to a
>> >> > void pointer and passed as a u32 array to set_context_pdp_root_pointer
>> >> > as a part of update_shadow_pdps.  This isn't wrong, per se, but we
>> >> > should properly cast it to an appropriately-sized u32 array before
>> >> > submission.
>> >> >
>> >> > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
>> >> > ---
>> >> >  drivers/gpu/drm/i915/gvt/scheduler.c | 6 ++++--
>> >> >  1 file changed, 4 insertions(+), 2 deletions(-)
>> >> >
>> >> > diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c
>> >> > index 15fdd514ca83..1a95c9f76faa 100644
>> >> > --- a/drivers/gpu/drm/i915/gvt/scheduler.c
>> >> > +++ b/drivers/gpu/drm/i915/gvt/scheduler.c
>> >> > @@ -72,6 +72,7 @@ static void update_shadow_pdps(struct intel_vgpu_workload *workload)
>> >> >  {
>> >> >  	struct execlist_ring_context *shadow_ring_context;
>> >> >  	struct intel_context *ctx = workload->req->context;
>> >> > +	u32 pdp[8];
>> >> >  
>> >> >  	if (WARN_ON(!workload->shadow_mm))
>> >> >  		return;
>> >> > @@ -79,9 +80,10 @@ static void update_shadow_pdps(struct intel_vgpu_workload *workload)
>> >> >  	if (WARN_ON(!atomic_read(&workload->shadow_mm->pincount)))
>> >> >  		return;
>> >> >  
>> >> > +	memcpy(pdp, workload->shadow_mm->ppgtt_mm.shadow_pdps,
>> >> > +	       sizeof(u64) * ARRAY_SIZE(workload->shadow_mm->ppgtt_mm.shadow_pdps));
>> >> >  	shadow_ring_context = (struct execlist_ring_context *)ctx->lrc_reg_state;
>> >> > -	set_context_pdp_root_pointer(shadow_ring_context,
>> >> > -			(void *)workload->shadow_mm->ppgtt_mm.shadow_pdps);
>> >> > +	set_context_pdp_root_pointer(shadow_ring_context, pdp);
>> >> >  }
>> >> >  
>> >> 
>> >> I think we'd better just cast the type instead of extra copy.
>> >
>> > I'm not certain that would resolve the static analysis issue.
>> >
>> > To specify, the static analyzer is complaining that we're taking a pointer to an object
>> > of type 'unsigned long long' and dereferencing it as an object of type 'unsigned int'.
>> > The analyzer is getting uppity about this causing unexpected results depending on
>> > machine endianness (which... it won't, but the static analyzer doesn't know that),
>> > so I suspect the only way to get it to calm down is to do a direct memory copy, as
>> > seen here.  Casting the type would just result in the same static analysis issue.
>> >
>> > This is the part of the email that I'd throw around terms like "strict aliasing" and
>> > "type punning" if I thought they were relevant.  They probably aren't, though.
>> >
>> 
>> I really don't want to do extra copy as this is hot path for every exec submission.
>> How about below change?
>> Btw, which static analysis you're using? Looks I don't get such warning
>> with either sparse or smatch...
>
> I'm not allowed to go into detail about that.  Sorry.
>
>> 
>> diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c
>> index 63ad1fed525a..3f09d6440827 100644
>> --- a/drivers/gpu/drm/i915/gvt/scheduler.c
>> +++ b/drivers/gpu/drm/i915/gvt/scheduler.c
>> @@ -1,3 +1,4 @@
>> +
>>  /*
>>   * Copyright(c) 2011-2016 Intel Corporation. All rights reserved.
>>   *
>> @@ -54,7 +55,7 @@
>>  
>>  static void set_context_pdp_root_pointer(
>>  		struct execlist_ring_context *ring_context,
>> -		u32 pdp[8])
>> +		u32 *pdp)
>>  {
>>  	int i;
>>  
>> @@ -75,7 +76,7 @@ static void update_shadow_pdps(struct intel_vgpu_workload *workload)
>>  
>>  	shadow_ring_context = (struct execlist_ring_context *)ctx->lrc_reg_state;
>>  	set_context_pdp_root_pointer(shadow_ring_context,
>> -			(void *)workload->shadow_mm->ppgtt_mm.shadow_pdps);
>> +				     (u32 *)workload->shadow_mm->ppgtt_mm.shadow_pdps);
>>  }
>
> This still results in us casting a u64 pointer to a u32 pointer.
>

It's normal in some driver cases we really need to access 32b fields indeed..

> If this change is undesirable, I can mark it as a false positive on my end.

Above version should be better but original one doesn't hurt... so in
case of private tool, please mark it so. Thanks.