| Message ID | 20220723224949.1089973-5-luzmaximilian@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | firmware: Add support for Qualcomm UEFI Secure Application | expand |
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.
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
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
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
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.
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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.
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. [...]
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
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
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
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
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
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
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
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
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"; + }; + }; diff --git a/MAINTAINERS b/MAINTAINERS index 6e014e16fc82..00436245189d 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -16607,6 +16607,7 @@ QUALCOMM UEFISECAPP DRIVER M: Maximilian Luz <luzmaximilian@gmail.com> L: linux-arm-msm@vger.kernel.org S: Maintained +F: Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml F: drivers/firmware/qcom_tee_uefisecapp.c QUALCOMM VENUS VIDEO ACCELERATOR DRIVER
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