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

From: Maximilian Luz <luzmaximilian@gmail.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
	Andy Gross <agross@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Ard Biesheuvel <ardb@kernel.org>
Cc: Konrad Dybcio <konrad.dybcio@somainline.org>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Steev Klimaszewski <steev@kali.org>,
	Shawn Guo <shawn.guo@linaro.org>,
	Sudeep Holla <sudeep.holla@arm.com>,
	Cristian Marussi <cristian.marussi@arm.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-arm-msm@vger.kernel.org, linux-efi@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
	Vinod Koul <vkoul@kernel.org>
Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client
Date: Tue, 26 Jul 2022 17:00:43 +0200	[thread overview]
Message-ID: <2e522bcd-5d55-e87f-126c-514f5edaa560@gmail.com> (raw)
In-Reply-To: <11e5c369-c0da-7756-b9e2-ac375dc78e9d@linaro.org>

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