From: Sumit Garg <sumit.garg@kernel.org>
To: Jens Wiklander <jens.wiklander@linaro.org>
Cc: linux-kernel@vger.kernel.org, linux-media@vger.kernel.org,
dri-devel@lists.freedesktop.org, linaro-mm-sig@lists.linaro.org,
op-tee@lists.trustedfirmware.org,
linux-arm-kernel@lists.infradead.org,
"Olivier Masse" <olivier.masse@nxp.com>,
"Thierry Reding" <thierry.reding@gmail.com>,
"Yong Wu" <yong.wu@mediatek.com>,
"Sumit Semwal" <sumit.semwal@linaro.org>,
"Benjamin Gaignard" <benjamin.gaignard@collabora.com>,
"Brian Starkey" <Brian.Starkey@arm.com>,
"John Stultz" <jstultz@google.com>,
"T . J . Mercier" <tjmercier@google.com>,
"Christian König" <christian.koenig@amd.com>,
"Matthias Brugger" <matthias.bgg@gmail.com>,
"AngeloGioacchino Del Regno"
<angelogioacchino.delregno@collabora.com>,
azarrabi@qti.qualcomm.com,
"Simona Vetter" <simona.vetter@ffwll.ch>,
"Daniel Stone" <daniel@fooishbar.org>
Subject: Re: [PATCH v6 03/10] optee: account for direction while converting parameters
Date: Tue, 25 Mar 2025 11:25:54 +0530 [thread overview]
Message-ID: <Z-JFagmbZBQd0hoM@sumit-X1> (raw)
In-Reply-To: <CAHUa44HY-jZCoDXKnx-f4gBoABRVdh1+6PA1AYHSva9aL=rDAA@mail.gmail.com>
On Thu, Mar 20, 2025 at 02:00:57PM +0100, Jens Wiklander wrote:
> Hi Sumit,
>
> On Thu, Mar 20, 2025 at 10:25 AM Sumit Garg <sumit.garg@kernel.org> wrote:
> >
> > Hi Jens,
> >
> > On Mon, Mar 17, 2025 at 08:42:01AM +0100, Jens Wiklander wrote:
> > > Hi Sumit,
> > >
> > > On Thu, Mar 13, 2025 at 11:41 AM Sumit Garg <sumit.garg@kernel.org> wrote:
> > > >
> > > > Hi Jens,
> > > >
> > > > On Wed, Mar 05, 2025 at 02:04:09PM +0100, Jens Wiklander wrote:
> > > > > The OP-TEE backend driver has two internal function pointers to convert
> > > > > between the subsystem type struct tee_param and the OP-TEE type struct
> > > > > optee_msg_param.
> > > > >
> > > > > The conversion is done from one of the types to the other, which is then
> > > > > involved in some operation and finally converted back to the original
> > > > > type. When converting to prepare the parameters for the operation, all
> > > > > fields must be taken into account, but then converting back, it's enough
> > > > > to update only out-values and out-sizes. So, an update_out parameter is
> > > > > added to the conversion functions to tell if all or only some fields
> > > > > must be copied.
> > > > >
> > > > > This is needed in a later patch where it might get confusing when
> > > > > converting back in from_msg_param() callback since an allocated
> > > > > restricted SHM can be using the sec_world_id of the used restricted
> > > > > memory pool and that doesn't translate back well.
> > > > >
> > > > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > > > ---
> > > > > drivers/tee/optee/call.c | 10 ++--
> > > > > drivers/tee/optee/ffa_abi.c | 43 +++++++++++++----
> > > > > drivers/tee/optee/optee_private.h | 42 +++++++++++------
> > > > > drivers/tee/optee/rpc.c | 31 +++++++++----
> > > > > drivers/tee/optee/smc_abi.c | 76 +++++++++++++++++++++++--------
> > > > > 5 files changed, 144 insertions(+), 58 deletions(-)
> > > > >
> > > > > diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
> > > > > index 16eb953e14bb..f1533b894726 100644
> > > > > --- a/drivers/tee/optee/call.c
> > > > > +++ b/drivers/tee/optee/call.c
> > > > > @@ -400,7 +400,8 @@ int optee_open_session(struct tee_context *ctx,
> > > > > export_uuid(msg_arg->params[1].u.octets, &client_uuid);
> > > > >
> > > > > rc = optee->ops->to_msg_param(optee, msg_arg->params + 2,
> > > > > - arg->num_params, param);
> > > > > + arg->num_params, param,
> > > > > + false /*!update_out*/);
> > > > > if (rc)
> > > > > goto out;
> > > > >
> > > > > @@ -427,7 +428,8 @@ int optee_open_session(struct tee_context *ctx,
> > > > > }
> > > > >
> > > > > if (optee->ops->from_msg_param(optee, param, arg->num_params,
> > > > > - msg_arg->params + 2)) {
> > > > > + msg_arg->params + 2,
> > > > > + true /*update_out*/)) {
> > > > > arg->ret = TEEC_ERROR_COMMUNICATION;
> > > > > arg->ret_origin = TEEC_ORIGIN_COMMS;
> > > > > /* Close session again to avoid leakage */
> > > > > @@ -541,7 +543,7 @@ int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg,
> > > > > msg_arg->cancel_id = arg->cancel_id;
> > > > >
> > > > > rc = optee->ops->to_msg_param(optee, msg_arg->params, arg->num_params,
> > > > > - param);
> > > > > + param, false /*!update_out*/);
> > > > > if (rc)
> > > > > goto out;
> > > > >
> > > > > @@ -551,7 +553,7 @@ int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg,
> > > > > }
> > > > >
> > > > > if (optee->ops->from_msg_param(optee, param, arg->num_params,
> > > > > - msg_arg->params)) {
> > > > > + msg_arg->params, true /*update_out*/)) {
> > > > > msg_arg->ret = TEEC_ERROR_COMMUNICATION;
> > > > > msg_arg->ret_origin = TEEC_ORIGIN_COMMS;
> > > > > }
> > > > > diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
> > > > > index 4ca1d5161b82..e4b08cd195f3 100644
> > > > > --- a/drivers/tee/optee/ffa_abi.c
> > > > > +++ b/drivers/tee/optee/ffa_abi.c
> > > > > @@ -122,15 +122,21 @@ static int optee_shm_rem_ffa_handle(struct optee *optee, u64 global_id)
> > > > > */
> > > > >
> > > > > static void from_msg_param_ffa_mem(struct optee *optee, struct tee_param *p,
> > > > > - u32 attr, const struct optee_msg_param *mp)
> > > > > + u32 attr, const struct optee_msg_param *mp,
> > > > > + bool update_out)
> > > > > {
> > > > > struct tee_shm *shm = NULL;
> > > > > u64 offs_high = 0;
> > > > > u64 offs_low = 0;
> > > > >
> > > > > + if (update_out) {
> > > > > + if (attr == OPTEE_MSG_ATTR_TYPE_FMEM_INPUT)
> > > > > + return;
> > > > > + goto out;
> > > > > + }
> > > > > +
> > > > > p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT +
> > > > > attr - OPTEE_MSG_ATTR_TYPE_FMEM_INPUT;
> > > > > - p->u.memref.size = mp->u.fmem.size;
> > > > >
> > > > > if (mp->u.fmem.global_id != OPTEE_MSG_FMEM_INVALID_GLOBAL_ID)
> > > > > shm = optee_shm_from_ffa_handle(optee, mp->u.fmem.global_id);
> > > > > @@ -141,6 +147,8 @@ static void from_msg_param_ffa_mem(struct optee *optee, struct tee_param *p,
> > > > > offs_high = mp->u.fmem.offs_high;
> > > > > }
> > > > > p->u.memref.shm_offs = offs_low | offs_high << 32;
> > > > > +out:
> > > > > + p->u.memref.size = mp->u.fmem.size;
> > > > > }
> > > > >
> > > > > /**
> > > > > @@ -150,12 +158,14 @@ static void from_msg_param_ffa_mem(struct optee *optee, struct tee_param *p,
> > > > > * @params: subsystem internal parameter representation
> > > > > * @num_params: number of elements in the parameter arrays
> > > > > * @msg_params: OPTEE_MSG parameters
> > > > > + * @update_out: update parameter for output only
> > > > > *
> > > > > * Returns 0 on success or <0 on failure
> > > > > */
> > > > > static int optee_ffa_from_msg_param(struct optee *optee,
> > > > > struct tee_param *params, size_t num_params,
> > > > > - const struct optee_msg_param *msg_params)
> > > > > + const struct optee_msg_param *msg_params,
> > > > > + bool update_out)
> > > > > {
> > > > > size_t n;
> > > > >
> > > > > @@ -166,18 +176,20 @@ static int optee_ffa_from_msg_param(struct optee *optee,
> > > > >
> > > > > switch (attr) {
> > > > > case OPTEE_MSG_ATTR_TYPE_NONE:
> > > > > + if (update_out)
> > > > > + break;
> > > > > p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_NONE;
> > > > > memset(&p->u, 0, sizeof(p->u));
> > > > > break;
> > > > > case OPTEE_MSG_ATTR_TYPE_VALUE_INPUT:
> > > > > case OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT:
> > > > > case OPTEE_MSG_ATTR_TYPE_VALUE_INOUT:
> > > > > - optee_from_msg_param_value(p, attr, mp);
> > > > > + optee_from_msg_param_value(p, attr, mp, update_out);
> > > > > break;
> > > > > case OPTEE_MSG_ATTR_TYPE_FMEM_INPUT:
> > > > > case OPTEE_MSG_ATTR_TYPE_FMEM_OUTPUT:
> > > > > case OPTEE_MSG_ATTR_TYPE_FMEM_INOUT:
> > > > > - from_msg_param_ffa_mem(optee, p, attr, mp);
> > > > > + from_msg_param_ffa_mem(optee, p, attr, mp, update_out);
> > > > > break;
> > > > > default:
> > > > > return -EINVAL;
> > > > > @@ -188,10 +200,16 @@ static int optee_ffa_from_msg_param(struct optee *optee,
> > > > > }
> > > > >
> > > > > static int to_msg_param_ffa_mem(struct optee_msg_param *mp,
> > > > > - const struct tee_param *p)
> > > > > + const struct tee_param *p, bool update_out)
> > > > > {
> > > > > struct tee_shm *shm = p->u.memref.shm;
> > > > >
> > > > > + if (update_out) {
> > > > > + if (p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT)
> > > > > + return 0;
> > > > > + goto out;
> > > > > + }
> > > > > +
> > > > > mp->attr = OPTEE_MSG_ATTR_TYPE_FMEM_INPUT + p->attr -
> > > > > TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT;
> > > > >
> > > > > @@ -211,6 +229,7 @@ static int to_msg_param_ffa_mem(struct optee_msg_param *mp,
> > > > > memset(&mp->u, 0, sizeof(mp->u));
> > > > > mp->u.fmem.global_id = OPTEE_MSG_FMEM_INVALID_GLOBAL_ID;
> > > > > }
> > > > > +out:
> > > > > mp->u.fmem.size = p->u.memref.size;
> > > > >
> > > > > return 0;
> > > > > @@ -222,13 +241,15 @@ static int to_msg_param_ffa_mem(struct optee_msg_param *mp,
> > > > > * @optee: main service struct
> > > > > * @msg_params: OPTEE_MSG parameters
> > > > > * @num_params: number of elements in the parameter arrays
> > > > > - * @params: subsystem itnernal parameter representation
> > > > > + * @params: subsystem internal parameter representation
> > > > > + * @update_out: update parameter for output only
> > > > > * Returns 0 on success or <0 on failure
> > > > > */
> > > > > static int optee_ffa_to_msg_param(struct optee *optee,
> > > > > struct optee_msg_param *msg_params,
> > > > > size_t num_params,
> > > > > - const struct tee_param *params)
> > > > > + const struct tee_param *params,
> > > > > + bool update_out)
> > > > > {
> > > > > size_t n;
> > > > >
> > > > > @@ -238,18 +259,20 @@ static int optee_ffa_to_msg_param(struct optee *optee,
> > > > >
> > > > > switch (p->attr) {
> > > > > case TEE_IOCTL_PARAM_ATTR_TYPE_NONE:
> > > > > + if (update_out)
> > > > > + break;
> > > > > mp->attr = TEE_IOCTL_PARAM_ATTR_TYPE_NONE;
> > > > > memset(&mp->u, 0, sizeof(mp->u));
> > > > > break;
> > > > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT:
> > > > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT:
> > > > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT:
> > > > > - optee_to_msg_param_value(mp, p);
> > > > > + optee_to_msg_param_value(mp, p, update_out);
> > > > > break;
> > > > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT:
> > > > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT:
> > > > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT:
> > > > > - if (to_msg_param_ffa_mem(mp, p))
> > > > > + if (to_msg_param_ffa_mem(mp, p, update_out))
> > > > > return -EINVAL;
> > > > > break;
> > > > > default:
> > > >
> > > > Can we rather handle it as follows to improve code readability and
> > > > maintainence long term? Ditto for all other places.
> > > >
> > > > static int optee_ffa_to_msg_param(struct optee *optee,
> > > > struct optee_msg_param *msg_params,
> > > > size_t num_params,
> > > > const struct tee_param *params,
> > > > bool update_out)
> > > > {
> > > > size_t n;
> > > >
> > > > for (n = 0; n < num_params; n++) {
> > > > const struct tee_param *p = params + n;
> > > > struct optee_msg_param *mp = msg_params + n;
> > > >
> > > > if (update_out && (p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_NONE ||
> > > > p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT ||
> > > > p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT))
> > > > continue;
> > >
> > > You're missing updating the length field for memrefs.
> > >
> >
> > Do we need to update length field for input memrefs when update_out is
> > set? I don't see that happening in your existing patch too.
>
> I'm sorry, I was unclear. The update_out parameter means only the
> output fields should be updated, not the attribute, offsets, ids, etc.
> That is, the length field for memrefs, and the value fields a, b, c
> for value params. Some of the memrefs aren't translated one-to-one
> with SDP, but the length field can and must be updated.
Isn't it rather better to add another attribute type to handled SDP
special handling?
-Sumit
>
> Cheers,
> Jens
>
> >
> > -Sumit
> >
> > > Cheers,
> > > Jens
> > >
> > > >
> > > > switch (p->attr) {
> > > > case TEE_IOCTL_PARAM_ATTR_TYPE_NONE:
> > > > mp->attr = TEE_IOCTL_PARAM_ATTR_TYPE_NONE;
> > > > memset(&mp->u, 0, sizeof(mp->u));
> > > > break;
> > > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT:
> > > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT:
> > > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT:
> > > > optee_to_msg_param_value(mp, p);
> > > > break;
> > > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT:
> > > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT:
> > > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT:
> > > > if (to_msg_param_ffa_mem(mp, p))
> > > > return -EINVAL;
> > > > break;
> > > > default:
> > > > return -EINVAL;
> > > > }
> > > > }
> > > >
> > > > return 0;
> > > > }
> > > >
> > > > -Sumit
next prev parent reply other threads:[~2025-03-25 5:58 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-03-05 13:04 [PATCH v6 00/10] TEE subsystem for restricted dma-buf allocations Jens Wiklander
2025-03-05 13:04 ` [PATCH v6 01/10] tee: tee_device_alloc(): copy dma_mask from parent device Jens Wiklander
2025-03-10 8:56 ` Sumit Garg
2025-03-05 13:04 ` [PATCH v6 02/10] optee: pass parent device to tee_device_alloc() Jens Wiklander
2025-03-10 8:57 ` Sumit Garg
2025-03-05 13:04 ` [PATCH v6 03/10] optee: account for direction while converting parameters Jens Wiklander
2025-03-13 10:41 ` Sumit Garg
2025-03-17 7:42 ` Jens Wiklander
2025-03-20 9:25 ` Sumit Garg
2025-03-20 13:00 ` Jens Wiklander
2025-03-25 5:55 ` Sumit Garg [this message]
2025-03-25 8:50 ` Jens Wiklander
2025-04-01 7:45 ` Sumit Garg
2025-04-01 8:21 ` Jens Wiklander
2025-03-05 13:04 ` [PATCH v6 04/10] optee: sync secure world ABI headers Jens Wiklander
2025-03-25 6:20 ` Sumit Garg
2025-03-27 7:41 ` Jens Wiklander
2025-03-05 13:04 ` [PATCH v6 05/10] tee: implement restricted DMA-heap Jens Wiklander
2025-03-25 6:33 ` Sumit Garg
2025-03-25 10:55 ` Jens Wiklander
2025-04-01 7:58 ` Sumit Garg
2025-04-01 8:33 ` Jens Wiklander
2025-04-08 9:14 ` Sumit Garg
2025-04-08 13:28 ` Jens Wiklander
2025-04-09 12:50 ` Sumit Garg
2025-04-10 6:49 ` Jens Wiklander
2025-03-05 13:04 ` [PATCH v6 06/10] tee: new ioctl to a register tee_shm from a dmabuf file descriptor Jens Wiklander
2025-03-25 6:50 ` Sumit Garg
2025-03-25 11:17 ` Jens Wiklander
2025-04-01 8:46 ` Sumit Garg
2025-04-01 13:50 ` Jens Wiklander
2025-03-05 13:04 ` [PATCH v6 07/10] tee: add tee_shm_alloc_cma_phys_mem() Jens Wiklander
2025-03-25 6:53 ` Sumit Garg
2025-03-05 13:04 ` [PATCH v6 08/10] optee: support restricted memory allocation Jens Wiklander
2025-03-25 7:07 ` Sumit Garg
2025-03-25 13:55 ` Jens Wiklander
2025-03-05 13:04 ` [PATCH v6 09/10] optee: FF-A: dynamic " Jens Wiklander
2025-03-25 7:41 ` Sumit Garg
2025-03-27 8:07 ` Jens Wiklander
2025-04-01 10:13 ` Sumit Garg
2025-04-01 12:26 ` Jens Wiklander
2025-04-08 9:20 ` Sumit Garg
2025-04-08 13:39 ` Jens Wiklander
2025-04-09 10:01 ` David Hildenbrand
2025-04-09 13:19 ` Sumit Garg
2025-03-05 13:04 ` [PATCH v6 10/10] optee: smc abi: " Jens Wiklander
2025-03-25 7:45 ` Sumit Garg
2025-03-27 8:27 ` [PATCH v6 00/10] TEE subsystem for restricted dma-buf allocations Jens Wiklander
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Z-JFagmbZBQd0hoM@sumit-X1 \
--to=sumit.garg@kernel.org \
--cc=Brian.Starkey@arm.com \
--cc=angelogioacchino.delregno@collabora.com \
--cc=azarrabi@qti.qualcomm.com \
--cc=benjamin.gaignard@collabora.com \
--cc=christian.koenig@amd.com \
--cc=daniel@fooishbar.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=jens.wiklander@linaro.org \
--cc=jstultz@google.com \
--cc=linaro-mm-sig@lists.linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=matthias.bgg@gmail.com \
--cc=olivier.masse@nxp.com \
--cc=op-tee@lists.trustedfirmware.org \
--cc=simona.vetter@ffwll.ch \
--cc=sumit.semwal@linaro.org \
--cc=thierry.reding@gmail.com \
--cc=tjmercier@google.com \
--cc=yong.wu@mediatek.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).