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: 6650 bytes --]

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.

> > I assume this change in general is to unbreak the case "3." from the link?
> 
> Yes. that's right.

Thanks.

-- 
 i.