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

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

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)

As far a I can tell nothing mandatory is introduced with version 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

I'll update the commit message.

Thanks,
Jens