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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Jan,

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.

In any case, if you do not think the tag should be kept, I am fine with
waiting for Bertrand to re-confirm it.


Best regards,
Mykola

[1] https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches

>
> Jan