Re: [XEN PATCH v10 04/24] xen/arm: tee: add a primitive FF-A mediator

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 21 Jul 2023, at 00:29, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Bertrand,
> 
> On 20/07/2023 11:20, Bertrand Marquis wrote:
>> Hi Jens,
>>> On 17 Jul 2023, at 09:20, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
>>> 
>>> Adds a FF-A version 1.1 [1] mediator to communicate with a Secure
>>> Partition in secure world.
>>> 
>>> This commit brings in only the parts needed to negotiate FF-A version
>>> number with guest and SPMC.
>>> 
>>> [1] https://developer.arm.com/documentation/den0077/e
>>> Signed-off-by: Jens Wiklander <jens.wiklander@xxxxxxxxxx>
>>> ---
>>> xen/arch/arm/include/asm/psci.h    |   4 +
>>> xen/arch/arm/include/asm/tee/ffa.h |  35 +++++
>>> xen/arch/arm/tee/Kconfig           |  11 ++
>>> xen/arch/arm/tee/Makefile          |   1 +
>>> xen/arch/arm/tee/ffa.c             | 225 +++++++++++++++++++++++++++++
>>> xen/arch/arm/vsmc.c                |  17 ++-
>>> xen/include/public/arch-arm.h      |   1 +
>>> 7 files changed, 291 insertions(+), 3 deletions(-)
>>> create mode 100644 xen/arch/arm/include/asm/tee/ffa.h
>>> create mode 100644 xen/arch/arm/tee/ffa.c
>>> 
>>> diff --git a/xen/arch/arm/include/asm/psci.h 
>>> b/xen/arch/arm/include/asm/psci.h
>>> index 832f77afff3a..4780972621bb 100644
>>> --- a/xen/arch/arm/include/asm/psci.h
>>> +++ b/xen/arch/arm/include/asm/psci.h
>>> @@ -24,6 +24,10 @@ void call_psci_cpu_off(void);
>>> void call_psci_system_off(void);
>>> void call_psci_system_reset(void);
>>> 
>>> +/* Range of allocated PSCI function numbers */
>>> +#define PSCI_FNUM_MIN_VALUE                 _AC(0,U)
>>> +#define PSCI_FNUM_MAX_VALUE                 _AC(0x1f,U)
>>> +
>>> /* PSCI v0.2 interface */
>>> #define PSCI_0_2_FN32(nr) ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,           
>>>   \
>>>                                              ARM_SMCCC_CONV_32,             
>>>   \
>>> diff --git a/xen/arch/arm/include/asm/tee/ffa.h 
>>> b/xen/arch/arm/include/asm/tee/ffa.h
>>> new file mode 100644
>>> index 000000000000..44361a4e78e4
>>> --- /dev/null
>>> +++ b/xen/arch/arm/include/asm/tee/ffa.h
>>> @@ -0,0 +1,35 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * xen/arch/arm/include/asm/tee/ffa.h
>>> + *
>>> + * Arm Firmware Framework for ARMv8-A(FFA) mediator
>>> + *
>>> + * Copyright (C) 2023  Linaro Limited
>>> + */
>>> +
>>> +#ifndef __ASM_ARM_TEE_FFA_H__
>>> +#define __ASM_ARM_TEE_FFA_H__
>>> +
>>> +#include <xen/const.h>
>>> +#include <xen/kconfig.h>
> 
> None of the headers aside xen/config.h will include xen/kconfig.h. The former 
> is included everything from the compiler command line. So I have removed it.
> 
>>> +
>>> +#include <asm/smccc.h>
>>> +#include <asm/types.h>
>>> +
>>> +#define FFA_FNUM_MIN_VALUE              _AC(0x60,U)
>>> +#define FFA_FNUM_MAX_VALUE              _AC(0x86,U)
>>> +
>>> +static inline bool is_ffa_fid(uint32_t fid)
>>> +{
>>> +    uint32_t fn = fid & ARM_SMCCC_FUNC_MASK;
>>> +
>>> +    return fn >= FFA_FNUM_MIN_VALUE && fn <= FFA_FNUM_MAX_VALUE;
>>> +}
>>> +
>>> +#ifdef CONFIG_FFA
>>> +#define FFA_NR_FUNCS    12
>>> +#else
>>> +#define FFA_NR_FUNCS    0
>>> +#endif
>>> +
>>> +#endif /*__ASM_ARM_TEE_FFA_H__*/
>>> diff --git a/xen/arch/arm/tee/Kconfig b/xen/arch/arm/tee/Kconfig
>>> index 392169b2559d..923f08ba8cb7 100644
>>> --- a/xen/arch/arm/tee/Kconfig
>>> +++ b/xen/arch/arm/tee/Kconfig
>>> @@ -8,3 +8,14 @@ config OPTEE
>>>  virtualization-enabled OP-TEE present. You can learn more
>>>  about virtualization for OP-TEE at
>>>  https://optee.readthedocs.io/architecture/virtualization.html
>>> +
>>> +config FFA
>>> + bool "Enable FF-A mediator support (UNSUPPORTED)" if UNSUPPORTED
>>> + default n
>>> + depends on ARM_64
>> Even if the tee Makefile is only included if CONFIG_TEE is activated,
>> the missing dependency on TEE here allows to select FFA without TEE
>> resulting in a config with FFA activated but not compiled in.
>> No build error is coming from this, FFA is just not in if selected without 
>> TEE.
>> Should be:
>> depends on ARM_64 && TEE
>> I am ok if this is fixed on commit and my R-B kept.
> 
> I have fixed it and committed up to patch #9. That said, I think it would be 
> best if we have a category TEE where both the OPTEE and FFA config is under. 
> This will help to make the menuconfig clearer and avoid the "depends TEE".

Thanks :-)

> 
> Bertrand, Jens, can one of you have a look?

The fact that both depend on TEE is making both of those appearing under TEE in 
Kconfig and in menuconfig as sub-items of TEE.

But i agree having a menu for those would be nicer and I will propose a patch 
for that later today.

Cheers
Bertrand