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.elkhl...@arm.com> > Signed-off-by: Gowtham Suresh Kumar <gowtham.sureshku...@arm.com> > Cc: Tom Rini <tr...@konsulko.com> > Cc: Simon Glass <s...@chromium.org> > Cc: Ilias Apalodimas <ilias.apalodi...@linaro.org> > Cc: Jens Wiklander <jens.wiklan...@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