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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241126205834.GA2651046@bhelgaas>
Date: Tue, 26 Nov 2024 14:58:34 -0600
From: Bjorn Helgaas <helgaas@...nel.org>
To: Dheeraj Kumar Srivastava <dheerajkumar.srivastava@....com>
Cc: joro@...tes.org, suravee.suthikulpanit@....com, will@...nel.org,
	robin.murphy@....com, linux-kernel@...r.kernel.org,
	iommu@...ts.linux.dev, vasant.hegde@....com
Subject: Re: [PATCH v2 2/8] iommu/amd: Add debugfs support to dump IOMMU MMIO
 registers

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