Re: [Xen-devel] [PATCH v2 3/6] xen/arm: add SMC wrapper that is compatible with SMCCC v1.0

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 26 Sep 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 09/26/2018 12:50 AM, Stefano Stabellini wrote:
> > On Tue, 25 Sep 2018, Julien Grall wrote:
> > > From: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
> > > 
> > > Existing SMC wrapper call_smc() allows only 4 parameters and
> > > returns only one value. This is enough for existing
> > > use in PSCI code, but TEE mediator will need a call that is
> > > fully compatible with ARM SMCCC v1.0.
> > > 
> > > This patch adds a wrapper for both arm32 and arm64. In the case of
> > > arm32, the wrapper is just an alias to the ARM SMCCC v1.1 as the
> > > convention is the same.
> > > 
> > > CC: "Edgar E. Iglesias" <edgar.iglesias@xxxxxxxxxx>
> > > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
> > > [julien: Rework the wrapper to make it closer to SMCC 1.1 wrapper]
> > > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> > 
> > I have been struggling to find the old doc for SMCCC v1.0, all the
> > references have been updated to v1.1 online now. Do you have a link?
> 
> Are you sure? All the references are still to v1.0 (DEN 0028B). See [1].

The lack of version in the doc confused me.


> > > diff --git a/xen/arch/arm/arm64/smc.S b/xen/arch/arm/arm64/smc.S
> > > new file mode 100644
> > > index 0000000000..b0752be57e
> > > --- /dev/null
> > > +++ b/xen/arch/arm/arm64/smc.S
> > > @@ -0,0 +1,32 @@
> > > +/*
> > > + * xen/arch/arm/arm64/smc.S
> > > + *
> > > + * Wrapper for Secure Monitors Calls
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License as published by
> > > + * the Free Software Foundation; either version 2 of the License.
> > > + *
> > > + * This program is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > + * GNU General Public License for more details.
> > > + */
> > > +
> > > +#include <asm/asm_defns.h>
> > > +#include <asm/macros.h>
> > > +
> > > +/*
> > > + * void __arm_smccc_1_0_smc(register_t a0, register_t a1, register_t a2,
> > > + *                          register_t a3, register_t a4, register_t a5,
> > > + *                          register_t a6, register_t a7,
> > > + *                          struct arm_smccc_res *res)
> > > + */
> > > +ENTRY(__arm_smccc_1_0_smc)
> > > +        smc     #0
> > > +        ldr     x4, [sp]
> > > +        cbz     x4, 1f          /* No need to store the result */
> > > +        stp     x0, x1, [x4, #SMCCC_RES_a0]
> > > +        stp     x2, x3, [x4, #SMCCC_RES_a2]
> > > +1:
> > > +        ret
> > 
> > As I mentioned, I couldn't find the doc, but it looks like the Linux
> > implementation always copies back the results
> > (arch/arm64/kernel/smccc-call.S)? Shouldn't we zero x0-x3 at least?
> Could you provide more details on what looks wrong?
> 
> The results are copied in the array res using stp instructions. The only
> difference with Linux implementation is we don't handle quirk.

The only difference is that in this implementation we handle `res' being
NULL, which I think is a good idea:

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel