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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 7190 bytes --]

On Tue, 4 Mar 2025, Shyam Sundar S K wrote:
> 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?

Yes.

-- 
 i.