* [PATCH v7 0/2] Base enablement of IOMMU debugfs support
@ 2018-05-14 17:20 ` Gary R Hook
0 siblings, 0 replies; 25+ messages in thread
From: Gary R Hook @ 2018-05-14 17:20 UTC (permalink / raw)
To: iommu; +Cc: joro, linux-kernel
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-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 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 | 10 +++++
drivers/iommu/Makefile | 6 +++
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 | 3 ++
drivers/iommu/iommu-debugfs.c | 72 +++++++++++++++++++++++++++++++++++++
drivers/iommu/iommu.c | 2 +
include/linux/iommu.h | 11 ++++++
9 files changed, 153 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] 25+ messages in thread* [PATCH v7 0/2] Base enablement of IOMMU debugfs support @ 2018-05-14 17:20 ` Gary R Hook 0 siblings, 0 replies; 25+ messages in thread From: Gary R Hook @ 2018-05-14 17:20 UTC (permalink / raw) To: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA 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-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 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 | 10 +++++ drivers/iommu/Makefile | 6 +++ 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 | 3 ++ drivers/iommu/iommu-debugfs.c | 72 +++++++++++++++++++++++++++++++++++++ drivers/iommu/iommu.c | 2 + include/linux/iommu.h | 11 ++++++ 9 files changed, 153 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] 25+ messages in thread
* [PATCH v7 1/2] iommu - Enable debugfs exposure of IOMMU driver internals @ 2018-05-14 17:20 ` Gary R Hook 0 siblings, 0 replies; 25+ messages in thread From: Gary R Hook @ 2018-05-14 17:20 UTC (permalink / raw) To: iommu; +Cc: joro, linux-kernel 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 f3a21343e636..2eab6a849a0a 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 */ ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v7 1/2] iommu - Enable debugfs exposure of IOMMU driver internals @ 2018-05-14 17:20 ` Gary R Hook 0 siblings, 0 replies; 25+ messages in thread From: Gary R Hook @ 2018-05-14 17:20 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 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-5C7GfCeVMHo@public.gmane.org> --- 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 f3a21343e636..2eab6a849a0a 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-5C7GfCeVMHo@public.gmane.org> + */ + +#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 */ ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v7 1/2] iommu - Enable debugfs exposure of IOMMU driver internals @ 2018-05-24 9:18 ` Greg KH 0 siblings, 0 replies; 25+ messages in thread From: Greg KH @ 2018-05-24 9:18 UTC (permalink / raw) To: Gary R Hook; +Cc: iommu, joro, linux-kernel On Mon, May 14, 2018 at 12:20:20PM -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 f3a21343e636..2eab6a849a0a 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 need a big "DO NOT ENABLE THIS OPTION UNLESS YOU REALLY REALLY KNOW WHAT YOU ARE DOING!!!" line here. To match up with your crazy kernel log warning. Otherwise distros will turn this on, I guarantee it. > + > 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) { No need to care about the return value, just keep going. Nothing you should, or should not, do depending on the return value of a debugfs call. > + 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; This function can be reduced to 1 line. But really, why do you need it at all? thanks, greg k-h ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v7 1/2] iommu - Enable debugfs exposure of IOMMU driver internals @ 2018-05-24 9:18 ` Greg KH 0 siblings, 0 replies; 25+ messages in thread From: Greg KH @ 2018-05-24 9:18 UTC (permalink / raw) To: Gary R Hook Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, linux-kernel-u79uwXL29TY76Z2rM5mHXA On Mon, May 14, 2018 at 12:20:20PM -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-5C7GfCeVMHo@public.gmane.org> > --- > 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 f3a21343e636..2eab6a849a0a 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 need a big "DO NOT ENABLE THIS OPTION UNLESS YOU REALLY REALLY KNOW WHAT YOU ARE DOING!!!" line here. To match up with your crazy kernel log warning. Otherwise distros will turn this on, I guarantee it. > + > 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-5C7GfCeVMHo@public.gmane.org> > + */ > + > +#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) { No need to care about the return value, just keep going. Nothing you should, or should not, do depending on the return value of a debugfs call. > + 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; This function can be reduced to 1 line. But really, why do you need it at all? thanks, greg k-h ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v7 1/2] iommu - Enable debugfs exposure of IOMMU driver internals @ 2018-06-05 16:48 ` Gary R Hook 0 siblings, 0 replies; 25+ messages in thread From: Gary R Hook @ 2018-06-05 16:48 UTC (permalink / raw) To: Greg KH; +Cc: iommu, joro, linux-kernel On 05/24/2018 04:18 AM, Greg KH wrote: > On Mon, May 14, 2018 at 12:20:20PM -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 f3a21343e636..2eab6a849a0a 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 need a big "DO NOT ENABLE THIS OPTION UNLESS YOU REALLY REALLY KNOW > WHAT YOU ARE DOING!!!" line here. To match up with your crazy kernel > log warning. > > Otherwise distros will turn this on, I guarantee it. Apologies for not seeing this. Notes from Greg have been going to junk mail >:-( I will add this text to the Kconfig item. > >> + >> 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) { > > No need to care about the return value, just keep going. Nothing you > should, or should not, do depending on the return value of a debugfs > call. Sorry. Some habits are hard to break. It just seemed that if there's no iommu debugfs directory, nothing else needed to be done. But plowing ahead works for me. Thank you. > >> + 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; > > This function can be reduced to 1 line. But really, why do you need it > at all? My intention was to not expose the base dentry. Which means that drivers need to call in to this file to create a vendor-specific subdirectory. At least, that was my take-away from the discussion. But you dislike this approach to the code, so I'm open to suggestions. At the very least, it's now a one liner. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v7 1/2] iommu - Enable debugfs exposure of IOMMU driver internals @ 2018-06-05 16:48 ` Gary R Hook 0 siblings, 0 replies; 25+ messages in thread From: Gary R Hook @ 2018-06-05 16:48 UTC (permalink / raw) To: Greg KH Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, linux-kernel-u79uwXL29TY76Z2rM5mHXA On 05/24/2018 04:18 AM, Greg KH wrote: > On Mon, May 14, 2018 at 12:20:20PM -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-5C7GfCeVMHo@public.gmane.org> >> --- >> 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 f3a21343e636..2eab6a849a0a 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 need a big "DO NOT ENABLE THIS OPTION UNLESS YOU REALLY REALLY KNOW > WHAT YOU ARE DOING!!!" line here. To match up with your crazy kernel > log warning. > > Otherwise distros will turn this on, I guarantee it. Apologies for not seeing this. Notes from Greg have been going to junk mail >:-( I will add this text to the Kconfig item. > >> + >> 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-5C7GfCeVMHo@public.gmane.org> >> + */ >> + >> +#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) { > > No need to care about the return value, just keep going. Nothing you > should, or should not, do depending on the return value of a debugfs > call. Sorry. Some habits are hard to break. It just seemed that if there's no iommu debugfs directory, nothing else needed to be done. But plowing ahead works for me. Thank you. > >> + 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; > > This function can be reduced to 1 line. But really, why do you need it > at all? My intention was to not expose the base dentry. Which means that drivers need to call in to this file to create a vendor-specific subdirectory. At least, that was my take-away from the discussion. But you dislike this approach to the code, so I'm open to suggestions. At the very least, it's now a one liner. ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v7 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU @ 2018-05-14 17:20 ` Gary R Hook 0 siblings, 0 replies; 25+ messages in thread From: Gary R Hook @ 2018-05-14 17:20 UTC (permalink / raw) To: iommu; +Cc: joro, 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/Makefile | 5 +++++ 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 | 3 +++ 5 files changed, 57 insertions(+), 2 deletions(-) create mode 100644 drivers/iommu/amd_iommu_debugfs.c diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile index 74cfbc392862..dd980f7dd8b6 100644 --- a/drivers/iommu/Makefile +++ b/drivers/iommu/Makefile @@ -30,3 +30,8 @@ obj-$(CONFIG_EXYNOS_IOMMU) += exynos-iommu.o obj-$(CONFIG_FSL_PAMU) += fsl_pamu.o fsl_pamu_domain.o obj-$(CONFIG_S390_IOMMU) += s390-iommu.o obj-$(CONFIG_QCOM_IOMMU) += qcom_iommu.o + +# This ensures that only the required files are compiled +ifeq ($(CONFIG_IOMMU_DEBUGFS), y) +obj-$(CONFIG_AMD_IOMMU) += amd_iommu_debugfs.o +endif 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..39053f11dda3 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_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 1c9b080276c9..2ca0959ae9e6 100644 --- a/drivers/iommu/amd_iommu_types.h +++ b/drivers/iommu/amd_iommu_types.h @@ -593,6 +593,9 @@ struct amd_iommu { u32 flags; volatile u64 __aligned(8) cmd_sem; + + /* DebugFS Info */ + struct dentry *debugfs; }; static inline struct amd_iommu *dev_to_amd_iommu(struct device *dev) ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v7 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU @ 2018-05-14 17:20 ` Gary R Hook 0 siblings, 0 replies; 25+ messages in thread From: Gary R Hook @ 2018-05-14 17:20 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/Makefile | 5 +++++ 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 | 3 +++ 5 files changed, 57 insertions(+), 2 deletions(-) create mode 100644 drivers/iommu/amd_iommu_debugfs.c diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile index 74cfbc392862..dd980f7dd8b6 100644 --- a/drivers/iommu/Makefile +++ b/drivers/iommu/Makefile @@ -30,3 +30,8 @@ obj-$(CONFIG_EXYNOS_IOMMU) += exynos-iommu.o obj-$(CONFIG_FSL_PAMU) += fsl_pamu.o fsl_pamu_domain.o obj-$(CONFIG_S390_IOMMU) += s390-iommu.o obj-$(CONFIG_QCOM_IOMMU) += qcom_iommu.o + +# This ensures that only the required files are compiled +ifeq ($(CONFIG_IOMMU_DEBUGFS), y) +obj-$(CONFIG_AMD_IOMMU) += amd_iommu_debugfs.o +endif 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-5C7GfCeVMHo@public.gmane.org> + */ + +#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..39053f11dda3 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_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 1c9b080276c9..2ca0959ae9e6 100644 --- a/drivers/iommu/amd_iommu_types.h +++ b/drivers/iommu/amd_iommu_types.h @@ -593,6 +593,9 @@ struct amd_iommu { u32 flags; volatile u64 __aligned(8) cmd_sem; + + /* DebugFS Info */ + struct dentry *debugfs; }; static inline struct amd_iommu *dev_to_amd_iommu(struct device *dev) ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v7 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU 2018-05-14 17:20 ` Gary R Hook (?) @ 2018-05-14 17:50 ` Randy Dunlap 2018-05-14 20:00 ` Gary R Hook -1 siblings, 1 reply; 25+ messages in thread From: Randy Dunlap @ 2018-05-14 17:50 UTC (permalink / raw) To: Gary R Hook, iommu; +Cc: joro, linux-kernel On 05/14/2018 10:20 AM, 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/Makefile | 5 +++++ > 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 | 3 +++ > 5 files changed, 57 insertions(+), 2 deletions(-) > create mode 100644 drivers/iommu/amd_iommu_debugfs.c > > diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile > index 74cfbc392862..dd980f7dd8b6 100644 > --- a/drivers/iommu/Makefile > +++ b/drivers/iommu/Makefile > @@ -30,3 +30,8 @@ obj-$(CONFIG_EXYNOS_IOMMU) += exynos-iommu.o > obj-$(CONFIG_FSL_PAMU) += fsl_pamu.o fsl_pamu_domain.o > obj-$(CONFIG_S390_IOMMU) += s390-iommu.o > obj-$(CONFIG_QCOM_IOMMU) += qcom_iommu.o > + > +# This ensures that only the required files are compiled > +ifeq ($(CONFIG_IOMMU_DEBUGFS), y) Most Makefiles don't use a space before the 'y', but since you tested it, I guess either way works. But why do this in the Makefile at all? Why not just add another Kconfig symbol and simplify the Makefile? > +obj-$(CONFIG_AMD_IOMMU) += amd_iommu_debugfs.o > +endif -- ~Randy ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v7 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU @ 2018-05-14 20:00 ` Gary R Hook 0 siblings, 0 replies; 25+ messages in thread From: Gary R Hook @ 2018-05-14 20:00 UTC (permalink / raw) To: Randy Dunlap, iommu; +Cc: joro, linux-kernel On 05/14/2018 12:50 PM, Randy Dunlap wrote: > On 05/14/2018 10:20 AM, 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/Makefile | 5 +++++ >> 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 | 3 +++ >> 5 files changed, 57 insertions(+), 2 deletions(-) >> create mode 100644 drivers/iommu/amd_iommu_debugfs.c >> >> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile >> index 74cfbc392862..dd980f7dd8b6 100644 >> --- a/drivers/iommu/Makefile >> +++ b/drivers/iommu/Makefile >> @@ -30,3 +30,8 @@ obj-$(CONFIG_EXYNOS_IOMMU) += exynos-iommu.o >> obj-$(CONFIG_FSL_PAMU) += fsl_pamu.o fsl_pamu_domain.o >> obj-$(CONFIG_S390_IOMMU) += s390-iommu.o >> obj-$(CONFIG_QCOM_IOMMU) += qcom_iommu.o >> + >> +# This ensures that only the required files are compiled >> +ifeq ($(CONFIG_IOMMU_DEBUGFS), y) > > Most Makefiles don't use a space before the 'y', but since you tested it, > I guess either way works. Pretty sure whitespace isn't used as a delimiter in this construct. I could be mistaken. But yes, it's perfectly serviceable. > But why do this in the Makefile at all? Why not just add another Kconfig > symbol and simplify the Makefile? > >> +obj-$(CONFIG_AMD_IOMMU) += amd_iommu_debugfs.o >> +endif This was brought up a few weeks ago in, I believe, version 3 of this patch. That question was discussed (because that's what I did the first time out), and _someone_ _else_ asked about why I didn't just do it the way I've done it here. Everyone has a preference. I chose to simplify the choices and avoid multiple symbols, instead opting for two switches: choose your device, and decide on Debug FS enablement for it. IMO Very simple. I can't fathom a scenario where this wouldn't work. Is there one? ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v7 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU @ 2018-05-14 20:00 ` Gary R Hook 0 siblings, 0 replies; 25+ messages in thread From: Gary R Hook @ 2018-05-14 20:00 UTC (permalink / raw) To: Randy Dunlap, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA On 05/14/2018 12:50 PM, Randy Dunlap wrote: > On 05/14/2018 10:20 AM, 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/Makefile | 5 +++++ >> 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 | 3 +++ >> 5 files changed, 57 insertions(+), 2 deletions(-) >> create mode 100644 drivers/iommu/amd_iommu_debugfs.c >> >> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile >> index 74cfbc392862..dd980f7dd8b6 100644 >> --- a/drivers/iommu/Makefile >> +++ b/drivers/iommu/Makefile >> @@ -30,3 +30,8 @@ obj-$(CONFIG_EXYNOS_IOMMU) += exynos-iommu.o >> obj-$(CONFIG_FSL_PAMU) += fsl_pamu.o fsl_pamu_domain.o >> obj-$(CONFIG_S390_IOMMU) += s390-iommu.o >> obj-$(CONFIG_QCOM_IOMMU) += qcom_iommu.o >> + >> +# This ensures that only the required files are compiled >> +ifeq ($(CONFIG_IOMMU_DEBUGFS), y) > > Most Makefiles don't use a space before the 'y', but since you tested it, > I guess either way works. Pretty sure whitespace isn't used as a delimiter in this construct. I could be mistaken. But yes, it's perfectly serviceable. > But why do this in the Makefile at all? Why not just add another Kconfig > symbol and simplify the Makefile? > >> +obj-$(CONFIG_AMD_IOMMU) += amd_iommu_debugfs.o >> +endif This was brought up a few weeks ago in, I believe, version 3 of this patch. That question was discussed (because that's what I did the first time out), and _someone_ _else_ asked about why I didn't just do it the way I've done it here. Everyone has a preference. I chose to simplify the choices and avoid multiple symbols, instead opting for two switches: choose your device, and decide on Debug FS enablement for it. IMO Very simple. I can't fathom a scenario where this wouldn't work. Is there one? ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v7 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU @ 2018-05-14 21:00 ` Randy Dunlap 0 siblings, 0 replies; 25+ messages in thread From: Randy Dunlap @ 2018-05-14 21:00 UTC (permalink / raw) To: Gary R Hook, iommu; +Cc: joro, linux-kernel On 05/14/2018 01:00 PM, Gary R Hook wrote: > On 05/14/2018 12:50 PM, Randy Dunlap wrote: >> On 05/14/2018 10:20 AM, 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/Makefile | 5 +++++ >>> 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 | 3 +++ >>> 5 files changed, 57 insertions(+), 2 deletions(-) >>> create mode 100644 drivers/iommu/amd_iommu_debugfs.c >>> >>> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile >>> index 74cfbc392862..dd980f7dd8b6 100644 >>> --- a/drivers/iommu/Makefile >>> +++ b/drivers/iommu/Makefile >>> @@ -30,3 +30,8 @@ obj-$(CONFIG_EXYNOS_IOMMU) += exynos-iommu.o >>> obj-$(CONFIG_FSL_PAMU) += fsl_pamu.o fsl_pamu_domain.o >>> obj-$(CONFIG_S390_IOMMU) += s390-iommu.o >>> obj-$(CONFIG_QCOM_IOMMU) += qcom_iommu.o >>> + >>> +# This ensures that only the required files are compiled >>> +ifeq ($(CONFIG_IOMMU_DEBUGFS), y) >> >> Most Makefiles don't use a space before the 'y', but since you tested it, >> I guess either way works. > > Pretty sure whitespace isn't used as a delimiter in this construct. I could be mistaken. But yes, it's perfectly serviceable. > >> But why do this in the Makefile at all? Why not just add another Kconfig >> symbol and simplify the Makefile? >> >>> +obj-$(CONFIG_AMD_IOMMU) += amd_iommu_debugfs.o >>> +endif > > > This was brought up a few weeks ago in, I believe, version 3 of this patch. That question was discussed (because that's what I did the first time out), and _someone_ _else_ asked about why I didn't just do it the way I've done it here. Sorry I missed it. I would have been glad to chime in at that time. > Everyone has a preference. > > I chose to simplify the choices and avoid multiple symbols, instead opting for two switches: choose your device, and decide on Debug FS enablement for it. IMO Very simple. > > I can't fathom a scenario where this wouldn't work. Is there one? Probably not. But we used to have ugly, messy, convoluted makefiles with very little config help. Then we got better kconfig tools (well, arguably :) and then the makefiles were cleaned up and simplified a lot (or A LOT!). We shouldn't want to go back there. -- ~Randy ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v7 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU @ 2018-05-14 21:00 ` Randy Dunlap 0 siblings, 0 replies; 25+ messages in thread From: Randy Dunlap @ 2018-05-14 21:00 UTC (permalink / raw) To: Gary R Hook, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA On 05/14/2018 01:00 PM, Gary R Hook wrote: > On 05/14/2018 12:50 PM, Randy Dunlap wrote: >> On 05/14/2018 10:20 AM, 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/Makefile | 5 +++++ >>> 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 | 3 +++ >>> 5 files changed, 57 insertions(+), 2 deletions(-) >>> create mode 100644 drivers/iommu/amd_iommu_debugfs.c >>> >>> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile >>> index 74cfbc392862..dd980f7dd8b6 100644 >>> --- a/drivers/iommu/Makefile >>> +++ b/drivers/iommu/Makefile >>> @@ -30,3 +30,8 @@ obj-$(CONFIG_EXYNOS_IOMMU) += exynos-iommu.o >>> obj-$(CONFIG_FSL_PAMU) += fsl_pamu.o fsl_pamu_domain.o >>> obj-$(CONFIG_S390_IOMMU) += s390-iommu.o >>> obj-$(CONFIG_QCOM_IOMMU) += qcom_iommu.o >>> + >>> +# This ensures that only the required files are compiled >>> +ifeq ($(CONFIG_IOMMU_DEBUGFS), y) >> >> Most Makefiles don't use a space before the 'y', but since you tested it, >> I guess either way works. > > Pretty sure whitespace isn't used as a delimiter in this construct. I could be mistaken. But yes, it's perfectly serviceable. > >> But why do this in the Makefile at all? Why not just add another Kconfig >> symbol and simplify the Makefile? >> >>> +obj-$(CONFIG_AMD_IOMMU) += amd_iommu_debugfs.o >>> +endif > > > This was brought up a few weeks ago in, I believe, version 3 of this patch. That question was discussed (because that's what I did the first time out), and _someone_ _else_ asked about why I didn't just do it the way I've done it here. Sorry I missed it. I would have been glad to chime in at that time. > Everyone has a preference. > > I chose to simplify the choices and avoid multiple symbols, instead opting for two switches: choose your device, and decide on Debug FS enablement for it. IMO Very simple. > > I can't fathom a scenario where this wouldn't work. Is there one? Probably not. But we used to have ugly, messy, convoluted makefiles with very little config help. Then we got better kconfig tools (well, arguably :) and then the makefiles were cleaned up and simplified a lot (or A LOT!). We shouldn't want to go back there. -- ~Randy _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v7 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU 2018-05-14 20:00 ` Gary R Hook (?) (?) @ 2018-05-15 13:46 ` Joerg Roedel 2018-05-18 15:20 ` Gary R Hook -1 siblings, 1 reply; 25+ messages in thread From: Joerg Roedel @ 2018-05-15 13:46 UTC (permalink / raw) To: Gary R Hook; +Cc: Randy Dunlap, iommu, linux-kernel On Mon, May 14, 2018 at 03:00:50PM -0500, Gary R Hook wrote: > This was brought up a few weeks ago in, I believe, version 3 of this patch. > That question was discussed (because that's what I did the first time out), > and _someone_ _else_ asked about why I didn't just do it the way I've done > it here. You don't have this problem if you put the code in amd_iommu.c in an IOMMU_DEBUGFS ifdef. Joerg ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v7 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU @ 2018-05-18 15:20 ` Gary R Hook 0 siblings, 0 replies; 25+ messages in thread From: Gary R Hook @ 2018-05-18 15:20 UTC (permalink / raw) To: Joerg Roedel; +Cc: Randy Dunlap, iommu, linux-kernel On 05/15/2018 08:46 AM, Joerg Roedel wrote: > On Mon, May 14, 2018 at 03:00:50PM -0500, Gary R Hook wrote: >> This was brought up a few weeks ago in, I believe, version 3 of this patch. >> That question was discussed (because that's what I did the first time out), >> and _someone_ _else_ asked about why I didn't just do it the way I've done >> it here. > > You don't have this problem if you put the code in amd_iommu.c in an > IOMMU_DEBUGFS ifdef. Of course. My preference, however, is a separate file to avoid size creep. That's why I've done it this way. To whit: there have been threads discussing the advisability/acceptability of using #ifdefs for debug code. My take-away was to avoid them. Perhaps I misunderstood. So: I don't understand your comment. Is this an observation, or is it an imperative statement? I'd like for a maintainer to clearly indicate what is acceptable, and I'll do it. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v7 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU @ 2018-05-18 15:20 ` Gary R Hook 0 siblings, 0 replies; 25+ messages in thread From: Gary R Hook @ 2018-05-18 15:20 UTC (permalink / raw) To: Joerg Roedel Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, Randy Dunlap, linux-kernel-u79uwXL29TY76Z2rM5mHXA On 05/15/2018 08:46 AM, Joerg Roedel wrote: > On Mon, May 14, 2018 at 03:00:50PM -0500, Gary R Hook wrote: >> This was brought up a few weeks ago in, I believe, version 3 of this patch. >> That question was discussed (because that's what I did the first time out), >> and _someone_ _else_ asked about why I didn't just do it the way I've done >> it here. > > You don't have this problem if you put the code in amd_iommu.c in an > IOMMU_DEBUGFS ifdef. Of course. My preference, however, is a separate file to avoid size creep. That's why I've done it this way. To whit: there have been threads discussing the advisability/acceptability of using #ifdefs for debug code. My take-away was to avoid them. Perhaps I misunderstood. So: I don't understand your comment. Is this an observation, or is it an imperative statement? I'd like for a maintainer to clearly indicate what is acceptable, and I'll do it. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v7 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU @ 2018-05-18 16:49 ` Randy Dunlap 0 siblings, 0 replies; 25+ messages in thread From: Randy Dunlap @ 2018-05-18 16:49 UTC (permalink / raw) To: Gary R Hook, Joerg Roedel; +Cc: iommu, linux-kernel On 05/18/2018 08:20 AM, Gary R Hook wrote: > On 05/15/2018 08:46 AM, Joerg Roedel wrote: >> On Mon, May 14, 2018 at 03:00:50PM -0500, Gary R Hook wrote: >>> This was brought up a few weeks ago in, I believe, version 3 of this patch. >>> That question was discussed (because that's what I did the first time out), >>> and _someone_ _else_ asked about why I didn't just do it the way I've done >>> it here. >> >> You don't have this problem if you put the code in amd_iommu.c in an >> IOMMU_DEBUGFS ifdef. > > Of course. My preference, however, is a separate file to avoid size creep. That's why I've done it this way. > > To whit: there have been threads discussing the advisability/acceptability of using #ifdefs for debug code. My take-away was to avoid them. Perhaps I misunderstood. > > So: I don't understand your comment. Is this an observation, or is it an imperative statement? I'd like for a maintainer to clearly indicate what is acceptable, and I'll do it. > > Hi, I looked back at Robin Murphy's comments on April 17: <quote> Well, you could do a makefile-level dependency i.e.: ifeq ($(CONFIG_IOMMU_DEBUG), y) obj-$(CONFIG_AMD_IOMMU) += amd_iommu_debugfs.o obj-$(CONFIG_BLAH_IOMMU) += blah_iommu_debugfs.o ... endif Or alternatively have an intermediate silent Kconfig option: config AMD_IOMMU_DEBUG def_bool y depends on AMD_IOMMU && IOMMU_DEBUG The makefile option is arguably ugly, but does at least scale better ;) </quote> I think the Kconfig option would have been the correct choice. -- ~Randy ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v7 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU @ 2018-05-18 16:49 ` Randy Dunlap 0 siblings, 0 replies; 25+ messages in thread From: Randy Dunlap @ 2018-05-18 16:49 UTC (permalink / raw) To: Gary R Hook, Joerg Roedel Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, linux-kernel-u79uwXL29TY76Z2rM5mHXA On 05/18/2018 08:20 AM, Gary R Hook wrote: > On 05/15/2018 08:46 AM, Joerg Roedel wrote: >> On Mon, May 14, 2018 at 03:00:50PM -0500, Gary R Hook wrote: >>> This was brought up a few weeks ago in, I believe, version 3 of this patch. >>> That question was discussed (because that's what I did the first time out), >>> and _someone_ _else_ asked about why I didn't just do it the way I've done >>> it here. >> >> You don't have this problem if you put the code in amd_iommu.c in an >> IOMMU_DEBUGFS ifdef. > > Of course. My preference, however, is a separate file to avoid size creep. That's why I've done it this way. > > To whit: there have been threads discussing the advisability/acceptability of using #ifdefs for debug code. My take-away was to avoid them. Perhaps I misunderstood. > > So: I don't understand your comment. Is this an observation, or is it an imperative statement? I'd like for a maintainer to clearly indicate what is acceptable, and I'll do it. > > Hi, I looked back at Robin Murphy's comments on April 17: <quote> Well, you could do a makefile-level dependency i.e.: ifeq ($(CONFIG_IOMMU_DEBUG), y) obj-$(CONFIG_AMD_IOMMU) += amd_iommu_debugfs.o obj-$(CONFIG_BLAH_IOMMU) += blah_iommu_debugfs.o ... endif Or alternatively have an intermediate silent Kconfig option: config AMD_IOMMU_DEBUG def_bool y depends on AMD_IOMMU && IOMMU_DEBUG The makefile option is arguably ugly, but does at least scale better ;) </quote> I think the Kconfig option would have been the correct choice. -- ~Randy ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v7 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU 2018-05-18 16:49 ` Randy Dunlap (?) @ 2018-05-18 21:02 ` Gary R Hook 2018-05-24 23:51 ` Gary R Hook 2018-05-29 12:09 ` Joerg Roedel -1 siblings, 2 replies; 25+ messages in thread From: Gary R Hook @ 2018-05-18 21:02 UTC (permalink / raw) To: Randy Dunlap, Joerg Roedel; +Cc: iommu, linux-kernel On 05/18/2018 11:49 AM, Randy Dunlap wrote: > On 05/18/2018 08:20 AM, Gary R Hook wrote: >> On 05/15/2018 08:46 AM, Joerg Roedel wrote: >>> On Mon, May 14, 2018 at 03:00:50PM -0500, Gary R Hook wrote: >>>> This was brought up a few weeks ago in, I believe, version 3 of this patch. >>>> That question was discussed (because that's what I did the first time out), >>>> and _someone_ _else_ asked about why I didn't just do it the way I've done >>>> it here. >>> >>> You don't have this problem if you put the code in amd_iommu.c in an >>> IOMMU_DEBUGFS ifdef. >> >> Of course. My preference, however, is a separate file to avoid size creep. That's why I've done it this way. >> >> To whit: there have been threads discussing the advisability/acceptability of using #ifdefs for debug code. My take-away was to avoid them. Perhaps I misunderstood. >> >> So: I don't understand your comment. Is this an observation, or is it an imperative statement? I'd like for a maintainer to clearly indicate what is acceptable, and I'll do it. >> >> > > Hi, > I looked back at Robin Murphy's comments on April 17: > > <quote> > Well, you could do a makefile-level dependency i.e.: > > ifeq ($(CONFIG_IOMMU_DEBUG), y) > obj-$(CONFIG_AMD_IOMMU) += amd_iommu_debugfs.o > obj-$(CONFIG_BLAH_IOMMU) += blah_iommu_debugfs.o > ... > endif > > Or alternatively have an intermediate silent Kconfig option: > > config AMD_IOMMU_DEBUG > def_bool y > depends on AMD_IOMMU && IOMMU_DEBUG > > The makefile option is arguably ugly, but does at least scale better ;) > </quote> > > > I think the Kconfig option would have been the correct choice. "Preferred", perhaps. Neither is incorrect. And really, the Makefile/Kconfig choice is somewhat separate from the organization issue. So I've made the changes for this. Now I'm waiting on Joerg to make a decision on the code/file organization. I still prefer a separate file for the debug fs code. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v7 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU @ 2018-05-24 23:51 ` Gary R Hook 0 siblings, 0 replies; 25+ messages in thread From: Gary R Hook @ 2018-05-24 23:51 UTC (permalink / raw) To: Joerg Roedel; +Cc: Randy Dunlap, iommu, linux-kernel On 05/18/2018 04:02 PM, Gary R Hook wrote: > On 05/18/2018 11:49 AM, Randy Dunlap wrote: >> I think the Kconfig option would have been the correct choice. > > "Preferred", perhaps. Neither is incorrect. And really, the > Makefile/Kconfig choice is somewhat separate from the organization issue. > > So I've made the changes for this. Now I'm waiting on Joerg to make a > decision on the code/file organization. I still prefer a separate file > for the debug fs code. Joerg: *poke* Any thoughts on this? ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v7 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU @ 2018-05-24 23:51 ` Gary R Hook 0 siblings, 0 replies; 25+ messages in thread From: Gary R Hook @ 2018-05-24 23:51 UTC (permalink / raw) To: Joerg Roedel Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, Randy Dunlap, linux-kernel-u79uwXL29TY76Z2rM5mHXA On 05/18/2018 04:02 PM, Gary R Hook wrote: > On 05/18/2018 11:49 AM, Randy Dunlap wrote: >> I think the Kconfig option would have been the correct choice. > > "Preferred", perhaps. Neither is incorrect. And really, the > Makefile/Kconfig choice is somewhat separate from the organization issue. > > So I've made the changes for this. Now I'm waiting on Joerg to make a > decision on the code/file organization. I still prefer a separate file > for the debug fs code. Joerg: *poke* Any thoughts on this? ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v7 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU @ 2018-05-29 12:09 ` Joerg Roedel 0 siblings, 0 replies; 25+ messages in thread From: Joerg Roedel @ 2018-05-29 12:09 UTC (permalink / raw) To: Gary R Hook; +Cc: Randy Dunlap, iommu, linux-kernel On Fri, May 18, 2018 at 04:02:46PM -0500, Gary R Hook wrote: > "Preferred", perhaps. Neither is incorrect. And really, the Makefile/Kconfig > choice is somewhat separate from the organization issue. > > So I've made the changes for this. Now I'm waiting on Joerg to make a > decision on the code/file organization. I still prefer a separate file for > the debug fs code. The Kconfig option looks nicer to me, please use it. Then you can also put the code into a separate file. Joerg ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v7 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU @ 2018-05-29 12:09 ` Joerg Roedel 0 siblings, 0 replies; 25+ messages in thread From: Joerg Roedel @ 2018-05-29 12:09 UTC (permalink / raw) To: Gary R Hook Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, Randy Dunlap, linux-kernel-u79uwXL29TY76Z2rM5mHXA On Fri, May 18, 2018 at 04:02:46PM -0500, Gary R Hook wrote: > "Preferred", perhaps. Neither is incorrect. And really, the Makefile/Kconfig > choice is somewhat separate from the organization issue. > > So I've made the changes for this. Now I'm waiting on Joerg to make a > decision on the code/file organization. I still prefer a separate file for > the debug fs code. The Kconfig option looks nicer to me, please use it. Then you can also put the code into a separate file. Joerg ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2018-06-05 16:48 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-14 17:20 [PATCH v7 0/2] Base enablement of IOMMU debugfs support Gary R Hook 2018-05-14 17:20 ` Gary R Hook 2018-05-14 17:20 ` [PATCH v7 1/2] iommu - Enable debugfs exposure of IOMMU driver internals Gary R Hook 2018-05-14 17:20 ` Gary R Hook 2018-05-24 9:18 ` Greg KH 2018-05-24 9:18 ` Greg KH 2018-06-05 16:48 ` Gary R Hook 2018-06-05 16:48 ` Gary R Hook 2018-05-14 17:20 ` [PATCH v7 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU Gary R Hook 2018-05-14 17:20 ` Gary R Hook 2018-05-14 17:50 ` Randy Dunlap 2018-05-14 20:00 ` Gary R Hook 2018-05-14 20:00 ` Gary R Hook 2018-05-14 21:00 ` Randy Dunlap 2018-05-14 21:00 ` Randy Dunlap 2018-05-15 13:46 ` Joerg Roedel 2018-05-18 15:20 ` Gary R Hook 2018-05-18 15:20 ` Gary R Hook 2018-05-18 16:49 ` Randy Dunlap 2018-05-18 16:49 ` Randy Dunlap 2018-05-18 21:02 ` Gary R Hook 2018-05-24 23:51 ` Gary R Hook 2018-05-24 23:51 ` Gary R Hook 2018-05-29 12:09 ` Joerg Roedel 2018-05-29 12:09 ` Joerg Roedel
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.