Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

[Maximilian Luz]

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