Re: [PATCH v2 5/5] iommu/amd - Add a debugfs entry to specify a IOMMU device table entry

[Gary R Hook]

On 03/13/2018 03:56 PM, Andy Shevchenko wrote:

On Tue, Mar 13, 2018 at 8:54 PM, Gary R Hook  wrote:

On 03/13/2018 12:20 PM, Andy Shevchenko wrote:



+   } else if (obuf[0] == '0' && obuf[1] == 'x') {
+   n = sscanf(obuf, "%x", _iommu_devid);
+   } else {
+   n = sscanf(obuf, "%d", _iommu_devid);
+   }



kstrtoint() ?



I see various mechanisms for this sort of thing, and simply chose one.
Am happy to use whatever is preferred.


sscanf() has an enormous overhead for cases like this.

simple

ret = kstrtoint();
if (ret)
  ... do error handling ...




Gotcha. Fixed.

Thanks,
Gary

Re: [PATCH v2 5/5] iommu/amd - Add a debugfs entry to specify a IOMMU device table entry

[Gary R Hook]

On 03/13/2018 03:56 PM, Andy Shevchenko wrote:

On Tue, Mar 13, 2018 at 8:54 PM, Gary R Hook  wrote:

On 03/13/2018 12:20 PM, Andy Shevchenko wrote:



+   } else if (obuf[0] == '0' && obuf[1] == 'x') {
+   n = sscanf(obuf, "%x", _iommu_devid);
+   } else {
+   n = sscanf(obuf, "%d", _iommu_devid);
+   }



kstrtoint() ?



I see various mechanisms for this sort of thing, and simply chose one.
Am happy to use whatever is preferred.


sscanf() has an enormous overhead for cases like this.

simple

ret = kstrtoint();
if (ret)
  ... do error handling ...




Gotcha. Fixed.

Thanks,
Gary

Re: [PATCH v2 5/5] iommu/amd - Add a debugfs entry to specify a IOMMU device table entry

[Andy Shevchenko]

On Tue, Mar 13, 2018 at 8:54 PM, Gary R Hook  wrote:
> On 03/13/2018 12:20 PM, Andy Shevchenko wrote:

>>> +   } else if (obuf[0] == '0' && obuf[1] == 'x') {
>>> +   n = sscanf(obuf, "%x", _iommu_devid);
>>> +   } else {
>>> +   n = sscanf(obuf, "%d", _iommu_devid);
>>> +   }

>> kstrtoint() ?

> I see various mechanisms for this sort of thing, and simply chose one.
> Am happy to use whatever is preferred.

sscanf() has an enormous overhead for cases like this.

simple

ret = kstrtoint();
if (ret)
 ... do error handling ...


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/5] iommu/amd - Add a debugfs entry to specify a IOMMU device table entry

[Andy Shevchenko]

On Tue, Mar 13, 2018 at 8:54 PM, Gary R Hook  wrote:
> On 03/13/2018 12:20 PM, Andy Shevchenko wrote:

>>> +   } else if (obuf[0] == '0' && obuf[1] == 'x') {
>>> +   n = sscanf(obuf, "%x", _iommu_devid);
>>> +   } else {
>>> +   n = sscanf(obuf, "%d", _iommu_devid);
>>> +   }

>> kstrtoint() ?

> I see various mechanisms for this sort of thing, and simply chose one.
> Am happy to use whatever is preferred.

sscanf() has an enormous overhead for cases like this.

simple

ret = kstrtoint();
if (ret)
 ... do error handling ...


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/5] iommu/amd - Add a debugfs entry to specify a IOMMU device table entry

[Gary R Hook]

On 03/13/2018 12:20 PM, Andy Shevchenko wrote:



+   oboff += OSCNPRINTF("%02x:%02x:%x (%u / %04x)\n",
+   PCI_BUS_NUM(amd_iommu_devid),
+   PCI_SLOT(amd_iommu_devid),
+   PCI_FUNC(amd_iommu_devid),


Perhaps at some point we will have an extension to %p to print PCI BDFs.


But until then  ;-)


+   if (strnchr(obuf, OBUFLEN, ':'))
+   {


Style


D'oh!


+   } else if (obuf[0] == '0' && obuf[1] == 'x') {
+   n = sscanf(obuf, "%x", _iommu_devid);
+   } else {
+   n = sscanf(obuf, "%d", _iommu_devid);
+   }


kstrtoint() ?


I see various mechanisms for this sort of thing, and simply chose one.
Am happy to use whatever is preferred.

Re: [PATCH v2 5/5] iommu/amd - Add a debugfs entry to specify a IOMMU device table entry

[Gary R Hook]

On 03/13/2018 12:20 PM, Andy Shevchenko wrote:



+   oboff += OSCNPRINTF("%02x:%02x:%x (%u / %04x)\n",
+   PCI_BUS_NUM(amd_iommu_devid),
+   PCI_SLOT(amd_iommu_devid),
+   PCI_FUNC(amd_iommu_devid),


Perhaps at some point we will have an extension to %p to print PCI BDFs.


But until then  ;-)


+   if (strnchr(obuf, OBUFLEN, ':'))
+   {


Style


D'oh!


+   } else if (obuf[0] == '0' && obuf[1] == 'x') {
+   n = sscanf(obuf, "%x", _iommu_devid);
+   } else {
+   n = sscanf(obuf, "%d", _iommu_devid);
+   }


kstrtoint() ?


I see various mechanisms for this sort of thing, and simply chose one.
Am happy to use whatever is preferred.

Re: [PATCH v2 5/5] iommu/amd - Add a debugfs entry to specify a IOMMU device table entry

[Andy Shevchenko]

On Fri, Mar 9, 2018 at 2:51 AM, Gary R Hook  wrote:
> Initially (at boot) the device table values dumped are all of the
> active devices.  Add a devid debugfs file to allow the user to select a
> single device table entry to dump (active or not). Let any devid value
> greater than the maximum allowable PCI ID (0x) restore the
> behavior to that effective at boot.

> +   oboff += OSCNPRINTF("%02x:%02x:%x (%u / %04x)\n",
> +   PCI_BUS_NUM(amd_iommu_devid),
> +   PCI_SLOT(amd_iommu_devid),
> +   PCI_FUNC(amd_iommu_devid),

Perhaps at some point we will have an extension to %p to print PCI BDFs.

> +   if (strnchr(obuf, OBUFLEN, ':'))
> +   {

Style

> +   } else if (obuf[0] == '0' && obuf[1] == 'x') {
> +   n = sscanf(obuf, "%x", _iommu_devid);
> +   } else {
> +   n = sscanf(obuf, "%d", _iommu_devid);
> +   }

kstrtoint() ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/5] iommu/amd - Add a debugfs entry to specify a IOMMU device table entry

[Andy Shevchenko]

On Fri, Mar 9, 2018 at 2:51 AM, Gary R Hook  wrote:
> Initially (at boot) the device table values dumped are all of the
> active devices.  Add a devid debugfs file to allow the user to select a
> single device table entry to dump (active or not). Let any devid value
> greater than the maximum allowable PCI ID (0x) restore the
> behavior to that effective at boot.

> +   oboff += OSCNPRINTF("%02x:%02x:%x (%u / %04x)\n",
> +   PCI_BUS_NUM(amd_iommu_devid),
> +   PCI_SLOT(amd_iommu_devid),
> +   PCI_FUNC(amd_iommu_devid),

Perhaps at some point we will have an extension to %p to print PCI BDFs.

> +   if (strnchr(obuf, OBUFLEN, ':'))
> +   {

Style

> +   } else if (obuf[0] == '0' && obuf[1] == 'x') {
> +   n = sscanf(obuf, "%x", _iommu_devid);
> +   } else {
> +   n = sscanf(obuf, "%d", _iommu_devid);
> +   }

kstrtoint() ?

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 5/5] iommu/amd - Add a debugfs entry to specify a IOMMU device table entry

[Gary R Hook]

Initially (at boot) the device table values dumped are all of the
active devices.  Add a devid debugfs file to allow the user to select a
single device table entry to dump (active or not). Let any devid value
greater than the maximum allowable PCI ID (0x) restore the
behavior to that effective at boot.

Signed-off-by: Gary R Hook 
---
 drivers/iommu/amd_iommu_debugfs.c |  109 ++---
 1 file changed, 100 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/amd_iommu_debugfs.c 
b/drivers/iommu/amd_iommu_debugfs.c
index c4e071f7a5b9..aa6935340163 100644
--- a/drivers/iommu/amd_iommu_debugfs.c
+++ b/drivers/iommu/amd_iommu_debugfs.c
@@ -28,6 +28,7 @@ static DEFINE_RWLOCK(iommu_debugfs_lock);
 #defineMAX_NAME_LEN20
 
 static unsigned int amd_iommu_verbose = 0;
+static unsigned int amd_iommu_devid = ~0;
 
 static unsigned int amd_iommu_count_valid_dtes(int start, int end)
 {
@@ -81,6 +82,76 @@ static const struct file_operations 
amd_iommu_debugfs_dtecount_ops = {
.write = NULL,
 };
 
+static ssize_t amd_iommu_debugfs_devid_read(struct file *filp,
+   char __user *ubuf,
+   size_t count, loff_t *offp)
+{
+   struct amd_iommu *iommu = filp->private_data;
+   unsigned int obuflen = 512;
+   unsigned int oboff = 0;
+   ssize_t ret;
+   char *obuf;
+
+   if (!iommu)
+   return 0;
+
+   obuf = kmalloc(OBUFLEN, GFP_KERNEL);
+   if (!obuf)
+   return -ENOMEM;
+
+   if (amd_iommu_verbose)
+   oboff += OSCNPRINTF("%02x:%02x:%x (%u / %04x)\n",
+   PCI_BUS_NUM(amd_iommu_devid),
+   PCI_SLOT(amd_iommu_devid),
+   PCI_FUNC(amd_iommu_devid),
+   amd_iommu_devid, amd_iommu_devid);
+   else
+   oboff += OSCNPRINTF("%u\n", amd_iommu_devid);
+
+   ret = simple_read_from_buffer(ubuf, count, offp, obuf, oboff);
+   kfree(obuf);
+
+   return ret;
+}
+
+static ssize_t amd_iommu_debugfs_devid_write(struct file *filp,
+   const char __user *ubuf,
+   size_t count, loff_t *offp)
+{
+   unsigned int pci_id, pci_slot, pci_func;
+   unsigned int obuflen = 80;
+   ssize_t ret;
+   char *obuf;
+   int n;
+
+   obuf = kmalloc(OBUFLEN, GFP_KERNEL);
+   if (!obuf)
+   return -ENOMEM;
+
+   ret = simple_write_to_buffer(obuf, OBUFLEN, offp, ubuf, count);
+
+   if (strnchr(obuf, OBUFLEN, ':'))
+   {
+   n = sscanf(obuf, "%x:%x.%x", _id, _slot, _func);
+   if (n == 3)
+   amd_iommu_devid = PCI_DEVID(pci_id, PCI_DEVFN(pci_slot, 
pci_func));
+   } else if (obuf[0] == '0' && obuf[1] == 'x') {
+   n = sscanf(obuf, "%x", _iommu_devid);
+   } else {
+   n = sscanf(obuf, "%d", _iommu_devid);
+   }
+   kfree(obuf);
+
+   return ret;
+}
+
+static const struct file_operations amd_iommu_debugfs_devid_ops = {
+   .owner = THIS_MODULE,
+   .open = simple_open,
+   .read = amd_iommu_debugfs_devid_read,
+   .write = amd_iommu_debugfs_devid_write,
+};
+
 #defineMAX_PCI_ID  0x
 
 #definePRINTDTE(i) OSCNPRINTF("%02x:%02x:%x - %016llx %016llx 
%016llx %016llx\n", \
@@ -106,19 +177,28 @@ static ssize_t amd_iommu_debugfs_dte_read(struct file 
*filp,
return 0;
 
/* Count the number of valid entries in the device table */
-   istart = 0;
-   iend = MAX_PCI_ID;
-   n = amd_iommu_count_valid_dtes(istart, iend);
+   if (amd_iommu_devid > MAX_PCI_ID) {
+   istart = 0;
+   iend = MAX_PCI_ID;
+   n = amd_iommu_count_valid_dtes(istart, iend);
+   } else {
+   n = 1;
+   }
obuflen = n * 80;
 
obuf = kmalloc(OBUFLEN, GFP_KERNEL);
if (!obuf)
return -ENOMEM;
 
-   for (i = istart ; i <= iend ; i++)
-   if ((amd_iommu_dev_table[i].data[0] ^ 0x3)
-|| amd_iommu_dev_table[i].data[1])
-   oboff += PRINTDTE(i);
+   if (amd_iommu_devid > MAX_PCI_ID) {
+   for (i = istart ; i <= iend ; i++)
+   if ((amd_iommu_dev_table[i].data[0] ^ 0x3)
+|| amd_iommu_dev_table[i].data[1])
+   oboff += PRINTDTE(i);
+   } else {
+   i = amd_iommu_devid;
+   oboff += PRINTDTE(i);
+   }
 
ret = simple_read_from_buffer(ubuf, count, offp, obuf, oboff);
kfree(obuf);
@@ -135,12 +215,17 @@ static const struct file_operations 
amd_iommu_debugfs_dte_ops = {
 
 static char readmetext[] =
 "devicetable Print active entries in the device 

[PATCH v2 5/5] iommu/amd - Add a debugfs entry to specify a IOMMU device table entry

[Gary R Hook]

Initially (at boot) the device table values dumped are all of the
active devices.  Add a devid debugfs file to allow the user to select a
single device table entry to dump (active or not). Let any devid value
greater than the maximum allowable PCI ID (0x) restore the
behavior to that effective at boot.

Signed-off-by: Gary R Hook 
---
 drivers/iommu/amd_iommu_debugfs.c |  109 ++---
 1 file changed, 100 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/amd_iommu_debugfs.c 
b/drivers/iommu/amd_iommu_debugfs.c
index c4e071f7a5b9..aa6935340163 100644
--- a/drivers/iommu/amd_iommu_debugfs.c
+++ b/drivers/iommu/amd_iommu_debugfs.c
@@ -28,6 +28,7 @@ static DEFINE_RWLOCK(iommu_debugfs_lock);
 #defineMAX_NAME_LEN20
 
 static unsigned int amd_iommu_verbose = 0;
+static unsigned int amd_iommu_devid = ~0;
 
 static unsigned int amd_iommu_count_valid_dtes(int start, int end)
 {
@@ -81,6 +82,76 @@ static const struct file_operations 
amd_iommu_debugfs_dtecount_ops = {
.write = NULL,
 };
 
+static ssize_t amd_iommu_debugfs_devid_read(struct file *filp,
+   char __user *ubuf,
+   size_t count, loff_t *offp)
+{
+   struct amd_iommu *iommu = filp->private_data;
+   unsigned int obuflen = 512;
+   unsigned int oboff = 0;
+   ssize_t ret;
+   char *obuf;
+
+   if (!iommu)
+   return 0;
+
+   obuf = kmalloc(OBUFLEN, GFP_KERNEL);
+   if (!obuf)
+   return -ENOMEM;
+
+   if (amd_iommu_verbose)
+   oboff += OSCNPRINTF("%02x:%02x:%x (%u / %04x)\n",
+   PCI_BUS_NUM(amd_iommu_devid),
+   PCI_SLOT(amd_iommu_devid),
+   PCI_FUNC(amd_iommu_devid),
+   amd_iommu_devid, amd_iommu_devid);
+   else
+   oboff += OSCNPRINTF("%u\n", amd_iommu_devid);
+
+   ret = simple_read_from_buffer(ubuf, count, offp, obuf, oboff);
+   kfree(obuf);
+
+   return ret;
+}
+
+static ssize_t amd_iommu_debugfs_devid_write(struct file *filp,
+   const char __user *ubuf,
+   size_t count, loff_t *offp)
+{
+   unsigned int pci_id, pci_slot, pci_func;
+   unsigned int obuflen = 80;
+   ssize_t ret;
+   char *obuf;
+   int n;
+
+   obuf = kmalloc(OBUFLEN, GFP_KERNEL);
+   if (!obuf)
+   return -ENOMEM;
+
+   ret = simple_write_to_buffer(obuf, OBUFLEN, offp, ubuf, count);
+
+   if (strnchr(obuf, OBUFLEN, ':'))
+   {
+   n = sscanf(obuf, "%x:%x.%x", _id, _slot, _func);
+   if (n == 3)
+   amd_iommu_devid = PCI_DEVID(pci_id, PCI_DEVFN(pci_slot, 
pci_func));
+   } else if (obuf[0] == '0' && obuf[1] == 'x') {
+   n = sscanf(obuf, "%x", _iommu_devid);
+   } else {
+   n = sscanf(obuf, "%d", _iommu_devid);
+   }
+   kfree(obuf);
+
+   return ret;
+}
+
+static const struct file_operations amd_iommu_debugfs_devid_ops = {
+   .owner = THIS_MODULE,
+   .open = simple_open,
+   .read = amd_iommu_debugfs_devid_read,
+   .write = amd_iommu_debugfs_devid_write,
+};
+
 #defineMAX_PCI_ID  0x
 
 #definePRINTDTE(i) OSCNPRINTF("%02x:%02x:%x - %016llx %016llx 
%016llx %016llx\n", \
@@ -106,19 +177,28 @@ static ssize_t amd_iommu_debugfs_dte_read(struct file 
*filp,
return 0;
 
/* Count the number of valid entries in the device table */
-   istart = 0;
-   iend = MAX_PCI_ID;
-   n = amd_iommu_count_valid_dtes(istart, iend);
+   if (amd_iommu_devid > MAX_PCI_ID) {
+   istart = 0;
+   iend = MAX_PCI_ID;
+   n = amd_iommu_count_valid_dtes(istart, iend);
+   } else {
+   n = 1;
+   }
obuflen = n * 80;
 
obuf = kmalloc(OBUFLEN, GFP_KERNEL);
if (!obuf)
return -ENOMEM;
 
-   for (i = istart ; i <= iend ; i++)
-   if ((amd_iommu_dev_table[i].data[0] ^ 0x3)
-|| amd_iommu_dev_table[i].data[1])
-   oboff += PRINTDTE(i);
+   if (amd_iommu_devid > MAX_PCI_ID) {
+   for (i = istart ; i <= iend ; i++)
+   if ((amd_iommu_dev_table[i].data[0] ^ 0x3)
+|| amd_iommu_dev_table[i].data[1])
+   oboff += PRINTDTE(i);
+   } else {
+   i = amd_iommu_devid;
+   oboff += PRINTDTE(i);
+   }
 
ret = simple_read_from_buffer(ubuf, count, offp, obuf, oboff);
kfree(obuf);
@@ -135,12 +215,17 @@ static const struct file_operations 
amd_iommu_debugfs_dte_ops = {
 
 static char readmetext[] =
 "devicetable Print active entries in the device table\n"
+"devid