"Cavitt, Jonathan" <[email protected]> writes: > -----Original Message----- > From: Zhenyu Wang <[email protected]> > Sent: Friday, February 20, 2026 6:41 AM > To: Cavitt, Jonathan <[email protected]>; > [email protected] > Cc: Gupta, Saurabhg <[email protected]>; Zuo, Alex > <[email protected]>; Cavitt, Jonathan <[email protected]> > Subject: RE: [PATCH] drm/i915/gvt: Cast u64 array to u32 array >> >> "Cavitt, Jonathan" <[email protected]> writes: >> >> > -----Original Message----- >> > From: Zhenyu Wang <[email protected]> >> > Sent: Friday, February 13, 2026 2:42 AM >> > To: Cavitt, Jonathan <[email protected]>; >> > [email protected] >> > Cc: Gupta, Saurabhg <[email protected]>; Zuo, Alex >> > <[email protected]>; Cavitt, Jonathan <[email protected]> >> > Subject: Re: [PATCH] drm/i915/gvt: Cast u64 array to u32 array >> >> >> >> Jonathan Cavitt <[email protected]> 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 <[email protected]> >> >> > --- >> >> > 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.
