Message-ID: <acd7b231-3167-e35c-5cdf-8b3127a7d710@gmail.com>
Date:   Wed, 27 Jul 2022 15:00:48 +0200
From:   Maximilian Luz <luzmaximilian@...il.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Ard Biesheuvel <ardb@...nel.org>
Cc:     Konrad Dybcio <konrad.dybcio@...ainline.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Steev Klimaszewski <steev@...i.org>,
        Shawn Guo <shawn.guo@...aro.org>,
        Sudeep Holla <sudeep.holla@....com>,
        Cristian Marussi <cristian.marussi@....com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-arm-msm@...r.kernel.org, linux-efi@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Vinod Koul <vkoul@...nel.org>
Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure
 Application client

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

Powered by blists - more mailing lists