[libvirt] [PATCH v6 10/17] qemu: auto-add pcie-root-port/dmi-to-pci-bridge controllers as needed

[Laine Stump]

On 11/09/2016 09:19 AM, Andrea Bolognani wrote:
> On Mon, 2016-11-07 at 14:50 -0500, Laine Stump wrote:
>> Previously libvirt would only add pci-bridge devices automatically
>> when an address was requested for a device that required a legacy PCI
>> slot and none was available. This patch expands that support to
>> dmi-to-pci-bridge (which is needed in order to add a pci-bridge on a
>> machine with a pcie-root), and pcie-root-port (which is needed to add
>> a hotpluggable PCIe device). It does *not* automatically add
>> pcie-switch-upstream-ports or pcie-switch-downstream-ports (and
>> currently there are no plans for that).
> [...]
>> Although libvirt will now automatically create a dmi-to-pci-bridge
>> when it's needed, the code still remains for now that forces a
>> dmi-to-pci-bridge on all domains with pcie-root (in
>> qemuDomainDefAddDefaultDevices()). That will be removed in the next
>> patch.
> s/in the next/in a future/
>
>> For now, the pcie-root-ports are added one to a slot, which is a bit
>> wasteful and means it will fail after 31 total PCIe devices (30 if
>> there are also some PCI devices), but helps keep the changeset down
>> for this patch. A future patch will have 8 pcie-root-ports sharing the
>> functions on a single slot.
> [...]
>> @@ -82,6 +82,30 @@ virDomainPCIControllerModelToConnectType(virDomainControllerModelPCI model)
>>        return 0;
>>    }
>>    
>> +
>> +static int
>> +virDomainPCIControllerConnectTypeToModel(virDomainPCIConnectFlags flags)
>> +{
>> +    if (flags & VIR_PCI_CONNECT_TYPE_PCIE_ROOT_PORT)
>> +        return VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT;
> Maybe adding an empty line between each branch would make
> this a little more readable? Just a thought.

Okay.

>
> Also too bad these are flags and we can't use a switch()
> statement here - the compiler will not warn us if we forget
> to handle a VIR_PCI_CONNECT_TYPE in the future :(


Yeah, I may rework the flags someday to separate out the hotplug and 
aggregate_slot flags so that the connect type can be switched on.


>
> [...]
>> @@ -351,32 +372,73 @@ virDomainPCIAddressSetGrow(virDomainPCIAddressSetPtr addrs,
>>    {
>>        int add;
>>        size_t i;
>> +    int model;
>> +    bool needDMIToPCIBridge = false;
>>    
>>        add = addr->bus - addrs->nbuses + 1;
>> -    i = addrs->nbuses;
>>        if (add <= 0)
>>            return 0;
>>    
>> -    /* auto-grow only works when we're adding plain PCI devices */
>> -    if (!(flags & VIR_PCI_CONNECT_TYPE_PCI_DEVICE)) {
>> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> -                       _("Cannot automatically add a new PCI bus for a "
>> -                         "device requiring a slot other than standard PCI."));
>> +    if (flags & VIR_PCI_CONNECT_TYPE_PCI_DEVICE) {
>> +        model = VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE;
>> +
>> +        /* if there aren't yet any buses that will accept a
>> +         * pci-bridge, and the caller is asking for one, we'll need to
>> +         * add a dmi-to-pci-bridge first.
>> +         */
>> +        needDMIToPCIBridge = true;
>> +        for (i = 0; i < addrs->nbuses; i++) {
>> +            if (addrs->buses[i].flags & VIR_PCI_CONNECT_TYPE_PCI_BRIDGE) {
>> +                needDMIToPCIBridge = false;
>> +                break;
>> +            }
>> +        }
>> +        if (needDMIToPCIBridge && add == 1) {
>> +            /* we need to add at least two buses - one dmi-to-pci,
>> +             * and the other the requested pci-bridge
>> +             */
>> +            add++;
>> +            addr->bus++;
>> +        }
> This last part got me confused for a bit, so I wouldn't mind
> having a more detailed comment here. Something like
>
>    We need to add a single pci-bridge to provide the bus our
>    legacy PCI device will be plugged into; however, we have
>    also determined that the pci-bridge we're about to add
>    itself needs to be plugged into a dmi-to-pci-bridge. To
>    accomodate both requirements, we're going to add both a
>    dmi-to-pci-bridge and a pci-bridge, and we're going to
>    bump the bus number of the device by one to account for
>    the extra controller.
>
> Yeah, that's quite a mouthful :/

Okay, I added a variation of that description.


>
>> +    } else if (flags & VIR_PCI_CONNECT_TYPE_PCI_BRIDGE &&
>> +               addrs->buses[0].model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) {
>> +        model = VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE;
> Mh, what about the case where we want to add a pci-bridge
> but the machine has a pci-root instead of a pcie-root?
> Shouldn't that case be handled as well?

It is handled - we'll want to add a pci-bridge if flags & 
VIR_PCI_TYPE_PCI_DEVICE is true (i.e. the first clause). In that case, 
since pci-root has the flag VIR_PCI_CONNECT_TYPE_PCI_BRIDGE set, the for 
loop will set neetDMIToPCIBridge = false, so we will end up adding just 
the one pci-bridge that we need.

>
>> +    } else if (flags & (VIR_PCI_CONNECT_TYPE_PCIE_DEVICE |
>> +                        VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_UPSTREAM_PORT)) {
>> +        model = VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT;
>> +    } else {
>> +        int existingContModel = virDomainPCIControllerConnectTypeToModel(flags);
>> +
>> +        if (existingContModel >= 0) {
>> +            virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                           _("a PCI slot is needed to connect a PCI controller "
>> +                             "model='%s', but none is available, and it "
>> +                             "cannot be automatically added"),
>> +                           virDomainControllerModelPCITypeToString(existingContModel));
>> +        } else {
>> +            virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                           _("Cannot automatically add a new PCI bus for a "
>> +                             "device with connect flags %.2x"), flags);
>> +        }
> So we error out for dmi-to-pci-bridge and
> pci{,e}-expander-bus... Shouldn't we be able to plug either
> into into pcie-root or pci-root?

Exactly. And since those buses already exist from the start, and can't 
be added later, there wouldn't ever be a situation where we needed to 
automatically grow the bus structure due to one of those devices and 
growing could actually do anything (i.e. you can't add a pcie-root or 
pci-root).

>
>>            return -1;
>>        }
>>    
>> +    i = addrs->nbuses;
>> +
>>        if (VIR_EXPAND_N(addrs->buses, addrs->nbuses, add) < 0)
>>            return -1;
>>    
>> -    for (; i < addrs->nbuses; i++) {
>> -        /* Any time we auto-add a bus, we will want a multi-slot
>> -         * bus. Currently the only type of bus we will auto-add is a
>> -         * pci-bridge, which is hot-pluggable and provides standard
>> -         * PCI slots.
>> +    if (needDMIToPCIBridge) {
>> +        /* first of the new buses is dmi-to-pci-bridge, the
>> +         * rest are of the requested type
>>             */
>> -        virDomainPCIAddressBusSetModel(&addrs->buses[i],
>> -                                       VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE);
>> +        virDomainPCIAddressBusSetModel(&addrs->buses[i++],
>> +                                       VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE);
> virDomainPCIAddressBusSetModel() can fail, yet you're not
> checking the return value neither here...

Yes, but the only way it can fail is if an unknown controller model is 
sent to it, and we can see by simple physical inspection that that isn't 
the case. If this was calling off to a function in some other file where 
we didn't know just how simple it is and couldn't easily see that it 
would never fail, then I would agree that we should check, but in this 
case it's superfluous - if this function had an ATTRIBUTE_RETURN_CHECK 
on it, I would have put these calls inside ignore_value().


>
>>        }
>> +
>> +    for (; i < addrs->nbuses; i++)
>> +        virDomainPCIAddressBusSetModel(&addrs->buses[i], model);
> ... nor here.

Same for this - the controller model is one of just a couple hardcoded 
values set within the same function, not sent in from somewhere else, 
and the function we're calling is a very simple function in the same file.

>
> [...]
>> @@ -947,7 +934,43 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def,
>>    
>>            if (virDomainPCIAddressBusSetModel(&addrs->buses[idx], cont->model) < 0)
>>                goto error;
>> -        }

Since I'm sure you'll bring up the fact that I *am* checking the return 
value of virDomainPCIAddressBusSetModel here (and everywhere else in 
qemu_domain_address.c), I'll point out that these calls are made from 
another file in another directory, so I'm being overly paranoid, not on 
any real factual basis, but just because it feels right :-)

>> +
>> +        if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)
>> +            hasPCIeRoot = true;
>> +    }
>> +
>> +    if (nbuses > 0 && !addrs->buses[0].model) {
>> +        /* This is just here to replicate a safety measure already in
>> +         * an older version of this code. In practice, the root bus
>> +         * should have already been added at index 0 prior to
>> +         * assigning addresses to devices.
>> +         */
>> +        if (virDomainPCIAddressBusSetModel(&addrs->buses[0],
>> +                                           VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) < 0)
>> +            goto error;
>> +    }
>> +
>> +    /* Now fill in a reasonable model for all the buses in the set
>> +     * that don't yet have a corresponding controller in the domain
>> +     * config.
>> +     */
>> +
> No empty line here.
>
>
> Rest looks good, but no ACK for you quite yet :)

So far I've changed comments and whitespace. You're welcome to dispute 
my opinion about checking return value from the SetModel function, but 
otherwise there's only cosmetic differences.


>
> -- 
> Andrea Bolognani / Red Hat / Virtualization
>