From: Mykola Kvach <mykola_kvach@epam.com>

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@xen.org>
Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
---
v3:
 - use PSCI_ARG_CONV for SYSTEM_SUSPEND

v2:
 - introduce PSCI_ARG_CONV() to centralize convention-dependent argument
   decoding for PSCI v0.2+ calls;
 - use smccc_is_conv_64(fid) instead of open-coding per-call SMC32 checks;
 - keep PSCI 0.1 handling unchanged, except switch on the already-decoded
   fid instead of re-reading x0/r0.

Link to discussion: https://patchew.org/Xen/cover.1751020456.git.mykola._5Fkvach@epam.com/072270e0940b6bcc2743d56a336363f4719ba60a.1751020456.git.mykola._5Fkvach@epam.com/#7070f416-119c-49f8-acd0-82c6e31f0fc6@xen.org
---
 xen/arch/arm/vpsci.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c
index bd87ec430d..ac6af6118f 100644
--- a/xen/arch/arm/vpsci.c
+++ b/xen/arch/arm/vpsci.c
@@ -305,13 +305,16 @@ static int32_t do_psci_1_0_features(uint32_t psci_func_id)
 #define PSCI_ARG32(reg, n) PSCI_ARG(reg, n)
 #endif
 
+#define PSCI_ARG_CONV(reg, n, conv_64) \
+    ((conv_64) ? PSCI_ARG(reg, n) : PSCI_ARG32(reg, n))
+
 /*
  * PSCI 0.1 calls. It will return false if the function ID is not
  * handled.
  */
 bool do_vpsci_0_1_call(struct cpu_user_regs *regs, uint32_t fid)
 {
-    switch ( (uint32_t)get_user_reg(regs, 0) )
+    switch ( fid )
     {
     case PSCI_cpu_off:
     {
@@ -346,6 +349,8 @@ bool do_vpsci_0_2_call(struct cpu_user_regs *regs, uint32_t fid)
      * adding/removing a function. SSSC_SMCCC_*_REVISION should be
      * updated once per release.
      */
+    bool is_conv_64 = smccc_is_conv_64(fid);
+
     switch ( fid )
     {
     case PSCI_0_2_FN32_PSCI_VERSION:
@@ -378,9 +383,9 @@ bool do_vpsci_0_2_call(struct cpu_user_regs *regs, uint32_t fid)
     case PSCI_0_2_FN32_CPU_ON:
     case PSCI_0_2_FN64_CPU_ON:
     {
-        register_t vcpuid = PSCI_ARG(regs, 1);
-        register_t epoint = PSCI_ARG(regs, 2);
-        register_t cid = PSCI_ARG(regs, 3);
+        register_t vcpuid = PSCI_ARG_CONV(regs, 1, is_conv_64);
+        register_t epoint = PSCI_ARG_CONV(regs, 2, is_conv_64);
+        register_t cid = PSCI_ARG_CONV(regs, 3, is_conv_64);
 
         perfc_incr(vpsci_cpu_on);
         PSCI_SET_RESULT(regs, do_psci_0_2_cpu_on(vcpuid, epoint, cid));
@@ -391,8 +396,8 @@ bool do_vpsci_0_2_call(struct cpu_user_regs *regs, uint32_t fid)
     case PSCI_0_2_FN64_CPU_SUSPEND:
     {
         uint32_t pstate = PSCI_ARG32(regs, 1);
-        register_t epoint = PSCI_ARG(regs, 2);
-        register_t cid = PSCI_ARG(regs, 3);
+        register_t epoint = PSCI_ARG_CONV(regs, 2, is_conv_64);
+        register_t cid = PSCI_ARG_CONV(regs, 3, is_conv_64);
 
         perfc_incr(vpsci_cpu_suspend);
         PSCI_SET_RESULT(regs, do_psci_0_2_cpu_suspend(pstate, epoint, cid));
@@ -402,7 +407,7 @@ bool do_vpsci_0_2_call(struct cpu_user_regs *regs, uint32_t fid)
     case PSCI_0_2_FN32_AFFINITY_INFO:
     case PSCI_0_2_FN64_AFFINITY_INFO:
     {
-        register_t taff = PSCI_ARG(regs, 1);
+        register_t taff = PSCI_ARG_CONV(regs, 1, is_conv_64);
         uint32_t laff = PSCI_ARG32(regs, 2);
 
         perfc_incr(vpsci_cpu_affinity_info);
@@ -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));
-- 
2.43.0

On 31.03.2026 20:31, Mykola Kvach wrote:
> From: Mykola Kvach <mykola_kvach@epam.com>
> 
> 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@xen.org>
> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

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
behavior looks to remain the same for PSCI_1_0_FN32_SYSTEM_SUSPEND, it
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.

Jan

Hi Jan,

On Wed, Apr 1, 2026 at 9:29 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 31.03.2026 20:31, Mykola Kvach wrote:
> > From: Mykola Kvach <mykola_kvach@epam.com>
> >
> > 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@xen.org>
> > Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> > Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>
> 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

On 01.04.2026 09:13, Mykola Kvach wrote:
> Hi Jan,
> 
> On Wed, Apr 1, 2026 at 9:29 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 31.03.2026 20:31, Mykola Kvach wrote:
>>> From: Mykola Kvach <mykola_kvach@epam.com>
>>>
>>> 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@xen.org>
>>> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
>>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>>
>> 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).

Jan

On Wed, Apr 1, 2026 at 11:14 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 01.04.2026 09:13, Mykola Kvach wrote:
> > Hi Jan,
> >
> > On Wed, Apr 1, 2026 at 9:29 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 31.03.2026 20:31, Mykola Kvach wrote:
> >>> From: Mykola Kvach <mykola_kvach@epam.com>
> >>>
> >>> 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@xen.org>
> >>> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> >>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
> >>
> >> 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.

If I am missing a behavioral difference in the FN64 case, please point
out which one.


Best regards,
Mykola

>
> Jan

On 01.04.2026 10:49, Mykola Kvach wrote:
> On Wed, Apr 1, 2026 at 11:14 AM Jan Beulich <jbeulich@suse.com> wrote:
>> On 01.04.2026 09:13, Mykola Kvach wrote:
>>> On Wed, Apr 1, 2026 at 9:29 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 31.03.2026 20:31, Mykola Kvach wrote:
>>>>> From: Mykola Kvach <mykola_kvach@epam.com>
>>>>>
>>>>> 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@xen.org>
>>>>> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
>>>>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>>>>
>>>> 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".

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

On Wed, Apr 1, 2026 at 12:22 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 01.04.2026 10:49, Mykola Kvach wrote:
> > On Wed, Apr 1, 2026 at 11:14 AM Jan Beulich <jbeulich@suse.com> wrote:
> >> On 01.04.2026 09:13, Mykola Kvach wrote:
> >>> On Wed, Apr 1, 2026 at 9:29 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>> On 31.03.2026 20:31, Mykola Kvach wrote:
> >>>>> From: Mykola Kvach <mykola_kvach@epam.com>
> >>>>>
> >>>>> 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@xen.org>
> >>>>> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> >>>>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
> >>>>
> >>>> 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

On 01.04.2026 11:51, Mykola Kvach wrote:
> On Wed, Apr 1, 2026 at 12:22 PM Jan Beulich <jbeulich@suse.com> wrote:
>> On 01.04.2026 10:49, Mykola Kvach wrote:
>>> On Wed, Apr 1, 2026 at 11:14 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 01.04.2026 09:13, Mykola Kvach wrote:
>>>>> On Wed, Apr 1, 2026 at 9:29 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>> On 31.03.2026 20:31, Mykola Kvach wrote:
>>>>>>> From: Mykola Kvach <mykola_kvach@epam.com>
>>>>>>>
>>>>>>> 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@xen.org>
>>>>>>> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
>>>>>>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>>>>>>
>>>>>> 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.

Jan