Re: [PATCH v7 0/5] Add Intel IOMMU debugfs support

[Jacob Pan]

On Thu, 29 Mar 2018 10:48:24 +0200
Joerg Roedel  wrote:

> [ Adding Gary from AMD to Cc ]
> 
> On Mon, Mar 19, 2018 at 09:37:14AM -0700, Jacob Pan wrote:
> > On Thu, 15 Mar 2018 14:18:54 +0100
> > Joerg Roedel  wrote:
> >   
> > > On Thu, Feb 15, 2018 at 08:38:11AM -0800, Jacob Pan wrote:  
> > > > Just wondering if your concern is on the implementation or the
> > > > debugfs idea in general. Perhaps have some common IOMMU
> > > > debugfs?
> > > 
> > > My concern mainly is that we add interfaces which reveal
> > > potentially security relevant information  
> > I don;t think security is any worse than existing kernel page table
> > in debugfs. i.e. /sys/kernel/debug/page_tables
> > This is a debug feature.  
> 
> Okay, so here is the way to go: Please introduce a basic debugfs
> facility to the core iommu code. It should basically only create a
> 'iommu/' directory in debugfs where drivers can create their own
> sub-directories. This must be enabled by a new kconfig option
> (CONFIG_IOMMU_DEBUGFS) and the kernel should print a big fat warning
> at boot when it is enabled. This hopefully prevents anyone from
> enabling it for production kernels.
> 
> Then in the next cycle I will review again more closely what
> information about VT-d and AMD-Vi is revealed there and will probably
> apply what I can live with.
> 
sounds great. we will provide vt-d info for both current and
potential extensions so that you can consider if there can be any
abstractions.

> Thanks,
> 
>   Joerg
> 

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

Re: [PATCH v7 0/5] Add Intel IOMMU debugfs support

[Gary R Hook]

On 03/29/2018 03:48 AM, Joerg Roedel wrote:

[ Adding Gary from AMD to Cc ]

On Mon, Mar 19, 2018 at 09:37:14AM -0700, Jacob Pan wrote:

On Thu, 15 Mar 2018 14:18:54 +0100
Joerg Roedel  wrote:


On Thu, Feb 15, 2018 at 08:38:11AM -0800, Jacob Pan wrote:

Just wondering if your concern is on the implementation or the
debugfs idea in general. Perhaps have some common IOMMU debugfs?


My concern mainly is that we add interfaces which reveal
potentially security relevant information

I don;t think security is any worse than existing kernel page table in
debugfs. i.e. /sys/kernel/debug/page_tables
This is a debug feature.


Okay, so here is the way to go: Please introduce a basic debugfs
facility to the core iommu code. It should basically only create a
'iommu/' directory in debugfs where drivers can create their own
sub-directories. This must be enabled by a new kconfig option
(CONFIG_IOMMU_DEBUGFS) and the kernel should print a big fat warning at
boot when it is enabled. This hopefully prevents anyone from enabling it
for production kernels.


I'm halfway through this. Where would you like to place the invocation 
of the initialization function?


There's an iommu_init() in iommu.c, But it's a core_initcall, which 
doesn't seem like a good spot. Not knowing enough about bring-up here, 
Would adding another __init function be suitable?


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

Re: [PATCH v7 0/5] Add Intel IOMMU debugfs support

[Joerg Roedel]

[ Adding Gary from AMD to Cc ]

On Mon, Mar 19, 2018 at 09:37:14AM -0700, Jacob Pan wrote:
> On Thu, 15 Mar 2018 14:18:54 +0100
> Joerg Roedel  wrote:
> 
> > On Thu, Feb 15, 2018 at 08:38:11AM -0800, Jacob Pan wrote:
> > > Just wondering if your concern is on the implementation or the
> > > debugfs idea in general. Perhaps have some common IOMMU debugfs?  
> > 
> > My concern mainly is that we add interfaces which reveal
> > potentially security relevant information
> I don;t think security is any worse than existing kernel page table in
> debugfs. i.e. /sys/kernel/debug/page_tables
> This is a debug feature.

Okay, so here is the way to go: Please introduce a basic debugfs
facility to the core iommu code. It should basically only create a
'iommu/' directory in debugfs where drivers can create their own
sub-directories. This must be enabled by a new kconfig option
(CONFIG_IOMMU_DEBUGFS) and the kernel should print a big fat warning at
boot when it is enabled. This hopefully prevents anyone from enabling it
for production kernels.

Then in the next cycle I will review again more closely what information
about VT-d and AMD-Vi is revealed there and will probably apply what I
can live with.

Thanks,

Joerg

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

Re: [PATCH v7 0/5] Add Intel IOMMU debugfs support

[Jacob Pan]

On Thu, 15 Mar 2018 14:18:54 +0100
Joerg Roedel  wrote:

> On Thu, Feb 15, 2018 at 08:38:11AM -0800, Jacob Pan wrote:
> > Just wondering if your concern is on the implementation or the
> > debugfs idea in general. Perhaps have some common IOMMU debugfs?  
> 
> My concern mainly is that we add interfaces which reveal
> potentially security relevant information
I don;t think security is any worse than existing kernel page table in
debugfs. i.e. /sys/kernel/debug/page_tables
This is a debug feature.
> to user-space and that tools
> come up using it so that this also becomes kABI and we can't easily
> change it anymore and this whole stuff turns into a maintence
> nightmare.
> 
Agreed, perhaps we can address that by only dumping user readable data
which avoid having a parser tool that relies on stable kABI?

> So that is definitly not something I'd like to see enabled in the
> distros, and its better to avoid it at all and search for better ways
> to debug upcoming issues.
> 
We can make it "def_bool n" so only used by advanced customers who can
recompile kernel.
> BPF tracers and tracing in general comes to mind here...
> 
my concern is that tracing is suitable for dynamic debugging, but these
context info are mostly static. Perhaps I am missing some tracing
features.

Thanks,

Jacob
> 
>   Joerg
> 

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

Re: [PATCH v7 0/5] Add Intel IOMMU debugfs support

[Joerg Roedel]

On Thu, Feb 15, 2018 at 08:38:11AM -0800, Jacob Pan wrote:
> Just wondering if your concern is on the implementation or the debugfs
> idea in general. Perhaps have some common IOMMU debugfs?

My concern mainly is that we add interfaces which reveal
potentially security relevant information to user-space and that tools
come up using it so that this also becomes kABI and we can't easily
change it anymore and this whole stuff turns into a maintence nightmare.

So that is definitly not something I'd like to see enabled in the
distros, and its better to avoid it at all and search for better ways to
debug upcoming issues.

BPF tracers and tracing in general comes to mind here...


Joerg

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

Re: [PATCH v7 0/5] Add Intel IOMMU debugfs support

[Jacob Pan]

On Thu, 22 Feb 2018 08:48:37 +0100
Yves-Alexis Perez  wrote:

> On Tue, 2018-02-20 at 14:25 -0800, Jacob Pan wrote:
> > I didn't know about chipsec but reading the code seems to rely on an
> > out-of-tree kernel module. I don't think it matches what we need
> > here.  
> 
> Yes good indeed, I had forgot about that. Maybe the userland part is
> still useful, but there's definitely a need for (protected) access to
> privileged memory (and access to /dev/mem is less practical than
> debugfs, I guess).
> 
Another reason we can't use /dev/mem is that the context entries are
not static, they are created for each device when the first map()
comes. So we need to rely on the list in the intel iommu driver to dump
the context.
> Regards,

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

Re: [PATCH v7 0/5] Add Intel IOMMU debugfs support

[Yves-Alexis Perez]

On Tue, 2018-02-20 at 14:25 -0800, Jacob Pan wrote:
> I didn't know about chipsec but reading the code seems to rely on an
> out-of-tree kernel module. I don't think it matches what we need here.

Yes good indeed, I had forgot about that. Maybe the userland part is still
useful, but there's definitely a need for (protected) access to privileged
memory (and access to /dev/mem is less practical than debugfs, I guess).

Regards,
-- 
Yves-Alexis

signature.asc
Description: This is a digitally signed message part
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v7 0/5] Add Intel IOMMU debugfs support

[Jacob Pan]

On Sun, 18 Feb 2018 23:15:32 +0100
Yves-Alexis Perez  wrote:

> On Tue, 2018-02-13 at 13:40 -0800, Raj, Ashok wrote:
> > This version has only hw dumps for now, but we plan to add some
> > other things like walking 2nd level page-tables, or get some SVM
> > specific data from the driver in the future.   
> 
> Hi,
> 
> I'm not sure how much you know about chipsec [1], but with a oneliner
> [2] you can have it print a lot of data from Vt-d configuration,
> including page tables. As far as I remember, it doesn't need access
> to /dev/mem (but I gidn't dig to check how it does it).
> 
> It might still be useful to have everything from debugfs because
> it'll always be consistent with the current running kernel, but at
> least it might help users reporting bugs.
> 
> [1] https://github.com/chipsec/chipsec
> [2] http://paste.debian.net/1010105

I didn't know about chipsec but reading the code seems to rely on an
out-of-tree kernel module. I don't think it matches what we need here.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v7 0/5] Add Intel IOMMU debugfs support

[Yves-Alexis Perez]

On Tue, 2018-02-13 at 13:40 -0800, Raj, Ashok wrote:
> This version has only hw dumps for now, but we plan to add some other things
> like walking 2nd level page-tables, or get some SVM specific data from the 
> driver in the future. 

Hi,

I'm not sure how much you know about chipsec [1], but with a oneliner [2] you
can have it print a lot of data from Vt-d configuration, including page
tables. As far as I remember, it doesn't need access to /dev/mem (but I gidn't
dig to check how it does it).

It might still be useful to have everything from debugfs because it'll always
be consistent with the current running kernel, but at least it might help
users reporting bugs.

[1] https://github.com/chipsec/chipsec
[2] http://paste.debian.net/1010105
-- 
Yves-Alexis

signature.asc
Description: This is a digitally signed message part
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v7 0/5] Add Intel IOMMU debugfs support

[Jacob Pan]

On Thu, 15 Feb 2018 10:53:38 +0100
Joerg Roedel  wrote:

> On Tue, Feb 13, 2018 at 02:53:32PM -0800, Jacob Pan wrote:
> > We did start out with /dev/mem but run into CONFIG_STRICT_DEVMEM
> > requirement which is turned on by default.
> > libpci is only limited to PCI config space access, right?  
> 
> Even if /dev/mem is not an option, I am still not convinced that this
> is a good idea. How should this be used? Will you just use it to debug
> Intel IOMMUs when you guys work on the driver or should users report
> the data back when they hit problems?
> 
It is for both but mainly the latter. when we have customers/users
reporting Intel IOMMU issues, it would be extremely helpful if we can
ask them to turn debugfs on and report the state. As we move to SVA, we
may also include io page table similar to /sys/kernel/debug/page_table
for PASID etc.

Just wondering if your concern is on the implementation or the debugfs
idea in general. Perhaps have some common IOMMU debugfs?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v7 0/5] Add Intel IOMMU debugfs support

[Joerg Roedel]

On Tue, Feb 13, 2018 at 02:53:32PM -0800, Jacob Pan wrote:
> We did start out with /dev/mem but run into CONFIG_STRICT_DEVMEM
> requirement which is turned on by default.
> libpci is only limited to PCI config space access, right?

Even if /dev/mem is not an option, I am still not convinced that this is
a good idea. How should this be used? Will you just use it to debug
Intel IOMMUs when you guys work on the driver or should users report the
data back when they hit problems?



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

Re: [PATCH v7 0/5] Add Intel IOMMU debugfs support

[Jacob Pan]

On Tue, 13 Feb 2018 13:40:02 -0800
"Raj, Ashok"  wrote:

> Hi Joerg,
> 
> On Tue, Feb 13, 2018 at 03:03:03PM +0100, Joerg Roedel wrote:
> > On Fri, Feb 02, 2018 at 04:49:56PM -0800, Sohil Mehta wrote:  
> > > This series aims to add debugfs support for Intel IOMMU. It
> > > exposes IOMMU registers, internal context and dumps individual
> > > table entries to help debug Intel IOMMUs.
> > > 
> > > The first patch does the ground work for the following patches by
> > > reorganizing some Intel IOMMU data structures. The following
> > > patches create a new Kconfig option - INTEL_IOMMU_DEBUG and add
> > > debugfs support for IOMMU context internals, register contents,
> > > PASID internals, and Interrupt remapping in that order. The
> > > information can be accessed in sysfs at
> > > '/sys/kernel/debug/intel_iommu/'.  
> > 
> > This looks like it only presents data from the iommu-hardware
> > (register state) or from in-memory data structures used by the
> > hardware. Can't all this be read out from user-space with libpci
> > and /dev/mem access?  
> 
> True, we can do the tools and keep it strictly user space, but find
> it very convenient to keep the kernel code and debugfs together. When
> there are bug-reports its rather easy for the user to collect some
> data and report it, and all the data-structures we need are readily
> available instead of finding a round-about way to capture the same
> data from user-space.
> 
> This version has only hw dumps for now, but we plan to add some other
> things like walking 2nd level page-tables, or get some SVM specific
> data from the driver in the future. 
> 
We did start out with /dev/mem but run into CONFIG_STRICT_DEVMEM
requirement which is turned on by default.
libpci is only limited to PCI config space access, right?

> > 
> > Things are different for kernel-defined data structures, as they
> > might change between releases and can be presented to user-space
> > via debugfs is needed, but the data structures used by the hardware
> > should be pretty stable.  
> 
> Cheers,
> Ashok

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

Re: [PATCH v7 0/5] Add Intel IOMMU debugfs support

[Raj, Ashok]

Hi Joerg,

On Tue, Feb 13, 2018 at 03:03:03PM +0100, Joerg Roedel wrote:
> On Fri, Feb 02, 2018 at 04:49:56PM -0800, Sohil Mehta wrote:
> > This series aims to add debugfs support for Intel IOMMU. It exposes IOMMU
> > registers, internal context and dumps individual table entries to help debug
> > Intel IOMMUs.
> > 
> > The first patch does the ground work for the following patches by 
> > reorganizing
> > some Intel IOMMU data structures. The following patches create a new Kconfig
> > option - INTEL_IOMMU_DEBUG and add debugfs support for IOMMU context 
> > internals,
> > register contents, PASID internals, and Interrupt remapping in that order. 
> > The
> > information can be accessed in sysfs at '/sys/kernel/debug/intel_iommu/'.
> 
> This looks like it only presents data from the iommu-hardware (register
> state) or from in-memory data structures used by the hardware. Can't all
> this be read out from user-space with libpci and /dev/mem access?

True, we can do the tools and keep it strictly user space, but find it very
convenient to keep the kernel code and debugfs together. When there are 
bug-reports its rather easy for the user to collect some data and report it, 
and all the data-structures we need are readily available instead of finding
a round-about way to capture the same data from user-space.

This version has only hw dumps for now, but we plan to add some other things
like walking 2nd level page-tables, or get some SVM specific data from the 
driver in the future. 

> 
> Things are different for kernel-defined data structures, as they might
> change between releases and can be presented to user-space via debugfs
> is needed, but the data structures used by the hardware should be pretty
> stable.

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

Re: [PATCH v7 0/5] Add Intel IOMMU debugfs support

[Joerg Roedel]

On Fri, Feb 02, 2018 at 04:49:56PM -0800, Sohil Mehta wrote:
> This series aims to add debugfs support for Intel IOMMU. It exposes IOMMU
> registers, internal context and dumps individual table entries to help debug
> Intel IOMMUs.
> 
> The first patch does the ground work for the following patches by reorganizing
> some Intel IOMMU data structures. The following patches create a new Kconfig
> option - INTEL_IOMMU_DEBUG and add debugfs support for IOMMU context 
> internals,
> register contents, PASID internals, and Interrupt remapping in that order. The
> information can be accessed in sysfs at '/sys/kernel/debug/intel_iommu/'.

This looks like it only presents data from the iommu-hardware (register
state) or from in-memory data structures used by the hardware. Can't all
this be read out from user-space with libpci and /dev/mem access?

Things are different for kernel-defined data structures, as they might
change between releases and can be presented to user-space via debugfs
is needed, but the data structures used by the hardware should be pretty
stable.


Joerg

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

Re: [PATCH v7 0/5] Add Intel IOMMU debugfs support

[Andy Shevchenko]

On Fri, 2018-02-02 at 16:49 -0800, Sohil Mehta wrote:
> Hi All,
> 
> This series aims to add debugfs support for Intel IOMMU. It exposes
> IOMMU
> registers, internal context and dumps individual table entries to help
> debug
> Intel IOMMUs.
> 
> The first patch does the ground work for the following patches by
> reorganizing
> some Intel IOMMU data structures. The following patches create a new
> Kconfig
> option - INTEL_IOMMU_DEBUG and add debugfs support for IOMMU context
> internals,
> register contents, PASID internals, and Interrupt remapping in that
> order. The
> information can be accessed in sysfs at
> '/sys/kernel/debug/intel_iommu/'.
> 
> 

Nice and clean in comparison to v1.

Reviewed-by: Andy Shevchenko 

Joerg, note, that macro, which patch 2 defines privately, likely will
make v4.16-rc1, thus patch 2 might need to be rebased. So, please, wait
till v4.16-rc1 before applying this.

> Regards,
> Sohil
>  
> Changes since v6:
>  - Split patch 1/5 and 2/5 differently
>  - Simplify and improve code formatting
>  - Use macro for register set definitions
>  - Fix compiler warning for readq
>  - Add Co-Developed-by tag to commit messages
> 
> Changes since v5:
>  - Change the order of includes to an alphabetical order
>  - Change seq_printf and seq_puts formatting
> 
> Changes since v4:
>  - Change to a SPDX license tag
>  - Fix seq_printf formatting and remove leading '\n's
> 
> Changes since v3:
>  - Remove an unused function parameter from some of the functions
>  - Fix checkpatch.pl warnings
>  - Remove error reporting for debugfs_create_file functions
>  - Fix unnecessary reprogramming of the context entries
>  - Simplify and merge the show context and extended context patch into
> one
>  - Remove redundant IOMMU null check under for_each_active_iommu
>  - Update the commit title to be consistent
> 
> Changes since v2:
>  - Added a macro for seq file operations based on recommendation by
> Andy 
>Shevchenko. The marco can be moved to seq_file.h at a future point
>  - Changed the debugfs file names to more relevant ones
>  - Added information for MTRR registers in the regset file
> 
> Changes since v1:
>  - Fixed seq_printf formatting
>  - Handled the case when Interrupt remapping is not enabled
> 
> Gayatri Kammela (4):
>   iommu/vt-d: Relocate struct/function declarations to its header
> files
>   iommu/vt-d: Enable debugfs support to show context internals
>   iommu/vt-d: Add debugfs support to show register contents
>   iommu/vt-d: Add debugfs support to show Pasid table contents
> 
> Sohil Mehta (1):
>   iommu/vt-d: Add debugfs support for Interrupt remapping
> 
>  drivers/iommu/Kconfig |   8 +
>  drivers/iommu/Makefile|   1 +
>  drivers/iommu/intel-iommu-debug.c | 338
> ++
>  drivers/iommu/intel-iommu.c   |  34 +---
>  drivers/iommu/intel-svm.c |   8 -
>  include/linux/intel-iommu.h   |  39 +
>  include/linux/intel-svm.h |  10 +-
>  7 files changed, 400 insertions(+), 38 deletions(-)
>  create mode 100644 drivers/iommu/intel-iommu-debug.c
> 

-- 
Andy Shevchenko 
Intel Finland Oy
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu