Re: [PATCH v2 2/8] iommu/amd: Add debugfs support to dump IOMMU MMIO registers

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Nov 06, 2024 at 01:16:33PM +0530, Dheeraj Kumar Srivastava wrote:
> Analyzing IOMMU MMIO registers gives a view of what IOMMU is
> configured with on the system and is helpful to debug issues
> with IOMMU.
> 
> eg.
> 1. To get mmio registers value for iommu<x>

s/mmio/MMIO/ to match usage above.

>    # echo "0x18" > /sys/kernel/debug/iommu/amd/iommu00/mmio
>    # cat /sys/kernel/debug/iommu/amd/iommu00/mmio_dump
> 
> Signed-off-by: Dheeraj Kumar Srivastava <dheerajkumar.srivastava@amd.com>
> ---
>  drivers/iommu/amd/debugfs.c | 58 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
> 
> diff --git a/drivers/iommu/amd/debugfs.c b/drivers/iommu/amd/debugfs.c
> index ff9520e002be..e56c050eb7c8 100644
> --- a/drivers/iommu/amd/debugfs.c
> +++ b/drivers/iommu/amd/debugfs.c
> @@ -15,6 +15,59 @@
>  static struct dentry *amd_iommu_debugfs;
>  
>  #define	MAX_NAME_LEN	20
> +#define	OFS_IN_SZ	8
> +
> +static int mmio_offset = -1;
> +
> +static ssize_t iommu_mmio_write(struct file *filp, const char __user *ubuf,
> +				size_t cnt, loff_t *ppos)
> +{
> +	struct seq_file *m = filp->private_data;
> +	struct amd_iommu *iommu = m->private;
> +	int ret;
> +
> +	if (cnt > OFS_IN_SZ)
> +		return -EINVAL;
> +
> +	ret = kstrtou32_from_user(ubuf, cnt, 0, &mmio_offset);
> +	if (ret)
> +		return ret;
> +
> +	if (mmio_offset > iommu->mmio_phys_end - 4) {
> +		mmio_offset = -1;
> +		return  -EINVAL;
> +	}
> +
> +	return cnt;
> +}
> +
> +static int iommu_mmio_show(struct seq_file *m, void *unused)
> +{
> +	if (mmio_offset >= 0)
> +		seq_printf(m, "0x%x\n", mmio_offset);
> +	else
> +		seq_puts(m, "No or invalid input provided\n");
> +
> +	return 0;
> +}
> +DEFINE_SHOW_STORE_ATTRIBUTE(iommu_mmio);
> +
> +static int iommu_mmio_dump_show(struct seq_file *m, void *unused)
> +{
> +	struct amd_iommu *iommu = m->private;
> +	u32 value;
> +
> +	if (mmio_offset < 0) {
> +		seq_puts(m, "Please provide mmio register's offset\n");
> +		return 0;
> +	}
> +
> +	value = readl(iommu->mmio_base + mmio_offset);
> +	seq_printf(m, "0x%08x\n", value);
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(iommu_mmio_dump);

I think this would be much simpler and more user-friendly if you made
this a single read-only file to dump all the registers, as
/sys/kernel/debug/iommu/intel/iommu_regset does.  Having to write
"mmio" and then read "mmio_dump" makes it hard to use, and it means
that two users can race with each other and confuse things.

This is part of the AMD IOMMU driver, which should know what registers
are of interest and how to dump and possibly even decode them.

>  void amd_iommu_debugfs_setup(void)
>  {
> @@ -26,5 +79,10 @@ void amd_iommu_debugfs_setup(void)
>  	for_each_iommu(iommu) {
>  		snprintf(name, MAX_NAME_LEN, "iommu%02d", iommu->index);
>  		iommu->debugfs = debugfs_create_dir(name, amd_iommu_debugfs);
> +
> +		debugfs_create_file("mmio", 0644, iommu->debugfs, iommu,
> +				    &iommu_mmio_fops);
> +		debugfs_create_file("mmio_dump", 0444, iommu->debugfs, iommu,
> +				    &iommu_mmio_dump_fops);
>  	}
>  }
> -- 
> 2.25.1
>