Hello Cristian,
On Fri, 22 Apr 2022 at 14:57, Cristian Marussi <[email protected]> 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 <[email protected]>
> >
>
> 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
>
On Fri, 22 Apr 2022 at 16:49, Etienne Carriere
<[email protected]> wrote:
>
> Hello Cristian,
>
> On Fri, 22 Apr 2022 at 14:57, Cristian Marussi <[email protected]> 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 <[email protected]>
> > >
> >
> > 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
> >