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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list