All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] Base enablement of IOMMU debugfs support
@ 2018-03-29 22:54 ` Gary R Hook
  0 siblings, 0 replies; 14+ messages in thread
From: Gary R Hook @ 2018-03-29 22:54 UTC (permalink / raw)
  To: iommu; +Cc: joro, jacob.jun.pan, linux-kernel

These patches create a top-level function to create a debugfs directory
for the IOMMU, under which drivers may create and populate-specific
directories for their device internals.

Patch 1: general IOMMU enablement
Patch 2: basic AMD enablement to demonstrate linkage with patch 1

Introduce a new Kconfig parameter IOMMU_DEBUG to globally allow or
disallow debugfs code to be built.

---

Gary R Hook (2):
      iommu - Enable debugfs exposure of the IOMMU
      iommu/amd: Add basic debugfs infrastructure for AMD IOMMU


 drivers/iommu/Kconfig             |    9 +++++++
 drivers/iommu/Makefile            |    2 ++
 drivers/iommu/amd_iommu_debugfs.c |   47 +++++++++++++++++++++++++++++++++++++
 drivers/iommu/amd_iommu_init.c    |    7 ++++--
 drivers/iommu/amd_iommu_proto.h   |    6 +++++
 drivers/iommu/amd_iommu_types.h   |    3 ++
 drivers/iommu/iommu-debugfs.c     |   32 +++++++++++++++++++++++++
 include/linux/iommu.h             |    4 +++
 8 files changed, 108 insertions(+), 2 deletions(-)
 create mode 100644 drivers/iommu/amd_iommu_debugfs.c
 create mode 100644 drivers/iommu/iommu-debugfs.c

--

^ permalink raw reply	[flat|nested] 14+ messages in thread
* [PATCH 0/2] Base enablement of IOMMU debugfs support
@ 2018-03-29 22:54 ` Gary R Hook
  0 siblings, 0 replies; 14+ messages in thread
From: Gary R Hook @ 2018-03-29 22:54 UTC (permalink / raw)
  To: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA
  Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA

These patches create a top-level function to create a debugfs directory
for the IOMMU, under which drivers may create and populate-specific
directories for their device internals.

Patch 1: general IOMMU enablement
Patch 2: basic AMD enablement to demonstrate linkage with patch 1

Introduce a new Kconfig parameter IOMMU_DEBUG to globally allow or
disallow debugfs code to be built.

---

Gary R Hook (2):
      iommu - Enable debugfs exposure of the IOMMU
      iommu/amd: Add basic debugfs infrastructure for AMD IOMMU


 drivers/iommu/Kconfig             |    9 +++++++
 drivers/iommu/Makefile            |    2 ++
 drivers/iommu/amd_iommu_debugfs.c |   47 +++++++++++++++++++++++++++++++++++++
 drivers/iommu/amd_iommu_init.c    |    7 ++++--
 drivers/iommu/amd_iommu_proto.h   |    6 +++++
 drivers/iommu/amd_iommu_types.h   |    3 ++
 drivers/iommu/iommu-debugfs.c     |   32 +++++++++++++++++++++++++
 include/linux/iommu.h             |    4 +++
 8 files changed, 108 insertions(+), 2 deletions(-)
 create mode 100644 drivers/iommu/amd_iommu_debugfs.c
 create mode 100644 drivers/iommu/iommu-debugfs.c

--

^ permalink raw reply	[flat|nested] 14+ messages in thread
* [PATCH 1/2] iommu - Enable debugfs exposure of the IOMMU
  2018-03-29 22:54 ` Gary R Hook
@ 2018-03-29 22:54   ` Gary R Hook
  -1 siblings, 0 replies; 14+ messages in thread
From: Gary R Hook @ 2018-03-29 22:54 UTC (permalink / raw)
  To: iommu; +Cc: joro, jacob.jun.pan, linux-kernel

Provide base enablement for using debugfs to expose internal data of
an IOMMU driver. When enabled, create the /sys/kernel/debug/iommu
directory.  Emit a strong warning at boot time to indicate that this
feature is enabled.

Signed-off-by: Gary R Hook <gary.hook@amd.com>
---
 drivers/iommu/Kconfig          |   11 +++++++++++
 drivers/iommu/Makefile         |    2 ++
 drivers/iommu/amd_iommu_init.c |    2 ++
 drivers/iommu/iommu-debugfs.c  |   32 ++++++++++++++++++++++++++++++++
 include/linux/iommu.h          |    4 ++++
 5 files changed, 51 insertions(+)
 create mode 100644 drivers/iommu/iommu-debugfs.c

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index f3a21343e636..c1e39dabfec2 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -60,6 +60,17 @@ config IOMMU_IO_PGTABLE_ARMV7S_SELFTEST
 
 endmenu
 
+config IOMMU_DEBUG
+	bool "Enable IOMMU internals in DebugFS"
+	depends on DEBUG_FS
+	default n
+	help
+	  Allows exposure of IOMMU device internals. This option enables
+	  the use of debugfs by IOMMU drivers as required. Devices can,
+	  at initialization time, cause the IOMMU code to create a top-level
+	  debug/iommu directory, and then populate a subdirectory with
+	  entries as required.
+
 config IOMMU_IOVA
 	tristate
 
diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
index 1fb695854809..5e5c3339681d 100644
--- a/drivers/iommu/Makefile
+++ b/drivers/iommu/Makefile
@@ -2,6 +2,7 @@
 obj-$(CONFIG_IOMMU_API) += iommu.o
 obj-$(CONFIG_IOMMU_API) += iommu-traces.o
 obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o
+obj-$(CONFIG_IOMMU_DEBUG) += iommu-debugfs.o
 obj-$(CONFIG_IOMMU_DMA) += dma-iommu.o
 obj-$(CONFIG_IOMMU_IO_PGTABLE) += io-pgtable.o
 obj-$(CONFIG_IOMMU_IO_PGTABLE_ARMV7S) += io-pgtable-arm-v7s.o
@@ -10,6 +11,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_init.c b/drivers/iommu/amd_iommu_init.c
index 6fe2d0346073..99d48c42a12f 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -2783,6 +2783,8 @@ int __init amd_iommu_detect(void)
 	iommu_detected = 1;
 	x86_init.iommu.iommu_init = amd_iommu_init;
 
+dump_stack();
+
 	return 1;
 }
 
diff --git a/drivers/iommu/iommu-debugfs.c b/drivers/iommu/iommu-debugfs.c
new file mode 100644
index 000000000000..94c9acc63b65
--- /dev/null
+++ b/drivers/iommu/iommu-debugfs.c
@@ -0,0 +1,32 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * AMD IOMMU driver
+ *
+ * Copyright (C) 2018 Advanced Micro Devices, Inc.
+ *
+ * Author: Gary R Hook <gary.hook@amd.com>
+ */
+
+#include <linux/pci.h>
+#include <linux/iommu.h>
+#include <linux/debugfs.h>
+
+static struct dentry *iommu_debugfs_dir;
+
+#define	MAX_NAME_LEN	20
+
+/* Return a zero on failure; 1 on successful setup */
+struct dentry *iommu_debugfs_setup(void)
+{
+	if (!debugfs_initialized())
+		return NULL;
+
+	if (!iommu_debugfs_dir)
+		iommu_debugfs_dir = debugfs_create_dir("iommu", NULL);
+
+	if (iommu_debugfs_dir)
+		pr_warn("WARNING: IOMMU DEBUGFS SUPPORT HAS BEEN ENABLED IN THIS KERNEL\n");
+
+	return iommu_debugfs_dir;
+}
+EXPORT_SYMBOL_GPL(iommu_debugfs_setup);
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 41b8c5757859..cb2957dac43b 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -696,6 +696,10 @@ const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
 	return NULL;
 }
 
+#ifdef	CONFIG_IOMMU_DEBUG
+struct dentry *iommu_debugfs_setup(void);
+#endif
+
 #endif /* CONFIG_IOMMU_API */
 
 #endif /* __LINUX_IOMMU_H */

^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH 1/2] iommu - Enable debugfs exposure of the IOMMU
@ 2018-03-29 22:54   ` Gary R Hook
  0 siblings, 0 replies; 14+ messages in thread
From: Gary R Hook @ 2018-03-29 22:54 UTC (permalink / raw)
  To: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA
  Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA

Provide base enablement for using debugfs to expose internal data of
an IOMMU driver. When enabled, create the /sys/kernel/debug/iommu
directory.  Emit a strong warning at boot time to indicate that this
feature is enabled.

Signed-off-by: Gary R Hook <gary.hook-5C7GfCeVMHo@public.gmane.org>
---
 drivers/iommu/Kconfig          |   11 +++++++++++
 drivers/iommu/Makefile         |    2 ++
 drivers/iommu/amd_iommu_init.c |    2 ++
 drivers/iommu/iommu-debugfs.c  |   32 ++++++++++++++++++++++++++++++++
 include/linux/iommu.h          |    4 ++++
 5 files changed, 51 insertions(+)
 create mode 100644 drivers/iommu/iommu-debugfs.c

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index f3a21343e636..c1e39dabfec2 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -60,6 +60,17 @@ config IOMMU_IO_PGTABLE_ARMV7S_SELFTEST
 
 endmenu
 
+config IOMMU_DEBUG
+	bool "Enable IOMMU internals in DebugFS"
+	depends on DEBUG_FS
+	default n
+	help
+	  Allows exposure of IOMMU device internals. This option enables
+	  the use of debugfs by IOMMU drivers as required. Devices can,
+	  at initialization time, cause the IOMMU code to create a top-level
+	  debug/iommu directory, and then populate a subdirectory with
+	  entries as required.
+
 config IOMMU_IOVA
 	tristate
 
diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
index 1fb695854809..5e5c3339681d 100644
--- a/drivers/iommu/Makefile
+++ b/drivers/iommu/Makefile
@@ -2,6 +2,7 @@
 obj-$(CONFIG_IOMMU_API) += iommu.o
 obj-$(CONFIG_IOMMU_API) += iommu-traces.o
 obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o
+obj-$(CONFIG_IOMMU_DEBUG) += iommu-debugfs.o
 obj-$(CONFIG_IOMMU_DMA) += dma-iommu.o
 obj-$(CONFIG_IOMMU_IO_PGTABLE) += io-pgtable.o
 obj-$(CONFIG_IOMMU_IO_PGTABLE_ARMV7S) += io-pgtable-arm-v7s.o
@@ -10,6 +11,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_init.c b/drivers/iommu/amd_iommu_init.c
index 6fe2d0346073..99d48c42a12f 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -2783,6 +2783,8 @@ int __init amd_iommu_detect(void)
 	iommu_detected = 1;
 	x86_init.iommu.iommu_init = amd_iommu_init;
 
+dump_stack();
+
 	return 1;
 }
 
diff --git a/drivers/iommu/iommu-debugfs.c b/drivers/iommu/iommu-debugfs.c
new file mode 100644
index 000000000000..94c9acc63b65
--- /dev/null
+++ b/drivers/iommu/iommu-debugfs.c
@@ -0,0 +1,32 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * AMD IOMMU driver
+ *
+ * Copyright (C) 2018 Advanced Micro Devices, Inc.
+ *
+ * Author: Gary R Hook <gary.hook-5C7GfCeVMHo@public.gmane.org>
+ */
+
+#include <linux/pci.h>
+#include <linux/iommu.h>
+#include <linux/debugfs.h>
+
+static struct dentry *iommu_debugfs_dir;
+
+#define	MAX_NAME_LEN	20
+
+/* Return a zero on failure; 1 on successful setup */
+struct dentry *iommu_debugfs_setup(void)
+{
+	if (!debugfs_initialized())
+		return NULL;
+
+	if (!iommu_debugfs_dir)
+		iommu_debugfs_dir = debugfs_create_dir("iommu", NULL);
+
+	if (iommu_debugfs_dir)
+		pr_warn("WARNING: IOMMU DEBUGFS SUPPORT HAS BEEN ENABLED IN THIS KERNEL\n");
+
+	return iommu_debugfs_dir;
+}
+EXPORT_SYMBOL_GPL(iommu_debugfs_setup);
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 41b8c5757859..cb2957dac43b 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -696,6 +696,10 @@ const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
 	return NULL;
 }
 
+#ifdef	CONFIG_IOMMU_DEBUG
+struct dentry *iommu_debugfs_setup(void);
+#endif
+
 #endif /* CONFIG_IOMMU_API */
 
 #endif /* __LINUX_IOMMU_H */

^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU
  2018-03-29 22:54 ` Gary R Hook
@ 2018-03-29 22:54   ` Gary R Hook
  -1 siblings, 0 replies; 14+ messages in thread
From: Gary R Hook @ 2018-03-29 22:54 UTC (permalink / raw)
  To: iommu; +Cc: joro, jacob.jun.pan, linux-kernel

Implement a skeleton framework for debugfs support in the
AMD IOMMU.


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

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index d40248446214..8d50151d5bf4 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -65,9 +65,9 @@ config IOMMU_DEBUG
 	depends on DEBUG_FS
 	default n
 	help
-	  Base enablement for access to any IOMMU device. Allows individual
-	  drivers to populate debugfs for access to IOMMU registers and
-	  data structures.
+	  Enable exposure of IOMMU device internals. Allow devices to
+	  populate debugfs for access to IOMMU registers and data
+	  structures.
 
 config IOMMU_IOVA
 	tristate
diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
index 5e5c3339681d..0ca250f626d9 100644
--- a/drivers/iommu/Makefile
+++ b/drivers/iommu/Makefile
@@ -11,7 +11,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_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..3547ad3339c1
--- /dev/null
+++ b/drivers/iommu/amd_iommu_debugfs.c
@@ -0,0 +1,47 @@
+// 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_IOMMU_DEBUG
+
+#include <linux/debugfs.h>
+#include <linux/iommu.h>
+#include <linux/pci.h>
+#include "amd_iommu_proto.h"
+#include "amd_iommu_types.h"
+
+static struct dentry *iommu_debugfs_dir;
+static DEFINE_RWLOCK(iommu_debugfs_lock);
+
+#define	MAX_NAME_LEN	20
+
+void amd_iommu_debugfs_setup(struct amd_iommu *iommu)
+{
+	char name[MAX_NAME_LEN + 1];
+	struct dentry *d_top;
+	unsigned long flags;
+
+	if (!debugfs_initialized())
+		return;
+
+	write_lock_irqsave(&iommu_debugfs_lock, flags);
+	if (!iommu_debugfs_dir && (d_top = iommu_debugfs_setup()))
+		iommu_debugfs_dir = debugfs_create_dir("amd", d_top);
+	write_unlock_irqrestore(&iommu_debugfs_lock, flags);
+	if (iommu_debugfs_dir) {
+		snprintf(name, MAX_NAME_LEN, "iommu%02d", iommu->index);
+		iommu->debugfs_instance = debugfs_create_dir(name,
+							     iommu_debugfs_dir);
+		if (!iommu->debugfs_instance)
+			debugfs_remove_recursive(iommu_debugfs_dir);
+	}
+
+	return;
+}
+
+#endif
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 99d48c42a12f..43856c7f4ea1 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
  *
@@ -2720,6 +2721,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);
@@ -2729,14 +2731,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;
 }
 
@@ -2783,8 +2786,6 @@ int __init amd_iommu_detect(void)
 	iommu_detected = 1;
 	x86_init.iommu.iommu_init = amd_iommu_init;
 
-dump_stack();
-
 	return 1;
 }
 
diff --git a/drivers/iommu/amd_iommu_proto.h b/drivers/iommu/amd_iommu_proto.h
index 640c286a0ab9..e19cebc5c740 100644
--- a/drivers/iommu/amd_iommu_proto.h
+++ b/drivers/iommu/amd_iommu_proto.h
@@ -33,6 +33,12 @@ extern void amd_iommu_uninit_devices(void);
 extern void amd_iommu_init_notifier(void);
 extern int amd_iommu_init_api(void);
 
+#ifdef	CONFIG_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 f6b24c7d8b70..6dca9fe38518 100644
--- a/drivers/iommu/amd_iommu_types.h
+++ b/drivers/iommu/amd_iommu_types.h
@@ -591,6 +591,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)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 98527f9b473b..dbfff811aa25 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -412,6 +412,10 @@ void iommu_fwspec_free(struct device *dev);
 int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
 const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode);
 
+#ifdef	CONFIG_IOMMU_DEBUG
+extern struct dentry *iommu_debugfs_setup(void);
+#endif
+
 #else /* CONFIG_IOMMU_API */
 
 struct iommu_ops {};
@@ -696,10 +700,6 @@ const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
 	return NULL;
 }
 
-#ifdef	CONFIG_IOMMU_DEBUG
-extern struct dentry *iommu_debugfs_setup(void);
-#endif
-
 #endif /* CONFIG_IOMMU_API */
 
 #endif /* __LINUX_IOMMU_H */

^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU
@ 2018-03-29 22:54   ` Gary R Hook
  0 siblings, 0 replies; 14+ messages in thread
From: Gary R Hook @ 2018-03-29 22:54 UTC (permalink / raw)
  To: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA
  Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA

Implement a skeleton framework for debugfs support in the
AMD IOMMU.


Signed-off-by: Gary R Hook <gary.hook-5C7GfCeVMHo@public.gmane.org>
---
 drivers/iommu/Kconfig             |    6 ++---
 drivers/iommu/Makefile            |    2 +-
 drivers/iommu/amd_iommu_debugfs.c |   47 +++++++++++++++++++++++++++++++++++++
 drivers/iommu/amd_iommu_init.c    |    9 ++++---
 drivers/iommu/amd_iommu_proto.h   |    6 +++++
 drivers/iommu/amd_iommu_types.h   |    3 ++
 include/linux/iommu.h             |    8 +++---
 7 files changed, 69 insertions(+), 12 deletions(-)
 create mode 100644 drivers/iommu/amd_iommu_debugfs.c

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index d40248446214..8d50151d5bf4 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -65,9 +65,9 @@ config IOMMU_DEBUG
 	depends on DEBUG_FS
 	default n
 	help
-	  Base enablement for access to any IOMMU device. Allows individual
-	  drivers to populate debugfs for access to IOMMU registers and
-	  data structures.
+	  Enable exposure of IOMMU device internals. Allow devices to
+	  populate debugfs for access to IOMMU registers and data
+	  structures.
 
 config IOMMU_IOVA
 	tristate
diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
index 5e5c3339681d..0ca250f626d9 100644
--- a/drivers/iommu/Makefile
+++ b/drivers/iommu/Makefile
@@ -11,7 +11,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_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..3547ad3339c1
--- /dev/null
+++ b/drivers/iommu/amd_iommu_debugfs.c
@@ -0,0 +1,47 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * AMD IOMMU driver
+ *
+ * Copyright (C) 2018 Advanced Micro Devices, Inc.
+ *
+ * Author: Gary R Hook <gary.hook-5C7GfCeVMHo@public.gmane.org>
+ */
+
+#ifdef	CONFIG_IOMMU_DEBUG
+
+#include <linux/debugfs.h>
+#include <linux/iommu.h>
+#include <linux/pci.h>
+#include "amd_iommu_proto.h"
+#include "amd_iommu_types.h"
+
+static struct dentry *iommu_debugfs_dir;
+static DEFINE_RWLOCK(iommu_debugfs_lock);
+
+#define	MAX_NAME_LEN	20
+
+void amd_iommu_debugfs_setup(struct amd_iommu *iommu)
+{
+	char name[MAX_NAME_LEN + 1];
+	struct dentry *d_top;
+	unsigned long flags;
+
+	if (!debugfs_initialized())
+		return;
+
+	write_lock_irqsave(&iommu_debugfs_lock, flags);
+	if (!iommu_debugfs_dir && (d_top = iommu_debugfs_setup()))
+		iommu_debugfs_dir = debugfs_create_dir("amd", d_top);
+	write_unlock_irqrestore(&iommu_debugfs_lock, flags);
+	if (iommu_debugfs_dir) {
+		snprintf(name, MAX_NAME_LEN, "iommu%02d", iommu->index);
+		iommu->debugfs_instance = debugfs_create_dir(name,
+							     iommu_debugfs_dir);
+		if (!iommu->debugfs_instance)
+			debugfs_remove_recursive(iommu_debugfs_dir);
+	}
+
+	return;
+}
+
+#endif
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 99d48c42a12f..43856c7f4ea1 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
  *
@@ -2720,6 +2721,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);
@@ -2729,14 +2731,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;
 }
 
@@ -2783,8 +2786,6 @@ int __init amd_iommu_detect(void)
 	iommu_detected = 1;
 	x86_init.iommu.iommu_init = amd_iommu_init;
 
-dump_stack();
-
 	return 1;
 }
 
diff --git a/drivers/iommu/amd_iommu_proto.h b/drivers/iommu/amd_iommu_proto.h
index 640c286a0ab9..e19cebc5c740 100644
--- a/drivers/iommu/amd_iommu_proto.h
+++ b/drivers/iommu/amd_iommu_proto.h
@@ -33,6 +33,12 @@ extern void amd_iommu_uninit_devices(void);
 extern void amd_iommu_init_notifier(void);
 extern int amd_iommu_init_api(void);
 
+#ifdef	CONFIG_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 f6b24c7d8b70..6dca9fe38518 100644
--- a/drivers/iommu/amd_iommu_types.h
+++ b/drivers/iommu/amd_iommu_types.h
@@ -591,6 +591,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)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 98527f9b473b..dbfff811aa25 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -412,6 +412,10 @@ void iommu_fwspec_free(struct device *dev);
 int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
 const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode);
 
+#ifdef	CONFIG_IOMMU_DEBUG
+extern struct dentry *iommu_debugfs_setup(void);
+#endif
+
 #else /* CONFIG_IOMMU_API */
 
 struct iommu_ops {};
@@ -696,10 +700,6 @@ const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
 	return NULL;
 }
 
-#ifdef	CONFIG_IOMMU_DEBUG
-extern struct dentry *iommu_debugfs_setup(void);
-#endif
-
 #endif /* CONFIG_IOMMU_API */
 
 #endif /* __LINUX_IOMMU_H */

^ permalink raw reply related	[flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] iommu - Enable debugfs exposure of the IOMMU
  2018-03-29 22:54   ` Gary R Hook
@ 2018-03-30  3:57     ` Tom Lendacky
  -1 siblings, 0 replies; 14+ messages in thread
From: Tom Lendacky @ 2018-03-30  3:57 UTC (permalink / raw)
  To: Gary R Hook, iommu; +Cc: joro, jacob.jun.pan, linux-kernel

On 3/29/2018 5:54 PM, Gary R Hook wrote:
> Provide base enablement for using debugfs to expose internal data of
> an IOMMU driver. When enabled, create the /sys/kernel/debug/iommu

So this can't actually create anything yet since nothing invokes the
function.  Maybe describe how it should be used by other drivers (and
probably include that description as a commment for the function) to
create the directory.

> directory.  Emit a strong warning at boot time to indicate that this
> feature is enabled.
> 
> Signed-off-by: Gary R Hook <gary.hook@amd.com>
> ---
>  drivers/iommu/Kconfig          |   11 +++++++++++
>  drivers/iommu/Makefile         |    2 ++
>  drivers/iommu/amd_iommu_init.c |    2 ++
>  drivers/iommu/iommu-debugfs.c  |   32 ++++++++++++++++++++++++++++++++
>  include/linux/iommu.h          |    4 ++++
>  5 files changed, 51 insertions(+)
>  create mode 100644 drivers/iommu/iommu-debugfs.c
> 
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index f3a21343e636..c1e39dabfec2 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -60,6 +60,17 @@ config IOMMU_IO_PGTABLE_ARMV7S_SELFTEST
>  
>  endmenu
>  
> +config IOMMU_DEBUG
> +	bool "Enable IOMMU internals in DebugFS"
> +	depends on DEBUG_FS
> +	default n
> +	help
> +	  Allows exposure of IOMMU device internals. This option enables
> +	  the use of debugfs by IOMMU drivers as required. Devices can,
> +	  at initialization time, cause the IOMMU code to create a top-level
> +	  debug/iommu directory, and then populate a subdirectory with
> +	  entries as required.
> +
>  config IOMMU_IOVA
>  	tristate
>  
> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
> index 1fb695854809..5e5c3339681d 100644
> --- a/drivers/iommu/Makefile
> +++ b/drivers/iommu/Makefile
> @@ -2,6 +2,7 @@
>  obj-$(CONFIG_IOMMU_API) += iommu.o
>  obj-$(CONFIG_IOMMU_API) += iommu-traces.o
>  obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o
> +obj-$(CONFIG_IOMMU_DEBUG) += iommu-debugfs.o
>  obj-$(CONFIG_IOMMU_DMA) += dma-iommu.o
>  obj-$(CONFIG_IOMMU_IO_PGTABLE) += io-pgtable.o
>  obj-$(CONFIG_IOMMU_IO_PGTABLE_ARMV7S) += io-pgtable-arm-v7s.o
> @@ -10,6 +11,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

Where is this CONFIG defined?

>  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_init.c b/drivers/iommu/amd_iommu_init.c
> index 6fe2d0346073..99d48c42a12f 100644
> --- a/drivers/iommu/amd_iommu_init.c
> +++ b/drivers/iommu/amd_iommu_init.c
> @@ -2783,6 +2783,8 @@ int __init amd_iommu_detect(void)
>  	iommu_detected = 1;
>  	x86_init.iommu.iommu_init = amd_iommu_init;
>  
> +dump_stack();
> +

This definitely shouldn't be here.

>  	return 1;
>  }
>  
> diff --git a/drivers/iommu/iommu-debugfs.c b/drivers/iommu/iommu-debugfs.c
> new file mode 100644
> index 000000000000..94c9acc63b65
> --- /dev/null
> +++ b/drivers/iommu/iommu-debugfs.c
> @@ -0,0 +1,32 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AMD IOMMU driver

This isn't the AMD IOMMU driver.

> + *
> + * Copyright (C) 2018 Advanced Micro Devices, Inc.
> + *
> + * Author: Gary R Hook <gary.hook@amd.com>
> + */
> +
> +#include <linux/pci.h>
> +#include <linux/iommu.h>
> +#include <linux/debugfs.h>
> +
> +static struct dentry *iommu_debugfs_dir;
> +
> +#define	MAX_NAME_LEN	20

This isn't used anywhere.

> +
> +/* Return a zero on failure; 1 on successful setup */

Return NULL on failure, pointer to IOMMU debugfs dentry on success.

> +struct dentry *iommu_debugfs_setup(void)
> +{
> +	if (!debugfs_initialized())
> +		return NULL;
> +
> +	if (!iommu_debugfs_dir)
> +		iommu_debugfs_dir = debugfs_create_dir("iommu", NULL);
> +
> +	if (iommu_debugfs_dir)
> +		pr_warn("WARNING: IOMMU DEBUGFS SUPPORT HAS BEEN ENABLED IN THIS KERNEL\n");
> +
> +	return iommu_debugfs_dir;
> +}
> +EXPORT_SYMBOL_GPL(iommu_debugfs_setup);
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 41b8c5757859..cb2957dac43b 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -696,6 +696,10 @@ const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
>  	return NULL;
>  }
>  
> +#ifdef	CONFIG_IOMMU_DEBUG

Should be a space between the #ifdef and the CONFIG_..., not a tab.

Thanks,
Tom

> +struct dentry *iommu_debugfs_setup(void);
> +#endif
> +
>  #endif /* CONFIG_IOMMU_API */
>  
>  #endif /* __LINUX_IOMMU_H */
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] iommu - Enable debugfs exposure of the IOMMU
@ 2018-03-30  3:57     ` Tom Lendacky
  0 siblings, 0 replies; 14+ messages in thread
From: Tom Lendacky @ 2018-03-30  3:57 UTC (permalink / raw)
  To: Gary R Hook, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA
  Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA

On 3/29/2018 5:54 PM, Gary R Hook wrote:
> Provide base enablement for using debugfs to expose internal data of
> an IOMMU driver. When enabled, create the /sys/kernel/debug/iommu

So this can't actually create anything yet since nothing invokes the
function.  Maybe describe how it should be used by other drivers (and
probably include that description as a commment for the function) to
create the directory.

> directory.  Emit a strong warning at boot time to indicate that this
> feature is enabled.
> 
> Signed-off-by: Gary R Hook <gary.hook-5C7GfCeVMHo@public.gmane.org>
> ---
>  drivers/iommu/Kconfig          |   11 +++++++++++
>  drivers/iommu/Makefile         |    2 ++
>  drivers/iommu/amd_iommu_init.c |    2 ++
>  drivers/iommu/iommu-debugfs.c  |   32 ++++++++++++++++++++++++++++++++
>  include/linux/iommu.h          |    4 ++++
>  5 files changed, 51 insertions(+)
>  create mode 100644 drivers/iommu/iommu-debugfs.c
> 
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index f3a21343e636..c1e39dabfec2 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -60,6 +60,17 @@ config IOMMU_IO_PGTABLE_ARMV7S_SELFTEST
>  
>  endmenu
>  
> +config IOMMU_DEBUG
> +	bool "Enable IOMMU internals in DebugFS"
> +	depends on DEBUG_FS
> +	default n
> +	help
> +	  Allows exposure of IOMMU device internals. This option enables
> +	  the use of debugfs by IOMMU drivers as required. Devices can,
> +	  at initialization time, cause the IOMMU code to create a top-level
> +	  debug/iommu directory, and then populate a subdirectory with
> +	  entries as required.
> +
>  config IOMMU_IOVA
>  	tristate
>  
> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
> index 1fb695854809..5e5c3339681d 100644
> --- a/drivers/iommu/Makefile
> +++ b/drivers/iommu/Makefile
> @@ -2,6 +2,7 @@
>  obj-$(CONFIG_IOMMU_API) += iommu.o
>  obj-$(CONFIG_IOMMU_API) += iommu-traces.o
>  obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o
> +obj-$(CONFIG_IOMMU_DEBUG) += iommu-debugfs.o
>  obj-$(CONFIG_IOMMU_DMA) += dma-iommu.o
>  obj-$(CONFIG_IOMMU_IO_PGTABLE) += io-pgtable.o
>  obj-$(CONFIG_IOMMU_IO_PGTABLE_ARMV7S) += io-pgtable-arm-v7s.o
> @@ -10,6 +11,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

Where is this CONFIG defined?

>  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_init.c b/drivers/iommu/amd_iommu_init.c
> index 6fe2d0346073..99d48c42a12f 100644
> --- a/drivers/iommu/amd_iommu_init.c
> +++ b/drivers/iommu/amd_iommu_init.c
> @@ -2783,6 +2783,8 @@ int __init amd_iommu_detect(void)
>  	iommu_detected = 1;
>  	x86_init.iommu.iommu_init = amd_iommu_init;
>  
> +dump_stack();
> +

This definitely shouldn't be here.

>  	return 1;
>  }
>  
> diff --git a/drivers/iommu/iommu-debugfs.c b/drivers/iommu/iommu-debugfs.c
> new file mode 100644
> index 000000000000..94c9acc63b65
> --- /dev/null
> +++ b/drivers/iommu/iommu-debugfs.c
> @@ -0,0 +1,32 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AMD IOMMU driver

This isn't the AMD IOMMU driver.

> + *
> + * Copyright (C) 2018 Advanced Micro Devices, Inc.
> + *
> + * Author: Gary R Hook <gary.hook-5C7GfCeVMHo@public.gmane.org>
> + */
> +
> +#include <linux/pci.h>
> +#include <linux/iommu.h>
> +#include <linux/debugfs.h>
> +
> +static struct dentry *iommu_debugfs_dir;
> +
> +#define	MAX_NAME_LEN	20

This isn't used anywhere.

> +
> +/* Return a zero on failure; 1 on successful setup */

Return NULL on failure, pointer to IOMMU debugfs dentry on success.

> +struct dentry *iommu_debugfs_setup(void)
> +{
> +	if (!debugfs_initialized())
> +		return NULL;
> +
> +	if (!iommu_debugfs_dir)
> +		iommu_debugfs_dir = debugfs_create_dir("iommu", NULL);
> +
> +	if (iommu_debugfs_dir)
> +		pr_warn("WARNING: IOMMU DEBUGFS SUPPORT HAS BEEN ENABLED IN THIS KERNEL\n");
> +
> +	return iommu_debugfs_dir;
> +}
> +EXPORT_SYMBOL_GPL(iommu_debugfs_setup);
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 41b8c5757859..cb2957dac43b 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -696,6 +696,10 @@ const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
>  	return NULL;
>  }
>  
> +#ifdef	CONFIG_IOMMU_DEBUG

Should be a space between the #ifdef and the CONFIG_..., not a tab.

Thanks,
Tom

> +struct dentry *iommu_debugfs_setup(void);
> +#endif
> +
>  #endif /* CONFIG_IOMMU_API */
>  
>  #endif /* __LINUX_IOMMU_H */
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU
  2018-03-29 22:54   ` Gary R Hook
@ 2018-03-30  4:16     ` Tom Lendacky
  -1 siblings, 0 replies; 14+ messages in thread
From: Tom Lendacky @ 2018-03-30  4:16 UTC (permalink / raw)
  To: Gary R Hook, iommu; +Cc: joro, jacob.jun.pan, linux-kernel

On 3/29/2018 5:54 PM, Gary R Hook wrote:
> Implement a skeleton framework for debugfs support in the
> AMD IOMMU.
> 
> 
> Signed-off-by: Gary R Hook <gary.hook@amd.com>
> ---
>  drivers/iommu/Kconfig             |    6 ++---
>  drivers/iommu/Makefile            |    2 +-
>  drivers/iommu/amd_iommu_debugfs.c |   47 +++++++++++++++++++++++++++++++++++++
>  drivers/iommu/amd_iommu_init.c    |    9 ++++---
>  drivers/iommu/amd_iommu_proto.h   |    6 +++++
>  drivers/iommu/amd_iommu_types.h   |    3 ++
>  include/linux/iommu.h             |    8 +++---
>  7 files changed, 69 insertions(+), 12 deletions(-)
>  create mode 100644 drivers/iommu/amd_iommu_debugfs.c
> 
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index d40248446214..8d50151d5bf4 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -65,9 +65,9 @@ config IOMMU_DEBUG
>  	depends on DEBUG_FS
>  	default n
>  	help
> -	  Base enablement for access to any IOMMU device. Allows individual
> -	  drivers to populate debugfs for access to IOMMU registers and
> -	  data structures.
> +	  Enable exposure of IOMMU device internals. Allow devices to
> +	  populate debugfs for access to IOMMU registers and data
> +	  structures.

This help text shouldn't change just because a driver is making use of
the interface that was created in the previous patch.

>  
>  config IOMMU_IOVA
>  	tristate
> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
> index 5e5c3339681d..0ca250f626d9 100644
> --- a/drivers/iommu/Makefile
> +++ b/drivers/iommu/Makefile
> @@ -11,7 +11,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_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..3547ad3339c1
> --- /dev/null
> +++ b/drivers/iommu/amd_iommu_debugfs.c
> @@ -0,0 +1,47 @@
> +// 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_IOMMU_DEBUG

Since the module won't be built unless this is defined, you don't
need this.

> +
> +#include <linux/debugfs.h>
> +#include <linux/iommu.h>
> +#include <linux/pci.h>
> +#include "amd_iommu_proto.h"
> +#include "amd_iommu_types.h"
> +
> +static struct dentry *iommu_debugfs_dir;
> +static DEFINE_RWLOCK(iommu_debugfs_lock);

Use amd_iommu_...

Also, didn't you run into an issue with the CCP and debugfs creation
during probe using a RWLOCK?  Should probably make this a mutex.

> +
> +#define	MAX_NAME_LEN	20
> +
> +void amd_iommu_debugfs_setup(struct amd_iommu *iommu)
> +{
> +	char name[MAX_NAME_LEN + 1];
> +	struct dentry *d_top;
> +	unsigned long flags;
> +
> +	if (!debugfs_initialized())
> +		return;
> +
> +	write_lock_irqsave(&iommu_debugfs_lock, flags);
> +	if (!iommu_debugfs_dir && (d_top = iommu_debugfs_setup()))
> +		iommu_debugfs_dir = debugfs_create_dir("amd", d_top);
> +	write_unlock_irqrestore(&iommu_debugfs_lock, flags);
> +	if (iommu_debugfs_dir) {
> +		snprintf(name, MAX_NAME_LEN, "iommu%02d", iommu->index);
> +		iommu->debugfs_instance = debugfs_create_dir(name,
> +							     iommu_debugfs_dir);
> +		if (!iommu->debugfs_instance)
> +			debugfs_remove_recursive(iommu_debugfs_dir);

So this might cause an error.  You remove it, but don't set the variable
to NULL, so the next IOMMU that is probed could try to use it.  But why
are you deleting it anyway?

> +	}
> +
> +	return;
> +}
> +
> +#endif
> diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
> index 99d48c42a12f..43856c7f4ea1 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
> +

Spurious newline.

>  /*
>   * ACPI table definitions
>   *
> @@ -2720,6 +2721,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);
> @@ -2729,14 +2731,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;
>  }
>  
> @@ -2783,8 +2786,6 @@ int __init amd_iommu_detect(void)
>  	iommu_detected = 1;
>  	x86_init.iommu.iommu_init = amd_iommu_init;
>  
> -dump_stack();
> -
>  	return 1;
>  }
>  
> diff --git a/drivers/iommu/amd_iommu_proto.h b/drivers/iommu/amd_iommu_proto.h
> index 640c286a0ab9..e19cebc5c740 100644
> --- a/drivers/iommu/amd_iommu_proto.h
> +++ b/drivers/iommu/amd_iommu_proto.h
> @@ -33,6 +33,12 @@ extern void amd_iommu_uninit_devices(void);
>  extern void amd_iommu_init_notifier(void);
>  extern int amd_iommu_init_api(void);
>  
> +#ifdef	CONFIG_IOMMU_DEBUG

Use space instead of tab.

> +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 f6b24c7d8b70..6dca9fe38518 100644
> --- a/drivers/iommu/amd_iommu_types.h
> +++ b/drivers/iommu/amd_iommu_types.h
> @@ -591,6 +591,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)
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 98527f9b473b..dbfff811aa25 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -412,6 +412,10 @@ void iommu_fwspec_free(struct device *dev);
>  int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
>  const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode);
>  
> +#ifdef	CONFIG_IOMMU_DEBUG
> +extern struct dentry *iommu_debugfs_setup(void);
> +#endif
> +

Any reason why this moved?  If it was not in the right place in the first
patch, that's where it should be fixed.

Thanks,
Tom

>  #else /* CONFIG_IOMMU_API */
>  
>  struct iommu_ops {};
> @@ -696,10 +700,6 @@ const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
>  	return NULL;
>  }
>  
> -#ifdef	CONFIG_IOMMU_DEBUG
> -extern struct dentry *iommu_debugfs_setup(void);
> -#endif
> -
>  #endif /* CONFIG_IOMMU_API */
>  
>  #endif /* __LINUX_IOMMU_H */
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU
@ 2018-03-30  4:16     ` Tom Lendacky
  0 siblings, 0 replies; 14+ messages in thread
From: Tom Lendacky @ 2018-03-30  4:16 UTC (permalink / raw)
  To: Gary R Hook, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA
  Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA

On 3/29/2018 5:54 PM, Gary R Hook wrote:
> Implement a skeleton framework for debugfs support in the
> AMD IOMMU.
> 
> 
> Signed-off-by: Gary R Hook <gary.hook-5C7GfCeVMHo@public.gmane.org>
> ---
>  drivers/iommu/Kconfig             |    6 ++---
>  drivers/iommu/Makefile            |    2 +-
>  drivers/iommu/amd_iommu_debugfs.c |   47 +++++++++++++++++++++++++++++++++++++
>  drivers/iommu/amd_iommu_init.c    |    9 ++++---
>  drivers/iommu/amd_iommu_proto.h   |    6 +++++
>  drivers/iommu/amd_iommu_types.h   |    3 ++
>  include/linux/iommu.h             |    8 +++---
>  7 files changed, 69 insertions(+), 12 deletions(-)
>  create mode 100644 drivers/iommu/amd_iommu_debugfs.c
> 
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index d40248446214..8d50151d5bf4 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -65,9 +65,9 @@ config IOMMU_DEBUG
>  	depends on DEBUG_FS
>  	default n
>  	help
> -	  Base enablement for access to any IOMMU device. Allows individual
> -	  drivers to populate debugfs for access to IOMMU registers and
> -	  data structures.
> +	  Enable exposure of IOMMU device internals. Allow devices to
> +	  populate debugfs for access to IOMMU registers and data
> +	  structures.

This help text shouldn't change just because a driver is making use of
the interface that was created in the previous patch.

>  
>  config IOMMU_IOVA
>  	tristate
> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
> index 5e5c3339681d..0ca250f626d9 100644
> --- a/drivers/iommu/Makefile
> +++ b/drivers/iommu/Makefile
> @@ -11,7 +11,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_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..3547ad3339c1
> --- /dev/null
> +++ b/drivers/iommu/amd_iommu_debugfs.c
> @@ -0,0 +1,47 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AMD IOMMU driver
> + *
> + * Copyright (C) 2018 Advanced Micro Devices, Inc.
> + *
> + * Author: Gary R Hook <gary.hook-5C7GfCeVMHo@public.gmane.org>
> + */
> +
> +#ifdef	CONFIG_IOMMU_DEBUG

Since the module won't be built unless this is defined, you don't
need this.

> +
> +#include <linux/debugfs.h>
> +#include <linux/iommu.h>
> +#include <linux/pci.h>
> +#include "amd_iommu_proto.h"
> +#include "amd_iommu_types.h"
> +
> +static struct dentry *iommu_debugfs_dir;
> +static DEFINE_RWLOCK(iommu_debugfs_lock);

Use amd_iommu_...

Also, didn't you run into an issue with the CCP and debugfs creation
during probe using a RWLOCK?  Should probably make this a mutex.

> +
> +#define	MAX_NAME_LEN	20
> +
> +void amd_iommu_debugfs_setup(struct amd_iommu *iommu)
> +{
> +	char name[MAX_NAME_LEN + 1];
> +	struct dentry *d_top;
> +	unsigned long flags;
> +
> +	if (!debugfs_initialized())
> +		return;
> +
> +	write_lock_irqsave(&iommu_debugfs_lock, flags);
> +	if (!iommu_debugfs_dir && (d_top = iommu_debugfs_setup()))
> +		iommu_debugfs_dir = debugfs_create_dir("amd", d_top);
> +	write_unlock_irqrestore(&iommu_debugfs_lock, flags);
> +	if (iommu_debugfs_dir) {
> +		snprintf(name, MAX_NAME_LEN, "iommu%02d", iommu->index);
> +		iommu->debugfs_instance = debugfs_create_dir(name,
> +							     iommu_debugfs_dir);
> +		if (!iommu->debugfs_instance)
> +			debugfs_remove_recursive(iommu_debugfs_dir);

So this might cause an error.  You remove it, but don't set the variable
to NULL, so the next IOMMU that is probed could try to use it.  But why
are you deleting it anyway?

> +	}
> +
> +	return;
> +}
> +
> +#endif
> diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
> index 99d48c42a12f..43856c7f4ea1 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
> +

Spurious newline.

>  /*
>   * ACPI table definitions
>   *
> @@ -2720,6 +2721,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);
> @@ -2729,14 +2731,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;
>  }
>  
> @@ -2783,8 +2786,6 @@ int __init amd_iommu_detect(void)
>  	iommu_detected = 1;
>  	x86_init.iommu.iommu_init = amd_iommu_init;
>  
> -dump_stack();
> -
>  	return 1;
>  }
>  
> diff --git a/drivers/iommu/amd_iommu_proto.h b/drivers/iommu/amd_iommu_proto.h
> index 640c286a0ab9..e19cebc5c740 100644
> --- a/drivers/iommu/amd_iommu_proto.h
> +++ b/drivers/iommu/amd_iommu_proto.h
> @@ -33,6 +33,12 @@ extern void amd_iommu_uninit_devices(void);
>  extern void amd_iommu_init_notifier(void);
>  extern int amd_iommu_init_api(void);
>  
> +#ifdef	CONFIG_IOMMU_DEBUG

Use space instead of tab.

> +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 f6b24c7d8b70..6dca9fe38518 100644
> --- a/drivers/iommu/amd_iommu_types.h
> +++ b/drivers/iommu/amd_iommu_types.h
> @@ -591,6 +591,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)
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 98527f9b473b..dbfff811aa25 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -412,6 +412,10 @@ void iommu_fwspec_free(struct device *dev);
>  int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
>  const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode);
>  
> +#ifdef	CONFIG_IOMMU_DEBUG
> +extern struct dentry *iommu_debugfs_setup(void);
> +#endif
> +

Any reason why this moved?  If it was not in the right place in the first
patch, that's where it should be fixed.

Thanks,
Tom

>  #else /* CONFIG_IOMMU_API */
>  
>  struct iommu_ops {};
> @@ -696,10 +700,6 @@ const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
>  	return NULL;
>  }
>  
> -#ifdef	CONFIG_IOMMU_DEBUG
> -extern struct dentry *iommu_debugfs_setup(void);
> -#endif
> -
>  #endif /* CONFIG_IOMMU_API */
>  
>  #endif /* __LINUX_IOMMU_H */
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] iommu - Enable debugfs exposure of the IOMMU
@ 2018-03-30 18:41       ` Gary R Hook
  0 siblings, 0 replies; 14+ messages in thread
From: Gary R Hook @ 2018-03-30 18:41 UTC (permalink / raw)
  To: Tom Lendacky, iommu; +Cc: joro, jacob.jun.pan, linux-kernel

On 03/29/2018 10:57 PM, Tom Lendacky wrote:
> On 3/29/2018 5:54 PM, Gary R Hook wrote:
>> Provide base enablement for using debugfs to expose internal data of
>> an IOMMU driver. When enabled, create the /sys/kernel/debug/iommu
> 
> So this can't actually create anything yet since nothing invokes the
> function.  Maybe describe how it should be used by other drivers (and
> probably include that description as a commment for the function) to
> create the directory.

Exactly. Given the approach I took, it takes another patch to take 
advantage of this. I can certainly elaborate on usage if we agree that 
this framework is acceptable.

> 
>> directory.  Emit a strong warning at boot time to indicate that this
>> feature is enabled.
>>
>> Signed-off-by: Gary R Hook <gary.hook@amd.com>
>> ---
>>   drivers/iommu/Kconfig          |   11 +++++++++++
>>   drivers/iommu/Makefile         |    2 ++
>>   drivers/iommu/amd_iommu_init.c |    2 ++
>>   drivers/iommu/iommu-debugfs.c  |   32 ++++++++++++++++++++++++++++++++
>>   include/linux/iommu.h          |    4 ++++
>>   5 files changed, 51 insertions(+)
>>   create mode 100644 drivers/iommu/iommu-debugfs.c
>>
>> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
>> index f3a21343e636..c1e39dabfec2 100644
>> --- a/drivers/iommu/Kconfig
>> +++ b/drivers/iommu/Kconfig
>> @@ -60,6 +60,17 @@ config IOMMU_IO_PGTABLE_ARMV7S_SELFTEST
>>   
>>   endmenu
>>   
>> +config IOMMU_DEBUG
>> +	bool "Enable IOMMU internals in DebugFS"
>> +	depends on DEBUG_FS
>> +	default n
>> +	help
>> +	  Allows exposure of IOMMU device internals. This option enables
>> +	  the use of debugfs by IOMMU drivers as required. Devices can,
>> +	  at initialization time, cause the IOMMU code to create a top-level
>> +	  debug/iommu directory, and then populate a subdirectory with
>> +	  entries as required.
>> +
>>   config IOMMU_IOVA
>>   	tristate
>>   
>> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
>> index 1fb695854809..5e5c3339681d 100644
>> --- a/drivers/iommu/Makefile
>> +++ b/drivers/iommu/Makefile
>> @@ -2,6 +2,7 @@
>>   obj-$(CONFIG_IOMMU_API) += iommu.o
>>   obj-$(CONFIG_IOMMU_API) += iommu-traces.o
>>   obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o
>> +obj-$(CONFIG_IOMMU_DEBUG) += iommu-debugfs.o
>>   obj-$(CONFIG_IOMMU_DMA) += dma-iommu.o
>>   obj-$(CONFIG_IOMMU_IO_PGTABLE) += io-pgtable.o
>>   obj-$(CONFIG_IOMMU_IO_PGTABLE_ARMV7S) += io-pgtable-arm-v7s.o
>> @@ -10,6 +11,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
> 
> Where is this CONFIG defined?

That, my friend is left-over cruft. Please ignore. Apologies, and thanks.

>>   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_init.c b/drivers/iommu/amd_iommu_init.c
>> index 6fe2d0346073..99d48c42a12f 100644
>> --- a/drivers/iommu/amd_iommu_init.c
>> +++ b/drivers/iommu/amd_iommu_init.c
>> @@ -2783,6 +2783,8 @@ int __init amd_iommu_detect(void)
>>   	iommu_detected = 1;
>>   	x86_init.iommu.iommu_init = amd_iommu_init;
>>   
>> +dump_stack();
>> +
> 
> This definitely shouldn't be here.

Dang it!

> 
>>   	return 1;
>>   }
>>   
>> diff --git a/drivers/iommu/iommu-debugfs.c b/drivers/iommu/iommu-debugfs.c
>> new file mode 100644
>> index 000000000000..94c9acc63b65
>> --- /dev/null
>> +++ b/drivers/iommu/iommu-debugfs.c
>> @@ -0,0 +1,32 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * AMD IOMMU driver
> 
> This isn't the AMD IOMMU driver.

Again: dang it!

>> + *
>> + * Copyright (C) 2018 Advanced Micro Devices, Inc.
>> + *
>> + * Author: Gary R Hook <gary.hook@amd.com>
>> + */
>> +
>> +#include <linux/pci.h>
>> +#include <linux/iommu.h>
>> +#include <linux/debugfs.h>
>> +
>> +static struct dentry *iommu_debugfs_dir;
>> +
>> +#define	MAX_NAME_LEN	20
> 
> This isn't used anywhere.

Thanks.

> 
>> +
>> +/* Return a zero on failure; 1 on successful setup */
> 
> Return NULL on failure, pointer to IOMMU debugfs dentry on success.

Roger.

> 
>> +struct dentry *iommu_debugfs_setup(void)
>> +{
>> +	if (!debugfs_initialized())
>> +		return NULL;
>> +
>> +	if (!iommu_debugfs_dir)
>> +		iommu_debugfs_dir = debugfs_create_dir("iommu", NULL);
>> +
>> +	if (iommu_debugfs_dir)
>> +		pr_warn("WARNING: IOMMU DEBUGFS SUPPORT HAS BEEN ENABLED IN THIS KERNEL\n");
>> +
>> +	return iommu_debugfs_dir;
>> +}
>> +EXPORT_SYMBOL_GPL(iommu_debugfs_setup);
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index 41b8c5757859..cb2957dac43b 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -696,6 +696,10 @@ const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
>>   	return NULL;
>>   }
>>   
>> +#ifdef	CONFIG_IOMMU_DEBUG
> 
> Should be a space between the #ifdef and the CONFIG_..., not a tab.

Roger that.

> 
> Thanks,
> Tom
> 
>> +struct dentry *iommu_debugfs_setup(void);
>> +#endif
>> +
>>   #endif /* CONFIG_IOMMU_API */
>>   
>>   #endif /* __LINUX_IOMMU_H */
>>

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] iommu - Enable debugfs exposure of the IOMMU
@ 2018-03-30 18:41       ` Gary R Hook
  0 siblings, 0 replies; 14+ messages in thread
From: Gary R Hook @ 2018-03-30 18:41 UTC (permalink / raw)
  To: Tom Lendacky, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA
  Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA

On 03/29/2018 10:57 PM, Tom Lendacky wrote:
> On 3/29/2018 5:54 PM, Gary R Hook wrote:
>> Provide base enablement for using debugfs to expose internal data of
>> an IOMMU driver. When enabled, create the /sys/kernel/debug/iommu
> 
> So this can't actually create anything yet since nothing invokes the
> function.  Maybe describe how it should be used by other drivers (and
> probably include that description as a commment for the function) to
> create the directory.

Exactly. Given the approach I took, it takes another patch to take 
advantage of this. I can certainly elaborate on usage if we agree that 
this framework is acceptable.

> 
>> directory.  Emit a strong warning at boot time to indicate that this
>> feature is enabled.
>>
>> Signed-off-by: Gary R Hook <gary.hook-5C7GfCeVMHo@public.gmane.org>
>> ---
>>   drivers/iommu/Kconfig          |   11 +++++++++++
>>   drivers/iommu/Makefile         |    2 ++
>>   drivers/iommu/amd_iommu_init.c |    2 ++
>>   drivers/iommu/iommu-debugfs.c  |   32 ++++++++++++++++++++++++++++++++
>>   include/linux/iommu.h          |    4 ++++
>>   5 files changed, 51 insertions(+)
>>   create mode 100644 drivers/iommu/iommu-debugfs.c
>>
>> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
>> index f3a21343e636..c1e39dabfec2 100644
>> --- a/drivers/iommu/Kconfig
>> +++ b/drivers/iommu/Kconfig
>> @@ -60,6 +60,17 @@ config IOMMU_IO_PGTABLE_ARMV7S_SELFTEST
>>   
>>   endmenu
>>   
>> +config IOMMU_DEBUG
>> +	bool "Enable IOMMU internals in DebugFS"
>> +	depends on DEBUG_FS
>> +	default n
>> +	help
>> +	  Allows exposure of IOMMU device internals. This option enables
>> +	  the use of debugfs by IOMMU drivers as required. Devices can,
>> +	  at initialization time, cause the IOMMU code to create a top-level
>> +	  debug/iommu directory, and then populate a subdirectory with
>> +	  entries as required.
>> +
>>   config IOMMU_IOVA
>>   	tristate
>>   
>> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
>> index 1fb695854809..5e5c3339681d 100644
>> --- a/drivers/iommu/Makefile
>> +++ b/drivers/iommu/Makefile
>> @@ -2,6 +2,7 @@
>>   obj-$(CONFIG_IOMMU_API) += iommu.o
>>   obj-$(CONFIG_IOMMU_API) += iommu-traces.o
>>   obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o
>> +obj-$(CONFIG_IOMMU_DEBUG) += iommu-debugfs.o
>>   obj-$(CONFIG_IOMMU_DMA) += dma-iommu.o
>>   obj-$(CONFIG_IOMMU_IO_PGTABLE) += io-pgtable.o
>>   obj-$(CONFIG_IOMMU_IO_PGTABLE_ARMV7S) += io-pgtable-arm-v7s.o
>> @@ -10,6 +11,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
> 
> Where is this CONFIG defined?

That, my friend is left-over cruft. Please ignore. Apologies, and thanks.

>>   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_init.c b/drivers/iommu/amd_iommu_init.c
>> index 6fe2d0346073..99d48c42a12f 100644
>> --- a/drivers/iommu/amd_iommu_init.c
>> +++ b/drivers/iommu/amd_iommu_init.c
>> @@ -2783,6 +2783,8 @@ int __init amd_iommu_detect(void)
>>   	iommu_detected = 1;
>>   	x86_init.iommu.iommu_init = amd_iommu_init;
>>   
>> +dump_stack();
>> +
> 
> This definitely shouldn't be here.

Dang it!

> 
>>   	return 1;
>>   }
>>   
>> diff --git a/drivers/iommu/iommu-debugfs.c b/drivers/iommu/iommu-debugfs.c
>> new file mode 100644
>> index 000000000000..94c9acc63b65
>> --- /dev/null
>> +++ b/drivers/iommu/iommu-debugfs.c
>> @@ -0,0 +1,32 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * AMD IOMMU driver
> 
> This isn't the AMD IOMMU driver.

Again: dang it!

>> + *
>> + * Copyright (C) 2018 Advanced Micro Devices, Inc.
>> + *
>> + * Author: Gary R Hook <gary.hook-5C7GfCeVMHo@public.gmane.org>
>> + */
>> +
>> +#include <linux/pci.h>
>> +#include <linux/iommu.h>
>> +#include <linux/debugfs.h>
>> +
>> +static struct dentry *iommu_debugfs_dir;
>> +
>> +#define	MAX_NAME_LEN	20
> 
> This isn't used anywhere.

Thanks.

> 
>> +
>> +/* Return a zero on failure; 1 on successful setup */
> 
> Return NULL on failure, pointer to IOMMU debugfs dentry on success.

Roger.

> 
>> +struct dentry *iommu_debugfs_setup(void)
>> +{
>> +	if (!debugfs_initialized())
>> +		return NULL;
>> +
>> +	if (!iommu_debugfs_dir)
>> +		iommu_debugfs_dir = debugfs_create_dir("iommu", NULL);
>> +
>> +	if (iommu_debugfs_dir)
>> +		pr_warn("WARNING: IOMMU DEBUGFS SUPPORT HAS BEEN ENABLED IN THIS KERNEL\n");
>> +
>> +	return iommu_debugfs_dir;
>> +}
>> +EXPORT_SYMBOL_GPL(iommu_debugfs_setup);
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index 41b8c5757859..cb2957dac43b 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -696,6 +696,10 @@ const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
>>   	return NULL;
>>   }
>>   
>> +#ifdef	CONFIG_IOMMU_DEBUG
> 
> Should be a space between the #ifdef and the CONFIG_..., not a tab.

Roger that.

> 
> Thanks,
> Tom
> 
>> +struct dentry *iommu_debugfs_setup(void);
>> +#endif
>> +
>>   #endif /* CONFIG_IOMMU_API */
>>   
>>   #endif /* __LINUX_IOMMU_H */
>>

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU
@ 2018-03-30 19:08       ` Gary R Hook
  0 siblings, 0 replies; 14+ messages in thread
From: Gary R Hook @ 2018-03-30 19:08 UTC (permalink / raw)
  To: Tom Lendacky, iommu; +Cc: joro, jacob.jun.pan, linux-kernel

On 03/29/2018 11:16 PM, Tom Lendacky wrote:
> On 3/29/2018 5:54 PM, Gary R Hook wrote:
>> Implement a skeleton framework for debugfs support in the
>> AMD IOMMU.
>>
>>
>> Signed-off-by: Gary R Hook <gary.hook@amd.com>
>> ---
>>   drivers/iommu/Kconfig             |    6 ++---
>>   drivers/iommu/Makefile            |    2 +-
>>   drivers/iommu/amd_iommu_debugfs.c |   47 +++++++++++++++++++++++++++++++++++++
>>   drivers/iommu/amd_iommu_init.c    |    9 ++++---
>>   drivers/iommu/amd_iommu_proto.h   |    6 +++++
>>   drivers/iommu/amd_iommu_types.h   |    3 ++
>>   include/linux/iommu.h             |    8 +++---
>>   7 files changed, 69 insertions(+), 12 deletions(-)
>>   create mode 100644 drivers/iommu/amd_iommu_debugfs.c
>>
>> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
>> index d40248446214..8d50151d5bf4 100644
>> --- a/drivers/iommu/Kconfig
>> +++ b/drivers/iommu/Kconfig
>> @@ -65,9 +65,9 @@ config IOMMU_DEBUG
>>   	depends on DEBUG_FS
>>   	default n
>>   	help
>> -	  Base enablement for access to any IOMMU device. Allows individual
>> -	  drivers to populate debugfs for access to IOMMU registers and
>> -	  data structures.
>> +	  Enable exposure of IOMMU device internals. Allow devices to
>> +	  populate debugfs for access to IOMMU registers and data
>> +	  structures.
> 
> This help text shouldn't change just because a driver is making use of
> the interface that was created in the previous patch.

Roger. It should be changed in the base patch only.
> 
>>   
>>   config IOMMU_IOVA
>>   	tristate
>> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
>> index 5e5c3339681d..0ca250f626d9 100644
>> --- a/drivers/iommu/Makefile
>> +++ b/drivers/iommu/Makefile
>> @@ -11,7 +11,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_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..3547ad3339c1
>> --- /dev/null
>> +++ b/drivers/iommu/amd_iommu_debugfs.c
>> @@ -0,0 +1,47 @@
>> +// 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_IOMMU_DEBUG
> 
> Since the module won't be built unless this is defined, you don't
> need this.

For some reason my build is trying to compile this file, even though I 
have the debug option disabled. Gotta track this down.

> 
>> +
>> +#include <linux/debugfs.h>
>> +#include <linux/iommu.h>
>> +#include <linux/pci.h>
>> +#include "amd_iommu_proto.h"
>> +#include "amd_iommu_types.h"
>> +
>> +static struct dentry *iommu_debugfs_dir;
>> +static DEFINE_RWLOCK(iommu_debugfs_lock);
> 
> Use amd_iommu_...
> 
> Also, didn't you run into an issue with the CCP and debugfs creation
> during probe using a RWLOCK?  Should probably make this a mutex.

Yes, and had to make a change. I will do so here.

> 
>> +
>> +#define	MAX_NAME_LEN	20
>> +
>> +void amd_iommu_debugfs_setup(struct amd_iommu *iommu)
>> +{
>> +	char name[MAX_NAME_LEN + 1];
>> +	struct dentry *d_top;
>> +	unsigned long flags;
>> +
>> +	if (!debugfs_initialized())
>> +		return;
>> +
>> +	write_lock_irqsave(&iommu_debugfs_lock, flags);
>> +	if (!iommu_debugfs_dir && (d_top = iommu_debugfs_setup()))
>> +		iommu_debugfs_dir = debugfs_create_dir("amd", d_top);
>> +	write_unlock_irqrestore(&iommu_debugfs_lock, flags);
>> +	if (iommu_debugfs_dir) {
>> +		snprintf(name, MAX_NAME_LEN, "iommu%02d", iommu->index);
>> +		iommu->debugfs_instance = debugfs_create_dir(name,
>> +							     iommu_debugfs_dir);
>> +		if (!iommu->debugfs_instance)
>> +			debugfs_remove_recursive(iommu_debugfs_dir);
> 
> So this might cause an error.  You remove it, but don't set the variable
> to NULL, so the next IOMMU that is probed could try to use it.  But why
> are you deleting it anyway?

Right you are. My thought was to remove everything driver-specific in 
the event of a failure. Do we not like that?

> 
>> +	}
>> +
>> +	return;
>> +}
>> +
>> +#endif
>> diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
>> index 99d48c42a12f..43856c7f4ea1 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
>> +
> 
> Spurious newline.

D'oh!

> 
>>   /*
>>    * ACPI table definitions
>>    *
>> @@ -2720,6 +2721,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);
>> @@ -2729,14 +2731,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;
>>   }
>>   
>> @@ -2783,8 +2786,6 @@ int __init amd_iommu_detect(void)
>>   	iommu_detected = 1;
>>   	x86_init.iommu.iommu_init = amd_iommu_init;
>>   
>> -dump_stack();
>> -
>>   	return 1;
>>   }
>>   
>> diff --git a/drivers/iommu/amd_iommu_proto.h b/drivers/iommu/amd_iommu_proto.h
>> index 640c286a0ab9..e19cebc5c740 100644
>> --- a/drivers/iommu/amd_iommu_proto.h
>> +++ b/drivers/iommu/amd_iommu_proto.h
>> @@ -33,6 +33,12 @@ extern void amd_iommu_uninit_devices(void);
>>   extern void amd_iommu_init_notifier(void);
>>   extern int amd_iommu_init_api(void);
>>   
>> +#ifdef	CONFIG_IOMMU_DEBUG
> 
> Use space instead of tab.

Yep, fixed it.

> 
>> +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 f6b24c7d8b70..6dca9fe38518 100644
>> --- a/drivers/iommu/amd_iommu_types.h
>> +++ b/drivers/iommu/amd_iommu_types.h
>> @@ -591,6 +591,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)
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index 98527f9b473b..dbfff811aa25 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -412,6 +412,10 @@ void iommu_fwspec_free(struct device *dev);
>>   int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
>>   const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode);
>>   
>> +#ifdef	CONFIG_IOMMU_DEBUG
>> +extern struct dentry *iommu_debugfs_setup(void);
>> +#endif
>> +
> 
> Any reason why this moved?  If it was not in the right place in the first
> patch, that's where it should be fixed.

It was in the wrong place, but you are correct. I'll properly move it in 
the other patch. Thanks.

> 
> Thanks,
> Tom
> 
>>   #else /* CONFIG_IOMMU_API */
>>   
>>   struct iommu_ops {};
>> @@ -696,10 +700,6 @@ const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
>>   	return NULL;
>>   }
>>   
>> -#ifdef	CONFIG_IOMMU_DEBUG
>> -extern struct dentry *iommu_debugfs_setup(void);
>> -#endif
>> -
>>   #endif /* CONFIG_IOMMU_API */
>>   
>>   #endif /* __LINUX_IOMMU_H */
>>

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU
@ 2018-03-30 19:08       ` Gary R Hook
  0 siblings, 0 replies; 14+ messages in thread
From: Gary R Hook @ 2018-03-30 19:08 UTC (permalink / raw)
  To: Tom Lendacky, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA
  Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA

On 03/29/2018 11:16 PM, Tom Lendacky wrote:
> On 3/29/2018 5:54 PM, Gary R Hook wrote:
>> Implement a skeleton framework for debugfs support in the
>> AMD IOMMU.
>>
>>
>> Signed-off-by: Gary R Hook <gary.hook-5C7GfCeVMHo@public.gmane.org>
>> ---
>>   drivers/iommu/Kconfig             |    6 ++---
>>   drivers/iommu/Makefile            |    2 +-
>>   drivers/iommu/amd_iommu_debugfs.c |   47 +++++++++++++++++++++++++++++++++++++
>>   drivers/iommu/amd_iommu_init.c    |    9 ++++---
>>   drivers/iommu/amd_iommu_proto.h   |    6 +++++
>>   drivers/iommu/amd_iommu_types.h   |    3 ++
>>   include/linux/iommu.h             |    8 +++---
>>   7 files changed, 69 insertions(+), 12 deletions(-)
>>   create mode 100644 drivers/iommu/amd_iommu_debugfs.c
>>
>> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
>> index d40248446214..8d50151d5bf4 100644
>> --- a/drivers/iommu/Kconfig
>> +++ b/drivers/iommu/Kconfig
>> @@ -65,9 +65,9 @@ config IOMMU_DEBUG
>>   	depends on DEBUG_FS
>>   	default n
>>   	help
>> -	  Base enablement for access to any IOMMU device. Allows individual
>> -	  drivers to populate debugfs for access to IOMMU registers and
>> -	  data structures.
>> +	  Enable exposure of IOMMU device internals. Allow devices to
>> +	  populate debugfs for access to IOMMU registers and data
>> +	  structures.
> 
> This help text shouldn't change just because a driver is making use of
> the interface that was created in the previous patch.

Roger. It should be changed in the base patch only.
> 
>>   
>>   config IOMMU_IOVA
>>   	tristate
>> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
>> index 5e5c3339681d..0ca250f626d9 100644
>> --- a/drivers/iommu/Makefile
>> +++ b/drivers/iommu/Makefile
>> @@ -11,7 +11,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_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..3547ad3339c1
>> --- /dev/null
>> +++ b/drivers/iommu/amd_iommu_debugfs.c
>> @@ -0,0 +1,47 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * AMD IOMMU driver
>> + *
>> + * Copyright (C) 2018 Advanced Micro Devices, Inc.
>> + *
>> + * Author: Gary R Hook <gary.hook-5C7GfCeVMHo@public.gmane.org>
>> + */
>> +
>> +#ifdef	CONFIG_IOMMU_DEBUG
> 
> Since the module won't be built unless this is defined, you don't
> need this.

For some reason my build is trying to compile this file, even though I 
have the debug option disabled. Gotta track this down.

> 
>> +
>> +#include <linux/debugfs.h>
>> +#include <linux/iommu.h>
>> +#include <linux/pci.h>
>> +#include "amd_iommu_proto.h"
>> +#include "amd_iommu_types.h"
>> +
>> +static struct dentry *iommu_debugfs_dir;
>> +static DEFINE_RWLOCK(iommu_debugfs_lock);
> 
> Use amd_iommu_...
> 
> Also, didn't you run into an issue with the CCP and debugfs creation
> during probe using a RWLOCK?  Should probably make this a mutex.

Yes, and had to make a change. I will do so here.

> 
>> +
>> +#define	MAX_NAME_LEN	20
>> +
>> +void amd_iommu_debugfs_setup(struct amd_iommu *iommu)
>> +{
>> +	char name[MAX_NAME_LEN + 1];
>> +	struct dentry *d_top;
>> +	unsigned long flags;
>> +
>> +	if (!debugfs_initialized())
>> +		return;
>> +
>> +	write_lock_irqsave(&iommu_debugfs_lock, flags);
>> +	if (!iommu_debugfs_dir && (d_top = iommu_debugfs_setup()))
>> +		iommu_debugfs_dir = debugfs_create_dir("amd", d_top);
>> +	write_unlock_irqrestore(&iommu_debugfs_lock, flags);
>> +	if (iommu_debugfs_dir) {
>> +		snprintf(name, MAX_NAME_LEN, "iommu%02d", iommu->index);
>> +		iommu->debugfs_instance = debugfs_create_dir(name,
>> +							     iommu_debugfs_dir);
>> +		if (!iommu->debugfs_instance)
>> +			debugfs_remove_recursive(iommu_debugfs_dir);
> 
> So this might cause an error.  You remove it, but don't set the variable
> to NULL, so the next IOMMU that is probed could try to use it.  But why
> are you deleting it anyway?

Right you are. My thought was to remove everything driver-specific in 
the event of a failure. Do we not like that?

> 
>> +	}
>> +
>> +	return;
>> +}
>> +
>> +#endif
>> diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
>> index 99d48c42a12f..43856c7f4ea1 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
>> +
> 
> Spurious newline.

D'oh!

> 
>>   /*
>>    * ACPI table definitions
>>    *
>> @@ -2720,6 +2721,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);
>> @@ -2729,14 +2731,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;
>>   }
>>   
>> @@ -2783,8 +2786,6 @@ int __init amd_iommu_detect(void)
>>   	iommu_detected = 1;
>>   	x86_init.iommu.iommu_init = amd_iommu_init;
>>   
>> -dump_stack();
>> -
>>   	return 1;
>>   }
>>   
>> diff --git a/drivers/iommu/amd_iommu_proto.h b/drivers/iommu/amd_iommu_proto.h
>> index 640c286a0ab9..e19cebc5c740 100644
>> --- a/drivers/iommu/amd_iommu_proto.h
>> +++ b/drivers/iommu/amd_iommu_proto.h
>> @@ -33,6 +33,12 @@ extern void amd_iommu_uninit_devices(void);
>>   extern void amd_iommu_init_notifier(void);
>>   extern int amd_iommu_init_api(void);
>>   
>> +#ifdef	CONFIG_IOMMU_DEBUG
> 
> Use space instead of tab.

Yep, fixed it.

> 
>> +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 f6b24c7d8b70..6dca9fe38518 100644
>> --- a/drivers/iommu/amd_iommu_types.h
>> +++ b/drivers/iommu/amd_iommu_types.h
>> @@ -591,6 +591,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)
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index 98527f9b473b..dbfff811aa25 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -412,6 +412,10 @@ void iommu_fwspec_free(struct device *dev);
>>   int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
>>   const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode);
>>   
>> +#ifdef	CONFIG_IOMMU_DEBUG
>> +extern struct dentry *iommu_debugfs_setup(void);
>> +#endif
>> +
> 
> Any reason why this moved?  If it was not in the right place in the first
> patch, that's where it should be fixed.

It was in the wrong place, but you are correct. I'll properly move it in 
the other patch. Thanks.

> 
> Thanks,
> Tom
> 
>>   #else /* CONFIG_IOMMU_API */
>>   
>>   struct iommu_ops {};
>> @@ -696,10 +700,6 @@ const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
>>   	return NULL;
>>   }
>>   
>> -#ifdef	CONFIG_IOMMU_DEBUG
>> -extern struct dentry *iommu_debugfs_setup(void);
>> -#endif
>> -
>>   #endif /* CONFIG_IOMMU_API */
>>   
>>   #endif /* __LINUX_IOMMU_H */
>>

^ permalink raw reply	[flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-03-30 19:08 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-29 22:54 [PATCH 0/2] Base enablement of IOMMU debugfs support Gary R Hook
2018-03-29 22:54 ` Gary R Hook
2018-03-29 22:54 ` [PATCH 1/2] iommu - Enable debugfs exposure of the IOMMU Gary R Hook
2018-03-29 22:54   ` Gary R Hook
2018-03-30  3:57   ` Tom Lendacky
2018-03-30  3:57     ` Tom Lendacky
2018-03-30 18:41     ` Gary R Hook
2018-03-30 18:41       ` Gary R Hook
2018-03-29 22:54 ` [PATCH 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU Gary R Hook
2018-03-29 22:54   ` Gary R Hook
2018-03-30  4:16   ` Tom Lendacky
2018-03-30  4:16     ` Tom Lendacky
2018-03-30 19:08     ` Gary R Hook
2018-03-30 19:08       ` Gary R Hook

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.