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

[Julien Grall <julien@xen.org>]

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?

Furthermore, your commit message explain why arm_smccc_1_2_smc() was 
introduced. But it seems to miss some words about exposing SMCCC v2.1 to 
the VM.

In general, I think it is better to split the host support from the VM 
support. The two are technically not independent (caller vs 
implementation) and there are different problematics for each (see above 
for an example). I don't think there are a lot to add here, so I would 
be OK to keep it in the same patch with a few words.

Lastly, can you provide a link to the spec in the commit message? This 
would help to find the pdf easily. I think in this case, you are using
ARM DEN 0028C (this is not the latest but describe 1.2):

https://developer.arm.com/documentation/den0028/c/?lang=en

Cheers,

-- 
Julien Grall