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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.04.2026 09:13, Mykola Kvach wrote:
> 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.

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