[PATCH v2 0/8] Introduce debugfs support in IOMMU

[PATCH v2 0/8] Introduce debugfs support in IOMMU

[Dheeraj Kumar Srivastava]

Introducing debugfs support in AMD/IOMMU driver that will allow
userspace to dump below IOMMU information
1) MMIO and Capability register per IOMMU
2) Command buffer
3) Device table entry
4) Interrupt remapping table entry

Analyzing contents of IOMMU data structures helps in understanding IOMMU
capabilities and behavior and debug issues faster.

1. MMIO and Capability registers - Add support to dump MMIO and Capability
   registers per IOMMU.

   Example:
   a. Write MMIO register offset to dump it
      $ echo 0x18 > /sys/kernel/debug/iommu/amd/iommu00/mmio
      $ cat /sys/kernel/debug/iommu/amd/iommu00/mmio_dump

      Output:
      $ 0x0003f48d

    b. Write capability register offset to dump it
       $ echo 0x10 > /sys/kernel/debug/iommu/amd/iommu00/capability
       $ cat /sys/kernel/debug/iommu/amd/iommu00/capability_dump

       Output:
       $ 0x00203040

2. Command buffer - Add support to dump per IOMMU command buffer.

   Example:
   a. cat /sys/kernel/debug/iommu/amd/iommu00/cmdbuf

   Output:
   CMD Buffer Head Offset:339 Tail Offset:339
     0: 00835001100000010000990000000000
     1: 0000000030000005fffff0037fffffff
     2: 00835001100000010000990100000000
     3: 0000000030000005fffff0037fffffff
     4: 00835001100000010000990200000000
   ...................................
   ...................................
   ...................................

3. Device table - Add support to dump device table per IOMMU.

   Example:
   a. Write device id to dump device table entry for that device
      $ echo 0000:01:00.0 > /sys/kernel/debug/iommu/amd/devid
      $ cat /sys/kernel/debug/iommu/amd/devid

      Output:
      0000:01:00.0

      Dump the device table entry for the input given
      $ cat /sys/kernel/debug/iommu/amd/devtbl

      Output:
      DeviceId             QWORD[3]         QWORD[2]         QWORD[1]         QWORD[0] iommu
      0000:01:00.0 0000000000000000 20000001373b8013 0000000000000038 6000000114d7b603 iommu3

    b. Write device id to dump device table entry for that device
      $ echo 01:00.0 > /sys/kernel/debug/iommu/amd/devid
      $ cat /sys/kernel/debug/iommu/amd/devid

      Output:
      0000:01:00.0

      Dump the device table entry for the input given
      $ cat /sys/kernel/debug/iommu/amd/devtbl

      Output:
      DeviceId             QWORD[3]         QWORD[2]         QWORD[1]         QWORD[0] iommu
      0000:01:00.0 0000000000000000 20000001373b8013 0000000000000038 6000000114d7b603 iommu3

4. Interrupt remapping table - Add support to dump IRT table valid entries in
   "iommu_irqtbl" file. This supports user input to dump IRT entry for a
   specific pci device.

   Example:
   a. Write device id to dump device table entry for that device
      $ echo 0000:01:00.0 > /sys/kernel/debug/iommu/amd/devid
      $ cat /sys/kernel/debug/iommu/amd/devid

      Output:
      0000:01:00.0

      Dump the device table entry for the input given
      $ cat /sys/kernel/debug/iommu/amd/irtbl

      Output:
      DeviceId 0000:01:00.0
      IRT[0000] 00000000000000200000000000000241
      IRT[0001] 00000000000000200000000000000841
      IRT[0002] 00000000000000200000000000002041
      IRT[0003] 00000000000000200000000000008041
      IRT[0004] 00000000000000200000000000020041
      ..........................................
      ..........................................
      ..........................................

   b. Write device id to dump device table entry for that device
      $ echo 01:00.0 > /sys/kernel/debug/iommu/amd/devid
      $ cat /sys/kernel/debug/iommu/amd/devid

      Output:
      0000:01:00.0

      Dump the device table entry for the input given
      $ cat /sys/kernel/debug/iommu/amd/irttbl

      Output:
      Device 0000:01:00.0
      IRT[0000] 00000000000000200000000000000241
      IRT[0001] 00000000000000200000000000000841
      IRT[0002] 00000000000000200000000000002041
      IRT[0003] 00000000000000200000000000008041
      IRT[0004] 00000000000000200000000000020041
      ..........................................
      ..........................................
      ..........................................

Changes since v1:
-> Patch 2/8 and 3/8: Use kstrtou32_from_user() instead of memdup_user_nul() --> kstrtou32()
-> Patch 4/8: Dump command buffer head and tail offset instead of head and tail pointer registers.
-> Patch 8/8: Fix bot reported warning on v1 upstream.

Dheeraj Kumar Srivastava (8):
  iommu/amd: Refactor AMD IOMMU debugfs initial setup
  iommu/amd: Add debugfs support to dump IOMMU MMIO registers
  iommu/amd: Add debugfs support to dump IOMMU Capability registers
  iommu/amd: Add debugfs support to dump IOMMU command buffer
  iommu/amd: Add support for device id user input
  iommu/amd: Add debugfs support to dump device table
  iommu/amd: Add debugfs support to dump IRT Table
  iommu/amd: Add documentation for AMD IOMMU debugfs support

 Documentation/ABI/testing/debugfs-amd-iommu | 140 ++++++++
 drivers/iommu/amd/amd_iommu.h               |   4 +-
 drivers/iommu/amd/amd_iommu_types.h         |   7 +
 drivers/iommu/amd/debugfs.c                 | 376 +++++++++++++++++++-
 drivers/iommu/amd/init.c                    |   5 +-
 drivers/iommu/amd/iommu.c                   |   7 -
 6 files changed, 518 insertions(+), 21 deletions(-)
 create mode 100644 Documentation/ABI/testing/debugfs-amd-iommu

-- 
2.25.1

[PATCH v2 1/8] iommu/amd: Refactor AMD IOMMU debugfs initial setup

[Dheeraj Kumar Srivastava]

Rearrange initial setup of AMD IOMMU debugfs to segregate per IOMMU
setup and setup which is common for all IOMMUs. This ensures that common
debugfs paths (introduced in subsequent patches) are created only once
instead of being created for each IOMMU.

With the change, there is no need to use lock as amd_iommu_debugfs_setup()
will be called only once during AMD IOMMU initialization. So remove lock
acquisition in amd_iommu_debugfs_setup().

Signed-off-by: Dheeraj Kumar Srivastava <dheerajkumar.srivastava@amd.com>
Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 drivers/iommu/amd/amd_iommu.h |  4 ++--
 drivers/iommu/amd/debugfs.c   | 16 +++++++---------
 drivers/iommu/amd/init.c      |  5 ++---
 3 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 6386fa4556d9..68821b62730c 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -29,9 +29,9 @@ void *__init iommu_alloc_4k_pages(struct amd_iommu *iommu,
 				  gfp_t gfp, size_t size);
 
 #ifdef CONFIG_AMD_IOMMU_DEBUGFS
-void amd_iommu_debugfs_setup(struct amd_iommu *iommu);
+void amd_iommu_debugfs_setup(void);
 #else
-static inline void amd_iommu_debugfs_setup(struct amd_iommu *iommu) {}
+static inline void amd_iommu_debugfs_setup(void) {}
 #endif
 
 /* Needed for interrupt remapping */
diff --git a/drivers/iommu/amd/debugfs.c b/drivers/iommu/amd/debugfs.c
index 545372fcc72f..ff9520e002be 100644
--- a/drivers/iommu/amd/debugfs.c
+++ b/drivers/iommu/amd/debugfs.c
@@ -13,20 +13,18 @@
 #include "amd_iommu.h"
 
 static struct dentry *amd_iommu_debugfs;
-static DEFINE_MUTEX(amd_iommu_debugfs_lock);
 
 #define	MAX_NAME_LEN	20
 
-void amd_iommu_debugfs_setup(struct amd_iommu *iommu)
+void amd_iommu_debugfs_setup(void)
 {
+	struct amd_iommu *iommu;
 	char name[MAX_NAME_LEN + 1];
 
-	mutex_lock(&amd_iommu_debugfs_lock);
-	if (!amd_iommu_debugfs)
-		amd_iommu_debugfs = debugfs_create_dir("amd",
-						       iommu_debugfs_dir);
-	mutex_unlock(&amd_iommu_debugfs_lock);
+	amd_iommu_debugfs = debugfs_create_dir("amd", iommu_debugfs_dir);
 
-	snprintf(name, MAX_NAME_LEN, "iommu%02d", iommu->index);
-	iommu->debugfs = debugfs_create_dir(name, amd_iommu_debugfs);
+	for_each_iommu(iommu) {
+		snprintf(name, MAX_NAME_LEN, "iommu%02d", iommu->index);
+		iommu->debugfs = debugfs_create_dir(name, amd_iommu_debugfs);
+	}
 }
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 43131c3a2172..d78dc96bbec3 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -3377,7 +3377,6 @@ int amd_iommu_enable_faulting(unsigned int cpu)
  */
 static int __init amd_iommu_init(void)
 {
-	struct amd_iommu *iommu;
 	int ret;
 
 	ret = iommu_go_to_state(IOMMU_INITIALIZED);
@@ -3391,8 +3390,8 @@ static int __init amd_iommu_init(void)
 	}
 #endif
 
-	for_each_iommu(iommu)
-		amd_iommu_debugfs_setup(iommu);
+	if (!ret)
+		amd_iommu_debugfs_setup();
 
 	return ret;
 }
-- 
2.25.1

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

[Dheeraj Kumar Srivastava]

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>
   # 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);
 
 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

[PATCH v2 3/8] iommu/amd: Add debugfs support to dump IOMMU Capability registers

[Dheeraj Kumar Srivastava]

IOMMU Capability registers defines capabilities of IOMMU and information
needed for initialising MMIO registers and device table. This is useful
to dump these registers for debugging IOMMU related issues.

e.g.To get capability registers value for iommu<x>
  # echo "0x10" > /sys/kernel/debug/iommu/amd/iommu00/capability
  # cat /sys/kernel/debug/iommu/amd/iommu00/capability_dump

Signed-off-by: Dheeraj Kumar Srivastava <dheerajkumar.srivastava@amd.com>
---
 drivers/iommu/amd/debugfs.c | 60 +++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/drivers/iommu/amd/debugfs.c b/drivers/iommu/amd/debugfs.c
index e56c050eb7c8..25a4e738d067 100644
--- a/drivers/iommu/amd/debugfs.c
+++ b/drivers/iommu/amd/debugfs.c
@@ -18,6 +18,7 @@ static struct dentry *amd_iommu_debugfs;
 #define	OFS_IN_SZ	8
 
 static int mmio_offset = -1;
+static int cap_offset = -1;
 
 static ssize_t iommu_mmio_write(struct file *filp, const char __user *ubuf,
 				size_t cnt, loff_t *ppos)
@@ -69,6 +70,61 @@ static int iommu_mmio_dump_show(struct seq_file *m, void *unused)
 }
 DEFINE_SHOW_ATTRIBUTE(iommu_mmio_dump);
 
+static ssize_t iommu_capability_write(struct file *filp, const char __user *ubuf,
+				      size_t cnt, loff_t *ppos)
+{
+	int ret;
+
+	if (cnt > OFS_IN_SZ)
+		return -EINVAL;
+
+	ret = kstrtou32_from_user(ubuf, cnt, 0, &cap_offset);
+	if (ret)
+		return ret;
+
+	/* Capability register at offset 0x14 is the last IOMMU capability register. */
+	if (cap_offset > 0x14) {
+		cap_offset = -1;
+		return -EINVAL;
+	}
+
+	return cnt;
+}
+
+static int iommu_capability_show(struct seq_file *m, void *unused)
+{
+	if (cap_offset >= 0)
+		seq_printf(m, "0x%x\n", cap_offset);
+	else
+		seq_puts(m, "No or invalid input provided\n");
+
+	return 0;
+}
+DEFINE_SHOW_STORE_ATTRIBUTE(iommu_capability);
+
+static int iommu_capability_dump_show(struct seq_file *m, void *unused)
+{
+	struct amd_iommu *iommu = m->private;
+	u32 value;
+	int err;
+
+	if (cap_offset < 0) {
+		seq_puts(m, "Please provide capability register's offset\n");
+		return 0;
+	}
+
+	err = pci_read_config_dword(iommu->dev, iommu->cap_ptr + cap_offset, &value);
+	if (err) {
+		seq_printf(m, "Not able to read capability register at 0x%x\n", cap_offset);
+		return 0;
+	}
+
+	seq_printf(m, "0x%08x\n", value);
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(iommu_capability_dump);
+
 void amd_iommu_debugfs_setup(void)
 {
 	struct amd_iommu *iommu;
@@ -84,5 +140,9 @@ void amd_iommu_debugfs_setup(void)
 				    &iommu_mmio_fops);
 		debugfs_create_file("mmio_dump", 0444, iommu->debugfs, iommu,
 				    &iommu_mmio_dump_fops);
+		debugfs_create_file("capability", 0644, iommu->debugfs, iommu,
+				    &iommu_capability_fops);
+		debugfs_create_file("capability_dump", 0444, iommu->debugfs,
+				    iommu, &iommu_capability_dump_fops);
 	}
 }
-- 
2.25.1

[PATCH v2 4/8] iommu/amd: Add debugfs support to dump IOMMU command buffer

[Dheeraj Kumar Srivastava]

IOMMU driver sends command to IOMMU hardware via command buffer. In cases
where IOMMU hardware fails to process commands in command buffer, dumping
it is a valuable input to debug the issue.

IOMMU hardware processes command buffer entry at offset equals to the head
pointer. Dumping just the entry at the head pointer may not always be
useful. The current head may not be pointing to the entry of the command
buffer which is causing the issue. IOMMU Hardware may have processed the
entry and updated the head pointer. So dumping the entire command buffer
gives a broad understanding of what hardware was/is doing. The command
buffer dump will have all entries from start to end of the command buffer.
Along with that, it will have a head and tail command buffer pointer
register dump to facilitate where the IOMMU driver and hardware are in
the command buffer for injecting and processing the entries respectively.

Command buffer is a per IOMMU data structure. So dumping on per IOMMU
basis.
eg. To get command buffer dump for iommu<x>
    #cat /sys/kernel/debug/iommu/amd/iommu<x>/cmdbuf

Signed-off-by: Dheeraj Kumar Srivastava <dheerajkumar.srivastava@amd.com>
---
 drivers/iommu/amd/amd_iommu_types.h |  7 +++++++
 drivers/iommu/amd/debugfs.c         | 26 ++++++++++++++++++++++++++
 drivers/iommu/amd/iommu.c           |  7 -------
 3 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 601fb4ee6900..876fa671ef91 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -878,6 +878,13 @@ extern struct list_head amd_iommu_list;
  */
 extern struct amd_iommu *amd_iommus[MAX_IOMMUS];
 
+/*
+ * Structure defining one entry in the command buffer
+ */
+struct iommu_cmd {
+	u32 data[4];
+};
+
 /*
  * Structure defining one entry in the device table
  */
diff --git a/drivers/iommu/amd/debugfs.c b/drivers/iommu/amd/debugfs.c
index 25a4e738d067..1b989d45257f 100644
--- a/drivers/iommu/amd/debugfs.c
+++ b/drivers/iommu/amd/debugfs.c
@@ -125,6 +125,30 @@ static int iommu_capability_dump_show(struct seq_file *m, void *unused)
 }
 DEFINE_SHOW_ATTRIBUTE(iommu_capability_dump);
 
+static int iommu_cmdbuf_show(struct seq_file *m, void *unused)
+{
+	struct amd_iommu *iommu = m->private;
+	struct iommu_cmd *cmd;
+	unsigned long flag;
+	u32 head, tail;
+	int i;
+
+	raw_spin_lock_irqsave(&iommu->lock, flag);
+	head = readl(iommu->mmio_base + MMIO_CMD_HEAD_OFFSET);
+	tail = readl(iommu->mmio_base + MMIO_CMD_TAIL_OFFSET);
+	seq_printf(m, "CMD Buffer Head Offset:%d Tail Offset:%d\n",
+		   (head >> 4) & 0x7fff, (tail >> 4) & 0x7fff);
+	for (i = 0; i < CMD_BUFFER_ENTRIES; i++) {
+		cmd = (struct iommu_cmd *)(iommu->cmd_buf + i * sizeof(*cmd));
+		seq_printf(m, "%3d: %08x%08x%08x%08x\n", i, cmd->data[0],
+			   cmd->data[1], cmd->data[2], cmd->data[3]);
+	}
+	raw_spin_unlock_irqrestore(&iommu->lock, flag);
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(iommu_cmdbuf);
+
 void amd_iommu_debugfs_setup(void)
 {
 	struct amd_iommu *iommu;
@@ -144,5 +168,7 @@ void amd_iommu_debugfs_setup(void)
 				    &iommu_capability_fops);
 		debugfs_create_file("capability_dump", 0444, iommu->debugfs,
 				    iommu, &iommu_capability_dump_fops);
+		debugfs_create_file("cmdbuf", 0444, iommu->debugfs, iommu,
+				    &iommu_cmdbuf_fops);
 	}
 }
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 8364cd6fa47d..efc2d1ddd7cc 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -63,13 +63,6 @@ static const struct iommu_dirty_ops amd_dirty_ops;
 
 int amd_iommu_max_glx_val = -1;
 
-/*
- * general struct to manage commands send to an IOMMU
- */
-struct iommu_cmd {
-	u32 data[4];
-};
-
 struct kmem_cache *amd_iommu_irq_cache;
 
 static void detach_device(struct device *dev);
-- 
2.25.1

[PATCH v2 5/8] iommu/amd: Add support for device id user input

[Dheeraj Kumar Srivastava]

Device id user input is needed for dumping IOMMU data structures like
device table, IRT etc in AMD IOMMU debugfs.

eg.
1. # echo 0000:01:00.0 > /sys/kernel/debug/iommu/amd/devid
   # cat /sys/kernel/debug/iommu/amd/devid
   Output : 0000:01:00.0

2. # echo 01:00.0 > /sys/kernel/debug/iommu/amd/devid
   # cat /sys/kernel/debug/iommu/amd/devid
   Output : 0000:01:00.0

Signed-off-by: Dheeraj Kumar Srivastava <dheerajkumar.srivastava@amd.com>
---
 drivers/iommu/amd/debugfs.c | 78 +++++++++++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/drivers/iommu/amd/debugfs.c b/drivers/iommu/amd/debugfs.c
index 1b989d45257f..74c88a80c6d0 100644
--- a/drivers/iommu/amd/debugfs.c
+++ b/drivers/iommu/amd/debugfs.c
@@ -16,9 +16,11 @@ static struct dentry *amd_iommu_debugfs;
 
 #define	MAX_NAME_LEN	20
 #define	OFS_IN_SZ	8
+#define	DEVID_IN_SZ	16
 
 static int mmio_offset = -1;
 static int cap_offset = -1;
+static int sbdf = -1;
 
 static ssize_t iommu_mmio_write(struct file *filp, const char __user *ubuf,
 				size_t cnt, loff_t *ppos)
@@ -149,6 +151,80 @@ static int iommu_cmdbuf_show(struct seq_file *m, void *unused)
 }
 DEFINE_SHOW_ATTRIBUTE(iommu_cmdbuf);
 
+static ssize_t devid_write(struct file *filp, const char __user *ubuf,
+			   size_t cnt, loff_t *ppos)
+{
+	struct amd_iommu_pci_seg *pci_seg;
+	int seg, bus, slot, func;
+	struct amd_iommu *iommu;
+	char *srcid_ptr;
+	u16 devid;
+	int i;
+
+	sbdf = -1;
+
+	if (cnt >= DEVID_IN_SZ)
+		return -EINVAL;
+
+	srcid_ptr = memdup_user_nul(ubuf, cnt);
+	if (IS_ERR(srcid_ptr))
+		return PTR_ERR(srcid_ptr);
+
+	i = sscanf(srcid_ptr, "%x:%x:%x.%x", &seg, &bus, &slot, &func);
+	if (i != 4) {
+		i = sscanf(srcid_ptr, "%x:%x.%x", &bus, &slot, &func);
+		if (i != 3) {
+			kfree(srcid_ptr);
+			return -EINVAL;
+		}
+		seg = 0;
+	}
+
+	devid = PCI_DEVID(bus, PCI_DEVFN(slot, func));
+
+	/* Check if user device id input is a valid input */
+	for_each_pci_segment(pci_seg) {
+		if (pci_seg->id != seg)
+			continue;
+		if (devid > pci_seg->last_bdf) {
+			kfree(srcid_ptr);
+			return -EINVAL;
+		}
+		iommu = pci_seg->rlookup_table[devid];
+		if (!iommu) {
+			kfree(srcid_ptr);
+			return -ENODEV;
+		}
+		break;
+	}
+
+	if (pci_seg->id != seg) {
+		kfree(srcid_ptr);
+		return -EINVAL;
+	}
+
+	sbdf = PCI_SEG_DEVID_TO_SBDF(seg, devid);
+
+	kfree(srcid_ptr);
+
+	return cnt;
+}
+
+static int devid_show(struct seq_file *m, void *unused)
+{
+	u16 devid;
+
+	if (sbdf >= 0) {
+		devid = PCI_SBDF_TO_DEVID(sbdf);
+		seq_printf(m, "%04x:%02x:%02x:%x\n", PCI_SBDF_TO_SEGID(sbdf),
+			   PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid));
+	} else
+		seq_puts(m, "No or Invalid input provided\n");
+
+	return 0;
+}
+DEFINE_SHOW_STORE_ATTRIBUTE(devid);
+
 void amd_iommu_debugfs_setup(void)
 {
 	struct amd_iommu *iommu;
@@ -171,4 +247,6 @@ void amd_iommu_debugfs_setup(void)
 		debugfs_create_file("cmdbuf", 0444, iommu->debugfs, iommu,
 				    &iommu_cmdbuf_fops);
 	}
+	debugfs_create_file("devid", 0644, amd_iommu_debugfs, NULL,
+			    &devid_fops);
 }
-- 
2.25.1

[PATCH v2 6/8] iommu/amd: Add debugfs support to dump device table

[Dheeraj Kumar Srivastava]

IOMMU uses device table data structure to get per-device information for
DMA remapping, interrupt remapping, and other functionalities. It's a
valuable data structure to visualize for debugging issues related to
IOMMU.

eg.
To dump device table entry for a particular device
   #echo 0000:c4:00.0 > /sys/kernel/debug/iommu/amd/devid
   #cat /sys/kernel/debug/iommu/amd/devtbl

   or
   #echo c4:00.0 > /sys/kernel/debug/iommu/amd/devid
   #cat /sys/kernel/debug/iommu/amd/devtbl

Signed-off-by: Dheeraj Kumar Srivastava <dheerajkumar.srivastava@amd.com>
---
 drivers/iommu/amd/debugfs.c | 49 +++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/drivers/iommu/amd/debugfs.c b/drivers/iommu/amd/debugfs.c
index 74c88a80c6d0..bba65848ef60 100644
--- a/drivers/iommu/amd/debugfs.c
+++ b/drivers/iommu/amd/debugfs.c
@@ -225,6 +225,53 @@ static int devid_show(struct seq_file *m, void *unused)
 }
 DEFINE_SHOW_STORE_ATTRIBUTE(devid);
 
+static void dump_dte(struct seq_file *m, struct amd_iommu_pci_seg *pci_seg, u16 devid)
+{
+	struct dev_table_entry *dev_table;
+	struct amd_iommu *iommu;
+
+	iommu = pci_seg->rlookup_table[devid];
+	if (!iommu)
+		return;
+
+	dev_table = get_dev_table(iommu);
+	if (!dev_table) {
+		seq_puts(m, "Device table not found");
+		return;
+	}
+
+	seq_printf(m, "%-12s %16s %16s %16s %16s iommu\n", "DeviceId",
+		   "QWORD[3]", "QWORD[2]", "QWORD[1]", "QWORD[0]");
+	seq_printf(m, "%04x:%02x:%02x:%x ", pci_seg->id, PCI_BUS_NUM(devid),
+		   PCI_SLOT(devid), PCI_FUNC(devid));
+	for (int i = 3; i >= 0; --i)
+		seq_printf(m, "%016llx ", dev_table[devid].data[i]);
+	seq_printf(m, "iommu%d\n", iommu->index);
+}
+
+static int iommu_devtbl_show(struct seq_file *m, void *unused)
+{
+	struct amd_iommu_pci_seg *pci_seg;
+	u16 seg, devid;
+
+	if (sbdf < 0) {
+		seq_puts(m, "Please provide valid device id input\n");
+		return 0;
+	}
+	seg = PCI_SBDF_TO_SEGID(sbdf);
+	devid = PCI_SBDF_TO_DEVID(sbdf);
+
+	for_each_pci_segment(pci_seg) {
+		if (pci_seg->id != seg)
+			continue;
+		dump_dte(m, pci_seg, devid);
+		break;
+	}
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(iommu_devtbl);
+
 void amd_iommu_debugfs_setup(void)
 {
 	struct amd_iommu *iommu;
@@ -249,4 +296,6 @@ void amd_iommu_debugfs_setup(void)
 	}
 	debugfs_create_file("devid", 0644, amd_iommu_debugfs, NULL,
 			    &devid_fops);
+	debugfs_create_file("devtbl", 0444, amd_iommu_debugfs, NULL,
+			    &iommu_devtbl_fops);
 }
-- 
2.25.1

[PATCH v2 7/8] iommu/amd: Add debugfs support to dump IRT Table

[Dheeraj Kumar Srivastava]

In cases where we have an issue in the device interrupt path with IOMMU
interrupt remapping enabled, dumping valid IRT table entries for the device
is very useful and good input for debugging the issue.

eg.
To dump irte entries for a particular device
   #echo "c4:00.0" > /sys/kernel/debug/iommu/amd/devid
   #cat /sys/kernel/debug/iommu/amd/irqtbl | less

   or

   #echo "0000:c4:00.0" > /sys/kernel/debug/iommu/amd/devid
   #cat /sys/kernel/debug/iommu/amd/irqtbl | less

Signed-off-by: Dheeraj Kumar Srivastava <dheerajkumar.srivastava@amd.com>
---
 drivers/iommu/amd/debugfs.c | 89 +++++++++++++++++++++++++++++++++++++
 1 file changed, 89 insertions(+)

diff --git a/drivers/iommu/amd/debugfs.c b/drivers/iommu/amd/debugfs.c
index bba65848ef60..3da3385a7d5d 100644
--- a/drivers/iommu/amd/debugfs.c
+++ b/drivers/iommu/amd/debugfs.c
@@ -11,6 +11,7 @@
 #include <linux/pci.h>
 
 #include "amd_iommu.h"
+#include "../irq_remapping.h"
 
 static struct dentry *amd_iommu_debugfs;
 
@@ -272,6 +273,92 @@ static int iommu_devtbl_show(struct seq_file *m, void *unused)
 }
 DEFINE_SHOW_ATTRIBUTE(iommu_devtbl);
 
+static void dump_128_irte(struct seq_file *m, struct irq_remap_table *table)
+{
+	struct irte_ga *ptr, *irte;
+	int index;
+
+	for (index = 0; index < MAX_IRQS_PER_TABLE; index++) {
+		ptr = (struct irte_ga *)table->table;
+		irte = &ptr[index];
+
+		if (AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) &&
+		    !irte->lo.fields_vapic.valid)
+			continue;
+		else if (!irte->lo.fields_remap.valid)
+			continue;
+		seq_printf(m, "IRT[%04d] %016llx%016llx\n", index, irte->hi.val, irte->lo.val);
+	}
+}
+
+static void dump_32_irte(struct seq_file *m, struct irq_remap_table *table)
+{
+	union irte *ptr, *irte;
+	int index;
+
+	for (index = 0; index < MAX_IRQS_PER_TABLE; index++) {
+		ptr = (union irte *)table->table;
+		irte = &ptr[index];
+
+		if (!irte->fields.valid)
+			continue;
+		seq_printf(m, "IRT[%04d] %08x\n", index, irte->val);
+	}
+}
+
+static void dump_irte(struct seq_file *m, u16 devid, struct amd_iommu_pci_seg *pci_seg)
+{
+	struct irq_remap_table *table;
+	unsigned long flags;
+
+	table = pci_seg->irq_lookup_table[devid];
+	if (!table) {
+		seq_printf(m, "IRQ lookup table not set for %04x:%02x:%02x:%x\n",
+			   pci_seg->id, PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid));
+		return;
+	}
+
+	seq_printf(m, "DeviceId %04x:%02x:%02x:%x\n", pci_seg->id, PCI_BUS_NUM(devid),
+		   PCI_SLOT(devid), PCI_FUNC(devid));
+
+	raw_spin_lock_irqsave(&table->lock, flags);
+	if (AMD_IOMMU_GUEST_IR_GA(amd_iommu_guest_ir))
+		dump_128_irte(m, table);
+	else
+		dump_32_irte(m, table);
+	seq_puts(m, "\n");
+	raw_spin_unlock_irqrestore(&table->lock, flags);
+}
+
+static int iommu_irqtbl_show(struct seq_file *m, void *unused)
+{
+	struct amd_iommu_pci_seg *pci_seg;
+	u16 devid, seg;
+
+	if (!irq_remapping_enabled) {
+		seq_puts(m, "Interrupt remapping is disabled\n");
+		return 0;
+	}
+
+	if (sbdf < 0) {
+		seq_puts(m, "Please provide valid device id input\n");
+		return 0;
+	}
+
+	seg = PCI_SBDF_TO_SEGID(sbdf);
+	devid = PCI_SBDF_TO_DEVID(sbdf);
+
+	for_each_pci_segment(pci_seg) {
+		if (pci_seg->id != seg)
+			continue;
+		dump_irte(m, devid, pci_seg);
+		break;
+	}
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(iommu_irqtbl);
+
 void amd_iommu_debugfs_setup(void)
 {
 	struct amd_iommu *iommu;
@@ -298,4 +385,6 @@ void amd_iommu_debugfs_setup(void)
 			    &devid_fops);
 	debugfs_create_file("devtbl", 0444, amd_iommu_debugfs, NULL,
 			    &iommu_devtbl_fops);
+	debugfs_create_file("irqtbl", 0444, amd_iommu_debugfs, NULL,
+			    &iommu_irqtbl_fops);
 }
-- 
2.25.1

[PATCH v2 8/8] iommu/amd: Add documentation for AMD IOMMU debugfs support

[Dheeraj Kumar Srivastava]

Add documentation describing how to use AMD IOMMU debugfs support to
dump IOMMU data structures - IRT table, Device table, Registers (MMIO and
Capability) and command buffer.

Signed-off-by: Dheeraj Kumar Srivastava <dheerajkumar.srivastava@amd.com>
---
 Documentation/ABI/testing/debugfs-amd-iommu | 140 ++++++++++++++++++++
 1 file changed, 140 insertions(+)
 create mode 100644 Documentation/ABI/testing/debugfs-amd-iommu

diff --git a/Documentation/ABI/testing/debugfs-amd-iommu b/Documentation/ABI/testing/debugfs-amd-iommu
new file mode 100644
index 000000000000..9f7741e05217
--- /dev/null
+++ b/Documentation/ABI/testing/debugfs-amd-iommu
@@ -0,0 +1,140 @@
+What:		/sys/kernel/debug/iommu/amd/iommu<x>/mmio
+Date:		September 2024
+Contact:	Dheeraj Kumar Srivastava <dheerajkumar.srivastava@amd.com>
+Description:
+		This is an input read/write access file. In this file, the user input
+		mmio register offset for iommu<x> to print corresponding mmio register
+		of iommu<x>.
+
+		Example:
+		$ echo "0x18" > /sys/kernel/debug/iommu/amd/iommu00/mmio
+		$ cat /sys/kernel/debug/iommu/amd/iommu00/mmio
+
+		Output:
+		0x18
+
+What:		/sys/kernel/debug/iommu/amd/iommu<x>/mmio_dump
+Date:		September 2024
+Contact:	Dheeraj Kumar Srivastava <dheerajkumar.srivastava@amd.com>
+Description:
+		This is an output read only file that contains mmio register
+		value at offset "/sys/kernel/debug/iommu/amd/iommu<x>/mmio".
+
+		Example:
+		$ cat /sys/kernel/debug/iommu/amd/iommu00/mmio_dump
+
+		Output:
+		0x0003f48d
+
+What:		/sys/kernel/debug/iommu/amd/iommu<x>/capability
+Date:		September 2024
+Contact:	Dheeraj Kumar Srivastava <dheerajkumar.srivastava@amd.com>
+Description:
+		This is an input read/write access file. In this file, the user input
+                capability register offset for iommu<x> to print corresponding capability
+		register of iommu<x>.
+
+		Example:
+		$ echo "0x10" > /sys/kernel/debug/iommu/amd/iommu00/capability
+		$ cat /sys/kernel/debug/iommu/amd/iommu00/capability
+
+		Output:
+		0x10
+
+What:		/sys/kernel/debug/iommu/amd/iommu<x>/capability_dump
+Date:		September 2024
+Contact:	Dheeraj Kumar Srivastava <dheerajkumar.srivastava@amd.com>
+Description:
+		This is an output read only file that contains capability register
+                value at offset "/sys/kernel/debug/iommu/amd/iommu<x>/capability".
+
+		Example:
+		$ cat /sys/kernel/debug/iommu/amd/iommu00/capability_dump
+
+		Output:
+		0x00203040
+
+What:		/sys/kernel/debug/iommu/amd/iommu<x>/cmdbuf
+Date:		September 2024
+Contact:	Dheeraj Kumar Srivastava <dheerajkumar.srivastava@amd.com>
+Description:
+		This file is an output read only file that contains iommu<x> command
+		buffer entries.
+
+		Examples:
+		$ cat /sys/kernel/debug/iommu/amd/iommu<x>/cmdbuf
+
+		Output:
+		CMD Buffer Head Offset:339 Tail Offset:339
+		  0: 008350011000000100003c0000000000
+		  1: 0000000030000005fffff0037fffffff
+		  2: 008350011000000100003c0100000000
+		  3: 0000000030000005fffff0037fffffff
+		  4: 008350011000000100003c0200000000
+		  5: 0000000030000005fffff0037fffffff
+		  6: 008350011000000100003c0300000000
+		  7: 0000000030000005fffff0037fffffff
+		  8: 008350011000000100003c0400000000
+		  9: 0000000030000005fffff0037fffffff
+		 10: 008350011000000100003c0500000000
+		 11: 0000000030000005fffff0037fffffff
+		[...]
+
+What:		/sys/kernel/debug/iommu/amd/devid
+Date:		September 2024
+Contact:	Dheeraj Kumar Srivastava <dheerajkumar.srivastava@amd.com>
+Description:
+		This is an input read/write file that takes device id user input.
+		This input can be used for dumping iommu data structures like
+		interrupt remapping table, device table etc.
+
+		Example:
+		1.
+		$ echo 0000:01:00.0 > /sys/kernel/debug/iommu/amd/devid
+		$ cat /sys/kernel/debug/iommu/amd/devid
+
+		Output:
+		0000:01:00.0
+
+		2.
+		$ echo 01:00.0 > /sys/kernel/debug/iommu/amd/devid
+		$ cat /sys/kernel/debug/iommu/amd/devid
+
+		Output:
+		0000:01:00.0
+
+What:		/sys/kernel/debug/iommu/devtbl
+Date:		September 2024
+Contact:	Dheeraj Kumar Srivastava <dheerajkumar.srivastava@amd.com>
+Description:
+		This is an output read only file that contains device table entry for
+		the device id input given in /sys/kernel/debug/iommu/amd/devid.
+
+		Example:
+		$ cat /sys/kernel/debug/iommu/amd/devtbl
+
+		Output:
+		DeviceId             QWORD[3]         QWORD[2]         QWORD[1]         QWORD[0] iommu
+		0000:01:00.0 0000000000000000 20000001373b8013 0000000000000038 6000000114d7b603 iommu3
+
+What:		/sys/kernel/debug/iommu/irqtbl
+Date:		September 2024
+Contact:	Dheeraj Kumar Srivastava <dheerajkumar.srivastava@amd.com>
+Description:
+		This is an output read only file that contains IRT table valid entries
+		for the device id input given in /sys/kernel/debug/iommu/amd/devid.
+
+		Example:
+		$ cat /sys/kernel/debug/iommu/amd/irqtbl
+
+		Output:
+		DeviceId 0000:01:00.0
+		IRT[0000] 00000000000000200000000000000241
+		IRT[0001] 00000000000000200000000000000841
+		IRT[0002] 00000000000000200000000000002041
+		IRT[0003] 00000000000000200000000000008041
+		IRT[0004] 00000000000000200000000000020041
+		IRT[0005] 00000000000000200000000000080041
+		IRT[0006] 00000000000000200000000000200041
+		IRT[0007] 00000000000000200000000000800041
+		[...]
-- 
2.25.1

Re: [PATCH v2 1/8] iommu/amd: Refactor AMD IOMMU debugfs initial setup

[Bjorn Helgaas]

On Wed, Nov 06, 2024 at 01:16:32PM +0530, Dheeraj Kumar Srivastava wrote:
> Rearrange initial setup of AMD IOMMU debugfs to segregate per IOMMU
> setup and setup which is common for all IOMMUs. This ensures that common
> debugfs paths (introduced in subsequent patches) are created only once
> instead of being created for each IOMMU.
> ...

> -void amd_iommu_debugfs_setup(struct amd_iommu *iommu)
> +void amd_iommu_debugfs_setup(void)
>  {
> +	struct amd_iommu *iommu;
>  	char name[MAX_NAME_LEN + 1];
>  
> -	mutex_lock(&amd_iommu_debugfs_lock);
> -	if (!amd_iommu_debugfs)
> -		amd_iommu_debugfs = debugfs_create_dir("amd",
> -						       iommu_debugfs_dir);
> -	mutex_unlock(&amd_iommu_debugfs_lock);
> +	amd_iommu_debugfs = debugfs_create_dir("amd", iommu_debugfs_dir);
>  
> -	snprintf(name, MAX_NAME_LEN, "iommu%02d", iommu->index);
> -	iommu->debugfs = debugfs_create_dir(name, amd_iommu_debugfs);
> +	for_each_iommu(iommu) {
> +		snprintf(name, MAX_NAME_LEN, "iommu%02d", iommu->index);
> +		iommu->debugfs = debugfs_create_dir(name, amd_iommu_debugfs);
> +	}

Doing this setup with for_each_iommu() precludes any hot-add of
IOMMUs, but I guess there's no indication of hotplug support anyway
given all the uses of for_each_iommu() in init_iommu_all(),
amd_iommu_init_pci(), amd_iommu_enable_interrupts(), etc.

>  }
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index 43131c3a2172..d78dc96bbec3 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -3377,7 +3377,6 @@ int amd_iommu_enable_faulting(unsigned int cpu)
>   */
>  static int __init amd_iommu_init(void)
>  {
> -	struct amd_iommu *iommu;
>  	int ret;
>  
>  	ret = iommu_go_to_state(IOMMU_INITIALIZED);
> @@ -3391,8 +3390,8 @@ static int __init amd_iommu_init(void)
>  	}
>  #endif
>  
> -	for_each_iommu(iommu)
> -		amd_iommu_debugfs_setup(iommu);
> +	if (!ret)
> +		amd_iommu_debugfs_setup();
>  
>  	return ret;
>  }
> -- 
> 2.25.1
> 

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

[Bjorn Helgaas]

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
> 

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

[Bjorn Helgaas]

On Wed, Nov 06, 2024 at 01:16:34PM +0530, Dheeraj Kumar Srivastava wrote:
> IOMMU Capability registers defines capabilities of IOMMU and information
> needed for initialising MMIO registers and device table. This is useful
> to dump these registers for debugging IOMMU related issues.
> 
> e.g.To get capability registers value for iommu<x>
>   # echo "0x10" > /sys/kernel/debug/iommu/amd/iommu00/capability
>   # cat /sys/kernel/debug/iommu/amd/iommu00/capability_dump

Same comment here.  Why does this need to be so complicated to use?
Can't you make a single read-only file that contains all the registers
of interest?

Bjorn

Re: [PATCH v2 5/8] iommu/amd: Add support for device id user input

[Bjorn Helgaas]

On Wed, Nov 06, 2024 at 01:16:36PM +0530, Dheeraj Kumar Srivastava wrote:
> Device id user input is needed for dumping IOMMU data structures like
> device table, IRT etc in AMD IOMMU debugfs.
> 
> eg.
> 1. # echo 0000:01:00.0 > /sys/kernel/debug/iommu/amd/devid
>    # cat /sys/kernel/debug/iommu/amd/devid
>    Output : 0000:01:00.0
> 
> 2. # echo 01:00.0 > /sys/kernel/debug/iommu/amd/devid
>    # cat /sys/kernel/debug/iommu/amd/devid
>    Output : 0000:01:00.0

The corresponding Intel IOMMU debugfs support encodes the BDF in the
pathname, e.g.,

  /sys/kernel/debug/iommu/intel/<bdf>/domain_translation_struct

Can you do the same?  I think it would be nicer to use a similar
style, and it would also make the code simpler.

Bjorn

Re: [PATCH v2 8/8] iommu/amd: Add documentation for AMD IOMMU debugfs support

[Bjorn Helgaas]

On Wed, Nov 06, 2024 at 01:16:39PM +0530, Dheeraj Kumar Srivastava wrote:
> Add documentation describing how to use AMD IOMMU debugfs support to
> dump IOMMU data structures - IRT table, Device table, Registers (MMIO and
> Capability) and command buffer.

> +What:		/sys/kernel/debug/iommu/devtbl

I guess you meant /sys/kernel/debug/iommu/amd/devtbl here?

> +What:		/sys/kernel/debug/iommu/irqtbl

and /sys/kernel/debug/iommu/amd/irqtbl here?

Re: [PATCH v2 8/8] iommu/amd: Add documentation for AMD IOMMU debugfs support

[Srivastava, Dheeraj Kumar]

Hi,

On 11/27/2024 2:49 AM, Bjorn Helgaas wrote:
> On Wed, Nov 06, 2024 at 01:16:39PM +0530, Dheeraj Kumar Srivastava wrote:
>> Add documentation describing how to use AMD IOMMU debugfs support to
>> dump IOMMU data structures - IRT table, Device table, Registers (MMIO and
>> Capability) and command buffer.
> 
>> +What:		/sys/kernel/debug/iommu/devtbl
> 
> I guess you meant /sys/kernel/debug/iommu/amd/devtbl here?
> 

Yeah. Will correct in next series.

>> +What:		/sys/kernel/debug/iommu/irqtbl
> 
> and /sys/kernel/debug/iommu/amd/irqtbl here?

Will correct in next series.

Thanks
Dheeraj

Re: [PATCH v2 1/8] iommu/amd: Refactor AMD IOMMU debugfs initial setup

[Srivastava, Dheeraj Kumar]

Hi,

On 11/27/2024 12:42 AM, Bjorn Helgaas wrote:
> On Wed, Nov 06, 2024 at 01:16:32PM +0530, Dheeraj Kumar Srivastava wrote:
>> Rearrange initial setup of AMD IOMMU debugfs to segregate per IOMMU
>> setup and setup which is common for all IOMMUs. This ensures that common
>> debugfs paths (introduced in subsequent patches) are created only once
>> instead of being created for each IOMMU.
>> ...
> 
>> -void amd_iommu_debugfs_setup(struct amd_iommu *iommu)
>> +void amd_iommu_debugfs_setup(void)
>>   {
>> +	struct amd_iommu *iommu;
>>   	char name[MAX_NAME_LEN + 1];
>>   
>> -	mutex_lock(&amd_iommu_debugfs_lock);
>> -	if (!amd_iommu_debugfs)
>> -		amd_iommu_debugfs = debugfs_create_dir("amd",
>> -						       iommu_debugfs_dir);
>> -	mutex_unlock(&amd_iommu_debugfs_lock);
>> +	amd_iommu_debugfs = debugfs_create_dir("amd", iommu_debugfs_dir);
>>   
>> -	snprintf(name, MAX_NAME_LEN, "iommu%02d", iommu->index);
>> -	iommu->debugfs = debugfs_create_dir(name, amd_iommu_debugfs);
>> +	for_each_iommu(iommu) {
>> +		snprintf(name, MAX_NAME_LEN, "iommu%02d", iommu->index);
>> +		iommu->debugfs = debugfs_create_dir(name, amd_iommu_debugfs);
>> +	}
> 
> Doing this setup with for_each_iommu() precludes any hot-add of
> IOMMUs, but I guess there's no indication of hotplug support anyway
> given all the uses of for_each_iommu() in init_iommu_all(),
> amd_iommu_init_pci(), amd_iommu_enable_interrupts(), etc.
> 

Yeah and here i don't see any concerns with the approach for now. Please 
do let me know if you have a better way to do this.

Thanks
Dheeraj

>>   }
>> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
>> index 43131c3a2172..d78dc96bbec3 100644
>> --- a/drivers/iommu/amd/init.c
>> +++ b/drivers/iommu/amd/init.c
>> @@ -3377,7 +3377,6 @@ int amd_iommu_enable_faulting(unsigned int cpu)
>>    */
>>   static int __init amd_iommu_init(void)
>>   {
>> -	struct amd_iommu *iommu;
>>   	int ret;
>>   
>>   	ret = iommu_go_to_state(IOMMU_INITIALIZED);
>> @@ -3391,8 +3390,8 @@ static int __init amd_iommu_init(void)
>>   	}
>>   #endif
>>   
>> -	for_each_iommu(iommu)
>> -		amd_iommu_debugfs_setup(iommu);
>> +	if (!ret)
>> +		amd_iommu_debugfs_setup();
>>   
>>   	return ret;
>>   }
>> -- 
>> 2.25.1
>>

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

[Srivastava, Dheeraj Kumar]

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

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

[Srivastava, Dheeraj Kumar]

Hi,

On 11/27/2024 2:29 AM, Bjorn Helgaas wrote:
> On Wed, Nov 06, 2024 at 01:16:34PM +0530, Dheeraj Kumar Srivastava wrote:
>> IOMMU Capability registers defines capabilities of IOMMU and information
>> needed for initialising MMIO registers and device table. This is useful
>> to dump these registers for debugging IOMMU related issues.
>>
>> e.g.To get capability registers value for iommu<x>
>>    # echo "0x10" > /sys/kernel/debug/iommu/amd/iommu00/capability
>>    # cat /sys/kernel/debug/iommu/amd/iommu00/capability_dump
> 
> Same comment here.  Why does this need to be so complicated to use?
> Can't you make a single read-only file that contains all the registers
> of interest?
> 

Please do let me know your concerns and views on my comments in the 
previous patch.

With the implemented approach we do need separate files for mmio 
registers and capability registers input/output files as to understand 
if user input is mmio's offset or capability register's offset.

Thanks
Dheeraj

> Bjorn

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

[Bjorn Helgaas]

On Tue, Jan 07, 2025 at 11:33:16AM +0530, Srivastava, Dheeraj Kumar wrote:
> On 11/27/2024 2:29 AM, Bjorn Helgaas wrote:
> > On Wed, Nov 06, 2024 at 01:16:34PM +0530, Dheeraj Kumar Srivastava wrote:
> > > IOMMU Capability registers defines capabilities of IOMMU and information
> > > needed for initialising MMIO registers and device table. This is useful
> > > to dump these registers for debugging IOMMU related issues.
> > > 
> > > e.g.To get capability registers value for iommu<x>
> > >    # echo "0x10" > /sys/kernel/debug/iommu/amd/iommu00/capability
> > >    # cat /sys/kernel/debug/iommu/amd/iommu00/capability_dump
> > 
> > Same comment here.  Why does this need to be so complicated to use?
> > Can't you make a single read-only file that contains all the registers
> > of interest?
> 
> Please do let me know your concerns and views on my comments in the
> previous patch.
> 
> With the implemented approach we do need separate files for mmio
> registers and capability registers input/output files as to
> understand if user input is mmio's offset or capability register's
> offset.

My comment is not about the difference between MMIO and config space
registers.  My concern is using two separate files to read the same
register.  That's inherently racy:

  UserA# echo "0x10" > /sys/kernel/debug/iommu/amd/iommu00/capability
  UserB# echo "0x20" > /sys/kernel/debug/iommu/amd/iommu00/capability
  UserA# cat /sys/kernel/debug/iommu/amd/iommu00/capability_dump

UserA expected to see the register at 0x10, but sees the one at 0x20
instead.

I think there's value in using a strategy similar to other IOMMU
drivers, e.g., intel.  But I'm not an IOMMU maintainer, so I'm just
kibbitzing here, and maybe your strategy is better.

Bjorn

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

[Srivastava, Dheeraj Kumar]

Hi,

On 1/8/2025 1:48 AM, Bjorn Helgaas wrote:
> On Tue, Jan 07, 2025 at 11:33:16AM +0530, Srivastava, Dheeraj Kumar wrote:
>> On 11/27/2024 2:29 AM, Bjorn Helgaas wrote:
>>> On Wed, Nov 06, 2024 at 01:16:34PM +0530, Dheeraj Kumar Srivastava wrote:
>>>> IOMMU Capability registers defines capabilities of IOMMU and information
>>>> needed for initialising MMIO registers and device table. This is useful
>>>> to dump these registers for debugging IOMMU related issues.
>>>>
>>>> e.g.To get capability registers value for iommu<x>
>>>>     # echo "0x10" > /sys/kernel/debug/iommu/amd/iommu00/capability
>>>>     # cat /sys/kernel/debug/iommu/amd/iommu00/capability_dump
>>>
>>> Same comment here.  Why does this need to be so complicated to use?
>>> Can't you make a single read-only file that contains all the registers
>>> of interest?
>>
>> Please do let me know your concerns and views on my comments in the
>> previous patch.
>>
>> With the implemented approach we do need separate files for mmio
>> registers and capability registers input/output files as to
>> understand if user input is mmio's offset or capability register's
>> offset.
> 
> My comment is not about the difference between MMIO and config space
> registers.  My concern is using two separate files to read the same
> register.  That's inherently racy:
> 
>    UserA# echo "0x10" > /sys/kernel/debug/iommu/amd/iommu00/capability
>    UserB# echo "0x20" > /sys/kernel/debug/iommu/amd/iommu00/capability
>    UserA# cat /sys/kernel/debug/iommu/amd/iommu00/capability_dump
> 
> UserA expected to see the register at 0x10, but sees the one at 0x20
> instead.
> 
> I think there's value in using a strategy similar to other IOMMU
> drivers, e.g., intel.  But I'm not an IOMMU maintainer, so I'm just
> kibbitzing here, and maybe your strategy is better.

Thanks for clarifying. This makes sense. Will update in the next series.

Regards
Dheeraj

> 
> Bjorn

Re: [PATCH v2 5/8] iommu/amd: Add support for device id user input

[Srivastava, Dheeraj Kumar]

On 11/27/2024 2:32 AM, Bjorn Helgaas wrote:
> On Wed, Nov 06, 2024 at 01:16:36PM +0530, Dheeraj Kumar Srivastava wrote:
>> Device id user input is needed for dumping IOMMU data structures like
>> device table, IRT etc in AMD IOMMU debugfs.
>>
>> eg.
>> 1. # echo 0000:01:00.0 > /sys/kernel/debug/iommu/amd/devid
>>     # cat /sys/kernel/debug/iommu/amd/devid
>>     Output : 0000:01:00.0
>>
>> 2. # echo 01:00.0 > /sys/kernel/debug/iommu/amd/devid
>>     # cat /sys/kernel/debug/iommu/amd/devid
>>     Output : 0000:01:00.0
> 
> The corresponding Intel IOMMU debugfs support encodes the BDF in the
> pathname, e.g.,
> 
>    /sys/kernel/debug/iommu/intel/<bdf>/domain_translation_struct
> 
> Can you do the same?  I think it would be nicer to use a similar
> style, and it would also make the code simpler.
> 

Hi, Yeah i do agree with your suggestion. But lets keep this as open 
question about how we want the interface to be and get more suggestions 
on this one. Will reiterate with all other changes suggested.


Regards
Dheeraj

> Bjorn