[libvirt] [PATCH 4/3] conf: restrict expander buses to connect only to a root bus

Laine Stump laine at laine.org
Tue Aug 9 15:59:12 UTC 2016


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?




More information about the libvir-list mailing list