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: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.

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.

* 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.