Re: [PATCH 10/15] platform/x86/amd/pmf: Add capability to sideload of policy binary

From: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
To: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
Cc: hdegoede@redhat.com, markgross@kernel.org,
	basavaraj.natikar@amd.com, jikos@kernel.org,
	benjamin.tissoires@redhat.com, alexander.deucher@amd.com,
	christian.koenig@amd.com, Xinhui.Pan@amd.com, airlied@gmail.com,
	daniel@ffwll.ch, Patil.Reddy@amd.com, mario.limonciello@amd.com,
	platform-driver-x86@vger.kernel.org, linux-input@vger.kernel.org,
	amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 10/15] platform/x86/amd/pmf: Add capability to sideload of policy binary
Date: Wed, 27 Sep 2023 15:33:19 +0300 (EEST)	[thread overview]
Message-ID: <1021e446-d522-4b38-c588-50597a20cb3e@linux.intel.com> (raw)
In-Reply-To: <20230922175056.244940-11-Shyam-sundar.S-k@amd.com>

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 <mario.limonciello@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>  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 <Shyam-sundar.S-k@amd.com>
>   */
>  
> +#include <linux/debugfs.h>
>  #include <linux/tee_drv.h>
>  #include <linux/uuid.h>
>  #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, &pb_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(&dev->pb_work);
> 

-- 
 i.