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

["Srivastava, Dheeraj Kumar" <dheerajkumar.srivastava@amd.com>]

Hi,

On 11/27/2024 2:28 AM, Bjorn Helgaas wrote:
> 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.
> 

Sure.

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

In my opinion,
1. Dumping register/s only of interest is more useful then dumping 
everything we have or defined to be dumped.
2. If we want to dump set of registers (not all in the mmio space), we 
need to maintain list of register's offset we want to dump. In this 
approach, if more registers are required to be dumped in a later point 
of time, then we have to extend the list each time with a separate patch 
whenever we want to do so.
3. With the implemented approach, we can dump any register/s extensively
without maintaining register's offset of user's interest. Also it reduce 
user side of extra code to extract the required register/s from the 
dumped data.
4. Writing offset in different file and getting corresponding registers 
dump in a different file is useful for the same reason.

Debugging issues internally we found that this implemented approach is 
more useful.

Please do let me know what you think.

Thanks
Dheeraj

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