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

[Shyam Sundar S K <Shyam-sundar.S-k@amd.com>]

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.

>>
>>  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?

Thanks,
Shyam

>>
>> Thanks,
>> Shyam
>>
>>>> ---
>>>>    drivers/platform/x86/amd/pmf/pmf.h | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h
>>>> b/drivers/platform/x86/amd/pmf/pmf.h
>>>> index a79808fda1d8..18f12aad46a9 100644
>>>> --- a/drivers/platform/x86/amd/pmf/pmf.h
>>>> +++ b/drivers/platform/x86/amd/pmf/pmf.h
>>>> @@ -106,7 +106,7 @@ struct cookie_header {
>>>>    #define PMF_TA_IF_VERSION_MAJOR                1
>>>>    #define TA_PMF_ACTION_MAX                    32
>>>>    #define TA_PMF_UNDO_MAX                        8
>>>> -#define TA_OUTPUT_RESERVED_MEM                906
>>>> +#define TA_OUTPUT_RESERVED_MEM                922
>>>>    #define MAX_OPERATION_PARAMS                    4
>>>>      #define PMF_IF_V1        1
>>>
>