[PATCH v8 0/2] Base enablement of IOMMU debugfs support

[PATCH v8 0/2] Base enablement of IOMMU debugfs support

[Gary R Hook]

These patches create a top-level function, called at IOMMU
initialization, to create a debugfs directory for the IOMMU. Under
this directory drivers may create and populate vendor-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_DEBUGFS to globally
allow/disallow debugfs code to be built.

The Makefile structure is intended to allow the use of a single
switch for turning on DebugFS.

Changes since v7:
 - Change the Kconfig approach to use a hidden boolean for a
   specific device
 - Change some #ifdefs to reference the new boolean

Changes since v6:
 - Rely on default Kconfig value for a bool
 - comment/doc fixes
 - use const where appropriate
 - fix inline declaration

Changes since v5:
 - Added parameters names in declarations/definitions
 - Reformatted an inline definition

Changes since v4:
 - Guard vendor-specific debugfs files in the Makefile
 - Call top-level routine from iommu_init()
 - Add function for instantiating a driver-specific directory
 - Change AMD driver code to use this new format

Changes since v3:
 - Remove superfluous calls to debugfs_initialized()
 - Emit a warning exactly one time
 - Change the Kconfig name to IOMMU_DEBUGFS
 - Change the way debugfs modules are made

Changes since v2:
 - Move a declaration to outside an ifdef
 - Remove a spurious blank line

Changes since v1:
 - Remove debug cruft
 - Remove cruft produced by design change
 - Change the lock to a mutex
 - Coding style fixes
 - Add a comment to document the framework

---

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


 drivers/iommu/Kconfig             |   14 +++++++
 drivers/iommu/Makefile            |    2 +
 drivers/iommu/amd_iommu_debugfs.c |   39 ++++++++++++++++++++
 drivers/iommu/amd_iommu_init.c    |    6 ++-
 drivers/iommu/amd_iommu_proto.h   |    6 +++
 drivers/iommu/amd_iommu_types.h   |    5 +++
 drivers/iommu/iommu-debugfs.c     |   72 +++++++++++++++++++++++++++++++++++++
 drivers/iommu/iommu.c             |    2 +
 include/linux/iommu.h             |   11 ++++++
 9 files changed, 155 insertions(+), 2 deletions(-)
 create mode 100644 drivers/iommu/amd_iommu_debugfs.c
 create mode 100644 drivers/iommu/iommu-debugfs.c

--
Signature

[PATCH v8 1/2] iommu - Enable debugfs exposure of IOMMU driver internals

[Gary R Hook]

Provide base enablement for using debugfs to expose internal data of an
IOMMU driver. When called, create the /sys/kernel/debug/iommu directory.

Emit a strong warning at boot time to indicate that this feature is
enabled.

This function is called from iommu_init, and creates the initial DebugFS
directory. Drivers may then call iommu_debugfs_new_driver_dir() to
instantiate a device-specific directory to expose internal data.
It will return a pointer to the new dentry structure created in
/sys/kernel/debug/iommu, or NULL in the event of a failure.

Since the IOMMU driver can not be removed from the running system, there
is no need for an "off" function.

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

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index c76157e57f6b..f9af25ac409f 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -60,6 +60,16 @@ config IOMMU_IO_PGTABLE_ARMV7S_SELFTEST
 
 endmenu
 
+config IOMMU_DEBUGFS
+	bool "Export IOMMU internals in DebugFS"
+	depends on DEBUG_FS
+	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..74cfbc392862 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_DEBUGFS) += 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
diff --git a/drivers/iommu/iommu-debugfs.c b/drivers/iommu/iommu-debugfs.c
new file mode 100644
index 000000000000..bb1bf2d0ac51
--- /dev/null
+++ b/drivers/iommu/iommu-debugfs.c
@@ -0,0 +1,72 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * IOMMU debugfs core infrastructure
+ *
+ * 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;
+
+/**
+ * iommu_debugfs_setup - create the top-level iommu directory in debugfs
+ *
+ * Provide base enablement for using debugfs to expose internal data of an
+ * IOMMU driver. When called, this function creates the
+ * /sys/kernel/debug/iommu directory.
+ *
+ * Emit a strong warning at boot time to indicate that this feature is
+ * enabled.
+ *
+ * This function is called from iommu_init; drivers may then call
+ * iommu_debugfs_new_driver_dir() to instantiate a vendor-specific
+ * directory to be used to expose internal data.
+ */
+void iommu_debugfs_setup(void)
+{
+	if (!iommu_debugfs_dir) {
+		iommu_debugfs_dir = debugfs_create_dir("iommu", NULL);
+		if (iommu_debugfs_dir) {
+			pr_warn("\n");
+			pr_warn("*************************************************************\n");
+			pr_warn("**     NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE    **\n");
+			pr_warn("**                                                         **\n");
+			pr_warn("**  IOMMU DebugFS SUPPORT HAS BEEN ENABLED IN THIS KERNEL  **\n");
+			pr_warn("**                                                         **\n");
+			pr_warn("** This means that this kernel is built to expose internal **\n");
+			pr_warn("** IOMMU data structures, which may compromise security on **\n");
+			pr_warn("** your system.                                            **\n");
+			pr_warn("**                                                         **\n");
+			pr_warn("** If you see this message and you are not debugging the   **\n");
+			pr_warn("** kernel, report this immediately to your vendor!         **\n");
+			pr_warn("**                                                         **\n");
+			pr_warn("**     NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE    **\n");
+			pr_warn("*************************************************************\n");
+		}
+	}
+}
+
+/**
+ * iommu_debugfs_new_driver_dir - create a vendor directory under debugfs/iommu
+ * @vendor: name of the vendor-specific subdirectory to create
+ *
+ * This function is called by an IOMMU driver to create the top-level debugfs
+ * directory for that driver.
+ *
+ * Return: upon success, a pointer to the dentry for the new directory.
+ *         NULL in case of failure.
+ */
+struct dentry *iommu_debugfs_new_driver_dir(const char *vendor)
+{
+	struct dentry *d_new;
+
+	d_new = debugfs_create_dir(vendor, iommu_debugfs_dir);
+
+	return d_new;
+}
+EXPORT_SYMBOL_GPL(iommu_debugfs_new_driver_dir);
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index d2aa23202bb9..350819f1c5e1 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1747,6 +1747,8 @@ static int __init iommu_init(void)
 					       NULL, kernel_kobj);
 	BUG_ON(!iommu_group_kset);
 
+	iommu_debugfs_setup();
+
 	return 0;
 }
 core_initcall(iommu_init);
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 19938ee6eb31..0933db261b9d 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -698,4 +698,15 @@ const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
 
 #endif /* CONFIG_IOMMU_API */
 
+#ifdef CONFIG_IOMMU_DEBUGFS
+void iommu_debugfs_setup(void);
+struct dentry *iommu_debugfs_new_driver_dir(const char *vendor);
+#else
+static inline void iommu_debugfs_setup(void) {}
+static inline struct dentry *iommu_debugfs_new_driver_dir(const char *vendor)
+{
+	return NULL;
+}
+#endif
+
 #endif /* __LINUX_IOMMU_H */

Re: [PATCH v8 1/2] iommu - Enable debugfs exposure of IOMMU driver internals

[Greg KH]

On Tue, May 29, 2018 at 01:23:14PM -0500, Gary R Hook wrote:
> Provide base enablement for using debugfs to expose internal data of an
> IOMMU driver. When called, create the /sys/kernel/debug/iommu directory.
> 
> Emit a strong warning at boot time to indicate that this feature is
> enabled.
> 
> This function is called from iommu_init, and creates the initial DebugFS
> directory. Drivers may then call iommu_debugfs_new_driver_dir() to
> instantiate a device-specific directory to expose internal data.
> It will return a pointer to the new dentry structure created in
> /sys/kernel/debug/iommu, or NULL in the event of a failure.
> 
> Since the IOMMU driver can not be removed from the running system, there
> is no need for an "off" function.
> 
> Signed-off-by: Gary R Hook <gary.hook@amd.com>
> ---
>  drivers/iommu/Kconfig         |   10 ++++++
>  drivers/iommu/Makefile        |    1 +
>  drivers/iommu/iommu-debugfs.c |   72 +++++++++++++++++++++++++++++++++++++++++
>  drivers/iommu/iommu.c         |    2 +
>  include/linux/iommu.h         |   11 ++++++
>  5 files changed, 96 insertions(+)
>  create mode 100644 drivers/iommu/iommu-debugfs.c
> 
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index c76157e57f6b..f9af25ac409f 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -60,6 +60,16 @@ config IOMMU_IO_PGTABLE_ARMV7S_SELFTEST
>  
>  endmenu
>  
> +config IOMMU_DEBUGFS
> +	bool "Export IOMMU internals in DebugFS"
> +	depends on DEBUG_FS
> +	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.

You didn't put a big enough warning here, like I asked last time :(



> +
>  config IOMMU_IOVA
>  	tristate
>  
> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
> index 1fb695854809..74cfbc392862 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_DEBUGFS) += 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
> diff --git a/drivers/iommu/iommu-debugfs.c b/drivers/iommu/iommu-debugfs.c
> new file mode 100644
> index 000000000000..bb1bf2d0ac51
> --- /dev/null
> +++ b/drivers/iommu/iommu-debugfs.c
> @@ -0,0 +1,72 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * IOMMU debugfs core infrastructure
> + *
> + * 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;
> +
> +/**
> + * iommu_debugfs_setup - create the top-level iommu directory in debugfs
> + *
> + * Provide base enablement for using debugfs to expose internal data of an
> + * IOMMU driver. When called, this function creates the
> + * /sys/kernel/debug/iommu directory.
> + *
> + * Emit a strong warning at boot time to indicate that this feature is
> + * enabled.
> + *
> + * This function is called from iommu_init; drivers may then call
> + * iommu_debugfs_new_driver_dir() to instantiate a vendor-specific
> + * directory to be used to expose internal data.
> + */
> +void iommu_debugfs_setup(void)
> +{
> +	if (!iommu_debugfs_dir) {
> +		iommu_debugfs_dir = debugfs_create_dir("iommu", NULL);
> +		if (iommu_debugfs_dir) {

Again, no, never test a return value from a debugfs call.  You do not
care, you should do the same exact thing if it is enabled or disabled or
somehow failing.  Just keep moving on.

> +			pr_warn("\n");
> +			pr_warn("*************************************************************\n");
> +			pr_warn("**     NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE    **\n");
> +			pr_warn("**                                                         **\n");
> +			pr_warn("**  IOMMU DebugFS SUPPORT HAS BEEN ENABLED IN THIS KERNEL  **\n");
> +			pr_warn("**                                                         **\n");
> +			pr_warn("** This means that this kernel is built to expose internal **\n");
> +			pr_warn("** IOMMU data structures, which may compromise security on **\n");
> +			pr_warn("** your system.                                            **\n");
> +			pr_warn("**                                                         **\n");
> +			pr_warn("** If you see this message and you are not debugging the   **\n");
> +			pr_warn("** kernel, report this immediately to your vendor!         **\n");
> +			pr_warn("**                                                         **\n");
> +			pr_warn("**     NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE    **\n");
> +			pr_warn("*************************************************************\n");
> +		}
> +	}
> +}
> +
> +/**
> + * iommu_debugfs_new_driver_dir - create a vendor directory under debugfs/iommu
> + * @vendor: name of the vendor-specific subdirectory to create
> + *
> + * This function is called by an IOMMU driver to create the top-level debugfs
> + * directory for that driver.
> + *
> + * Return: upon success, a pointer to the dentry for the new directory.
> + *         NULL in case of failure.
> + */
> +struct dentry *iommu_debugfs_new_driver_dir(const char *vendor)
> +{
> +	struct dentry *d_new;
> +
> +	d_new = debugfs_create_dir(vendor, iommu_debugfs_dir);
> +
> +	return d_new;
> +}
> +EXPORT_SYMBOL_GPL(iommu_debugfs_new_driver_dir);

Why are you wrapping a debugfs call?  Why not just export
iommu_debugfs_dir instead?

thanks,

greg k-h

Re: [PATCH v8 1/2] iommu - Enable debugfs exposure of IOMMU driver internals

[Gary R Hook]

On 05/29/2018 01:41 PM, Greg KH wrote:
> On Tue, May 29, 2018 at 01:23:14PM -0500, Gary R Hook wrote:
>> Provide base enablement for using debugfs to expose internal data of an
>> IOMMU driver. When called, create the /sys/kernel/debug/iommu directory.
>>
>> Emit a strong warning at boot time to indicate that this feature is
>> enabled.
>>
>> This function is called from iommu_init, and creates the initial DebugFS
>> directory. Drivers may then call iommu_debugfs_new_driver_dir() to
>> instantiate a device-specific directory to expose internal data.
>> It will return a pointer to the new dentry structure created in
>> /sys/kernel/debug/iommu, or NULL in the event of a failure.
>>
>> Since the IOMMU driver can not be removed from the running system, there
>> is no need for an "off" function.
>>
>> Signed-off-by: Gary R Hook <gary.hook@amd.com>
>> ---
>>   drivers/iommu/Kconfig         |   10 ++++++
>>   drivers/iommu/Makefile        |    1 +
>>   drivers/iommu/iommu-debugfs.c |   72 +++++++++++++++++++++++++++++++++++++++++
>>   drivers/iommu/iommu.c         |    2 +
>>   include/linux/iommu.h         |   11 ++++++
>>   5 files changed, 96 insertions(+)
>>   create mode 100644 drivers/iommu/iommu-debugfs.c
>>
>> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
>> index c76157e57f6b..f9af25ac409f 100644
>> --- a/drivers/iommu/Kconfig
>> +++ b/drivers/iommu/Kconfig
>> @@ -60,6 +60,16 @@ config IOMMU_IO_PGTABLE_ARMV7S_SELFTEST
>>   
>>   endmenu
>>   
>> +config IOMMU_DEBUGFS
>> +	bool "Export IOMMU internals in DebugFS"
>> +	depends on DEBUG_FS
>> +	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.
> 
> You didn't put a big enough warning here, like I asked last time :(

As I noted a few moments ago, I didn't see these until now. Very 
annoying, that.

> 
>> +
>>   config IOMMU_IOVA
>>   	tristate
>>   
>> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
>> index 1fb695854809..74cfbc392862 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_DEBUGFS) += 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
>> diff --git a/drivers/iommu/iommu-debugfs.c b/drivers/iommu/iommu-debugfs.c
>> new file mode 100644
>> index 000000000000..bb1bf2d0ac51
>> --- /dev/null
>> +++ b/drivers/iommu/iommu-debugfs.c
>> @@ -0,0 +1,72 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * IOMMU debugfs core infrastructure
>> + *
>> + * 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;
>> +
>> +/**
>> + * iommu_debugfs_setup - create the top-level iommu directory in debugfs
>> + *
>> + * Provide base enablement for using debugfs to expose internal data of an
>> + * IOMMU driver. When called, this function creates the
>> + * /sys/kernel/debug/iommu directory.
>> + *
>> + * Emit a strong warning at boot time to indicate that this feature is
>> + * enabled.
>> + *
>> + * This function is called from iommu_init; drivers may then call
>> + * iommu_debugfs_new_driver_dir() to instantiate a vendor-specific
>> + * directory to be used to expose internal data.
>> + */
>> +void iommu_debugfs_setup(void)
>> +{
>> +	if (!iommu_debugfs_dir) {
>> +		iommu_debugfs_dir = debugfs_create_dir("iommu", NULL);
>> +		if (iommu_debugfs_dir) {
> 
> Again, no, never test a return value from a debugfs call.  You do not
> care, you should do the same exact thing if it is enabled or disabled or
> somehow failing.  Just keep moving on.

Yes, noted.

> 
>> +			pr_warn("\n");
>> +			pr_warn("*************************************************************\n");
>> +			pr_warn("**     NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE    **\n");
>> +			pr_warn("**                                                         **\n");
>> +			pr_warn("**  IOMMU DebugFS SUPPORT HAS BEEN ENABLED IN THIS KERNEL  **\n");
>> +			pr_warn("**                                                         **\n");
>> +			pr_warn("** This means that this kernel is built to expose internal **\n");
>> +			pr_warn("** IOMMU data structures, which may compromise security on **\n");
>> +			pr_warn("** your system.                                            **\n");
>> +			pr_warn("**                                                         **\n");
>> +			pr_warn("** If you see this message and you are not debugging the   **\n");
>> +			pr_warn("** kernel, report this immediately to your vendor!         **\n");
>> +			pr_warn("**                                                         **\n");
>> +			pr_warn("**     NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE    **\n");
>> +			pr_warn("*************************************************************\n");
>> +		}
>> +	}
>> +}
>> +
>> +/**
>> + * iommu_debugfs_new_driver_dir - create a vendor directory under debugfs/iommu
>> + * @vendor: name of the vendor-specific subdirectory to create
>> + *
>> + * This function is called by an IOMMU driver to create the top-level debugfs
>> + * directory for that driver.
>> + *
>> + * Return: upon success, a pointer to the dentry for the new directory.
>> + *         NULL in case of failure.
>> + */
>> +struct dentry *iommu_debugfs_new_driver_dir(const char *vendor)
>> +{
>> +	struct dentry *d_new;
>> +
>> +	d_new = debugfs_create_dir(vendor, iommu_debugfs_dir);
>> +
>> +	return d_new;
>> +}
>> +EXPORT_SYMBOL_GPL(iommu_debugfs_new_driver_dir);
> 
> Why are you wrapping a debugfs call?  Why not just export
> iommu_debugfs_dir instead?

It was a choice, as I stated in my other post. It is not a requirement.
If you resolutely reject this approach, that's fine. I'll change it, no 
worries.

Re: [PATCH v8 1/2] iommu - Enable debugfs exposure of IOMMU driver internals

[Greg KH]

On Tue, Jun 05, 2018 at 12:01:41PM -0500, Gary R Hook wrote:
> > > +/**
> > > + * iommu_debugfs_new_driver_dir - create a vendor directory under debugfs/iommu
> > > + * @vendor: name of the vendor-specific subdirectory to create
> > > + *
> > > + * This function is called by an IOMMU driver to create the top-level debugfs
> > > + * directory for that driver.
> > > + *
> > > + * Return: upon success, a pointer to the dentry for the new directory.
> > > + *         NULL in case of failure.
> > > + */
> > > +struct dentry *iommu_debugfs_new_driver_dir(const char *vendor)
> > > +{
> > > +	struct dentry *d_new;
> > > +
> > > +	d_new = debugfs_create_dir(vendor, iommu_debugfs_dir);
> > > +
> > > +	return d_new;
> > > +}
> > > +EXPORT_SYMBOL_GPL(iommu_debugfs_new_driver_dir);
> > 
> > Why are you wrapping a debugfs call?  Why not just export
> > iommu_debugfs_dir instead?
> 
> It was a choice, as I stated in my other post. It is not a requirement.
> If you resolutely reject this approach, that's fine. I'll change it, no
> worries.

Either is fine, but if it stays, it should stay a single line function
:)

thanks,

greg k-h

Re: [PATCH v8 1/2] iommu - Enable debugfs exposure of IOMMU driver internals

[Gary R Hook]

On 06/05/2018 12:08 PM, Greg KH wrote:
> On Tue, Jun 05, 2018 at 12:01:41PM -0500, Gary R Hook wrote:
>>>> +/**
>>>> + * iommu_debugfs_new_driver_dir - create a vendor directory under debugfs/iommu
>>>> + * @vendor: name of the vendor-specific subdirectory to create
>>>> + *
>>>> + * This function is called by an IOMMU driver to create the top-level debugfs
>>>> + * directory for that driver.
>>>> + *
>>>> + * Return: upon success, a pointer to the dentry for the new directory.
>>>> + *         NULL in case of failure.
>>>> + */
>>>> +struct dentry *iommu_debugfs_new_driver_dir(const char *vendor)
>>>> +{
>>>> +	struct dentry *d_new;
>>>> +
>>>> +	d_new = debugfs_create_dir(vendor, iommu_debugfs_dir);
>>>> +
>>>> +	return d_new;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(iommu_debugfs_new_driver_dir);
>>>
>>> Why are you wrapping a debugfs call?  Why not just export
>>> iommu_debugfs_dir instead?
>>
>> It was a choice, as I stated in my other post. It is not a requirement.
>> If you resolutely reject this approach, that's fine. I'll change it, no
>> worries.
> 
> Either is fine, but if it stays, it should stay a single line function
> :)
> 
> thanks,
> 
> greg k-h
> 

Then I shall leave it as a black-box function. Single line, of course.

Thank you.

[PATCH v8 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU

[Gary R Hook]

Implement a skeleton framework for debugfs support in the
AMD IOMMU. Add a hidden boolean to Kconfig that is defined
for the AMD IOMMU when general IOMMY DebugFS support is
enabled.

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

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index f9af25ac409f..ec223f6f4ad4 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -137,6 +137,10 @@ config AMD_IOMMU
 	  your BIOS for an option to enable it or if you have an IVRS ACPI
 	  table.
 
+config AMD_IOMMU_DEBUGFS
+	def_bool y
+	depends on AMD_IOMMU && IOMMU_DEBUGFS
+
 config AMD_IOMMU_V2
 	tristate "AMD IOMMU Version 2 driver"
 	depends on AMD_IOMMU
diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
index 74cfbc392862..47fd6ea9de2d 100644
--- a/drivers/iommu/Makefile
+++ b/drivers/iommu/Makefile
@@ -11,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_DEBUGFS) += 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..6dff98552969
--- /dev/null
+++ b/drivers/iommu/amd_iommu_debugfs.c
@@ -0,0 +1,39 @@
+// 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/debugfs.h>
+#include <linux/iommu.h>
+#include <linux/pci.h>
+#include "amd_iommu_proto.h"
+#include "amd_iommu_types.h"
+
+static struct dentry *amd_iommu_debugfs;
+static DEFINE_MUTEX(amd_iommu_debugfs_lock);
+
+#define	MAX_NAME_LEN	20
+
+void amd_iommu_debugfs_setup(struct amd_iommu *iommu)
+{
+	char name[MAX_NAME_LEN + 1];
+
+	mutex_lock(&amd_iommu_debugfs_lock);
+	if (!amd_iommu_debugfs)
+		amd_iommu_debugfs = iommu_debugfs_new_driver_dir("amd");
+	mutex_unlock(&amd_iommu_debugfs_lock);
+
+	if (amd_iommu_debugfs) {
+		snprintf(name, MAX_NAME_LEN, "iommu%02d", iommu->index);
+		iommu->debugfs = debugfs_create_dir(name,
+						    amd_iommu_debugfs);
+		if (!iommu->debugfs) {
+			debugfs_remove_recursive(amd_iommu_debugfs);
+			amd_iommu_debugfs = NULL;
+		}
+	}
+}
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 904c575d1677..031e6dbb8345 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -2721,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);
@@ -2730,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;
 }
 
diff --git a/drivers/iommu/amd_iommu_proto.h b/drivers/iommu/amd_iommu_proto.h
index 640c286a0ab9..a8cd0296fb16 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_AMD_IOMMU_DEBUGFS
+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 986cbe0cc189..cfac9d842b0f 100644
--- a/drivers/iommu/amd_iommu_types.h
+++ b/drivers/iommu/amd_iommu_types.h
@@ -594,6 +594,11 @@ struct amd_iommu {
 
 	u32 flags;
 	volatile u64 __aligned(8) cmd_sem;
+
+#ifdef CONFIG_AMD_IOMMU_DEBUGFS
+	/* DebugFS Info */
+	struct dentry *debugfs;
+#endif
 };
 
 static inline struct amd_iommu *dev_to_amd_iommu(struct device *dev)

Re: [PATCH v8 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU

[Greg KH]

On Tue, May 29, 2018 at 01:23:23PM -0500, Gary R Hook wrote:
> Implement a skeleton framework for debugfs support in the
> AMD IOMMU. Add a hidden boolean to Kconfig that is defined
> for the AMD IOMMU when general IOMMY DebugFS support is
> enabled.
> 
> Signed-off-by: Gary R Hook <gary.hook@amd.com>
> ---
>  drivers/iommu/Kconfig             |    4 ++++
>  drivers/iommu/Makefile            |    1 +
>  drivers/iommu/amd_iommu_debugfs.c |   39 +++++++++++++++++++++++++++++++++++++
>  drivers/iommu/amd_iommu_init.c    |    6 ++++--
>  drivers/iommu/amd_iommu_proto.h   |    6 ++++++
>  drivers/iommu/amd_iommu_types.h   |    5 +++++
>  6 files changed, 59 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/iommu/amd_iommu_debugfs.c
> 
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index f9af25ac409f..ec223f6f4ad4 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -137,6 +137,10 @@ config AMD_IOMMU
>  	  your BIOS for an option to enable it or if you have an IVRS ACPI
>  	  table.
>  
> +config AMD_IOMMU_DEBUGFS
> +	def_bool y

Why default y?  Can you not boot a box without this?  If not, it should
not be Y.

> +	depends on AMD_IOMMU && IOMMU_DEBUGFS
> +
>  config AMD_IOMMU_V2
>  	tristate "AMD IOMMU Version 2 driver"
>  	depends on AMD_IOMMU
> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
> index 74cfbc392862..47fd6ea9de2d 100644
> --- a/drivers/iommu/Makefile
> +++ b/drivers/iommu/Makefile
> @@ -11,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_DEBUGFS) += 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..6dff98552969
> --- /dev/null
> +++ b/drivers/iommu/amd_iommu_debugfs.c
> @@ -0,0 +1,39 @@
> +// 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/debugfs.h>
> +#include <linux/iommu.h>
> +#include <linux/pci.h>
> +#include "amd_iommu_proto.h"
> +#include "amd_iommu_types.h"
> +
> +static struct dentry *amd_iommu_debugfs;
> +static DEFINE_MUTEX(amd_iommu_debugfs_lock);
> +
> +#define	MAX_NAME_LEN	20
> +
> +void amd_iommu_debugfs_setup(struct amd_iommu *iommu)
> +{
> +	char name[MAX_NAME_LEN + 1];
> +
> +	mutex_lock(&amd_iommu_debugfs_lock);
> +	if (!amd_iommu_debugfs)
> +		amd_iommu_debugfs = iommu_debugfs_new_driver_dir("amd");
> +	mutex_unlock(&amd_iommu_debugfs_lock);
> +
> +	if (amd_iommu_debugfs) {

Do not test this.

> +		snprintf(name, MAX_NAME_LEN, "iommu%02d", iommu->index);
> +		iommu->debugfs = debugfs_create_dir(name,
> +						    amd_iommu_debugfs);
> +		if (!iommu->debugfs) {

No, you don't care about the return value of any debugfs call.  Just
save the directory and move on.  If it's not correct, find, nothing is
wrong, just keep going.

thanks,

greg k-h

Re: [PATCH v8 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU

[Randy Dunlap]

On 05/29/2018 11:39 AM, Greg KH wrote:
> On Tue, May 29, 2018 at 01:23:23PM -0500, Gary R Hook wrote:
>> Implement a skeleton framework for debugfs support in the
>> AMD IOMMU. Add a hidden boolean to Kconfig that is defined
>> for the AMD IOMMU when general IOMMY DebugFS support is
>> enabled.
>>
>> Signed-off-by: Gary R Hook <gary.hook@amd.com>
>> ---
>>  drivers/iommu/Kconfig             |    4 ++++
>>  drivers/iommu/Makefile            |    1 +
>>  drivers/iommu/amd_iommu_debugfs.c |   39 +++++++++++++++++++++++++++++++++++++
>>  drivers/iommu/amd_iommu_init.c    |    6 ++++--
>>  drivers/iommu/amd_iommu_proto.h   |    6 ++++++
>>  drivers/iommu/amd_iommu_types.h   |    5 +++++
>>  6 files changed, 59 insertions(+), 2 deletions(-)
>>  create mode 100644 drivers/iommu/amd_iommu_debugfs.c
>>
>> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
>> index f9af25ac409f..ec223f6f4ad4 100644
>> --- a/drivers/iommu/Kconfig
>> +++ b/drivers/iommu/Kconfig
>> @@ -137,6 +137,10 @@ config AMD_IOMMU
>>  	  your BIOS for an option to enable it or if you have an IVRS ACPI
>>  	  table.
>>  
>> +config AMD_IOMMU_DEBUGFS
>> +	def_bool y
> 
> Why default y?  Can you not boot a box without this?  If not, it should
> not be Y.
> 
>> +	depends on AMD_IOMMU && IOMMU_DEBUGFS
>> +
>>  config AMD_IOMMU_V2
>>  	tristate "AMD IOMMU Version 2 driver"
>>  	depends on AMD_IOMMU

Gary,

By far, most driver-debugfs additions are optional and include a user Kconfig prompt
so that user's can choose whether to enable it or not.

I suggest that the way forward is to fix Greg's debugfs_() api comments
and to add a prompt string to AMD_IOMMU_DEBUGFS.


-- 
~Randy

Re: [PATCH v8 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU

[Gary R Hook]

On 06/04/2018 08:23 PM, Randy Dunlap wrote:
> On 05/29/2018 11:39 AM, Greg KH wrote:
>> On Tue, May 29, 2018 at 01:23:23PM -0500, Gary R Hook wrote:
>>> Implement a skeleton framework for debugfs support in the
>>> AMD IOMMU. Add a hidden boolean to Kconfig that is defined
>>> for the AMD IOMMU when general IOMMY DebugFS support is
>>> enabled.
>>>
>>> Signed-off-by: Gary R Hook <gary.hook@amd.com>
>>> ---
>>>   drivers/iommu/Kconfig             |    4 ++++
>>>   drivers/iommu/Makefile            |    1 +
>>>   drivers/iommu/amd_iommu_debugfs.c |   39 +++++++++++++++++++++++++++++++++++++
>>>   drivers/iommu/amd_iommu_init.c    |    6 ++++--
>>>   drivers/iommu/amd_iommu_proto.h   |    6 ++++++
>>>   drivers/iommu/amd_iommu_types.h   |    5 +++++
>>>   6 files changed, 59 insertions(+), 2 deletions(-)
>>>   create mode 100644 drivers/iommu/amd_iommu_debugfs.c
>>>
>>> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
>>> index f9af25ac409f..ec223f6f4ad4 100644
>>> --- a/drivers/iommu/Kconfig
>>> +++ b/drivers/iommu/Kconfig
>>> @@ -137,6 +137,10 @@ config AMD_IOMMU
>>>   	  your BIOS for an option to enable it or if you have an IVRS ACPI
>>>   	  table.
>>>   
>>> +config AMD_IOMMU_DEBUGFS
>>> +	def_bool y
>>
>> Why default y?  Can you not boot a box without this?  If not, it should
>> not be Y.
>>
>>> +	depends on AMD_IOMMU && IOMMU_DEBUGFS
>>> +
>>>   config AMD_IOMMU_V2
>>>   	tristate "AMD IOMMU Version 2 driver"
>>>   	depends on AMD_IOMMU
> 
> Gary,
> 
> By far, most driver-debugfs additions are optional and include a user Kconfig prompt
> so that user's can choose whether to enable it or not.
> 
> I suggest that the way forward is to fix Greg's debugfs_() api comments
> and to add a prompt string to AMD_IOMMU_DEBUGFS.

Roger. I think we have that all worked out. Will send another version soon.

Thanks.

Re: [PATCH v8 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU

[Gary R Hook]

On 05/29/2018 01:39 PM, Greg KH wrote:
> On Tue, May 29, 2018 at 01:23:23PM -0500, Gary R Hook wrote:
>> Implement a skeleton framework for debugfs support in the
>> AMD IOMMU. Add a hidden boolean to Kconfig that is defined
>> for the AMD IOMMU when general IOMMY DebugFS support is
>> enabled.
>>
>> Signed-off-by: Gary R Hook <gary.hook@amd.com>
>> ---
>>   drivers/iommu/Kconfig             |    4 ++++
>>   drivers/iommu/Makefile            |    1 +
>>   drivers/iommu/amd_iommu_debugfs.c |   39 +++++++++++++++++++++++++++++++++++++
>>   drivers/iommu/amd_iommu_init.c    |    6 ++++--
>>   drivers/iommu/amd_iommu_proto.h   |    6 ++++++
>>   drivers/iommu/amd_iommu_types.h   |    5 +++++
>>   6 files changed, 59 insertions(+), 2 deletions(-)
>>   create mode 100644 drivers/iommu/amd_iommu_debugfs.c
>>
>> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
>> index f9af25ac409f..ec223f6f4ad4 100644
>> --- a/drivers/iommu/Kconfig
>> +++ b/drivers/iommu/Kconfig
>> @@ -137,6 +137,10 @@ config AMD_IOMMU
>>   	  your BIOS for an option to enable it or if you have an IVRS ACPI
>>   	  table.
>>   
>> +config AMD_IOMMU_DEBUGFS
>> +	def_bool y
> 
> Why default y?  Can you not boot a box without this?  If not, it should
> not be Y.

Again, apologies for not seeing this sooner.

Yes, the system can boot without this. The idea of a hidden option was 
surfaced by Robin, and after my first approach was shot down, I tried this.

Logic: If the over-arching IOMMU debugfs option is enabled, then 
AMD_IOMMU_DEBUGFS gets defined, and AMD IOMMU code gets included.

This issue was discussed a few weeks ago. No single approach appears to 
satisfy everyone. I like this because it depends upon one switch: Do you 
want DebugFS support enabled in the IOMMU driver, period? 
Vendor-specific code can then choose to implement support or not, and a 
builder doesn't have to worry about enabling/disabling multiple Kconfig 
options.

At least, that was my line of reasoning.

I'm not married to any approach, and I don't find clever use of Kconfig 
options too terribly challenging. And I'm not defending, I'm just 
explaining.



> 
>> +	depends on AMD_IOMMU && IOMMU_DEBUGFS
>> +
>>   config AMD_IOMMU_V2
>>   	tristate "AMD IOMMU Version 2 driver"
>>   	depends on AMD_IOMMU
>> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
>> index 74cfbc392862..47fd6ea9de2d 100644
>> --- a/drivers/iommu/Makefile
>> +++ b/drivers/iommu/Makefile
>> @@ -11,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_DEBUGFS) += 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..6dff98552969
>> --- /dev/null
>> +++ b/drivers/iommu/amd_iommu_debugfs.c
>> @@ -0,0 +1,39 @@
>> +// 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/debugfs.h>
>> +#include <linux/iommu.h>
>> +#include <linux/pci.h>
>> +#include "amd_iommu_proto.h"
>> +#include "amd_iommu_types.h"
>> +
>> +static struct dentry *amd_iommu_debugfs;
>> +static DEFINE_MUTEX(amd_iommu_debugfs_lock);
>> +
>> +#define	MAX_NAME_LEN	20
>> +
>> +void amd_iommu_debugfs_setup(struct amd_iommu *iommu)
>> +{
>> +	char name[MAX_NAME_LEN + 1];
>> +
>> +	mutex_lock(&amd_iommu_debugfs_lock);
>> +	if (!amd_iommu_debugfs)
>> +		amd_iommu_debugfs = iommu_debugfs_new_driver_dir("amd");
>> +	mutex_unlock(&amd_iommu_debugfs_lock);
>> +
>> +	if (amd_iommu_debugfs) {
> 
> Do not test this.

Okay, noted. I'll stop worrying about this.

> 
>> +		snprintf(name, MAX_NAME_LEN, "iommu%02d", iommu->index);
>> +		iommu->debugfs = debugfs_create_dir(name,
>> +						    amd_iommu_debugfs);
>> +		if (!iommu->debugfs) {
> 
> No, you don't care about the return value of any debugfs call.  Just
> save the directory and move on.  If it's not correct, find, nothing is
> wrong, just keep going.

Roger.

Thank you,
Gary

Re: [PATCH v8 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU

[Greg KH]

On Tue, Jun 05, 2018 at 11:58:13AM -0500, Gary R Hook wrote:
> On 05/29/2018 01:39 PM, Greg KH wrote:
> > On Tue, May 29, 2018 at 01:23:23PM -0500, Gary R Hook wrote:
> > > Implement a skeleton framework for debugfs support in the
> > > AMD IOMMU. Add a hidden boolean to Kconfig that is defined
> > > for the AMD IOMMU when general IOMMY DebugFS support is
> > > enabled.
> > > 
> > > Signed-off-by: Gary R Hook <gary.hook@amd.com>
> > > ---
> > >   drivers/iommu/Kconfig             |    4 ++++
> > >   drivers/iommu/Makefile            |    1 +
> > >   drivers/iommu/amd_iommu_debugfs.c |   39 +++++++++++++++++++++++++++++++++++++
> > >   drivers/iommu/amd_iommu_init.c    |    6 ++++--
> > >   drivers/iommu/amd_iommu_proto.h   |    6 ++++++
> > >   drivers/iommu/amd_iommu_types.h   |    5 +++++
> > >   6 files changed, 59 insertions(+), 2 deletions(-)
> > >   create mode 100644 drivers/iommu/amd_iommu_debugfs.c
> > > 
> > > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> > > index f9af25ac409f..ec223f6f4ad4 100644
> > > --- a/drivers/iommu/Kconfig
> > > +++ b/drivers/iommu/Kconfig
> > > @@ -137,6 +137,10 @@ config AMD_IOMMU
> > >   	  your BIOS for an option to enable it or if you have an IVRS ACPI
> > >   	  table.
> > > +config AMD_IOMMU_DEBUGFS
> > > +	def_bool y
> > 
> > Why default y?  Can you not boot a box without this?  If not, it should
> > not be Y.
> 
> Again, apologies for not seeing this sooner.
> 
> Yes, the system can boot without this. The idea of a hidden option was
> surfaced by Robin, and after my first approach was shot down, I tried this.
> 
> Logic: If the over-arching IOMMU debugfs option is enabled, then
> AMD_IOMMU_DEBUGFS gets defined, and AMD IOMMU code gets included.
> 
> This issue was discussed a few weeks ago. No single approach appears to
> satisfy everyone. I like this because it depends upon one switch: Do you
> want DebugFS support enabled in the IOMMU driver, period? Vendor-specific
> code can then choose to implement support or not, and a builder doesn't have
> to worry about enabling/disabling multiple Kconfig options.
> 
> At least, that was my line of reasoning.
> 
> I'm not married to any approach, and I don't find clever use of Kconfig
> options too terribly challenging. And I'm not defending, I'm just
> explaining.

The issue is, no one sets Kconfig options except a very tiny subset of
kernel developers.  Distros allways enable everything, as they have to
do that.

If you are creating something here that is so dangerous that you spam
the kernel log with big warning messages, you should not be making it
easy to enable, let alone be enabled by default :)

Just make it an option, have it rely on the kernel debugging option, and
say "DO NOT ENABLE THIS UNLESS YOU REALLY REALLY REALLY KNOW WHAT YOU
ARE DOING!"

thanks,

greg k-h

Re: [PATCH v8 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU

[Gary R Hook]

On 06/05/2018 12:06 PM, Greg KH wrote:
> On Tue, Jun 05, 2018 at 11:58:13AM -0500, Gary R Hook wrote:
>> On 05/29/2018 01:39 PM, Greg KH wrote:
>>> On Tue, May 29, 2018 at 01:23:23PM -0500, Gary R Hook wrote:
>>>> Implement a skeleton framework for debugfs support in the
>>>> AMD IOMMU. Add a hidden boolean to Kconfig that is defined
>>>> for the AMD IOMMU when general IOMMY DebugFS support is
>>>> enabled.
>>>>
>>>> Signed-off-by: Gary R Hook <gary.hook@amd.com>
>>>> ---
>>>>    drivers/iommu/Kconfig             |    4 ++++
>>>>    drivers/iommu/Makefile            |    1 +
>>>>    drivers/iommu/amd_iommu_debugfs.c |   39 +++++++++++++++++++++++++++++++++++++
>>>>    drivers/iommu/amd_iommu_init.c    |    6 ++++--
>>>>    drivers/iommu/amd_iommu_proto.h   |    6 ++++++
>>>>    drivers/iommu/amd_iommu_types.h   |    5 +++++
>>>>    6 files changed, 59 insertions(+), 2 deletions(-)
>>>>    create mode 100644 drivers/iommu/amd_iommu_debugfs.c
>>>>
>>>> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
>>>> index f9af25ac409f..ec223f6f4ad4 100644
>>>> --- a/drivers/iommu/Kconfig
>>>> +++ b/drivers/iommu/Kconfig
>>>> @@ -137,6 +137,10 @@ config AMD_IOMMU
>>>>    	  your BIOS for an option to enable it or if you have an IVRS ACPI
>>>>    	  table.
>>>> +config AMD_IOMMU_DEBUGFS
>>>> +	def_bool y
>>>
>>> Why default y?  Can you not boot a box without this?  If not, it should
>>> not be Y.
>>
>> Again, apologies for not seeing this sooner.
>>
>> Yes, the system can boot without this. The idea of a hidden option was
>> surfaced by Robin, and after my first approach was shot down, I tried this.
>>
>> Logic: If the over-arching IOMMU debugfs option is enabled, then
>> AMD_IOMMU_DEBUGFS gets defined, and AMD IOMMU code gets included.
>>
>> This issue was discussed a few weeks ago. No single approach appears to
>> satisfy everyone. I like this because it depends upon one switch: Do you
>> want DebugFS support enabled in the IOMMU driver, period? Vendor-specific
>> code can then choose to implement support or not, and a builder doesn't have
>> to worry about enabling/disabling multiple Kconfig options.
>>
>> At least, that was my line of reasoning.
>>
>> I'm not married to any approach, and I don't find clever use of Kconfig
>> options too terribly challenging. And I'm not defending, I'm just
>> explaining.
> 
> The issue is, no one sets Kconfig options except a very tiny subset of
> kernel developers.  Distros allways enable everything, as they have to
> do that.
> 
> If you are creating something here that is so dangerous that you spam
> the kernel log with big warning messages, you should not be making it
> easy to enable, let alone be enabled by default :)

Okay, I get that. Totally understand.

> Just make it an option, have it rely on the kernel debugging option, and
> say "DO NOT ENABLE THIS UNLESS YOU REALLY REALLY REALLY KNOW WHAT YOU
> ARE DOING!"

Nah, Randy voted for separate options per device, on top of the IOMMU 
option. So I'll go with that. With loud messages, of course.