mbox series

[v2,0/7] Asynchronous notifications from secure world

Message ID 20210616103649.2662395-1-jens.wiklander@linaro.org (mailing list archive)
Headers show
Series Asynchronous notifications from secure world | expand

Message

Jens Wiklander June 16, 2021, 10:36 a.m. UTC
Hi all,

This adds support for asynchronous notifications from OP-TEE in secure
world to the OP-TEE driver. This allows a design with a top half and bottom
half type of driver where the top half runs in secure interrupt context and
a notifications tells normal world to schedule a yielding call to do the
bottom half processing.

An interrupt is used to notify the driver that there are asynchronous
notifications pending.

v2:
* Added documentation
* Converted optee bindings to json-schema and added interrupt property
* Configure notification interrupt from DT instead of getting it
  from secure world, suggested by Ard Biesheuvel <ardb@kernel.org>.

Thanks,
Jens

Jens Wiklander (7):
  docs: staging/tee.rst: add a section on OP-TEE notifications
  dt-bindings: arm: Convert optee binding to json-schema
  dt-bindings: arm: optee: add interrupt property
  tee: fix put order in teedev_close_context()
  tee: add tee_dev_open_helper() primitive
  optee: separate notification functions
  optee: add asynchronous notifications

 .../bindings/arm/firmware/linaro,optee-tz.txt |  31 ---
 .../arm/firmware/linaro,optee-tz.yaml         |  57 +++++
 Documentation/staging/tee.rst                 |  27 +++
 drivers/tee/optee/Makefile                    |   1 +
 drivers/tee/optee/call.c                      |  27 +++
 drivers/tee/optee/core.c                      |  87 +++++--
 drivers/tee/optee/notif.c                     | 226 ++++++++++++++++++
 drivers/tee/optee/optee_msg.h                 |   9 +
 drivers/tee/optee/optee_private.h             |  23 +-
 drivers/tee/optee/optee_rpc_cmd.h             |  31 +--
 drivers/tee/optee/optee_smc.h                 |  75 +++++-
 drivers/tee/optee/rpc.c                       |  73 +-----
 drivers/tee/tee_core.c                        |  37 ++-
 include/linux/tee_drv.h                       |  27 +++
 14 files changed, 576 insertions(+), 155 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt
 create mode 100644 Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
 create mode 100644 drivers/tee/optee/notif.c

Comments

Sumit Garg June 17, 2021, 4:33 a.m. UTC | #1
Hi Jens,

On Wed, 16 Jun 2021 at 16:07, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Hi all,
>
> This adds support for asynchronous notifications from OP-TEE in secure
> world to the OP-TEE driver. This allows a design with a top half and bottom
> half type of driver where the top half runs in secure interrupt context and
> a notifications tells normal world to schedule a yielding call to do the
> bottom half processing.
>
> An interrupt is used to notify the driver that there are asynchronous
> notifications pending.
>

It looks like a nice feature. I would like to get hands on with this.
Can I test this feature on Qemu?

-Sumit

> v2:
> * Added documentation
> * Converted optee bindings to json-schema and added interrupt property
> * Configure notification interrupt from DT instead of getting it
>   from secure world, suggested by Ard Biesheuvel <ardb@kernel.org>.
>
> Thanks,
> Jens
>
> Jens Wiklander (7):
>   docs: staging/tee.rst: add a section on OP-TEE notifications
>   dt-bindings: arm: Convert optee binding to json-schema
>   dt-bindings: arm: optee: add interrupt property
>   tee: fix put order in teedev_close_context()
>   tee: add tee_dev_open_helper() primitive
>   optee: separate notification functions
>   optee: add asynchronous notifications
>
>  .../bindings/arm/firmware/linaro,optee-tz.txt |  31 ---
>  .../arm/firmware/linaro,optee-tz.yaml         |  57 +++++
>  Documentation/staging/tee.rst                 |  27 +++
>  drivers/tee/optee/Makefile                    |   1 +
>  drivers/tee/optee/call.c                      |  27 +++
>  drivers/tee/optee/core.c                      |  87 +++++--
>  drivers/tee/optee/notif.c                     | 226 ++++++++++++++++++
>  drivers/tee/optee/optee_msg.h                 |   9 +
>  drivers/tee/optee/optee_private.h             |  23 +-
>  drivers/tee/optee/optee_rpc_cmd.h             |  31 +--
>  drivers/tee/optee/optee_smc.h                 |  75 +++++-
>  drivers/tee/optee/rpc.c                       |  73 +-----
>  drivers/tee/tee_core.c                        |  37 ++-
>  include/linux/tee_drv.h                       |  27 +++
>  14 files changed, 576 insertions(+), 155 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt
>  create mode 100644 Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
>  create mode 100644 drivers/tee/optee/notif.c
>
> --
> 2.31.1
>
Jens Wiklander June 17, 2021, 6:10 a.m. UTC | #2
Hi Sumit,

On Thu, Jun 17, 2021 at 6:33 AM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> Hi Jens,
>
> On Wed, 16 Jun 2021 at 16:07, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > Hi all,
> >
> > This adds support for asynchronous notifications from OP-TEE in secure
> > world to the OP-TEE driver. This allows a design with a top half and bottom
> > half type of driver where the top half runs in secure interrupt context and
> > a notifications tells normal world to schedule a yielding call to do the
> > bottom half processing.
> >
> > An interrupt is used to notify the driver that there are asynchronous
> > notifications pending.
> >
>
> It looks like a nice feature. I would like to get hands on with this.
> Can I test this feature on Qemu?

Absolutely, you can get this into the normal OP-TEE development repo setup with:
repo init -u https://github.com/OP-TEE/manifest.git -m default.xml
repo sync
Update optee_os with
https://github.com/jenswi-linaro/optee_os/tree/async_notif_v2
Update linux with https://github.com/jenswi-linaro/linux-1/tree/async_notif_v2
cd build
make all -j...
make run-only

If you type anything at the secure console you'll notice how it
changes behaviour once the Linux kernel has booted.

Cheers,
Jens

>
> -Sumit
>
> > v2:
> > * Added documentation
> > * Converted optee bindings to json-schema and added interrupt property
> > * Configure notification interrupt from DT instead of getting it
> >   from secure world, suggested by Ard Biesheuvel <ardb@kernel.org>.
> >
> > Thanks,
> > Jens
> >
> > Jens Wiklander (7):
> >   docs: staging/tee.rst: add a section on OP-TEE notifications
> >   dt-bindings: arm: Convert optee binding to json-schema
> >   dt-bindings: arm: optee: add interrupt property
> >   tee: fix put order in teedev_close_context()
> >   tee: add tee_dev_open_helper() primitive
> >   optee: separate notification functions
> >   optee: add asynchronous notifications
> >
> >  .../bindings/arm/firmware/linaro,optee-tz.txt |  31 ---
> >  .../arm/firmware/linaro,optee-tz.yaml         |  57 +++++
> >  Documentation/staging/tee.rst                 |  27 +++
> >  drivers/tee/optee/Makefile                    |   1 +
> >  drivers/tee/optee/call.c                      |  27 +++
> >  drivers/tee/optee/core.c                      |  87 +++++--
> >  drivers/tee/optee/notif.c                     | 226 ++++++++++++++++++
> >  drivers/tee/optee/optee_msg.h                 |   9 +
> >  drivers/tee/optee/optee_private.h             |  23 +-
> >  drivers/tee/optee/optee_rpc_cmd.h             |  31 +--
> >  drivers/tee/optee/optee_smc.h                 |  75 +++++-
> >  drivers/tee/optee/rpc.c                       |  73 +-----
> >  drivers/tee/tee_core.c                        |  37 ++-
> >  include/linux/tee_drv.h                       |  27 +++
> >  14 files changed, 576 insertions(+), 155 deletions(-)
> >  delete mode 100644 Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt
> >  create mode 100644 Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> >  create mode 100644 drivers/tee/optee/notif.c
> >
> > --
> > 2.31.1
> >
Sumit Garg July 6, 2021, 7:25 a.m. UTC | #3
On Thu, 17 Jun 2021 at 11:40, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Hi Sumit,
>
> On Thu, Jun 17, 2021 at 6:33 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > Hi Jens,
> >
> > On Wed, 16 Jun 2021 at 16:07, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > >
> > > Hi all,
> > >
> > > This adds support for asynchronous notifications from OP-TEE in secure
> > > world to the OP-TEE driver. This allows a design with a top half and bottom
> > > half type of driver where the top half runs in secure interrupt context and
> > > a notifications tells normal world to schedule a yielding call to do the
> > > bottom half processing.
> > >
> > > An interrupt is used to notify the driver that there are asynchronous
> > > notifications pending.
> > >
> >
> > It looks like a nice feature. I would like to get hands on with this.
> > Can I test this feature on Qemu?
>
> Absolutely, you can get this into the normal OP-TEE development repo setup with:
> repo init -u https://github.com/OP-TEE/manifest.git -m default.xml
> repo sync
> Update optee_os with
> https://github.com/jenswi-linaro/optee_os/tree/async_notif_v2
> Update linux with https://github.com/jenswi-linaro/linux-1/tree/async_notif_v2
> cd build
> make all -j...
> make run-only
>
> If you type anything at the secure console you'll notice how it
> changes behaviour once the Linux kernel has booted.
>

Thanks for sharing instructions as I now got some time to test and
deep dive into this feature. It looks like a pretty useful feature to
realize interrupt support in the secure world in its true sense. This
feature works for me as per your instructions.

I could recognise it's requirement from the time while I was playing
with secure timer interrupt support for OP-TEE RNG driver on
Developerbox. In that case I had to strip down the secure interrupt
handler to a minimum that would just collect entropy and dump into the
secure buffer. But with asynchronous notifications support, I could
add more functionality like entropy health tests in the bottom half
instead of doing those health tests while retrieving entropy from the
secure world.

Given that, have you explored the possibility to leverage SGI rather
than a platform specific SPI for notifying the normal world? If it's
possible to leverage Architecture specific SGI for this purpose then I
think this feature will come automatically enabled for every platform
without the need to reserve a platform specific SPI.

-Sumit

> Cheers,
> Jens
>
> >
> > -Sumit
> >
> > > v2:
> > > * Added documentation
> > > * Converted optee bindings to json-schema and added interrupt property
> > > * Configure notification interrupt from DT instead of getting it
> > >   from secure world, suggested by Ard Biesheuvel <ardb@kernel.org>.
> > >
> > > Thanks,
> > > Jens
> > >
> > > Jens Wiklander (7):
> > >   docs: staging/tee.rst: add a section on OP-TEE notifications
> > >   dt-bindings: arm: Convert optee binding to json-schema
> > >   dt-bindings: arm: optee: add interrupt property
> > >   tee: fix put order in teedev_close_context()
> > >   tee: add tee_dev_open_helper() primitive
> > >   optee: separate notification functions
> > >   optee: add asynchronous notifications
> > >
> > >  .../bindings/arm/firmware/linaro,optee-tz.txt |  31 ---
> > >  .../arm/firmware/linaro,optee-tz.yaml         |  57 +++++
> > >  Documentation/staging/tee.rst                 |  27 +++
> > >  drivers/tee/optee/Makefile                    |   1 +
> > >  drivers/tee/optee/call.c                      |  27 +++
> > >  drivers/tee/optee/core.c                      |  87 +++++--
> > >  drivers/tee/optee/notif.c                     | 226 ++++++++++++++++++
> > >  drivers/tee/optee/optee_msg.h                 |   9 +
> > >  drivers/tee/optee/optee_private.h             |  23 +-
> > >  drivers/tee/optee/optee_rpc_cmd.h             |  31 +--
> > >  drivers/tee/optee/optee_smc.h                 |  75 +++++-
> > >  drivers/tee/optee/rpc.c                       |  73 +-----
> > >  drivers/tee/tee_core.c                        |  37 ++-
> > >  include/linux/tee_drv.h                       |  27 +++
> > >  14 files changed, 576 insertions(+), 155 deletions(-)
> > >  delete mode 100644 Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt
> > >  create mode 100644 Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> > >  create mode 100644 drivers/tee/optee/notif.c
> > >
> > > --
> > > 2.31.1
> > >
Marc Zyngier July 6, 2021, 10:36 a.m. UTC | #4
On Tue, 06 Jul 2021 08:25:26 +0100,
Sumit Garg <sumit.garg@linaro.org> wrote:
> 
> On Thu, 17 Jun 2021 at 11:40, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > Hi Sumit,
> >
> > On Thu, Jun 17, 2021 at 6:33 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> > >
> > > Hi Jens,
> > >
> > > On Wed, 16 Jun 2021 at 16:07, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > > >
> > > > Hi all,
> > > >
> > > > This adds support for asynchronous notifications from OP-TEE in secure
> > > > world to the OP-TEE driver. This allows a design with a top half and bottom
> > > > half type of driver where the top half runs in secure interrupt context and
> > > > a notifications tells normal world to schedule a yielding call to do the
> > > > bottom half processing.
> > > >
> > > > An interrupt is used to notify the driver that there are asynchronous
> > > > notifications pending.
> > > >
> > >
> > > It looks like a nice feature. I would like to get hands on with this.
> > > Can I test this feature on Qemu?
> >
> > Absolutely, you can get this into the normal OP-TEE development repo setup with:
> > repo init -u https://github.com/OP-TEE/manifest.git -m default.xml
> > repo sync
> > Update optee_os with
> > https://github.com/jenswi-linaro/optee_os/tree/async_notif_v2
> > Update linux with https://github.com/jenswi-linaro/linux-1/tree/async_notif_v2
> > cd build
> > make all -j...
> > make run-only
> >
> > If you type anything at the secure console you'll notice how it
> > changes behaviour once the Linux kernel has booted.
> >
> 
> Thanks for sharing instructions as I now got some time to test and
> deep dive into this feature. It looks like a pretty useful feature to
> realize interrupt support in the secure world in its true sense. This
> feature works for me as per your instructions.
> 
> I could recognise it's requirement from the time while I was playing
> with secure timer interrupt support for OP-TEE RNG driver on
> Developerbox. In that case I had to strip down the secure interrupt
> handler to a minimum that would just collect entropy and dump into the
> secure buffer. But with asynchronous notifications support, I could
> add more functionality like entropy health tests in the bottom half
> instead of doing those health tests while retrieving entropy from the
> secure world.
> 
> Given that, have you explored the possibility to leverage SGI rather
> than a platform specific SPI for notifying the normal world? If it's
> possible to leverage Architecture specific SGI for this purpose then I

What does "Architecture specific SGI" mean?

> think this feature will come automatically enabled for every platform
> without the need to reserve a platform specific SPI.

That old chestnut again...

- How do you discover that the secure side has graced you with a
  Group-1 SGI (no, you can't use one of the first 8)? for both DT and
  ACPI?

- How do you find which CPUs are targeted by this SGI? All? One? A
  subset? What is the expected behaviour with CPU hotplug? How can the
  NS side (Linux) can inform the secure side about the CPUs it wants
  to use?

- Is there any case where you would instead need a level interrupt
  (which a SGI cannot provide)?

In general, cross world SGIs are a really bad idea. Yes, some people
like them. I still think they are misguided, and I don't intend to
provide a generic request interface for this.

	M.
Sumit Garg July 6, 2021, 11:39 a.m. UTC | #5
Hi Marc,

On Tue, 6 Jul 2021 at 16:06, Marc Zyngier <maz@kernel.org> wrote:
>
> On Tue, 06 Jul 2021 08:25:26 +0100,
> Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > On Thu, 17 Jun 2021 at 11:40, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > >
> > > Hi Sumit,
> > >
> > > On Thu, Jun 17, 2021 at 6:33 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> > > >
> > > > Hi Jens,
> > > >
> > > > On Wed, 16 Jun 2021 at 16:07, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > > > >
> > > > > Hi all,
> > > > >
> > > > > This adds support for asynchronous notifications from OP-TEE in secure
> > > > > world to the OP-TEE driver. This allows a design with a top half and bottom
> > > > > half type of driver where the top half runs in secure interrupt context and
> > > > > a notifications tells normal world to schedule a yielding call to do the
> > > > > bottom half processing.
> > > > >
> > > > > An interrupt is used to notify the driver that there are asynchronous
> > > > > notifications pending.
> > > > >
> > > >
> > > > It looks like a nice feature. I would like to get hands on with this.
> > > > Can I test this feature on Qemu?
> > >
> > > Absolutely, you can get this into the normal OP-TEE development repo setup with:
> > > repo init -u https://github.com/OP-TEE/manifest.git -m default.xml
> > > repo sync
> > > Update optee_os with
> > > https://github.com/jenswi-linaro/optee_os/tree/async_notif_v2
> > > Update linux with https://github.com/jenswi-linaro/linux-1/tree/async_notif_v2
> > > cd build
> > > make all -j...
> > > make run-only
> > >
> > > If you type anything at the secure console you'll notice how it
> > > changes behaviour once the Linux kernel has booted.
> > >
> >
> > Thanks for sharing instructions as I now got some time to test and
> > deep dive into this feature. It looks like a pretty useful feature to
> > realize interrupt support in the secure world in its true sense. This
> > feature works for me as per your instructions.
> >
> > I could recognise it's requirement from the time while I was playing
> > with secure timer interrupt support for OP-TEE RNG driver on
> > Developerbox. In that case I had to strip down the secure interrupt
> > handler to a minimum that would just collect entropy and dump into the
> > secure buffer. But with asynchronous notifications support, I could
> > add more functionality like entropy health tests in the bottom half
> > instead of doing those health tests while retrieving entropy from the
> > secure world.
> >
> > Given that, have you explored the possibility to leverage SGI rather
> > than a platform specific SPI for notifying the normal world? If it's
> > possible to leverage Architecture specific SGI for this purpose then I
>
> What does "Architecture specific SGI" mean?
>

Here I meant that SGI is specific to Arm architecture and doesn't
require to be specific to per platform like an SPI.

> > think this feature will come automatically enabled for every platform
> > without the need to reserve a platform specific SPI.
>
> That old chestnut again...

Okay, can you provide reference to earlier threads?

>
> - How do you discover that the secure side has graced you with a
>   Group-1 SGI (no, you can't use one of the first 8)? for both DT and
>   ACPI?

I think the secure world can be probed for that during the OP-TEE
driver probe. And I agree with you that the first 7 SGIs are already
pre-occupied and I guess you remember mine patch-set that tried to
leverage 8th SGI as pseudo NMI for kernel debug purposes.

So yes for this use-case, the secure world can reserve one of the
latter 8 SGIs (8 to 15) for cross world notification and I guess your
earlier work to make SGIs to be requested as normal IRQs should make
it easier to implement this as well.

>
> - How do you find which CPUs are targeted by this SGI? All? One? A
>   subset? What is the expected behaviour with CPU hotplug? How can the
>   NS side (Linux) can inform the secure side about the CPUs it wants
>   to use?

For the current OP-TEE use-case, I think targeting all CPUs would be
efficient. So wouldn't it be possible for the CPU which receives the
secure interrupt to raise that SGI to self that would in turn notify
the normal world (Linux) to create a thread for OP-TEE to do bottom
half processing?

>
> - Is there any case where you would instead need a level interrupt
>   (which a SGI cannot provide)?

I think SGI should be sufficient to suffice OP-TEE notifications use-case.

>
> In general, cross world SGIs are a really bad idea. Yes, some people
> like them. I still think they are misguided, and I don't intend to
> provide a generic request interface for this.

Okay, as I mentioned above having it specific to OP-TEE driver
requesting secure world donated SGI would work for you?

-Sumit

>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.
Marc Zyngier July 6, 2021, 12:46 p.m. UTC | #6
Sumit,

On Tue, 06 Jul 2021 12:39:13 +0100,
Sumit Garg <sumit.garg@linaro.org> wrote:
> 
> Hi Marc,
> 
> On Tue, 6 Jul 2021 at 16:06, Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Tue, 06 Jul 2021 08:25:26 +0100,
> > Sumit Garg <sumit.garg@linaro.org> wrote:
> > >
> > > I could recognise it's requirement from the time while I was playing
> > > with secure timer interrupt support for OP-TEE RNG driver on
> > > Developerbox. In that case I had to strip down the secure interrupt
> > > handler to a minimum that would just collect entropy and dump into the
> > > secure buffer. But with asynchronous notifications support, I could
> > > add more functionality like entropy health tests in the bottom half
> > > instead of doing those health tests while retrieving entropy from the
> > > secure world.
> > >
> > > Given that, have you explored the possibility to leverage SGI rather
> > > than a platform specific SPI for notifying the normal world? If it's
> > > possible to leverage Architecture specific SGI for this purpose then I
> >
> > What does "Architecture specific SGI" mean?
> >
> 
> Here I meant that SGI is specific to Arm architecture and doesn't
> require to be specific to per platform like an SPI.

SGIs are, by definition *software* specific (the clue is in the name),
and the architecture spec has *zero* say into what they are used for.
It says even less when it comes to specifying cross-world signalling.

> 
> > > think this feature will come automatically enabled for every platform
> > > without the need to reserve a platform specific SPI.
> >
> > That old chestnut again...
> 
> Okay, can you provide reference to earlier threads?

They show up every other year. Lore is your friend.

> 
> >
> > - How do you discover that the secure side has graced you with a
> >   Group-1 SGI (no, you can't use one of the first 8)? for both DT and
> >   ACPI?
> 
> I think the secure world can be probed

How? With what guarantees?

> for that during the OP-TEE driver probe.

Oh, so it is only for the benefit of a single driver?

> And I agree with you that the first 7 SGIs are already
> pre-occupied and I guess you remember mine patch-set that tried to
> leverage 8th SGI as pseudo NMI for kernel debug purposes.

I do remember, and I'm definitely not keen on spending this last SGI
on this feature.

> So yes for this use-case, the secure world can reserve one of the
> latter 8 SGIs (8 to 15) for cross world notification and I guess your
> earlier work to make SGIs to be requested as normal IRQs should make
> it easier to implement this as well.
>
> >
> > - How do you find which CPUs are targeted by this SGI? All? One? A
> >   subset? What is the expected behaviour with CPU hotplug? How can the
> >   NS side (Linux) can inform the secure side about the CPUs it wants
> >   to use?
> 
> For the current OP-TEE use-case, I think targeting all CPUs would be
> efficient.

Efficient? How? Broadcast? One of N? Random?

> So wouldn't it be possible for the CPU which receives the
> secure interrupt to raise that SGI to self that would in turn notify
> the normal world (Linux) to create a thread for OP-TEE to do bottom
> half processing?

You are assuming that this is the way the NS side wants to work, and I
question this assumption.

> 
> >
> > - Is there any case where you would instead need a level interrupt
> >   (which a SGI cannot provide)?
> 
> I think SGI should be sufficient to suffice OP-TEE notifications use-case.

I don't care about OP-TEE. If you are proposing a contract between S
and NS, it has to be TEE and OS independent. That's how the
architecture works.

> >
> > In general, cross world SGIs are a really bad idea. Yes, some people
> > like them. I still think they are misguided, and I don't intend to
> > provide a generic request interface for this.
> 
> Okay, as I mentioned above having it specific to OP-TEE driver
> requesting secure world donated SGI would work for you?

No. I want a proper architecture between secure and non-secure that
explain how messages are conveyed between the two world, how
signalling is done, how CPU PM is handled, how targeting is
negotiated. And at the end of the day, this is starting to look a lot
like FFA.

If you want a custom OP-TEE hack, you don't need my blessing for
that. You'll even get to keep the pieces once it breaks. But if you
are going to invent a new universal way of signalling things across
world, you'd better start specifying things the right way, taking into
considerations systems where the interrupt controller doesn't allow
cross-world signalling.

	M.
Sumit Garg July 7, 2021, 5:52 a.m. UTC | #7
On Tue, 6 Jul 2021 at 18:16, Marc Zyngier <maz@kernel.org> wrote:
>
> Sumit,
>
> On Tue, 06 Jul 2021 12:39:13 +0100,
> Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > Hi Marc,
> >
> > On Tue, 6 Jul 2021 at 16:06, Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > On Tue, 06 Jul 2021 08:25:26 +0100,
> > > Sumit Garg <sumit.garg@linaro.org> wrote:
> > > >
> > > > I could recognise it's requirement from the time while I was playing
> > > > with secure timer interrupt support for OP-TEE RNG driver on
> > > > Developerbox. In that case I had to strip down the secure interrupt
> > > > handler to a minimum that would just collect entropy and dump into the
> > > > secure buffer. But with asynchronous notifications support, I could
> > > > add more functionality like entropy health tests in the bottom half
> > > > instead of doing those health tests while retrieving entropy from the
> > > > secure world.
> > > >
> > > > Given that, have you explored the possibility to leverage SGI rather
> > > > than a platform specific SPI for notifying the normal world? If it's
> > > > possible to leverage Architecture specific SGI for this purpose then I
> > >
> > > What does "Architecture specific SGI" mean?
> > >
> >
> > Here I meant that SGI is specific to Arm architecture and doesn't
> > require to be specific to per platform like an SPI.
>
> SGIs are, by definition *software* specific (the clue is in the name),
> and the architecture spec has *zero* say into what they are used for.
> It says even less when it comes to specifying cross-world signalling.
>

Agree.

> >
> > > > think this feature will come automatically enabled for every platform
> > > > without the need to reserve a platform specific SPI.
> > >
> > > That old chestnut again...
> >
> > Okay, can you provide reference to earlier threads?
>
> They show up every other year. Lore is your friend.
>

Okay.

> >
> > >
> > > - How do you discover that the secure side has graced you with a
> > >   Group-1 SGI (no, you can't use one of the first 8)? for both DT and
> > >   ACPI?
> >
> > I think the secure world can be probed
>
> How? With what guarantees?
>

It can simply be a fast SMC call to OP-TEE to retrieve the SGI to be
used for notification using similar SMC as
OPTEE_SMC_FUNCID_GET_ASYNC_NOTIF_VALUE that Jens has used in this
patch-set.

I am not sure how that would fail as we do maintain backwards
compatibility with prior OP-TEE versions.

> > for that during the OP-TEE driver probe.
>
> Oh, so it is only for the benefit of a single driver?
>

Yeah.

> > And I agree with you that the first 7 SGIs are already
> > pre-occupied and I guess you remember mine patch-set that tried to
> > leverage 8th SGI as pseudo NMI for kernel debug purposes.
>
> I do remember, and I'm definitely not keen on spending this last SGI
> on this feature.

Agree and that's why we allowed that last SGI for debug purposes if it
is not used anywhere else. Let's keep this discussion to the
corresponding patch-set only as otherwise we would unnecessarily
derail discussion for this OP-TEE specific feature.

>
> > So yes for this use-case, the secure world can reserve one of the
> > latter 8 SGIs (8 to 15) for cross world notification and I guess your
> > earlier work to make SGIs to be requested as normal IRQs should make
> > it easier to implement this as well.
> >
> > >
> > > - How do you find which CPUs are targeted by this SGI? All? One? A
> > >   subset? What is the expected behaviour with CPU hotplug? How can the
> > >   NS side (Linux) can inform the secure side about the CPUs it wants
> > >   to use?
> >
> > For the current OP-TEE use-case, I think targeting all CPUs would be
> > efficient.
>
> Efficient? How? Broadcast? One of N? Random?
>

By efficient here I meant that we would enable that SGI for every CPU
rather than a subset so that any CPU which receives a secure interrupt
(PPI or SPI) would be able to raise this SGI to itself in order to
notify Linux to create a thread for OP-TEE.

> > So wouldn't it be possible for the CPU which receives the
> > secure interrupt to raise that SGI to self that would in turn notify
> > the normal world (Linux) to create a thread for OP-TEE to do bottom
> > half processing?
>
> You are assuming that this is the way the NS side wants to work, and I
> question this assumption.
>

Actually this is the way that Jens has implemented notifications among
Linux and OP-TEE using a SPI in this patch-set. The only difference
with SGI is that it's a per CPU interrupt.

> >
> > >
> > > - Is there any case where you would instead need a level interrupt
> > >   (which a SGI cannot provide)?
> >
> > I think SGI should be sufficient to suffice OP-TEE notifications use-case.
>
> I don't care about OP-TEE. If you are proposing a contract between S
> and NS, it has to be TEE and OS independent. That's how the
> architecture works.
>

Agree, here we are not proposing a common contract among the S and NS
world that every TEE (based on Arm TrustZone) will use to communicate
with REE (Linux in our case) but rather an OP-TEE specific
notifications feature that is built on top of OP-TEE specific ABIs.

And I can see your arguments coming from an FFA perspective but there
are platforms like the ones based on Armv7 which don't support FFA
ABI. Maybe Jens can elaborate how this feature will fit in when FFA
comes into picture?

> > >
> > > In general, cross world SGIs are a really bad idea. Yes, some people
> > > like them. I still think they are misguided, and I don't intend to
> > > provide a generic request interface for this.
> >
> > Okay, as I mentioned above having it specific to OP-TEE driver
> > requesting secure world donated SGI would work for you?
>
> No. I want a proper architecture between secure and non-secure that
> explain how messages are conveyed between the two world, how
> signalling is done, how CPU PM is handled, how targeting is
> negotiated. And at the end of the day, this is starting to look a lot
> like FFA.

AFAIK when FFA comes in picture than OP-TEE will use the standard
interface provided by FFA ABIs but if FFA isn't supported by a
particular platform (eg. based on Armv7) then we need to rely on TEE
specific ABI like what OP-TEE currently provides:

1. how messages are conveyed between the two worlds -> OP-TEE specific
ABI (yielding SMC calls).
2. how signalling is done -> OP-TEE specific ABI (fast SMC calls).
3. how CPU PM is handled -> OP-TEE is notified on PSCI CPU ON, OFF and
SUSPEND calls.
4. how targeting is negotiated -> SGI would be targeted to the same
CPU which receives the secure interrupt (PPI or SPI).

>
> If you want a custom OP-TEE hack, you don't need my blessing for
> that. You'll even get to keep the pieces once it breaks. But if you
> are going to invent a new universal way of signalling things across
> world, you'd better start specifying things the right way, taking into
> considerations systems where the interrupt controller doesn't allow
> cross-world signalling.

As I mentioned above, this patch-set adds an OP-TEE specific
notifications feature. AFAIK, the interrupt controllers supported by
OP-TEE (GICv2, GICv3 etc.) don't restrict cross-world signaling.

So given the explanation above, if you still think requesting an SGI
as an IRQ by drivers isn't allowed then I am fine with the approach
that Jens has already implemented in this patch-set to use platform
specific SPI.

-Sumit

>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.
Jens Wiklander July 7, 2021, 6:54 a.m. UTC | #8
Hi,

On Wed, Jul 7, 2021 at 7:52 AM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On Tue, 6 Jul 2021 at 18:16, Marc Zyngier <maz@kernel.org> wrote:
> >
[snip]
> > > > - Is there any case where you would instead need a level interrupt
> > > >   (which a SGI cannot provide)?
> > >
> > > I think SGI should be sufficient to suffice OP-TEE notifications use-case.
> >
> > I don't care about OP-TEE. If you are proposing a contract between S
> > and NS, it has to be TEE and OS independent. That's how the
> > architecture works.
> >
>
> Agree, here we are not proposing a common contract among the S and NS
> world that every TEE (based on Arm TrustZone) will use to communicate
> with REE (Linux in our case) but rather an OP-TEE specific
> notifications feature that is built on top of OP-TEE specific ABIs.
>
> And I can see your arguments coming from an FFA perspective but there
> are platforms like the ones based on Armv7 which don't support FFA
> ABI. Maybe Jens can elaborate how this feature will fit in when FFA
> comes into picture?

OP-TEE has one official ABI at the moment, the SMC based one. It's
about to get another one based on FF-A instead. The two ABIs will
never be used at the same time. It's a build time option for the
OP-TEE firmware to either use SMC or FF-A based communication.

The patches I've posted here concern the SMC based ABI. Asynchronous
notification in OP-TEE with a FF-A based ABI will use the notification
framework provided by FF-A instead to implement that counterpart
provided by these patches. So the OP-TEE driver here in the kernel
will use the FF-A framework in the kernel instead of registering an
interrupt handler directly.

Cheers,
Jens
Sudeep Holla July 7, 2021, 5:51 p.m. UTC | #9
Hi Sumit,

I was holding off you reply as I didn't have all the background on this.
Achin did mention that this is preparatory work for FFA notifications.
I did mention to him that this is more than that, it is custom extension
to address what FF-A notification is trying to in standard way.

I share same opinion as Marc Z.

On Wed, Jul 07, 2021 at 11:22:23AM +0530, Sumit Garg wrote:
> On Tue, 6 Jul 2021 at 18:16, Marc Zyngier <maz@kernel.org> wrote:

[...]

> >
> > I don't care about OP-TEE. If you are proposing a contract between S
> > and NS, it has to be TEE and OS independent. That's how the
> > architecture works.
> >
> 
> Agree, here we are not proposing a common contract among the S and NS
> world that every TEE (based on Arm TrustZone) will use to communicate
> with REE (Linux in our case) but rather an OP-TEE specific
> notifications feature that is built on top of OP-TEE specific ABIs.
> 
> And I can see your arguments coming from an FFA perspective but there
> are platforms like the ones based on Armv7 which don't support FFA
> ABI. Maybe Jens can elaborate how this feature will fit in when FFA
> comes into picture?
>

I can understand that but won't those platforms add the support both in
the kernel(current series) and secure world to address notifications.
While you could argue that it is small extension to what is already present
but I prefer they support FF-A is they need such a support instead of adding
custom mechanisms. It is hard to maintain and each vendor will deviate
from this custom mechanism and soon we will have bunch of them to handle.

> > > >
> > > > In general, cross world SGIs are a really bad idea. Yes, some people
> > > > like them. I still think they are misguided, and I don't intend to
> > > > provide a generic request interface for this.
> > >
> > > Okay, as I mentioned above having it specific to OP-TEE driver
> > > requesting secure world donated SGI would work for you?
> >
> > No. I want a proper architecture between secure and non-secure that
> > explain how messages are conveyed between the two world, how
> > signalling is done, how CPU PM is handled, how targeting is
> > negotiated. And at the end of the day, this is starting to look a lot
> > like FFA.
> 
> AFAIK when FFA comes in picture than OP-TEE will use the standard
> interface provided by FFA ABIs but if FFA isn't supported by a
> particular platform (eg. based on Armv7) then we need to rely on TEE
> specific ABI like what OP-TEE currently provides:
>

Who are asking for this ? Can we ask them to migrate to FF-A if this
(new) notification support is needed on their platforms ? It is help to
know the requesters so that they can be included in FF-A spec discussions.

> > that. You'll even get to keep the pieces once it breaks. But if you
> > are going to invent a new universal way of signalling things across
> > world, you'd better start specifying things the right way, taking into
> > considerations systems where the interrupt controller doesn't allow
> > cross-world signalling.
> 
> As I mentioned above, this patch-set adds an OP-TEE specific
> notifications feature. AFAIK, the interrupt controllers supported by
> OP-TEE (GICv2, GICv3 etc.) don't restrict cross-world signaling.
> 
> So given the explanation above, if you still think requesting an SGI
> as an IRQ by drivers isn't allowed then I am fine with the approach
> that Jens has already implemented in this patch-set to use platform
> specific SPI.
>

And I assume these platforms in question have SPI to spare and way to
trigger it from secure world ?
Sumit Garg July 8, 2021, 12:56 p.m. UTC | #10
Hi Sudeep,

On Wed, 7 Jul 2021 at 23:22, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> Hi Sumit,
>
> I was holding off you reply as I didn't have all the background on this.
> Achin did mention that this is preparatory work for FFA notifications.
> I did mention to him that this is more than that, it is custom extension
> to address what FF-A notification is trying to in standard way.
>
> I share same opinion as Marc Z.
>
> On Wed, Jul 07, 2021 at 11:22:23AM +0530, Sumit Garg wrote:
> > On Tue, 6 Jul 2021 at 18:16, Marc Zyngier <maz@kernel.org> wrote:
>
> [...]
>
> > >
> > > I don't care about OP-TEE. If you are proposing a contract between S
> > > and NS, it has to be TEE and OS independent. That's how the
> > > architecture works.
> > >
> >
> > Agree, here we are not proposing a common contract among the S and NS
> > world that every TEE (based on Arm TrustZone) will use to communicate
> > with REE (Linux in our case) but rather an OP-TEE specific
> > notifications feature that is built on top of OP-TEE specific ABIs.
> >
> > And I can see your arguments coming from an FFA perspective but there
> > are platforms like the ones based on Armv7 which don't support FFA
> > ABI. Maybe Jens can elaborate how this feature will fit in when FFA
> > comes into picture?
> >
>
> I can understand that but won't those platforms add the support both in
> the kernel(current series) and secure world to address notifications.

Agree.

> While you could argue that it is small extension to what is already present
> but I prefer they support FF-A is they need such a support instead of adding
> custom mechanisms. It is hard to maintain and each vendor will deviate
> from this custom mechanism and soon we will have bunch of them to handle.
>

I haven't had a deep dive into FF-A spec, maybe you can clarify on the
following queries regarding Armv7 compatibility:
- As you may be aware, secure monitor implementation on Armv7 is
tightly coupled to trusted OS (part of the same code base), so would
you like each trusted OS vendor to implement a common FF-A interface?
- IIRC, FF-A spec has the notion of multiple secure partitions, are
those supported on Armv7? If yes then how?

> > > > >
> > > > > In general, cross world SGIs are a really bad idea. Yes, some people
> > > > > like them. I still think they are misguided, and I don't intend to
> > > > > provide a generic request interface for this.
> > > >
> > > > Okay, as I mentioned above having it specific to OP-TEE driver
> > > > requesting secure world donated SGI would work for you?
> > >
> > > No. I want a proper architecture between secure and non-secure that
> > > explain how messages are conveyed between the two world, how
> > > signalling is done, how CPU PM is handled, how targeting is
> > > negotiated. And at the end of the day, this is starting to look a lot
> > > like FFA.
> >
> > AFAIK when FFA comes in picture than OP-TEE will use the standard
> > interface provided by FFA ABIs but if FFA isn't supported by a
> > particular platform (eg. based on Armv7) then we need to rely on TEE
> > specific ABI like what OP-TEE currently provides:
> >
>
> Who are asking for this ? Can we ask them to migrate to FF-A if this
> (new) notification support is needed on their platforms ? It is help to
> know the requesters so that they can be included in FF-A spec discussions.
>

I would let Jens answer that.

> > > that. You'll even get to keep the pieces once it breaks. But if you
> > > are going to invent a new universal way of signalling things across
> > > world, you'd better start specifying things the right way, taking into
> > > considerations systems where the interrupt controller doesn't allow
> > > cross-world signalling.
> >
> > As I mentioned above, this patch-set adds an OP-TEE specific
> > notifications feature. AFAIK, the interrupt controllers supported by
> > OP-TEE (GICv2, GICv3 etc.) don't restrict cross-world signaling.
> >
> > So given the explanation above, if you still think requesting an SGI
> > as an IRQ by drivers isn't allowed then I am fine with the approach
> > that Jens has already implemented in this patch-set to use platform
> > specific SPI.
> >
>
> And I assume these platforms in question have SPI to spare and way to
> trigger it from secure world ?
>

Yeah, that is the requirement on the platform if we rely on SPI (Qemu
test example [1]) which wouldn't be the case if we use secure world
donated SGI.

BTW, is this notification mechanism discussed in the case of FF-A? If
yes, can you throw some light on that?

[1] https://github.com/jenswi-linaro/optee_os/commit/9007f8184deb9b7995da8d590779cb3ba2783394

-Sumit

> --
> Regards,
> Sudeep