10:59 a.m.
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?