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

[Volodymyr Babchuk <volodymyr_babchuk@epam.com>]

Hi,

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@xilinx.com>
>>
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>> ---
>>
>> 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.

I'm sorry for bothering you.

>> +        mov     r12, sp
>> +        push    {r4-r7}
>> +    /*
>> +     * According to ARMv7 (or aarch32) ABI, first 4 parameters of
>> +     * call_smccc_smc are passed in r0-r3 and other 4 are on stack.
>> +     * Load a4-a7 from stack to r4-r7.
>> +     */
>> +        ldm     r12, {r4-r7}
>> +        smc     #0
>> +        pop     {r4-r7}
>> +    /* Load pointer to res (4th parameter on stack) to r12 */
>> +        ldr     r12, [sp, #(4 * 4)]
>> +    /* Store returned results from r0-r3 to res */
>> +        stm     r12, {r0-r3}
>> +        bx      lr
> 
> Something looks wrong with the indentation. There seem to be a mix of 
> hard tab and soft tab.
> 
>> diff --git a/xen/arch/arm/arm64/Makefile b/xen/arch/arm/arm64/Makefile
>> index bb5c610..7ecdad7 100644
>> --- a/xen/arch/arm/arm64/Makefile
>> +++ b/xen/arch/arm/arm64/Makefile
>> @@ -9,6 +9,7 @@ obj-y += entry.o
>>   obj-y += insn.o
>>   obj-$(CONFIG_LIVEPATCH) += livepatch.o
>>   obj-y += smpboot.o
>> +obj-y += smc.o
>>   obj-y += traps.o
>>   obj-y += vfp.o
>>   obj-y += vsysreg.o
>> diff --git a/xen/arch/arm/arm64/asm-offsets.c 
>> b/xen/arch/arm/arm64/asm-offsets.c
>> index ce24e44..5353fe8 100644
>> --- a/xen/arch/arm/arm64/asm-offsets.c
>> +++ b/xen/arch/arm/arm64/asm-offsets.c
>> @@ -50,6 +50,10 @@ void __dummy__(void)
>>      BLANK();
>>      OFFSET(INITINFO_stack, struct init_info, stack);
>> +
>> +   BLANK();
>> +   OFFSET(SMCCC_RES_a0, struct smccc_res, a0);
>> +   OFFSET(SMCCC_RES_a2, struct smccc_res, a2);
>>   }
>>   /*
>> diff --git a/xen/arch/arm/arm64/smc.S b/xen/arch/arm/arm64/smc.S
>> new file mode 100644
>> index 0000000..40843c0
>> --- /dev/null
>> +++ b/xen/arch/arm/arm64/smc.S
>> @@ -0,0 +1,30 @@
>> +/*
>> + * 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, 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/asm_defns.h>
>> +#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)
>> +        smc     #0
>> +        ldr     x4, [sp]
>> +        stp     x0, x1, [x4, #SMCCC_RES_a0]
>> +        stp     x2, x3, [x4, #SMCCC_RES_a2]
>> +        ret
>> diff --git a/xen/include/asm-arm/processor.h 
>> b/xen/include/asm-arm/processor.h
>> index 222a02d..946a2db 100644
>> --- a/xen/include/asm-arm/processor.h
>> +++ b/xen/include/asm-arm/processor.h
>> @@ -650,6 +650,13 @@ union hsr {
>>   };
>> +
>> +struct smccc_res {
>> +    register_t a0;
>> +    register_t a1;
>> +    register_t a2;
>> +    register_t a3;
>> +};
> 
> Any reason to not use arm_smccc_res?
> 
>>   #endif
>>   /* HSR.EC == HSR_CP{15,14,10}_32 */
>> @@ -815,6 +822,10 @@ void vcpu_regs_user_to_hyp(struct vcpu *vcpu,
>>   int call_smc(register_t function_id, register_t arg0, register_t arg1,
>>                register_t arg2);
>> +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);
>> +
>>   void do_trap_hyp_serror(struct cpu_user_regs *regs);
>>   void do_trap_guest_serror(struct cpu_user_regs *regs);
>>
> 
> Cheers,
> 

-- 
Volodymyr Babchuk

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