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

[Julien Grall <julien@xxxxxxx>]

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