Hi, On Thu, Jul 28, 2022 at 9:15 PM Julien Grall <jul...@xen.org> wrote: > > Hi, > > On 26/07/2022 07:17, Jens Wiklander wrote: > > On Fri, Jul 8, 2022 at 3:41 PM Julien Grall <jul...@xen.org> wrote: > >> > >> Hi Jens, > >> > >> I haven't checked whether the FFA driver is complaint with the spec. I > >> mainly checked whether the code makes sense from Xen PoV. > >> > >> This is a fairly long patch to review. So I will split my review in > >> multiple session/e-mail. > >> > >> On 22/06/2022 14:42, Jens Wiklander wrote: > >>> Adds a FF-A version 1.1 [1] mediator to communicate with a Secure > >>> Partition in secure world. > >>> > >>> The implementation is the bare minimum to be able to communicate with > >>> OP-TEE running as an SPMC at S-EL1. > >>> > >>> This is loosely based on the TEE mediator framework and the OP-TEE > >>> mediator. > >>> > >>> [1] https://developer.arm.com/documentation/den0077/latest > >>> Signed-off-by: Jens Wiklander <jens.wiklan...@linaro.org> > >>> --- > >>> SUPPORT.md | 7 + > >>> tools/libs/light/libxl_arm.c | 3 + > >>> tools/libs/light/libxl_types.idl | 1 + > >>> tools/xl/xl_parse.c | 3 + > >> > >> These changes would need an ack from the toolstack maintainers. > > > > OK, I'll keep them in CC. > > > >> > >>> xen/arch/arm/Kconfig | 11 + > >>> xen/arch/arm/Makefile | 1 + > >>> xen/arch/arm/domain.c | 10 + > >>> xen/arch/arm/domain_build.c | 1 + > >>> xen/arch/arm/ffa.c | 1683 +++++++++++++++++++++++++++++ > >>> xen/arch/arm/include/asm/domain.h | 4 + > >>> xen/arch/arm/include/asm/ffa.h | 71 ++ > >>> xen/arch/arm/vsmc.c | 17 +- > >>> xen/include/public/arch-arm.h | 2 + > >>> 13 files changed, 1811 insertions(+), 3 deletions(-) > >>> create mode 100644 xen/arch/arm/ffa.c > >>> create mode 100644 xen/arch/arm/include/asm/ffa.h > >>> > >>> diff --git a/SUPPORT.md b/SUPPORT.md > >>> index 70e98964cbc0..215bb3c9043b 100644 > >>> --- a/SUPPORT.md > >>> +++ b/SUPPORT.md > >>> @@ -785,6 +785,13 @@ that covers the DMA of the device to be passed > >>> through. > >>> > >>> No support for QEMU backends in a 16K or 64K domain. > >>> > >>> +### ARM: Firmware Framework for Arm A-profile (FF-A) Mediator > >>> + > >>> + Status, Arm64: Tech Preview > >>> + > >>> +There are still some code paths where a vCPU may hog a pCPU longer than > >>> +necessary. The FF-A mediator is not yet implemented for Arm32. > >>> + > >>> ### ARM: Guest Device Tree support > >>> > >>> Status: Supported > >>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c > >>> index eef1de093914..a985609861c7 100644 > >>> --- a/tools/libs/light/libxl_arm.c > >>> +++ b/tools/libs/light/libxl_arm.c > >>> @@ -101,6 +101,9 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, > >>> return ERROR_FAIL; > >>> } > >>> > >>> + config->arch.ffa_enabled = > >>> + libxl_defbool_val(d_config->b_info.arch_arm.ffa_enabled); > >>> + > >>> return 0; > >>> } > >>> > >>> diff --git a/tools/libs/light/libxl_types.idl > >>> b/tools/libs/light/libxl_types.idl > >>> index 2a42da2f7d78..bf4544bef399 100644 > >>> --- a/tools/libs/light/libxl_types.idl > >>> +++ b/tools/libs/light/libxl_types.idl > >>> @@ -646,6 +646,7 @@ libxl_domain_build_info = Struct("domain_build_info",[ > >>> > >>> ("arch_arm", Struct(None, [("gic_version", libxl_gic_version), > >>> ("vuart", libxl_vuart_type), > >>> + ("ffa_enabled", libxl_defbool), > >> > >> This needs to be accompagnied with a define LIBXL_HAVE_* in > >> tools/include/libxl.h. Please see the examples in the header. > > > > OK, I'll add something. I'm not entirely sure how this is used so I'm > > afraid it will be a bit of Cargo Cult programming from my side. > > The LIBXL_HAVE* by toolstack built on top of libxl (like virtio) to know > whether a future is supported by the current library. > > [...] > > >> > >>> + > >>> +static inline uint64_t reg_pair_to_64(uint32_t reg0, uint32_t reg1) > >>> +{ > >>> + return (uint64_t)reg0 << 32 | reg1; > >>> +} > >>> + > >>> +static void inline reg_pair_from_64(uint32_t *reg0, uint32_t *reg1, > >>> + uint64_t val) > >>> +{ > >>> + *reg0 = val >> 32; > >>> + *reg1 = val; > >>> +} > >> > >> Those two functions are the same as optee.c but with a different. I > >> would rather prefer if we avoid the duplication and provide generic > >> helpers in a pre-requisite. > > > > These functions are trivial but at the same time for a special purpose > > which happens to coincide with the usage in optee.c. I can't find a > > suitable common .h file and creating a new one seems a bit much. > > I would implement it in regs.h.
OK, thanks. > > [...] > > >>> + .a4 = pg_count, > >>> + }; > >>> + struct arm_smccc_1_2_regs resp; > >>> + > >>> + /* > >>> + * For arm64 we must use 64-bit calling convention if the buffer > >>> isn't > >>> + * passed in our tx buffer. > >>> + */ > >> > >> Can you explain why we would want to use the 32-bit calling convention > >> if addr is 0? > > > > I was trying to avoid the 64-bit calling convention where possible, > > OOI, why are you trying to avoid the 64-bit calling convention? To make it easier to support a use-case where the SPMC is using AArch32, but I'm not sure it's realistic any longer. Cheers, Jens
