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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

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.

On that basis, I do not see a behavioral change for the FN64
SYSTEM_SUSPEND case in v3.


Best regards,
Mykola

>
> Personally I'm taking what's written in a pretty strict sense: If in
> doubt, drop tags which may no longer cover all changes they would
> apply to. (This is, in my interpretation, generally less of a problem
> for A-b, as that only conveys "this kind of change is okay to make",
> without covering much of the details. In the case here retaining A-b
> would probably have been acceptable, albeit there's still room for
> interpretation. For example, if an A-b was offered based on somebody
> else's R-b, then likely the A-b would need dropping if the R-b is
> dropped.)
>
> Jan