Re: [PATCH] iommu/amd: fix missing tag from dev_err message

[Hook, Gary]

On 7/3/2018 11:24 AM, Colin Ian King wrote:

On 03/07/18 17:21, Hook, Gary wrote:

On 7/3/2018 10:55 AM, Joe Perches wrote:

On Tue, 2018-07-03 at 07:56 -0500, Gary R Hook wrote:

On 07/03/2018 05:07 AM, Joe Perches wrote:

On Tue, 2018-07-03 at 07:40 +0100, Colin King wrote:

Currently tag is being assigned but not used, it is missing from
the dev_err message, so add it in.

Cleans up clang warning:
warning: variable 'tag' set but not used [-Wunused-but-set-variable]


[]

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c


[]

@@ -616,9 +616,9 @@ static void iommu_print_event(struct amd_iommu
*iommu, void *__evt)
    pasid = ((event[0] >> 16) & 0x)
    | ((event[1] << 6) & 0xF);
    tag = event[1] & 0x03FF;
-    dev_err(dev, "INVALID_PPR_REQUEST device=%02x:%02x.%x
pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
+    dev_err(dev, "INVALID_PPR_REQUEST device=%02x:%02x.%x
pasid=0x%05x address=0x%016llx flags=0x%04x tag=0x%03x]\n",
    PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
-    pasid, address, flags);
+    pasid, address, flags, tag);


Seems to have a superfluous ] that should be removed.


Yeah, I pretty much messed up all of the log messages in that function.
My apologies. I'll create a patch for that problem; it shouldn't be
fixed here.


Well, no, I misremembered. The extraneous square brace has been there
forever. Needs fixin', though.



The opening square bracket is much earlier:

dev_err(dev, "AMD-Vi: Event logged [");

..and all the subsequent dev_err messages have the trailing square bracket.


Gah! Mystery solved. I'd forgotten about that.

"Never mind."


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

Re: [PATCH] iommu/amd: fix missing tag from dev_err message

[Hook, Gary]

On 7/3/2018 10:55 AM, Joe Perches wrote:

On Tue, 2018-07-03 at 07:56 -0500, Gary R Hook wrote:

On 07/03/2018 05:07 AM, Joe Perches wrote:

On Tue, 2018-07-03 at 07:40 +0100, Colin King wrote:

Currently tag is being assigned but not used, it is missing from
the dev_err message, so add it in.

Cleans up clang warning:
warning: variable 'tag' set but not used [-Wunused-but-set-variable]


[]

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c


[]

@@ -616,9 +616,9 @@ static void iommu_print_event(struct amd_iommu *iommu, void 
*__evt)
pasid = ((event[0] >> 16) & 0x)
| ((event[1] << 6) & 0xF);
tag = event[1] & 0x03FF;
-   dev_err(dev, "INVALID_PPR_REQUEST device=%02x:%02x.%x pasid=0x%05x 
address=0x%016llx flags=0x%04x]\n",
+   dev_err(dev, "INVALID_PPR_REQUEST device=%02x:%02x.%x pasid=0x%05x 
address=0x%016llx flags=0x%04x tag=0x%03x]\n",
PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
-   pasid, address, flags);
+   pasid, address, flags, tag);


Seems to have a superfluous ] that should be removed.


Yeah, I pretty much messed up all of the log messages in that function.
My apologies. I'll create a patch for that problem; it shouldn't be
fixed here.


Well, no, I misremembered. The extraneous square brace has been there 
forever. Needs fixin', though.




I also wonder why event is declared volatile and then
dereferenced with [] multiple times.

Maybe each array dereference should be stored as a
local variable instead.


(I know you know this, but as I understand it) Event is pointing into 
the (hardware's) event buffer, and the data structure has the potential 
of changing out from under us if the device does something without our 
knowledge. Since volatile hints to the compiler of this possibility, I 
believe the compiler should manage this situation. But I could be wrong.


I don't know that we need to atomically copy all 16 bytes into a local 
buffer, as I don't think it's possible for the device to step on itself. 
It will just stop recording events if the buffer gets full. At this 
moment I think volatile is overkill, at least for the EPYC/Ryzen IOMMU.

___
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

[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 v3 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU

[Hook, Gary]

On 4/18/2018 4:16 PM, Mehta, Sohil wrote:

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



debugfs_initialized() was introduced in commit c0f92ba99 back in 
2.6.30-rc1. It was intended as a helper, not as a gatekeeper, which is 
why one doesn't see it used. Given that my use in this proposed patch is 
straightforward, I'm not seeing the need here. I had just seen some 
other code that used it, and copied the model.


Unless someone comes along to say, yes, use it, I'll not.

Gary
___
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

[Hook, Gary]

On 4/17/2018 1:55 PM, Robin Murphy wrote:

On 17/04/18 18:36, Hook, Gary wrote:

On 4/13/2018 7:55 PM, Mehta, Sohil wrote:

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.


Ah, so it isn't. Thank you.




+    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?


That works, yes.


Or I guess you could just roll the pr_warn() into the previous if() 
condition, i.e. only warn when the singleton debugfs_dir is actually 
created.


That makes more sense for a code path this isn't going to be hit more 
than a few times at most.




Robin.






+    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


___
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

[Hook, Gary]

On 4/6/2018 9:17 AM, 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 patch adds a top-level function that will create the (above)
directory, under which a driver may create a hw-specific directory for
its use. The function

iommu_debugfs_setup()

returns a pointer to the new dentry structure created for
/sys/kernel/debug/iommu, or NULL in the event of a failure. An IOMMU
driver should call this function first, and then create a directory
beneath it. A driver implementation might look something like:

static struct dentry *my_debugfs;

struct dentry *d_top;
if (!my_debugfs) {
d_top = iommu_debugfs_setup();
if (d_top)
my_debugfs = debugfs_create_dir("vendor", d_top);
}

Since the IOMMU driver can not be removed from the running system, this
patch only provides an "on" function.

Signed-off-by: Gary R Hook 
---
  drivers/iommu/Kconfig |   11 
  drivers/iommu/Makefile|1 +
  drivers/iommu/iommu-debugfs.c |   58 +
  include/linux/iommu.h |4 +++
  4 files changed, 74 insertions(+)
  create mode 100644 drivers/iommu/iommu-debugfs.c

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index f3a21343e636..c1e39dabfec2 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -60,6 +60,17 @@ config IOMMU_IO_PGTABLE_ARMV7S_SELFTEST
  
  endmenu
  
+config IOMMU_DEBUG

+   bool "Enable 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.


I should explicitly ask about this:

Joerg had suggested IOMMU_DEBUGFS, but here I've changed to IOMMU_DEBUG. 
I'm not seeing a lot of CONFIG options that use DEBUGFS for debugfs 
options, so I chose to follow an implied convention.


Question: should this indeed be IOMMU_DEBUGFS?
^

___
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

[Hook, Gary]

On 4/16/2018 8:52 PM, Mehta, Sohil wrote:

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?


That's a good point. My intention was that only one switch was required 
to incorporate any DebugFS support, but I see now that I didn't consider 
all of the cases.


It appears that a per-device switch is also necessary. Unless someone 
has a better idea.



  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

[Hook, Gary]

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.




+   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.


I'm not sure it matters, given the finite number of IOMMUs in a system, 
and the fact that this work is done exactly once. However, removal of a 
lock is fine thing, so I'll move this around.





+   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

[Hook, Gary]

On 4/13/2018 7:55 PM, Mehta, Sohil wrote:

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.


Ah, so it isn't. Thank you.




+   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?


That works, yes.




+   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] iommu/amd - Use dev_err to send events to the system log

[Hook, Gary]

On 2/14/2018 6:49 AM, Joerg Roedel wrote:

On Tue, Feb 13, 2018 at 02:24:56PM -0500, Hook, Gary wrote:

Without actually running a driver and getting some debug info, I'll just
say that my example compiled, the amd_iommu structure points to a pci_dev
which contains a device, and the two possibilities are likely equivalent.

I'll vet that, of course, but working under the assumption that they are
equivalent, the question becomes, do you have a preference?


They are not equivalent, your version points to the pci_dev containing
the IOMMU, my version points to the (virtual) iommu-device for that
particular iommu in the kernel.

So the difference in the prefix is:

your version: ':00:00.2:'

vs. my version: 'ivhd0:'

The latter is the better prefix, because it points to the iommu. A
pci_dev can contain more than one iommu, so that prefix is not specific
enough. I know that there is currently no hardware implementing multiple
iommus on one pci_dev, but it is allowed according to the spec.


Excellent; specificity is good. I'll crank a new version using that
device.

Thank you!

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

Re: [PATCH] iommu/amd - Use dev_err to send events to the system log

[Hook, Gary]

On 2/13/2018 11:00 AM, Joerg Roedel wrote:

On Tue, Feb 13, 2018 at 10:47:55AM -0500, Hook, Gary wrote:


 dev_err(>dev->dev, "ILLEGAL...


I think its more something like iommu->iommu->dev.





Without actually running a driver and getting some debug info, I'll just
say that my example compiled, the amd_iommu structure points to a 
pci_dev which contains a device, and the two possibilities are likely 
equivalent.


I'll vet that, of course, but working under the assumption that they are 
equivalent, the question becomes, do you have a preference?


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

Re: [PATCH] iommu/amd - Use dev_err to send events to the system log

[Hook, Gary]

On 2/13/2018 8:22 AM, Joerg Roedel wrote:

On Tue, Jan 30, 2018 at 07:04:27PM -0600, Gary R Hook wrote:

+   dev_err(dev, "ILLEGAL_DEV_TABLE_ENTRY device=%02x:%02x.%x "
+   "address=0x%016llx flags=0x%04x]\n",
+   PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
+   address, flags);


That prints the PCI device-name of the IOMMU in front of the message. Is
that actually helpful?

If at all, we should print the name of the iommu-device in front of the
message, not the PCI device that hosts the IOMMU.


so (e.g.):

dev_err(>dev->dev, "ILLEGAL...

?

I'm okay with that. It does make more sense.

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

Re: [PATCH] iommu/vt-d: Fix shift overflow in qi_flush_dev_iotlb

[Hook, Gary]

On 12/13/2017 11:15 AM, Alex Williamson wrote:

On Wed, 13 Dec 2017 10:41:47 -0600
"Hook, Gary" <gh...@amd.com> wrote:


On 12/13/2017 9:58 AM, Alex Williamson wrote:

On Wed, 13 Dec 2017 15:13:55 +0800
Peter Xu <pet...@redhat.com> wrote:
   

On Tue, Dec 12, 2017 at 03:43:08PM -0700, Alex Williamson wrote:

[...]
  

diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index 9a7ffd13c7f0..87888b102057 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -1345,7 +1345,9 @@ void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 
sid, u16 qdep,
struct qi_desc desc;
   
   	if (mask) {

-   BUG_ON(addr & ((1 << (VTD_PAGE_SHIFT + mask)) - 1));
+   BUG_ON((mask > MAX_AGAW_PFN_WIDTH) ||
+  ((mask == MAX_AGAW_PFN_WIDTH) && addr) ||
+  (addr & ((1 << (VTD_PAGE_SHIFT + mask)) - 1)));


Could it work if we just use 1ULL instead of 1 here?  Thanks,


In either case we're talking about shifting off the end of the
variable, which I understand to be undefined.  Right?  Thanks,


How so? Bits fall off the left (MSB) end, zeroes fill in the right (LSB)
end. I believe that behavior is pretty set.


Maybe I'm relying too much on stackoverflow, but:

https://stackoverflow.com/questions/11270492/what-does-the-c-standard-say-about-bitshifting-more-bits-than-the-width-of-type


No, probably not. I don't have my copy of c99 handy, so can't check it. 
But it is beyond me why any compiler implementation would choose to use 
a rotate instead of a shift... probably a performance issue.


So, yeah, when you have silly parameters, you get what you get.

I'll stick to my suggestion. Which seems unambiguous... but I could be 
wrong.

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

Re: [PATCH] iommu/vt-d: Fix shift overflow in qi_flush_dev_iotlb

[Hook, Gary]

On 12/13/2017 9:58 AM, Alex Williamson wrote:

On Wed, 13 Dec 2017 15:13:55 +0800
Peter Xu  wrote:


On Tue, Dec 12, 2017 at 03:43:08PM -0700, Alex Williamson wrote:

[...]


diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index 9a7ffd13c7f0..87888b102057 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -1345,7 +1345,9 @@ void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 
sid, u16 qdep,
struct qi_desc desc;
  
  	if (mask) {

-   BUG_ON(addr & ((1 << (VTD_PAGE_SHIFT + mask)) - 1));
+   BUG_ON((mask > MAX_AGAW_PFN_WIDTH) ||
+  ((mask == MAX_AGAW_PFN_WIDTH) && addr) ||
+  (addr & ((1 << (VTD_PAGE_SHIFT + mask)) - 1)));


Could it work if we just use 1ULL instead of 1 here?  Thanks,


In either case we're talking about shifting off the end of the
variable, which I understand to be undefined.  Right?  Thanks,


How so? Bits fall off the left (MSB) end, zeroes fill in the right (LSB) 
end. I believe that behavior is pretty set.


The only problem here is that the promotion of integral types is (at the 
very least) unclear in this statement. As for the proposal, do we know 
that 1ULL is always going to be the same size as addr?


I would suggest, as pedantic as it sounds, creating a local u64 
variable, initialized to 1, and using that in the BUG_ON:


u64 ubit = 1;

...

if (mask) {
BUG_ON(addr & ((ubit << (VTD_PAGE_SHIFT + mask)) - 1));

I believe this forces everything to be appropriately wide, and the same 
size?

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