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

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

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