On Tue, Nov 15, 2022 at 11:03:51AM +0200, Ilias Apalodimas wrote: > 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
Thanks. Addressed in v8 patchset. > > > > > +#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? I agree, this is addressed in v8 patchset. > > > /** > > * 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. Addressed in v8 patchset. > > > + 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) Addressed in v8 patchset. > > > [...] > > Thanks > /Ilias