[libvirt] [PATCH 0/3] Adjust device compat. rules for dmi-to-pci-bridge and pcie-bus-expander
I got a few things wrong in the original implementation. Details in the individual patches. Laine Stump (3): conf: improve error log when PCI devices don't match requested controller conf: don't allow connecting upstream-port directly to pce-expander-bus conf: restrict where dmi-to-pci-bridge can be connected docs/formatdomain.html.in | 24 ++++--- src/conf/domain_addr.c | 79 ++++++++++++---------- src/conf/domain_addr.h | 4 +- .../qemuxml2argv-q35-dmi-bad-address1.xml | 39 +++++++++++ .../qemuxml2argv-q35-dmi-bad-address2.xml | 39 +++++++++++ tests/qemuxml2argvtest.c | 16 +++++ 6 files changed, 155 insertions(+), 46 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-dmi-bad-address1.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-dmi-bad-address2.xml -- 2.7.4
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
On Fri, 2016-08-05 at 23:01 -0400, Laine Stump wrote:
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);
s/hot-plug/hotplug/g ?
} return false; }
ACK -- Andrea Bolognani / Red Hat / Virtualization
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
On Fri, 2016-08-05 at 23:01 -0400, Laine Stump wrote:
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
This hunk is unrelated to the fix, please split it into a separate patch (which you can definitely then push as trivial).
@@ -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
s/pcie-root-bus/pcie-root-port/
+ 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; }
How did this hunk even get here? :)
@@ -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;
ACK with the comments addressed. -- Andrea Bolognani / Red Hat / Virtualization
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 Fri, 2016-08-05 at 23:01 -0400, Laine Stump wrote:
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
Doesn't https://bugzilla.redhat.com/1363648#c4 tell us that we *need* to use a pcie-root-port when plugging stuff into a pcie-expander-bus? Looks good otherwise, but I'd like to have Marcel's confirm that plugging a dmi-to-pci-bridge directly into a pcie-expander-bus is something we want before you push it.
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,
Can you maybe trim the capabilities a bit? QEMU_CAPS_VGA_QXL and friends look like they're hardly a requirement for a test case about PCI bridges. Of course I'm okay with this if you're already planning to post a follow-up patch that trims the capabilities of all q35-* test cases ;) -- Andrea Bolognani / Red Hat / Virtualization
On 08/08/2016 02:25 PM, Andrea Bolognani wrote:
On Fri, 2016-08-05 at 23:01 -0400, Laine Stump wrote:
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 Doesn't
https://bugzilla.redhat.com/1363648#c4
tell us that we *need* to use a pcie-root-port when plugging stuff into a pcie-expander-bus?
No, I think you're misunderstanding Marcel's comment:
(In reply to Laine Stump fromcomment #2 <https://bugzilla.redhat.com/show_bug.cgi?id=1363648#c2>)
Marcel - is this actually supported by qemu?
Yes, and if doesn't work is a bug. The best way to think of pxb/pbx-pcie is that it exposes a new pcie.0 like bus. The only difference is that you cannot have devices plugged directly into it (integrated device).
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.
Looks good otherwise, but I'd like to have Marcel's confirm that plugging a dmi-to-pci-bridge directly into a pcie-expander-bus is something we want before you push it.
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.
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, Can you maybe trim the capabilities a bit? QEMU_CAPS_VGA_QXL and friends look like they're hardly a requirement for a test case about PCI bridges.
Sure, I can remove those caps, but I think I'll also need to remove the <video> from the XML. (consider them removed).
Of course I'm okay with this if you're already planning to post a follow-up patch that trims the capabilities of all q35-* test cases ;)
I'll leave the mass changes to test case files to Cole :-P
On Mon, 2016-08-08 at 15:33 -0400, Laine Stump wrote:
Marcel - is this actually supported by qemu? Yes, and if doesn't work is a bug. The best way to think of pxb/pbx-pcie is that it exposes a new pcie.0 like bus. The only difference is that you cannot have devices plugged
Doesn't https://bugzilla.redhat.com/1363648#c4 tell us that we *need* to use a pcie-root-port when plugging stuff into a pcie-expander-bus? No, I think you're misunderstanding Marcel's comment: (In reply to Laine Stump from comment #2) directly into it (integrated device). 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.
The "endpoint" part is the one I was missing. It makes sense now :) -- Andrea Bolognani / Red Hat / Virtualization
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 Sat, 2016-08-06 at 19:22 -0400, Laine Stump wrote:
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
s/ontly/only/
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)
The second line of the comment seems a little redundant. More to the point, since this function is just mapping controller models to connect flags, commenting on how the returned flags are going to be used seem out of place - better to keep that kind of comment for when setting the connect flags on a bus IMHO.
+ */ + 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;
Same as above.
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);
Please don't mix FLAG1 | FLAG2 with FLAG1 | FLAG2 Actually, the second style is only really used in a couple of spots AFAICT, so it would be preferrable to stick to the first one for consistency's sake. Ideally that would be done on a separate patch placed before this one, but if you don't feel like doing that I can clean it up with a follow-up patch - just don't mix the two styles in your patches, please :)
+ 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);
The comment above this reads /* slots 1 - 31, no hotplug, PCIe endpoint device or * pcie-root-port only, unless the address was specified in * user config *and* the particular device being attached also * allows it. */ It looks like it should be updated to reflect your changes and the fact that dmi-to-pci-bridge is also allowed. Don't know about the last bit :) One thing that's not quite clear to me about pcie-expander-bus: according to the code and comment, it has a single slot that can accomodate either a pcie-root-port or a dmi-to-pci-bridge. It seems like that would limit its usefulness quite a lot... I would expect at least a pcie-switch-upstream-port to be usable as well. Marcel, can you confirm either way?
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);
Development artifact, I suppose? -- Andrea Bolognani / Red Hat / Virtualization
On 08/09/2016 05:01 AM, Andrea Bolognani wrote:
On Sat, 2016-08-06 at 19:22 -0400, Laine Stump wrote:
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 s/ontly/only/
buses. This patch adds that restriction.
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.
--- 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) The second line of the comment seems a little redundant.
More to the point, since this function is just mapping controller models to connect flags, commenting on how the returned flags are going to be used seem out of place - better to keep that kind of comment for when setting the connect flags on a bus IMHO.
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.
+ */ + 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; Same as above.
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);
Please don't mix
FLAG1 | FLAG2
with
FLAG1 | FLAG2
Actually, the second style is only really used in a couple of spots AFAICT, so it would be preferrable to stick to the first one for consistency's sake.
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)
Ideally that would be done on a separate patch placed before this one, but if you don't feel like doing that I can clean it up with a follow-up patch - just don't mix the two styles in your patches, please :)
+ 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); The comment above this reads
/* slots 1 - 31, no hotplug, PCIe endpoint device or * pcie-root-port only, unless the address was specified in * user config *and* the particular device being attached also * allows it. */
It looks like it should be updated to reflect your changes and the fact that dmi-to-pci-bridge is also allowed. Don't know about the last bit :)
One thing that's not quite clear to me about pcie-expander-bus: according to the code and comment, it has a single slot that can accomodate either a pcie-root-port or a dmi-to-pci-bridge.
It seems like that would limit its usefulness quite a lot... I would expect at least a pcie-switch-upstream-port to be usable as well. Marcel, can you confirm either way?
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).
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);
Development artifact, I suppose?
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?
On Tue, 2016-08-09 at 11:59 -0400, Laine Stump wrote:
One thing that's not quite clear to me about pcie-expander-bus: according to the code and comment, it has a single slot that can accomodate either a pcie-root-port or a dmi-to-pci-bridge. It seems like that would limit its usefulness quite a lot... I would expect at least a pcie-switch-upstream-port to be usable as well. Marcel, can you confirm either way? 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).
I see. That's a bit unfortunate, I guess, but at least it's not entirely pointless :)
So does this get ACK with the changes/explanations above? Or do you want me to re-send it?
https://i.imgflip.com/18o30w.jpg -- Andrea Bolognani / Red Hat / Virtualization
participants (2)
-
Andrea Bolognani -
Laine Stump