The virDomainPCIAddressFlagsCompatible() error logs report that a device required a controller that accepted standard PCI endpoint devices, or PCI Express endpoint devices, and if hotplug was required by the configuration but not provided by the selected controller. But the wording of the error messages was apparently confusing (according to the bugzilla report referenced below). On top of that, if the device was something other than an endpoint device (e.g. a pcie-switch-downstream-port) the error message was a complete punt - it would just say that the flags were incorrect. This patch makes the messages for PCI/PCIe endpoint and hotplug requirements more clear, and also specifically indicates what was the device type when it is other than an endpoint device. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1363627 --- src/conf/domain_addr.c | 58 ++++++++++++++++++++++++++++---------------------- 1 file changed, 33 insertions(+), 25 deletions(-) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index a0c2f88..98176e2 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -118,38 +118,46 @@ virDomainPCIAddressFlagsCompatible(virPCIDeviceAddressPtr addr, * hot-plug and this bus doesn't have it, return false. */ if (!(devFlags & busFlags & VIR_PCI_CONNECT_TYPES_MASK)) { - if (reportError) { - if (devFlags & VIR_PCI_CONNECT_TYPE_PCI_DEVICE) { - virReportError(errType, - _("PCI bus is not compatible with the device " - "at %s. Device requires a standard PCI slot, " - "which is not provided by bus %.4x:%.2x"), - addrStr, addr->domain, addr->bus); - } else if (devFlags & VIR_PCI_CONNECT_TYPE_PCIE_DEVICE) { - virReportError(errType, - _("PCI bus is not compatible with the device " - "at %s. Device requires a PCI Express slot, " - "which is not provided by bus %.4x:%.2x"), - addrStr, addr->domain, addr->bus); - } else { - /* this should never happen. If it does, there is a - * bug in the code that sets the flag bits for devices. - */ - virReportError(errType, - _("The device information for %s has no PCI " - "connection types listed"), addrStr); - } + const char *connectStr; + + if (!reportError) + return false; + + if (devFlags & VIR_PCI_CONNECT_TYPE_PCI_DEVICE) { + connectStr = "standard PCI device"; + } else if (devFlags & VIR_PCI_CONNECT_TYPE_PCIE_DEVICE) { + connectStr = "PCI Express device"; + } else if (devFlags & VIR_PCI_CONNECT_TYPE_PCIE_ROOT_PORT) { + connectStr = "pcie-root-port"; + } else if (devFlags & VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_UPSTREAM_PORT) { + connectStr = "pci-switch-upstream-port"; + } else if (devFlags & VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_DOWNSTREAM_PORT) { + connectStr = "pci-switch-downstream-port"; + } else { + /* this should never happen. If it does, there is a + * bug in the code that sets the flag bits for devices. + */ + virReportError(errType, + _("The device at PCI address %s has " + "unrecognized connection type flags 0x%.2x"), + addrStr, devFlags & VIR_PCI_CONNECT_TYPES_MASK); + return false; } + virReportError(errType, + _("The device at PCI address %s cannot be " + "plugged into the PCI controller with index 0x%.2x. " + "It requires a controller that accepts a %s."), + addrStr, addr->bus, connectStr); return false; } if ((devFlags & VIR_PCI_CONNECT_HOTPLUGGABLE) && !(busFlags & VIR_PCI_CONNECT_HOTPLUGGABLE)) { if (reportError) { virReportError(errType, - _("PCI bus is not compatible with the device " - "at %s. Device requires hot-plug capability, " - "which is not provided by bus %.4x:%.2x"), - addrStr, addr->domain, addr->bus); + _("The device at PCI address %s requires " + "hot-plug capability, but the PCI controller " + "at index %.2x doesn't support hot-plug"), + addrStr, addr->bus); } return false; } -- 2.7.4

I apparently misunderstood Marcel's description of what could and couldn't be plugged into qemu's pxb-pcie controller (known as pcie-expander-bus in libvirt) - I specifically allowed directly connecting a pcie-switch-upstream-port, and it turns out that causes the guest kernel to crash. This patch forbids such a connection, and updates the xml docs appropriately. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1361172 --- docs/formatdomain.html.in | 24 +++++++++++++----------- src/conf/domain_addr.c | 9 +++------ 2 files changed, 16 insertions(+), 17 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 5acb3b9..eddb8dd 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3346,8 +3346,8 @@ this expander bus, and bus numbers less than the specified value will be available to the next lower expander-bus (or the root-bus if there are no lower expander buses). If you do not - specify a busNumber, libvirt will find the lowest existing - busNumber in all other expander buses (or use 256 if there are + specify a busNr, libvirt will find the lowest existing + busNr in all other expander buses (or use 256 if there are no others) and auto-assign the busNr of that found bus - 2, which provides one bus number for the pci-expander-bus and one for the pci-bridge that is automatically attached to it (if @@ -3360,15 +3360,17 @@ 2nd bus-number is just being reserved for the pcie-root-port that must necessarily be connected to the bus in order to actually plug in an endpoint device. If you intend to plug - multiple devices into a pcie-expander-bus, you must instead - connect a pcie-switch-upstream-port to the - pcie-expander-bus, and multiple pcie-switch-downstream-ports - to the pcie-switch-downstream-port, and of course for this - to work properly, you will need to decrease the - pcie-expander-bus' busNr accordingly so that there are - enough unused bus numbers above it to accomodate giving out - one bus number for the upstream-port and one for each - downstream-port). + multiple devices into a pcie-expander-bus, you must connect + a pcie-switch-upstream-port to the pcie-root-bus that is + plugged into the pcie-expander-bus, and multiple + pcie-switch-downstream-ports to the + pcie-switch-upstream-port, and of course for this to work + properly, you will need to decrease the pcie-expander-bus' + busNr accordingly so that there are enough unused bus + numbers above it to accomodate giving out one bus number for + the upstream-port and one for each downstream-port (in + addition to the pcie-root-port and the pcie-expander-bus + itself). </p> </dd> <dt><code>node</code></dt> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 98176e2..faf5a5a 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -161,7 +161,7 @@ virDomainPCIAddressFlagsCompatible(virPCIDeviceAddressPtr addr, } return false; } - return true; + return true; } @@ -291,11 +291,8 @@ virDomainPCIAddressBusSetModel(virDomainPCIAddressBusPtr bus, bus->maxSlot = VIR_PCI_ADDRESS_SLOT_LAST; break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: - /* single slot, no hotplug, only accepts pcie-root-port or - * pcie-switch-upstream-port. - */ - bus->flags = (VIR_PCI_CONNECT_TYPE_PCIE_ROOT_PORT - | VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_UPSTREAM_PORT); + /* single slot, no hotplug, only accepts pcie-root-port */ + bus->flags = VIR_PCI_CONNECT_TYPE_PCIE_ROOT_PORT; bus->minSlot = 0; bus->maxSlot = 0; break; -- 2.7.4

libvirt had allowed a dmi-to-pci-bridge to be plugged in anywhere a normal PCIe endpoint can be connected, but this is wrong - it will only work if it's plugged into pcie-root (the PCIe root complex) or a pcie-expander-bus (the qemu device pxb-pcie). This patch adjusts the connection flags accordingly. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1363648 --- src/conf/domain_addr.c | 22 +++++++----- src/conf/domain_addr.h | 4 ++- .../qemuxml2argv-q35-dmi-bad-address1.xml | 39 ++++++++++++++++++++++ .../qemuxml2argv-q35-dmi-bad-address2.xml | 39 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 16 +++++++++ 5 files changed, 111 insertions(+), 9 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-dmi-bad-address1.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-dmi-bad-address2.xml diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index faf5a5a..caffd71 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -58,15 +58,17 @@ virDomainPCIControllerModelToConnectType(virDomainControllerModelPCI model) */ return VIR_PCI_CONNECT_TYPE_PCI_DEVICE; - case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: - /* dmi-to-pci-bridge and pcie-expander-bus are treated like - * PCIe devices (the part of pcie-expander-bus that is plugged - * in isn't the expander bus itself, but a companion device - * used for setting it up). + /* pcie-expander-bus is treated like a standard PCIe endpoint + * device (the part of pcie-expander-bus that is plugged in + * isn't the expander bus itself, but a companion device used + * for setting it up). */ return VIR_PCI_CONNECT_TYPE_PCIE_DEVICE; + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + return VIR_PCI_CONNECT_TYPE_DMI_TO_PCI_BRIDGE; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: return VIR_PCI_CONNECT_TYPE_PCIE_ROOT_PORT; @@ -260,7 +262,8 @@ virDomainPCIAddressBusSetModel(virDomainPCIAddressBusPtr bus, * allows it. */ bus->flags = (VIR_PCI_CONNECT_TYPE_PCIE_DEVICE - | VIR_PCI_CONNECT_TYPE_PCIE_ROOT_PORT); + | VIR_PCI_CONNECT_TYPE_PCIE_ROOT_PORT + | VIR_PCI_CONNECT_TYPE_DMI_TO_PCI_BRIDGE); bus->minSlot = 1; bus->maxSlot = VIR_PCI_ADDRESS_SLOT_LAST; break; @@ -291,8 +294,11 @@ virDomainPCIAddressBusSetModel(virDomainPCIAddressBusPtr bus, bus->maxSlot = VIR_PCI_ADDRESS_SLOT_LAST; break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: - /* single slot, no hotplug, only accepts pcie-root-port */ - bus->flags = VIR_PCI_CONNECT_TYPE_PCIE_ROOT_PORT; + /* single slot, no hotplug, only accepts pcie-root-port or + * dmi-to-pci-bridge + */ + bus->flags = (VIR_PCI_CONNECT_TYPE_PCIE_ROOT_PORT + | VIR_PCI_CONNECT_TYPE_DMI_TO_PCI_BRIDGE); bus->minSlot = 0; bus->maxSlot = 0; break; diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index 4584e0a..4ce4139 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -40,6 +40,7 @@ typedef enum { VIR_PCI_CONNECT_TYPE_PCIE_ROOT_PORT = 1 << 3, VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_UPSTREAM_PORT = 1 << 4, VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_DOWNSTREAM_PORT = 1 << 5, + VIR_PCI_CONNECT_TYPE_DMI_TO_PCI_BRIDGE = 1 << 6, } virDomainPCIConnectFlags; /* a combination of all bits that describe the type of connections @@ -49,7 +50,8 @@ typedef enum { (VIR_PCI_CONNECT_TYPE_PCI_DEVICE | VIR_PCI_CONNECT_TYPE_PCIE_DEVICE | \ VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_UPSTREAM_PORT | \ VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_DOWNSTREAM_PORT | \ - VIR_PCI_CONNECT_TYPE_PCIE_ROOT_PORT) + VIR_PCI_CONNECT_TYPE_PCIE_ROOT_PORT | \ + VIR_PCI_CONNECT_TYPE_DMI_TO_PCI_BRIDGE) /* combination of all bits that could be used to connect a normal * endpoint device (i.e. excluding the connection possible between an diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35-dmi-bad-address1.xml b/tests/qemuxml2argvdata/qemuxml2argv-q35-dmi-bad-address1.xml new file mode 100644 index 0000000..3a946dd --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35-dmi-bad-address1.xml @@ -0,0 +1,39 @@ +<domain type='qemu'> + <name>q35-test</name> + <uuid>11dbdcdd-4c3b-482b-8903-9bdb8c0a2774</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>2097152</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='sda' bus='sata'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <model name='i82801b11-bridge'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='56'/> + </controller> + <controller type='pci' index='3' model='pcie-root-port'/> + <controller type='pci' index='4' model='dmi-to-pci-bridge'> + <address type='pci' bus='2' slot='3'/> + </controller> + <video> + <model type='qxl' ram='65536' vram='32768' vgamem='8192' heads='1'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35-dmi-bad-address2.xml b/tests/qemuxml2argvdata/qemuxml2argv-q35-dmi-bad-address2.xml new file mode 100644 index 0000000..c3c1b6a --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35-dmi-bad-address2.xml @@ -0,0 +1,39 @@ +<domain type='qemu'> + <name>q35-test</name> + <uuid>11dbdcdd-4c3b-482b-8903-9bdb8c0a2774</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>2097152</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='sda' bus='sata'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <model name='i82801b11-bridge'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='56'/> + </controller> + <controller type='pci' index='3' model='pcie-root-port'/> + <controller type='pci' index='4' model='dmi-to-pci-bridge'> + <address type='pci' bus='3'/> + </controller> + <video> + <model type='qxl' ram='65536' vram='32768' vgamem='8192' heads='1'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index d594836..933f9be 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1631,6 +1631,22 @@ mymain(void) QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_ICH9_USB_EHCI1, QEMU_CAPS_DEVICE_VIDEO_PRIMARY, QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL); + DO_TEST_PARSE_ERROR("q35-dmi-bad-address1", + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_ICH9_AHCI, + QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_ICH9_USB_EHCI1, + QEMU_CAPS_DEVICE_VIDEO_PRIMARY, + QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL); + DO_TEST_PARSE_ERROR("q35-dmi-bad-address2", + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_ICH9_AHCI, + QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_ICH9_USB_EHCI1, + QEMU_CAPS_DEVICE_VIDEO_PRIMARY, + QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL); DO_TEST("q35-pm-disable", QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, QEMU_CAPS_ICH9_AHCI, -- 2.7.4

On 08/08/2016 02:25 PM, Andrea Bolognani wrote: No, I think you're misunderstanding Marcel's comment: I specifically asked him if what the reporter attempted (plugging a dmi-to-pci-bridge directly into a pcie-expander-bus) was supported, and he said "yes". He then said that the only difference is that you can't plug [endpoint] devices directly into it. A dmi-to-pci-bridge is not an endpoint device. Aside from endpoint devices, the only other things you are supposed to plug into pcie-root are pcie-root-ports and dmi-to-pci-bridges. I'll wait for Marcel to respond as well, but without this, there is no way to put a legacy PCI slot in a NUMA node on a Q35 guest, and it *does* apparently work. Sure, I can remove those caps, but I think I'll also need to remove the <video> from the XML. (consider them removed). I'll leave the mass changes to test case files to Cole :-P

More misunderstanding/mistaken assumptions on my part - I had thought that a pci-expander-bus could be plugged into any legacy PCI slot, and that pcie-expander-bus could be plugged into any PCIe slot. This isn't correct - they can both be plugged ontly into their respective root buses. This patch adds that restriction. --- src/conf/domain_addr.c | 38 ++++++++++++----- src/conf/domain_addr.h | 6 ++- src/qemu/qemu_domain_address.c | 2 + .../qemuxml2argv-pci-expander-bus-bad-bus.xml | 26 ++++++++++++ .../qemuxml2argv-pcie-expander-bus-bad-bus.xml | 48 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 8 ++++ 6 files changed, 116 insertions(+), 12 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pci-expander-bus-bad-bus.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-expander-bus-bad-bus.xml diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index caffd71..27c41cd 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -51,20 +51,25 @@ virDomainPCIControllerModelToConnectType(virDomainControllerModelPCI model) return 0; case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: - case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: - /* pci-bridge and pci-expander-bus are treated like a standard - * PCI endpoint device, because they can plug into any - * standard PCI slot. + /* pci-bridge is treated like a standard + * PCI endpoint device, because it can plug into any + * standard PCI slot (it just can't be hotplugged). */ return VIR_PCI_CONNECT_TYPE_PCI_DEVICE; + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: + /* pci-expander-bus can only be plugged into pci-root + * (and pci-root is the only bus that has this flag set) + */ + return VIR_PCI_CONNECT_TYPE_PCI_EXPANDER_BUS; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: - /* pcie-expander-bus is treated like a standard PCIe endpoint - * device (the part of pcie-expander-bus that is plugged in - * isn't the expander bus itself, but a companion device used - * for setting it up). + /* pcie-expander-bus can only be plugged into pcie-root (the + * part of pcie-expander-bus that is plugged in isn't the + * expander bus itself, but a companion device used for + * setting it up). */ - return VIR_PCI_CONNECT_TYPE_PCIE_DEVICE; + return VIR_PCI_CONNECT_TYPE_PCIE_EXPANDER_BUS; case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: return VIR_PCI_CONNECT_TYPE_DMI_TO_PCI_BRIDGE; @@ -135,6 +140,10 @@ virDomainPCIAddressFlagsCompatible(virPCIDeviceAddressPtr addr, connectStr = "pci-switch-upstream-port"; } else if (devFlags & VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_DOWNSTREAM_PORT) { connectStr = "pci-switch-downstream-port"; + } else if (devFlags & VIR_PCI_CONNECT_TYPE_PCI_EXPANDER_BUS) { + connectStr = "pci-expander-bus"; + } else if (devFlags & VIR_PCI_CONNECT_TYPE_PCIE_EXPANDER_BUS) { + connectStr = "pcie-expander-bus"; } else { /* this should never happen. If it does, there is a * bug in the code that sets the flag bits for devices. @@ -241,9 +250,15 @@ virDomainPCIAddressBusSetModel(virDomainPCIAddressBusPtr bus, * bus. */ switch (model) { - case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: bus->flags = (VIR_PCI_CONNECT_HOTPLUGGABLE | + VIR_PCI_CONNECT_TYPE_PCI_DEVICE + | VIR_PCI_CONNECT_TYPE_PCI_EXPANDER_BUS); + bus->minSlot = 1; + bus->maxSlot = VIR_PCI_ADDRESS_SLOT_LAST; + break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + bus->flags = (VIR_PCI_CONNECT_HOTPLUGGABLE | VIR_PCI_CONNECT_TYPE_PCI_DEVICE); bus->minSlot = 1; bus->maxSlot = VIR_PCI_ADDRESS_SLOT_LAST; @@ -263,7 +278,8 @@ virDomainPCIAddressBusSetModel(virDomainPCIAddressBusPtr bus, */ bus->flags = (VIR_PCI_CONNECT_TYPE_PCIE_DEVICE | VIR_PCI_CONNECT_TYPE_PCIE_ROOT_PORT - | VIR_PCI_CONNECT_TYPE_DMI_TO_PCI_BRIDGE); + | VIR_PCI_CONNECT_TYPE_DMI_TO_PCI_BRIDGE + | VIR_PCI_CONNECT_TYPE_PCIE_EXPANDER_BUS); bus->minSlot = 1; bus->maxSlot = VIR_PCI_ADDRESS_SLOT_LAST; break; diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index 4ce4139..596cd4c 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -41,6 +41,8 @@ typedef enum { VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_UPSTREAM_PORT = 1 << 4, VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_DOWNSTREAM_PORT = 1 << 5, VIR_PCI_CONNECT_TYPE_DMI_TO_PCI_BRIDGE = 1 << 6, + VIR_PCI_CONNECT_TYPE_PCI_EXPANDER_BUS = 1 << 7, + VIR_PCI_CONNECT_TYPE_PCIE_EXPANDER_BUS = 1 << 8, } virDomainPCIConnectFlags; /* a combination of all bits that describe the type of connections @@ -51,7 +53,9 @@ typedef enum { VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_UPSTREAM_PORT | \ VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_DOWNSTREAM_PORT | \ VIR_PCI_CONNECT_TYPE_PCIE_ROOT_PORT | \ - VIR_PCI_CONNECT_TYPE_DMI_TO_PCI_BRIDGE) + VIR_PCI_CONNECT_TYPE_DMI_TO_PCI_BRIDGE | \ + VIR_PCI_CONNECT_TYPE_PCI_EXPANDER_BUS | \ + VIR_PCI_CONNECT_TYPE_PCIE_EXPANDER_BUS) /* combination of all bits that could be used to connect a normal * endpoint device (i.e. excluding the connection possible between an diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 3d52d72..202f92b 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1014,6 +1014,8 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def, * controller/bus to connect it to on the upstream side. */ flags = virDomainPCIControllerModelToConnectType(model); + VIR_WARN("flags = %04x model = %d", + flags, model); if (virDomainPCIAddressReserveNextSlot(addrs, &def->controllers[i]->info, flags) < 0) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pci-expander-bus-bad-bus.xml b/tests/qemuxml2argvdata/qemuxml2argv-pci-expander-bus-bad-bus.xml new file mode 100644 index 0000000..85c1115 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pci-expander-bus-bad-bus.xml @@ -0,0 +1,26 @@ +<domain type='qemu'> + <name>expander-test</name> + <uuid>3ec6cbe1-b5a2-4515-b800-31a61855df41</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>16</vcpu> + <os> + <type arch='x86_64' machine='pc-i440fx-2.5'>hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0' model='none'/> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <controller type='pci' index='1' model='pci-bridge'> + </controller> + <controller type='pci' index='2' model='pci-expander-bus'> + <address type='pci' bus='1' slot='1'/> + </controller> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-expander-bus-bad-bus.xml b/tests/qemuxml2argvdata/qemuxml2argv-pcie-expander-bus-bad-bus.xml new file mode 100644 index 0000000..0305f35 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-expander-bus-bad-bus.xml @@ -0,0 +1,48 @@ +<domain type='qemu'> + <name>pcie-expander-bus-test</name> + <uuid>11dbdcdd-4c3b-482b-8903-9bdb8c0a2774</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>2097152</currentMemory> + <vcpu placement='static'>16</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <cpu> + <topology sockets='2' cores='4' threads='2'/> + <numa> + <cell cpus='0-7' memory='109550' unit='KiB'/> + <cell cpus='8-15' memory='109550' unit='KiB'/> + </numa> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='sda' bus='sata'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <model name='i82801b11-bridge'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='56'/> + </controller> + <controller type='pci' index='3' model='pcie-root-port'> + <target busNr='220'> + <node>1</node> + </target> + <address type='pci' bus='0x00'/> + </controller> + <controller type='pci' index='4' model='pcie-expander-bus'> + <address type='pci' bus='3'/> + </controller> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 933f9be..2de4f0a 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1738,6 +1738,9 @@ mymain(void) DO_TEST_PARSE_ERROR("pci-expander-bus-bad-machine", QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_PXB); + DO_TEST_PARSE_ERROR("pci-expander-bus-bad-bus", + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_PXB); DO_TEST("pcie-expander-bus", QEMU_CAPS_DEVICE_PCI_BRIDGE, @@ -1753,6 +1756,11 @@ mymain(void) QEMU_CAPS_DEVICE_X3130_UPSTREAM, QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM, QEMU_CAPS_DEVICE_PXB_PCIE); + DO_TEST_PARSE_ERROR("pcie-expander-bus-bad-bus", + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_DEVICE_PXB_PCIE); DO_TEST("hostdev-scsi-lsi", QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_SCSI_LSI, -- 2.7.4

On 08/09/2016 05:01 AM, Andrea Bolognani wrote: BTW, I forgot to add this to the commit log: Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1358712 I've added it now. Yeah, these comments are remnants of an earlier time when there wasn't a nearly 1:1 match between the controller names and connect types. I had originally thought that some of the controllers behaved identically with each other and/or endpoint devices, so there wasn't always a direct conversion, and the comments here made sense (since this is where e.g. DMI_TO_PCI_BRIDGE became PCIE_ENDPOINT). But one by one I've found behavior for every one of the controllers (except pci-bridge) that warrants them having their own connect class. I've removed the comments here. Yeah, I noticed that and fixed it locally within a few hours after I posted the patches. I did a grep for | at the beginning and at the end of lines, and found it was 10:1 in favor of putting the | at the end of the line, so that's how they all are now. (it's a bit difficult for me to overcome muscle memory on this topic - at a couple of previous jobs the standard was to have operators of multiline expressions be at the beginning of the 2nd line) You can't plug a pcie-switch-upstream-port directly into a pcie-expander bus (see https://bugzilla.redhat.com/show_bug.cgi?id=1361172 which was the subject of patch 2) You can however plug in a pcie-root-port, and then connect a pcie-switch-upstream-port into the pcie-root-port. (and then of course plug a bunch of pcie-switch-downstream-ports into the upstream-port, and devices into the downstream ports). Yep. Already noticed and removed locally. So does this get ACK with the changes/explanations above? Or do you want me to re-send it?