Re: [PATCH v4 2/2] xen/arm: add FF-A mediator

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

Hi,

On Thu, Jul 28, 2022 at 9:15 PM Julien Grall <julien@xxxxxxx> wrote:
>
> Hi,
>
> On 26/07/2022 07:17, Jens Wiklander wrote:
> > On Fri, Jul 8, 2022 at 3:41 PM Julien Grall <julien@xxxxxxx> 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.wiklander@xxxxxxxxxx>
> >>> ---
> >>>    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