Messages in this thread | | | Subject | Re: [PATCH v6 1/2] iommu - Enable debugfs exposure of IOMMU driver internals | From | Gary R Hook <> | Date | Fri, 11 May 2018 14:41:42 -0500 |
| |
On 05/11/2018 10:22 AM, Robin Murphy wrote: > Hi Gary, > > Just a few trivial nitpicks below, otherwise: > > Reviewed-by: Robin Murphy <robin.murphy@arm.com> > > On 11/05/18 15:34, 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 | 11 ++++++ >> drivers/iommu/Makefile | 1 + >> drivers/iommu/iommu-debugfs.c | 70 >> +++++++++++++++++++++++++++++++++++++++++ >> drivers/iommu/iommu.c | 2 + >> include/linux/iommu.h | 10 ++++++ >> 5 files changed, 94 insertions(+) >> create mode 100644 drivers/iommu/iommu-debugfs.c >> >> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig >> index f3a21343e636..ff511fa8ca7d 100644 >> --- a/drivers/iommu/Kconfig >> +++ b/drivers/iommu/Kconfig >> @@ -60,6 +60,17 @@ config IOMMU_IO_PGTABLE_ARMV7S_SELFTEST >> endmenu >> +config IOMMU_DEBUGFS >> + bool "Export IOMMU internals in DebugFS" >> + depends on DEBUG_FS >> + default n > > bool implicitly defaults to n anyway, so you don't really need to say it.
Roger.
> >> + 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..9df3b44aef55 >> --- /dev/null >> +++ b/drivers/iommu/iommu-debugfs.c >> @@ -0,0 +1,70 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * IOMMU driver > > "driver"? ;) I'd have thought something like "IOMMU debugfs core > infrastructure", but arguably it's self-evident enough that it doesn't > necessarily need describing at all.
Changed to your suggestion
> >> + * >> + * 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. The return value is the dentry for the >> requested >> + * vendor directory, or NULL in case of failure. > > According to kernel-doc.rst, return values should be in a dedicated > section (i.e. "Return: The dentry...") rather than as part of the > function description.
A style change that happened along the way; my model was apparently older code. I've adjusted for this new style.
> >> + */ >> +struct dentry *iommu_debugfs_new_driver_dir(char *vendor) > > const char *?
Sure thing.
> >> +{ >> + 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..25018ac0fdab 100644 >> --- a/include/linux/iommu.h >> +++ b/include/linux/iommu.h >> @@ -698,4 +698,14 @@ 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(char *vendor); >> +#else >> +static inline void iommu_debugfs_setup(void) {} >> +static inline struct dentry *iommu_debugfs_new_driver_dir(char >> *vendor) { \ > > Since this is a function, not a macro, it doesn't really need the line > continuations.
Yep. Done. And moved the curly brace to the next line.
> > Robin. > >> + return NULL; \ >> +} >> +#endif >> + >> #endif /* __LINUX_IOMMU_H */ >> >> _______________________________________________ >> iommu mailing list >> iommu@lists.linux-foundation.org >> https://lists.linuxfoundation.org/mailman/listinfo/iommu >>
| |