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