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

2018-05-08 Thread Joe Perches 
On Tue, 2018-05-08 at 15:07 -0500, Gary R Hook wrote:
> On 05/08/2018 01:48 PM, Joe Perches wrote:
> > On Tue, 2018-05-08 at 12:08 -0500, Hook, Gary wrote:
> > > On 5/7/2018 6:47 PM, kbuild test robot wrote:
> > > > 
> > > > All error/warnings (new ones prefixed by >>):
> > > > 
> > > >  In file included from include/linux/intel-iommu.h:32:0,
> > > >   from drivers/gpu/drm/i915/i915_drv.h:41,
> > > >   from drivers/gpu/drm/i915/i915_oa_bxt.c:31:
> > > >  include/linux/iommu.h: In function 'iommu_debugfs_new_driver_dir':
> > > > > > include/linux/iommu.h:706:8: error: parameter name omitted
> > > > 
> > > >   struct dentry *iommu_debugfs_new_driver_dir(char *) {};
> > > >  ^~
> > > >  In file included from include/linux/intel-iommu.h:32:0,
> > > >   from drivers/gpu/drm/i915/i915_drv.h:41,
> > > >   from drivers/gpu/drm/i915/i915_oa_bxt.c:31:
> > > > > > include/linux/iommu.h:706:8: warning: control reaches end of 
> > > > > > non-void function [-Wreturn-type]
> > > > 
> > > >   struct dentry *iommu_debugfs_new_driver_dir(char *) {};
> > > >  ^~
> > > > 
> > > > vim +706 include/linux/iommu.h
> > > > 
> > > >  700
> > > >  701#ifdef CONFIG_IOMMU_DEBUGFS
> > > >  702void iommu_debugfs_setup(void);
> > > >  703struct dentry *iommu_debugfs_new_driver_dir(char *);
> > > >  704#else
> > > >  705static inline void iommu_debugfs_setup(void) {}
> > > >> 706struct dentry *iommu_debugfs_new_driver_dir(char *) {};
> > > >  707#endif
> > > >  708
> > > 
> > > I have no problems with adding parameter names. But
> > > scripts/checkpatch.pl doesn't seem to check for this, nor require it.
> > > Should checkpatch be updated?
> > 
> > I'm pretty sure that's not feasible.
> 
> Ugh. This is a definition, not a declaration. My bad. Which is likely 
> why I decided to apologize up front.
> 
> > And when the compiler tells you you've stuffed up some
> > syntactical bit, why should checkpatch duplicate the
> > output error message too?
> 
> Well, that's the point: neither the 4.8 nor 5.4 compiler complained 
> about this.

Perhaps because CONFIG_IOMMU_DEBUGFS was set in the .config
for all the compilation previously performed?

>  Not as an error, despite the fact that (now that I read what 
> is actually here, as opposed to what I think is there) this is wrong. 
> Had an error message been emitted, and the make stopped, I would have 
> figure this out before embarrassing myself in front of the entire interwebs.

There's no reason for that figuring out to be necessary.
Linux compilation complexity is pretty high and almost
no one understands it completely.

cheers, Joe
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

2018-05-08 Thread Gary R Hook 

On 05/08/2018 03:42 PM, Joe Perches wrote:

On Tue, 2018-05-08 at 15:07 -0500, Gary R Hook wrote:

On 05/08/2018 01:48 PM, Joe Perches wrote:

On Tue, 2018-05-08 at 12:08 -0500, Hook, Gary wrote:

On 5/7/2018 6:47 PM, kbuild test robot wrote:


All error/warnings (new ones prefixed by >>):

  In file included from include/linux/intel-iommu.h:32:0,
   from drivers/gpu/drm/i915/i915_drv.h:41,
   from drivers/gpu/drm/i915/i915_oa_bxt.c:31:
  include/linux/iommu.h: In function 'iommu_debugfs_new_driver_dir':

include/linux/iommu.h:706:8: error: parameter name omitted


   struct dentry *iommu_debugfs_new_driver_dir(char *) {};
  ^~
  In file included from include/linux/intel-iommu.h:32:0,
   from drivers/gpu/drm/i915/i915_drv.h:41,
   from drivers/gpu/drm/i915/i915_oa_bxt.c:31:

include/linux/iommu.h:706:8: warning: control reaches end of non-void function 
[-Wreturn-type]


   struct dentry *iommu_debugfs_new_driver_dir(char *) {};
  ^~

vim +706 include/linux/iommu.h

  700   
  701   #ifdef CONFIG_IOMMU_DEBUGFS
  702   void iommu_debugfs_setup(void);
  703   struct dentry *iommu_debugfs_new_driver_dir(char *);
  704   #else
  705   static inline void iommu_debugfs_setup(void) {}
> 706struct dentry *iommu_debugfs_new_driver_dir(char *) {};
  707   #endif
  708   


I have no problems with adding parameter names. But
scripts/checkpatch.pl doesn't seem to check for this, nor require it.
Should checkpatch be updated?


I'm pretty sure that's not feasible.


Ugh. This is a definition, not a declaration. My bad. Which is likely
why I decided to apologize up front.


And when the compiler tells you you've stuffed up some
syntactical bit, why should checkpatch duplicate the
output error message too?


Well, that's the point: neither the 4.8 nor 5.4 compiler complained
about this.


Perhaps because CONFIG_IOMMU_DEBUGFS was set in the .config
for all the compilation previously performed?


Well, you'd think maybe so, but I forced a recompilation of that one 
file (i915_oa_bxt.c) and no complaint with 5.4. Weird.


Ah, well. Onward to patch version 6.

Thanks again.

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

2018-05-08 Thread Gary R Hook 

On 05/08/2018 01:48 PM, Joe Perches wrote:

On Tue, 2018-05-08 at 12:08 -0500, Hook, Gary wrote:

On 5/7/2018 6:47 PM, kbuild test robot wrote:


All error/warnings (new ones prefixed by >>):

 In file included from include/linux/intel-iommu.h:32:0,
  from drivers/gpu/drm/i915/i915_drv.h:41,
  from drivers/gpu/drm/i915/i915_oa_bxt.c:31:
 include/linux/iommu.h: In function 'iommu_debugfs_new_driver_dir':

include/linux/iommu.h:706:8: error: parameter name omitted


  struct dentry *iommu_debugfs_new_driver_dir(char *) {};
 ^~
 In file included from include/linux/intel-iommu.h:32:0,
  from drivers/gpu/drm/i915/i915_drv.h:41,
  from drivers/gpu/drm/i915/i915_oa_bxt.c:31:

include/linux/iommu.h:706:8: warning: control reaches end of non-void function 
[-Wreturn-type]


  struct dentry *iommu_debugfs_new_driver_dir(char *) {};
 ^~

vim +706 include/linux/iommu.h

 700
 701#ifdef CONFIG_IOMMU_DEBUGFS
 702void iommu_debugfs_setup(void);
 703struct dentry *iommu_debugfs_new_driver_dir(char *);
 704#else
 705static inline void iommu_debugfs_setup(void) {}
   > 706 struct dentry *iommu_debugfs_new_driver_dir(char *) {};
 707#endif
 708


I have no problems with adding parameter names. But
scripts/checkpatch.pl doesn't seem to check for this, nor require it.
Should checkpatch be updated?


I'm pretty sure that's not feasible.


Ugh. This is a definition, not a declaration. My bad. Which is likely 
why I decided to apologize up front.



And when the compiler tells you you've stuffed up some
syntactical bit, why should checkpatch duplicate the
output error message too?


Well, that's the point: neither the 4.8 nor 5.4 compiler complained 
about this. Not as an error, despite the fact that (now that I read what 
is actually here, as opposed to what I think is there) this is wrong. 
Had an error message been emitted, and the make stopped, I would have 
figure this out before embarrassing myself in front of the entire interwebs.



btw:  That's an unnecessary ; at the end of that non-void
function and it should probably be something like:


You are correct, sir. I've made a change on this.



static inline struct dentry *iommu_debugfs_new_driver_dir(char *dir)
{
return NULL;
}


Thanks for taking a few moments to comment. Much appreciated.

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

2018-05-08 Thread Joe Perches 
On Tue, 2018-05-08 at 12:08 -0500, Hook, Gary wrote:
> On 5/7/2018 6:47 PM, kbuild test robot wrote:
> > Hi Gary,
> > 
> 
> I imagine these questions have been asked before, so I'll ask for
> forgiveness up front.
> 
> 
> > Thank you for the patch! Yet something to improve:
> > 
> > [auto build test ERROR on iommu/next]
> > [also build test ERROR on v4.17-rc4 next-20180507]
> > [if your patch is applied to the wrong git tree, please drop us a note to 
> > help improve the system]
> > 
> > url:
> > https://github.com/0day-ci/linux/commits/Gary-R-Hook/iommu-Enable-debugfs-exposure-of-IOMMU-driver-internals/20180508-062918
> > base:   https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git next
> > config: x86_64-randconfig-x016-201818 (attached as .config)
> > compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
> > reproduce:
> >  # save the attached .config to linux build tree
> >  make ARCH=x86_64
> 
> Is the gcc 7 compiler now a requirement to build the kernel? Or only to 
> run krobot tests?
> 
> Is this the earliest version of the compiler that can be used? I'm still 
> using 4.8 and 5.4, which seems to suffice for the kernel.
> 
> Googling about this has been fruitless.
> 
> > 
> > All error/warnings (new ones prefixed by >>):
> > 
> > In file included from include/linux/intel-iommu.h:32:0,
> >  from drivers/gpu/drm/i915/i915_drv.h:41,
> >  from drivers/gpu/drm/i915/i915_oa_bxt.c:31:
> > include/linux/iommu.h: In function 'iommu_debugfs_new_driver_dir':
> > > > include/linux/iommu.h:706:8: error: parameter name omitted
> > 
> >  struct dentry *iommu_debugfs_new_driver_dir(char *) {};
> > ^~
> > In file included from include/linux/intel-iommu.h:32:0,
> >  from drivers/gpu/drm/i915/i915_drv.h:41,
> >  from drivers/gpu/drm/i915/i915_oa_bxt.c:31:
> > > > include/linux/iommu.h:706:8: warning: control reaches end of non-void 
> > > > function [-Wreturn-type]
> > 
> >  struct dentry *iommu_debugfs_new_driver_dir(char *) {};
> > ^~
> > 
> > vim +706 include/linux/iommu.h
> > 
> > 700 
> > 701 #ifdef CONFIG_IOMMU_DEBUGFS
> > 702 void iommu_debugfs_setup(void);
> > 703 struct dentry *iommu_debugfs_new_driver_dir(char *);
> > 704 #else
> > 705 static inline void iommu_debugfs_setup(void) {}
> >   > 706 struct dentry *iommu_debugfs_new_driver_dir(char *) {};
> > 707 #endif
> > 708 
> 
> I have no problems with adding parameter names. But 
> scripts/checkpatch.pl doesn't seem to check for this, nor require it. 
> Should checkpatch be updated?

I'm pretty sure that's not feasible.

And when the compiler tells you you've stuffed up some
syntactical bit, why should checkpatch duplicate the
output error message too?

btw:  That's an unnecessary ; at the end of that non-void
function and it should probably be something like:

static inline struct dentry *iommu_debugfs_new_driver_dir(char *dir)
{
return NULL;
}

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

2018-05-08 Thread Hook, Gary 

On 5/7/2018 6:47 PM, kbuild test robot wrote:

Hi Gary,



I imagine these questions have been asked before, so I'll ask for
forgiveness up front.



Thank you for the patch! Yet something to improve:

[auto build test ERROR on iommu/next]
[also build test ERROR on v4.17-rc4 next-20180507]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Gary-R-Hook/iommu-Enable-debugfs-exposure-of-IOMMU-driver-internals/20180508-062918
base:   https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git next
config: x86_64-randconfig-x016-201818 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
 # save the attached .config to linux build tree
 make ARCH=x86_64


Is the gcc 7 compiler now a requirement to build the kernel? Or only to 
run krobot tests?


Is this the earliest version of the compiler that can be used? I'm still 
using 4.8 and 5.4, which seems to suffice for the kernel.


Googling about this has been fruitless.



All error/warnings (new ones prefixed by >>):

In file included from include/linux/intel-iommu.h:32:0,
 from drivers/gpu/drm/i915/i915_drv.h:41,
 from drivers/gpu/drm/i915/i915_oa_bxt.c:31:
include/linux/iommu.h: In function 'iommu_debugfs_new_driver_dir':

include/linux/iommu.h:706:8: error: parameter name omitted

 struct dentry *iommu_debugfs_new_driver_dir(char *) {};
^~
In file included from include/linux/intel-iommu.h:32:0,
 from drivers/gpu/drm/i915/i915_drv.h:41,
 from drivers/gpu/drm/i915/i915_oa_bxt.c:31:

include/linux/iommu.h:706:8: warning: control reaches end of non-void function 
[-Wreturn-type]

 struct dentry *iommu_debugfs_new_driver_dir(char *) {};
^~

vim +706 include/linux/iommu.h

700 
701 #ifdef CONFIG_IOMMU_DEBUGFS
702 void iommu_debugfs_setup(void);
703 struct dentry *iommu_debugfs_new_driver_dir(char *);
704 #else
705 static inline void iommu_debugfs_setup(void) {}
  > 706  struct dentry *iommu_debugfs_new_driver_dir(char *) {};
707 #endif
708 


I have no problems with adding parameter names. But 
scripts/checkpatch.pl doesn't seem to check for this, nor require it. 
Should checkpatch be updated?

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

2018-05-07 Thread kbuild test robot 
Hi Gary,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on iommu/next]
[also build test ERROR on v4.17-rc4 next-20180507]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Gary-R-Hook/iommu-Enable-debugfs-exposure-of-IOMMU-driver-internals/20180508-062918
base:   https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git next
config: x86_64-randconfig-x016-201818 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

All error/warnings (new ones prefixed by >>):

   In file included from include/linux/intel-iommu.h:32:0,
from drivers/gpu/drm/i915/i915_drv.h:41,
from drivers/gpu/drm/i915/i915_oa_bxt.c:31:
   include/linux/iommu.h: In function 'iommu_debugfs_new_driver_dir':
>> include/linux/iommu.h:706:8: error: parameter name omitted
struct dentry *iommu_debugfs_new_driver_dir(char *) {};
   ^~
   In file included from include/linux/intel-iommu.h:32:0,
from drivers/gpu/drm/i915/i915_drv.h:41,
from drivers/gpu/drm/i915/i915_oa_bxt.c:31:
>> include/linux/iommu.h:706:8: warning: control reaches end of non-void 
>> function [-Wreturn-type]
struct dentry *iommu_debugfs_new_driver_dir(char *) {};
   ^~

vim +706 include/linux/iommu.h

   700  
   701  #ifdef CONFIG_IOMMU_DEBUGFS
   702  void iommu_debugfs_setup(void);
   703  struct dentry *iommu_debugfs_new_driver_dir(char *);
   704  #else
   705  static inline void iommu_debugfs_setup(void) {}
 > 706  struct dentry *iommu_debugfs_new_driver_dir(char *) {};
   707  #endif
   708  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

2018-05-07 Thread 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 
---
 drivers/iommu/Kconfig |   11 +++
 drivers/iommu/Makefile|1 +
 drivers/iommu/iommu-debugfs.c |   64 +
 drivers/iommu/iommu.c |2 +
 include/linux/iommu.h |8 +
 5 files changed, 86 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
+   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 ..d82a10b2478b
--- /dev/null
+++ b/drivers/iommu/iommu-debugfs.c
@@ -0,0 +1,64 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * IOMMU driver
+ *
+ * Copyright (C) 2018 Advanced Micro Devices, Inc.
+ *
+ * Author: Gary R Hook 
+ */
+
+#include 
+#include 
+#include 
+
+static struct dentry *iommu_debugfs_dir;
+
+/*
+ * 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.
+ */
+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");
+