Re: [PATCH] iommu/vt-d: Add Scalable Mode fault information

[Mehta, Sohil]

On Tue, 2019-09-10 at 10:08 +0200, Joerg Roedel wrote:
> > + "Unknown", "Unknown", "Unknown", "Unknown", "Unknown",
> "Unknown", "Unknown", /* 0x49-0x4F */
> 
> Maybe add the number (0x49-0x4f) to the respecting "Unknown" fields?
> If
> we can't give a reason we should give the number for easier debugging
> in
> the future. Same for the "Unknown" fields below.

I believe a fault number is always printed in dmar_fault_do_one() even
if the reason is unknown.

DMAR: [DMA Write] Request device [00:02.0] fault addr 108a000 [fault
reason 23] Unknown

--Sohil

Re: [PATCH 5/6] iommu/vt-d: Cleanup after delegating DMA domain to generic iommu

[Mehta, Sohil]

On Sun, 2019-06-09 at 10:38 +0800, Lu Baolu wrote:
>  static int __init si_domain_init(int hw)
> @@ -3306,14 +3252,13 @@ static int __init init_dmars(void)
> if (pasid_supported(iommu))
> intel_svm_init(iommu);
>  #endif
> -   }
>  
> -   /*
> -    * Now that qi is enabled on all iommus, set the root entry
> and flush
> -    * caches. This is required on some Intel X58 chipsets,
> otherwise the
> -    * flush_context function will loop forever and the boot
> hangs.
> -    */
> -   for_each_active_iommu(iommu, drhd) {
> +   /*
> +    * Now that qi is enabled on all iommus, set the root
> entry and
> +    * flush caches. This is required on some Intel X58
> chipsets,
> +    * otherwise the flush_context function will loop
> forever and
> +    * the boot hangs.
> +    */
> iommu_flush_write_buffer(iommu);
> iommu_set_root_entry(iommu);
> iommu->flush.flush_context(iommu, 0, 0, 0,
> DMA_CCMD_GLOBAL_INVL);


This changes the intent of the original code. As the comment says
enable QI on all IOMMUs, then flush the caches and set the root entry.
The order of setting the root entries has changed now.

Refer: 
Commit a4c34ff1c029 ('iommu/vt-d: Enable QI on all IOMMUs before
setting root entry')

--Sohil

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

[Mehta, Sohil]

On Wed, 2018-04-18 at 08:31 +, Yang, Shunyong wrote:
> Maybe the original design is to call debugfs_initialized() before
> calling debugfs_create_xxx()?

I am unaware of the original design. Someone else would probably have
more context. However, looking at other places in the kernel where
debugfs_create_xx() is used, the common convention seems to be to avoid
calling debugfs_initialized().

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

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

[Mehta, Sohil]

On Wed, 2018-04-18 at 05:58 +, Yang, Shunyong wrote:
> Hi, Gary and Sohil,
> 
> On Tue, 2018-04-17 at 13:38 -0400, Hook, Gary wrote:
> > On 4/13/2018 8:08 PM, Mehta, Sohil wrote:
> > > 
> > > On Fri, 2018-04-06 at 08:17 -0500, Gary R Hook wrote:
> > > > 
> > > >   
> > > > +
> > > > +void amd_iommu_debugfs_setup(struct amd_iommu *iommu)
> > > > +{
> > > > + char name[MAX_NAME_LEN + 1];
> > > > + struct dentry *d_top;
> > > > +
> > > > + if (!debugfs_initialized())
> > > Probably not needed.
> > Right.
> 
> When will this check is needed?
> IMO, this function is to check debugfs ready status before we want to
> use debugfs. I just want to understand when we should use
> debugfs_initialized();
> 

You are right debugfs_initialized() can be used to check if debugfs is
ready. However in this case we can also rely on debugfs_create_dir()
which is called in iommu_debufs_setup().

debugfs_create_dir() says:

 * If debugfs is not enabled in the kernel, the value -%ENODEV will be
 * returned.

Sohil

> Thanks.
> Shunyong.
> 
> > 
> > > 
> > > 
> > > > 
> > > > + return;
> > > > +
> > > > + mutex_lock(_iommu_debugfs_lock);
> > > > + if (!amd_iommu_debugfs) {
> > > > + d_top = iommu_debugfs_setup();
> > > > + if (d_top)
> > > > + amd_iommu_debugfs =
> > > > debugfs_create_dir("amd", d_top);
> > > > + }
> > > > + mutex_unlock(_iommu_debugfs_lock);
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Mehta, Sohil]

On Fri, 2018-04-06 at 08:17 -0500, Gary R Hook wrote:
> 
> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
> index 5eb1121d54b9..0ca250f626d9 100644
> --- a/drivers/iommu/Makefile
> +++ b/drivers/iommu/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_IOMMU_IOVA) += iova.o
>  obj-$(CONFIG_OF_IOMMU) += of_iommu.o
>  obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o
>  obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o amd_iommu_init.o
> +obj-$(CONFIG_IOMMU_DEBUG) += amd_iommu_debugfs.o

Compiling amd_iommu_debugfs.c seems to depend only
on CONFIG_IOMMU_DEBUG. Can we prevent the code within
amd_iommu_debugfs.c from getting compiled when either CONFIG_AMD_IOMMU
or CONFIG_IOMMU_DEBUG is disabled?

>  obj-$(CONFIG_AMD_IOMMU_V2) += amd_iommu_v2.o
>  obj-$(CONFIG_ARM_SMMU) += arm-smmu.o
>  obj-$(CONFIG_ARM_SMMU_V3) += arm-smmu-v3.o
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Mehta, Sohil]

On Fri, 2018-04-06 at 08:17 -0500, Gary R Hook wrote:
> 
> +
> +void amd_iommu_debugfs_setup(struct amd_iommu *iommu)
> +{
> + char name[MAX_NAME_LEN + 1];
> + struct dentry *d_top;
> +
> + if (!debugfs_initialized())

Probably not needed.

> + return;
> +
> + mutex_lock(_iommu_debugfs_lock);
> + if (!amd_iommu_debugfs) {
> + d_top = iommu_debugfs_setup();
> + if (d_top)
> + amd_iommu_debugfs =
> debugfs_create_dir("amd", d_top);
> + }
> + mutex_unlock(_iommu_debugfs_lock);


You can do the above only once if you iterate over the IOMMUs here
 instead of doing it in amd_iommu_init.

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

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

Re: [PATCH v3 1/2] iommu - Enable debugfs exposure of the IOMMU

[Mehta, Sohil]

On Fri, 2018-04-06 at 08:17 -0500, Gary R Hook wrote:
> 
> 
> +struct dentry *iommu_debugfs_setup(void)
> +{
> + if (!debugfs_initialized())

This check is probably not needed.

> + return NULL;
> +
> + if (!iommu_debugfs_dir)
> + iommu_debugfs_dir = debugfs_create_dir("iommu",
> NULL);
> +
> + if (iommu_debugfs_dir)
> + pr_warn("WARNING: IOMMU DEBUGFS SUPPORT HAS BEEN
> ENABLED IN THIS KERNEL\n");
> +

As this gets called for each IOMMU, do you want to use pr_warn_once?

> + return iommu_debugfs_dir;
> +}
> +EXPORT_SYMBOL_GPL(iommu_debugfs_setup);

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

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

[Mehta, Sohil]

On Wed, 2018-01-10 at 15:46 +0200, Andy Shevchenko wrote:
> On Tue, 2018-01-09 at 19:48 -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
> > creating a
> > new Kconfig option - INTEL_IOMMU_DEBUG. It also reorganizes some
> > Intel
> > IOMMU
> > data structures. The next four patches 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/'.
> > 
> Thanks for an update.
> I found small issues, otherwise looks pretty much good.
> 
> P.S. During re-indendation take care of the lines that can be
> combined
> to one line.
> 

Thanks Andy for the review. Will fix the issues that you pointed and
send out the updated series.

Sohil

> > 
> > Regards,
> > Sohil
> > 
> > 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: Add debugfs support for Intel IOMMU internals
> >   iommu/vt-d: Add 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 |  10 ++
> >  drivers/iommu/Makefile|   1 +
> >  drivers/iommu/intel-iommu-debug.c | 346
> > ++
> >  drivers/iommu/intel-iommu.c   |  35 +---
> >  drivers/iommu/intel-svm.c |   8 -
> >  include/linux/intel-iommu.h   |  34 
> >  include/linux/intel-svm.h |  10 +-
> >  7 files changed, 407 insertions(+), 37 deletions(-)
> >  create mode 100644 drivers/iommu/intel-iommu-debug.c
> > 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Mehta, Sohil]

On Tue, 2017-12-19 at 23:25 +0200, Andy Shevchenko wrote:
> 
> Perhaps you need to switch to SPDX license pointer.
> I dunno if Thomas' patch with documentation how to do this made
> upstream
>  / linux-next yet.
> 

Thanks. I found the information at https://patchwork.kernel.org/patch/1
0091607/. 
I'll replace the GPL 2.0 license information in intel-iommu-debug.c
with the tag:
// SPDX-License-Identifier: GPL-2.0

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

Re: [PATCH v4 5/5] iommu/vt-d: Add debugfs support for Interrupt remapping

[Mehta, Sohil]

On Tue, 2017-12-19 at 23:30 +0200, Andy Shevchenko wrote:
> On Tue, 2017-12-19 at 13:08 -0800, Sohil Mehta wrote:
> > 
> > Debugfs extension for Intel IOMMU to dump Interrupt remapping table
> > entries for Interrupt remapping and Interrupt posting.
> > 
> > The file /sys/kernel/debug/intel_iommu/ir_translation_struct
> > provides
> > detailed information, such as Index, Source Id, Destination Id,
> > Vector
> > and the raw values for entries with the present bit set, in the
> > format
> > shown.
> > 
> > Remapped Interrupt supported on IOMMU: dmar5
> >  IR table address:93e09d54c310
> > ---
> >  Index  SID  Dest_ID  Vct Raw_value_high   Raw_value_low
> >  1  3a00 0600 2c  00043a00 062c0009
> >  1114301 0900 a2  00044301 09a20009
> > 
> > Posted Interrupt supported on IOMMU: dmar5
> >  IR table address:93e09d54c310
> > -
> > ---
> >  Index  SID  PDA_high PDA_low  Vct Raw_value_high   Raw_value_low
> >  4  4300 0010 40c7c880 41  00144300
> > 40c7c88000418001
> >  5  4300 0010 40c7c880 51  00144300
> > 40c7c88000518001
> > 
> > 
> > 
> > +   seq_printf(m, "\nRemapped Interrupt supported on
> > IOMMU: %s\n"
> Please, avoid leading \n.

Sure. I'll add a separate seq_puts(m, "\n") after each of the loops to
avoid having the leading '\n's.

> 
> > 
> > +      " IR table address:%p\n", iommu->name,
> > +      iommu->ir_table);
> > +
> > +   seq_puts(m, "-
> > -
> > -"
> > +    "\n");
> It's okay to use long string literal on one line. So, don't split (or
> for multi-line string literals, split by \n like you do above).
> 
Thanks. Will fix this and the other one.

> > 
> > +   seq_puts(m,
> > "\n\t\t\t\t\t\t\t\n");
> Leading \n.
> 
> > 
> > +   seq_printf(m, "\nPosted Interrupt supported on
> > IOMMU:
> > %s\n"
> Ditto.
> 
> > 
> > +      " IR table address:%p\n", iommu->name,
> > +      iommu->ir_table);
> > +
> > +   seq_puts(m, "-
> > -
> > --"
> > +    "\n");
> Join back.
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 2/6] iommu/vt-d: Add Intel IOMMU debugfs to show context internals

[Mehta, Sohil]

On Wed, 2017-12-13 at 10:28 +0800, Lu Baolu wrote:
> 
> > Would the recommendation be to use pr_warn instead of pr_err or
> > should
> > we entirely skip the message altogether?
> Greg ever educated me about the use of debugfs_ functions in
> this thread.
> 
> https://spinics.net/lists/linux-usb/msg159384.html
> 
> At least we should avoid the warning/error messages here.
> 

Thanks. We'll remove the error messages.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 3/6] iommu/vt-d: Add Intel IOMMU debugfs to show extended context internals

[Mehta, Sohil]

On Wed, 2017-12-06 at 16:17 +0800, Lu Baolu wrote:
> Hi,
> 
> On 12/06/2017 11:43 AM, Sohil Mehta wrote:
> > 
> > From: Gayatri Kammela 
> > 
> > +
> > +   if (new_ext) {
> > +   seq_printf(m, "Higher Context tbl entries for Bus:
> > %d\n", bus);
> > +   ctx_lo = context[0].lo;
> > +
> > +   if (!(ctx_lo & CONTEXT_PASIDE)) {
> > +   context[1].hi = (u64)virt_to_phys(
> > +   iommu->pasid_state_table);
> > +   context[1].lo = (u64)virt_to_phys(iommu-
> > >pasid_table) |
> > +   intel_iommu_get_pts(iommu)
> > ;
> Why do you change the context entries here?
> 

Thanks for catching this. The context entires were mistakenly getting
changed. We'll remove it.

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

Re: [PATCH v3 2/6] iommu/vt-d: Add Intel IOMMU debugfs to show context internals

[Mehta, Sohil]

On Wed, 2017-12-06 at 16:16 +0800, Lu Baolu wrote:
> Hi,
> 
> On 12/06/2017 11:43 AM, Sohil Mehta wrote:
> > 
> > From: Gayatri Kammela 
> > 
> > 
> > +   seq_printf(m, "%s Context table entries for Bus: %d\n",
> > +      ext ? "Lower" : "", bus);
> > +   seq_printf(m, "[entry]\tDID :B :D .F\tLow\t\tHigh\n");
> WARNING: Prefer seq_puts to seq_printf
> #119: FILE: drivers/iommu/intel-iommu-debug.c:59:
> +seq_printf(m, "[entry]\tDID :B :D .F\tLow\t\tHigh\n");
> 
> (caught by checkpatch.pl)
> 

Hi Lu,

We'll fix this and the other checkpatch.pl warnings.


> > +
> > +static void root_tbl_entry_show(struct seq_file *m, void *unused,
> Why do you define the "unused" parameter which will never been used?
> The same questions to other show functions.
> 

Some functions in our code that are registered with seq_file needed to
have an unused parameter since seq_file.h defines the show function as:
int (*show) (struct seq_file *m, void *v);

But a lot of other functions including the one you pointed don't need
to have the unused parameter. We'll remove it from those.


> > +void __init intel_iommu_debugfs_init(void)
> > +{
> > +   struct dentry *iommu_debug_root;
> > +
> > +   iommu_debug_root = debugfs_create_dir("intel_iommu",
> > NULL);
> > +
> > +   if (!iommu_debug_root) {
> > +   pr_err("can't create debugfs dir\n");
> I don't think we need a pr_err() here. System works well even
> debugfs_create_dir() returns NULL.
> 
> This is same to all pr_err() in this file.
> 

Would the recommendation be to use pr_warn instead of pr_err or should
we entirely skip the message altogether?

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

Re: [PATCH v2 4/6] iommu/vt-d: Add debugfs extension to show register contents

[Mehta, Sohil]

On Mon, 2017-11-27 at 20:52 +, Kammela, Gayatri wrote:
> 
> > 
> > From: Andy Shevchenko [mailto:andriy.shevche...@linux.intel.com]
> > Sent: Wednesday, November 22, 2017 1:19 PM
> > 
> > Moreover, see the patch I have just sent [1] and use same
> > DEFINE_SHOW_ATTRIBUTE() macro here. In that case we would easily
> > move it
> > to seq_file.h for everyone to use in the future.
> That makes sense! I see your patch is not merged yet, so really
> cannot reuse the same macro. I will have to redefine it.

Thanks Andy. That's a good suggestion. Are you planning to move the
macro to seq_file.h soon? If so, should we include this marco in our
patch after it has been merged?

Sohil

> > 
> > 
> > [1]: https://marc.info/?l=linux-bluetooth=151138535801354=2
> > 
> > 
> > --
> > Andy Shevchenko 
> > Intel Finland Oy
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu