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

[Abdellatif El Khlifi]

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