Re: [PATCH 10/15] platform/x86/amd/pmf: Add capability to sideload of policy binary
On Fri, 22 Sep 2023, Shyam Sundar S K wrote:
> A policy binary is OS agnostic, and the same policies are expected to work
> across the OSes. At times it becomes difficult to debug when the policies
> inside the policy binaries starts to misbehave. Add a way to sideload such
> policies independently to debug them via a debugfs entry.
>
> Reviewed-by: Mario Limonciello
> Signed-off-by: Shyam Sundar S K
> ---
> drivers/platform/x86/amd/pmf/pmf.h| 1 +
> drivers/platform/x86/amd/pmf/tee-if.c | 60 +++
> 2 files changed, 61 insertions(+)
>
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h
> b/drivers/platform/x86/amd/pmf/pmf.h
> index 61a0f3225b62..780c442239e3 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -215,6 +215,7 @@ struct amd_pmf_dev {
> bool cnqf_supported;
> struct notifier_block pwr_src_notifier;
> /* Smart PC solution builder */
> + struct dentry *esbin;
> unsigned char *policy_buf;
> u32 policy_sz;
> struct tee_context *tee_ctx;
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c
> b/drivers/platform/x86/amd/pmf/tee-if.c
> index 4844782d93c7..fa37cfab2dc7 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -8,6 +8,7 @@
> * Author: Shyam Sundar S K
> */
>
> +#include
> #include
> #include
> #include "pmf.h"
> @@ -21,6 +22,13 @@ module_param(pb_actions_ms, int, 0644);
> MODULE_PARM_DESC(pb_actions_ms, "Policy binary actions sampling frequency
> (default = 1000ms)");
> #endif
>
> +#ifdef CONFIG_AMD_PMF_DEBUG
> +/* Sideload policy binaries to debug policy failures */
> +static bool pb_side_load;
> +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);
>
> @@ -267,6 +275,49 @@ static int amd_pmf_start_policy_engine(struct
> amd_pmf_dev *dev)
> return 0;
> }
>
> +#ifdef CONFIG_AMD_PMF_DEBUG
> +static ssize_t amd_pmf_get_pb_data(struct file *filp, const char __user *buf,
> +size_t length, loff_t *pos)
> +{
> + struct amd_pmf_dev *dev = filp->private_data;
> + int ret;
> +
> + /* policy binary size cannot exceed POLICY_BUF_MAX_SZ */
> + if (length > POLICY_BUF_MAX_SZ || length == 0)
> + return -EINVAL;
> +
> + dev->policy_sz = length;
> + if (copy_from_user(dev->policy_buf, buf, dev->policy_sz))
> + return -EFAULT;
> +
> + ret = amd_pmf_start_policy_engine(dev);
> + if (ret)
> + return -EINVAL;
> +
> + return length;
> +}
> +
> +static const struct file_operations pb_fops = {
> + .write = amd_pmf_get_pb_data,
> + .open = simple_open,
> +};
> +
> +int amd_pmf_open_pb(struct amd_pmf_dev *dev, struct dentry *debugfs_root)
> +{
> + struct dentry *file = NULL;
> +
> + dev->esbin = debugfs_create_dir("pb", debugfs_root);
> + if (IS_ERR(dev->esbin))
> + return -EINVAL;
> +
> + file = debugfs_create_file("update_policy", 0644, dev->esbin, dev,
> _fops);
> + if (!file)
> + return -EINVAL;
> +
> + return 0;
> +}
> +#endif
> +
> static int amd_pmf_get_bios_buffer(struct amd_pmf_dev *dev)
> {
> dev->policy_buf = kzalloc(dev->policy_sz, GFP_KERNEL);
> @@ -279,6 +330,11 @@ static int amd_pmf_get_bios_buffer(struct amd_pmf_dev
> *dev)
>
> memcpy(dev->policy_buf, dev->policy_base, dev->policy_sz);
>
> +#ifdef CONFIG_AMD_PMF_DEBUG
> + if (pb_side_load)
Can't this go into amd_pmf_open_pb() as early return?
> + amd_pmf_open_pb(dev, dev->dbgfs_dir);
If you provide #else and empty amd_pmf_open_pb() above, you don't need to
do ifdefs here.
> +#endif
> +
> return amd_pmf_start_policy_engine(dev);
> }
>
> @@ -381,6 +437,10 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>
> void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev)
> {
> +#ifdef CONFIG_AMD_PMF_DEBUG
> + if (pb_side_load)
> + debugfs_remove_recursive(dev->esbin);
> +#endif
Likewise here, if you add amd_pmf_remove_pb() into the above #ifdef + new
#else block with empty body, you can just call it w/o #ifdefs here.
> kfree(dev->prev_data);
> kfree(dev->policy_buf);
> cancel_delayed_work_sync(>pb_work);
>
--
i.
Re: [PATCH 10/15] platform/x86/amd/pmf: Add capability to sideload of policy binary
Hi Shyam,
kernel test robot noticed the following build warnings:
[auto build test WARNING on hid/for-next]
[also build test WARNING on linus/master v6.6-rc3 next-20230921]
[cannot apply to drm-misc/drm-misc-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url:
https://github.com/intel-lab-lkp/linux/commits/Shyam-Sundar-S-K/platform-x86-amd-pmf-Add-PMF-TEE-interface/20230923-015418
base: https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git for-next
patch link:
https://lore.kernel.org/r/20230922175056.244940-11-Shyam-sundar.S-k%40amd.com
patch subject: [PATCH 10/15] platform/x86/amd/pmf: Add capability to sideload
of policy binary
config: x86_64-allyesconfig
(https://download.01.org/0day-ci/archive/20230925/202309251031.awddkrgs-...@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build):
(https://download.01.org/0day-ci/archive/20230925/202309251031.awddkrgs-...@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Closes:
https://lore.kernel.org/oe-kbuild-all/202309251031.awddkrgs-...@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/platform/x86/amd/pmf/tee-if.c:305:5: warning: no previous prototype
>> for 'amd_pmf_open_pb' [-Wmissing-prototypes]
305 | int amd_pmf_open_pb(struct amd_pmf_dev *dev, struct dentry
*debugfs_root)
| ^~~
vim +/amd_pmf_open_pb +305 drivers/platform/x86/amd/pmf/tee-if.c
304
> 305 int amd_pmf_open_pb(struct amd_pmf_dev *dev, struct dentry
> *debugfs_root)
306 {
307 struct dentry *file = NULL;
308
309 dev->esbin = debugfs_create_dir("pb", debugfs_root);
310 if (IS_ERR(dev->esbin))
311 return -EINVAL;
312
313 file = debugfs_create_file("update_policy", 0644, dev->esbin,
dev, _fops);
314 if (!file)
315 return -EINVAL;
316
317 return 0;
318 }
319 #endif
320
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
[PATCH 10/15] platform/x86/amd/pmf: Add capability to sideload of policy binary
A policy binary is OS agnostic, and the same policies are expected to work
across the OSes. At times it becomes difficult to debug when the policies
inside the policy binaries starts to misbehave. Add a way to sideload such
policies independently to debug them via a debugfs entry.
Reviewed-by: Mario Limonciello
Signed-off-by: Shyam Sundar S K
---
drivers/platform/x86/amd/pmf/pmf.h| 1 +
drivers/platform/x86/amd/pmf/tee-if.c | 60 +++
2 files changed, 61 insertions(+)
diff --git a/drivers/platform/x86/amd/pmf/pmf.h
b/drivers/platform/x86/amd/pmf/pmf.h
index 61a0f3225b62..780c442239e3 100644
--- a/drivers/platform/x86/amd/pmf/pmf.h
+++ b/drivers/platform/x86/amd/pmf/pmf.h
@@ -215,6 +215,7 @@ struct amd_pmf_dev {
bool cnqf_supported;
struct notifier_block pwr_src_notifier;
/* Smart PC solution builder */
+ struct dentry *esbin;
unsigned char *policy_buf;
u32 policy_sz;
struct tee_context *tee_ctx;
diff --git a/drivers/platform/x86/amd/pmf/tee-if.c
b/drivers/platform/x86/amd/pmf/tee-if.c
index 4844782d93c7..fa37cfab2dc7 100644
--- a/drivers/platform/x86/amd/pmf/tee-if.c
+++ b/drivers/platform/x86/amd/pmf/tee-if.c
@@ -8,6 +8,7 @@
* Author: Shyam Sundar S K
*/
+#include
#include
#include
#include "pmf.h"
@@ -21,6 +22,13 @@ module_param(pb_actions_ms, int, 0644);
MODULE_PARM_DESC(pb_actions_ms, "Policy binary actions sampling frequency
(default = 1000ms)");
#endif
+#ifdef CONFIG_AMD_PMF_DEBUG
+/* Sideload policy binaries to debug policy failures */
+static bool pb_side_load;
+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);
@@ -267,6 +275,49 @@ static int amd_pmf_start_policy_engine(struct amd_pmf_dev
*dev)
return 0;
}
+#ifdef CONFIG_AMD_PMF_DEBUG
+static ssize_t amd_pmf_get_pb_data(struct file *filp, const char __user *buf,
+ size_t length, loff_t *pos)
+{
+ struct amd_pmf_dev *dev = filp->private_data;
+ int ret;
+
+ /* policy binary size cannot exceed POLICY_BUF_MAX_SZ */
+ if (length > POLICY_BUF_MAX_SZ || length == 0)
+ return -EINVAL;
+
+ dev->policy_sz = length;
+ if (copy_from_user(dev->policy_buf, buf, dev->policy_sz))
+ return -EFAULT;
+
+ ret = amd_pmf_start_policy_engine(dev);
+ if (ret)
+ return -EINVAL;
+
+ return length;
+}
+
+static const struct file_operations pb_fops = {
+ .write = amd_pmf_get_pb_data,
+ .open = simple_open,
+};
+
+int amd_pmf_open_pb(struct amd_pmf_dev *dev, struct dentry *debugfs_root)
+{
+ struct dentry *file = NULL;
+
+ dev->esbin = debugfs_create_dir("pb", debugfs_root);
+ if (IS_ERR(dev->esbin))
+ return -EINVAL;
+
+ file = debugfs_create_file("update_policy", 0644, dev->esbin, dev,
_fops);
+ if (!file)
+ return -EINVAL;
+
+ return 0;
+}
+#endif
+
static int amd_pmf_get_bios_buffer(struct amd_pmf_dev *dev)
{
dev->policy_buf = kzalloc(dev->policy_sz, GFP_KERNEL);
@@ -279,6 +330,11 @@ static int amd_pmf_get_bios_buffer(struct amd_pmf_dev *dev)
memcpy(dev->policy_buf, dev->policy_base, dev->policy_sz);
+#ifdef CONFIG_AMD_PMF_DEBUG
+ if (pb_side_load)
+ amd_pmf_open_pb(dev, dev->dbgfs_dir);
+#endif
+
return amd_pmf_start_policy_engine(dev);
}
@@ -381,6 +437,10 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev)
{
+#ifdef CONFIG_AMD_PMF_DEBUG
+ if (pb_side_load)
+ debugfs_remove_recursive(dev->esbin);
+#endif
kfree(dev->prev_data);
kfree(dev->policy_buf);
cancel_delayed_work_sync(>pb_work);
--
2.25.1
