[libvirt] [RESEND] pci: add support for VMD domains
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
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)
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
Hi Laine, Thanks for the thoughtful reply. On 08/08/2017 05:35 PM, Laine Stump wrote: 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.
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,
参加者 (2)
-
Jon Derrick -
Laine Stump