mbox series

[0/4] firmware: Add support for Qualcomm UEFI Secure Application

Message ID 20220723224949.1089973-1-luzmaximilian@gmail.com
Headers show
Series firmware: Add support for Qualcomm UEFI Secure Application | expand

Message

Maximilian Luz July 23, 2022, 10:49 p.m. UTC
On modern Qualcomm platforms, access to EFI variables is restricted to
the secure world / TrustZone, i.e. the Trusted Execution Environment
(TrEE or TEE) as Qualcomm seems to call it. To access EFI variables, we
therefore need to talk to the UEFI Secure Application (uefisecapp),
residing in the TrEE.

This series adds support for accessing EFI variables on those platforms.

To do this, we first need to add some SCM call functions used to manage
and talk to Secure Applications. A very small subset of this interface
is added in the second patch (whereas the first one exports the required
functions for that). Interface specifications are extracted from [1].
While this does not (yet) support re-entrant SCM calls (including
callbacks and listeners), this is enough to talk to the aforementioned
uefisecapp on a couple of platforms (I've tested this on a Surface Pro X
and heard reports from Lenovo Flex 5G, Lenovo Thinkpad x13s, and Lenovo
Yoga C630 devices).

The third patch adds a client driver for uefisecapp, installing the
respective efivar operations. The application interface has been reverse
engineered from the Windows QcTrEE8180.sys driver.

Apart from uefisecapp, there are more Secure Applications running that
we might want to support in the future. For example, on the Surface Pro
X (sc8180x-based), the TPM is also managed via one.

I'm not sure whether this should go to drivers/firmware or to
drivers/soc/qcom. I've put this into firmware as all of this is
essentially an interface to the secure firmware running in the TrustZone
(and SCM stuff is handled here already), but please let me know if I
should move this.

Regards,
Max

[1]: https://git.codelinaro.org/clo/la/kernel/msm-4.14/-/blob/auto-kernel.lnx.4.14.c34/drivers/misc/qseecom.c

Maximilian Luz (4):
  firmware: qcom_scm: Export SCM call functions
  firmware: Add support for Qualcomm Trusted Execution Environment SCM
    calls
  firmware: Add support for Qualcomm UEFI Secure Application
  dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

 .../firmware/qcom,tee-uefisecapp.yaml         |  38 +
 MAINTAINERS                                   |  14 +
 drivers/firmware/Kconfig                      |  20 +
 drivers/firmware/Makefile                     |   2 +
 drivers/firmware/qcom_scm.c                   | 118 ++-
 drivers/firmware/qcom_scm.h                   |  47 --
 drivers/firmware/qcom_tee.c                   | 213 +++++
 drivers/firmware/qcom_tee_uefisecapp.c        | 761 ++++++++++++++++++
 include/linux/qcom_scm.h                      |  49 ++
 include/linux/qcom_tee.h                      | 179 ++++
 10 files changed, 1355 insertions(+), 86 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml
 create mode 100644 drivers/firmware/qcom_tee.c
 create mode 100644 drivers/firmware/qcom_tee_uefisecapp.c
 create mode 100644 include/linux/qcom_tee.h

Comments

Rob Herring (Arm) July 25, 2022, 1:06 a.m. UTC | #1
On Sun, 24 Jul 2022 00:49:49 +0200, Maximilian Luz wrote:
> Add bindings for the Qualcomm Trusted Execution Environment (TrEE) UEFI
> Secure application (uefisecapp) client.
> 
> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
> ---
>  .../firmware/qcom,tee-uefisecapp.yaml         | 38 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 39 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
./Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml: $id: relative path/filename doesn't match actual path or filename
	expected: http://devicetree.org/schemas/firmware/qcom,tee-uefisecapp.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/soc/qcom/qcom,rpmh-rsc.yaml: duplicate '$id' value 'http://devicetree.org/schemas/soc/qcom/qcom,rpmh-rsc.yaml#'
Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.example.dtb:0:0: /example-0/firmware/scm: failed to match any schema with compatible: ['qcom,scm-sc8180x', 'qcom,scm']
Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.example.dtb:0:0: /example-0/firmware/scm: failed to match any schema with compatible: ['qcom,scm-sc8180x', 'qcom,scm']
Documentation/devicetree/bindings/soc/qcom/qcom,rpmh-rsc.example.dtb:0:0: /example-0/rsc@179c0000: failed to match any schema with compatible: ['qcom,rpmh-rsc']
Documentation/devicetree/bindings/soc/qcom/qcom,rpmh-rsc.example.dtb:0:0: /example-1/rsc@af20000: failed to match any schema with compatible: ['qcom,rpmh-rsc']
Documentation/devicetree/bindings/soc/qcom/qcom,rpmh-rsc.example.dtb:0:0: /example-2/rsc@18200000: failed to match any schema with compatible: ['qcom,rpmh-rsc']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.
Rob Herring (Arm) July 25, 2022, 7:27 p.m. UTC | #2
On Sun, Jul 24, 2022 at 12:49:45AM +0200, Maximilian Luz wrote:
> On modern Qualcomm platforms, access to EFI variables is restricted to
> the secure world / TrustZone, i.e. the Trusted Execution Environment
> (TrEE or TEE) as Qualcomm seems to call it. To access EFI variables, we
> therefore need to talk to the UEFI Secure Application (uefisecapp),
> residing in the TrEE.

The whole point of UEFI is providing a standard interface. Why can't the 
UEFI implementation call the TEE itself?

I'm not sure custom interfaces is something we want.


> This series adds support for accessing EFI variables on those platforms.
> 
> To do this, we first need to add some SCM call functions used to manage
> and talk to Secure Applications. A very small subset of this interface
> is added in the second patch (whereas the first one exports the required
> functions for that). Interface specifications are extracted from [1].
> While this does not (yet) support re-entrant SCM calls (including
> callbacks and listeners), this is enough to talk to the aforementioned
> uefisecapp on a couple of platforms (I've tested this on a Surface Pro X
> and heard reports from Lenovo Flex 5G, Lenovo Thinkpad x13s, and Lenovo
> Yoga C630 devices).

What does Windows do on these devices? I'm surprised something like this 
would fly with Microsoft.

Rob
Maximilian Luz July 25, 2022, 8:16 p.m. UTC | #3
On 7/25/22 21:27, Rob Herring wrote:
> On Sun, Jul 24, 2022 at 12:49:45AM +0200, Maximilian Luz wrote:
>> On modern Qualcomm platforms, access to EFI variables is restricted to
>> the secure world / TrustZone, i.e. the Trusted Execution Environment
>> (TrEE or TEE) as Qualcomm seems to call it. To access EFI variables, we
>> therefore need to talk to the UEFI Secure Application (uefisecapp),
>> residing in the TrEE.
> 
> The whole point of UEFI is providing a standard interface. Why can't the
> UEFI implementation call the TEE itself?
> 
> I'm not sure custom interfaces is something we want.

Unfortunately, I'm not a Qualcomm engineer and in no way affiliated with
them. So I probably can't convince them otherwise. Believe me, I'd like
to :/

First: The uefisecapp-driver is based on reverse-engineering. So please
take things below with a grain of salt, I may be wrong. I've tried to
lay this out in a bit more detail in patch 3, but I'll try to be more
precise here:

For some reason unknown to me, Qualcomm decided to lock away UEFI
variable access via their TrEE framework for applications running in the
TrustZone (or whatever that is exactly). To call to TrEE applications
(like uefisecapp), they use SCM calls. Those SCM calls unfortunately can
be a bit more complex. As far as I can tell, you essentially call to
some hypervisor in the TrustZone which redistributes them (if necessary)
to the respective application. Their downstream driver for that is at
[1] and supports callbacks and re-entrant calls. As far as I can tell,
the latter means that you can't run multiple SCM calls in parallel (at
least not to that TzOS/TrEE interface) and can only ever have one
"client" performing them.

And as you can only ever have one entity performing those SCM calls, you
cannot have both UEFI and the kernel doing them. To me, it seems to be a
deliberate decision by Qualcomm to return EFI_UNSUPPORTED from the
GetVariable etc. calls after exiting boot services. They do work just
fine before that. Essentially exiting boot services transfers ownership
of that SCM interface from UEFI to the kernel.

Note: uefisecapp is also not the only TrEE application in use by those
kinds of devices. For example, on the SC8180X based Surface Pro X that
I'm using, there are at least an app for the TPM, a bunch of apps for
HDCP, some winsecapp, and as far as I can tell also other cryptographic
interfaces. I've tried to collect my findings about those in [2].

>> This series adds support for accessing EFI variables on those platforms.
>>
>> To do this, we first need to add some SCM call functions used to manage
>> and talk to Secure Applications. A very small subset of this interface
>> is added in the second patch (whereas the first one exports the required
>> functions for that). Interface specifications are extracted from [1].
>> While this does not (yet) support re-entrant SCM calls (including
>> callbacks and listeners), this is enough to talk to the aforementioned
>> uefisecapp on a couple of platforms (I've tested this on a Surface Pro X
>> and heard reports from Lenovo Flex 5G, Lenovo Thinkpad x13s, and Lenovo
>> Yoga C630 devices).
> 
> What does Windows do on these devices? I'm surprised something like this
> would fly with Microsoft.

It looks like Microsoft accepts this. They even seem to have some sort
of interface for EFI variables via trusted execution environments: [3].
This is essentially what the QcTrEE8180.sys driver I've reverse
engineered this from seems to provide.

So unless there's some way to make EFI variables work via the standard
functions that I've missed, I don't see any alternatives. I think it's
fairly unlikely that we can convince Qualcomm to make their UEFI
implementation behave properly (variables are not the only issue, it
seems that other functions are either partially or completely broken in
some way or another...) and then also push updates for a bunch of
devices (e.g. the Lenovo C630 also using this stuff is discontinued, as
far as I can tell).

I am open to suggestions though...

Note that this series also doesn't really introduce a new interface for
EFI variables themselves to the kernel. It relies on the existing
efivars_register() / efivars_unregister() functions and the interface
provided by them to enable access to EFI variables. So we already do
have a "workaround" for broken UEFI variable access in the kernel.

Regards,
Max

[1]: https://git.codelinaro.org/clo/la/kernel/msm-4.14/-/blob/auto-kernel.lnx.4.14.c34/drivers/misc/qseecom.c
[2]: https://github.com/linux-surface/surface-pro-x/issues/37
[3]: https://github.com/tpn/winsdk-10/blob/9b69fd26ac0c7d0b83d378dba01080e93349c2ed/Include/10.0.16299.0/km/treevariableservice.h#L11-L12
Krzysztof Kozlowski July 26, 2022, 10:17 a.m. UTC | #4
On 24/07/2022 00:49, Maximilian Luz wrote:
> Add bindings for the Qualcomm Trusted Execution Environment (TrEE) UEFI
> Secure application (uefisecapp) client.
> 
> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
> ---
>  .../firmware/qcom,tee-uefisecapp.yaml         | 38 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 39 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml
> 
> diff --git a/Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml b/Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml
> new file mode 100644
> index 000000000000..9e5de1005d5c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml
> @@ -0,0 +1,38 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/qcom/qcom,rpmh-rsc.yaml#

Does not look like you tested the bindings. Please run `make
dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Trusted Execution Environment UEFI Secure Application
> +
> +maintainers:
> +  - Maximilian Luz <luzmaximilian@gmail.com>
> +
> +description: |
> +  Various Qualcomm SoCs do not allow direct access to UEFI variables. Instead,
> +  these need to be accessed via the UEFI Secure Application (uefisecapp),
> +  residing in the Trusted Execution Environment (TrEE). These bindings mark the
> +  presence of uefisecapp and allow the respective client driver to load and
> +  install efivar operations, providing the kernel with access to UEFI
> +  variables.
> +
> +properties:
> +  compatible:
> +    const: qcom,tee-uefisecapp

Isn't this SoC-specific device? Generic compatibles are usually not
expected.

> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    firmware {
> +        scm {
> +            compatible = "qcom,scm-sc8180x", "qcom,scm";
> +        };
> +        tee-uefisecapp {
> +            compatible = "qcom,tee-uefisecapp";

You did not model here any dependency on SCM. This is not full
description of the firmware/hardware.



Best regards,
Krzysztof
Maximilian Luz July 26, 2022, 11:15 a.m. UTC | #5
Hi,

On 7/26/22 12:17, Krzysztof Kozlowski wrote:
> On 24/07/2022 00:49, Maximilian Luz wrote:
>> Add bindings for the Qualcomm Trusted Execution Environment (TrEE) UEFI
>> Secure application (uefisecapp) client.
>>
>> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
>> ---
>>   .../firmware/qcom,tee-uefisecapp.yaml         | 38 +++++++++++++++++++
>>   MAINTAINERS                                   |  1 +
>>   2 files changed, 39 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml b/Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml
>> new file mode 100644
>> index 000000000000..9e5de1005d5c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml
>> @@ -0,0 +1,38 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/soc/qcom/qcom,rpmh-rsc.yaml#
> 
> Does not look like you tested the bindings. Please run `make
> dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).

Sorry, first time submitting a schema. Already saw the warning of Rob's
bot and Will fix this in v2.

>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm Trusted Execution Environment UEFI Secure Application
>> +
>> +maintainers:
>> +  - Maximilian Luz <luzmaximilian@gmail.com>
>> +
>> +description: |
>> +  Various Qualcomm SoCs do not allow direct access to UEFI variables. Instead,
>> +  these need to be accessed via the UEFI Secure Application (uefisecapp),
>> +  residing in the Trusted Execution Environment (TrEE). These bindings mark the
>> +  presence of uefisecapp and allow the respective client driver to load and
>> +  install efivar operations, providing the kernel with access to UEFI
>> +  variables.
>> +
>> +properties:
>> +  compatible:
>> +    const: qcom,tee-uefisecapp
> 
> Isn't this SoC-specific device? Generic compatibles are usually not
> expected.

This is essentially software (kernel driver) talking to software (in the
TrustZone), so I don't expect there to be anything SoC specific about it.

>> +
>> +required:
>> +  - compatible
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    firmware {
>> +        scm {
>> +            compatible = "qcom,scm-sc8180x", "qcom,scm";
>> +        };
>> +        tee-uefisecapp {
>> +            compatible = "qcom,tee-uefisecapp";
> 
> You did not model here any dependency on SCM. This is not full
> description of the firmware/hardware

How would I do that? A lot of other stuff also depends on SCM being
present (e.g. qcom_q6v5_pas for loading mdt files) and I don't see them
declare this in the device tree. As far as I can tell, SCM is pretty
much expected to be there at all times (i.e. can't be unloaded) and
drivers check for it when probing via qcom_scm_is_available(),
deferring probe if not.

Don't take this as an excuse as in "I want to leave that out", it's just
that I don't know how one would declare such a dependency explicitly. If
you can tell me how to fix it, I'll include that for v2.

Regards,
Max
Krzysztof Kozlowski July 26, 2022, 1:25 p.m. UTC | #6
On 26/07/2022 13:15, Maximilian Luz wrote:
>>> +properties:
>>> +  compatible:
>>> +    const: qcom,tee-uefisecapp
>>
>> Isn't this SoC-specific device? Generic compatibles are usually not
>> expected.
> 
> This is essentially software (kernel driver) talking to software (in the
> TrustZone), so I don't expect there to be anything SoC specific about it.

You are documenting here firmware in TZ (not kernel driver). Isn't this
a specific piece which might vary from device to device?

IOW, do you expect the same compatible to work for all possible Qualcomm
boards (past and future like in 10 years from now)?

> 
>>> +
>>> +required:
>>> +  - compatible
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    firmware {
>>> +        scm {
>>> +            compatible = "qcom,scm-sc8180x", "qcom,scm";
>>> +        };
>>> +        tee-uefisecapp {
>>> +            compatible = "qcom,tee-uefisecapp";
>>
>> You did not model here any dependency on SCM. This is not full
>> description of the firmware/hardware
> 
> How would I do that? A lot of other stuff also depends on SCM being
> present (e.g. qcom_q6v5_pas for loading mdt files) and I don't see them
> declare this in the device tree. As far as I can tell, SCM is pretty
> much expected to be there at all times (i.e. can't be unloaded) and
> drivers check for it when probing via qcom_scm_is_available(),
> deferring probe if not.

It seems this will be opening a can of worms...

The problem with existing approach is:
1. Lack of any probe ordering or probe deferral support.
2. Lack of any other dependencies, e.g. for PM.

Unloading is "solved" only by disallowing the unload, not by proper
device links and module get/put.

I understand that SCM must be there, but the same for several other
components and for these others we have ways to pass reference around
(e.g. syscon regmap, PHYs handles).

> 
> Don't take this as an excuse as in "I want to leave that out", it's just
> that I don't know how one would declare such a dependency explicitly. If
> you can tell me how to fix it, I'll include that for v2.

I think there are no dedicated subsystem helpers for this (like for
provider/consumer of resets/power domains/clocks etc), so one way would
be something like nvidia,bpmp is doing.

meson_sm_get is a bit similar - looking by compatible. This is less
portable and I would prefer the bpmp way (just like syscon phandles).

The qcom_q6v5_pas could be converted later to use similar approach and
eventually the "tatic struct qcom_scm *__scm;" can be entirely removed.

Any comments on this approach from Konrad, Bjorn, Dmitry, Vinod and
anyone else?


Best regards,
Krzysztof
Sudeep Holla July 26, 2022, 2:30 p.m. UTC | #7
On Sun, Jul 24, 2022 at 12:49:49AM +0200, Maximilian Luz wrote:
> Add bindings for the Qualcomm Trusted Execution Environment (TrEE) UEFI
> Secure application (uefisecapp) client.
> 
> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
> ---
>  .../firmware/qcom,tee-uefisecapp.yaml         | 38 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 39 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml
> 
> diff --git a/Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml b/Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml
> new file mode 100644
> index 000000000000..9e5de1005d5c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml
> @@ -0,0 +1,38 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/qcom/qcom,rpmh-rsc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Trusted Execution Environment UEFI Secure Application
> +
> +maintainers:
> +  - Maximilian Luz <luzmaximilian@gmail.com>
> +
> +description: |
> +  Various Qualcomm SoCs do not allow direct access to UEFI variables. Instead,
> +  these need to be accessed via the UEFI Secure Application (uefisecapp),
> +  residing in the Trusted Execution Environment (TrEE). These bindings mark the
> +  presence of uefisecapp and allow the respective client driver to load and
> +  install efivar operations, providing the kernel with access to UEFI
> +  variables.
> +
> +properties:
> +  compatible:
> +    const: qcom,tee-uefisecapp
> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    firmware {
> +        scm {
> +            compatible = "qcom,scm-sc8180x", "qcom,scm";
> +        };
> +        tee-uefisecapp {
> +            compatible = "qcom,tee-uefisecapp";
> +        };

Do you expect some issues using the scm driver APIs without the
any additions in the DT ? I mean can't you auto-discover by using the
APIs. I haven't looked at the driver or any other patches in the series,
but I would like to know if we can avoid adding any new bindings if it
can be discovered via those SCM driver APIs.
Maximilian Luz July 26, 2022, 3 p.m. UTC | #8
On 7/26/22 15:25, Krzysztof Kozlowski wrote:
> On 26/07/2022 13:15, Maximilian Luz wrote:
>>>> +properties:
>>>> +  compatible:
>>>> +    const: qcom,tee-uefisecapp
>>>
>>> Isn't this SoC-specific device? Generic compatibles are usually not
>>> expected.
>>
>> This is essentially software (kernel driver) talking to software (in the
>> TrustZone), so I don't expect there to be anything SoC specific about it.
> 
> You are documenting here firmware in TZ (not kernel driver). Isn't this
> a specific piece which might vary from device to device?
> 
> IOW, do you expect the same compatible to work for all possible Qualcomm
> boards (past and future like in 10 years from now)?

I'm not sure if Qualcomm will still use the "uefisecapp" approach in 10
years, but I don't expect the interface of uefisecapp to change. The
interface is modeled after the respective UEFI functions, which are spec
and thus I don't expect those to change. Also, it seems to have been
around for a couple of generations and it hasn't changed. The oldest
tested is sdm850 (Lenovo Yoga C630), and the latest is sc8280xp
(Thinkpad X13s).

Why not make this behave like a "normal" third-party device? If the
interface ever changes use qcom,tee-uefisecapp-v2 or something like
that? Again, this does not seem to be directly tied to the SoC.

Then again, if you prefer to name everything based on
"qcom,<device>-<soc>" I don't have any strong arguments against it and
I'm happy to change that. I just think it will unnecessarily introduce
a bunch of compatibles and doesn't reflect the interface "versioning"
situation as I see it.

>>>> +
>>>> +required:
>>>> +  - compatible
>>>> +
>>>> +additionalProperties: false
>>>> +
>>>> +examples:
>>>> +  - |
>>>> +    firmware {
>>>> +        scm {
>>>> +            compatible = "qcom,scm-sc8180x", "qcom,scm";
>>>> +        };
>>>> +        tee-uefisecapp {
>>>> +            compatible = "qcom,tee-uefisecapp";
>>>
>>> You did not model here any dependency on SCM. This is not full
>>> description of the firmware/hardware
>>
>> How would I do that? A lot of other stuff also depends on SCM being
>> present (e.g. qcom_q6v5_pas for loading mdt files) and I don't see them
>> declare this in the device tree. As far as I can tell, SCM is pretty
>> much expected to be there at all times (i.e. can't be unloaded) and
>> drivers check for it when probing via qcom_scm_is_available(),
>> deferring probe if not.
> 
> It seems this will be opening a can of worms...

Indeed.

> The problem with existing approach is:
> 1. Lack of any probe ordering or probe deferral support.
> 2. Lack of any other dependencies, e.g. for PM.

I'm not entirely sure what you mean by "lack of probe deferral support".
We have qcom_scm_is_available() and defer probe if that fails. So
deferral works, unless I'm misunderstanding something.

But yes, correct on the other points.

> Unloading is "solved" only by disallowing the unload, not by proper
> device links and module get/put.
> 
> I understand that SCM must be there, but the same for several other
> components and for these others we have ways to pass reference around
> (e.g. syscon regmap, PHYs handles).
> 
>>
>> Don't take this as an excuse as in "I want to leave that out", it's just
>> that I don't know how one would declare such a dependency explicitly. If
>> you can tell me how to fix it, I'll include that for v2.
> 
> I think there are no dedicated subsystem helpers for this (like for
> provider/consumer of resets/power domains/clocks etc), so one way would
> be something like nvidia,bpmp is doing.

I assume you're referring to tegra_bpmp_get()? Does this correctly
handle PM dependencies? At least as far as I can tell it doesn't
explicitly establish a device link, it only gets a reference to the
device, which doesn't guarantee the presence of a driver. Nor correct PM
ordering. Please correct me if I'm wrong. As far as I know automatic
creation of device links only works with certain props defined in
of_supplier_bindings, right?

So unless I'm wrong there is also a bunch of other stuff that may be
subtly broken. (Again, not a justification to include these changes,
just wondering whether there should be a conscious approach to find and
fix these things... rather than discover them patch-by-patch).

> meson_sm_get is a bit similar - looking by compatible. This is less
> portable and I would prefer the bpmp way (just like syscon phandles).

I have another example (that could be improved via a phandle in DT): For
the Surface System Aggregator (in ACPI-land), we have ssam_client_bind().
This function 1) checks if the controller is available and ready, 2) if
it is gets a reference to it, and 3) establishes a device link for
PM-ordering, before 4) returning the reference to that controller to the
client. This combined with deferring probe ensures that we will always
have a valid reference. And since we're in DT-land, we could hook that
up with a phandle reference to SCM and load that instead of having to
use a global static.

> The qcom_q6v5_pas could be converted later to use similar approach and
> eventually the "tatic struct qcom_scm *__scm;" can be entirely removed.
> 
> Any comments on this approach from Konrad, Bjorn, Dmitry, Vinod and
> anyone else?

Regards,
Max
Maximilian Luz July 26, 2022, 3:15 p.m. UTC | #9
On 7/26/22 16:30, Sudeep Holla wrote:
> On Sun, Jul 24, 2022 at 12:49:49AM +0200, Maximilian Luz wrote:
>> Add bindings for the Qualcomm Trusted Execution Environment (TrEE) UEFI
>> Secure application (uefisecapp) client.
>>

[...]

>> +examples:
>> +  - |
>> +    firmware {
>> +        scm {
>> +            compatible = "qcom,scm-sc8180x", "qcom,scm";
>> +        };
>> +        tee-uefisecapp {
>> +            compatible = "qcom,tee-uefisecapp";
>> +        };
> 
> Do you expect some issues using the scm driver APIs without the
> any additions in the DT ? I mean can't you auto-discover by using the
> APIs. I haven't looked at the driver or any other patches in the series,
> but I would like to know if we can avoid adding any new bindings if it
> can be discovered via those SCM driver APIs.

Not at scale, at least as far as I can tell.

Part of the setup-process of this driver is to query an "application ID"
from a unique string identifying the application (in this case
"qcom.tz.uefisecapp"). If that call fails, we know the app is not there.

But: If we'd want to support more than just "uefisecapp" we'd have to
query each app in some predefined list. As far as I can tell, there's no
method to enumerate all present/loaded ones. The Windows driver seems to
use a hard-coded list of apps that are present on some specific SoC.

It might be possible that there exists such a method, but if it does, the
Windows driver doesn't seem to use it and I don't know about it.

Also, there would need to be at least some type of compatible to
indicate the presence of that TrEE / Secure Application interface used by
uefisecapp. Unless you want to send some potentially unsupported SCM
commands on every platform with qcom,scm and see what comes back.

So ultimately I think it's better to add a DT entry for it. That also
(hopefully) ensures that someone tested and (at least in some way)
validated this. Again, It's a reverse engineered driver.

Regards,
Max
Sudeep Holla July 26, 2022, 3:41 p.m. UTC | #10
On Tue, Jul 26, 2022 at 05:15:41PM +0200, Maximilian Luz wrote:
> 
> So ultimately I think it's better to add a DT entry for it.

I disagree for the reason that once you discover more apps running on the
secure side, you want to add more entries and update DT on the platform
every time you discover some new firmware entity and you wish to interact
with it from the non-secure side.

As along as get this application ID can handle any random name, I prefer
to use that as the discover mechanism and not have this DT.
--
Regards,
Sudeep
Maximilian Luz July 26, 2022, 5:01 p.m. UTC | #11
On 7/26/22 17:41, Sudeep Holla wrote:
> On Tue, Jul 26, 2022 at 05:15:41PM +0200, Maximilian Luz wrote:
>>
>> So ultimately I think it's better to add a DT entry for it.
> 
> I disagree for the reason that once you discover more apps running on the
> secure side, you want to add more entries and update DT on the platform
> every time you discover some new firmware entity and you wish to interact
> with it from the non-secure side.

Just as you'll have to add a driver to the kernel and update whatever is
probing the TrEE interface and add those strings to that interface. If
you then start doing SoC-specific lists, I think you'd be pretty much
re-implementing a DT in the kernel driver...

I don't quite understand why this is a problem. I think per device,
there's a reasonably limited set of apps that we would want to interact
with from the kernel. And for one single device, that set doesn't change
over time. So what's the difference to, say, an I2C device?

> As along as get this application ID can handle any random name, I prefer
> to use that as the discover mechanism and not have this DT.

Apart from the above, some apps must also be loaded from the system. And
those you can't detect: If an app isn't running, it doesn't have an ID
(uefisecapp and the tpm app are loaded by the firmware at boot). Those
are mostly vendor-specific things as far as I can tell, or HDCP stuff.
So you'd need to specify those as firmware somehow, and since (as far as
I can tell) those are signed specifically by/for that vendor and
potentially device (similar to the GPU zap shader or remoteproc
firmware), you'll need to use per-device paths.

That means you either hard-code them in the driver and have a compatible
per model, do DMI matching, or something similar (again, essentially
baking DTs into the kernel driver...), or just store them in the DT
(like we already do for GPU/remoteprocs). While you could hard-code some
known loaded-by-firmware apps and use the DT for others, I think we
should keep everything in the same place.

Windows uses a hard-coded list of supported apps in the driver (to
specify which apps the driver supports) and in addition to that uses
Registry entries (added via .inf files) to specify which app is supposed
to be present on the device and, for apps that need to be loaded, where
the firmware to be loaded is stored. It only ever attempts to connect to
those apps that are marked present in the registry. Which, again, may be
model/vendor specific.

And this is another reason I'm hesitant to use that function: Windows
doesn't use it in this way and that I don't know whether there can be
any subtle breakage or unexpected behavior if we use the function like
that (i.e., who's to say some broken firmware returns "app is present"
but the app is broken?).

Regards,
Max
Krzysztof Kozlowski July 27, 2022, 11:24 a.m. UTC | #12
On 26/07/2022 17:00, Maximilian Luz wrote:
> On 7/26/22 15:25, Krzysztof Kozlowski wrote:
>> On 26/07/2022 13:15, Maximilian Luz wrote:
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: qcom,tee-uefisecapp
>>>>
>>>> Isn't this SoC-specific device? Generic compatibles are usually not
>>>> expected.
>>>
>>> This is essentially software (kernel driver) talking to software (in the
>>> TrustZone), so I don't expect there to be anything SoC specific about it.
>>
>> You are documenting here firmware in TZ (not kernel driver). Isn't this
>> a specific piece which might vary from device to device?
>>
>> IOW, do you expect the same compatible to work for all possible Qualcomm
>> boards (past and future like in 10 years from now)?
> 
> I'm not sure if Qualcomm will still use the "uefisecapp" approach in 10
> years, but I don't expect the interface of uefisecapp to change. The
> interface is modeled after the respective UEFI functions, which are spec
> and thus I don't expect those to change. Also, it seems to have been
> around for a couple of generations and it hasn't changed. The oldest
> tested is sdm850 (Lenovo Yoga C630), and the latest is sc8280xp
> (Thinkpad X13s).

Expectation is not the same as having a specification saying it will not
change.
> 
> Why not make this behave like a "normal" third-party device? If the
> interface ever changes use qcom,tee-uefisecapp-v2 or something like
> that? Again, this does not seem to be directly tied to the SoC.

Such approach is not "normal" for third-party devices. Compatible for
devices has model number. If the block has specification, then v2 would
have sense, otherwise you would invent own versioning...

I would say that firmware implementation can easily change. How much of
your code is tied to it, I don't know, but argument "I don't expect
Qualcomm to change something in their firmware" is not the correct argument.

> 
> Then again, if you prefer to name everything based on
> "qcom,<device>-<soc>" I don't have any strong arguments against it and
> I'm happy to change that. I just think it will unnecessarily introduce
> a bunch of compatibles and doesn't reflect the interface "versioning"
> situation as I see it.

Why bunch? All devices could bind to one specific compatible, as they
are compatible.

> 
>>>>> +
>>>>> +required:
>>>>> +  - compatible
>>>>> +
>>>>> +additionalProperties: false
>>>>> +
>>>>> +examples:
>>>>> +  - |
>>>>> +    firmware {
>>>>> +        scm {
>>>>> +            compatible = "qcom,scm-sc8180x", "qcom,scm";
>>>>> +        };
>>>>> +        tee-uefisecapp {
>>>>> +            compatible = "qcom,tee-uefisecapp";
>>>>
>>>> You did not model here any dependency on SCM. This is not full
>>>> description of the firmware/hardware
>>>
>>> How would I do that? A lot of other stuff also depends on SCM being
>>> present (e.g. qcom_q6v5_pas for loading mdt files) and I don't see them
>>> declare this in the device tree. As far as I can tell, SCM is pretty
>>> much expected to be there at all times (i.e. can't be unloaded) and
>>> drivers check for it when probing via qcom_scm_is_available(),
>>> deferring probe if not.
>>
>> It seems this will be opening a can of worms...
> 
> Indeed.
> 
>> The problem with existing approach is:
>> 1. Lack of any probe ordering or probe deferral support.
>> 2. Lack of any other dependencies, e.g. for PM.
> 
> I'm not entirely sure what you mean by "lack of probe deferral support".
> We have qcom_scm_is_available() and defer probe if that fails. So
> deferral works, unless I'm misunderstanding something.

And how do you differentiate that qcom_scm_is_available() failed because
it is not yet available (defer probe) or it is broken and will never
load? All regular consumer-provider interfaces have it sorted out.

> 
> But yes, correct on the other points.
> 
>> Unloading is "solved" only by disallowing the unload, not by proper
>> device links and module get/put.
>>
>> I understand that SCM must be there, but the same for several other
>> components and for these others we have ways to pass reference around
>> (e.g. syscon regmap, PHYs handles).
>>
>>>
>>> Don't take this as an excuse as in "I want to leave that out", it's just
>>> that I don't know how one would declare such a dependency explicitly. If
>>> you can tell me how to fix it, I'll include that for v2.
>>
>> I think there are no dedicated subsystem helpers for this (like for
>> provider/consumer of resets/power domains/clocks etc), so one way would
>> be something like nvidia,bpmp is doing.
> 
> I assume you're referring to tegra_bpmp_get()? Does this correctly
> handle PM dependencies? At least as far as I can tell it doesn't
> explicitly establish a device link, it only gets a reference to the
> device, which doesn't guarantee the presence of a driver. Nor correct PM
> ordering. Please correct me if I'm wrong. As far as I know automatic
> creation of device links only works with certain props defined in
> of_supplier_bindings, right?

The Tegra choice is not complete, but it implements at least parts of it
and firmware dependencies are modeled in DTS. Other way would be to add
your device as child of SMC firmware and then you do not need bindings
at all...

> 
> So unless I'm wrong there is also a bunch of other stuff that may be
> subtly broken. (Again, not a justification to include these changes,
> just wondering whether there should be a conscious approach to find and
> fix these things... rather than discover them patch-by-patch).
> 
>> meson_sm_get is a bit similar - looking by compatible. This is less
>> portable and I would prefer the bpmp way (just like syscon phandles).
> 
> I have another example (that could be improved via a phandle in DT): For
> the Surface System Aggregator (in ACPI-land), we have ssam_client_bind().
> This function 1) checks if the controller is available and ready, 2) if
> it is gets a reference to it, and 3) establishes a device link for
> PM-ordering, before 4) returning the reference to that controller to the
> client. This combined with deferring probe ensures that we will always
> have a valid reference. And since we're in DT-land, we could hook that
> up with a phandle reference to SCM and load that instead of having to
> use a global static.

Yes, that's better example than Tegra BPMP.

>> The qcom_q6v5_pas could be converted later to use similar approach and
>> eventually the "tatic struct qcom_scm *__scm;" can be entirely removed.
>>
>> Any comments on this approach from Konrad, Bjorn, Dmitry, Vinod and
>> anyone else?
> 
> Regards,
> Max


Best regards,
Krzysztof
Krzysztof Kozlowski July 27, 2022, 11:38 a.m. UTC | #13
On 26/07/2022 19:01, Maximilian Luz wrote:
> On 7/26/22 17:41, Sudeep Holla wrote:
>> On Tue, Jul 26, 2022 at 05:15:41PM +0200, Maximilian Luz wrote:
>>>
>>> So ultimately I think it's better to add a DT entry for it.
>>
>> I disagree for the reason that once you discover more apps running on the
>> secure side, you want to add more entries and update DT on the platform
>> every time you discover some new firmware entity and you wish to interact
>> with it from the non-secure side.
> 
> Just as you'll have to add a driver to the kernel and update whatever is
> probing the TrEE interface and add those strings to that interface. If
> you then start doing SoC-specific lists, I think you'd be pretty much
> re-implementing a DT in the kernel driver...

But you don't have any of these names in the DT either. Your DT node
only indicates the presence of your driver, but does not hold any
additional information like these IDs.

Basically we start modelling firmware components in devicetree. :/

Best regards,
Krzysztof
Maximilian Luz July 27, 2022, 1 p.m. UTC | #14
On 7/27/22 13:24, Krzysztof Kozlowski wrote:
> On 26/07/2022 17:00, Maximilian Luz wrote:
>> On 7/26/22 15:25, Krzysztof Kozlowski wrote:
>>> On 26/07/2022 13:15, Maximilian Luz wrote:
>>>>>> +properties:
>>>>>> +  compatible:
>>>>>> +    const: qcom,tee-uefisecapp
>>>>>
>>>>> Isn't this SoC-specific device? Generic compatibles are usually not
>>>>> expected.
>>>>
>>>> This is essentially software (kernel driver) talking to software (in the
>>>> TrustZone), so I don't expect there to be anything SoC specific about it.
>>>
>>> You are documenting here firmware in TZ (not kernel driver). Isn't this
>>> a specific piece which might vary from device to device?
>>>
>>> IOW, do you expect the same compatible to work for all possible Qualcomm
>>> boards (past and future like in 10 years from now)?
>>
>> I'm not sure if Qualcomm will still use the "uefisecapp" approach in 10
>> years, but I don't expect the interface of uefisecapp to change. The
>> interface is modeled after the respective UEFI functions, which are spec
>> and thus I don't expect those to change. Also, it seems to have been
>> around for a couple of generations and it hasn't changed. The oldest
>> tested is sdm850 (Lenovo Yoga C630), and the latest is sc8280xp
>> (Thinkpad X13s).
> 
> Expectation is not the same as having a specification saying it will not
> change.
>>
>> Why not make this behave like a "normal" third-party device? If the
>> interface ever changes use qcom,tee-uefisecapp-v2 or something like
>> that? Again, this does not seem to be directly tied to the SoC.
> 
> Such approach is not "normal" for third-party devices. Compatible for
> devices has model number. If the block has specification, then v2 would
> have sense, otherwise you would invent own versioning...
> 
> I would say that firmware implementation can easily change. How much of
> your code is tied to it, I don't know, but argument "I don't expect
> Qualcomm to change something in their firmware" is not the correct argument.

Fair points.

>> Then again, if you prefer to name everything based on
>> "qcom,<device>-<soc>" I don't have any strong arguments against it and
>> I'm happy to change that. I just think it will unnecessarily introduce
>> a bunch of compatibles and doesn't reflect the interface "versioning"
>> situation as I see it.
> 
> Why bunch? All devices could bind to one specific compatible, as they
> are compatible.

Ah, I think I misunderstood you there. I thought you were advocating for
creating compatibles for each SoC just because it's a new SoC and things
might be different. I'm not at all against naming this something like
qcom,tee-uefisecapp-sc8180x then using that on all platforms that work.
I just didn't like the idea of having a bunch of different
qcom,tee-uefisecapp-<soc> pointing to the exact same thing without any
difference at all.

>>>>>> +
>>>>>> +required:
>>>>>> +  - compatible
>>>>>> +
>>>>>> +additionalProperties: false
>>>>>> +
>>>>>> +examples:
>>>>>> +  - |
>>>>>> +    firmware {
>>>>>> +        scm {
>>>>>> +            compatible = "qcom,scm-sc8180x", "qcom,scm";
>>>>>> +        };
>>>>>> +        tee-uefisecapp {
>>>>>> +            compatible = "qcom,tee-uefisecapp";
>>>>>
>>>>> You did not model here any dependency on SCM. This is not full
>>>>> description of the firmware/hardware
>>>>
>>>> How would I do that? A lot of other stuff also depends on SCM being
>>>> present (e.g. qcom_q6v5_pas for loading mdt files) and I don't see them
>>>> declare this in the device tree. As far as I can tell, SCM is pretty
>>>> much expected to be there at all times (i.e. can't be unloaded) and
>>>> drivers check for it when probing via qcom_scm_is_available(),
>>>> deferring probe if not.
>>>
>>> It seems this will be opening a can of worms...
>>
>> Indeed.
>>
>>> The problem with existing approach is:
>>> 1. Lack of any probe ordering or probe deferral support.
>>> 2. Lack of any other dependencies, e.g. for PM.
>>
>> I'm not entirely sure what you mean by "lack of probe deferral support".
>> We have qcom_scm_is_available() and defer probe if that fails. So
>> deferral works, unless I'm misunderstanding something.
> 
> And how do you differentiate that qcom_scm_is_available() failed because
> it is not yet available (defer probe) or it is broken and will never
> load? All regular consumer-provider interfaces have it sorted out.

Fair point. By shifting that to device links you'll at least know what
it's waiting for and the driver won't attempt to probe until that's
resolved. But your question applies to that then as well: How do you
differentiate between the device link or supplier being broken somehow
and the supplier being just not ready yet?

>> But yes, correct on the other points.
>>
>>> Unloading is "solved" only by disallowing the unload, not by proper
>>> device links and module get/put.
>>>
>>> I understand that SCM must be there, but the same for several other
>>> components and for these others we have ways to pass reference around
>>> (e.g. syscon regmap, PHYs handles).
>>>
>>>>
>>>> Don't take this as an excuse as in "I want to leave that out", it's just
>>>> that I don't know how one would declare such a dependency explicitly. If
>>>> you can tell me how to fix it, I'll include that for v2.
>>>
>>> I think there are no dedicated subsystem helpers for this (like for
>>> provider/consumer of resets/power domains/clocks etc), so one way would
>>> be something like nvidia,bpmp is doing.
>>
>> I assume you're referring to tegra_bpmp_get()? Does this correctly
>> handle PM dependencies? At least as far as I can tell it doesn't
>> explicitly establish a device link, it only gets a reference to the
>> device, which doesn't guarantee the presence of a driver. Nor correct PM
>> ordering. Please correct me if I'm wrong. As far as I know automatic
>> creation of device links only works with certain props defined in
>> of_supplier_bindings, right?
> 
> The Tegra choice is not complete, but it implements at least parts of it
> and firmware dependencies are modeled in DTS. Other way would be to add
> your device as child of SMC firmware and then you do not need bindings
> at all...

Looking at the TrEE driver in [1] and thinking of future additions
(re-entrant calls, callbacks/listeners, ..., all of which would require
some state), combining both might be the best option: Have a TrEE main
device for the interface that links up with SCM and then define the apps
as children under that TrEE device.

Regards,
Max

[1]: https://git.codelinaro.org/clo/la/kernel/msm-4.14/-/blob/auto-kernel.lnx.4.14.c34/drivers/misc/qseecom.c
Maximilian Luz July 27, 2022, 1:03 p.m. UTC | #15
On 7/27/22 13:38, Krzysztof Kozlowski wrote:
> On 26/07/2022 19:01, Maximilian Luz wrote:
>> On 7/26/22 17:41, Sudeep Holla wrote:
>>> On Tue, Jul 26, 2022 at 05:15:41PM +0200, Maximilian Luz wrote:
>>>>
>>>> So ultimately I think it's better to add a DT entry for it.
>>>
>>> I disagree for the reason that once you discover more apps running on the
>>> secure side, you want to add more entries and update DT on the platform
>>> every time you discover some new firmware entity and you wish to interact
>>> with it from the non-secure side.
>>
>> Just as you'll have to add a driver to the kernel and update whatever is
>> probing the TrEE interface and add those strings to that interface. If
>> you then start doing SoC-specific lists, I think you'd be pretty much
>> re-implementing a DT in the kernel driver...
> 
> But you don't have any of these names in the DT either. Your DT node
> only indicates the presence of your driver, but does not hold any
> additional information like these IDs.

Because the compatible implicates the ID-string which implicates the driver
interface. If the ID-string for uefisecapp would be different we'd very likely
need a different driver for that as well, meaning a new compatible too. I
thought it would be superfluous to put that in the DT.
  
> Basically we start modelling firmware components in devicetree. :/

Is there really a good way around it? As far as I can see the
alternative (especially for the apps that need to be loaded manually) is
hard-coding everything in the driver. Which IMHO just spreads device
specific information everywhere.

Also: Let's use the TPM app as example. If that would be a SPI or I2C
device, you'd model it in the DT. Just because it's a hardware device
that's accessible via SCM/firmware you now don't?

If I were absolutely certain that there is a reliable mechanism to
detect these apps, I'd agree with having a driver to instantiate those
devices. But I am not.

Regards,
Max
Sudeep Holla July 27, 2022, 1:24 p.m. UTC | #16
On Wed, Jul 27, 2022 at 03:03:49PM +0200, Maximilian Luz wrote:
>
> Is there really a good way around it?

Yes rely on the firmware preferably auto discover, if that is not an option,
how about query. It seem to be working in your case.

> As far as I can see the alternative (especially for the apps that
> need to be loaded manually) is hard-coding everything in the driver.
> Which IMHO just spreads device specific information everywhere.
>

It may not be too bad compared to putting loads of firmware details
in the DT. What happens if you get a firmware upgrade with changed
number of firmware entities or even if the names are changed.

Are these name user ABI in a way that they won't be changed ? Generally
these entities tend to use UUID and the name you have might get changed.

I would ideally prefer even the name to be supplied from the userspace.
In this particular case, make this a driver and have the name as the
parameter. If the secure side services are used by some non-secure
applications, then you will need to have a user-interface which means
you can get the named from the userspace. No need to change the driver
in either case. Please let me know if I am missing anything to consider
here.

> Also: Let's use the TPM app as example. If that would be a SPI or I2C
> device, you'd model it in the DT. Just because it's a hardware device
> that's accessible via SCM/firmware you now don't?
>

Not sure if I understand the comparison here. But if there is some device
that is access restricted but needs to be accessed and has mechanism to
access, then you would model it as device in DT.

But the one $subject is addressing looks pure software and doesn't make
sense to model in DT IMO.

> If I were absolutely certain that there is a reliable mechanism to
> detect these apps, I'd agree with having a driver to instantiate those
> devices. But I am not.
>

You did say you use some query API to check this. I haven't seen the driver,
so relying on what you said earlier.

--
Regards,
Sudeep
Maximilian Luz July 27, 2022, 2:49 p.m. UTC | #17
On 7/27/22 15:24, Sudeep Holla wrote:
> On Wed, Jul 27, 2022 at 03:03:49PM +0200, Maximilian Luz wrote:
>>
>> Is there really a good way around it?
> 
> Yes rely on the firmware preferably auto discover, if that is not an option,
> how about query. It seem to be working in your case.
> 
>> As far as I can see the alternative (especially for the apps that
>> need to be loaded manually) is hard-coding everything in the driver.
>> Which IMHO just spreads device specific information everywhere.
>>
> 
> It may not be too bad compared to putting loads of firmware details
> in the DT. What happens if you get a firmware upgrade with changed
> number of firmware entities or even if the names are changed.
> 
> Are these name user ABI in a way that they won't be changed ? Generally
> these entities tend to use UUID and the name you have might get changed.

I am pretty certain that these names do not change for a device once it's
been released. The full ID of the uefisecapp is "qcom.tz.uefisecapp". The
built-in firmware parts here are core components. So I really do not expect
them to just remove or rename things. If they would do that, that would
mean that, on Windows, access to things like the TPM or UEFI variables
would be broken if both the driver and Registry are not updated in parallel
with the firmware. So while I can't myself guarantee that this is a stable
name and interface, it's very much in MS/Qualcomm's interest to keep it
stable.

Also, I'm not advocating on putting loads of details in the DT. I'm (in
this series) advocating for a DT compatible that says "this device stores
EFI variables via that firmware interface". I'd be very surprised if
MS/Qualcomm suddenly decided to change that out for another interface,
potentially breaking their own software and devices.

> I would ideally prefer even the name to be supplied from the userspace.
> In this particular case, make this a driver and have the name as the
> parameter. If the secure side services are used by some non-secure
> applications, then you will need to have a user-interface which means
> you can get the named from the userspace. No need to change the driver
> in either case. Please let me know if I am missing anything to consider
> here.

 From userspace? For access to EFI variables and (hopefully in the future
if I've managed to reverse-engineer that) the TPM? Those are things that
should work out-of-the-box and not require the user to first have to
configure something... Also, those are things that the kernel might want
to use (e.g. EFI variables as pstore for crashdumps) before the user is
even able to configure something (unless we now want to specify things
on the kernel command line...).

If this were something that only userspace would use then sure, let
userspace load it and do all the work. But it isn't.

> 
>> Also: Let's use the TPM app as example. If that would be a SPI or I2C
>> device, you'd model it in the DT. Just because it's a hardware device
>> that's accessible via SCM/firmware you now don't?
>>
> 
> Not sure if I understand the comparison here. But if there is some device
> that is access restricted but needs to be accessed and has mechanism to
> access, then you would model it as device in DT.
> 
> But the one $subject is addressing looks pure software and doesn't make
> sense to model in DT IMO.

So as soon as access runs via some firmware mechanism, it should not be
in the DT? The TPM in the example above would also be accessed via some
firmware API. EFI variables are stored on some SPI flash that is managed
by the TrustZone. So in both cases kernel calls to firmware calls to
device. Where do you draw the line?

>> If I were absolutely certain that there is a reliable mechanism to
>> detect these apps, I'd agree with having a driver to instantiate those
>> devices. But I am not.
>>
> 
> You did say you use some query API to check this. I haven't seen the driver,
> so relying on what you said earlier.

I did say that there is an API that turns a unique identifying string ID
of a secure application into a runtime-dependent integer ID of the
running application, returning an error if the application is not
running. I very much doubt that is supposed to be used for checking
support of certain applications. It could _maybe_ be used that way, but
the Windows driver doesn't, which makes me not very comfortable doing
that either.

Further: As far as I can tell, there is also no way of checking whether
that lookup failure is due to the application not being present or whether
something internal to the firmware failed. the respective results that the
call can (as far as I can tell) return are:

	QCTEE_OS_RESULT_SUCCESS			= 0,
	QCTEE_OS_RESULT_INCOMPLETE		= 1,
	QCTEE_OS_RESULT_BLOCKED_ON_LISTENER	= 2,
	QCTEE_OS_RESULT_FAILURE			= 0xFFFFFFFF,

And it will return QCTEE_OS_RESULT_FAILURE when the app name is wrong.

Again, while it _might_ be possible to use that, I don't think it makes a
very sound approach and I would really prefer not using it in that way.

Regards,
Max
Ilias Apalodimas July 28, 2022, 6:03 a.m. UTC | #18
Hi all,

On Wed, 27 Jul 2022 at 16:24, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Wed, Jul 27, 2022 at 03:03:49PM +0200, Maximilian Luz wrote:
> >
> > Is there really a good way around it?
>
> Yes rely on the firmware preferably auto discover, if that is not an option,
> how about query. It seem to be working in your case.

That's a good point.  We have a similar situation with some Arm
devices and U-Boot.  Let me try to explain a bit.

There's code plugged in in OP-TEE and U-Boot atm which allows you to
store EFI variables on an RPMB.  This is a nice alternative if your
device doesn't have any other secure storage,  however it presents
some challenges after ExitBootServices, similar to the ones you have
here.

The eMMC controller usually lives in the non-secure world.  OP-TEE
can't access that, so it relies on a userspace supplicant to perform
the RPMB accesses.  That supplicant is present in U-Boot and
Get/SetVariable works fine before ExitBootServices.  Once Linux boots,
 the 'U-Boot supplicant' goes away and we launch the linux equivalent
one from userspace.  Since variable accessing is a runtime service and
it still has to go through the firmware we can't use those anymore
since U-Boot doesn't preserve the supplicant, the eMMC driver and the
OP-TEE portions needed in the runtime section(and even if it did we
would now have 2 drivers racing to access the same hardware).  Instead
U-Boot copies the variables in runtime memory and
GetVariable/GetNextVariable still works, but SetVariable returns
EFI_UNSUPPORTED.

I've spent enough time looking at available solutions and although
this indeed breaks the EFI spec, something along the lines of
replacing the runtime services with ones that give you direct access
to the secure world, completely bypassing the firmware is imho our
least bad option.

I have an ancient branch somewhere that I can polish up and send an
RFC [1],  but the way I enabled that was to install an empty config
table from the firmware.  That empty table is basically an indication
to the kernel saying "Hey I can't store variables, can you do that for
me".

Is there any chance we can do something similar on that device (or
find a reasonable way of inferring that we need to replace some
services).  That way we could at least have a common entry point to
the kernel and leave out the DT changes.

[1] https://git.linaro.org/people/ilias.apalodimas/net-next.git/log/?h=setvar_rt_optee_3

Thanks
/Ilias

>
> > As far as I can see the alternative (especially for the apps that
> > need to be loaded manually) is hard-coding everything in the driver.
> > Which IMHO just spreads device specific information everywhere.
> >
>
> It may not be too bad compared to putting loads of firmware details
> in the DT. What happens if you get a firmware upgrade with changed
> number of firmware entities or even if the names are changed.
>
> Are these name user ABI in a way that they won't be changed ? Generally
> these entities tend to use UUID and the name you have might get changed.
>
> I would ideally prefer even the name to be supplied from the userspace.
> In this particular case, make this a driver and have the name as the
> parameter. If the secure side services are used by some non-secure
> applications, then you will need to have a user-interface which means
> you can get the named from the userspace. No need to change the driver
> in either case. Please let me know if I am missing anything to consider
> here.
>
> > Also: Let's use the TPM app as example. If that would be a SPI or I2C
> > device, you'd model it in the DT. Just because it's a hardware device
> > that's accessible via SCM/firmware you now don't?
> >
>
> Not sure if I understand the comparison here. But if there is some device
> that is access restricted but needs to be accessed and has mechanism to
> access, then you would model it as device in DT.
>
> But the one $subject is addressing looks pure software and doesn't make
> sense to model in DT IMO.
>
> > If I were absolutely certain that there is a reliable mechanism to
> > detect these apps, I'd agree with having a driver to instantiate those
> > devices. But I am not.
> >
>
> You did say you use some query API to check this. I haven't seen the driver,
> so relying on what you said earlier.
>
> --
> Regards,
> Sudeep
Krzysztof Kozlowski July 28, 2022, 7:48 a.m. UTC | #19
On 27/07/2022 15:00, Maximilian Luz wrote:
>>> Then again, if you prefer to name everything based on
>>> "qcom,<device>-<soc>" I don't have any strong arguments against it and
>>> I'm happy to change that. I just think it will unnecessarily introduce
>>> a bunch of compatibles and doesn't reflect the interface "versioning"
>>> situation as I see it.
>>
>> Why bunch? All devices could bind to one specific compatible, as they
>> are compatible.
> 
> Ah, I think I misunderstood you there. I thought you were advocating for
> creating compatibles for each SoC just because it's a new SoC and things
> might be different. I'm not at all against naming this something like
> qcom,tee-uefisecapp-sc8180x then using that on all platforms that work.
> I just didn't like the idea of having a bunch of different
> qcom,tee-uefisecapp-<soc> pointing to the exact same thing without any
> difference at all.

You start with one specific compatible and if needed later either add
more specific upfront (qcom,sc8280x-tee-uefisecapp,
qcom,sc8180x-tee-uefisecapp) or as entirely new one if it is not compatible.

> 
>>>>>>> +
>>>>>>> +required:
>>>>>>> +  - compatible
>>>>>>> +
>>>>>>> +additionalProperties: false
>>>>>>> +
>>>>>>> +examples:
>>>>>>> +  - |
>>>>>>> +    firmware {
>>>>>>> +        scm {
>>>>>>> +            compatible = "qcom,scm-sc8180x", "qcom,scm";
>>>>>>> +        };
>>>>>>> +        tee-uefisecapp {
>>>>>>> +            compatible = "qcom,tee-uefisecapp";
>>>>>>
>>>>>> You did not model here any dependency on SCM. This is not full
>>>>>> description of the firmware/hardware
>>>>>
>>>>> How would I do that? A lot of other stuff also depends on SCM being
>>>>> present (e.g. qcom_q6v5_pas for loading mdt files) and I don't see them
>>>>> declare this in the device tree. As far as I can tell, SCM is pretty
>>>>> much expected to be there at all times (i.e. can't be unloaded) and
>>>>> drivers check for it when probing via qcom_scm_is_available(),
>>>>> deferring probe if not.
>>>>
>>>> It seems this will be opening a can of worms...
>>>
>>> Indeed.
>>>
>>>> The problem with existing approach is:
>>>> 1. Lack of any probe ordering or probe deferral support.
>>>> 2. Lack of any other dependencies, e.g. for PM.
>>>
>>> I'm not entirely sure what you mean by "lack of probe deferral support".
>>> We have qcom_scm_is_available() and defer probe if that fails. So
>>> deferral works, unless I'm misunderstanding something.
>>
>> And how do you differentiate that qcom_scm_is_available() failed because
>> it is not yet available (defer probe) or it is broken and will never
>> load? All regular consumer-provider interfaces have it sorted out.
> 
> Fair point. By shifting that to device links you'll at least know what
> it's waiting for and the driver won't attempt to probe until that's
> resolved. But your question applies to that then as well: How do you
> differentiate between the device link or supplier being broken somehow
> and the supplier being just not ready yet?

For example like tegra_bpmp_get() is doing.

Best regards,
Krzysztof
Sudeep Holla July 28, 2022, 8:23 a.m. UTC | #20
On Tue, Jul 26, 2022 at 07:01:28PM +0200, Maximilian Luz wrote:
> On 7/26/22 17:41, Sudeep Holla wrote:
> > On Tue, Jul 26, 2022 at 05:15:41PM +0200, Maximilian Luz wrote:
> > >
> > > So ultimately I think it's better to add a DT entry for it.
> >
> > I disagree for the reason that once you discover more apps running on the
> > secure side, you want to add more entries and update DT on the platform
> > every time you discover some new firmware entity and you wish to interact
> > with it from the non-secure side.
>
> Just as you'll have to add a driver to the kernel and update whatever is
> probing the TrEE interface and add those strings to that interface. If
> you then start doing SoC-specific lists, I think you'd be pretty much
> re-implementing a DT in the kernel driver...
>

Yes at the cost of DT being dumping ground for all the SoC specific firmware
crap. Firmware can be and must be discoverable, no point in dumping it in
DT as it forces DT upgrade every time something changes in the firmware i.e.
it can go out of sync quite quickly.

> I don't quite understand why this is a problem. I think per device,
> there's a reasonably limited set of apps that we would want to interact
> with from the kernel. And for one single device, that set doesn't change
> over time. So what's the difference to, say, an I2C device?
>

As I said we don't want DT to be dumping ground for all the not well designed
firmware interface. The whole point of firmware being another piece of
software that can be change unlike hardware makes it fragile to present any
more that what you need in the DT. I see this as one of the example.

Anyways I don't have the final say, I leave it to the DT maintainers.

> > As along as get this application ID can handle any random name, I prefer
> > to use that as the discover mechanism and not have this DT.
>
> Apart from the above, some apps must also be loaded from the system. And
> those you can't detect: If an app isn't running, it doesn't have an ID
> (uefisecapp and the tpm app are loaded by the firmware at boot). Those
> are mostly vendor-specific things as far as I can tell, or HDCP stuff.
> So you'd need to specify those as firmware somehow, and since (as far as
> I can tell) those are signed specifically by/for that vendor and
> potentially device (similar to the GPU zap shader or remoteproc
> firmware), you'll need to use per-device paths.
>

Sounds to me like more can be pushed to user space as it gets loaded at
runtime.

> That means you either hard-code them in the driver and have a compatible
> per model, do DMI matching, or something similar (again, essentially
> baking DTs into the kernel driver...), or just store them in the DT
> (like we already do for GPU/remoteprocs). While you could hard-code some
> known loaded-by-firmware apps and use the DT for others, I think we
> should keep everything in the same place.
>

Worst case I am fine with that as this needs to be one of and future
platforms must get their act right in designing their f/w interface.

--
Regards,
Sudeep
Maximilian Luz July 28, 2022, 10:05 a.m. UTC | #21
On 7/28/22 10:23, Sudeep Holla wrote:
> On Tue, Jul 26, 2022 at 07:01:28PM +0200, Maximilian Luz wrote:
>> On 7/26/22 17:41, Sudeep Holla wrote:
>>> On Tue, Jul 26, 2022 at 05:15:41PM +0200, Maximilian Luz wrote:
>>>>
>>>> So ultimately I think it's better to add a DT entry for it.
>>>
>>> I disagree for the reason that once you discover more apps running on the
>>> secure side, you want to add more entries and update DT on the platform
>>> every time you discover some new firmware entity and you wish to interact
>>> with it from the non-secure side.
>>
>> Just as you'll have to add a driver to the kernel and update whatever is
>> probing the TrEE interface and add those strings to that interface. If
>> you then start doing SoC-specific lists, I think you'd be pretty much
>> re-implementing a DT in the kernel driver...
>>
> 
> Yes at the cost of DT being dumping ground for all the SoC specific firmware
> crap. Firmware can be and must be discoverable, no point in dumping it in
> DT as it forces DT upgrade every time something changes in the firmware i.e.
> it can go out of sync quite quickly.

I fully agree with you here on the design level. Firmware _should_ be
discoverable. Unfortunately, in this case it really isn't.

Again, in Windows, this information is stored via the Registry and set
when the driver is installed. An example:

     ; UEFIVAR SECURE APP SERVICE
     HKR,%EFIVarService.RegKey%,Enabled,%REG_DWORD%,1
     HKR,%EFIVarService.RegKey%,MajorVersion,%REG_DWORD%,1
     HKR,%EFIVarService.RegKey%,MinorVersion,%REG_DWORD%,0
     
     ; WINSECAPP SECURE APP SERVICE
     HKR,%WinSecAppService.RegKey%,Enabled,%REG_DWORD%,1
     HKR,%WinSecAppService.RegKey%,SecureApp,%REG_DWORD%,1
     HKR,%WinSecAppService.RegKey%,LoadApp,%REG_DWORD%,0
     HKR,%WinSecAppService.RegKey%,AppName,,"qcom.tz.winsecapp"
     HKR,%WinSecAppService.RegKey%,MajorVersion,%REG_DWORD%,1
     HKR,%WinSecAppService.RegKey%,MinorVersion,%REG_DWORD%,0
     HKR,%WinSecAppService.RegKey%,OSDependencies,%REG_MULTI_SZ%,%RpmbOsService%
     
     ; HDCP v2.2 SECURE APP SERVICE
     HKR,%Hdcp2p2Service.RegKey%,Enabled,%REG_DWORD%,1
     HKR,%Hdcp2p2Service.RegKey%,SecureApp,%REG_DWORD%,1
     HKR,%Hdcp2p2Service.RegKey%,LoadApp,%REG_DWORD%,1
     HKR,%Hdcp2p2Service.RegKey%,AppName,,"qcom.tz.hdcp2p2"
     HKR,%Hdcp2p2Service.RegKey%,FileName,,"hdcp2p2.mbn"
     HKR,%Hdcp2p2Service.RegKey%,MajorVersion,%REG_DWORD%,1
     HKR,%Hdcp2p2Service.RegKey%,MinorVersion,%REG_DWORD%,0
     HKR,%Hdcp2p2Service.RegKey%,OSDependencies,%REG_MULTI_SZ%,%RpmbOsService%,%TzAppsOsService%

The '.RegKey' contains a GUID that specifies the _driver_ interface that
is registered by the driver to the kernel (i.e. is not related to the
specific firmware and firmware version), e.g. [1]. For uefisecapp, the
driver also maps this GUID to the name-string.

[1]: https://github.com/tpn/winsdk-10/blob/9b69fd26ac0c7d0b83d378dba01080e93349c2ed/Include/10.0.16299.0/km/treevariableservice.h#L35

>> I don't quite understand why this is a problem. I think per device,
>> there's a reasonably limited set of apps that we would want to interact
>> with from the kernel. And for one single device, that set doesn't change
>> over time. So what's the difference to, say, an I2C device?
>>
> 
> As I said we don't want DT to be dumping ground for all the not well designed
> firmware interface. The whole point of firmware being another piece of
> software that can be change unlike hardware makes it fragile to present any
> more that what you need in the DT. I see this as one of the example.

I can see your point. But this interface has apparently been around
since at least sdm850 (e.g. Lenovo C630) and hasn't changed. As I've
argued elsewhere: All parties involved have a vested interest that this
interface doesn't change in a breaking way. The interface is modeled
similar to syscalls, so I very much expect them to extend it if needed,
instead of changing/breaking it.

Sure, it _could_ be changed in a breaking way. But again, I believe that
to be _very_ unlikely.

> Anyways I don't have the final say, I leave it to the DT maintainers.
> 
>>> As along as get this application ID can handle any random name, I prefer
>>> to use that as the discover mechanism and not have this DT.
>>
>> Apart from the above, some apps must also be loaded from the system. And
>> those you can't detect: If an app isn't running, it doesn't have an ID
>> (uefisecapp and the tpm app are loaded by the firmware at boot). Those
>> are mostly vendor-specific things as far as I can tell, or HDCP stuff.
>> So you'd need to specify those as firmware somehow, and since (as far as
>> I can tell) those are signed specifically by/for that vendor and
>> potentially device (similar to the GPU zap shader or remoteproc
>> firmware), you'll need to use per-device paths.
>>
> 
> Sounds to me like more can be pushed to user space as it gets loaded at
> runtime.

If we have user-space available at the time when these things should be
loaded or if they are more or less optional, sure.

>> That means you either hard-code them in the driver and have a compatible
>> per model, do DMI matching, or something similar (again, essentially
>> baking DTs into the kernel driver...), or just store them in the DT
>> (like we already do for GPU/remoteprocs). While you could hard-code some
>> known loaded-by-firmware apps and use the DT for others, I think we
>> should keep everything in the same place.
>>
> 
> Worst case I am fine with that as this needs to be one of and future
> platforms must get their act right in designing their f/w interface.

Again, I fully agree with you that this situation shouldn't exist. But
reality is sadly different.

Regards,
Max
Maximilian Luz July 28, 2022, 10:25 a.m. UTC | #22
On 7/28/22 09:48, Krzysztof Kozlowski wrote:

[...]

>>>>> The problem with existing approach is:
>>>>> 1. Lack of any probe ordering or probe deferral support.
>>>>> 2. Lack of any other dependencies, e.g. for PM.
>>>>
>>>> I'm not entirely sure what you mean by "lack of probe deferral support".
>>>> We have qcom_scm_is_available() and defer probe if that fails. So
>>>> deferral works, unless I'm misunderstanding something.
>>>
>>> And how do you differentiate that qcom_scm_is_available() failed because
>>> it is not yet available (defer probe) or it is broken and will never
>>> load? All regular consumer-provider interfaces have it sorted out.
>>
>> Fair point. By shifting that to device links you'll at least know what
>> it's waiting for and the driver won't attempt to probe until that's
>> resolved. But your question applies to that then as well: How do you
>> differentiate between the device link or supplier being broken somehow
>> and the supplier being just not ready yet?
> 
> For example like tegra_bpmp_get() is doing.

But tegra_bpmp_get() can also not differentiate whether the supplier driver is
ever going to be successfully probed or not. I'm not sure you can ever really
solve that. The only thing it does in addition is check whether the phandle and
device is there. Or do you mean those not being present by "broken"? That's a
point I agree should be improved with SCM.

Regards,
Max
Krzysztof Kozlowski July 28, 2022, 10:38 a.m. UTC | #23
On 28/07/2022 12:25, Maximilian Luz wrote:
> On 7/28/22 09:48, Krzysztof Kozlowski wrote:
> 
> [...]
> 
>>
>> For example like tegra_bpmp_get() is doing.
> 
> But tegra_bpmp_get() can also not differentiate whether the supplier driver is
> ever going to be successfully probed or not. I'm not sure you can ever really
> solve that. The only thing it does in addition is check whether the phandle and
> device is there. Or do you mean those not being present by "broken"? That's a
> point I agree should be improved with SCM.

Yes, at least it checks if phandles points to proper device and device
is there. That's what we want.

We are not solving here case of providing being in a module which never
gets loaded (thus endless EPROBE_DEFER). Such case is ok.

Best regards,
Krzysztof
Maximilian Luz July 28, 2022, 10:48 a.m. UTC | #24
On 7/28/22 08:03, Ilias Apalodimas wrote:
> Hi all,
> 
> On Wed, 27 Jul 2022 at 16:24, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>
>> On Wed, Jul 27, 2022 at 03:03:49PM +0200, Maximilian Luz wrote:
>>>
>>> Is there really a good way around it?
>>
>> Yes rely on the firmware preferably auto discover, if that is not an option,
>> how about query. It seem to be working in your case.
> 
> That's a good point.  We have a similar situation with some Arm
> devices and U-Boot.  Let me try to explain a bit.
> 
> There's code plugged in in OP-TEE and U-Boot atm which allows you to
> store EFI variables on an RPMB.  This is a nice alternative if your
> device doesn't have any other secure storage,  however it presents
> some challenges after ExitBootServices, similar to the ones you have
> here.
> 
> The eMMC controller usually lives in the non-secure world.  OP-TEE
> can't access that, so it relies on a userspace supplicant to perform
> the RPMB accesses.  That supplicant is present in U-Boot and
> Get/SetVariable works fine before ExitBootServices.  Once Linux boots,
>   the 'U-Boot supplicant' goes away and we launch the linux equivalent
> one from userspace.  Since variable accessing is a runtime service and
> it still has to go through the firmware we can't use those anymore
> since U-Boot doesn't preserve the supplicant, the eMMC driver and the
> OP-TEE portions needed in the runtime section(and even if it did we
> would now have 2 drivers racing to access the same hardware).  Instead
> U-Boot copies the variables in runtime memory and
> GetVariable/GetNextVariable still works, but SetVariable returns
> EFI_UNSUPPORTED.
> 
> I've spent enough time looking at available solutions and although
> this indeed breaks the EFI spec, something along the lines of
> replacing the runtime services with ones that give you direct access
> to the secure world, completely bypassing the firmware is imho our
> least bad option.

This sounds very similar to what Qualcomm may be doing on some devices.
The TrEE interface allows for callbacks and there are indications that
one such callback-service is for RPMB. I believe that at least on some
platforms, Qualcomm also stores UEFI variables in RPMB and uses the same
uefisecapp interface in combination with RPMB listeners installed by the
kernel to access them.

> I have an ancient branch somewhere that I can polish up and send an
> RFC [1],  but the way I enabled that was to install an empty config
> table from the firmware.  That empty table is basically an indication
> to the kernel saying "Hey I can't store variables, can you do that for
> me".
> 
> Is there any chance we can do something similar on that device (or
> find a reasonable way of inferring that we need to replace some
> services).  That way we could at least have a common entry point to
> the kernel and leave out the DT changes.
> 
> [1] https://git.linaro.org/people/ilias.apalodimas/net-next.git/log/?h=setvar_rt_optee_3

I would very much like to avoid the need for special bootloaders. The
devices we're talking about are WoA devices, meaning they _should_
ideally boot just fine with EFI and ACPI.

 From an end-user perspective, it's annoying enough that we'll have to
stick with DTs for the time being due to the use of PEPs in ACPI. I
really don't want to add some special bootloader for fixups to that.
Also, this would just move the problem from kernel to bootloader.

If you have any suggestions for another way of detecting this, please
feel free to share. I, unfortunately, don't.

Regards,
Max
Maximilian Luz July 28, 2022, 10:49 a.m. UTC | #25
On 7/28/22 12:38, Krzysztof Kozlowski wrote:
> On 28/07/2022 12:25, Maximilian Luz wrote:
>> On 7/28/22 09:48, Krzysztof Kozlowski wrote:
>>
>> [...]
>>
>>>
>>> For example like tegra_bpmp_get() is doing.
>>
>> But tegra_bpmp_get() can also not differentiate whether the supplier driver is
>> ever going to be successfully probed or not. I'm not sure you can ever really
>> solve that. The only thing it does in addition is check whether the phandle and
>> device is there. Or do you mean those not being present by "broken"? That's a
>> point I agree should be improved with SCM.
> 
> Yes, at least it checks if phandles points to proper device and device
> is there. That's what we want.
> 
> We are not solving here case of providing being in a module which never
> gets loaded (thus endless EPROBE_DEFER). Such case is ok.

Got it, thanks for that clarification!

Regards,
Max
Sudeep Holla July 28, 2022, 11:21 a.m. UTC | #26
On Thu, Jul 28, 2022 at 12:05:15PM +0200, Maximilian Luz wrote:
> On 7/28/22 10:23, Sudeep Holla wrote:

[...]

> > Worst case I am fine with that as this needs to be one of and future
> > platforms must get their act right in designing their f/w interface.
>
> Again, I fully agree with you that this situation shouldn't exist. But
> reality is sadly different.
>

As I mentioned I don't have final authority to say yes or no to DT bindings.
I have expressed my opinion and I thing allowing this to be generic via DT
bindings gives no incentive to get the firmware story right. Hence I am happy
to see this as one-off driver change and then we more changes are added to
the driver or similar drivers get added in the future, we have a change to
demand what action has been taken to fix the firmware story.

Just adding DT support(which I disagree) will make future platform to just
use it and not get improvements in areas of discovery or query from the
firmware.

--
Regards,
Sudeep
Sudeep Holla July 28, 2022, 11:33 a.m. UTC | #27
On Thu, Jul 28, 2022 at 12:48:19PM +0200, Maximilian Luz wrote:

[...]

>
> I would very much like to avoid the need for special bootloaders. The
> devices we're talking about are WoA devices, meaning they _should_
> ideally boot just fine with EFI and ACPI.
>

Completely agreed.

> From an end-user perspective, it's annoying enough that we'll have to
> stick with DTs for the time being due to the use of PEPs in ACPI.

But have we explored or investigated what it takes to rewrite ACPI f/w
to just use standard methods ? Does it require more firmware changes or
new firmware entities or impossible at any cost ?

For me that is more important than just getting this one on DT. Because
if you take that path, we will have to keep doing that, with loads of
unnecessary drivers if they are not shared with any other SoC with DT
support upstream. We might also miss chance to get things added to the ACPI
spec as we don't care which means that we never be able to use ACPI on
similar future platforms even though they get shipped with ACPI.

It will be a loop where we constantly keep converting this ACPI shipped
platform into DT upstream. IMHO we don't want to be there.

--
Regards,
Sudeep
Maximilian Luz July 28, 2022, 11:45 a.m. UTC | #28
On 7/28/22 13:21, Sudeep Holla wrote:
> On Thu, Jul 28, 2022 at 12:05:15PM +0200, Maximilian Luz wrote:
>> On 7/28/22 10:23, Sudeep Holla wrote:
> 
> [...]
> 
>>> Worst case I am fine with that as this needs to be one of and future
>>> platforms must get their act right in designing their f/w interface.
>>
>> Again, I fully agree with you that this situation shouldn't exist. But
>> reality is sadly different.
>>
> 
> As I mentioned I don't have final authority to say yes or no to DT bindings.
> I have expressed my opinion and I thing allowing this to be generic via DT
> bindings gives no incentive to get the firmware story right. Hence I am happy
> to see this as one-off driver change and then we more changes are added to
> the driver or similar drivers get added in the future, we have a change to
> demand what action has been taken to fix the firmware story.
> 
> Just adding DT support(which I disagree) will make future platform to just
> use it and not get improvements in areas of discovery or query from the
> firmware.

Okay, that is a good point. Although it's probably debatable how much
control we have over what goes on with WoA devices.

Would something like this work for you: Add a compatible for the TrEE
interface (e.g. qcom,sc8180x-tee) but not for the specific apps running
in that and then instantiate the app-specific sub-devices from that. We
would still have to hard-code app-names in the driver (i.e. shift the
problem from DT to driver and potentially create soc-specific lists),
but they're no longer in DT (again, I'm not a particular fan of this but
I could live with that, if needed).

We can then look for a solution for apps that need to be manually loaded
or vendor/device specific apps once those becomes an issue.

Regards,
Max
Maximilian Luz July 28, 2022, 12:13 p.m. UTC | #29
On 7/28/22 13:33, Sudeep Holla wrote:
> On Thu, Jul 28, 2022 at 12:48:19PM +0200, Maximilian Luz wrote:
> 
> [...]
> 
>>
>> I would very much like to avoid the need for special bootloaders. The
>> devices we're talking about are WoA devices, meaning they _should_
>> ideally boot just fine with EFI and ACPI.
>>
> 
> Completely agreed.
> 
>>  From an end-user perspective, it's annoying enough that we'll have to
>> stick with DTs for the time being due to the use of PEPs in ACPI.
> 
> But have we explored or investigated what it takes to rewrite ACPI f/w
> to just use standard methods ? Does it require more firmware changes or
> new firmware entities or impossible at any cost ?

Again, I'm not a Qualcomm employee. I would prefer it they'd use
standard methods in the future. Rewriting the ACPI tables based on the
information that we have is probably possible, but we'd again have to do
this on a device-by-device basis, so why not just write a DT instead?

Again, I'm not a Qualcomm employee. I would prefer it they'd use
standard methods in the future. I cannot say why they are using PEPs and
whether they can't just use something "normal". Rewriting the ACPI
tables based on the information that we have is probably possible, but
we'd again have to do this manually, on a device-by-device basis. So why
not just write a DT instead?

Apart from that they also unfortunately hard-code a lot of SoC specific
MMIO addresses into their drivers, so, for each SoC, they essentially
have their own ACPI HID even if the specific hardware interface hasn't
changed. It's bad all around... and I don't like it one bit either.

> For me that is more important than just getting this one on DT. Because
> if you take that path, we will have to keep doing that, with loads of
> unnecessary drivers if they are not shared with any other SoC with DT
> support upstream. We might also miss chance to get things added to the ACPI
> spec as we don't care which means that we never be able to use ACPI on
> similar future platforms even though they get shipped with ACPI.
> 
> It will be a loop where we constantly keep converting this ACPI shipped
> platform into DT upstream. IMHO we don't want to be there.

I fully agree with that. And that is also something that I fear.

Unfortunately, the only way out that I can see is either Qualcomm
changing its ways or us supporting ACPI PEPs, doing hard-coded register
addresses based on ACPI HIDs, and converting a lot of existing drivers
written for DT/OF to support ACPI. I personally would prefer if we'd do
all that and hope that we can one day support PEPs.

Once we do, we'd at least "only" have to add the needed MMIO definitions
for drivers via HID matches and write a PEP driver for that specific SoC
(which would then be similar to regulator or clock controllers). Still
some work but a lot less than having to write DTs for each and every
possible model.

As much as I'd like to support and work on that, I'm doing this in my
free time, and this sounds like a big undertaking. At the moment, my
efforts are focused on making the Surface Pro X play (relatively) nice
with Linux (via DT). I had thought about this, but my time to work on
this is unfortunately limited. You'd probably have to ask e.g. the
Linaro folks for help and input (some of which, e.g. Bjorn Andersson
are currently working on DTs for WoA devices), and also convince the
ACPI maintainers.

Regards,
Max
Ilias Apalodimas July 28, 2022, 12:24 p.m. UTC | #30
On Thu, 28 Jul 2022 at 14:33, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Jul 28, 2022 at 12:48:19PM +0200, Maximilian Luz wrote:
>
> [...]
>
> >
> > I would very much like to avoid the need for special bootloaders. The
> > devices we're talking about are WoA devices, meaning they _should_
> > ideally boot just fine with EFI and ACPI.
> >
>
> Completely agreed.

This is not a special bootloader though.  Quite the opposite.  It's a
standard UEFI compliant bootloader, which uses the fact that EFI is
supposed to be extensible.  It installs a linux specific config table,
similar to how we install a linux specific protocol to load our initrd
and it's certainly lot more scalable than adding new stuff to the
device tree.

>
> > From an end-user perspective, it's annoying enough that we'll have to
> > stick with DTs for the time being due to the use of PEPs in ACPI.
>
> But have we explored or investigated what it takes to rewrite ACPI f/w
> to just use standard methods ? Does it require more firmware changes or
> new firmware entities or impossible at any cost ?
>
> For me that is more important than just getting this one on DT. Because
> if you take that path, we will have to keep doing that, with loads of
> unnecessary drivers if they are not shared with any other SoC with DT
> support upstream. We might also miss chance to get things added to the ACPI
> spec as we don't care which means that we never be able to use ACPI on
> similar future platforms even though they get shipped with ACPI.
>
> It will be a loop where we constantly keep converting this ACPI shipped
> platform into DT upstream. IMHO we don't want to be there.
>
> --
> Regards,
> Sudeep

Regards
/Ilias
Ilias Apalodimas July 28, 2022, 12:35 p.m. UTC | #31
Hi Maximilian

On Thu, 28 Jul 2022 at 13:48, Maximilian Luz <luzmaximilian@gmail.com> wrote:
>
> On 7/28/22 08:03, Ilias Apalodimas wrote:
> > Hi all,
> >
> > On Wed, 27 Jul 2022 at 16:24, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >>
> >> On Wed, Jul 27, 2022 at 03:03:49PM +0200, Maximilian Luz wrote:
> >>>
> >>> Is there really a good way around it?
> >>
> >> Yes rely on the firmware preferably auto discover, if that is not an option,
> >> how about query. It seem to be working in your case.
> >
> > That's a good point.  We have a similar situation with some Arm
> > devices and U-Boot.  Let me try to explain a bit.
> >
> > There's code plugged in in OP-TEE and U-Boot atm which allows you to
> > store EFI variables on an RPMB.  This is a nice alternative if your
> > device doesn't have any other secure storage,  however it presents
> > some challenges after ExitBootServices, similar to the ones you have
> > here.
> >
> > The eMMC controller usually lives in the non-secure world.  OP-TEE
> > can't access that, so it relies on a userspace supplicant to perform
> > the RPMB accesses.  That supplicant is present in U-Boot and
> > Get/SetVariable works fine before ExitBootServices.  Once Linux boots,
> >   the 'U-Boot supplicant' goes away and we launch the linux equivalent
> > one from userspace.  Since variable accessing is a runtime service and
> > it still has to go through the firmware we can't use those anymore
> > since U-Boot doesn't preserve the supplicant, the eMMC driver and the
> > OP-TEE portions needed in the runtime section(and even if it did we
> > would now have 2 drivers racing to access the same hardware).  Instead
> > U-Boot copies the variables in runtime memory and
> > GetVariable/GetNextVariable still works, but SetVariable returns
> > EFI_UNSUPPORTED.
> >
> > I've spent enough time looking at available solutions and although
> > this indeed breaks the EFI spec, something along the lines of
> > replacing the runtime services with ones that give you direct access
> > to the secure world, completely bypassing the firmware is imho our
> > least bad option.
>
> This sounds very similar to what Qualcomm may be doing on some devices.
> The TrEE interface allows for callbacks and there are indications that
> one such callback-service is for RPMB. I believe that at least on some
> platforms, Qualcomm also stores UEFI variables in RPMB and uses the same
> uefisecapp interface in combination with RPMB listeners installed by the
> kernel to access them.
>
> > I have an ancient branch somewhere that I can polish up and send an
> > RFC [1],  but the way I enabled that was to install an empty config
> > table from the firmware.  That empty table is basically an indication
> > to the kernel saying "Hey I can't store variables, can you do that for
> > me".
> >
> > Is there any chance we can do something similar on that device (or
> > find a reasonable way of inferring that we need to replace some
> > services).  That way we could at least have a common entry point to
> > the kernel and leave out the DT changes.
> >
> > [1] https://git.linaro.org/people/ilias.apalodimas/net-next.git/log/?h=setvar_rt_optee_3
>
> I would very much like to avoid the need for special bootloaders. The
> devices we're talking about are WoA devices, meaning they _should_
> ideally boot just fine with EFI and ACPI.

I've already responded to following email, but I'll repeat it here for
completeness. It's not a special bootloader.  It's the opposite, it's
a generic UEFI compliant bootloader which takes advantage of the fact
EFI is extensible. We are doing something very similar in how we load
our initrd via the EFI_LOAD_FILE2 protocol.  Whether Qualcomm can add
that to their bootloaders is a different topic though.  But at some
point we need to draw a line than keep overloading the DT because a
vendor decided to go down it's own path.

>
>  From an end-user perspective, it's annoying enough that we'll have to
> stick with DTs for the time being due to the use of PEPs in ACPI. I
> really don't want to add some special bootloader for fixups to that.
> Also, this would just move the problem from kernel to bootloader.

But it *is* a bootloader problem.  The bootloader is aware of the fact
that it can't provide runtime services for X reasons and that's
exactly why we are trying to set EFI_RT_PROPERTIES_TABLE correctly
from the firmware.  All we are doing is install a config table to tell
the OS "I can't do that, can you find a way around it?".

Regards
/Ilias

>
> If you have any suggestions for another way of detecting this, please
> feel free to share. I, unfortunately, don't.
>
> Regards,
> Max
Maximilian Luz July 28, 2022, 12:49 p.m. UTC | #32
On 7/28/22 14:35, Ilias Apalodimas wrote:
> Hi Maximilian
> 
> On Thu, 28 Jul 2022 at 13:48, Maximilian Luz <luzmaximilian@gmail.com> wrote:
>>
>> On 7/28/22 08:03, Ilias Apalodimas wrote:
>>> Hi all,
>>>
>>> On Wed, 27 Jul 2022 at 16:24, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>>>
>>>> On Wed, Jul 27, 2022 at 03:03:49PM +0200, Maximilian Luz wrote:
>>>>>
>>>>> Is there really a good way around it?
>>>>
>>>> Yes rely on the firmware preferably auto discover, if that is not an option,
>>>> how about query. It seem to be working in your case.
>>>
>>> That's a good point.  We have a similar situation with some Arm
>>> devices and U-Boot.  Let me try to explain a bit.
>>>
>>> There's code plugged in in OP-TEE and U-Boot atm which allows you to
>>> store EFI variables on an RPMB.  This is a nice alternative if your
>>> device doesn't have any other secure storage,  however it presents
>>> some challenges after ExitBootServices, similar to the ones you have
>>> here.
>>>
>>> The eMMC controller usually lives in the non-secure world.  OP-TEE
>>> can't access that, so it relies on a userspace supplicant to perform
>>> the RPMB accesses.  That supplicant is present in U-Boot and
>>> Get/SetVariable works fine before ExitBootServices.  Once Linux boots,
>>>    the 'U-Boot supplicant' goes away and we launch the linux equivalent
>>> one from userspace.  Since variable accessing is a runtime service and
>>> it still has to go through the firmware we can't use those anymore
>>> since U-Boot doesn't preserve the supplicant, the eMMC driver and the
>>> OP-TEE portions needed in the runtime section(and even if it did we
>>> would now have 2 drivers racing to access the same hardware).  Instead
>>> U-Boot copies the variables in runtime memory and
>>> GetVariable/GetNextVariable still works, but SetVariable returns
>>> EFI_UNSUPPORTED.
>>>
>>> I've spent enough time looking at available solutions and although
>>> this indeed breaks the EFI spec, something along the lines of
>>> replacing the runtime services with ones that give you direct access
>>> to the secure world, completely bypassing the firmware is imho our
>>> least bad option.
>>
>> This sounds very similar to what Qualcomm may be doing on some devices.
>> The TrEE interface allows for callbacks and there are indications that
>> one such callback-service is for RPMB. I believe that at least on some
>> platforms, Qualcomm also stores UEFI variables in RPMB and uses the same
>> uefisecapp interface in combination with RPMB listeners installed by the
>> kernel to access them.
>>
>>> I have an ancient branch somewhere that I can polish up and send an
>>> RFC [1],  but the way I enabled that was to install an empty config
>>> table from the firmware.  That empty table is basically an indication
>>> to the kernel saying "Hey I can't store variables, can you do that for
>>> me".
>>>
>>> Is there any chance we can do something similar on that device (or
>>> find a reasonable way of inferring that we need to replace some
>>> services).  That way we could at least have a common entry point to
>>> the kernel and leave out the DT changes.
>>>
>>> [1] https://git.linaro.org/people/ilias.apalodimas/net-next.git/log/?h=setvar_rt_optee_3
>>
>> I would very much like to avoid the need for special bootloaders. The
>> devices we're talking about are WoA devices, meaning they _should_
>> ideally boot just fine with EFI and ACPI.
> 
> I've already responded to following email, but I'll repeat it here for
> completeness. It's not a special bootloader.  It's the opposite, it's
> a generic UEFI compliant bootloader which takes advantage of the fact
> EFI is extensible. We are doing something very similar in how we load
> our initrd via the EFI_LOAD_FILE2 protocol.  Whether Qualcomm can add
> that to their bootloaders is a different topic though.  But at some
> point we need to draw a line than keep overloading the DT because a
> vendor decided to go down it's own path.

But still, you're asking users to install an extra thing in the boot
chain. That's what I mean by "special". So the situation would then be
this: User needs a) GRUB (or something similar) for booting the kernel
(or dual-booting, ...), b) DTBLoader for loading the device-tree because
we don't support the ACPI Qualcomm provided, and c) your thing for EFI
variables and potentially other firmware fix-ups. b) and c) are both
things that "normal" users don't expect. IMHO we should try to get rid
of those "non-standard" things, not add more.

>>   From an end-user perspective, it's annoying enough that we'll have to
>> stick with DTs for the time being due to the use of PEPs in ACPI. I
>> really don't want to add some special bootloader for fixups to that.
>> Also, this would just move the problem from kernel to bootloader.
> 
> But it *is* a bootloader problem.  The bootloader is aware of the fact
> that it can't provide runtime services for X reasons and that's
> exactly why we are trying to set EFI_RT_PROPERTIES_TABLE correctly
> from the firmware.  All we are doing is install a config table to tell
> the OS "I can't do that, can you find a way around it?".

Sure, but is making the Linux installation process more device
dependent and complicated really the best way to solve this?

Regards,
Max
Sudeep Holla July 28, 2022, 1:42 p.m. UTC | #33
On Thu, Jul 28, 2022 at 01:45:57PM +0200, Maximilian Luz wrote:
>
> Would something like this work for you: Add a compatible for the TrEE
> interface (e.g. qcom,sc8180x-tee) but not for the specific apps running

IIUC, you would introduce a compatible for each unique production if there
is a need. This constitutes as a strong need for that, but you need just
that, no need to have any notion or info to indicate TrEE/TEE as you know
this product runs those in TEE.

In short, just use the platform specific(not SoC or SoC family) specific
compatible to initialise your driver and don't introduce any specific
compatible for this particular firmware interface need.

--
Regards,
Sudeep
Maximilian Luz July 28, 2022, 2:09 p.m. UTC | #34
On 7/28/22 15:42, Sudeep Holla wrote:
> On Thu, Jul 28, 2022 at 01:45:57PM +0200, Maximilian Luz wrote:
>>
>> Would something like this work for you: Add a compatible for the TrEE
>> interface (e.g. qcom,sc8180x-tee) but not for the specific apps running
> 
> IIUC, you would introduce a compatible for each unique production if there
> is a need. This constitutes as a strong need for that, but you need just
> that, no need to have any notion or info to indicate TrEE/TEE as you know
> this product runs those in TEE.
> 
> In short, just use the platform specific(not SoC or SoC family) specific
> compatible to initialise your driver and don't introduce any specific
> compatible for this particular firmware interface need.

As Krzysztof mentioned, it would be good to ensure proper device
ordering wrt. SCM. Having a device node for the overall TrEE interface
would allow specifying that via DT. We could then still use the platform
compatible to load the specific things inside that driver.
Alternatively, we would need to do this extra stuff in qcom_scm.

I think separating this from qcom_scm into a new driver would be better
from a code separation and maintenance point of view. Also, this
reflects what's present in ACPI: There is a QCOM040B device for SCM and
a QCOM0476 device for TrEE.

Regards,
Max
Ard Biesheuvel July 28, 2022, 3:05 p.m. UTC | #35
On Thu, 28 Jul 2022 at 04:33, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Jul 28, 2022 at 12:48:19PM +0200, Maximilian Luz wrote:
>
> [...]
>
> >
> > I would very much like to avoid the need for special bootloaders. The
> > devices we're talking about are WoA devices, meaning they _should_
> > ideally boot just fine with EFI and ACPI.
> >
>
> Completely agreed.
>
> > From an end-user perspective, it's annoying enough that we'll have to
> > stick with DTs for the time being due to the use of PEPs in ACPI.
>
> But have we explored or investigated what it takes to rewrite ACPI f/w
> to just use standard methods ? Does it require more firmware changes or
> new firmware entities or impossible at any cost ?
>
> For me that is more important than just getting this one on DT. Because
> if you take that path, we will have to keep doing that, with loads of
> unnecessary drivers if they are not shared with any other SoC with DT
> support upstream. We might also miss chance to get things added to the ACPI
> spec as we don't care which means that we never be able to use ACPI on
> similar future platforms even though they get shipped with ACPI.
>
> It will be a loop where we constantly keep converting this ACPI shipped
> platform into DT upstream. IMHO we don't want to be there.
>

Supporting these devices in Linux in ACPI mode would involve
reimplementing the PEP subsystem, and reimplementing PEP drivers for
all these QCOM peripherals to manage the probing and the power states.
I don't think this is realistic at all, and a huge waste of
engineering effort otherwise.

It is also orthogonal to the discussion, as far as I understand: ACPI
is not telling the system whether or not these TZ services should be
used instead of EFI runtime calls.

So I think this is a reasonable way to expose these EFI services,
although I am not thrilled about the fact that it is needed.
Surprisingly, Microsoft also supports this model both on x86 and arm64
for platforms that keep their variables on eMMC (or any other kind of
storage that sits behind a controller that cannot be shared between
the OS and the firmware). So if we agree that we will support these
systems as best we can, supporting EFI variables at runtime is
something that we should support as well. (Note that I am not
convinced about the latter point myself: on many systems, the EFI
variable store is used precisely once, when GRUB gets installed and
its path added to the boot order, so if we could find a way to
streamline that without EFI runtime services, the story around why EFI
runtime services are important becomes quite weak)

As for the use of efivars_register(): I think this is reasonable, and
the way these patches use it is exactly why it exists in the first
place. Do not that a substantial overhaul of efivars is queued up in
efi/next, although I suspect those changes do not conflict with this
series.
Ilias Apalodimas July 28, 2022, 3:16 p.m. UTC | #36
Hi Ard,

[...]

> > > From an end-user perspective, it's annoying enough that we'll have to
> > > stick with DTs for the time being due to the use of PEPs in ACPI.
> >
> > But have we explored or investigated what it takes to rewrite ACPI f/w
> > to just use standard methods ? Does it require more firmware changes or
> > new firmware entities or impossible at any cost ?
> >
> > For me that is more important than just getting this one on DT. Because
> > if you take that path, we will have to keep doing that, with loads of
> > unnecessary drivers if they are not shared with any other SoC with DT
> > support upstream. We might also miss chance to get things added to the ACPI
> > spec as we don't care which means that we never be able to use ACPI on
> > similar future platforms even though they get shipped with ACPI.
> >
> > It will be a loop where we constantly keep converting this ACPI shipped
> > platform into DT upstream. IMHO we don't want to be there.
> >
>
> Supporting these devices in Linux in ACPI mode would involve
> reimplementing the PEP subsystem, and reimplementing PEP drivers for
> all these QCOM peripherals to manage the probing and the power states.
> I don't think this is realistic at all, and a huge waste of
> engineering effort otherwise.
>
> It is also orthogonal to the discussion, as far as I understand: ACPI
> is not telling the system whether or not these TZ services should be
> used instead of EFI runtime calls.
>
> So I think this is a reasonable way to expose these EFI services,
> although I am not thrilled about the fact that it is needed.
> Surprisingly, Microsoft also supports this model both on x86 and arm64
> for platforms that keep their variables on eMMC (or any other kind of
> storage that sits behind a controller that cannot be shared between
> the OS and the firmware). So if we agree that we will support these
> systems as best we can, supporting EFI variables at runtime is
> something that we should support as well. (Note that I am not
> convinced about the latter point myself: on many systems, the EFI
> variable store is used precisely once, when GRUB gets installed and
> its path added to the boot order, so if we could find a way to
> streamline that without EFI runtime services, the story around why EFI
> runtime services are important becomes quite weak)

Unfortunately this is not entirely true.  Yes the majority of use
cases is what you describe, however we also need SetVariable for
capsule update on disk.

[...]
Sudeep Holla July 28, 2022, 4:16 p.m. UTC | #37
On Thu, Jul 28, 2022 at 08:05:58AM -0700, Ard Biesheuvel wrote:
> On Thu, 28 Jul 2022 at 04:33, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Thu, Jul 28, 2022 at 12:48:19PM +0200, Maximilian Luz wrote:
> >
> > [...]
> >
> > >
> > > I would very much like to avoid the need for special bootloaders. The
> > > devices we're talking about are WoA devices, meaning they _should_
> > > ideally boot just fine with EFI and ACPI.
> > >
> >
> > Completely agreed.
> >
> > > From an end-user perspective, it's annoying enough that we'll have to
> > > stick with DTs for the time being due to the use of PEPs in ACPI.
> >
> > But have we explored or investigated what it takes to rewrite ACPI f/w
> > to just use standard methods ? Does it require more firmware changes or
> > new firmware entities or impossible at any cost ?
> >
> > For me that is more important than just getting this one on DT. Because
> > if you take that path, we will have to keep doing that, with loads of
> > unnecessary drivers if they are not shared with any other SoC with DT
> > support upstream. We might also miss chance to get things added to the ACPI
> > spec as we don't care which means that we never be able to use ACPI on
> > similar future platforms even though they get shipped with ACPI.
> >
> > It will be a loop where we constantly keep converting this ACPI shipped
> > platform into DT upstream. IMHO we don't want to be there.
> >
>
> Supporting these devices in Linux in ACPI mode would involve
> reimplementing the PEP subsystem, and reimplementing PEP drivers for
> all these QCOM peripherals to manage the probing and the power states.
> I don't think this is realistic at all, and a huge waste of
> engineering effort otherwise.
>

I am aware of that and hence I am happy to see these as one off drivers
if needed. But if we don't stop that or keep converting them to DT,
IMO we will be in vicious circle of this conversion and will never be
able to support ACPI natively on these platforms. I know it is huge
effort and not expecting that to be done here, but we need to convey the
message to use ACPI standards or improve it if there is a need. Using
PEP is not helpful to run Linux in the long run. Also we may hit a point
when it may not be trivial to do that ACPI<->DT conversion.

> It is also orthogonal to the discussion, as far as I understand: ACPI
> is not telling the system whether or not these TZ services should be
> used instead of EFI runtime calls.
>

Agreed and I don't want to block any such discussions. Sorry if I derailed
the discussion, that was not my intentions.

--
Regards,
Sudeep
Konrad Dybcio July 28, 2022, 4:24 p.m. UTC | #38
On 28.07.2022 18:16, Sudeep Holla wrote:
> On Thu, Jul 28, 2022 at 08:05:58AM -0700, Ard Biesheuvel wrote:
>> On Thu, 28 Jul 2022 at 04:33, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>>
>>> On Thu, Jul 28, 2022 at 12:48:19PM +0200, Maximilian Luz wrote:
>>>
>>> [...]
>>>
>>>>
>>>> I would very much like to avoid the need for special bootloaders. The
>>>> devices we're talking about are WoA devices, meaning they _should_
>>>> ideally boot just fine with EFI and ACPI.
>>>>
>>>
>>> Completely agreed.
>>>
>>>> From an end-user perspective, it's annoying enough that we'll have to
>>>> stick with DTs for the time being due to the use of PEPs in ACPI.
>>>
>>> But have we explored or investigated what it takes to rewrite ACPI f/w
>>> to just use standard methods ? Does it require more firmware changes or
>>> new firmware entities or impossible at any cost ?
>>>
>>> For me that is more important than just getting this one on DT. Because
>>> if you take that path, we will have to keep doing that, with loads of
>>> unnecessary drivers if they are not shared with any other SoC with DT
>>> support upstream. We might also miss chance to get things added to the ACPI
>>> spec as we don't care which means that we never be able to use ACPI on
>>> similar future platforms even though they get shipped with ACPI.
>>>
>>> It will be a loop where we constantly keep converting this ACPI shipped
>>> platform into DT upstream. IMHO we don't want to be there.
>>>
>>
>> Supporting these devices in Linux in ACPI mode would involve
>> reimplementing the PEP subsystem, and reimplementing PEP drivers for
>> all these QCOM peripherals to manage the probing and the power states.
>> I don't think this is realistic at all, and a huge waste of
>> engineering effort otherwise.
>>
> 
> I am aware of that and hence I am happy to see these as one off drivers
> if needed. But if we don't stop that or keep converting them to DT,
> IMO we will be in vicious circle of this conversion and will never be
> able to support ACPI natively on these platforms. 
I think that people have given up on ACPI on Snapdragon, as it was not
providing enough information in some cases (such as TLMM pins that are
not accessible from the AP due to being marked 'secure') that needed to
be hardcoded.

New WoA laptop support is added using FDT and I haven't seen any patches
even adding ACPI matchlists for a long long time.

Konrad
I know it is huge
> effort and not expecting that to be done here, but we need to convey the
> message to use ACPI standards or improve it if there is a need. Using
> PEP is not helpful to run Linux in the long run. Also we may hit a point
> when it may not be trivial to do that ACPI<->DT conversion.
> 
>> It is also orthogonal to the discussion, as far as I understand: ACPI
>> is not telling the system whether or not these TZ services should be
>> used instead of EFI runtime calls.
>>
> 
> Agreed and I don't want to block any such discussions. Sorry if I derailed
> the discussion, that was not my intentions.
> 
> --
> Regards,
> Sudeep
Ilias Apalodimas July 28, 2022, 4:56 p.m. UTC | #39
On Thu, 28 Jul 2022 at 15:49, Maximilian Luz <luzmaximilian@gmail.com> wrote:
>

[...]

> >>
> >>> I have an ancient branch somewhere that I can polish up and send an
> >>> RFC [1],  but the way I enabled that was to install an empty config
> >>> table from the firmware.  That empty table is basically an indication
> >>> to the kernel saying "Hey I can't store variables, can you do that for
> >>> me".
> >>>
> >>> Is there any chance we can do something similar on that device (or
> >>> find a reasonable way of inferring that we need to replace some
> >>> services).  That way we could at least have a common entry point to
> >>> the kernel and leave out the DT changes.
> >>>
> >>> [1] https://git.linaro.org/people/ilias.apalodimas/net-next.git/log/?h=setvar_rt_optee_3
> >>
> >> I would very much like to avoid the need for special bootloaders. The
> >> devices we're talking about are WoA devices, meaning they _should_
> >> ideally boot just fine with EFI and ACPI.
> >
> > I've already responded to following email, but I'll repeat it here for
> > completeness. It's not a special bootloader.  It's the opposite, it's
> > a generic UEFI compliant bootloader which takes advantage of the fact
> > EFI is extensible. We are doing something very similar in how we load
> > our initrd via the EFI_LOAD_FILE2 protocol.  Whether Qualcomm can add
> > that to their bootloaders is a different topic though.  But at some
> > point we need to draw a line than keep overloading the DT because a
> > vendor decided to go down it's own path.
>
> But still, you're asking users to install an extra thing in the boot
> chain.

Not users.  EFI firmware implementations that want to support this in
a generic way.

> That's what I mean by "special". So the situation would then be
> this: User needs a) GRUB (or something similar) for booting the kernel
> (or dual-booting, ...), b) DTBLoader for loading the device-tree because
> we don't support the ACPI Qualcomm provided, and c) your thing for EFI
> variables and potentially other firmware fix-ups. b) and c) are both
> things that "normal" users don't expect. IMHO we should try to get rid
> of those "non-standard" things, not add more.

But that's exactly why EFI is extensible .  You can have non standard
functionality on your firmware for cases like this which doesn't need
to land in the spec.

>
> >>   From an end-user perspective, it's annoying enough that we'll have to
> >> stick with DTs for the time being due to the use of PEPs in ACPI. I
> >> really don't want to add some special bootloader for fixups to that.
> >> Also, this would just move the problem from kernel to bootloader.
> >
> > But it *is* a bootloader problem.  The bootloader is aware of the fact
> > that it can't provide runtime services for X reasons and that's
> > exactly why we are trying to set EFI_RT_PROPERTIES_TABLE correctly
> > from the firmware.  All we are doing is install a config table to tell
> > the OS "I can't do that, can you find a way around it?".
>
> Sure, but is making the Linux installation process more device
> dependent and complicated really the best way to solve this?

Isn't it device dependent already?  That boat has sailed already since
we need to change the very definition of runtime services and replace
them with OS specific ones.  If we add it on the DT, you'll end up
with different DTs per OS and potentially per use case.  In my head
the DTs should be part of the firmware (and authenticated by the
firmware as well) instead of loading whatever we want each time.  By
using a config table we can add a u64 (random thought),  that tells
the kernel which TEE implementation will handle variable storage.  So
we can have a common extension to boot loaders, which at least uses
EFI interfaces to communicate the functionality.

I really prefer this to adding it on a DT, but I am not that picky.
Your email raises an important topic of replacing runtime services
with OS specific ones,  which is unfortunately very much needed and we
should fix that.

Thanks
/Ilias
>
> Regards,
> Max
Maximilian Luz July 28, 2022, 5:27 p.m. UTC | #40
On 7/28/22 18:56, Ilias Apalodimas wrote:
> On Thu, 28 Jul 2022 at 15:49, Maximilian Luz <luzmaximilian@gmail.com> wrote:
>>
> 
> [...]
> 
>>>>
>>>>> I have an ancient branch somewhere that I can polish up and send an
>>>>> RFC [1],  but the way I enabled that was to install an empty config
>>>>> table from the firmware.  That empty table is basically an indication
>>>>> to the kernel saying "Hey I can't store variables, can you do that for
>>>>> me".
>>>>>
>>>>> Is there any chance we can do something similar on that device (or
>>>>> find a reasonable way of inferring that we need to replace some
>>>>> services).  That way we could at least have a common entry point to
>>>>> the kernel and leave out the DT changes.
>>>>>
>>>>> [1] https://git.linaro.org/people/ilias.apalodimas/net-next.git/log/?h=setvar_rt_optee_3
>>>>
>>>> I would very much like to avoid the need for special bootloaders. The
>>>> devices we're talking about are WoA devices, meaning they _should_
>>>> ideally boot just fine with EFI and ACPI.
>>>
>>> I've already responded to following email, but I'll repeat it here for
>>> completeness. It's not a special bootloader.  It's the opposite, it's
>>> a generic UEFI compliant bootloader which takes advantage of the fact
>>> EFI is extensible. We are doing something very similar in how we load
>>> our initrd via the EFI_LOAD_FILE2 protocol.  Whether Qualcomm can add
>>> that to their bootloaders is a different topic though.  But at some
>>> point we need to draw a line than keep overloading the DT because a
>>> vendor decided to go down it's own path.
>>
>> But still, you're asking users to install an extra thing in the boot
>> chain.
> 
> Not users.  EFI firmware implementations that want to support this in
> a generic way.

The whole point here is that we don't have control over that. I'd like
to fix the firmware, but we're talking about WoA devices where, let's
face it, both device and SoC vendor don't really care about Linux. Even
if you'd convince them to implement that for future generations, you'd
still need them to push firmware updates for older generations.
Generations that are end-of-life. IMHO, we should still try support
those. Or we just say "sorry, Linux doesn't support that on your WoA
device".

>> That's what I mean by "special". So the situation would then be
>> this: User needs a) GRUB (or something similar) for booting the kernel
>> (or dual-booting, ...), b) DTBLoader for loading the device-tree because
>> we don't support the ACPI Qualcomm provided, and c) your thing for EFI
>> variables and potentially other firmware fix-ups. b) and c) are both
>> things that "normal" users don't expect. IMHO we should try to get rid
>> of those "non-standard" things, not add more.
> 
> But that's exactly why EFI is extensible .  You can have non standard
> functionality on your firmware for cases like this which doesn't need
> to land in the spec.
> 
>>
>>>>    From an end-user perspective, it's annoying enough that we'll have to
>>>> stick with DTs for the time being due to the use of PEPs in ACPI. I
>>>> really don't want to add some special bootloader for fixups to that.
>>>> Also, this would just move the problem from kernel to bootloader.
>>>
>>> But it *is* a bootloader problem.  The bootloader is aware of the fact
>>> that it can't provide runtime services for X reasons and that's
>>> exactly why we are trying to set EFI_RT_PROPERTIES_TABLE correctly
>>> from the firmware.  All we are doing is install a config table to tell
>>> the OS "I can't do that, can you find a way around it?".
>>
>> Sure, but is making the Linux installation process more device
>> dependent and complicated really the best way to solve this?
> 
> Isn't it device dependent already?  That boat has sailed already since
> we need to change the very definition of runtime services and replace
> them with OS specific ones.  If we add it on the DT, you'll end up
> with different DTs per OS and potentially per use case.  In my head
> the DTs should be part of the firmware (and authenticated by the
> firmware as well) instead of loading whatever we want each time.  By
> using a config table we can add a u64 (random thought),  that tells
> the kernel which TEE implementation will handle variable storage.  So
> we can have a common extension to boot loaders, which at least uses
> EFI interfaces to communicate the functionality.

The only thing that is making the installation-process for end-users
device dependent is installing the DTB. We can handle the device
specific stuff in the kernel, just as we already handle buggy devices.

Further, you seem to assume that these devices provide a DT in the first
place. WoA devices use ACPI, so they don't. But for the time being (as
discussed elsewhere) we unfortunately need to stick with DTs and can't
really use ACPI. I agree that we should avoid OS and use-case specific
DTs, but I don't see how this would make a DT use-case or OS specific.
Things are firmware specific, the interface doesn't change with a
different OS, and we're only indicating the presence of that interface.

My current suggestion (already sent to Sudeep earlier) is (roughly)
this: Add one compatible for the TrEE / TrustZone interface. Then decide
to load or instantiate what needs to be loaded in the driver for that.
That (depending on maybe SoC / platform / vendor) includes installing
the efivar operations. This way we don't have to fill the DT with the
specific things running in firmware.

Regards,
Max
Sudeep Holla July 29, 2022, 8:52 a.m. UTC | #41
On Thu, Jul 28, 2022 at 07:27:19PM +0200, Maximilian Luz wrote:

[...]

> My current suggestion (already sent to Sudeep earlier) is (roughly)
> this: Add one compatible for the TrEE / TrustZone interface.

Still I don't understand why you need extra compatible if you know
this laptop(with a unique compatible to identify it) always runs this
TrEE interface.

--
Regards,
Sudeep
Maximilian Luz July 29, 2022, 3:11 p.m. UTC | #42
On 7/29/22 10:52, Sudeep Holla wrote:
> On Thu, Jul 28, 2022 at 07:27:19PM +0200, Maximilian Luz wrote:
> 
> [...]
> 
>> My current suggestion (already sent to Sudeep earlier) is (roughly)
>> this: Add one compatible for the TrEE / TrustZone interface.
> 
> Still I don't understand why you need extra compatible if you know
> this laptop(with a unique compatible to identify it) always runs this
> TrEE interface.

First of all, to recap: I suggest adding a device and driver for the TrEE
interface, with a compatible for that. That then (based on platform)
instantiates devices and drivers for the applications running in TrEE. The
compatible I'm talking about is for that general TrEE interface. Not any
specific application.

a) Because this better reflects the ACPI tables on those devices. As I've said,
    there is a HID specifically for the TrEE interface. You were concerned
    earlier that we should try to add support for that, and now you want to
    create a purely artificial divide between ACPI and DT? Ideally, we can have
    the driver load via both the DT compatible and the ACPI HID depending on
    whether we use one or the other without many other changes.

    Would you equally suggest that we not load the driver by its ACPI HID and
    instead do DMI matching?

b) Qualcomm also has a DT compatible for this (qcom,qseecom), see e.g. [1].
    Note: they seem to have changed the name from Secure Execution Environment
    to Trusted Execution Environment, at least in their Windows driver. This is
    why I used "tee" instead of "see" (also their naming of things is somewhat
    confusing and seems to change randomly). Fundamentally, this is the same
    interface (they just implement a lot more things in their driver, the couple
    of functions I proposed here handle the absolute minimum required for
    uefisecapp, it can always be extended later when needed).

c) Given their naming of the DT compatible, this interface itself is pretty
    much guaranteed to be stable. It's definitely not going away with some
    firmware update. So your earlier concerns about having to update the DT in
    case of firmware changes do simply not apply here. It is a core component of
    these platforms. As far as I can see, your "let's load the TrEE driver via
    the platform compatible" suggestion is now exactly the same as a "let's load
    some PCIe controller via the platform/SoC compatible". It's an interface
    that is either present or not present, depending on the device. We're not
    encoding any firmware specifics (ie. what's running inside the TrEE) in the
    DT, we just say that it's there (the rest is decided by the driver, e.g. via
    platform compatibles or DMI matching).

d) By specifying it in the DT, we can properly link it up via a phandle to the
    SCM and properly model the supplier/client relation between them. While we
    can't do that with ACPI, I think it's still a good idea to handle this
    properly in times we can.

Regards,
Max

[1]: https://git.codelinaro.org/clo/la/kernel/msm-4.14/-/blob/auto-kernel.lnx.4.14.c34/drivers/misc/qseecom.c
Ilias Apalodimas July 31, 2022, 9:54 a.m. UTC | #43
Hi Maximilian,

On Thu, 28 Jul 2022 at 20:27, Maximilian Luz <luzmaximilian@gmail.com> wrote:
>

[...]

> >>>>>
> >>>>> [1] https://git.linaro.org/people/ilias.apalodimas/net-next.git/log/?h=setvar_rt_optee_3
> >>>>
> >>>> I would very much like to avoid the need for special bootloaders. The
> >>>> devices we're talking about are WoA devices, meaning they _should_
> >>>> ideally boot just fine with EFI and ACPI.
> >>>
> >>> I've already responded to following email, but I'll repeat it here for
> >>> completeness. It's not a special bootloader.  It's the opposite, it's
> >>> a generic UEFI compliant bootloader which takes advantage of the fact
> >>> EFI is extensible. We are doing something very similar in how we load
> >>> our initrd via the EFI_LOAD_FILE2 protocol.  Whether Qualcomm can add
> >>> that to their bootloaders is a different topic though.  But at some
> >>> point we need to draw a line than keep overloading the DT because a
> >>> vendor decided to go down it's own path.
> >>
> >> But still, you're asking users to install an extra thing in the boot
> >> chain.
> >
> > Not users.  EFI firmware implementations that want to support this in
> > a generic way.
>
> The whole point here is that we don't have control over that. I'd like
> to fix the firmware, but we're talking about WoA devices where, let's
> face it, both device and SoC vendor don't really care about Linux. Even
> if you'd convince them to implement that for future generations, you'd
> still need them to push firmware updates for older generations.
> Generations that are end-of-life. IMHO, we should still try support
> those. Or we just say "sorry, Linux doesn't support that on your WoA
> device".

Yea we agree on that.  I've mentioned on a previous mail that whether
Qualcomm wants to support this in a generic way is questionable, since
we have no control over the firmware.  My only 'objection' is that the
kernel has a generic way of discovering which runtime services are
supported, which we will completely ignore based on some DT entries.

In any case let's find something that fits OP-TEE as well, since I can
send those patches afterwards.

>
> >> That's what I mean by "special". So the situation would then be
> >> this: User needs a) GRUB (or something similar) for booting the kernel
> >> (or dual-booting, ...), b) DTBLoader for loading the device-tree because
> >> we don't support the ACPI Qualcomm provided, and c) your thing for EFI
> >> variables and potentially other firmware fix-ups. b) and c) are both
> >> things that "normal" users don't expect. IMHO we should try to get rid
> >> of those "non-standard" things, not add more.
> >
> > But that's exactly why EFI is extensible .  You can have non standard
> > functionality on your firmware for cases like this which doesn't need
> > to land in the spec.
> >
> >>
> >>>>    From an end-user perspective, it's annoying enough that we'll have to
> >>>> stick with DTs for the time being due to the use of PEPs in ACPI. I
> >>>> really don't want to add some special bootloader for fixups to that.
> >>>> Also, this would just move the problem from kernel to bootloader.
> >>>
> >>> But it *is* a bootloader problem.  The bootloader is aware of the fact
> >>> that it can't provide runtime services for X reasons and that's
> >>> exactly why we are trying to set EFI_RT_PROPERTIES_TABLE correctly
> >>> from the firmware.  All we are doing is install a config table to tell
> >>> the OS "I can't do that, can you find a way around it?".
> >>
> >> Sure, but is making the Linux installation process more device
> >> dependent and complicated really the best way to solve this?
> >
> > Isn't it device dependent already?  That boat has sailed already since
> > we need to change the very definition of runtime services and replace
> > them with OS specific ones.  If we add it on the DT, you'll end up
> > with different DTs per OS and potentially per use case.  In my head
> > the DTs should be part of the firmware (and authenticated by the
> > firmware as well) instead of loading whatever we want each time.  By
> > using a config table we can add a u64 (random thought),  that tells
> > the kernel which TEE implementation will handle variable storage.  So
> > we can have a common extension to boot loaders, which at least uses
> > EFI interfaces to communicate the functionality.
>
> The only thing that is making the installation-process for end-users
> device dependent is installing the DTB. We can handle the device
> specific stuff in the kernel, just as we already handle buggy devices.
>
> Further, you seem to assume that these devices provide a DT in the first
> place. WoA devices use ACPI, so they don't. But for the time being (as
> discussed elsewhere) we unfortunately need to stick with DTs and can't
> really use ACPI. I agree that we should avoid OS and use-case specific
> DTs, but I don't see how this would make a DT use-case or OS specific.
> Things are firmware specific, the interface doesn't change with a
> different OS, and we're only indicating the presence of that interface.
>
> My current suggestion (already sent to Sudeep earlier) is (roughly)
> this: Add one compatible for the TrEE / TrustZone interface. Then decide
> to load or instantiate what needs to be loaded in the driver for that.
> That (depending on maybe SoC / platform / vendor) includes installing
> the efivar operations. This way we don't have to fill the DT with the
> specific things running in firmware.

As far as OP-TEE is concerned, I think we can make the 'feature'
discoverable.  I'll go propose that to op-tee but if that gets
accepted, we don't need any extra nodes (other than the existing one),
to wire up efivars_register().

Thanks
/Ilias
>
> Regards,
> Max
Maximilian Luz July 31, 2022, 10:48 p.m. UTC | #44
Hi,

On 7/31/22 11:54, Ilias Apalodimas wrote:
> Hi Maximilian,
> 
> On Thu, 28 Jul 2022 at 20:27, Maximilian Luz <luzmaximilian@gmail.com> wrote:
>>
> 
> [...]
> 
>>>>>>>
>>>>>>> [1] https://git.linaro.org/people/ilias.apalodimas/net-next.git/log/?h=setvar_rt_optee_3
>>>>>>
>>>>>> I would very much like to avoid the need for special bootloaders. The
>>>>>> devices we're talking about are WoA devices, meaning they _should_
>>>>>> ideally boot just fine with EFI and ACPI.
>>>>>
>>>>> I've already responded to following email, but I'll repeat it here for
>>>>> completeness. It's not a special bootloader.  It's the opposite, it's
>>>>> a generic UEFI compliant bootloader which takes advantage of the fact
>>>>> EFI is extensible. We are doing something very similar in how we load
>>>>> our initrd via the EFI_LOAD_FILE2 protocol.  Whether Qualcomm can add
>>>>> that to their bootloaders is a different topic though.  But at some
>>>>> point we need to draw a line than keep overloading the DT because a
>>>>> vendor decided to go down it's own path.
>>>>
>>>> But still, you're asking users to install an extra thing in the boot
>>>> chain.
>>>
>>> Not users.  EFI firmware implementations that want to support this in
>>> a generic way.
>>
>> The whole point here is that we don't have control over that. I'd like
>> to fix the firmware, but we're talking about WoA devices where, let's
>> face it, both device and SoC vendor don't really care about Linux. Even
>> if you'd convince them to implement that for future generations, you'd
>> still need them to push firmware updates for older generations.
>> Generations that are end-of-life. IMHO, we should still try support
>> those. Or we just say "sorry, Linux doesn't support that on your WoA
>> device".
> 
> Yea we agree on that.  I've mentioned on a previous mail that whether
> Qualcomm wants to support this in a generic way is questionable, since
> we have no control over the firmware.  My only 'objection' is that the
> kernel has a generic way of discovering which runtime services are
> supported, which we will completely ignore based on some DT entries.

Right, sorry. That makes sense. If we have a realistic possibility, then
I agree that making it discoverable in firmware is the best option. My
point was just that we can't rely on Windows-focused vendors to
implement it.

> In any case let's find something that fits OP-TEE as well, since I can
> send those patches afterwards.

I think it's a great idea to try and find some sort of standardized
solution for OP-TEE and other interested projects similar to it, but we
still have to use a workaround for the Qualcomm WoA devices we have :(

Nevertheless, I'm happy to provide some input for a generic solution,
although I'm not sure I'm the best person to talk to about this.

>>>> That's what I mean by "special". So the situation would then be
>>>> this: User needs a) GRUB (or something similar) for booting the kernel
>>>> (or dual-booting, ...), b) DTBLoader for loading the device-tree because
>>>> we don't support the ACPI Qualcomm provided, and c) your thing for EFI
>>>> variables and potentially other firmware fix-ups. b) and c) are both
>>>> things that "normal" users don't expect. IMHO we should try to get rid
>>>> of those "non-standard" things, not add more.
>>>
>>> But that's exactly why EFI is extensible .  You can have non standard
>>> functionality on your firmware for cases like this which doesn't need
>>> to land in the spec.
>>>
>>>>
>>>>>>     From an end-user perspective, it's annoying enough that we'll have to
>>>>>> stick with DTs for the time being due to the use of PEPs in ACPI. I
>>>>>> really don't want to add some special bootloader for fixups to that.
>>>>>> Also, this would just move the problem from kernel to bootloader.
>>>>>
>>>>> But it *is* a bootloader problem.  The bootloader is aware of the fact
>>>>> that it can't provide runtime services for X reasons and that's
>>>>> exactly why we are trying to set EFI_RT_PROPERTIES_TABLE correctly
>>>>> from the firmware.  All we are doing is install a config table to tell
>>>>> the OS "I can't do that, can you find a way around it?".
>>>>
>>>> Sure, but is making the Linux installation process more device
>>>> dependent and complicated really the best way to solve this?
>>>
>>> Isn't it device dependent already?  That boat has sailed already since
>>> we need to change the very definition of runtime services and replace
>>> them with OS specific ones.  If we add it on the DT, you'll end up
>>> with different DTs per OS and potentially per use case.  In my head
>>> the DTs should be part of the firmware (and authenticated by the
>>> firmware as well) instead of loading whatever we want each time.  By
>>> using a config table we can add a u64 (random thought),  that tells
>>> the kernel which TEE implementation will handle variable storage.  So
>>> we can have a common extension to boot loaders, which at least uses
>>> EFI interfaces to communicate the functionality.
>>
>> The only thing that is making the installation-process for end-users
>> device dependent is installing the DTB. We can handle the device
>> specific stuff in the kernel, just as we already handle buggy devices.
>>
>> Further, you seem to assume that these devices provide a DT in the first
>> place. WoA devices use ACPI, so they don't. But for the time being (as
>> discussed elsewhere) we unfortunately need to stick with DTs and can't
>> really use ACPI. I agree that we should avoid OS and use-case specific
>> DTs, but I don't see how this would make a DT use-case or OS specific.
>> Things are firmware specific, the interface doesn't change with a
>> different OS, and we're only indicating the presence of that interface.
>>
>> My current suggestion (already sent to Sudeep earlier) is (roughly)
>> this: Add one compatible for the TrEE / TrustZone interface. Then decide
>> to load or instantiate what needs to be loaded in the driver for that.
>> That (depending on maybe SoC / platform / vendor) includes installing
>> the efivar operations. This way we don't have to fill the DT with the
>> specific things running in firmware.
> 
> As far as OP-TEE is concerned, I think we can make the 'feature'
> discoverable.  I'll go propose that to op-tee but if that gets
> accepted, we don't need any extra nodes (other than the existing one),
> to wire up efivars_register().

Right. I think you (either in your patches or mails) already mentioned
having an integer ID for the implementation (or maybe implementation +
vendor?). Apart from that, I think it might also make sense to have a
bit-field similar to efi.runtime_supported_mask that tells the kernel
which functions are taken over.

So with that you could call efivars_register() based on the firmware
table in the driver for linaro,optee-tz (I assume) whether for qcom,tee
(or whatever we'd call that) we'd have to hard-code it based on some
platform/model identifier.

Regards,
Max
Srinivas Kandagatla Aug. 2, 2022, 11:51 a.m. UTC | #45
Hi Maximilian,

On 23/07/2022 23:49, Maximilian Luz wrote:
> On modern Qualcomm platforms, access to EFI variables is restricted to
> the secure world / TrustZone, i.e. the Trusted Execution Environment
> (TrEE or TEE) as Qualcomm seems to call it. To access EFI variables, we
> therefore need to talk to the UEFI Secure Application (uefisecapp),
> residing in the TrEE.
> 
> This series adds support for accessing EFI variables on those platforms.
> 
> To do this, we first need to add some SCM call functions used to manage
> and talk to Secure Applications. A very small subset of this interface
> is added in the second patch (whereas the first one exports the required
> functions for that). Interface specifications are extracted from [1].
> While this does not (yet) support re-entrant SCM calls (including
> callbacks and listeners), this is enough to talk to the aforementioned
> uefisecapp on a couple of platforms (I've tested this on a Surface Pro X
> and heard reports from Lenovo Flex 5G, Lenovo Thinkpad x13s, and Lenovo
> Yoga C630 devices).
> 
> The third patch adds a client driver for uefisecapp, installing the
> respective efivar operations. The application interface has been reverse
> engineered from the Windows QcTrEE8180.sys driver.
> 
> Apart from uefisecapp, there are more Secure Applications running that
> we might want to support in the future. For example, on the Surface Pro
> X (sc8180x-based), the TPM is also managed via one.
> 
> I'm not sure whether this should go to drivers/firmware or to
> drivers/soc/qcom. I've put this into firmware as all of this is
> essentially an interface to the secure firmware running in the TrustZone
> (and SCM stuff is handled here already), but please let me know if I
> should move this.

 From what I see so far is that this is adapted from downstream qseecom 
driver, this approach could work for a limited usecases but not 
scalable, as we cannot add drivers for each Qualcomm specific TA in kernel.
This has to be handled in much generic way using Linux TEE framework, 
and let the userspace side deal with TA specific bits.

AFAIU, Qualcomm is moving away from qseecom interface to new smc-invoke 
interface, most of Qualcomm SoCs starting from SDM660 already have 
support to this.

This interface provides a better abstracted IPC mechanism to talk to TA. 
Most of these TA specific interfaces are packed in closed userspace source.
Having said that QTEE smcinvoke driver can be modeled as a proper TEE 
driver with Userspace driving the TA specific bits using existing tee uapis.
This also brings in other features like loading, Listeners aka 
callbacks, secure memory allocations..etc.

In the past, I have tried to do a prototype of this smcinvoke driver as 
a proper tee driver, incase you are interested patches are at 
https://git.linaro.org/landing-teams/working/qualcomm/kernel.git/log/?h=tracking-qcomlt-qcomtee
Plan is to discuss with Qualcomm and send it for upstream review.


I think its worth exploring if uefisecapp can talk smcinvoke.
I can ping Qualcomm engineers to see if that is doable.


thanks,
Srini


> 
> Regards,
> Max
> 
> [1]: https://git.codelinaro.org/clo/la/kernel/msm-4.14/-/blob/auto-kernel.lnx.4.14.c34/drivers/misc/qseecom.c
> 
> Maximilian Luz (4):
>    firmware: qcom_scm: Export SCM call functions
>    firmware: Add support for Qualcomm Trusted Execution Environment SCM
>      calls
>    firmware: Add support for Qualcomm UEFI Secure Application
>    dt-bindings: firmware: Add Qualcomm UEFI Secure Application client
> 
>   .../firmware/qcom,tee-uefisecapp.yaml         |  38 +
>   MAINTAINERS                                   |  14 +
>   drivers/firmware/Kconfig                      |  20 +
>   drivers/firmware/Makefile                     |   2 +
>   drivers/firmware/qcom_scm.c                   | 118 ++-
>   drivers/firmware/qcom_scm.h                   |  47 --
>   drivers/firmware/qcom_tee.c                   | 213 +++++
>   drivers/firmware/qcom_tee_uefisecapp.c        | 761 ++++++++++++++++++
>   include/linux/qcom_scm.h                      |  49 ++
>   include/linux/qcom_tee.h                      | 179 ++++
>   10 files changed, 1355 insertions(+), 86 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml
>   create mode 100644 drivers/firmware/qcom_tee.c
>   create mode 100644 drivers/firmware/qcom_tee_uefisecapp.c
>   create mode 100644 include/linux/qcom_tee.h
>
Maximilian Luz Aug. 2, 2022, 1:22 p.m. UTC | #46
On 8/2/22 13:51, Srinivas Kandagatla wrote:
> Hi Maximilian,
> 
> On 23/07/2022 23:49, Maximilian Luz wrote:
>> On modern Qualcomm platforms, access to EFI variables is restricted to
>> the secure world / TrustZone, i.e. the Trusted Execution Environment
>> (TrEE or TEE) as Qualcomm seems to call it. To access EFI variables, we
>> therefore need to talk to the UEFI Secure Application (uefisecapp),
>> residing in the TrEE.
>>
>> This series adds support for accessing EFI variables on those platforms.
>>
>> To do this, we first need to add some SCM call functions used to manage
>> and talk to Secure Applications. A very small subset of this interface
>> is added in the second patch (whereas the first one exports the required
>> functions for that). Interface specifications are extracted from [1].
>> While this does not (yet) support re-entrant SCM calls (including
>> callbacks and listeners), this is enough to talk to the aforementioned
>> uefisecapp on a couple of platforms (I've tested this on a Surface Pro X
>> and heard reports from Lenovo Flex 5G, Lenovo Thinkpad x13s, and Lenovo
>> Yoga C630 devices).
>>
>> The third patch adds a client driver for uefisecapp, installing the
>> respective efivar operations. The application interface has been reverse
>> engineered from the Windows QcTrEE8180.sys driver.
>>
>> Apart from uefisecapp, there are more Secure Applications running that
>> we might want to support in the future. For example, on the Surface Pro
>> X (sc8180x-based), the TPM is also managed via one.
>>
>> I'm not sure whether this should go to drivers/firmware or to
>> drivers/soc/qcom. I've put this into firmware as all of this is
>> essentially an interface to the secure firmware running in the TrustZone
>> (and SCM stuff is handled here already), but please let me know if I
>> should move this.
> 
>  From what I see so far is that this is adapted from downstream qseecom driver, this approach could work for a limited usecases but not scalable, as we cannot add drivers for each Qualcomm specific TA in kernel.
> This has to be handled in much generic way using Linux TEE framework, and let the userspace side deal with TA specific bits.

I generally agree with the sentiment, however UEFI variables should IMHO be
handled by the kernel. Moving handling of those to userspace breaks things like
EFI-based pstore and efivarfs. The latter will in turn break some user-space
tools (most notably efibootmgr used by e.g. GRUB and I think fwupdmgr which
needs to set some capsule variables). Ideally, we would find a way to not break
these, i.e. have them work out-of-the-box.

A similar argumentation might apply to the TPM app.

> AFAIU, Qualcomm is moving away from qseecom interface to new smc-invoke interface, most of Qualcomm SoCs starting from SDM660 already have support to this.
> 
> This interface provides a better abstracted IPC mechanism to talk to TA. Most of these TA specific interfaces are packed in closed userspace source.
> Having said that QTEE smcinvoke driver can be modeled as a proper TEE driver with Userspace driving the TA specific bits using existing tee uapis.
> This also brings in other features like loading, Listeners aka callbacks, secure memory allocations..etc.
> 
> In the past, I have tried to do a prototype of this smcinvoke driver as a proper tee driver, incase you are interested patches are at https://git.linaro.org/landing-teams/working/qualcomm/kernel.git/log/?h=tracking-qcomlt-qcomtee
> Plan is to discuss with Qualcomm and send it for upstream review.

Thanks for this information! So as far as I understand it, this is currently an
interface to user-space only, i.e. does not allow in-kernel drivers for apps?
It would be great if this could then be extended to handle (the bare minimum
of) in-kernel drivers (i.e. only things that the kernel itself needs, like EFI
variables). Alternatively, I'm happy to hear suggestions on how we not break
the aforementioned things while moving handling off to userspace.

> I think its worth exploring if uefisecapp can talk smcinvoke.
> I can ping Qualcomm engineers to see if that is doable.

I think that would be great! Thanks!

Regards,
Max
Ard Biesheuvel Aug. 2, 2022, 2:02 p.m. UTC | #47
On Tue, 2 Aug 2022 at 15:22, Maximilian Luz <luzmaximilian@gmail.com> wrote:
>
>
>
> On 8/2/22 13:51, Srinivas Kandagatla wrote:
> > Hi Maximilian,
> >
> > On 23/07/2022 23:49, Maximilian Luz wrote:
> >> On modern Qualcomm platforms, access to EFI variables is restricted to
> >> the secure world / TrustZone, i.e. the Trusted Execution Environment
> >> (TrEE or TEE) as Qualcomm seems to call it. To access EFI variables, we
> >> therefore need to talk to the UEFI Secure Application (uefisecapp),
> >> residing in the TrEE.
> >>
> >> This series adds support for accessing EFI variables on those platforms.
> >>
> >> To do this, we first need to add some SCM call functions used to manage
> >> and talk to Secure Applications. A very small subset of this interface
> >> is added in the second patch (whereas the first one exports the required
> >> functions for that). Interface specifications are extracted from [1].
> >> While this does not (yet) support re-entrant SCM calls (including
> >> callbacks and listeners), this is enough to talk to the aforementioned
> >> uefisecapp on a couple of platforms (I've tested this on a Surface Pro X
> >> and heard reports from Lenovo Flex 5G, Lenovo Thinkpad x13s, and Lenovo
> >> Yoga C630 devices).
> >>
> >> The third patch adds a client driver for uefisecapp, installing the
> >> respective efivar operations. The application interface has been reverse
> >> engineered from the Windows QcTrEE8180.sys driver.
> >>
> >> Apart from uefisecapp, there are more Secure Applications running that
> >> we might want to support in the future. For example, on the Surface Pro
> >> X (sc8180x-based), the TPM is also managed via one.
> >>
> >> I'm not sure whether this should go to drivers/firmware or to
> >> drivers/soc/qcom. I've put this into firmware as all of this is
> >> essentially an interface to the secure firmware running in the TrustZone
> >> (and SCM stuff is handled here already), but please let me know if I
> >> should move this.
> >
> >  From what I see so far is that this is adapted from downstream qseecom driver, this approach could work for a limited usecases but not scalable, as we cannot add drivers for each Qualcomm specific TA in kernel.
> > This has to be handled in much generic way using Linux TEE framework, and let the userspace side deal with TA specific bits.
>
> I generally agree with the sentiment, however UEFI variables should IMHO be
> handled by the kernel. Moving handling of those to userspace breaks things like
> EFI-based pstore and efivarfs. The latter will in turn break some user-space
> tools (most notably efibootmgr used by e.g. GRUB and I think fwupdmgr which
> needs to set some capsule variables). Ideally, we would find a way to not break
> these, i.e. have them work out-of-the-box.
>

Only capsule-on-disk requires SetVariable() at runtime, and I doubt
whether these platforms implement any of that.

> A similar argumentation might apply to the TPM app.
>

There is a difference, though - the TPM is modeled as a device and
runtime access to it is implemented as a device driver, which is only
accessed from user space.

> > AFAIU, Qualcomm is moving away from qseecom interface to new smc-invoke interface, most of Qualcomm SoCs starting from SDM660 already have support to this.
> >
> > This interface provides a better abstracted IPC mechanism to talk to TA. Most of these TA specific interfaces are packed in closed userspace source.
> > Having said that QTEE smcinvoke driver can be modeled as a proper TEE driver with Userspace driving the TA specific bits using existing tee uapis.
> > This also brings in other features like loading, Listeners aka callbacks, secure memory allocations..etc.
> >
> > In the past, I have tried to do a prototype of this smcinvoke driver as a proper tee driver, incase you are interested patches are at https://git.linaro.org/landing-teams/working/qualcomm/kernel.git/log/?h=tracking-qcomlt-qcomtee
> > Plan is to discuss with Qualcomm and send it for upstream review.
>
> Thanks for this information! So as far as I understand it, this is currently an
> interface to user-space only, i.e. does not allow in-kernel drivers for apps?
> It would be great if this could then be extended to handle (the bare minimum
> of) in-kernel drivers (i.e. only things that the kernel itself needs, like EFI
> variables). Alternatively, I'm happy to hear suggestions on how we not break
> the aforementioned things while moving handling off to userspace.
>
> > I think its worth exploring if uefisecapp can talk smcinvoke.
> > I can ping Qualcomm engineers to see if that is doable.
>
> I think that would be great! Thanks!
>
> Regards,
> Max
Maximilian Luz Aug. 2, 2022, 7:11 p.m. UTC | #48
On 8/2/22 16:02, Ard Biesheuvel wrote:
> On Tue, 2 Aug 2022 at 15:22, Maximilian Luz <luzmaximilian@gmail.com> wrote:

[...]

>> I generally agree with the sentiment, however UEFI variables should IMHO be
>> handled by the kernel. Moving handling of those to userspace breaks things like
>> EFI-based pstore and efivarfs. The latter will in turn break some user-space
>> tools (most notably efibootmgr used by e.g. GRUB and I think fwupdmgr which
>> needs to set some capsule variables). Ideally, we would find a way to not break
>> these, i.e. have them work out-of-the-box.
>>
> 
> Only capsule-on-disk requires SetVariable() at runtime, and I doubt
> whether these platforms implement any of that.
> 
>> A similar argumentation might apply to the TPM app.
>>
> 
> There is a difference, though - the TPM is modeled as a device and
> runtime access to it is implemented as a device driver, which is only
> accessed from user space.

Ah, thanks for that info! I wasn't sure about that last part.

But we'd still need _something_ in the kernel. All the common software
using TPMs would expect the TPM to be present as /dev/tpmX. So, while it
doesn't have to be a full secure-app driver, we'd need at least some way
to manage a TPM device from user-space (unless we want to tell all
software using TPMs to just support some non-standard thing instead).

For EFI variables, something similar might be possible (i.e. running
efivar operations through a user-space driver), but that will break
pstore in the times it's most usable (i.e. when no user-space exists or
things are sufficiently broken that we can't run things through it any
more).

And then (at least for me) there's the question whether that all seems
sound: Sure, we can maintain some userspace-daemon outside the kernel,
but if it is common enough (i.e. not a one-off used only by some single
vendor and model) and can be easily implemented in the kernel, why not?
Moving it to userspace makes things more complex. You'll need new
userspace APIs (as mentioned above, if you don't want to force all
existing software to adapt to some non-standard thing) and you need to
tell users to install and set up some daemon(s) (making it yet more
difficult to produce a single proper install media that works well on
all the common AArch64 or WoA platforms). All the while you still need
to maintain essentially the same piece of code (whether it is inside or
outside of the kernel), so you don't really win anything there either.

Regards,
Max
Sumit Garg Sept. 2, 2022, 7:26 a.m. UTC | #49
Hi Maximilian,

On 02/08/22 18:52, Maximilian Luz wrote:
>
>
> On 8/2/22 13:51, Srinivas Kandagatla wrote:
>> Hi Maximilian,
>>
>> On 23/07/2022 23:49, Maximilian Luz wrote:
>>> On modern Qualcomm platforms, access to EFI variables is restricted to
>>> the secure world / TrustZone, i.e. the Trusted Execution Environment
>>> (TrEE or TEE) as Qualcomm seems to call it. To access EFI variables, we
>>> therefore need to talk to the UEFI Secure Application (uefisecapp),
>>> residing in the TrEE.
>>>
>>> This series adds support for accessing EFI variables on those 
>>> platforms.
>>>
>>> To do this, we first need to add some SCM call functions used to manage
>>> and talk to Secure Applications. A very small subset of this interface
>>> is added in the second patch (whereas the first one exports the 
>>> required
>>> functions for that). Interface specifications are extracted from [1].
>>> While this does not (yet) support re-entrant SCM calls (including
>>> callbacks and listeners), this is enough to talk to the aforementioned
>>> uefisecapp on a couple of platforms (I've tested this on a Surface 
>>> Pro X
>>> and heard reports from Lenovo Flex 5G, Lenovo Thinkpad x13s, and Lenovo
>>> Yoga C630 devices).
>>>
>>> The third patch adds a client driver for uefisecapp, installing the
>>> respective efivar operations. The application interface has been 
>>> reverse
>>> engineered from the Windows QcTrEE8180.sys driver.
>>>
>>> Apart from uefisecapp, there are more Secure Applications running that
>>> we might want to support in the future. For example, on the Surface Pro
>>> X (sc8180x-based), the TPM is also managed via one.
>>>
>>> I'm not sure whether this should go to drivers/firmware or to
>>> drivers/soc/qcom. I've put this into firmware as all of this is
>>> essentially an interface to the secure firmware running in the 
>>> TrustZone
>>> (and SCM stuff is handled here already), but please let me know if I
>>> should move this.
>>
>>  From what I see so far is that this is adapted from downstream 
>> qseecom driver, this approach could work for a limited usecases but 
>> not scalable, as we cannot add drivers for each Qualcomm specific TA 
>> in kernel.
>> This has to be handled in much generic way using Linux TEE framework, 
>> and let the userspace side deal with TA specific bits.
>
> I generally agree with the sentiment, however UEFI variables should 
> IMHO be
> handled by the kernel. Moving handling of those to userspace breaks 
> things like
> EFI-based pstore and efivarfs. The latter will in turn break some 
> user-space
> tools (most notably efibootmgr used by e.g. GRUB and I think fwupdmgr 
> which
> needs to set some capsule variables). Ideally, we would find a way to 
> not break
> these, i.e. have them work out-of-the-box.
>
> A similar argumentation might apply to the TPM app.

See below, there is already an existing TPM app driver [2] in kernel 
although the app is based on OP-TEE.

>
>> AFAIU, Qualcomm is moving away from qseecom interface to new 
>> smc-invoke interface, most of Qualcomm SoCs starting from SDM660 
>> already have support to this.
>>
>> This interface provides a better abstracted IPC mechanism to talk to 
>> TA. Most of these TA specific interfaces are packed in closed 
>> userspace source.
>> Having said that QTEE smcinvoke driver can be modeled as a proper TEE 
>> driver with Userspace driving the TA specific bits using existing tee 
>> uapis.
>> This also brings in other features like loading, Listeners aka 
>> callbacks, secure memory allocations..etc.
>>
>> In the past, I have tried to do a prototype of this smcinvoke driver 
>> as a proper tee driver, incase you are interested patches are at 
>> https://git.linaro.org/landing-teams/working/qualcomm/kernel.git/log/?h=tracking-qcomlt-qcomtee
>> Plan is to discuss with Qualcomm and send it for upstream review.
>
> Thanks for this information! So as far as I understand it, this is 
> currently an
> interface to user-space only, i.e. does not allow in-kernel drivers 
> for apps?

The Linux TEE framework already provides an in-kernel interface to TEE 
as well via TEE bus [1]. There are already multiple kernel drivers [2] 
[3] [4] [5] [6] [7] using it. So an EFI driver can be an addition to that.

Now coming on to TEE implementations, the drivers I mentioned are based 
on OP-TEE where devices are queried/enumerated during OP-TEE probe here 
[8]. So in similar manner QTEE smcinvoke driver should be able to 
register devices on the TEE bus.

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/staging/tee.rst#n56

[2] drivers/char/tpm/tpm_ftpm_tee.c

[3] drivers/char/hw_random/optee-rng.c

[4] drivers/firmware/arm_scmi/optee.c

[5] security/keys/trusted-keys/trusted_tee.c

[6] drivers/firmware/broadcom/tee_bnxt_fw.c

[7] drivers/rtc/rtc-optee.c

[8] drivers/tee/optee/device.c

-Sumit

PS. TBH, I haven't looked into detail workings for the QTEE smcinvoke 
driver.

> It would be great if this could then be extended to handle (the bare 
> minimum
> of) in-kernel drivers (i.e. only things that the kernel itself needs, 
> like EFI
> variables). Alternatively, I'm happy to hear suggestions on how we not 
> break
> the aforementioned things while moving handling off to userspace.
>
>> I think its worth exploring if uefisecapp can talk smcinvoke.
>> I can ping Qualcomm engineers to see if that is doable.
>
> I think that would be great! Thanks!
>
> Regards,
> Max
>
Maximilian Luz Sept. 2, 2022, 1:18 p.m. UTC | #50
Hi,

On 9/2/22 09:26, Sumit Garg wrote:
> Hi Maximilian,
> 
> On 02/08/22 18:52, Maximilian Luz wrote:

[...]

>> Thanks for this information! So as far as I understand it, this is currently an
>> interface to user-space only, i.e. does not allow in-kernel drivers for apps?
> 
> The Linux TEE framework already provides an in-kernel interface to TEE as well via TEE bus [1]. There are already multiple kernel drivers [2] [3] [4] [5] [6] [7] using it. So an EFI driver can be an addition to that.
> 
> Now coming on to TEE implementations, the drivers I mentioned are based on OP-TEE where devices are queried/enumerated during OP-TEE probe here [8]. So in similar manner QTEE smcinvoke driver should be able to register devices on the TEE bus.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/staging/tee.rst#n56
> 
> [2] drivers/char/tpm/tpm_ftpm_tee.c
> 
> [3] drivers/char/hw_random/optee-rng.c
> 
> [4] drivers/firmware/arm_scmi/optee.c
> 
> [5] security/keys/trusted-keys/trusted_tee.c
> 
> [6] drivers/firmware/broadcom/tee_bnxt_fw.c
> 
> [7] drivers/rtc/rtc-optee.c
> 
> [8] drivers/tee/optee/device.c

Thanks for those links!

I think it would indeed be good if we could make it work via that
interface and I guess that should generally be possible. As far as I can
see, the biggest problem might be that the current firmware doesn't seem
to use UUIDs, so I guess we might need to emulate them somehow.

It would be great if someone with some actual knowledge of the firmware
used on those devices could have a look at this and provide some
insights.

My plan for now is to hold off on the UEFI variable driver until we have
a (proper) TEE driver, which unfortunately might be a bit out of my
depth. I'm happy to help out in any way I can though.

Regards,
Max
Sumit Garg Sept. 5, 2022, 6:50 a.m. UTC | #51
+ TEE ML

On Fri, 2 Sept 2022 at 18:48, Maximilian Luz <luzmaximilian@gmail.com> wrote:
>
> Hi,
>
> On 9/2/22 09:26, Sumit Garg wrote:
> > Hi Maximilian,
> >
> > On 02/08/22 18:52, Maximilian Luz wrote:
>
> [...]
>
> >> Thanks for this information! So as far as I understand it, this is currently an
> >> interface to user-space only, i.e. does not allow in-kernel drivers for apps?
> >
> > The Linux TEE framework already provides an in-kernel interface to TEE as well via TEE bus [1]. There are already multiple kernel drivers [2] [3] [4] [5] [6] [7] using it. So an EFI driver can be an addition to that.
> >
> > Now coming on to TEE implementations, the drivers I mentioned are based on OP-TEE where devices are queried/enumerated during OP-TEE probe here [8]. So in similar manner QTEE smcinvoke driver should be able to register devices on the TEE bus.
> >
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/staging/tee.rst#n56
> >
> > [2] drivers/char/tpm/tpm_ftpm_tee.c
> >
> > [3] drivers/char/hw_random/optee-rng.c
> >
> > [4] drivers/firmware/arm_scmi/optee.c
> >
> > [5] security/keys/trusted-keys/trusted_tee.c
> >
> > [6] drivers/firmware/broadcom/tee_bnxt_fw.c
> >
> > [7] drivers/rtc/rtc-optee.c
> >
> > [8] drivers/tee/optee/device.c
>
> Thanks for those links!
>
> I think it would indeed be good if we could make it work via that
> interface and I guess that should generally be possible. As far as I can
> see, the biggest problem might be that the current firmware doesn't seem
> to use UUIDs, so I guess we might need to emulate them somehow.
>

Okay, so I had a brief look at your driver to get an idea how QTEE
identifies its trusted/secure applications. AFAIU, it uses constant
strings as follows:

#define QCTEE_UEFISEC_APP_NAME "qcom.tz.uefisecapp"

I think we should be able to extend the TEE bus concept to accept
constant strings as device IDs as well. So if a driver wants to
support both OP-TEE and QTEE based apps then it can put corresponding
identifiers (UUID or a constant string) in the TEE device match ID
table. This way we should be able to support other TEE implementations
as I think any other identifier apart from UUID can be represented as
a constant string.

If anyone else has any better then feel free to discuss.

-Sumit

> It would be great if someone with some actual knowledge of the firmware
> used on those devices could have a look at this and provide some
> insights.
>
> My plan for now is to hold off on the UEFI variable driver until we have
> a (proper) TEE driver, which unfortunately might be a bit out of my
> depth. I'm happy to help out in any way I can though.
>
> Regards,
> Max
Srinivas Kandagatla Nov. 23, 2022, 11:22 a.m. UTC | #52
Hi Max,

On 02/08/2022 14:22, Maximilian Luz wrote:
> 
> 
> On 8/2/22 13:51, Srinivas Kandagatla wrote:
>> Hi Maximilian,
>>
>> On 23/07/2022 23:49, Maximilian Luz wrote:
>>> On modern Qualcomm platforms, access to EFI variables is restricted to
>>> the secure world / TrustZone, i.e. the Trusted Execution Environment
>>> (TrEE or TEE) as Qualcomm seems to call it. To access EFI variables, we
>>> therefore need to talk to the UEFI Secure Application (uefisecapp),
>>> residing in the TrEE.
>>>
>>> This series adds support for accessing EFI variables on those platforms.
>>>
>>> To do this, we first need to add some SCM call functions used to manage
>>> and talk to Secure Applications. A very small subset of this interface
>>> is added in the second patch (whereas the first one exports the required
>>> functions for that). Interface specifications are extracted from [1].
>>> While this does not (yet) support re-entrant SCM calls (including
>>> callbacks and listeners), this is enough to talk to the aforementioned
>>> uefisecapp on a couple of platforms (I've tested this on a Surface Pro X
>>> and heard reports from Lenovo Flex 5G, Lenovo Thinkpad x13s, and Lenovo
>>> Yoga C630 devices).
>>>
>>> The third patch adds a client driver for uefisecapp, installing the
>>> respective efivar operations. The application interface has been reverse
>>> engineered from the Windows QcTrEE8180.sys driver.
>>>
>>> Apart from uefisecapp, there are more Secure Applications running that
>>> we might want to support in the future. For example, on the Surface Pro
>>> X (sc8180x-based), the TPM is also managed via one.
>>>
>>> I'm not sure whether this should go to drivers/firmware or to
>>> drivers/soc/qcom. I've put this into firmware as all of this is
>>> essentially an interface to the secure firmware running in the TrustZone
>>> (and SCM stuff is handled here already), but please let me know if I
>>> should move this.
>>
>>  From what I see so far is that this is adapted from downstream 
>> qseecom driver, this approach could work for a limited usecases but 
>> not scalable, as we cannot add drivers for each Qualcomm specific TA 
>> in kernel.
>> This has to be handled in much generic way using Linux TEE framework, 
>> and let the userspace side deal with TA specific bits.
> 
> I generally agree with the sentiment, however UEFI variables should IMHO be
> handled by the kernel. Moving handling of those to userspace breaks 
> things like
> EFI-based pstore and efivarfs. The latter will in turn break some 
> user-space
> tools (most notably efibootmgr used by e.g. GRUB and I think fwupdmgr which
> needs to set some capsule variables). Ideally, we would find a way to 
> not break
> these, i.e. have them work out-of-the-box.
> 
> A similar argumentation might apply to the TPM app.
> 
>> AFAIU, Qualcomm is moving away from qseecom interface to new 
>> smc-invoke interface, most of Qualcomm SoCs starting from SDM660 
>> already have support to this.
>>
>> This interface provides a better abstracted IPC mechanism to talk to 
>> TA. Most of these TA specific interfaces are packed in closed 
>> userspace source.
>> Having said that QTEE smcinvoke driver can be modeled as a proper TEE 
>> driver with Userspace driving the TA specific bits using existing tee 
>> uapis.
>> This also brings in other features like loading, Listeners aka 
>> callbacks, secure memory allocations..etc.
>>
>> In the past, I have tried to do a prototype of this smcinvoke driver 
>> as a proper tee driver, incase you are interested patches are at 
>> https://git.linaro.org/landing-teams/working/qualcomm/kernel.git/log/?h=tracking-qcomlt-qcomtee
>> Plan is to discuss with Qualcomm and send it for upstream review.
> 
> Thanks for this information! So as far as I understand it, this is 
> currently an
> interface to user-space only, i.e. does not allow in-kernel drivers for 
> apps?
> It would be great if this could then be extended to handle (the bare 
> minimum
> of) in-kernel drivers (i.e. only things that the kernel itself needs, 
> like EFI
> variables). Alternatively, I'm happy to hear suggestions on how we not 
> break
> the aforementioned things while moving handling off to userspace.
> 
>> I think its worth exploring if uefisecapp can talk smcinvoke.
>> I can ping Qualcomm engineers to see if that is doable.
> 
> I think that would be great! Thanks!
Sorry for such a long delay to reply to this,

here is what I have.

Access to TA using SCM calls remain valid and it will continue to work 
till SM8550 and probably after that if the TA is *NOT* loaded using 
smcinvoke for some reasons.

But overall by default on all new SoCs after sm8550 all the access to TA 
is only done via smcinvoke, and the underlying scm call that are used in 
this patchset will not be supported anymore.

 From SM8550, we will need to move this driver to a proper TEE client 
kernel driver.

So, with that in mind, I would suggest that we carefully name the 
compatibles based on SoC rather than generic ones.


--srini




> 
> Regards,
> Max
Maximilian Luz Nov. 23, 2022, 12:05 p.m. UTC | #53
Hi,

On 11/23/22 12:22, Srinivas Kandagatla wrote:
> Hi Max,
> 
> On 02/08/2022 14:22, Maximilian Luz wrote:
>>
>>
>> On 8/2/22 13:51, Srinivas Kandagatla wrote:

[...]

>>> I think its worth exploring if uefisecapp can talk smcinvoke.
>>> I can ping Qualcomm engineers to see if that is doable.
>>
>> I think that would be great! Thanks!
> Sorry for such a long delay to reply to this,
> 
> here is what I have.
> 
> Access to TA using SCM calls remain valid and it will continue to work till SM8550 and probably after that if the TA is *NOT* loaded using smcinvoke for some reasons.
> 
> But overall by default on all new SoCs after sm8550 all the access to TA is only done via smcinvoke, and the underlying scm call that are used in this patchset will not be supported anymore.
> 
>  From SM8550, we will need to move this driver to a proper TEE client kernel driver.
> 
> So, with that in mind, I would suggest that we carefully name the compatibles based on SoC rather than generic ones.

Thanks! That makes sense.

Regards,
Max