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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

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>

Cheers
Bertrand

> 
> Jan