diff mbox series

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

Message ID 20241023063245.1404420-6-Shyam-sundar.S-k@amd.com (mailing list archive)
State New
Headers show
Series platform/x86/amd/pmf: Updates to AMD PMF driver | expand

Commit Message

Shyam Sundar S K Oct. 23, 2024, 6:32 a.m. UTC
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>
---
 drivers/platform/x86/amd/pmf/pmf.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Mario Limonciello Oct. 23, 2024, 2:11 p.m. UTC | #1
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.
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?

2) Or is there a command to the TA that can query the expected output size?

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.

> ---
>   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
Shyam Sundar S K Oct. 23, 2024, 2:29 p.m. UTC | #2
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.

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

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

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
>
Mario Limonciello Oct. 23, 2024, 2:34 p.m. UTC | #3
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?

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

> 
> 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
>>
Shyam Sundar S K Oct. 23, 2024, 3:32 p.m. UTC | #4
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
>>>
>
Mario Limonciello Oct. 23, 2024, 3:40 p.m. UTC | #5
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.
Shyam Sundar S K Oct. 23, 2024, 3:52 p.m. UTC | #6
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.


> * 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
Mario Limonciello Oct. 23, 2024, 4:20 p.m. UTC | #7
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.
diff mbox series

Patch

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