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

Ilias Apalodimas
2023-06-21T06:22:05Z

Hi Abdellatif,

On Fri, Jun 16, 2023 at 04:28:16PM +0100, Abdellatif El Khlifi wrote:
> Add MM communication support using FF-A transport
>
> This feature allows accessing MM partitions services through
> EFI MM communication protocol. MM partitions such as StandAlonneMM
> or smm-gateway secure partitions which reside in secure world.
>
> An MM shared buffer and a door bell event are used to exchange
> the data.
>
> The data is used by EFI services such as GetVariable()/SetVariable()
> and copied from the communication buffer to the MM shared buffer.
>
> The secure partition is notified about availability of data in the
> MM shared buffer by an FF-A message (door bell).
>
> On such event, MM SP can read the data and updates the MM shared
> buffer with the response data.
>
> The response data is copied back to the communication buffer and
> consumed by the EFI subsystem.
>
> MM communication protocol supports FF-A 64-bit direct messaging.
>
> Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> Signed-off-by: Gowtham Suresh Kumar <gowtham.sureshkumar@arm.com>
> Cc: Tom Rini <trini@konsulko.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Cc: Jens Wiklander <jens.wiklander@linaro.org>
>
> ---
>
> Changelog:
> ===============
>
> v13:
>
> * remove FF-A and Optee ifdefs

Thanks this looks a lot saner now. I got one last nit and I think this
patch is ready

> + * Return:
> + *
> + * 0 on success
> + */
> +static int ffa_notify_mm_sp(void)
> +{
> + struct ffa_send_direct_data msg = {0};
> + int ret;
> + int sp_event_ret = -1;
> + struct udevice *dev;
> +
> + ret = uclass_first_device_err(UCLASS_FFA, &dev);
> + if (ret) {
> + log_err("EFI: Cannot find FF-A bus device, notify MM SP failure\n");
> + return ret;
> + }
> +
> + msg.data0 = FFA_SHARED_MM_BUFFER_OFFSET; /* x3 */
> +
> + ret = ffa_sync_send_receive(dev, mm_sp_id, &msg, 1);
> + if (ret)
> + return ret;
> +
> + sp_event_ret = msg.data0; /* x3 */
> +
> + if (sp_event_ret == MM_SUCCESS)
> + return 0;
> +
> + /* Failure to notify the MM SP */
> +
> + return -EACCES;

Doesn't FFA and the SMM_GATEWAY have discrete returns results that would
make more sense? Your other patches only define MM_SUCCESS but in
ffa_mm_communicate() you are trying to map ernnos to EFI return codes.
I think we should map errnos to ffa errors as well in a similar fashion.

You can look at optee_mm_communicate() which already does that.

> +}
> +
> +/**
> + * ffa_discover_mm_sp_id() - Query the MM partition ID
> + *
> +/**
> + * get_mm_comms() - detect the available MM transport
> + *
> + * Make sure the FF-A bus is probed successfully
> + * which means FF-A communication with secure world works and ready
> + * for use.
> + *
> + * If FF-A bus is not ready, use OPTEE comms.
> + *
> + * Return:
> + *
> + * MM_COMMS_FFA or MM_COMMS_OPTEE
> + */
> +static enum mm_comms_select get_mm_comms(void)
> +{
> + struct udevice *dev;
> + int ret;
> +
> + ret = uclass_first_device_err(UCLASS_FFA, &dev);
> + if (ret) {
> + log_err("EFI: Cannot find FF-A bus device, trying Optee comms\n");
> + return MM_COMMS_OPTEE;
> + }
> +
> + return MM_COMMS_FFA;
> +}
> +
> +/**
> + * mm_communicate() - Adjust the communication buffer to the MM SP and send
> * it to OP-TEE
> *
> - * @comm_buf: locally allocted communcation buffer
> + * @comm_buf: locally allocated communication buffer
> * @dsize: buffer size
> + *
> + * The SP (also called partition) can be any MM SP such as StandAlonneMM or smm-gateway.
> + * The comm_buf format is the same for both partitions.
> + * When using the u-boot OP-TEE driver, StandAlonneMM is supported.
> + * When using the u-boot FF-A driver, any MM SP is supported.
> + *
> * Return: status code
> */
> static efi_status_t mm_communicate(u8 *comm_buf, efi_uintn_t dsize)
> {
> efi_status_t ret;
> + enum mm_comms_select mm_comms;
> struct efi_mm_communicate_header *mm_hdr;
> struct smm_variable_communicate_header *var_hdr;
>
> @@ -162,7 +400,12 @@ static efi_status_t mm_communicate(u8 *comm_buf, efi_uintn_t dsize)
> mm_hdr = (struct efi_mm_communicate_header *)comm_buf;
> var_hdr = (struct smm_variable_communicate_header *)mm_hdr->data;
>
> - ret = optee_mm_communicate(comm_buf, dsize);
> + mm_comms = get_mm_comms();
> + if (mm_comms == MM_COMMS_FFA)
> + ret = ffa_mm_communicate(comm_buf, dsize);
> + else
> + ret = optee_mm_communicate(comm_buf, dsize);
> +
> if (ret != EFI_SUCCESS) {
> log_err("%s failed!\n", __func__);
> return ret;
> @@ -232,6 +475,7 @@ static u8 *setup_mm_hdr(void **dptr, efi_uintn_t payload_size,
> */
> efi_status_t EFIAPI get_max_payload(efi_uintn_t *size)
> {
> + enum mm_comms_select mm_comms;
> struct smm_variable_payload_size *var_payload = NULL;
> efi_uintn_t payload_size;
> u8 *comm_buf = NULL;
> @@ -258,6 +502,12 @@ efi_status_t EFIAPI get_max_payload(efi_uintn_t *size)
> goto out;
> }
> *size = var_payload->size;
> +
> + mm_comms = get_mm_comms();
> + if (mm_comms == MM_COMMS_FFA && *size > FFA_SHARED_MM_BUFFER_SIZE)
> + *size = FFA_SHARED_MM_BUFFER_SIZE - MM_COMMUNICATE_HEADER_SIZE -
> + MM_VARIABLE_COMMUNICATE_SIZE;
> +

Can you please move this check? The check preceding this is generic -- it
tries to make sure there's space for at least a variable. This is ffa
specific, so is there any reason ffa_mm_communicate() doesn't return the
corrected size?

> /*
> * There seems to be a bug in EDK2 miscalculating the boundaries and
> * size checks, so deduct 2 more bytes to fulfill this requirement. Fix
> @@ -697,7 +947,7 @@ void efi_variables_boot_exit_notify(void)
> ret = EFI_NOT_FOUND;
>
> if (ret != EFI_SUCCESS)
> - log_err("Unable to notify StMM for ExitBootServices\n");
> + log_err("Unable to notify the MM partition for ExitBootServices\n");
> free(comm_buf);
>
> /*
> --
> 2.25.1
>

Thanks
/Ilias