[PATCH v2 0/5] Add debugfs info for the AMD IOMMU

[PATCH v2 0/5] Add debugfs info for the AMD IOMMU

[Gary R Hook]

The following series creates a debugfs directory for AMD IOMMUs,
constructs a framework for additional entries, an online README,
and a method for dumping device table entries. Data is reported
in a default concise mode, but a verbose mode is enabled via a
filesystem entry.

This is the first of three patch series that will expose a number
of IOMMU registers.

Changes since v1:
- Correctly use CONFIG_AMD_IOMMU_DEBUG in Makefile and header file

---

Gary R Hook (5):
      iommu/amd - Add debugfs support
      iommu/amd - Add a 'verbose' switch for IOMMU debugfs
      iommu/amd - Add a README variable for the IOMMU debugfs
      iommu/amd - Expose the active IOMMU device table entries
      iommu/amd - Add a debugfs entry to specify a IOMMU device table entry


 drivers/iommu/Kconfig             |    8 +
 drivers/iommu/Makefile            |    1 
 drivers/iommu/amd_iommu_debugfs.c |  310 +++++++++++++++++++++++++++++++++++++
 drivers/iommu/amd_iommu_init.c    |    7 +
 drivers/iommu/amd_iommu_proto.h   |    7 +
 drivers/iommu/amd_iommu_types.h   |    3 
 6 files changed, 334 insertions(+), 2 deletions(-)
 create mode 100644 drivers/iommu/amd_iommu_debugfs.c

--

[PATCH v2 1/5] iommu/amd - Add debugfs support

[Gary R Hook]

Expose the IOMMU MMIO registers and device table

Signed-off-by: Gary R Hook <gary.hook@amd.com>
---
 drivers/iommu/Kconfig             |    8 +++
 drivers/iommu/Makefile            |    1 
 drivers/iommu/amd_iommu_debugfs.c |  112 +++++++++++++++++++++++++++++++++++++
 drivers/iommu/amd_iommu_init.c    |    7 ++
 drivers/iommu/amd_iommu_proto.h   |    7 ++
 drivers/iommu/amd_iommu_types.h   |    3 +
 6 files changed, 136 insertions(+), 2 deletions(-)
 create mode 100644 drivers/iommu/amd_iommu_debugfs.c

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index f3a21343e636..0fa85e73d5dd 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -135,6 +135,14 @@ config AMD_IOMMU_V2
 	  hardware. Select this option if you want to use devices that support
 	  the PCI PRI and PASID interface.
 
+config AMD_IOMMU_DEBUG
+	bool "Expose AMD IOMMU internals in DebugFS"
+	depends on AMD_IOMMU && DEBUG_FS
+	default n
+	help
+	  Provides debugfs access to IOMMU data such as registers and device
+	  table entries.
+
 # Intel IOMMU support
 config DMAR_TABLE
 	bool
diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
index 1fb695854809..64fba8b1ca4f 100644
--- a/drivers/iommu/Makefile
+++ b/drivers/iommu/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_IOMMU_IOVA) += iova.o
 obj-$(CONFIG_OF_IOMMU)	+= of_iommu.o
 obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o
 obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o amd_iommu_init.o
+obj-$(CONFIG_AMD_IOMMU_DEBUG) += amd_iommu_debugfs.o
 obj-$(CONFIG_AMD_IOMMU_V2) += amd_iommu_v2.o
 obj-$(CONFIG_ARM_SMMU) += arm-smmu.o
 obj-$(CONFIG_ARM_SMMU_V3) += arm-smmu-v3.o
diff --git a/drivers/iommu/amd_iommu_debugfs.c b/drivers/iommu/amd_iommu_debugfs.c
new file mode 100644
index 000000000000..b0395a47bc32
--- /dev/null
+++ b/drivers/iommu/amd_iommu_debugfs.c
@@ -0,0 +1,112 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * AMD IOMMU driver
+ *
+ * Copyright (C) 2018 Advanced Micro Devices, Inc.
+ *
+ * Author: Gary R Hook <gary.hook@amd.com>
+ */
+
+#ifdef	CONFIG_DEBUG_FS
+
+#include <linux/pci.h>
+#include <linux/iommu.h>
+#include <linux/debugfs.h>
+#include "amd_iommu_proto.h"
+#include "amd_iommu_types.h"
+
+/* DebugFS helpers */
+#define	OBUFP		(obuf + oboff)
+#define	OBUFLEN		obuflen
+#define	OBUFSPC		(OBUFLEN - oboff)
+#define	OSCNPRINTF(fmt, ...) \
+		scnprintf(OBUFP, OBUFSPC, fmt, ## __VA_ARGS__)
+
+static struct dentry *iommu_debugfs_dir;
+static DEFINE_RWLOCK(iommu_debugfs_lock);
+
+#define	MAX_NAME_LEN	20
+
+static unsigned int amd_iommu_count_valid_dtes(int start, int end)
+{
+	unsigned int n = 0;
+	int i;
+
+	/* Scan the DTE table from entry 'start' through entry 'end' for
+	 * active entries
+	 */
+	for (i = start ; i <= end ; i++)
+		if ((amd_iommu_dev_table[i].data[0] ^ 0x3)
+		    || amd_iommu_dev_table[i].data[1])
+			n++;
+	return n;
+}
+
+static ssize_t amd_iommu_debugfs_dtecount_read(struct file *filp,
+					  char __user *ubuf,
+					  size_t count, loff_t *offp)
+{
+	struct amd_iommu *iommu = filp->private_data;
+	unsigned int obuflen = 512;
+	unsigned int oboff = 0;
+	unsigned int n;
+	ssize_t ret;
+	char *obuf;
+
+	if (!iommu)
+		return 0;
+
+	obuf = kmalloc(OBUFLEN, GFP_KERNEL);
+	if (!obuf)
+		return -ENOMEM;
+
+	n = amd_iommu_count_valid_dtes(0, 0xFFFF);
+	oboff += OSCNPRINTF("%d\n", n);
+
+	ret = simple_read_from_buffer(ubuf, count, offp, obuf, oboff);
+	kfree(obuf);
+
+	return ret;
+}
+
+static const struct file_operations amd_iommu_debugfs_dtecount_ops = {
+	.owner = THIS_MODULE,
+	.open = simple_open,
+	.read = amd_iommu_debugfs_dtecount_read,
+	.write = NULL,
+};
+
+void amd_iommu_debugfs_setup(struct amd_iommu *iommu)
+{
+	char name[MAX_NAME_LEN + 1];
+	struct dentry *d_dte;
+	unsigned long flags;
+
+	if (!debugfs_initialized())
+		return;
+
+	write_lock_irqsave(&iommu_debugfs_lock, flags);
+	if (!iommu_debugfs_dir)
+		iommu_debugfs_dir = debugfs_create_dir(KBUILD_MODNAME, NULL);
+	write_unlock_irqrestore(&iommu_debugfs_lock, flags);
+	if (!iommu_debugfs_dir)
+		goto err;
+
+	snprintf(name, MAX_NAME_LEN, "iommu%02x", iommu->index);
+	iommu->debugfs_instance = debugfs_create_dir(name, iommu_debugfs_dir);
+	if (!iommu->debugfs_instance)
+		goto err;
+
+	d_dte = debugfs_create_file("count", 0400,
+				    iommu->debugfs_instance, iommu,
+				    &amd_iommu_debugfs_dtecount_ops);
+	if (!d_dte)
+		goto err;
+
+	return;
+
+err:
+	debugfs_remove_recursive(iommu->debugfs_instance);
+}
+
+#endif
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 4e4a615bf13f..501433c29a3e 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -89,6 +89,7 @@
 #define ACPI_DEVFLAG_ATSDIS             0x10000000
 
 #define LOOP_TIMEOUT	100000
+
 /*
  * ACPI table definitions
  *
@@ -2721,6 +2722,7 @@ int __init amd_iommu_enable_faulting(void)
  */
 static int __init amd_iommu_init(void)
 {
+	struct amd_iommu *iommu;
 	int ret;
 
 	ret = iommu_go_to_state(IOMMU_INITIALIZED);
@@ -2730,14 +2732,15 @@ static int __init amd_iommu_init(void)
 			disable_iommus();
 			free_iommu_resources();
 		} else {
-			struct amd_iommu *iommu;
-
 			uninit_device_table_dma();
 			for_each_iommu(iommu)
 				iommu_flush_all_caches(iommu);
 		}
 	}
 
+	for_each_iommu(iommu)
+		amd_iommu_debugfs_setup(iommu);
+
 	return ret;
 }
 
diff --git a/drivers/iommu/amd_iommu_proto.h b/drivers/iommu/amd_iommu_proto.h
index 640c286a0ab9..db5872d3d40f 100644
--- a/drivers/iommu/amd_iommu_proto.h
+++ b/drivers/iommu/amd_iommu_proto.h
@@ -33,6 +33,13 @@ extern void amd_iommu_uninit_devices(void);
 extern void amd_iommu_init_notifier(void);
 extern int amd_iommu_init_api(void);
 
+#ifdef	CONFIG_AMD_IOMMU_DEBUG
+extern void amd_iommu_debugfs_setup(struct amd_iommu *iommu);
+#else
+static inline void amd_iommu_debugfs_setup(struct amd_iommu *iommu) {}
+#endif
+
+
 /* Needed for interrupt remapping */
 extern int amd_iommu_prepare(void);
 extern int amd_iommu_enable(void);
diff --git a/drivers/iommu/amd_iommu_types.h b/drivers/iommu/amd_iommu_types.h
index 6a877ebd058b..43c008dc7ade 100644
--- a/drivers/iommu/amd_iommu_types.h
+++ b/drivers/iommu/amd_iommu_types.h
@@ -593,6 +593,9 @@ struct amd_iommu {
 
 	u32 flags;
 	volatile u64 __aligned(8) cmd_sem;
+
+	/* DebugFS Info */
+	struct dentry *debugfs_instance;
 };
 
 static inline struct amd_iommu *dev_to_amd_iommu(struct device *dev)

[PATCH v2 2/5] iommu/amd - Add a 'verbose' switch for IOMMU debugfs

[Gary R Hook]

Enable more descriptive debugfs output via a 'verbose' variable.

Signed-off-by: Gary R Hook <gary.hook@amd.com>
---
 drivers/iommu/amd_iommu_debugfs.c |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/amd_iommu_debugfs.c b/drivers/iommu/amd_iommu_debugfs.c
index b0395a47bc32..481f4d86f8f8 100644
--- a/drivers/iommu/amd_iommu_debugfs.c
+++ b/drivers/iommu/amd_iommu_debugfs.c
@@ -27,6 +27,8 @@ static DEFINE_RWLOCK(iommu_debugfs_lock);
 
 #define	MAX_NAME_LEN	20
 
+static unsigned int amd_iommu_verbose = 0;
+
 static unsigned int amd_iommu_count_valid_dtes(int start, int end)
 {
 	unsigned int n = 0;
@@ -61,7 +63,10 @@ static ssize_t amd_iommu_debugfs_dtecount_read(struct file *filp,
 		return -ENOMEM;
 
 	n = amd_iommu_count_valid_dtes(0, 0xFFFF);
-	oboff += OSCNPRINTF("%d\n", n);
+	if (amd_iommu_verbose)
+		oboff += OSCNPRINTF("# DTEs:  %d\n", n);
+	else
+		oboff += OSCNPRINTF("%d\n", n);
 
 	ret = simple_read_from_buffer(ubuf, count, offp, obuf, oboff);
 	kfree(obuf);
@@ -79,6 +84,7 @@ static const struct file_operations amd_iommu_debugfs_dtecount_ops = {
 void amd_iommu_debugfs_setup(struct amd_iommu *iommu)
 {
 	char name[MAX_NAME_LEN + 1];
+	struct dentry *d_verbose;
 	struct dentry *d_dte;
 	unsigned long flags;
 
@@ -97,6 +103,12 @@ void amd_iommu_debugfs_setup(struct amd_iommu *iommu)
 	if (!iommu->debugfs_instance)
 		goto err;
 
+	d_verbose = debugfs_create_u32("verbose", 0600,
+				       iommu->debugfs_instance,
+				       &amd_iommu_verbose);
+	if (!d_verbose)
+		goto err;
+
 	d_dte = debugfs_create_file("count", 0400,
 				    iommu->debugfs_instance, iommu,
 				    &amd_iommu_debugfs_dtecount_ops);

[PATCH v2 3/5] iommu/amd - Add a README variable for the IOMMU debugfs

[Gary R Hook]

Provide help text via a filesystem entry

Signed-off-by: Gary R Hook <gary.hook@amd.com>
---
 drivers/iommu/amd_iommu_debugfs.c |   31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/iommu/amd_iommu_debugfs.c b/drivers/iommu/amd_iommu_debugfs.c
index 481f4d86f8f8..79945ce1199d 100644
--- a/drivers/iommu/amd_iommu_debugfs.c
+++ b/drivers/iommu/amd_iommu_debugfs.c
@@ -81,6 +81,31 @@ static const struct file_operations amd_iommu_debugfs_dtecount_ops = {
 	.write = NULL,
 };
 
+static char readmetext[] =
+"count                   Count of active devices\n"
+"verbose                 Provide additional descriptive text\n"
+"\n";
+
+static ssize_t amd_iommu_debugfs_readme_read(struct file *filp,
+				      char __user *ubuf,
+				      size_t count, loff_t *offp)
+{
+	ssize_t ret;
+
+	ret = simple_read_from_buffer(ubuf, count, offp,
+				      readmetext, strlen(readmetext));
+
+	return ret;
+}
+
+
+static const struct file_operations amd_iommu_debugfs_readme_ops = {
+	.owner = THIS_MODULE,
+	.open = simple_open,
+	.read = amd_iommu_debugfs_readme_read,
+	.write = NULL,
+};
+
 void amd_iommu_debugfs_setup(struct amd_iommu *iommu)
 {
 	char name[MAX_NAME_LEN + 1];
@@ -115,6 +140,12 @@ void amd_iommu_debugfs_setup(struct amd_iommu *iommu)
 	if (!d_dte)
 		goto err;
 
+	d_dte = debugfs_create_file("README", 0400,
+				    iommu->debugfs_instance, iommu,
+				    &amd_iommu_debugfs_readme_ops);
+	if (!d_dte)
+		goto err;
+
 	return;
 
 err:

[PATCH v2 4/5] iommu/amd - Expose the active IOMMU device table entries

[Gary R Hook]

Add a debugfs entry to dump the active device table entries from
the IOMMU's table. Active is determined by non-default values
in the first and second long words of the DTE. Aside from IOMMU
devices, this output should list every device reported by lspci.

Signed-off-by: Gary R Hook <gary.hook@amd.com>
---
 drivers/iommu/amd_iommu_debugfs.c |   64 +++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/drivers/iommu/amd_iommu_debugfs.c b/drivers/iommu/amd_iommu_debugfs.c
index 79945ce1199d..c4e071f7a5b9 100644
--- a/drivers/iommu/amd_iommu_debugfs.c
+++ b/drivers/iommu/amd_iommu_debugfs.c
@@ -81,9 +81,66 @@ static const struct file_operations amd_iommu_debugfs_dtecount_ops = {
 	.write = NULL,
 };
 
+#define	MAX_PCI_ID	0xFFFF
+
+#define	PRINTDTE(i)	OSCNPRINTF("%02x:%02x:%x - %016llx %016llx %016llx %016llx\n", \
+				   PCI_BUS_NUM(i), PCI_SLOT(i), PCI_FUNC(i), \
+				   amd_iommu_dev_table[i].data[0], \
+				   amd_iommu_dev_table[i].data[1], \
+				   amd_iommu_dev_table[i].data[2], \
+				   amd_iommu_dev_table[i].data[3]);
+
+static ssize_t amd_iommu_debugfs_dte_read(struct file *filp,
+					  char __user *ubuf,
+					  size_t count, loff_t *offp)
+{
+	struct amd_iommu *iommu = filp->private_data;
+	unsigned int obuflen;
+	unsigned int oboff = 0;
+	unsigned int istart, iend;
+	ssize_t ret;
+	u32 i, n;
+	char *obuf;
+
+	if (!iommu)
+		return 0;
+
+	/* Count the number of valid entries in the device table */
+	istart = 0;
+	iend = MAX_PCI_ID;
+	n = amd_iommu_count_valid_dtes(istart, iend);
+	obuflen = n * 80;
+
+	obuf = kmalloc(OBUFLEN, GFP_KERNEL);
+	if (!obuf)
+		return -ENOMEM;
+
+	for (i = istart ; i <= iend ; i++)
+		if ((amd_iommu_dev_table[i].data[0] ^ 0x3)
+		     || amd_iommu_dev_table[i].data[1])
+			oboff += PRINTDTE(i);
+
+	ret = simple_read_from_buffer(ubuf, count, offp, obuf, oboff);
+	kfree(obuf);
+
+	return ret;
+}
+
+static const struct file_operations amd_iommu_debugfs_dte_ops = {
+	.owner = THIS_MODULE,
+	.open = simple_open,
+	.read = amd_iommu_debugfs_dte_read,
+	.write = NULL,
+};
+
 static char readmetext[] =
+"devicetable             Print active entries in the device table\n"
 "count                   Count of active devices\n"
 "verbose                 Provide additional descriptive text\n"
+"\n"
+"                        Dumping the Device Table\n"
+"The device table is scanned for entries that appear to be active. The\n"
+"default range is from 0 to 0xFFFF, and only active entries will be reported\n"
 "\n";
 
 static ssize_t amd_iommu_debugfs_readme_read(struct file *filp,
@@ -134,6 +191,13 @@ void amd_iommu_debugfs_setup(struct amd_iommu *iommu)
 	if (!d_verbose)
 		goto err;
 
+	/* Device Table Entries */
+	d_dte = debugfs_create_file("devicetable", 0400,
+				    iommu->debugfs_instance, iommu,
+				    &amd_iommu_debugfs_dte_ops);
+	if (!d_dte)
+		goto err;
+
 	d_dte = debugfs_create_file("count", 0400,
 				    iommu->debugfs_instance, iommu,
 				    &amd_iommu_debugfs_dtecount_ops);

[PATCH v2 5/5] iommu/amd - Add a debugfs entry to specify a IOMMU device table entry

[Gary R Hook]

Initially (at boot) the device table values dumped are all of the
active devices.  Add a devid debugfs file to allow the user to select a
single device table entry to dump (active or not). Let any devid value
greater than the maximum allowable PCI ID (0xFFFF) restore the
behavior to that effective at boot.

Signed-off-by: Gary R Hook <gary.hook@amd.com>
---
 drivers/iommu/amd_iommu_debugfs.c |  109 ++++++++++++++++++++++++++++++++++---
 1 file changed, 100 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/amd_iommu_debugfs.c b/drivers/iommu/amd_iommu_debugfs.c
index c4e071f7a5b9..aa6935340163 100644
--- a/drivers/iommu/amd_iommu_debugfs.c
+++ b/drivers/iommu/amd_iommu_debugfs.c
@@ -28,6 +28,7 @@ static DEFINE_RWLOCK(iommu_debugfs_lock);
 #define	MAX_NAME_LEN	20
 
 static unsigned int amd_iommu_verbose = 0;
+static unsigned int amd_iommu_devid = ~0;
 
 static unsigned int amd_iommu_count_valid_dtes(int start, int end)
 {
@@ -81,6 +82,76 @@ static const struct file_operations amd_iommu_debugfs_dtecount_ops = {
 	.write = NULL,
 };
 
+static ssize_t amd_iommu_debugfs_devid_read(struct file *filp,
+					    char __user *ubuf,
+					    size_t count, loff_t *offp)
+{
+	struct amd_iommu *iommu = filp->private_data;
+	unsigned int obuflen = 512;
+	unsigned int oboff = 0;
+	ssize_t ret;
+	char *obuf;
+
+	if (!iommu)
+		return 0;
+
+	obuf = kmalloc(OBUFLEN, GFP_KERNEL);
+	if (!obuf)
+		return -ENOMEM;
+
+	if (amd_iommu_verbose)
+		oboff += OSCNPRINTF("%02x:%02x:%x (%u / %04x)\n",
+				    PCI_BUS_NUM(amd_iommu_devid),
+				    PCI_SLOT(amd_iommu_devid),
+				    PCI_FUNC(amd_iommu_devid),
+				    amd_iommu_devid, amd_iommu_devid);
+	else
+		oboff += OSCNPRINTF("%u\n", amd_iommu_devid);
+
+	ret = simple_read_from_buffer(ubuf, count, offp, obuf, oboff);
+	kfree(obuf);
+
+	return ret;
+}
+
+static ssize_t amd_iommu_debugfs_devid_write(struct file *filp,
+					    const char __user *ubuf,
+					    size_t count, loff_t *offp)
+{
+	unsigned int pci_id, pci_slot, pci_func;
+	unsigned int obuflen = 80;
+	ssize_t ret;
+	char *obuf;
+	int n;
+
+	obuf = kmalloc(OBUFLEN, GFP_KERNEL);
+	if (!obuf)
+		return -ENOMEM;
+
+	ret = simple_write_to_buffer(obuf, OBUFLEN, offp, ubuf, count);
+
+	if (strnchr(obuf, OBUFLEN, ':'))
+	{
+		n = sscanf(obuf, "%x:%x.%x", &pci_id, &pci_slot, &pci_func);
+		if (n == 3)
+			amd_iommu_devid = PCI_DEVID(pci_id, PCI_DEVFN(pci_slot, pci_func));
+	} else if (obuf[0] == '0' && obuf[1] == 'x') {
+		n = sscanf(obuf, "%x", &amd_iommu_devid);
+	} else {
+		n = sscanf(obuf, "%d", &amd_iommu_devid);
+	}
+	kfree(obuf);
+
+	return ret;
+}
+
+static const struct file_operations amd_iommu_debugfs_devid_ops = {
+	.owner = THIS_MODULE,
+	.open = simple_open,
+	.read = amd_iommu_debugfs_devid_read,
+	.write = amd_iommu_debugfs_devid_write,
+};
+
 #define	MAX_PCI_ID	0xFFFF
 
 #define	PRINTDTE(i)	OSCNPRINTF("%02x:%02x:%x - %016llx %016llx %016llx %016llx\n", \
@@ -106,19 +177,28 @@ static ssize_t amd_iommu_debugfs_dte_read(struct file *filp,
 		return 0;
 
 	/* Count the number of valid entries in the device table */
-	istart = 0;
-	iend = MAX_PCI_ID;
-	n = amd_iommu_count_valid_dtes(istart, iend);
+	if (amd_iommu_devid > MAX_PCI_ID) {
+		istart = 0;
+		iend = MAX_PCI_ID;
+		n = amd_iommu_count_valid_dtes(istart, iend);
+	} else {
+		n = 1;
+	}
 	obuflen = n * 80;
 
 	obuf = kmalloc(OBUFLEN, GFP_KERNEL);
 	if (!obuf)
 		return -ENOMEM;
 
-	for (i = istart ; i <= iend ; i++)
-		if ((amd_iommu_dev_table[i].data[0] ^ 0x3)
-		     || amd_iommu_dev_table[i].data[1])
-			oboff += PRINTDTE(i);
+	if (amd_iommu_devid > MAX_PCI_ID) {
+		for (i = istart ; i <= iend ; i++)
+			if ((amd_iommu_dev_table[i].data[0] ^ 0x3)
+			     || amd_iommu_dev_table[i].data[1])
+				oboff += PRINTDTE(i);
+	} else {
+		i = amd_iommu_devid;
+		oboff += PRINTDTE(i);
+	}
 
 	ret = simple_read_from_buffer(ubuf, count, offp, obuf, oboff);
 	kfree(obuf);
@@ -135,12 +215,17 @@ static const struct file_operations amd_iommu_debugfs_dte_ops = {
 
 static char readmetext[] =
 "devicetable             Print active entries in the device table\n"
+"devid                   Controls which device IDs are printed\n"
 "count                   Count of active devices\n"
 "verbose                 Provide additional descriptive text\n"
 "\n"
 "                        Dumping the Device Table\n"
-"The device table is scanned for entries that appear to be active. The\n"
-"default range is from 0 to 0xFFFF, and only active entries will be reported\n"
+"The device table is scanned for entries that appear to be active.\n"
+"The default (initial) range is from 0 to 0xFFFF, represented by a devid\n"
+"value greater than 0xFFFF, and only active entries will be reported.\n"
+"If devid is set to a specific value, only that device entry will be\n"
+"displayed (active or not). devid may be specified as ##:##:#, a decimal\n"
+"value, or a hex value.\n"
 "\n";
 
 static ssize_t amd_iommu_debugfs_readme_read(struct file *filp,
@@ -204,6 +289,12 @@ void amd_iommu_debugfs_setup(struct amd_iommu *iommu)
 	if (!d_dte)
 		goto err;
 
+	d_dte = debugfs_create_file("devid", 0400,
+				    iommu->debugfs_instance, iommu,
+				    &amd_iommu_debugfs_devid_ops);
+	if (!d_dte)
+		goto err;
+
 	d_dte = debugfs_create_file("README", 0400,
 				    iommu->debugfs_instance, iommu,
 				    &amd_iommu_debugfs_readme_ops);

Re: [PATCH v2 1/5] iommu/amd - Add debugfs support

[Andy Shevchenko]

On Fri, Mar 9, 2018 at 2:50 AM, Gary R Hook <gary.hook@amd.com> wrote:

> +       default n

Redundant


> +#include <linux/pci.h>
> +#include <linux/iommu.h>
> +#include <linux/debugfs.h>

Keep in order?

> +#include "amd_iommu_proto.h"
> +#include "amd_iommu_types.h"

> +/* DebugFS helpers */
> +#define        OBUFP           (obuf + oboff)
> +#define        OBUFLEN         obuflen
> +#define        OBUFSPC         (OBUFLEN - oboff)
> +#define        OSCNPRINTF(fmt, ...) \
> +               scnprintf(OBUFP, OBUFSPC, fmt, ## __VA_ARGS__)

I don't see any advantages of this. Other way around, they will simple
makes things hard to read an understand in place.


> +       for (i = start ; i <= end ; i++)

Missed {}

> +               if ((amd_iommu_dev_table[i].data[0] ^ 0x3)
> +                   || amd_iommu_dev_table[i].data[1])
> +                       n++;
> +       return n;
> +}

> +
> +static ssize_t amd_iommu_debugfs_dtecount_read(struct file *filp,
> +                                         char __user *ubuf,
> +                                         size_t count, loff_t *offp)
> +{
> +       struct amd_iommu *iommu = filp->private_data;

> +       unsigned int obuflen = 512;

Sounds like way too much.

> +       if (!iommu)
> +               return 0;

When this possible?

> +       obuf = kmalloc(OBUFLEN, GFP_KERNEL);
> +       if (!obuf)
> +               return -ENOMEM;
> +
> +       n = amd_iommu_count_valid_dtes(0, 0xFFFF);
> +       oboff += OSCNPRINTF("%d\n", n);

> +       return ret;
> +}


> @@ -89,6 +89,7 @@
>  #define ACPI_DEVFLAG_ATSDIS             0x10000000
>
>  #define LOOP_TIMEOUT   100000
> +
>  /*
>   * ACPI table definitions
>   *

Doesn't belong to the patch.

> +#endif
> +
> +

Extra unneeded line.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/5] iommu/amd - Add a debugfs entry to specify a IOMMU device table entry

[Andy Shevchenko]

On Fri, Mar 9, 2018 at 2:51 AM, Gary R Hook <gary.hook@amd.com> wrote:
> Initially (at boot) the device table values dumped are all of the
> active devices.  Add a devid debugfs file to allow the user to select a
> single device table entry to dump (active or not). Let any devid value
> greater than the maximum allowable PCI ID (0xFFFF) restore the
> behavior to that effective at boot.

> +               oboff += OSCNPRINTF("%02x:%02x:%x (%u / %04x)\n",
> +                                   PCI_BUS_NUM(amd_iommu_devid),
> +                                   PCI_SLOT(amd_iommu_devid),
> +                                   PCI_FUNC(amd_iommu_devid),

Perhaps at some point we will have an extension to %p to print PCI BDFs.

> +       if (strnchr(obuf, OBUFLEN, ':'))
> +       {

Style

> +       } else if (obuf[0] == '0' && obuf[1] == 'x') {
> +               n = sscanf(obuf, "%x", &amd_iommu_devid);
> +       } else {
> +               n = sscanf(obuf, "%d", &amd_iommu_devid);
> +       }

kstrtoint() ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/5] iommu/amd - Add debugfs support

[Gary R Hook]

On 03/13/2018 12:16 PM, Andy Shevchenko wrote:
> On Fri, Mar 9, 2018 at 2:50 AM, Gary R Hook <gary.hook@amd.com> wrote:
> 
>> +       default n
> 
> Redundant

Roger that.

>> +#include <linux/pci.h>
>> +#include <linux/iommu.h>
>> +#include <linux/debugfs.h>
> 
> Keep in order?

What order would that be? These few needed files are listed in the same
order as which they appear in amd_iommu.c. I'm gonna need a preference
spelled out, please (and a rationale, so I may better understand).

>> +#include "amd_iommu_proto.h"
>> +#include "amd_iommu_types.h"
> 
>> +/* DebugFS helpers */
>> +#define        OBUFP           (obuf + oboff)
>> +#define        OBUFLEN         obuflen
>> +#define        OBUFSPC         (OBUFLEN - oboff)
>> +#define        OSCNPRINTF(fmt, ...) \
>> +               scnprintf(OBUFP, OBUFSPC, fmt, ## __VA_ARGS__)
> 
> I don't see any advantages of this. Other way around, they will simple
> makes things hard to read an understand in place.

I used this technique in the CCP driver code (where it was accepted), in 
an effort to do the opposite of what you claim: make the code more 
readable. Given the 80 column limit, a large number of arguments, and 
very long statements, IMO something needs to give. I don't find the use 
of #defines to be obfuscating.

I'm not trying to argue, but rather simply state the perspective / 
reasoning I used to create a source file I feel is manageable. I have 17 
more iommu patches built upon this strategy, and this seems to be 
advantageous for all of them.

> 
> 
>> +       for (i = start ; i <= end ; i++)
> 
> Missed {}

Wasn't sure about the M.O. given that the body of this loop is a single 
if statement. And I don't see anywhere in
https://www.kernel.org/doc/html/latest/process/coding-style.html
in section 3.1 where curly braces are called for in this situation. May 
I ask for clarification on the style rule, please?

> 
>> +               if ((amd_iommu_dev_table[i].data[0] ^ 0x3)
>> +                   || amd_iommu_dev_table[i].data[1])
>> +                       n++;
>> +       return n;
>> +}
> 
>> +
>> +static ssize_t amd_iommu_debugfs_dtecount_read(struct file *filp,
>> +                                         char __user *ubuf,
>> +                                         size_t count, loff_t *offp)
>> +{
>> +       struct amd_iommu *iommu = filp->private_data;
> 
>> +       unsigned int obuflen = 512;
> 
> Sounds like way too much.

I can tune these up.

> 
>> +       if (!iommu)
>> +               return 0;
> 
> When this possible?

It was intended as a sanity check, but if this happens, much worse has 
already gone wrong. I'll remove.

> 
>> +       obuf = kmalloc(OBUFLEN, GFP_KERNEL);
>> +       if (!obuf)
>> +               return -ENOMEM;
>> +
>> +       n = amd_iommu_count_valid_dtes(0, 0xFFFF);
>> +       oboff += OSCNPRINTF("%d\n", n);
> 
>> +       return ret;
>> +}
> 
> 
>> @@ -89,6 +89,7 @@
>>   #define ACPI_DEVFLAG_ATSDIS             0x10000000
>>
>>   #define LOOP_TIMEOUT   100000
>> +
>>   /*
>>    * ACPI table definitions
>>    *
> 
> Doesn't belong to the patch.

I'm sorry, I don't understand. The added blank line doesn't belong to 
the patch?

> 
>> +#endif
>> +
>> +
> 
> Extra unneeded line.
> 
Thanks,

Re: [PATCH v2 5/5] iommu/amd - Add a debugfs entry to specify a IOMMU device table entry

[Gary R Hook]

On 03/13/2018 12:20 PM, Andy Shevchenko wrote:
> 
>> +               oboff += OSCNPRINTF("%02x:%02x:%x (%u / %04x)\n",
>> +                                   PCI_BUS_NUM(amd_iommu_devid),
>> +                                   PCI_SLOT(amd_iommu_devid),
>> +                                   PCI_FUNC(amd_iommu_devid),
> 
> Perhaps at some point we will have an extension to %p to print PCI BDFs.

But until then....  ;-)

>> +       if (strnchr(obuf, OBUFLEN, ':'))
>> +       {
> 
> Style

D'oh!

>> +       } else if (obuf[0] == '0' && obuf[1] == 'x') {
>> +               n = sscanf(obuf, "%x", &amd_iommu_devid);
>> +       } else {
>> +               n = sscanf(obuf, "%d", &amd_iommu_devid);
>> +       }
> 
> kstrtoint() ?

I see various mechanisms for this sort of thing, and simply chose one.
Am happy to use whatever is preferred.

Re: [PATCH v2 1/5] iommu/amd - Add debugfs support

[Andy Shevchenko]

On Tue, Mar 13, 2018 at 8:54 PM, Gary R Hook <gary.hook@amd.com> wrote:
> On 03/13/2018 12:16 PM, Andy Shevchenko wrote:
>> On Fri, Mar 9, 2018 at 2:50 AM, Gary R Hook <gary.hook@amd.com> wrote:

>>> +#include <linux/pci.h>
>>> +#include <linux/iommu.h>
>>> +#include <linux/debugfs.h>
>>
>>
>> Keep in order?

> What order would that be? These few needed files are listed in the same
> order as which they appear in amd_iommu.c. I'm gonna need a preference
> spelled out, please (and a rationale, so I may better understand).

To increase readability and avoid potential header duplication (here
is can bus protocol implementation where the problem exists for real,
even in new code!)

>>> +/* DebugFS helpers */
>>> +#define        OBUFP           (obuf + oboff)
>>> +#define        OBUFLEN         obuflen
>>> +#define        OBUFSPC         (OBUFLEN - oboff)
>>> +#define        OSCNPRINTF(fmt, ...) \
>>> +               scnprintf(OBUFP, OBUFSPC, fmt, ## __VA_ARGS__)
>>
>>
>> I don't see any advantages of this. Other way around, they will simple
>> makes things hard to read an understand in place.
>
>
> I used this technique in the CCP driver code (where it was accepted), in an
> effort to do the opposite of what you claim: make the code more readable.
> Given the 80 column limit, a large number of arguments, and very long
> statements, IMO something needs to give. I don't find the use of #defines to
> be obfuscating.
>
> I'm not trying to argue, but rather simply state the perspective / reasoning
> I used to create a source file I feel is manageable. I have 17 more iommu
> patches built upon this strategy, and this seems to be advantageous for all
> of them.

It's fine to me as long as it's fine to maintainer, but honestly
speaking I would avoid such code as much as possible. Imagine that
your "advantage" basically becomes disadvantage to everyone else who
is not familiar with the code.

Each time I see macro in the code, I would need to at least step on
it, run cscope, read, and come back. And at this point of time I
already forgot what this code is doing, it does use sNprintf() or
sCNprintf() or whatever wrapper on top of either...

>>> +       for (i = start ; i <= end ; i++)

>> Missed {}

> Wasn't sure about the M.O. given that the body of this loop is a single if
> statement. And I don't see anywhere in
> https://www.kernel.org/doc/html/latest/process/coding-style.html
> in section 3.1 where curly braces are called for in this situation. May I
> ask for clarification on the style rule, please?

You can do nothing, though I'm guided by the end of section 3.0
(though it tells only about 'if' case).

>>> @@ -89,6 +89,7 @@
>>>   #define ACPI_DEVFLAG_ATSDIS             0x10000000
>>>
>>>   #define LOOP_TIMEOUT   100000
>>> +
>>>   /*
>>>    * ACPI table definitions
>>>    *

>> Doesn't belong to the patch.

> I'm sorry, I don't understand. The added blank line doesn't belong to the
> patch?

Correct.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/5] iommu/amd - Add a debugfs entry to specify a IOMMU device table entry

[Andy Shevchenko]

On Tue, Mar 13, 2018 at 8:54 PM, Gary R Hook <gary.hook@amd.com> wrote:
> On 03/13/2018 12:20 PM, Andy Shevchenko wrote:

>>> +       } else if (obuf[0] == '0' && obuf[1] == 'x') {
>>> +               n = sscanf(obuf, "%x", &amd_iommu_devid);
>>> +       } else {
>>> +               n = sscanf(obuf, "%d", &amd_iommu_devid);
>>> +       }

>> kstrtoint() ?

> I see various mechanisms for this sort of thing, and simply chose one.
> Am happy to use whatever is preferred.

sscanf() has an enormous overhead for cases like this.

simple

ret = kstrtoint();
if (ret)
 ... do error handling ...


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/5] iommu/amd - Add a debugfs entry to specify a IOMMU device table entry

[Gary R Hook]

On 03/13/2018 03:56 PM, Andy Shevchenko wrote:
> On Tue, Mar 13, 2018 at 8:54 PM, Gary R Hook <gary.hook@amd.com> wrote:
>> On 03/13/2018 12:20 PM, Andy Shevchenko wrote:
> 
>>>> +       } else if (obuf[0] == '0' && obuf[1] == 'x') {
>>>> +               n = sscanf(obuf, "%x", &amd_iommu_devid);
>>>> +       } else {
>>>> +               n = sscanf(obuf, "%d", &amd_iommu_devid);
>>>> +       }
> 
>>> kstrtoint() ?
> 
>> I see various mechanisms for this sort of thing, and simply chose one.
>> Am happy to use whatever is preferred.
> 
> sscanf() has an enormous overhead for cases like this.
> 
> simple
> 
> ret = kstrtoint();
> if (ret)
>   ... do error handling ...
> 
> 

Gotcha. Fixed.

Thanks,
Gary

Re: [PATCH v2 1/5] iommu/amd - Add debugfs support

[Gary R Hook]

On 03/13/2018 03:23 PM, Andy Shevchenko wrote:
> On Tue, Mar 13, 2018 at 8:54 PM, Gary R Hook <gary.hook@amd.com> wrote:
>> On 03/13/2018 12:16 PM, Andy Shevchenko wrote:
>>> On Fri, Mar 9, 2018 at 2:50 AM, Gary R Hook <gary.hook@amd.com> wrote:
> 
>>>> +#include <linux/pci.h>
>>>> +#include <linux/iommu.h>
>>>> +#include <linux/debugfs.h>
>>>
>>>
>>> Keep in order?
> 
>> What order would that be? These few needed files are listed in the same
>> order as which they appear in amd_iommu.c. I'm gonna need a preference
>> spelled out, please (and a rationale, so I may better understand).
> 
> To increase readability and avoid potential header duplication (here
> is can bus protocol implementation where the problem exists for real,
> even in new code!)

With all due respect, I don't find that you clearly answered my 
question. I will hazard a guess that you mean to -alphabetize- them? 
Which I am happy to do, and will do so in the next version.

If that is not your meaning, I'll have to ask you to use small words, 
and not presume any understanding on my (or anyone's) part about 
preferences that are not documented in the style guide. I don't mean to 
be thick, but I have to ask for clarity.

Given that this is a preference, and that there are reasons for -not- 
doing so, I would also like to hear other comments on this suggestionn.


>>>> +       for (i = start ; i <= end ; i++)
> 
>>> Missed {}
> 
>> Wasn't sure about the M.O. given that the body of this loop is a single if
>> statement. And I don't see anywhere in
>> https://www.kernel.org/doc/html/latest/process/coding-style.html
>> in section 3.1 where curly braces are called for in this situation. May I
>> ask for clarification on the style rule, please?
> 
> You can do nothing, though I'm guided by the end of section 3.0
> (though it tells only about 'if' case).

Fixed this.

> 
>>>> @@ -89,6 +89,7 @@
>>>>    #define ACPI_DEVFLAG_ATSDIS             0x10000000
>>>>
>>>>    #define LOOP_TIMEOUT   100000
>>>> +
>>>>    /*
>>>>     * ACPI table definitions
>>>>     *
> 
>>> Doesn't belong to the patch.
> 
>> I'm sorry, I don't understand. The added blank line doesn't belong to the
>> patch?
> 
> Correct.
> 
Fixed this.

Thanks,
Gary

Re: [PATCH v2 1/5] iommu/amd - Add debugfs support

[Andy Shevchenko]

On Wed, Mar 14, 2018 at 5:24 PM, Gary R Hook <gary.hook@amd.com> wrote:
> On 03/13/2018 03:23 PM, Andy Shevchenko wrote:

>>>>> +#include <linux/pci.h>
>>>>> +#include <linux/iommu.h>
>>>>> +#include <linux/debugfs.h>

>>>> Keep in order?

>>> What order would that be? These few needed files are listed in the same
>>> order as which they appear in amd_iommu.c. I'm gonna need a preference
>>> spelled out, please (and a rationale, so I may better understand).

>> To increase readability and avoid potential header duplication (here
>> is can bus protocol implementation where the problem exists for real,
>> even in new code!)

> With all due respect, I don't find that you clearly answered my question.

Alphabetical order I meant.

> I
> will hazard a guess that you mean to -alphabetize- them? Which I am happy to
> do, and will do so in the next version.

Yes, please.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/5] iommu/amd - Add debugfs support

[Gary R Hook]

On 03/14/2018 10:29 AM, Andy Shevchenko wrote:
> On Wed, Mar 14, 2018 at 5:24 PM, Gary R Hook <gary.hook@amd.com> wrote:
>> On 03/13/2018 03:23 PM, Andy Shevchenko wrote:
> 
>>>>>> +#include <linux/pci.h>
>>>>>> +#include <linux/iommu.h>
>>>>>> +#include <linux/debugfs.h>
> 
>>>>> Keep in order?
> 
>>>> What order would that be? These few needed files are listed in the same
>>>> order as which they appear in amd_iommu.c. I'm gonna need a preference
>>>> spelled out, please (and a rationale, so I may better understand).
> 
>>> To increase readability and avoid potential header duplication (here
>>> is can bus protocol implementation where the problem exists for real,
>>> even in new code!)
> 
>> With all due respect, I don't find that you clearly answered my question.
> 
> Alphabetical order I meant.

Thank you very much.

Gary