Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <4e777590-616a-558a-031e-3ef1f1e492b4@gmail.com>
Date:   Thu, 28 Jul 2022 12:05:15 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure
 Application client
Content-Language: en-US
To:     Sudeep Holla <sudeep.holla@arm.com>
Cc:     Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Ard Biesheuvel <ardb@kernel.org>,
        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>,
        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
References: <20220723224949.1089973-1-luzmaximilian@gmail.com>
 <20220723224949.1089973-5-luzmaximilian@gmail.com>
 <20220726143005.wt4be7yo7sbd3xut@bogus>
 <829c8fee-cae5-597d-933d-784b4b57bd73@gmail.com>
 <20220726154138.74avqs6iqlzqpzjk@bogus>
 <d1bc99bb-82ce-aa6e-7fad-e9309fa1c19b@gmail.com>
 <20220728082330.w4ppmzvjaeywsglu@bogus>
From:   Maximilian Luz <luzmaximilian@gmail.com>
In-Reply-To: <20220728082330.w4ppmzvjaeywsglu@bogus>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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