VMD domains start at 0x10000, so expand dev->name to fit at least this
many characters.

Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
 src/util/virpci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/util/virpci.c b/src/util/virpci.c
index 2c1b758..b3afefe 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -50,7 +50,7 @@ VIR_LOG_INIT("util.pci");
 
 #define PCI_SYSFS "/sys/bus/pci/"
 #define PCI_ID_LEN 10   /* "XXXX XXXX" */
-#define PCI_ADDR_LEN 13 /* "XXXX:XX:XX.X" */
+#define PCI_ADDR_LEN 14 /* "XXXXX:XX:XX.X" */
 
 VIR_ENUM_IMPL(virPCIELinkSpeed, VIR_PCIE_LINK_SPEED_LAST,
               "", "2.5", "5", "8", "16")
-- 
2.9.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 08/07/2017 03:46 PM, Jon Derrick wrote:
> VMD domains start at 0x10000, so expand dev->name to fit at least this > many characters. > > Signed-off-by: Jon Derrick
<jonathan.derrick@intel.com> > --- > src/util/virpci.c | 2 +- > 1 file
changed, 1 insertion(+), 1 deletion(-) > > diff --git
a/src/util/virpci.c b/src/util/virpci.c > index 2c1b758..b3afefe 100644
> --- a/src/util/virpci.c > +++ b/src/util/virpci.c > @@ -50,7 +50,7 @@
VIR_LOG_INIT("util.pci"); > > #define PCI_SYSFS "/sys/bus/pci/" >
#define PCI_ID_LEN 10 /* "XXXX XXXX" */ > -#define PCI_ADDR_LEN 13 /*
"XXXX:XX:XX.X" */ > +#define PCI_ADDR_LEN 14 /* "XXXXX:XX:XX.X" */ > >
VIR_ENUM_IMPL(virPCIELinkSpeed, VIR_PCIE_LINK_SPEED_LAST, > "", "2.5",
"5", "8", "16")
Does just this change by itself enable new functionality? Or are other
changes required? (e.g. the type "pciDomain" in the XML schema is a
uint16, so any domain > 0xFFFF in the config would fail validation).

Assuming that the VMD domain needs to be referenced in config somewhere
in order to be useful, along with changing the type for pciDomain in
docs/schemes/basictypes.rng, we would also need at least one new test
case for the qemuxml2argv and qemuxml2xml tests (see the examples in the
"hostdev-vfio-multidomain" and "net-hostdev-multidomain").

Also, do all versions of qemu support domains > 0xFFFF? If not, is there
a feature that can be used to detect that support so we can have a
capability bit for it and warn if someone tries to use such a domain
when the installed version of qemu doesn't support it? (If there is no
way to tell in advance, then we'll just have to live with reporting any
qemu error after the fact)

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Hi Laine,

Thanks for the thoughtful reply.

On 08/08/2017 05:35 PM, Laine Stump wrote:
> On 08/07/2017 03:46 PM, Jon Derrick wrote:
>> VMD domains start at 0x10000, so expand dev->name to fit at least this > many characters. > > Signed-off-by: Jon Derrick
> <jonathan.derrick@intel.com> > --- > src/util/virpci.c | 2 +- > 1 file
> changed, 1 insertion(+), 1 deletion(-) > > diff --git
> a/src/util/virpci.c b/src/util/virpci.c > index 2c1b758..b3afefe 100644
>> --- a/src/util/virpci.c > +++ b/src/util/virpci.c > @@ -50,7 +50,7 @@
> VIR_LOG_INIT("util.pci"); > > #define PCI_SYSFS "/sys/bus/pci/" >
> #define PCI_ID_LEN 10 /* "XXXX XXXX" */ > -#define PCI_ADDR_LEN 13 /*
> "XXXX:XX:XX.X" */ > +#define PCI_ADDR_LEN 14 /* "XXXXX:XX:XX.X" */ > >
> VIR_ENUM_IMPL(virPCIELinkSpeed, VIR_PCIE_LINK_SPEED_LAST, > "", "2.5",
> "5", "8", "16")
> Does just this change by itself enable new functionality? Or are other
> changes required? (e.g. the type "pciDomain" in the XML schema is a
> uint16, so any domain > 0xFFFF in the config would fail validation).
It doesn't actually enable new functionality. The VMD product itself
doesn't currently support passthrough of child devices, so a 32-bit
domain will never be bound to a VM. This is being ensured by a kernel
patch effort[1], so this patch can wait until those land.

Onto other points, I see that pciDomain is uint16_t, however with my
patch I am able to return the following (which I'm not sure if it means
it still needs changing or not):
<device>
  <name>pci_10000_11_00_0</name>

<path>/sys/devices/pci0000:5d/0000:5d:05.5/pci10000:00/10000:00:00.0/10000:01:00.0/10000:02:07.0/10000:06:00.0/10000:07:10.0/10000:11:00.0</path>
  <parent>pci_10000_07_10_0</parent>
  <driver>
    <name>nvme</name>
  </driver>
  <capability type='pci'>
    <domain>65536</domain>
    <bus>17</bus>
    <slot>0</slot>
    <function>0</function>
    <product id='0x0953'>PCIe Data Center SSD</product>
    <vendor id='0x8086'>Intel Corporation</vendor>
    <numa node='0'/>
    <pci-express>
      <link validity='cap' port='0' speed='8' width='4'/>
      <link validity='sta' speed='8' width='4'/>
    </pci-express>
  </capability>
</device>



However the main goal of the patch was to squash the following warning
and potential issue:
Feb 23 21:16:24 localhost journal: internal error: dev->name buffer
overflow: 10000:00:00.0

If you would prefer, I can resend the patch with the above error
pointing out that the patch fixes this issue alone.


[1]: https://patchwork.ozlabs.org/patch/798846/


> 
> Assuming that the VMD domain needs to be referenced in config somewhere
> in order to be useful, along with changing the type for pciDomain in
> docs/schemes/basictypes.rng, we would also need at least one new test
> case for the qemuxml2argv and qemuxml2xml tests (see the examples in the
> "hostdev-vfio-multidomain" and "net-hostdev-multidomain").
> 
I'd be happy to look into this when, if ever, we do actually support
passthrough of VMD child devices within the kernel.

> Also, do all versions of qemu support domains > 0xFFFF? If not, is there
> a feature that can be used to detect that support so we can have a
> capability bit for it and warn if someone tries to use such a domain
> when the installed version of qemu doesn't support it? (If there is no
> way to tell in advance, then we'll just have to live with reporting any
> qemu error after the fact)
> 
> 
At a first glance, QEMU somewhat supports it. Several domain references
use int, but others use uint16_t. I can take a look at cleaning this up,
but as stated above, VMD child devices won't actually be passed-through
in the foreseeable future. In the past I have done QEMU passthrough
tests with just the VMD endpoint (which resides on a 16-bit domain). The
guest is then responsible for child devices. This mode of operation is
the only one we officially support.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Hi Laine,

Here's an alternate patch which is probably a lot less potentially
harmful. Please let me know what you think:


commit 931ac8a0a32c3399c92efd18e4478342aedec9e1
Author: Jon Derrick <jonathan.derrick@intel.com>
Date:   Wed Aug 9 13:10:01 2017 -0600

    pci: Restrict devices to 16-bit domains

    Most pci devices are limited to 16-bit domains, but VMD uses 32-bit
    domains for child devices. In the current VMD model, child devices
    cannot be assigned to VMs. So restrict the domains to 16-bits rather
    than allowing virpci to attempt to enumerate the devices and failing
    with the following error message:

    internal error: dev->name buffer overflow: 10000:00:00.0

    Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>

diff --git a/src/util/virpci.c b/src/util/virpci.c
index 2c1b758..7aaaddd 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1769,6 +1769,12 @@ virPCIDeviceNew(unsigned int domain,
     dev->address.slot = slot;
     dev->address.function = function;

+    /* This is already implied by the following snprintf, but restrict
devices
+     * to 16-bit domains to prevent the error message
+     */
+    if (domain > 0xffff)
+        return NULL;
+
     if (snprintf(dev->name, sizeof(dev->name), "%.4x:%.2x:%.2x.%.1x",
                  domain, bus, slot, function) >= sizeof(dev->name)) {
         virReportError(VIR_ERR_INTERNAL_ERROR,

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list