| Message ID | 20250217064937.98702-2-Shyam-sundar.S-k@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] platform/x86/amd/pmf: Propagate PMF-TA return codes | expand |
On 2/17/2025 00:49, 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 Small nit that this should be s/Introduce/introduce/. I wouldn't change it if nothing else is raised. > 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 | 49 +++++++++++++++++++-------- > 2 files changed, 38 insertions(+), 16 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..43bda6f98a11 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,9 +534,28 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev) > goto error; > } > > - ret = amd_pmf_start_policy_engine(dev); > - if (ret) > - goto error; > + 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; > + } > After the loop I think you're missing one more case of if (!status) goto error; Otherwise can't you potentially end up with both attempts returning an error code? If you think it makes sense to still be able to "sideload" a PB in this case perhaps it would be best to change it to if (!status && !pb_side_load) goto error; > if (pb_side_load) > amd_pmf_open_pb(dev, dev->dbgfs_dir);
On 2/17/2025 23:00, Mario Limonciello wrote: > On 2/17/2025 00:49, 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 > > Small nit that this should be s/Introduce/introduce/. I wouldn't > change it if nothing else is raised. > Ack. >> 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 | 49 ++++++++++++++++++ >> +-------- >> 2 files changed, 38 insertions(+), 16 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..43bda6f98a11 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,9 +534,28 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev) >> goto error; >> } >> - ret = amd_pmf_start_policy_engine(dev); >> - if (ret) >> - goto error; >> + 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; >> + } >> > > After the loop I think you're missing one more case of > > if (!status) > goto error; > > Otherwise can't you potentially end up with both attempts returning an > error code? > > If you think it makes sense to still be able to "sideload" a PB in > this case perhaps it would be best to change it to > > if (!status && !pb_side_load) > goto error; > >> if (pb_side_load) >> amd_pmf_open_pb(dev, dev->dbgfs_dir); > I thought sideload will take a different path. Let me respin a new version based on your thoughts. Thanks, Shyam
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..43bda6f98a11 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,9 +534,28 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev) goto error; } - ret = amd_pmf_start_policy_engine(dev); - if (ret) - goto error; + 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 (pb_side_load) amd_pmf_open_pb(dev, dev->dbgfs_dir);