Re: [PATCH v3 5/5] platform/x86/amd/pmf: Add PMF driver changes to make compatible with PMF-TA

[Mario Limonciello <mario.limonciello@amd.com>]

On 10/23/2024 10:52, Shyam Sundar S K wrote:
> 
> 
> On 10/23/2024 21:10, Mario Limonciello wrote:
>> On 10/23/2024 10:32, Shyam Sundar S K wrote:
>>>
>>>
>>> On 10/23/2024 20:04, Mario Limonciello wrote:
>>>> On 10/23/2024 09:29, Shyam Sundar S K wrote:
>>>>>
>>>>>
>>>>> On 10/23/2024 19:41, Mario Limonciello wrote:
>>>>>> On 10/23/2024 01:32, Shyam Sundar S K wrote:
>>>>>>> The PMF driver will allocate shared buffer memory using the
>>>>>>> tee_shm_alloc_kernel_buf(). This allocated memory is located in the
>>>>>>> secure world and is used for communication with the PMF-TA.
>>>>>>>
>>>>>>> The latest PMF-TA version introduces new structures with OEM debug
>>>>>>> information and additional policy input conditions for
>>>>>>> evaluating the
>>>>>>> policy binary. Consequently, the shared memory size must be
>>>>>>> increased to
>>>>>>> ensure compatibility between the PMF driver and the updated PMF-TA.
>>>>>>>
>>>>>>> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>>>>>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>>>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>>>>>>
>>>>>> How does this present to a user?  From what you describe it seems to
>>>>>> me like this means a new TA will fail on older kernel in some way.
>>>>>
>>>>> Newer TA will not fail on older systems. This change is just about
>>>>> the
>>>>> increase in TA reserved memory that is presented as "shared memory",
>>>>> as TA needs the additional memory for its own debug data structures.
>>>>
>>>> Thx for comments. But so if you use new TA with older kernel driver,
>>>> what will happen?  Can TA do a buffer overrun because the presented
>>>> shared memory was too small?
>>>>
>>>
>>> New TA will fail on older kernel and hence this change will be
>>> required for new TA to work.
>>
>> OK, that's what I was worried about.
>>
>>>
>>>>>
>>>>>    From user standpoint, always be on latest FW, irrespective of the
>>>>> platform. At this point in time, I don't see a need for FW versioning
>>>>> name (in the future, if there is a need for having a limited support
>>>>> to older platforms, we can carve out a logic to do versioning stuff).
>>>>
>>>> I wish we could enforce this, but In the Linux world there is an
>>>> expectation that these two trains don't need to arrive at station at
>>>> the same time.
>>>>
>>>>>
>>>>>> Some ideas:
>>>>>>
>>>>>> 1) Should there be header version check on the TA and dynamically
>>>>>> allocate the structure size based on the version of the F/W?
>>>>>>
>>>>>
>>>>> This can be done, when the TA versioning upgrade happens, like from
>>>>> 1.3 to 1.4, apart from that there is no header stuff association.
>>>>>
>>>>>> 2) Or is there a command to the TA that can query the expected
>>>>>> output
>>>>>> size?
>>>>>>
>>>>>
>>>>> No, this is just the initial shared memory that the driver allocates
>>>>> to pass the inputs and the commands to TA.
>>>>>
>>>>>> 3) Or should the new TA filename be versioned, and the driver has a
>>>>>> fallback policy?
>>>>>>
>>>>>> Whatever the outcome is; I think it's best that if possible this
>>>>>> change goes back to stable to try to minimize regressions to
>>>>>> users as
>>>>>> distros update linux-firmware.  For example Fedora updates this
>>>>>> monthly, but also tracks stable kernels.
>>>>>>
>>>>>
>>>>> Advisory to distros should be to pick the latest PMF TA (note that, I
>>>>> have not still submitted to new TA FW).
>>>>
>>>> Yeah we can advise distros to pick it up when upstreamed as long as
>>>> there isn't tight dependency on this patch being present.
>>>>
>>>
>>> That is the reason I am waiting for this change to land. Once that is
>>> done, I will submit the new TA, you can send out a advisory to upgrade
>>> the kernel or this change has to be back-ported to stable/oem kernels
>>> for their enablement.
>>>
>>> Makes sense?
>>>
>>
>> I think we need Hans' and Ilpo's comments here to decide what to do.
>>
> 
> Sure.
> 
>> I will say that when we had this happen in amdgpu for a breaking
>> reason there was a new firmware binary filename created/upstreamed for
>> the breaking version (IIRC foo.bin -> foo_1.bin) and amdgpu had to
>> have fallback code so it could be compatible with either binary.
>>
> 
> True. In case of amdgpu, the FW loading is part of the amdgpu driver.
> But in case of PMF, the PMF TA gets picked from the AMD TEE driver
> through the TEE commands.
> 
> So, there is no need for FW versioning logic in PMF driver.
> 

That's a very good point, and this is a lot of complexity then.

> 
>> * If user on older kernel took newer linux-firmware package they used
>> older binary.
>> * If user on newer kernel took older linux-firmware package they used
>> older binary.
>> * If user on newer kernel took newer linux-firmware package they used
>> newer binary.
>>
>> If the decision is this goes in "as is" it definitely needs to go back
>> to stable kernels.
>>
> 
> IMHO, let's not put too many fallback mechanisms. The philosophy
> should be use latest driver and latest FW that avoids a lot of
> confusion and yeah for that to happen this change has to go to stable.
> 
> Thanks,
> Shyam

Of course Hans and Ilpo make the final call, but I think from our 
discussions here it would be ideal that patch 1 and patch 5 from this 
series go into 6.12 and have stable tags, the rest would be 6.13 material.