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

[Maximilian Luz <luzmaximilian@gmail.com>]

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