Ping: Re: [PATCH v3] xen/arm: vpsci: ignore upper 32 bits for SMC32 PSCI arguments

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi all,

Gentle ping on this patch.

Bertrand has re-confirmed his Reviewed-by, and the previous discussion
looks resolved from my side.

Could this be picked up if there are no remaining concerns?

Best regards,
Mykola

On Thu, Apr 23, 2026 at 8:13 AM Mykola Kvach <xakep.amatop@xxxxxxxxx> wrote:
>
> Hi Bertrand,
>
> On Wed, Apr 15, 2026 at 3:31 PM Bertrand Marquis
> <Bertrand.Marquis@xxxxxxx> wrote:
> >
> > Hi Jan and Mykola,
> >
> > > On 1 Apr 2026, at 14:24, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> > >
> > > On 01.04.2026 11:51, Mykola Kvach wrote:
> > >> On Wed, Apr 1, 2026 at 12:22 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> > >>> On 01.04.2026 10:49, Mykola Kvach wrote:
> > >>>> On Wed, Apr 1, 2026 at 11:14 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> > >>>>> On 01.04.2026 09:13, Mykola Kvach wrote:
> > >>>>>> On Wed, Apr 1, 2026 at 9:29 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> > >>>>>>> On 31.03.2026 20:31, Mykola Kvach wrote:
> > >>>>>>>> From: Mykola Kvach <mykola_kvach@xxxxxxxx>
> > >>>>>>>>
> > >>>>>>>> SMCCC DEN0028G, section 3.1, states that for AArch64 SMC/HVC calls
> > >>>>>>>> using Wn, only the least significant 32 bits are significant and 
> > >>>>>>>> the
> > >>>>>>>> upper 32 bits must be ignored by the implementation.
> > >>>>>>>>
> > >>>>>>>> So for SMC32 PSCI calls, Xen must not treat non-zero upper bits in 
> > >>>>>>>> the
> > >>>>>>>> argument registers as an error. Instead, they should be discarded 
> > >>>>>>>> when
> > >>>>>>>> decoding the arguments.
> > >>>>>>>>
> > >>>>>>>> Arm ARM DDI 0487J.a (D1-5406) also notes that the upper 32 bits 
> > >>>>>>>> may be
> > >>>>>>>> implementation defined when entering from AArch32. Xen zeros them 
> > >>>>>>>> on
> > >>>>>>>> entry, but that guarantee is only relevant for 32-bit domains.
> > >>>>>>>>
> > >>>>>>>> Update PSCI v0.2+ CPU_ON, CPU_SUSPEND, AFFINITY_INFO and 
> > >>>>>>>> SYSTEM_SUSPEND
> > >>>>>>>> to read SMC32 arguments via PSCI_ARG32(), while keeping the SMC64
> > >>>>>>>> handling unchanged.
> > >>>>>>>>
> > >>>>>>>> No functional change is intended for PSCI 0.1.
> > >>>>>>>>
> > >>>>>>>> Suggested-by: Julien Grall <julien@xxxxxxx>
> > >>>>>>>> Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> > >>>>>>>> Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> > >>>>>>>
> > >>>>>>> I thought I might as well include this in my next commit sweep, but 
> > >>>>>>> isn't
> > >>>>>>> this R-b being invalidated by ...
> > >>>>>>>
> > >>>>>>>> ---
> > >>>>>>>> v3:
> > >>>>>>>> - use PSCI_ARG_CONV for SYSTEM_SUSPEND
> > >>>>>>>
> > >>>>>>> ... this change. That's ...
> > >>>>>>>
> > >>>>>>>> @@ -422,14 +427,8 @@ bool do_vpsci_0_2_call(struct cpu_user_regs 
> > >>>>>>>> *regs, uint32_t fid)
> > >>>>>>>>     case PSCI_1_0_FN32_SYSTEM_SUSPEND:
> > >>>>>>>>     case PSCI_1_0_FN64_SYSTEM_SUSPEND:
> > >>>>>>>>     {
> > >>>>>>>> -        register_t epoint = PSCI_ARG(regs, 1);
> > >>>>>>>> -        register_t cid = PSCI_ARG(regs, 2);
> > >>>>>>>> -
> > >>>>>>>> -        if ( fid == PSCI_1_0_FN32_SYSTEM_SUSPEND )
> > >>>>>>>> -        {
> > >>>>>>>> -            epoint &= GENMASK(31, 0);
> > >>>>>>>> -            cid &= GENMASK(31, 0);
> > >>>>>>>> -        }
> > >>>>>>>> +        register_t epoint = PSCI_ARG_CONV(regs, 1, is_conv_64);
> > >>>>>>>> +        register_t cid = PSCI_ARG_CONV(regs, 2, is_conv_64);
> > >>>>>>>>
> > >>>>>>>>         perfc_incr(vpsci_system_suspend);
> > >>>>>>>>         PSCI_SET_RESULT(regs, do_psci_1_0_system_suspend(epoint, 
> > >>>>>>>> cid));
> > >>>>>>>
> > >>>>>>> ... this hunk aiui, which is far from merely cosmetic imo. While
> > >>>>>>
> > >>>>>> Nobody said that the change had to be purely cosmetic in order to 
> > >>>>>> keep
> > >>>>>> the tag. I understood it differently from the official Xen
> > >>>>>> documentation pages.
> > >>>>>>
> > >>>>>>> behavior looks to remain the same for PSCI_1_0_FN32_SYSTEM_SUSPEND, 
> > >>>>>>> it
> > >>>>>>
> > >>>>>> Exactly. If the changes are not substantial, I do not see a reason to
> > >>>>>> drop the tag ...
> > >>>>>>
> > >>>>>>> clearly changes for PSCI_1_0_FN64_SYSTEM_SUSPEND. That may be 
> > >>>>>>> intended
> > >>>>>>> and for the better, but the change clearly wasn't reviewed by 
> > >>>>>>> Bertrand,
> > >>>>>>> nor - when offering the R-b - did he ask for this extra change.
> > >>>>>>
> > >>>>>> ... and this is also how I understood the Xen patch submission
> > >>>>>> guidelines [1], which say:
> > >>>>>>
> > >>>>>> "Note that if there are several revisions of a patch, you ought to
> > >>>>>> copy tags that have accumulated during the review. For example, if
> > >>>>>> person A and person B added a Reviewed-by: tag to v1 of your patch,
> > >>>>>> include it into v2 of your patch. If you make substantial changes
> > >>>>>> after certain tags were already applied, you will want to consider
> > >>>>>> which ones are no longer applicable (and may require re-providing)."
> > >>>>>>
> > >>>>>> So my understanding was that tags should normally be kept across
> > >>>>>> revisions, unless the changes are substantial enough to make them no
> > >>>>>> longer applicable.
> > >>>>>
> > >>>>> Maybe our understanding of "substantial" differs. To me that's 
> > >>>>> anything
> > >>>>> changing functionality. Style adjustments, typo corrections, and alike
> > >>>>> generally aren't substantial (albeit even then there may be 
> > >>>>> exceptions).
> > >>>>
> > >>>> Thanks for clarifying what you consider substantial.
> > >>>>
> > >>>> Even under that interpretation, I do not see a functionality change
> > >>>> here. "Refactoring" seems like the more accurate term in this case:
> > >>>> the internal form changes, but the intended external behavior does
> > >>>> not.
> > >>>>
> > >>>> It may be that we are using "functional change" in slightly different
> > >>>> senses here.
> > >>>>
> > >>>> For v3, the switch to PSCI_ARG_CONV() in SYSTEM_SUSPEND was meant to
> > >>>> make this case consistent with the helper-based argument decoding used
> > >>>> elsewhere, not to change behavior.
> > >>>>
> > >>>> In particular, I do not see a functional change for
> > >>>> PSCI_1_0_FN64_SYSTEM_SUSPEND: v2 used PSCI_ARG(regs, 1/2), and in v3
> > >>>> PSCI_ARG_CONV(regs, 1/2, is_conv_64) should resolve to the same thing
> > >>>> when is_conv_64 is true.
> > >>>
> > >>> Isn't the whole point of the patch to alter behavior when is_conv_64 is
> > >>> false? For that case PSCI_1_0_FN64_SYSTEM_SUSPEND behavior looks to
> > >>> change in v3, when it didn't in v2. Whereas for
> > >>> PSCI_1_0_FN32_SYSTEM_SUSPEND the v3 change indeed only eliminates open-
> > >>> coding, which one may or may not regard as "substantial".
> > >>
> > >> I think the point I was trying to make is slightly narrower: in this
> > >> code path, is_conv_64 is derived directly from fid via
> > >> smccc_is_conv_64(fid) before the switch (fid).
> > >>
> > >> So for PSCI_1_0_FN64_SYSTEM_SUSPEND, I do not see how
> > >> is_conv_64 == false could arise here: if we are in the FN64 case,
> > >> the function ID already encodes the 64-bit convention.
> > >>
> > >> Conversely, if is_conv_64 is false here, then this cannot be the
> > >> FN64 case.
> > >
> > > Ah, I see. To figure that out, I would have had to do a proper review. I
> > > was after committing only, which ought to be an entirely mechanical step.
> > >
> > >> On that basis, I do not see a behavioral change for the FN64
> > >> SYSTEM_SUSPEND case in v3.
> > >
> > > I agree (now). I'm still not going to pick up that patch, but rather
> > > leave it to the Arm maintainers. While not as clear cut as it first
> > > seemed to me, I still consider it within the grey area.
> >
> > Sorry for the delay, this felt through in my filters as it was reviewed-by 
> > already.
> >
> > I am ok with the changes done which make sense (mask is now done
> > directly).
> >
> > Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
>
> Thank you for taking another look.
>
> With your Reviewed-by in place, and since I do not expect any further
> changes from my side, I believe this patch should now be ready for the
> Arm maintainers to pick up.
>
> Best regards,
> Mykola
>
> >
> > Cheers
> > Bertrand
> >
> > >
> > > Jan
> >
> >