[PATCH v2 1/2] platform/x86/amd/pmf: Propagate PMF-TA return codes

[PATCH v2 1/2] platform/x86/amd/pmf: Propagate PMF-TA return codes

[Shyam Sundar S K]

In the amd_pmf_invoke_cmd_init() function within the PMF driver ensure
that the actual result from the PMF-TA is returned rather than a generic
EIO. This change allows for proper handling of errors originating from the
PMF-TA.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.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/tee-if.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
index 8c88769ea1d8..b404764550c4 100644
--- a/drivers/platform/x86/amd/pmf/tee-if.c
+++ b/drivers/platform/x86/amd/pmf/tee-if.c
@@ -323,7 +323,7 @@ static int amd_pmf_start_policy_engine(struct amd_pmf_dev *dev)
 	} else {
 		dev_err(dev->dev, "ta invoke cmd init failed err: %x\n", res);
 		dev->smart_pc_enabled = false;
-		return -EIO;
+		return res;
 	}
 
 	return 0;
-- 
2.34.1

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

[Shyam Sundar S K]

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);
+		if (ret)
+			return ret;
+
+		ret = amd_pmf_start_policy_engine(dev);
+		switch (ret) {
+		case TA_PMF_TYPE_SUCCESS:
+			status = true;
+			break;
+		case TA_ERROR_CRYPTO_INVALID_PARAM:
+		case TA_ERROR_CRYPTO_BIN_TOO_LARGE:
+			amd_pmf_tee_deinit(dev);
+			status = false;
+			break;
+		default:
+			goto error;
+		}
+
+		if (status)
+			break;
+	}
+
+	if (!status && !pb_side_load)
 		goto error;
 
 	if (pb_side_load)
-- 
2.34.1

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

[Mario Limonciello]

On 2/18/2025 06:06, 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>

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

Ilpo, Hans,

As you can tell this adds compatibility for both old TA and new TA.  But 
new TA won't work with older kernel.

So if you're amenable would you mind if this went to current 6.14-rc 
cycle and added stable tag?  I think it would be better to spread this 
widely to LTS kernel so that the new TA or old TA can both work on many 
kernels.

Thx,

> ---
>   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);
> +		if (ret)
> +			return ret;
> +
> +		ret = amd_pmf_start_policy_engine(dev);
> +		switch (ret) {
> +		case TA_PMF_TYPE_SUCCESS:
> +			status = true;
> +			break;
> +		case TA_ERROR_CRYPTO_INVALID_PARAM:
> +		case TA_ERROR_CRYPTO_BIN_TOO_LARGE:
> +			amd_pmf_tee_deinit(dev);
> +			status = false;
> +			break;
> +		default:
> +			goto error;
> +		}
> +
> +		if (status)
> +			break;
> +	}
> +
> +	if (!status && !pb_side_load)
>   		goto error;
>   
>   	if (pb_side_load)

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

[Ilpo Järvinen]

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"?

I assume this change in general is to unbreak the case "3." from the link?

> +		if (ret)
> +			return ret;
> +
> +		ret = amd_pmf_start_policy_engine(dev);
> +		switch (ret) {
> +		case TA_PMF_TYPE_SUCCESS:
> +			status = true;
> +			break;
> +		case TA_ERROR_CRYPTO_INVALID_PARAM:
> +		case TA_ERROR_CRYPTO_BIN_TOO_LARGE:
> +			amd_pmf_tee_deinit(dev);
> +			status = false;
> +			break;
> +		default:
> +			goto error;
> +		}
> +
> +		if (status)
> +			break;
> +	}
> +
> +	if (!status && !pb_side_load)
>  		goto error;
>  
>  	if (pb_side_load)
> 

-- 
 i.

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

[Shyam Sundar S K]

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 assume this change in general is to unbreak the case "3." from the link?

Yes. that's right.

Thanks,
Shyam

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

[Ilpo Järvinen]

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

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

[Shyam Sundar S K]

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

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

[Ilpo Järvinen]

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