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

[Julien Grall <julien@xxxxxxx>]

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".
Bertrand, Jens, can one of you have a look?

Cheers,

--
Julien Grall