Re: [PATCH v2 2/2] platform/x86/amd/pmf: Update PMF Driver for Compatibility with new PMF-TA

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

On 3/4/2025 21:02, Ilpo Järvinen wrote:
> On Tue, 4 Mar 2025, Shyam Sundar S K wrote:
>> On 3/4/2025 19:16, Ilpo Järvinen wrote:
>>> On Tue, 18 Feb 2025, Shyam Sundar S K wrote:
>>>
>>>> The PMF driver allocates a shared memory buffer using
>>>> tee_shm_alloc_kernel_buf() 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.
>>>>
>>>> To do so, introduce the new PMF-TA UUID and update the PMF shared memory
>>>> configuration to ensure compatibility with the latest PMF-TA version.
>>>> Additionally, export the TA UUID.
>>>>
>>>> These updates will result in modifications to the prototypes of
>>>> amd_pmf_tee_init() and amd_pmf_ta_open_session().
>>>>
>>>> Link: https://lore.kernel.org/all/55ac865f-b1c7-fa81-51c4-d211c7963e7e@linux.intel.com/
>>>> 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    |  5 ++-
>>>>  drivers/platform/x86/amd/pmf/tee-if.c | 50 +++++++++++++++++++--------
>>>>  2 files changed, 40 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
>>>> index 41b2b91b8fdc..e6bdee68ccf3 100644
>>>> --- a/drivers/platform/x86/amd/pmf/pmf.h
>>>> +++ b/drivers/platform/x86/amd/pmf/pmf.h
>>>> @@ -106,9 +106,12 @@ 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 TA_ERROR_CRYPTO_INVALID_PARAM				0x20002
>>>> +#define TA_ERROR_CRYPTO_BIN_TOO_LARGE				0x2000d
>>>> +
>>>>  #define PMF_IF_V1		1
>>>>  #define PMF_IF_V2		2
>>>>  
>>>> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
>>>> index b404764550c4..a81c661abd7e 100644
>>>> --- a/drivers/platform/x86/amd/pmf/tee-if.c
>>>> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
>>>> @@ -27,8 +27,11 @@ module_param(pb_side_load, bool, 0444);
>>>>  MODULE_PARM_DESC(pb_side_load, "Sideload policy binaries debug policy failures");
>>>>  #endif
>>>>  
>>>> -static const uuid_t amd_pmf_ta_uuid = UUID_INIT(0x6fd93b77, 0x3fb8, 0x524d,
>>>> -						0xb1, 0x2d, 0xc5, 0x29, 0xb1, 0x3d, 0x85, 0x43);
>>>> +static const uuid_t amd_pmf_ta_uuid[] = { UUID_INIT(0xd9b39bf2, 0x66bd, 0x4154, 0xaf, 0xb8, 0x8a,
>>>> +						    0xcc, 0x2b, 0x2b, 0x60, 0xd6),
>>>> +					  UUID_INIT(0x6fd93b77, 0x3fb8, 0x524d, 0xb1, 0x2d, 0xc5,
>>>> +						    0x29, 0xb1, 0x3d, 0x85, 0x43),
>>>> +					};
>>>>  
>>>>  static const char *amd_pmf_uevent_as_str(unsigned int state)
>>>>  {
>>>> @@ -321,7 +324,7 @@ static int amd_pmf_start_policy_engine(struct amd_pmf_dev *dev)
>>>>  		 */
>>>>  		schedule_delayed_work(&dev->pb_work, msecs_to_jiffies(pb_actions_ms * 3));
>>>>  	} else {
>>>> -		dev_err(dev->dev, "ta invoke cmd init failed err: %x\n", res);
>>>> +		dev_dbg(dev->dev, "ta invoke cmd init failed err: %x\n", res);
>>>>  		dev->smart_pc_enabled = false;
>>>>  		return res;
>>>>  	}
>>>> @@ -390,12 +393,12 @@ static int amd_pmf_amdtee_ta_match(struct tee_ioctl_version_data *ver, const voi
>>>>  	return ver->impl_id == TEE_IMPL_ID_AMDTEE;
>>>>  }
>>>>  
>>>> -static int amd_pmf_ta_open_session(struct tee_context *ctx, u32 *id)
>>>> +static int amd_pmf_ta_open_session(struct tee_context *ctx, u32 *id, int index)
>>>>  {
>>>>  	struct tee_ioctl_open_session_arg sess_arg = {};
>>>>  	int rc;
>>>>  
>>>> -	export_uuid(sess_arg.uuid, &amd_pmf_ta_uuid);
>>>> +	export_uuid(sess_arg.uuid, &amd_pmf_ta_uuid[index]);
>>>>  	sess_arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC;
>>>>  	sess_arg.num_params = 0;
>>>>  
>>>> @@ -434,7 +437,7 @@ static int amd_pmf_register_input_device(struct amd_pmf_dev *dev)
>>>>  	return 0;
>>>>  }
>>>>  
>>>> -static int amd_pmf_tee_init(struct amd_pmf_dev *dev)
>>>> +static int amd_pmf_tee_init(struct amd_pmf_dev *dev, int index)
>>>>  {
>>>>  	u32 size;
>>>>  	int ret;
>>>> @@ -445,7 +448,7 @@ static int amd_pmf_tee_init(struct amd_pmf_dev *dev)
>>>>  		return PTR_ERR(dev->tee_ctx);
>>>>  	}
>>>>  
>>>> -	ret = amd_pmf_ta_open_session(dev->tee_ctx, &dev->session_id);
>>>> +	ret = amd_pmf_ta_open_session(dev->tee_ctx, &dev->session_id, index);
>>>>  	if (ret) {
>>>>  		dev_err(dev->dev, "Failed to open TA session (%d)\n", ret);
>>>>  		ret = -EINVAL;
>>>> @@ -489,7 +492,8 @@ static void amd_pmf_tee_deinit(struct amd_pmf_dev *dev)
>>>>  
>>>>  int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>>>>  {
>>>> -	int ret;
>>>> +	bool status;
>>>> +	int ret, i;
>>>>  
>>>>  	ret = apmf_check_smart_pc(dev);
>>>>  	if (ret) {
>>>> @@ -502,10 +506,6 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>>>>  		return -ENODEV;
>>>>  	}
>>>>  
>>>> -	ret = amd_pmf_tee_init(dev);
>>>> -	if (ret)
>>>> -		return ret;
>>>> -
>>>>  	INIT_DELAYED_WORK(&dev->pb_work, amd_pmf_invoke_cmd);
>>>>  
>>>>  	ret = amd_pmf_set_dram_addr(dev, true);
>>>> @@ -534,8 +534,30 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>>>>  		goto error;
>>>>  	}
>>>>  
>>>> -	ret = amd_pmf_start_policy_engine(dev);
>>>> -	if (ret)
>>>> +	for (i = 0; i < ARRAY_SIZE(amd_pmf_ta_uuid); i++) {
>>>> +		ret = amd_pmf_tee_init(dev, i);
>>>
>>> Any reason why you just pass the uuid pointer as it seems more obvious as 
>>> a parameter than something as vague as "index"?
>>>
>>
>> The objective is to select the appropriate Trusted Application (TA)
>> binary from the /lib/firmware/amdtee/ directory. This selection is
>> determined by the UUIDs listed in the amd_pmf_ta_uuid[].
>>
>> Typically, the most recent TA version should be located at the first
>> index, with the next most recent version at the second index, and so on.
>>
>> All these had to be done so that we don't end up in the version
>> compatibility issues we had encountered last time.
> 
> I'm sorry, my editing messed the meaning of that comment up (I forgot to 
> add the "not" word there). I only meant that you could pass uuid pointer instead
> of the index.

OK. I can respin, just to make sure are you expecting this?

//ret = amd_pmf_tee_init(dev, i);
ret = amd_pmf_tee_init(dev, &amd_pmf_ta_uuid[i]);

so that I propogate the uuid to the export_guid() rather than the index?

Thanks,
Shyam