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

[Julien Grall <julien.grall@arm.com>]

Hi,

On 08/23/2018 04:16 PM, Volodymyr Babchuk wrote:
> 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@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.
>>
>> 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?

This may not be entirely trivial to do it as arm_smccc_1_1 is based on 
macros to limit the number of parameters saved. But I would welcome such 
patch.

> 
>> 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.

Well, SMCCC 1.1 has been introduced last January for Meltdown/Spectre 
mitigations. I consider anything before that ancient :).

Cheers,

-- 
Julien Grall

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