[PATCH v2] firmware: arm_scmi: support optee shared memory in optee transport

[PATCH v2] firmware: arm_scmi: support optee shared memory in optee transport

[Etienne Carriere]

Adds support for tee shared memory in optee scmi transport. When using
tee shared memory, scmi optee transport manages SCMI messages using
msg protocol (from msg.c) in shared memory, whereas smt (from shmem.c)
protocol is used with static IOMEM shared buffers.

Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
---
 drivers/firmware/arm_scmi/optee.c | 128 ++++++++++++++++++++++++------
 1 file changed, 104 insertions(+), 24 deletions(-)

diff --git a/drivers/firmware/arm_scmi/optee.c b/drivers/firmware/arm_scmi/optee.c
index 8302a2b4aeeb..6676b17d2587 100644
--- a/drivers/firmware/arm_scmi/optee.c
+++ b/drivers/firmware/arm_scmi/optee.c
@@ -64,6 +64,22 @@ enum scmi_optee_pta_cmd {
 	 * [in]     value[0].b: Requested capabilities mask (enum pta_scmi_caps)
 	 */
 	PTA_SCMI_CMD_GET_CHANNEL = 3,
+
+	/*
+	 * PTA_SCMI_CMD_PROCESS_MSG_CHANNEL - Process SCMI message in a MSG
+	 * buffer pointed by memref parameters
+	 *
+	 * [in]     value[0].a: Channel handle
+	 * [in]     memref[1]: Message buffer (MSG and SCMI payload)
+	 * [out]    memref[2]: Response buffer (MSG and SCMI payload)
+	 *
+	 * Shared memories used for SCMI message/response are MSG buffers
+	 * referenced by param[1] and param[2]. MSG transport protocol
+	 * uses a 32bit header to carry SCMI meta-data (protocol ID and
+	 * protocol message ID) followed by the effective SCMI message
+	 * payload.
+	 */
+	PTA_SCMI_CMD_PROCESS_MSG_CHANNEL = 4,
 };
 
 /*
@@ -72,9 +88,17 @@ enum scmi_optee_pta_cmd {
  * PTA_SCMI_CAPS_SMT_HEADER
  * When set, OP-TEE supports command using SMT header protocol (SCMI shmem) in
  * shared memory buffers to carry SCMI protocol synchronisation information.
+ *
+ * PTA_SCMI_CAPS_MSG_HEADER
+ * When set, OP-TEE supports command using MSG header protocol in an OP-TEE
+ * shared memory to carry SCMI protocol synchronisation information and SCMI
+ * message payload.
  */
 #define PTA_SCMI_CAPS_NONE		0
 #define PTA_SCMI_CAPS_SMT_HEADER	BIT(0)
+#define PTA_SCMI_CAPS_MSG_HEADER	BIT(1)
+#define PTA_SCMI_CAPS_MASK		(PTA_SCMI_CAPS_SMT_HEADER | \
+					 PTA_SCMI_CAPS_MSG_HEADER)
 
 /**
  * struct scmi_optee_channel - Description of an OP-TEE SCMI channel
@@ -85,7 +109,8 @@ enum scmi_optee_pta_cmd {
  * @mu: Mutex protection on channel access
  * @cinfo: SCMI channel information
  * @shmem: Virtual base address of the shared memory
- * @tee_shm: Reference to TEE shared memory or NULL if using static shmem
+ * @req: Shared memory protocol handle for SCMI request and synchronous response
+ * @tee_shm: TEE shared memory handle @req or NULL if using IOMEM shmem
  * @link: Reference in agent's channel list
  */
 struct scmi_optee_channel {
@@ -94,7 +119,10 @@ struct scmi_optee_channel {
 	u32 caps;
 	struct mutex mu;
 	struct scmi_chan_info *cinfo;
-	struct scmi_shared_mem __iomem *shmem;
+	union {
+		struct scmi_shared_mem __iomem *shmem;
+		struct scmi_msg_payld *msg;
+	} req;
 	struct tee_shm *tee_shm;
 	struct list_head link;
 };
@@ -178,8 +206,8 @@ static int get_capabilities(struct scmi_optee_agent *agent)
 
 	caps = param[0].u.value.a;
 
-	if (!(caps & PTA_SCMI_CAPS_SMT_HEADER)) {
-		dev_err(agent->dev, "OP-TEE SCMI PTA doesn't support SMT\n");
+	if (!(caps & (PTA_SCMI_CAPS_SMT_HEADER | PTA_SCMI_CAPS_MSG_HEADER))) {
+		dev_err(agent->dev, "OP-TEE SCMI PTA doesn't support SMT and MSG\n");
 		return -EOPNOTSUPP;
 	}
 
@@ -193,9 +221,14 @@ static int get_channel(struct scmi_optee_channel *channel)
 	struct device *dev = scmi_optee_private->dev;
 	struct tee_ioctl_invoke_arg arg = { };
 	struct tee_param param[1] = { };
-	unsigned int caps = PTA_SCMI_CAPS_SMT_HEADER;
+	unsigned int caps = 0;
 	int ret;
 
+	if (channel->tee_shm)
+		caps = PTA_SCMI_CAPS_MSG_HEADER;
+	else
+		caps = PTA_SCMI_CAPS_SMT_HEADER;
+
 	arg.func = PTA_SCMI_CMD_GET_CHANNEL;
 	arg.session = channel->tee_session;
 	arg.num_params = 1;
@@ -249,6 +282,37 @@ static int invoke_process_smt_channel(struct scmi_optee_channel *channel)
 	return 0;
 }
 
+static int invoke_process_msg_channel(struct scmi_optee_channel *channel, size_t msg_size)
+{
+	struct tee_ioctl_invoke_arg arg = {
+		.func = PTA_SCMI_CMD_PROCESS_MSG_CHANNEL,
+		.session = channel->tee_session,
+		.num_params = 3,
+	};
+	struct tee_param param[3] = { };
+	int ret;
+
+	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
+	param[0].u.value.a = channel->channel_id;
+
+	param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT;
+	param[1].u.memref.shm = channel->tee_shm;
+	param[1].u.memref.size = msg_size;
+
+	param[2].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
+	param[2].u.memref.shm = channel->tee_shm;
+	param[2].u.memref.size = SCMI_OPTEE_MAX_MSG_SIZE;
+
+	ret = tee_client_invoke_func(scmi_optee_private->tee_ctx, &arg, param);
+	if (ret < 0 || arg.ret) {
+		dev_err(scmi_optee_private->dev, "Can't invoke channel %u: %d / %#x\n",
+			channel->channel_id, ret, arg.ret);
+		return -EIO;
+	}
+
+	return 0;
+}
+
 static int scmi_optee_link_supplier(struct device *dev)
 {
 	if (!scmi_optee_private) {
@@ -279,7 +343,26 @@ static void scmi_optee_clear_channel(struct scmi_chan_info *cinfo)
 {
 	struct scmi_optee_channel *channel = cinfo->transport_info;
 
-	shmem_clear_channel(channel->shmem);
+	if (!channel->tee_shm)
+		shmem_clear_channel(channel->req.shmem);
+}
+
+static int setup_dynamic_shmem(struct device *dev, struct scmi_optee_channel *channel)
+{
+	const size_t msg_size = SCMI_OPTEE_MAX_MSG_SIZE;
+	void *shbuf;
+
+	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;
+	}
+
+	shbuf = tee_shm_get_va(channel->tee_shm, 0);
+	memset(shbuf, 0, msg_size);
+	channel->req.msg = shbuf;
+
+	return 0;
 }
 
 static int setup_static_shmem(struct device *dev, struct scmi_chan_info *cinfo,
@@ -304,8 +387,8 @@ static int setup_static_shmem(struct device *dev, struct scmi_chan_info *cinfo,
 
 	size = resource_size(&res);
 
-	channel->shmem = devm_ioremap(dev, res.start, size);
-	if (!channel->shmem) {
+	channel->req.shmem = devm_ioremap(dev, res.start, size);
+	if (!channel->req.shmem) {
 		dev_err(dev, "Failed to ioremap SCMI Tx shared memory\n");
 		ret = -EADDRNOTAVAIL;
 		goto out;
@@ -325,7 +408,7 @@ static int setup_shmem(struct device *dev, struct scmi_chan_info *cinfo,
 	if (of_find_property(cinfo->dev->of_node, "shmem", NULL))
 		return setup_static_shmem(dev, cinfo, channel);
 	else
-		return -ENOMEM;
+		return setup_dynamic_shmem(dev, channel);
 }
 
 static int scmi_optee_chan_setup(struct scmi_chan_info *cinfo, struct device *dev, bool tx)
@@ -405,27 +488,22 @@ static int scmi_optee_chan_free(int id, void *p, void *data)
 	return 0;
 }
 
-static struct scmi_shared_mem __iomem *
-get_channel_shm(struct scmi_optee_channel *chan, struct scmi_xfer *xfer)
-{
-	if (!chan)
-		return NULL;
-
-	return chan->shmem;
-}
-
-
 static int scmi_optee_send_message(struct scmi_chan_info *cinfo,
 				   struct scmi_xfer *xfer)
 {
 	struct scmi_optee_channel *channel = cinfo->transport_info;
-	struct scmi_shared_mem __iomem *shmem = get_channel_shm(channel, xfer);
 	int ret;
 
 	mutex_lock(&channel->mu);
-	shmem_tx_prepare(shmem, xfer);
 
-	ret = invoke_process_smt_channel(channel);
+	if (channel->tee_shm) {
+		msg_tx_prepare(channel->req.msg, xfer);
+		ret = invoke_process_msg_channel(channel, msg_command_size(xfer));
+	} else {
+		shmem_tx_prepare(channel->req.shmem, xfer);
+		ret = invoke_process_smt_channel(channel);
+	}
+
 	if (ret)
 		mutex_unlock(&channel->mu);
 
@@ -436,9 +514,11 @@ static void scmi_optee_fetch_response(struct scmi_chan_info *cinfo,
 				      struct scmi_xfer *xfer)
 {
 	struct scmi_optee_channel *channel = cinfo->transport_info;
-	struct scmi_shared_mem __iomem *shmem = get_channel_shm(channel, xfer);
 
-	shmem_fetch_response(shmem, xfer);
+	if (channel->tee_shm)
+		msg_fetch_response(channel->req.msg, SCMI_OPTEE_MAX_MSG_SIZE, xfer);
+	else
+		shmem_fetch_response(channel->req.shmem, xfer);
 }
 
 static void scmi_optee_mark_txdone(struct scmi_chan_info *cinfo, int ret,
-- 
2.17.1


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

Re: [PATCH v2] firmware: arm_scmi: support optee shared memory in optee transport

[kernel test robot]

Hi Etienne,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on soc/for-next]
[also build test ERROR on linus/master v5.18-rc3 next-20220421]
[cannot apply to linux/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Etienne-Carriere/firmware-arm_scmi-support-optee-shared-memory-in-optee-transport/20220421-155132
base:   https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git for-next
config: arm64-randconfig-r034-20220421 (https://download.01.org/0day-ci/archive/20220422/202204220711.5cl9KJrB-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project bac6cd5bf85669e3376610cfc4c4f9ca015e7b9b)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/8baa12e16d3e48bce0e6d86e8c7d0415439b7c98
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Etienne-Carriere/firmware-arm_scmi-support-optee-shared-memory-in-optee-transport/20220421-155132
        git checkout 8baa12e16d3e48bce0e6d86e8c7d0415439b7c98
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> ld.lld: error: undefined symbol: msg_tx_prepare
   >>> referenced by optee.c
   >>> firmware/arm_scmi/optee.o:(scmi_optee_send_message) in archive drivers/built-in.a
--
>> ld.lld: error: undefined symbol: msg_command_size
   >>> referenced by optee.c
   >>> firmware/arm_scmi/optee.o:(scmi_optee_send_message) in archive drivers/built-in.a
--
>> ld.lld: error: undefined symbol: msg_fetch_response
   >>> referenced by optee.c
   >>> firmware/arm_scmi/optee.o:(scmi_optee_fetch_response) in archive drivers/built-in.a

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

Re: [PATCH v2] firmware: arm_scmi: support optee shared memory in optee transport

[Cristian Marussi]

On Thu, Apr 21, 2022 at 09:49:35AM +0200, Etienne Carriere wrote:
> Adds support for tee shared memory in optee scmi transport. When using
> tee shared memory, scmi optee transport manages SCMI messages using
> msg protocol (from msg.c) in shared memory, whereas smt (from shmem.c)
> protocol is used with static IOMEM shared buffers.
> 
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
>

Hi Etienne,

the bot complaints on linking failure are due to a missing dependency:

diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
index 7794bd41eaa0..1e7b7fec97d9 100644
--- a/drivers/firmware/arm_scmi/Kconfig
+++ b/drivers/firmware/arm_scmi/Kconfig
@@ -59,6 +59,7 @@ config ARM_SCMI_TRANSPORT_OPTEE
        depends on OPTEE=y || OPTEE=ARM_SCMI_PROTOCOL
        select ARM_SCMI_HAVE_TRANSPORT
        select ARM_SCMI_HAVE_SHMEM
+       select ARM_SCMI_HAVE_MSG
        default y
        help
          This enables the OP-TEE service based transport for SCMI.

needed when you compile without any other transport using MSGs.

One remark down below.

>  drivers/firmware/arm_scmi/optee.c | 128 ++++++++++++++++++++++++------
>  1 file changed, 104 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/optee.c b/drivers/firmware/arm_scmi/optee.c
> index 8302a2b4aeeb..6676b17d2587 100644
> --- a/drivers/firmware/arm_scmi/optee.c
> +++ b/drivers/firmware/arm_scmi/optee.c
> @@ -64,6 +64,22 @@ enum scmi_optee_pta_cmd {
>  	 * [in]     value[0].b: Requested capabilities mask (enum pta_scmi_caps)
>  	 */
>  	PTA_SCMI_CMD_GET_CHANNEL = 3,
> +
> +	/*
> +	 * PTA_SCMI_CMD_PROCESS_MSG_CHANNEL - Process SCMI message in a MSG
> +	 * buffer pointed by memref parameters
> +	 *
> +	 * [in]     value[0].a: Channel handle
> +	 * [in]     memref[1]: Message buffer (MSG and SCMI payload)
> +	 * [out]    memref[2]: Response buffer (MSG and SCMI payload)
> +	 *
> +	 * Shared memories used for SCMI message/response are MSG buffers
> +	 * referenced by param[1] and param[2]. MSG transport protocol
> +	 * uses a 32bit header to carry SCMI meta-data (protocol ID and
> +	 * protocol message ID) followed by the effective SCMI message
> +	 * payload.
> +	 */
> +	PTA_SCMI_CMD_PROCESS_MSG_CHANNEL = 4,
>  };
>  
>  /*
> @@ -72,9 +88,17 @@ enum scmi_optee_pta_cmd {
>   * PTA_SCMI_CAPS_SMT_HEADER
>   * When set, OP-TEE supports command using SMT header protocol (SCMI shmem) in
>   * shared memory buffers to carry SCMI protocol synchronisation information.
> + *
> + * PTA_SCMI_CAPS_MSG_HEADER
> + * When set, OP-TEE supports command using MSG header protocol in an OP-TEE
> + * shared memory to carry SCMI protocol synchronisation information and SCMI
> + * message payload.
>   */
>  #define PTA_SCMI_CAPS_NONE		0
>  #define PTA_SCMI_CAPS_SMT_HEADER	BIT(0)
> +#define PTA_SCMI_CAPS_MSG_HEADER	BIT(1)
> +#define PTA_SCMI_CAPS_MASK		(PTA_SCMI_CAPS_SMT_HEADER | \
> +					 PTA_SCMI_CAPS_MSG_HEADER)
>  
>  /**
>   * struct scmi_optee_channel - Description of an OP-TEE SCMI channel
> @@ -85,7 +109,8 @@ enum scmi_optee_pta_cmd {
>   * @mu: Mutex protection on channel access
>   * @cinfo: SCMI channel information
>   * @shmem: Virtual base address of the shared memory
> - * @tee_shm: Reference to TEE shared memory or NULL if using static shmem
> + * @req: Shared memory protocol handle for SCMI request and synchronous response
> + * @tee_shm: TEE shared memory handle @req or NULL if using IOMEM shmem
>   * @link: Reference in agent's channel list
>   */
>  struct scmi_optee_channel {
> @@ -94,7 +119,10 @@ struct scmi_optee_channel {
>  	u32 caps;
>  	struct mutex mu;
>  	struct scmi_chan_info *cinfo;
> -	struct scmi_shared_mem __iomem *shmem;
> +	union {
> +		struct scmi_shared_mem __iomem *shmem;
> +		struct scmi_msg_payld *msg;
> +	} req;
>  	struct tee_shm *tee_shm;
>  	struct list_head link;
>  };
> @@ -178,8 +206,8 @@ static int get_capabilities(struct scmi_optee_agent *agent)
>  
>  	caps = param[0].u.value.a;
>  
> -	if (!(caps & PTA_SCMI_CAPS_SMT_HEADER)) {
> -		dev_err(agent->dev, "OP-TEE SCMI PTA doesn't support SMT\n");
> +	if (!(caps & (PTA_SCMI_CAPS_SMT_HEADER | PTA_SCMI_CAPS_MSG_HEADER))) {
> +		dev_err(agent->dev, "OP-TEE SCMI PTA doesn't support SMT and MSG\n");
>  		return -EOPNOTSUPP;
>  	}
>  
> @@ -193,9 +221,14 @@ static int get_channel(struct scmi_optee_channel *channel)
>  	struct device *dev = scmi_optee_private->dev;
>  	struct tee_ioctl_invoke_arg arg = { };
>  	struct tee_param param[1] = { };
> -	unsigned int caps = PTA_SCMI_CAPS_SMT_HEADER;
> +	unsigned int caps = 0;
>  	int ret;
>  
> +	if (channel->tee_shm)
> +		caps = PTA_SCMI_CAPS_MSG_HEADER;
> +	else
> +		caps = PTA_SCMI_CAPS_SMT_HEADER;
> +
>  	arg.func = PTA_SCMI_CMD_GET_CHANNEL;
>  	arg.session = channel->tee_session;
>  	arg.num_params = 1;
> @@ -249,6 +282,37 @@ static int invoke_process_smt_channel(struct scmi_optee_channel *channel)
>  	return 0;
>  }
>  

Looking at the invoke_process_smt_channel() code, I see that it still refers to
->tee_shm in an if condition when you send a PTA_SCMI_CMD_PROCESS_SMT_CHANNEL_MESSAGE
but that channel->tee_shm is always NULL when calling invoke_process_smt_channel()
since tee_shm is only allocated by non-SMT transport via setup_dynamic_shmem(),
so when this if-branch inside invoke_process_smt_channel() is supposed
to be used ? or is it a leftover ? (or I am missing something :D)


> +static int invoke_process_msg_channel(struct scmi_optee_channel *channel, size_t msg_size)
> +{
> +	struct tee_ioctl_invoke_arg arg = {
> +		.func = PTA_SCMI_CMD_PROCESS_MSG_CHANNEL,
> +		.session = channel->tee_session,
> +		.num_params = 3,
> +	};
> +	struct tee_param param[3] = { };
> +	int ret;
> +
> +	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
> +	param[0].u.value.a = channel->channel_id;
> +
> +	param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT;
> +	param[1].u.memref.shm = channel->tee_shm;
> +	param[1].u.memref.size = msg_size;
> +
> +	param[2].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> +	param[2].u.memref.shm = channel->tee_shm;
> +	param[2].u.memref.size = SCMI_OPTEE_MAX_MSG_SIZE;
> +
> +	ret = tee_client_invoke_func(scmi_optee_private->tee_ctx, &arg, param);
> +	if (ret < 0 || arg.ret) {
> +		dev_err(scmi_optee_private->dev, "Can't invoke channel %u: %d / %#x\n",
> +			channel->channel_id, ret, arg.ret);
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +

Thanks,
Cristian


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

Re: [PATCH v2] firmware: arm_scmi: support optee shared memory in optee transport

[Etienne Carriere]

Hello Cristian,

On Fri, 22 Apr 2022 at 14:57, Cristian Marussi <cristian.marussi@arm.com> wrote:
>
> On Thu, Apr 21, 2022 at 09:49:35AM +0200, Etienne Carriere wrote:
> > Adds support for tee shared memory in optee scmi transport. When using
> > tee shared memory, scmi optee transport manages SCMI messages using
> > msg protocol (from msg.c) in shared memory, whereas smt (from shmem.c)
> > protocol is used with static IOMEM shared buffers.
> >
> > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> >
>
> Hi Etienne,
>
> the bot complaints on linking failure are due to a missing dependency:
>
> diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
> index 7794bd41eaa0..1e7b7fec97d9 100644
> --- a/drivers/firmware/arm_scmi/Kconfig
> +++ b/drivers/firmware/arm_scmi/Kconfig
> @@ -59,6 +59,7 @@ config ARM_SCMI_TRANSPORT_OPTEE
>         depends on OPTEE=y || OPTEE=ARM_SCMI_PROTOCOL
>         select ARM_SCMI_HAVE_TRANSPORT
>         select ARM_SCMI_HAVE_SHMEM
> +       select ARM_SCMI_HAVE_MSG
>         default y
>         help
>           This enables the OP-TEE service based transport for SCMI.
>
> needed when you compile without any other transport using MSGs.

Thanks! I could not find why the bot failed to build this. I was
focused on looking at the wrong place.

>
> One remark down below.
>
> >  drivers/firmware/arm_scmi/optee.c | 128 ++++++++++++++++++++++++------
> >  1 file changed, 104 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/firmware/arm_scmi/optee.c b/drivers/firmware/arm_scmi/optee.c
> > index 8302a2b4aeeb..6676b17d2587 100644
> > --- a/drivers/firmware/arm_scmi/optee.c
> > +++ b/drivers/firmware/arm_scmi/optee.c
> > @@ -64,6 +64,22 @@ enum scmi_optee_pta_cmd {
> >        * [in]     value[0].b: Requested capabilities mask (enum pta_scmi_caps)
> >        */
> >       PTA_SCMI_CMD_GET_CHANNEL = 3,
> > +
> > +     /*
> > +      * PTA_SCMI_CMD_PROCESS_MSG_CHANNEL - Process SCMI message in a MSG
> > +      * buffer pointed by memref parameters
> > +      *
> > +      * [in]     value[0].a: Channel handle
> > +      * [in]     memref[1]: Message buffer (MSG and SCMI payload)
> > +      * [out]    memref[2]: Response buffer (MSG and SCMI payload)
> > +      *
> > +      * Shared memories used for SCMI message/response are MSG buffers
> > +      * referenced by param[1] and param[2]. MSG transport protocol
> > +      * uses a 32bit header to carry SCMI meta-data (protocol ID and
> > +      * protocol message ID) followed by the effective SCMI message
> > +      * payload.
> > +      */
> > +     PTA_SCMI_CMD_PROCESS_MSG_CHANNEL = 4,
> >  };
> >
> >  /*
> > @@ -72,9 +88,17 @@ enum scmi_optee_pta_cmd {
> >   * PTA_SCMI_CAPS_SMT_HEADER
> >   * When set, OP-TEE supports command using SMT header protocol (SCMI shmem) in
> >   * shared memory buffers to carry SCMI protocol synchronisation information.
> > + *
> > + * PTA_SCMI_CAPS_MSG_HEADER
> > + * When set, OP-TEE supports command using MSG header protocol in an OP-TEE
> > + * shared memory to carry SCMI protocol synchronisation information and SCMI
> > + * message payload.
> >   */
> >  #define PTA_SCMI_CAPS_NONE           0
> >  #define PTA_SCMI_CAPS_SMT_HEADER     BIT(0)
> > +#define PTA_SCMI_CAPS_MSG_HEADER     BIT(1)
> > +#define PTA_SCMI_CAPS_MASK           (PTA_SCMI_CAPS_SMT_HEADER | \
> > +                                      PTA_SCMI_CAPS_MSG_HEADER)
> >
> >  /**
> >   * struct scmi_optee_channel - Description of an OP-TEE SCMI channel
> > @@ -85,7 +109,8 @@ enum scmi_optee_pta_cmd {
> >   * @mu: Mutex protection on channel access
> >   * @cinfo: SCMI channel information
> >   * @shmem: Virtual base address of the shared memory
> > - * @tee_shm: Reference to TEE shared memory or NULL if using static shmem
> > + * @req: Shared memory protocol handle for SCMI request and synchronous response
> > + * @tee_shm: TEE shared memory handle @req or NULL if using IOMEM shmem
> >   * @link: Reference in agent's channel list
> >   */
> >  struct scmi_optee_channel {
> > @@ -94,7 +119,10 @@ struct scmi_optee_channel {
> >       u32 caps;
> >       struct mutex mu;
> >       struct scmi_chan_info *cinfo;
> > -     struct scmi_shared_mem __iomem *shmem;
> > +     union {
> > +             struct scmi_shared_mem __iomem *shmem;
> > +             struct scmi_msg_payld *msg;
> > +     } req;
> >       struct tee_shm *tee_shm;
> >       struct list_head link;
> >  };
> > @@ -178,8 +206,8 @@ static int get_capabilities(struct scmi_optee_agent *agent)
> >
> >       caps = param[0].u.value.a;
> >
> > -     if (!(caps & PTA_SCMI_CAPS_SMT_HEADER)) {
> > -             dev_err(agent->dev, "OP-TEE SCMI PTA doesn't support SMT\n");
> > +     if (!(caps & (PTA_SCMI_CAPS_SMT_HEADER | PTA_SCMI_CAPS_MSG_HEADER))) {
> > +             dev_err(agent->dev, "OP-TEE SCMI PTA doesn't support SMT and MSG\n");
> >               return -EOPNOTSUPP;
> >       }
> >
> > @@ -193,9 +221,14 @@ static int get_channel(struct scmi_optee_channel *channel)
> >       struct device *dev = scmi_optee_private->dev;
> >       struct tee_ioctl_invoke_arg arg = { };
> >       struct tee_param param[1] = { };
> > -     unsigned int caps = PTA_SCMI_CAPS_SMT_HEADER;
> > +     unsigned int caps = 0;
> >       int ret;
> >
> > +     if (channel->tee_shm)
> > +             caps = PTA_SCMI_CAPS_MSG_HEADER;
> > +     else
> > +             caps = PTA_SCMI_CAPS_SMT_HEADER;
> > +
> >       arg.func = PTA_SCMI_CMD_GET_CHANNEL;
> >       arg.session = channel->tee_session;
> >       arg.num_params = 1;
> > @@ -249,6 +282,37 @@ static int invoke_process_smt_channel(struct scmi_optee_channel *channel)
> >       return 0;
> >  }
> >
>
> Looking at the invoke_process_smt_channel() code, I see that it still refers to
> ->tee_shm in an if condition when you send a PTA_SCMI_CMD_PROCESS_SMT_CHANNEL_MESSAGE
> but that channel->tee_shm is always NULL when calling invoke_process_smt_channel()
> since tee_shm is only allocated by non-SMT transport via setup_dynamic_shmem(),
> so when this if-branch inside invoke_process_smt_channel() is supposed
> to be used ? or is it a leftover ? (or I am missing something :D)

I caught it right, definitely a leftover. Thanks. I'll clean that.

Br,
Etienne

>
>
> > +static int invoke_process_msg_channel(struct scmi_optee_channel *channel, size_t msg_size)
> > +{
> > +     struct tee_ioctl_invoke_arg arg = {
> > +             .func = PTA_SCMI_CMD_PROCESS_MSG_CHANNEL,
> > +             .session = channel->tee_session,
> > +             .num_params = 3,
> > +     };
> > +     struct tee_param param[3] = { };
> > +     int ret;
> > +
> > +     param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
> > +     param[0].u.value.a = channel->channel_id;
> > +
> > +     param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT;
> > +     param[1].u.memref.shm = channel->tee_shm;
> > +     param[1].u.memref.size = msg_size;
> > +
> > +     param[2].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> > +     param[2].u.memref.shm = channel->tee_shm;
> > +     param[2].u.memref.size = SCMI_OPTEE_MAX_MSG_SIZE;
> > +
> > +     ret = tee_client_invoke_func(scmi_optee_private->tee_ctx, &arg, param);
> > +     if (ret < 0 || arg.ret) {
> > +             dev_err(scmi_optee_private->dev, "Can't invoke channel %u: %d / %#x\n",
> > +                     channel->channel_id, ret, arg.ret);
> > +             return -EIO;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
>
> Thanks,
> Cristian
>

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

Re: [PATCH v2] firmware: arm_scmi: support optee shared memory in optee transport

[Etienne Carriere]

On Fri, 22 Apr 2022 at 16:49, Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> Hello Cristian,
>
> On Fri, 22 Apr 2022 at 14:57, Cristian Marussi <cristian.marussi@arm.com> wrote:
> >
> > On Thu, Apr 21, 2022 at 09:49:35AM +0200, Etienne Carriere wrote:
> > > Adds support for tee shared memory in optee scmi transport. When using
> > > tee shared memory, scmi optee transport manages SCMI messages using
> > > msg protocol (from msg.c) in shared memory, whereas smt (from shmem.c)
> > > protocol is used with static IOMEM shared buffers.
> > >
> > > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > >
> >
> > Hi Etienne,
> >
> > the bot complaints on linking failure are due to a missing dependency:
> >
> > diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
> > index 7794bd41eaa0..1e7b7fec97d9 100644
> > --- a/drivers/firmware/arm_scmi/Kconfig
> > +++ b/drivers/firmware/arm_scmi/Kconfig
> > @@ -59,6 +59,7 @@ config ARM_SCMI_TRANSPORT_OPTEE
> >         depends on OPTEE=y || OPTEE=ARM_SCMI_PROTOCOL
> >         select ARM_SCMI_HAVE_TRANSPORT
> >         select ARM_SCMI_HAVE_SHMEM
> > +       select ARM_SCMI_HAVE_MSG
> >         default y
> >         help
> >           This enables the OP-TEE service based transport for SCMI.
> >
> > needed when you compile without any other transport using MSGs.
>
> Thanks! I could not find why the bot failed to build this. I was
> focused on looking at the wrong place.
>
> >
> > One remark down below.
> >
> > >  drivers/firmware/arm_scmi/optee.c | 128 ++++++++++++++++++++++++------
> > >  1 file changed, 104 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/drivers/firmware/arm_scmi/optee.c b/drivers/firmware/arm_scmi/optee.c
> > > index 8302a2b4aeeb..6676b17d2587 100644
> > > --- a/drivers/firmware/arm_scmi/optee.c
> > > +++ b/drivers/firmware/arm_scmi/optee.c
> > > @@ -64,6 +64,22 @@ enum scmi_optee_pta_cmd {
> > >        * [in]     value[0].b: Requested capabilities mask (enum pta_scmi_caps)
> > >        */
> > >       PTA_SCMI_CMD_GET_CHANNEL = 3,
> > > +
> > > +     /*
> > > +      * PTA_SCMI_CMD_PROCESS_MSG_CHANNEL - Process SCMI message in a MSG
> > > +      * buffer pointed by memref parameters
> > > +      *
> > > +      * [in]     value[0].a: Channel handle
> > > +      * [in]     memref[1]: Message buffer (MSG and SCMI payload)
> > > +      * [out]    memref[2]: Response buffer (MSG and SCMI payload)
> > > +      *
> > > +      * Shared memories used for SCMI message/response are MSG buffers
> > > +      * referenced by param[1] and param[2]. MSG transport protocol
> > > +      * uses a 32bit header to carry SCMI meta-data (protocol ID and
> > > +      * protocol message ID) followed by the effective SCMI message
> > > +      * payload.
> > > +      */
> > > +     PTA_SCMI_CMD_PROCESS_MSG_CHANNEL = 4,
> > >  };
> > >
> > >  /*
> > > @@ -72,9 +88,17 @@ enum scmi_optee_pta_cmd {
> > >   * PTA_SCMI_CAPS_SMT_HEADER
> > >   * When set, OP-TEE supports command using SMT header protocol (SCMI shmem) in
> > >   * shared memory buffers to carry SCMI protocol synchronisation information.
> > > + *
> > > + * PTA_SCMI_CAPS_MSG_HEADER
> > > + * When set, OP-TEE supports command using MSG header protocol in an OP-TEE
> > > + * shared memory to carry SCMI protocol synchronisation information and SCMI
> > > + * message payload.
> > >   */
> > >  #define PTA_SCMI_CAPS_NONE           0
> > >  #define PTA_SCMI_CAPS_SMT_HEADER     BIT(0)
> > > +#define PTA_SCMI_CAPS_MSG_HEADER     BIT(1)
> > > +#define PTA_SCMI_CAPS_MASK           (PTA_SCMI_CAPS_SMT_HEADER | \
> > > +                                      PTA_SCMI_CAPS_MSG_HEADER)
> > >
> > >  /**
> > >   * struct scmi_optee_channel - Description of an OP-TEE SCMI channel
> > > @@ -85,7 +109,8 @@ enum scmi_optee_pta_cmd {
> > >   * @mu: Mutex protection on channel access
> > >   * @cinfo: SCMI channel information
> > >   * @shmem: Virtual base address of the shared memory
> > > - * @tee_shm: Reference to TEE shared memory or NULL if using static shmem
> > > + * @req: Shared memory protocol handle for SCMI request and synchronous response
> > > + * @tee_shm: TEE shared memory handle @req or NULL if using IOMEM shmem
> > >   * @link: Reference in agent's channel list
> > >   */
> > >  struct scmi_optee_channel {
> > > @@ -94,7 +119,10 @@ struct scmi_optee_channel {
> > >       u32 caps;
> > >       struct mutex mu;
> > >       struct scmi_chan_info *cinfo;
> > > -     struct scmi_shared_mem __iomem *shmem;
> > > +     union {
> > > +             struct scmi_shared_mem __iomem *shmem;
> > > +             struct scmi_msg_payld *msg;
> > > +     } req;
> > >       struct tee_shm *tee_shm;
> > >       struct list_head link;
> > >  };
> > > @@ -178,8 +206,8 @@ static int get_capabilities(struct scmi_optee_agent *agent)
> > >
> > >       caps = param[0].u.value.a;
> > >
> > > -     if (!(caps & PTA_SCMI_CAPS_SMT_HEADER)) {
> > > -             dev_err(agent->dev, "OP-TEE SCMI PTA doesn't support SMT\n");
> > > +     if (!(caps & (PTA_SCMI_CAPS_SMT_HEADER | PTA_SCMI_CAPS_MSG_HEADER))) {
> > > +             dev_err(agent->dev, "OP-TEE SCMI PTA doesn't support SMT and MSG\n");
> > >               return -EOPNOTSUPP;
> > >       }
> > >
> > > @@ -193,9 +221,14 @@ static int get_channel(struct scmi_optee_channel *channel)
> > >       struct device *dev = scmi_optee_private->dev;
> > >       struct tee_ioctl_invoke_arg arg = { };
> > >       struct tee_param param[1] = { };
> > > -     unsigned int caps = PTA_SCMI_CAPS_SMT_HEADER;
> > > +     unsigned int caps = 0;
> > >       int ret;
> > >
> > > +     if (channel->tee_shm)
> > > +             caps = PTA_SCMI_CAPS_MSG_HEADER;
> > > +     else
> > > +             caps = PTA_SCMI_CAPS_SMT_HEADER;
> > > +
> > >       arg.func = PTA_SCMI_CMD_GET_CHANNEL;
> > >       arg.session = channel->tee_session;
> > >       arg.num_params = 1;
> > > @@ -249,6 +282,37 @@ static int invoke_process_smt_channel(struct scmi_optee_channel *channel)
> > >       return 0;
> > >  }
> > >
> >
> > Looking at the invoke_process_smt_channel() code, I see that it still refers to
> > ->tee_shm in an if condition when you send a PTA_SCMI_CMD_PROCESS_SMT_CHANNEL_MESSAGE
> > but that channel->tee_shm is always NULL when calling invoke_process_smt_channel()
> > since tee_shm is only allocated by non-SMT transport via setup_dynamic_shmem(),
> > so when this if-branch inside invoke_process_smt_channel() is supposed
> > to be used ? or is it a leftover ? (or I am missing something :D)
>
> I caught it right, definitely a leftover. Thanks. I'll clean that.

Typo,
**You** caught it right :)>

br,
etienne

>
> Br,
> Etienne
>
> >
> >
> > > +static int invoke_process_msg_channel(struct scmi_optee_channel *channel, size_t msg_size)
> > > +{
> > > +     struct tee_ioctl_invoke_arg arg = {
> > > +             .func = PTA_SCMI_CMD_PROCESS_MSG_CHANNEL,
> > > +             .session = channel->tee_session,
> > > +             .num_params = 3,
> > > +     };
> > > +     struct tee_param param[3] = { };
> > > +     int ret;
> > > +
> > > +     param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
> > > +     param[0].u.value.a = channel->channel_id;
> > > +
> > > +     param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT;
> > > +     param[1].u.memref.shm = channel->tee_shm;
> > > +     param[1].u.memref.size = msg_size;
> > > +
> > > +     param[2].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> > > +     param[2].u.memref.shm = channel->tee_shm;
> > > +     param[2].u.memref.size = SCMI_OPTEE_MAX_MSG_SIZE;
> > > +
> > > +     ret = tee_client_invoke_func(scmi_optee_private->tee_ctx, &arg, param);
> > > +     if (ret < 0 || arg.ret) {
> > > +             dev_err(scmi_optee_private->dev, "Can't invoke channel %u: %d / %#x\n",
> > > +                     channel->channel_id, ret, arg.ret);
> > > +             return -EIO;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +
> >
> > Thanks,
> > Cristian
> >

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