Re: [PATCH v12 09/10] arm_ffa: efi: introduce FF-A MM communication

Abdellatif El Khlifi
2023-05-19T13:37:12Z

Hi Ilias,

> Hi Abdellatif
>
> I still have some concerns on this
>
> In the past [0] I asking why this needs to be a Kconfig option. Since FF-A
> is a mechanism we can use to discover SPs, in theory we dont need the
> ifdefery. We might need something if the code difference grows too much
> but I think we are fine for now.

Is my understanding correct you prefer that CONFIG_EFI_MM_COMM_TEE implies
CONFIG_ARM_FFA_TRANSPORT ? So, no ifdefs anymore in efi_variable_tee.c.

>
> On top of that I am nissing how was this tested? did you test the current
> SMC to optee and the stmm for the RPMB backend?

We use Corstone-1000 platform for testing FF-A.
U-Boot communicates with smm-gateway (leight version of stmm) using FF-A SMC ABIs.
In the secure world we use TF-A, Optee OS and Trusted Services providing smm-gateway SP.
In U-Boot we don't use the Optee driver because we use FF-A communication only.
I hope this clears all doubts.

Cheers
Abdellatif

>
> > +++ b/include/mm_communication.h
> > @@ -6,6 +6,9 @@
> > * Copyright (c) 2017, Intel Corporation. All rights reserved.
> > * Copyright (C) 2020 Linaro Ltd. <sughosh.ganu@linaro.org>
> > * Copyright (C) 2020 Linaro Ltd. <ilias.apalodimas@linaro.org>
> > + * Copyright 2022-2023 Arm Limited and/or its affiliates <open-source-office@arm.com>
> > + * Authors:
> > + * Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> > */
> >
> > #ifndef _MM_COMMUNICATION_H_
> > @@ -13,6 +16,9 @@
> >
> > #include <part_efi.h>
> >
> > +/* MM service UUID string (big-endian format). This UUID is common across all MM SPs */
> > +#define MM_SP_UUID "33d532ed-e699-0942-c09c-a798d9cd722d"
> > +
> > /*
> > * Interface to the pseudo Trusted Application (TA), which provides a
> > * communication channel with the Standalone MM (Management Mode)
> > @@ -248,4 +254,11 @@ struct smm_variable_var_check_property {
> > u16 name[];
> > };
> >
> > +/* supported MM transports */
> > +enum mm_comms_select {
> > + MM_COMMS_UNDEFINED,
> > + MM_COMMS_FFA,
> > + MM_COMMS_OPTEE
> > +};
> > +
> > #endif /* _MM_COMMUNICATION_H_ */
> > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > index c5835e6ef6..08a6b84101 100644
> > --- a/lib/efi_loader/Kconfig
> > +++ b/lib/efi_loader/Kconfig
> > @@ -55,13 +55,23 @@ config EFI_VARIABLE_FILE_STORE
> > stored as file /ubootefi.var on the EFI system partition.
> >
> > config EFI_MM_COMM_TEE
> > - bool "UEFI variables storage service via OP-TEE"
> > - depends on OPTEE
> > + bool "UEFI variables storage service via the trusted world"
> > + depends on OPTEE || ARM_FFA_TRANSPORT
> > help
> > + Allowing access to the MM SP services (SPs such as StandAlonneMM, smm-gateway).
> > + When using the u-boot OP-TEE driver, StandAlonneMM is supported.
> > + When using the u-boot FF-A driver any MM SP is supported.
> > +
> > If OP-TEE is present and running StandAloneMM, dispatch all UEFI
> > variable related operations to that. The application will verify,
> > authenticate and store the variables on an RPMB.
> >
> > + When ARM_FFA_TRANSPORT is used, dispatch all UEFI variable related
> > + operations to the MM SP running in the secure world.
> > + A door bell mechanism is used to notify the SP when there is data in the shared
> > + MM buffer. The data is copied by u-boot to the shared buffer before issuing
> > + the door bell event.
> > +
> > config EFI_VARIABLE_NO_STORE
> > bool "Don't persist non-volatile UEFI variables"
> > help
> > diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
> > index dfef18435d..2effb705d7 100644
> > --- a/lib/efi_loader/efi_variable_tee.c
> > +++ b/lib/efi_loader/efi_variable_tee.c
> > @@ -4,9 +4,14 @@
> > *
> > * Copyright (C) 2019 Linaro Ltd. <sughosh.ganu@linaro.org>
> > * Copyright (C) 2019 Linaro Ltd. <ilias.apalodimas@linaro.org>
> > + * Copyright 2022-2023 Arm Limited and/or its affiliates <open-source-office@arm.com>
> > + *
> > + * Authors:
> > + * Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> > */
> >
> > #include <common.h>
> > +#include <dm.h>
> > #include <efi.h>
> > #include <efi_api.h>
> > #include <efi_loader.h>
> > @@ -15,6 +20,36 @@
> > #include <malloc.h>
> > #include <mm_communication.h>
> >
> > +#if IS_ENABLED(CONFIG_ARM_FFA_TRANSPORT)
> > +
> > +#include <arm_ffa.h>
> > +#include <cpu_func.h>
> > +#include <mapmem.h>
> > +
> > +#ifndef FFA_SHARED_MM_BUFFER_SIZE
> > +#error "FFA_SHARED_MM_BUFFER_SIZE must be defined in include/configs/<board>.h"
> > +#define FFA_SHARED_MM_BUFFER_SIZE 0
> > +#endif
> > +
> > +#ifndef FFA_SHARED_MM_BUFFER_OFFSET
> > +#error "FFA_SHARED_MM_BUFFER_OFFSET must be defined in include/configs/<board>.h"
> > +#define FFA_SHARED_MM_BUFFER_OFFSET 0
> > +#endif
> > +
> > +#ifndef FFA_SHARED_MM_BUFFER_ADDR
> > +#error "FFA_SHARED_MM_BUFFER_ADDR must be defined in include/configs/<board>.h"
> > +#define FFA_SHARED_MM_BUFFER_ADDR 0
> > +#endif
> > +
> > +/* MM return codes */
> > +#define MM_SUCCESS (0)
> > +
> > +static const char *mm_sp_svc_uuid = MM_SP_UUID;
> > +
> > +static u16 mm_sp_id;
> > +
> > +#endif
> > +
> > extern struct efi_var_file __efi_runtime_data *efi_var_buf;
> > static efi_uintn_t max_buffer_size; /* comm + var + func + data */
> > static efi_uintn_t max_payload_size; /* func + data */
> > @@ -24,6 +59,7 @@ struct mm_connection {
> > u32 session;
> > };
> >
> > +#if (IS_ENABLED(CONFIG_OPTEE))
>
> > /**
> > * get_connection() - Retrieve OP-TEE session for a specific UUID.
> > *
>
> [...]
>
> Isn't this problematic? At the moment there's not > 8.4 hardware out
> there. As a result the SPMC *is* implemented in OP-TEE. So how do we
> expect FF-A to work?
>
> [...]
>
> [0] https://lore.kernel.org/u-boot/Y3NV991bKRgas1zZ@hera/
>
> Thanks
> /Ilias