On 10/30/2024 21:38, Mario Limonciello wrote: > On 10/30/2024 11:03, Shyam Sundar S K wrote: >> Hi, >> >> On 10/30/2024 19:30, Hans de Goede wrote: >>> Hi, >>> >>> On 29-Oct-24 3:07 PM, Ilpo Järvinen wrote: >>>> Hi Hens, >>>> >>>> There a question / item needing your input below. >>>> >>>> On Wed, 23 Oct 2024, Mario Limonciello wrote: >>>>> 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@xxxxxxx> >>>>>>>>>>>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@xxxxxxx> >>>>>>>>>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx> >>>>>>>>>>> >>>>>>>>>>> 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. >>>> >>>> Distros and SW component management challenges are more in the >>>> domain of >>>> Hans' expertise so I'd prefer to hear his opinion on this. >>>> >>>> Personally I feel though that the commit message is not entirely >>>> honest >>>> on all the impact as is. The wordings are sounding quite innocent >>>> while if >>>> I infer the above right, an incorrect combination will cause a >>>> non-gracious failure. >>> >>> There are basically 4 possible scenarios and to me it >>> is only clear from this thread what will happen in 3 of >>> the 4 scenarios : >>> >>> 1. Old TA fw, Old kernel (TA_OUTPUT_RESERVED_MEM=906) -> works >>> 2. New TA fw, Old kernel (TA_OUTPUT_RESERVED_MEM=906) -> broken >>> 3. Old TA fw, new kernel (TA_OUTPUT_RESERVED_MEM=922) -> ??? >>> 4. New TA fw, new kernel (TA_OUTPUT_RESERVED_MEM=922) -> works >>> >>> If the answer to 3 is: "works" then I agree that this patch >>> should be submitted to Linus as a fix with Cc: stable ASAP >>> and then once that has hit most stable series it should be >>> ok to upgrade the fw in linux-firmware >>> >> >> Short answer, "yes" it does not work for "3." and you can consider it >> a broken. >> >>> Note this is still not ideal but IMHO it would be ok. >>> >>> But if the answer is "broken" then we will really need to >>> find some way to unbreak this, which could be as simple >>> as querying the fw-version and basing the size on this, >>> but having a kernel change which will regress things for >>> users who do not have the old firmware yet is simply >>> not acceptable. >>> >> >> I am not sure if there is a firmware versioning interface that the ASP >> (AMD Security Processor) returns back the kernel/driver. > > The code path in this case is: >> >> AMD PMF driver -> AMD TEE driver -> AMD CCP driver -> ASP TEE -> ASP >> TA -> ASP HW. >> >> So, I uncertain which module has this information and where exactly >> the code of fw versioning has to reside. It will take a while for me >> to dig this in. > > As a solution to this, can amd-pmf explicitly do it's own > request_firmware() call to load the firmware binary and determine the > size to use in the array and then discard the loaded binary? > > This would let the TEE module still do it's own load later like normal > without having to plumb this information across subsystems. > TEE driver feeds in a lot of metadata and the structure information for PSP headers and I don't think just having a request_firmware() will help. Sidebar, TEE driver has a lot of plumbing that can be used decrypt the policy binaries to debug issues related to TA load failures and policy binary issues (basically the descriptors) So we might end up in replicating a majority of TEE code into PMF driver, which might not be a good design choice. Let me first talk to internal folks to see if we can solve it by not making complex changes. Thanks, Shyam >> >> Meanwhile, shall I drop this patch and resend the series (by >> addressing the dev_dbg change Mario commented) so that this atleast >> becomes a 6.13 material? >> > >> Thanks, >> Shyam >> >>> Regards, >>> >>> Hans >>> >>> >>> >>> >
