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

Ilias Apalodimas
2022-11-15T09:04:01Z

Hi Abdellatif

On Mon, Nov 07, 2022 at 07:20:54PM +0000, 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.
>
> 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

This shouldn't rely on both. It's either OP-TEE or FF-A

>
> +#if (IS_ENABLED(CONFIG_OPTEE))

This separation is a bit problematic. A user can configure bot OP-TEE and
FF-A. Those are not mutually exclusive, but for the EFI
variables case they are. We need a better way to isolate the compilation
choices. Why don't we make ffa_bus_discover() return -1 if FF-A isn't
compiled in?

> /**
> * get_connection() - Retrieve OP-TEE session for a specific UUID.
> *
> @@ -143,13 +176,229 @@ static efi_status_t optee_mm_communicate(void *comm_buf, ulong dsize)
>
> return ret;
> }
> +#endif
> +
> +#if (IS_ENABLED(CONFIG_ARM_FFA_TRANSPORT))
> +
> +/**
> + * ffa_notify_mm_sp() - Announce there is data in the shared buffer
> + *
> + * Notifies the MM partition in the trusted world that
> + * data is available in the shared buffer.
> + * This is a blocking call during which trusted world has exclusive access
> + * to the MM shared buffer.
> + *
> + * 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;
> +
> + if (!ffa_bus_ops_get())
> + return -EINVAL;
> +
> + msg.data0 = FFA_SHARED_MM_BUFFER_OFFSET; /* x3 */
> +
> + ret = ffa_bus_ops_get()->sync_send_receive(mm_sp_id, &msg, 1);
> + if (ret != 0)
> + return ret;
> +
> + sp_event_ret = msg.data0; /* x3 */
> +
> + if (sp_event_ret == MM_SUCCESS)
> + return 0;
> +
> + /*
> + * Failure to notify the MM SP
> + */
> +
> + return -EACCES;
> +}
> +
> +/**
> + * ffa_discover_mm_sp_id() - Query the MM partition ID
> + *
> + * Use the FF-A driver to get the MM partition ID.
> + * If multiple partitions are found, use the first one.
> + * This is a boot time function.
> + *
> + * Return:
> + *
> + * 0 on success
> + */
> +static int ffa_discover_mm_sp_id(void)
> +{
> + u32 count = 0, size = 0;
> + int ret;
> + struct ffa_partition_info *parts_info;
> +
> + if (!ffa_bus_ops_get())
> + return -EINVAL;
> +
> + /*
> + * get from the driver the count of the SPs matching the UUID
> + */
> + ret = ffa_bus_ops_get()->partition_info_get(mm_sp_svc_uuid, &count, NULL);
> + if (ret != 0) {
> + log_err("EFI: Failure in querying partitions count (error code: %d)\n", ret);
> + return ret;
> + }
> +
> + if (!count) {
> + log_info("EFI: No MM partition found\n");
> + return ret;
> + }
> +
> + /*
> + * pre-allocate a buffer to be filled by the driver
> + * with ffa_partition_info structs
> + */
> +
> + log_info("EFI: Pre-allocating %d partition(s) info structures\n", count);
> +
> + parts_info = calloc(count, sizeof(struct ffa_partition_info));

I prefer sizeof(*parts_info). Same goes for all sizeof() calls.

> + if (!parts_info)
> + return -ENOMEM;
> +
> + size = count * sizeof(struct ffa_partition_info);
> +
> + /*
> + * ask the driver to fill the
> + * buffer with the SPs info
> + */
> + ret = ffa_bus_ops_get()->partition_info_get(mm_sp_svc_uuid, &size, parts_info);
> + if (ret != 0) {

if (!ret)

[...]

Thanks
/Ilias