Re: [PATCH v2 1/2] xen/arm: smccc: add support for SMCCCv1.2 extended input/output registers

[Julien Grall <julien@xen.org>]

Hi Jens,

On 15/06/2022 23:09, Jens Wiklander wrote:
> On Wed, Jun 15, 2022 at 08:01:28PM +0100, Julien Grall wrote:
>> Hi,
>>
>> On 15/06/2022 16:58, Jens Wiklander wrote:
>>> On Fri, Jun 10, 2022 at 05:41:33PM -0700, Stefano Stabellini wrote:
>>>>>    #endif /* __ASSEMBLY__ */
>>>>>    /*
>>>>> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
>>>>> index 676740ef1520..6f90c08a6304 100644
>>>>> --- a/xen/arch/arm/vsmc.c
>>>>> +++ b/xen/arch/arm/vsmc.c
>>>>> @@ -93,7 +93,7 @@ static bool handle_arch(struct cpu_user_regs *regs)
>>>>>        switch ( fid )
>>>>>        {
>>>>>        case ARM_SMCCC_VERSION_FID:
>>>>> -        set_user_reg(regs, 0, ARM_SMCCC_VERSION_1_1);
>>>>> +        set_user_reg(regs, 0, ARM_SMCCC_VERSION_1_2);
>>>>>            return true;
>>>> This is going to be a problem for ARM32 given that ARM_SMCCC_VERSION_1_2
>>>> is unimplemented on ARM32. If there is an ARM32 implementation in Linux
>>>> for ARM_SMCCC_VERSION_1_2 you might as well import it too.
>>>>
>>>> Otherwise we'll have to abstract it away, e.g.:
>>>>
>>>> #ifdef CONFIG_ARM_64
>>>> #define ARM_VSMCCC_VERSION ARM_SMCCC_VERSION_1_2
>>>> #else
>>>> #define ARM_VSMCCC_VERSION ARM_SMCCC_VERSION_1_1
>>>> #endif
>>>
>>> I couldn't find an ARM32 implementation for ARM_SMCCC_VERSION_1_2. But
>>> I'm not sure it's needed at this point. From what I've understood r4-17
>>> are either preserved or updated by the function ID in question. So
>>> claiming ARM_SMCCC_VERSION_1_2 shouldn't break anything.
>>
>> So in Xen, we always take a snapshot of the registers on entry to the
>> hypervisor and only touch it when necessary. Therefore, it doesn't matter
>> whether we claim to be complaient with 1.1 or 1.2 based on the argument
>> passing convention.
>>
>> However, the spec is not only about arguments. For instance, SMCCC v1.1 also
>> added some mandatory functions (e.g. detection the version). I haven't
>> looked closely on whether the SMCCC v1.2 introduced such thing. Can you
>> confirm what mandatory feature comes with 1.2?
> 
> There's a nice summary in a table at the end of the C version of DEN0028
> you linked below. For SMCCC v1.2:
> Argument/Result register set:
> Permits calls to use R4—R7 as return register (Section 4.1).
> Permits calls to use X4—X17 as return registers(Section3.1).
> Permits calls to use X8—X17 as argument registers (Section 3.1).
> Introduces:
> SMCCC_ARCH_SOC_ID (Section 7.4)
> Deprecates:
> UID, Revision Queries on Arm Architecture Service (Section 6.2)
> Count Query on all services (Section 6.2)

Thanks for posting here!

> 
> As far a I can tell nothing mandatory is introduced with version 1.2.

Agree. So it is safe to expose 1.2 unconditionally to the VMs.

Cheers,

-- 
Julien Grall