Re: [Xen-devel] [PATCH v1 1/6] arm: add SMC wrapper that is compatible with SMCCC

[Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>]

Hi,

On 23.08.18 17:45, Julien Grall wrote:
> On 08/23/2018 03:35 PM, Volodymyr Babchuk wrote:
> > Hi,
> Hi Volodymyr,
>
>
> > On 22.08.18 19:46, Julien Grall wrote:
> > > Hi Volodymyr,
> > >
> > > On 22/08/18 15:11, Volodymyr Babchuk wrote:
> > > > 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.
> > > > This patch adds this call for both arm32 and arm64.
> > > >
> > > > There was similar patch by Edgar E. Iglesias ([1]), but looks
> > > > like it is abandoned.
> > > >
> > > > [1] 
> > > > https://lists.xenproject.org/archives/html/xen-devel/2017-02/msg00636.html 
> > > >
> > > > CC: "Edgar E. Iglesias" <edgar.iglesias@xxxxxxxxxx>
> > > >
> > > > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
> > > > ---
> > > >
> > > > changes from "RFC":
> > > >    - response now stored in structure instead of array
> > > >    - added comments for arm32 assembly code
> > > >    - added offset (instead of magic values) for arm32 asm code
> > > Did you mean arm64?
> > Yes, you are right.
> >
> > > >   xen/arch/arm/arm32/Makefile      |  1 +
> > > >   xen/arch/arm/arm32/smc.S         | 39 
> > > > +++++++++++++++++++++++++++++++++++++++
> > > >   xen/arch/arm/arm64/Makefile      |  1 +
> > > >   xen/arch/arm/arm64/asm-offsets.c |  4 ++++
> > > >   xen/arch/arm/arm64/smc.S         | 30 ++++++++++++++++++++++++++++++
> > > >   xen/include/asm-arm/processor.h  | 11 +++++++++++
> > > >   6 files changed, 86 insertions(+)
> > > >   create mode 100644 xen/arch/arm/arm32/smc.S
> > > >   create mode 100644 xen/arch/arm/arm64/smc.S
> > > >
> > > > diff --git a/xen/arch/arm/arm32/Makefile b/xen/arch/arm/arm32/Makefile
> > > > index 0ac254f..c69f35e 100644
> > > > --- a/xen/arch/arm/arm32/Makefile
> > > > +++ b/xen/arch/arm/arm32/Makefile
> > > > @@ -10,4 +10,5 @@ obj-y += proc-v7.o proc-caxx.o
> > > >   obj-y += smpboot.o
> > > >   obj-y += traps.o
> > > >   obj-y += vfp.o
> > > > +obj-y += smc.o
> > > > diff --git a/xen/arch/arm/arm32/smc.S b/xen/arch/arm/arm32/smc.S
> > > > new file mode 100644
> > > > index 0000000..56f7982
> > > > --- /dev/null
> > > > +++ b/xen/arch/arm/arm32/smc.S
> > > > @@ -0,0 +1,39 @@
> > > > +/*
> > > > + * xen/arch/arm/arm32/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, or
> > > > + * (at your option) any later version.
> > > Xen is GPLv2 only. Can you please update the copyright accordingly?
> > >
> > > Also, the code is pretty much a copy of the Linux part. So it is 
> > > probably worth to keep the copy-right around.
> > > > + *
> > > > + * 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/macros.h>
> > > > +
> > > > +/*
> > > > + * void call_smccc_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 
> > > > smccc_res *res)
> > > > + */
> > > > +ENTRY(call_smccc_smc)
> > > I would rather avoid to have 2 function to handle the same 
> > > convention. Can you please either drop call_smc or rework it to 
> > > handle more parameters?
> > > But it looks like you could re-use arm_smccc_1_1 as it follows the 
> > > 1.0 convention for 32-bits.
> > Oops, I missed introduction of arm_smccc_1_1. I think, it will be 
> > sufficient for my tasks. Looks like, I'll drop this patch at all.
> Are you suggesting that OP-TEE will always use SMCCC 1.1 calling 
> convention?
It depends on ARM-TF, I think. At least, on ARMv8 platforms. But you are 
right. I can't be sure that SMCCC 1.1 is supported.
Looks like XEN should determine at boot time which calling convention is 
supported and then use appropriate function in all places where SMCCC 
calls are made. What do you think?
> I would be quite surprised given that you seem to have platform with 
> ancient firmware.
It is not so ancient :) At least it have ARM-TF.
Also, I have found that qemu-armv8 speeds up debugging, as I don't need 
to tinker with real HW most of the time.
--
Volodymyr Babchuk

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