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