Re: [PATCH v8 2/2] firmware: arm_scmi: Add optee transport

[Sudeep Holla <sudeep.holla@arm.com>]

On Thu, Oct 28, 2021 at 04:00:09PM +0200, Etienne Carriere wrote:
> Add a new transport channel to the SCMI firmware interface driver for
> SCMI message exchange based on optee transport channel. The optee
> transport is realized by connecting and invoking OP-TEE SCMI service
> interface PTA.
> 
> Optee transport support (CONFIG_ARM_SCMI_TRANSPORT_OPTEE) is default
> enabled when optee driver (CONFIG_OPTEE) is enabled. Effective optee
> transport is setup upon OP-TEE SCMI service discovery at optee
> device initialization. For this SCMI UUID is registered to the optee
> bus for probing. This is done from the link_supplier operator of the
> SCMI optee transport.
> 
> The optee transport can use a statically defined shared memory in
> which case SCMI device tree node defines it using an "arm,scmi-shmem"
> compatible phandle through property shmem. Alternatively, optee transport
> allocates the shared memory buffer from the optee driver when no shmem
> property is defined.
> 
> The protocol used to exchange SCMI message over that shared memory is
> negotiated between optee transport driver and the OP-TEE service through
> capabilities exchange.
> 
> OP-TEE SCMI service is integrated in OP-TEE since its release tag 3.13.0.
> The service interface is published in [1].
> 
> Link: [1] https://github.com/OP-TEE/optee_os/blob/3.13.0/lib/libutee/include/pta_scmi_client.h
> Cc: Cristian Marussi <cristian.marussi@arm.com>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
> Changes since v7:
>  - Fix transport exit to not unregister scmi optee driver from tee bus
>    if the transport was not initialized (hence registered to tee bus).
> 
> Changes since v6:
>  - Fixed at last s/CFG_OPTEE/CONFIG_OPTEE/ in commit log.
> 
> Changes since v5:
>  - scmi_optee_link_supplier() doesn't test scmi_optee_private->tee_ctx.
>  - Free allocated shared memory when scmi_optee_chan_setup() fails.
>  - Close session to TEE SCMI service when SCMI channel is freed.
>  - Use SCMI_OPTEE_MAX_MSG_SIZE in SCMI transport descriptor.
> 
> Changes since v4:
>  - Fix commit log that was not updated to v4 changes.
>  - Operator scmi_optee_chan_setup() don't need the defer probe
>    operation, it's already done from scmi_optee_link_supplier().
> 
> Changes since v3:
>  - Fix use of configuration switches when CONFIG_OPTEE and
>    CONFIG_ARM_SCMI_PROTOCOL are enabled/modules/disabled.
>    Mimics scmi virtio integration.
>  - Implement link_supplier operator for the scmi_optee transport
>    to possibly defer probing when optee bus has not yet enumerated
>    the SCMI OP-TEE service. The function ensures scmi_optee registers
>    to optee bus enumeration when probe is deferred.
>  - Add memory barriers to protect global optee service reference
>    when it's updated at transport initialization and removal.
>  - Replace enum pta_scmi_caps with macro definitions as enumerated
>    types do not really match bit flags definitions. The capabilities
>    data is now of type u32.
>  - Use scmi_optee_ prefix for scmi transport operator handles
>    and few other resources.
>  - Fix typo: s/optee_smci_pta_cmd/optee_scmi_pta_cmd/
>  - Remove useless DRIVER_NAME.
>  - Minor reordering in struct optee_channel.
>  - Removed some useless empty lines.
> 
> Changes since v2:
> - Rebase on for-next/scmi, based on Linux v5.15-rc1.
> - Implement support for dynamic and static shared memory.
> - Factorize some functions and simplify transport exit sequence.
> - Rename driver source file from optee_service.c to optee.c.
> 
> No change since v1
> ---
>  drivers/firmware/arm_scmi/Kconfig  |  12 +
>  drivers/firmware/arm_scmi/Makefile |   1 +
>  drivers/firmware/arm_scmi/common.h |   3 +
>  drivers/firmware/arm_scmi/driver.c |   3 +
>  drivers/firmware/arm_scmi/optee.c  | 581 +++++++++++++++++++++++++++++
>  5 files changed, 600 insertions(+)
>  create mode 100644 drivers/firmware/arm_scmi/optee.c
> 

[...]

> +static int setup_dynamic_shmem(struct device *dev, struct scmi_optee_channel *channel)
> +{
> +	const size_t msg_size = SCMI_OPTEE_MAX_MSG_SIZE;
> +
> +	channel->tee_shm = tee_shm_alloc_kernel_buf(scmi_optee_private->tee_ctx, msg_size);
> +	if (IS_ERR(channel->tee_shm)) {
> +		dev_err(channel->cinfo->dev, "shmem allocation failed\n");
> +		return -ENOMEM;
> +	}
> +
> +	channel->shmem = (void *)tee_shm_get_va(channel->tee_shm, 0);
> +	memset(channel->shmem, 0, msg_size);
> +	shmem_clear_channel(channel->shmem);
> +
> +	return 0;
> +}

I was holding on applying this patch as I reviewed this partially when I was
on vacation and couldn't remember the comment I had until I just replied now
with applied message 😄.

Anyways, is this dynamic_shmem tested on a hardware ?
shmem_* apis are based on __iomem and IIUC tee_shm_alloc_kernel_buf returns
normal memory, so you can't use those shmem_* apis as is. Please drop the
whole dynamic_shmem and send me a patch for v5.17 as we may need more changes
to support that.

I remember Broadcom or someone else wanted this with normal memory, we can
add that support correctly at once involving them too. For now, please drop
that or I can post a patch to do that if you agree with my arguments.

Sorry for completely forgetting about this.
--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel