MIME-Version: 1.0
References: <20230130094157.1082712-1-etienne.carriere@linaro.org>
 <CAFA6WYPxzB45pgWkkh++jRaY-d0eHWnrjzu+Z9059PjK5=M-mQ@mail.gmail.com>
 <CAHUa44G3jCqyiXtdZAAEw=8WPC+m5fD8tqsPfGc3MkV3JjRDCA@mail.gmail.com>
 <CAFA6WYN7i+7riJGPH4BEUFK77-kAx4J89Tn3=oX=g6rFUFtDkQ@mail.gmail.com> <CAHUa44G1yQtgF1eAUJVA+wtctKHfqYFBhs0PBnpoN-gD8_x8eg@mail.gmail.com>
In-Reply-To: <CAHUa44G1yQtgF1eAUJVA+wtctKHfqYFBhs0PBnpoN-gD8_x8eg@mail.gmail.com>
From:   Etienne Carriere <etienne.carriere@linaro.org>
Date:   Wed, 8 Feb 2023 18:09:17 +0100
Message-ID: <CAN5uoS_M4uMWkf=Q8XFLCrNSvyUdjLgCPixqonKv3mRwRTr-nQ@mail.gmail.com>
Subject: Re: [PATCH 1/2] tee: system invocation
To:     Jens Wiklander <jens.wiklander@linaro.org>
Cc:     Sumit Garg <sumit.garg@linaro.org>, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        Sudeep Holla <sudeep.holla@arm.com>,
        Cristian Marussi <cristian.marussi@arm.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hello Sumit, Jens,

On Tue, 7 Feb 2023 at 11:36, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> On Tue, Feb 7, 2023 at 10:52 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > On Tue, 7 Feb 2023 at 14:38, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > >
> > > Hi,
> > >
> > > On Tue, Feb 7, 2023 at 8:27 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> > > >
> > > > Hi Etienne,
> > > >
> > > > On Mon, 30 Jan 2023 at 15:12, Etienne Carriere
> > > > <etienne.carriere@linaro.org> wrote:
> > > > >
> > > > > Adds TEE context flag sys_service to be enabled for invocation contexts
> > > > > that should used TEE provisioned system resources. OP-TEE SMC ABI entry
> > > >
> > > > s/used/use/
> > > >
> > > > > rely this information to use a dedicated entry function to request
> > > > > allocation of a system thread from a dedicated system context pool.
> > > > >
> > > > > This feature is needed when a TEE invocation cannot afford to wait for
> > > > > a free TEE thread when all TEE threads context are used and suspended
> > > > > as these may be suspended waiting for a system service, as an SCMI clock
> > > > > or voltage regulator, to be enabled. An example is when OP-TEE invokes
> > > > > a Linux OS remove service (RPC) to access an eMMC RPMB partition and
> > > >
> > > > s/remove/remote/
> > > >
> > > > > the eMMC device is supplied by an OP-TEE SCMI regulator.
> > > > >
> > > > > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > > > > ---
> > > > >  drivers/tee/optee/optee_smc.h | 14 +++++++++++---
> > > > >  drivers/tee/optee/smc_abi.c   |  6 +++++-
> > > > >  include/linux/tee_drv.h       |  4 ++++
> > > > >  3 files changed, 20 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/tee/optee/optee_smc.h b/drivers/tee/optee/optee_smc.h
> > > > > index 73b5e7760d10..7c7eedf183c5 100644
> > > > > --- a/drivers/tee/optee/optee_smc.h
> > > > > +++ b/drivers/tee/optee/optee_smc.h
> > > > > @@ -108,7 +108,8 @@ struct optee_smc_call_get_os_revision_result {
> > > > >   * Call with struct optee_msg_arg as argument
> > > > >   *
> > > > >   * When called with OPTEE_SMC_CALL_WITH_RPC_ARG or
> > > > > - * OPTEE_SMC_CALL_WITH_REGD_ARG in a0 there is one RPC struct optee_msg_arg
> > > > > + * OPTEE_SMC_CALL_WITH_REGD_ARG or OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG
> > > > > + * in a0 there is one RPC struct optee_msg_arg
> > > > >   * following after the first struct optee_msg_arg. The RPC struct
> > > > >   * optee_msg_arg has reserved space for the number of RPC parameters as
> > > > >   * returned by OPTEE_SMC_EXCHANGE_CAPABILITIES.
> > > > > @@ -130,8 +131,8 @@ struct optee_smc_call_get_os_revision_result {
> > > > >   * a4-6        Not used
> > > > >   * a7  Hypervisor Client ID register
> > > > >   *
> > > > > - * Call register usage, OPTEE_SMC_CALL_WITH_REGD_ARG:
> > > > > - * a0  SMC Function ID, OPTEE_SMC_CALL_WITH_REGD_ARG
> > > > > + * Call register usage, OPTEE_SMC_CALL_WITH_REGD_ARG and OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG:
> > > > > + * a0  SMC Function ID, OPTEE_SMC_CALL_WITH_REGD_ARG or OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG
> > > > >   * a1  Upper 32 bits of a 64-bit shared memory cookie
> > > > >   * a2  Lower 32 bits of a 64-bit shared memory cookie
> > > > >   * a3  Offset of the struct optee_msg_arg in the shared memory with the
> > > > > @@ -175,6 +176,8 @@ struct optee_smc_call_get_os_revision_result {
> > > > >         OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG)
> > > > >  #define OPTEE_SMC_CALL_WITH_REGD_ARG \
> > > > >         OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG)
> > > > > +#define OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG \
> > > > > +       OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG)
> > > > >
> > > > >  /*
> > > > >   * Get Shared Memory Config
> > > > > @@ -254,6 +257,8 @@ struct optee_smc_get_shm_config_result {
> > > > >  #define OPTEE_SMC_SEC_CAP_ASYNC_NOTIF          BIT(5)
> > > > >  /* Secure world supports pre-allocating RPC arg struct */
> > > > >  #define OPTEE_SMC_SEC_CAP_RPC_ARG              BIT(6)
> > > > > +/* Secure world provisions thread for system service invocation */
> > > > > +#define OPTEE_SMC_SEC_CAP_SYSTEM_THREAD                BIT(7)
> > > > >
> > > > >  #define OPTEE_SMC_FUNCID_EXCHANGE_CAPABILITIES 9
> > > > >  #define OPTEE_SMC_EXCHANGE_CAPABILITIES \
> > > > > @@ -426,6 +431,9 @@ struct optee_smc_disable_shm_cache_result {
> > > > >  /* See OPTEE_SMC_CALL_WITH_REGD_ARG above */
> > > > >  #define OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG    19
> > > > >
> > > > > +/* See OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG above */
> > > > > +#define OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG     20
> > > > > +
> > > > >  /*
> > > > >   * Resume from RPC (for example after processing a foreign interrupt)
> > > > >   *
> > > > > diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
> > > > > index a1c1fa1a9c28..513038a138f6 100644
> > > > > --- a/drivers/tee/optee/smc_abi.c
> > > > > +++ b/drivers/tee/optee/smc_abi.c
> > > > > @@ -889,7 +889,11 @@ static int optee_smc_do_call_with_arg(struct tee_context *ctx,
> > > > >         }
> > > > >
> > > > >         if  (rpc_arg && tee_shm_is_dynamic(shm)) {
> > > > > -               param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> > > > > +               if (ctx->sys_service &&
> > > > > +                   (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_SYSTEM_THREAD))
> > > > > +                       param.a0 = OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG;
> > > > > +               else
> > > > > +                       param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> > > >
> > > > This system thread flag should also be applicable to platforms without
> > > > registered arguments support. IOW, we need similar equivalents for
> > > > OPTEE_SMC_FUNCID_CALL_WITH_ARG and OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG
> > > > too. So I would rather suggest that we add following flag to all 3
> > > > call types:
> > > >
> > > > #define OPTEE_SMC_CALL_SYSTEM_THREAD_FLAG    0x8000
> > >
> > > The main reason platforms don't support registered arguments is that
> > > they haven't been updated since this was introduced. So if a platform
> > > needs system threads it could update to use registered arguments too.
> >
> > Are we hinting at deprecating reserved shared memory support? If yes,
> > wouldn't it be better to be explicit about it with a boot time warning
> > message about its deprecation?
> >
> > Otherwise it will be difficult to debug for the end user to find out
> > why system thread support isn't activated.
> >
> > > The Linux kernel already supports registered arguments. An advantage
> > > with the current approach is that the ABI is easier to implement
> > > since we have distinct SMC IDs for each function.
> >
> > I see your point but my initial thought was that we don't end up
> > making that list too large that it becomes cumbersome to maintain,
> > involving all the combinatorial.
>
> You have a point. Etienne, do you think we could give it a try at
> https://github.com/OP-TEE/optee_os/pull/5789 to better see how this
> would play out?
>

Indeed I miss that...
With the patch proposed here, indeed if OP-TEE does not support
dynamic shared memory then Linux will never use the provisioned TEE
thread. This is weird as in such a case OP-TEE provisions resources
that will never be used, which is the exact opposite goal of this
feature. Verified on our qemu-arm setup.

For simplicity, I think this system invocation should require OP-TEE
supports dyn shm.

If OP-TEE could know when Linux does not support TEE system
invocation, then OP-TEE could let any invocation use these provisioned
resources so that they are not wasted.
I think a good way would be Linux to expose if it supports this
capability, during capabilities exchange.
Would you agree with this approach?

Etienne


> Cheers,
> Jens