Date:   Thu, 9 Feb 2023 10:19:37 +0100
From:   Jens Wiklander <jens.wiklander@linaro.org>
To:     Etienne Carriere <etienne.carriere@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>
Subject: Re: [PATCH 1/2] tee: system invocation
Message-ID: <Y+S6qbgtAViopMPd@jade>
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>
 <CAN5uoS_M4uMWkf=Q8XFLCrNSvyUdjLgCPixqonKv3mRwRTr-nQ@mail.gmail.com>
 <Y+SdRrwSq/a9OgGr@jade>
 <CAN5uoS-7Mk6Cy9T9978-5hRUK55UOcCXPFe7Kv8ZUvkJZPi6pw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <CAN5uoS-7Mk6Cy9T9978-5hRUK55UOcCXPFe7Kv8ZUvkJZPi6pw@mail.gmail.com>
Precedence: bulk

Hi,

On Thu, Feb 09, 2023 at 09:11:53AM +0100, Etienne Carriere wrote:
> Hi Jens,
> 
> 
> On Thu, 9 Feb 2023 at 08:14, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > Hi Etienne,
> >
> > On Wed, Feb 08, 2023 at 06:09:17PM +0100, Etienne Carriere wrote:
> > > Hello Sumit, Jens,
> > >
> > [snip]
> > > > > > > >
> > > > > > > >         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.
> >
> > It's not obvious to me that this will easier to implement and maintain.
> > Looking at the code in optee_os it looks like using a flag bit as
> > proposed by Sumit would be quite easy to handle.
> 
> OP-TEE could auto disable thread provis when dyn shm is disabled, right.
> Will it be sufficient? We will still face cases where an OP-TEE
> provisions thread but Linux kernel is not aware (older vanilla kernel
> used with a recent OP-TEE OS). Not much platforms are really affected
> I guess but those executing with pager in small RAMs where a 4kB
> thread context costs.

When you add exceptions you make it more complicated. Now we must
remember to always use dyn shm if we are to succeed in configuring with
system threads. What if both dyn shm and static shm is configured in
OP-TEE, but the kernel only uses static 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?
> >
> > No, I'm not so keen on adding that side effect to
> > OPTEE_SMC_EXCHANGE_CAPABILITIES.
> 
> It is a capability REE would exchanges with TEE.
> What kind of side effects do you fear?

I was hoping to keep it stateless. One thing less to keep track of when
handing over from a boot stage to the kernel.

> > The way you're describing the problem it sounds like it's a normal world
> > problem to know how many system threads are needed. How about adding a
> > fast call where normal world can request how many system threads should
> > be reserved?  If none are requested, none will be reserved.
> 
> Well, could be. With caps exchange, we have an SMC funcID to REE to
> say to TEE: "reserved the default configured number of sys thread". I
> think it is simpler.

Until you realize the that the default number of system threads doesn't
match what you need.

> 
> With REE calling TEE to provision thread, we would need another call
> to release the reservation. Whe caps exchange, we have a single SMC to
> reconfig the negotiated caps.

A single SMC with growing complexity in its arguments.

Cheers,
Jens