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?