[libvirt] [PATCH v5 03/15] qemu: set/use info->pciConnectFlags when validating/assigning PCI addresses

Laine Stump laine at laine.org
Tue Oct 25 18:08:56 UTC 2016


On 10/25/2016 12:53 PM, Andrea Bolognani wrote:
> On Tue, 2016-10-25 at 09:49 -0400, Laine Stump wrote:
>> Set pciConnectFlags in each device's DeviceInfo and then use those
>> flags later when validating existing addresses in
>> qemuDomainCollectPCIAddress() and when assigning new addresses with
>> qemuDomainPCIAddressReserveNextAddr() (rather than scattering the
>> logic about which devices need which type of slot all over the place).
>>   
>> Note that the exact flags set by
>> qemuDomainDeviceCalculatePCIConnectFlags() are different from the
>> flags previously set manually in qemuDomainCollectPCIAddress(), but
>> this doesn't matter because all validation of addresses in that case
>> ignores the setting of the HOTPLUGGABLE flag, and treats PCIE_DEVICE
>> and PCI_DEVICE the same (this lax checking was done on purpose,
>> because there are some things that we want to allow the user to
>> specify manually, e.g. assigning a PCIe device to a PCI slot, that we
>> *don't* ever want libvirt to do automatically. The flag settings that
>> we *really* want to match are 1) the old flag settings in
>> qemuDomainAssignDevicePCISlots() (which is HOTPLUGGABLE | PCI_DEVICE
>> for everything except PCI controllers) and the new flag settings done
> [...] and 2) the new [...]
>
>> by qemuDomainDeviceCalculatePCIConnectFlags() (which are currently
>> exactly that - HOTPLUGGABLE | PCI_DEVICE for everything except PCI
>> controllers).
>> ---
>>    src/qemu/qemu_domain_address.c | 205 +++++++++++++++--------------------------
>>    1 file changed, 74 insertions(+), 131 deletions(-)
>>   
>> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
>> index 602179c..d731b19 100644
>> --- a/src/qemu/qemu_domain_address.c
>> +++ b/src/qemu/qemu_domain_address.c
>> @@ -721,7 +721,7 @@ qemuDomainFillDevicePCIConnectFlagsIter(virDomainDefPtr def ATTRIBUTE_UNUSED,
>>     * failure would be some internal problem with
>>     * virDomainDeviceInfoIterate())
>>     */
>> -static int ATTRIBUTE_UNUSED
>> +static int
>>    qemuDomainFillAllPCIConnectFlags(virDomainDefPtr def,
>>                                     virQEMUCapsPtr qemuCaps)
>>    {
> You should remove ATTRIBUTE_UNUSED from
> qemuDomainFillDevicePCIConnectFlags() as well.
>
> [...]
>> @@ -926,7 +857,7 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,
>>        entireSlot = (addr->function == 0 &&
>>                      addr->multi != VIR_TRISTATE_SWITCH_ON);
>>    
>> -    if (virDomainPCIAddressReserveAddr(addrs, addr, flags,
>> +    if (virDomainPCIAddressReserveAddr(addrs, addr, info->pciConnectFlags,
>>                                           entireSlot, true) < 0)
> Would it be cleaner to have a qemuDomainPCIAddressReserveAddr()
> function that takes @info directly?

Actually in a later series (the one that cleans up the *Slot() vs 
*Addr() naming), I eliminated all but one of the 
qemuDomainPCIAddressReserve*() functions anyway. After that series, 
there are only two *PCIAddressReserve*() functions used in this file: 
qemuDomainPCIAddressReserveNextAddr() (21 times), and 
virDomainPCIAddressReserveAddr() (12 times). The latter can't have a 
nice flags-removing wrapper added in qemu_domain_address.c (like the 
former does) because it often is called with a bare address - no DeviceInfo

(Well, I don't know, maybe it could be done by reorganizing some of the 
calls, I'll have to look at it).




>
> It would be used only a single time, so it could very well be
> argued that it would be overkill... On the other hand, it would
> be neat not to use virDomainPCIAddressReserve*() functions at
> all in the qemu driver and rely solely on the wrappers instead.
>
> Speaking of which, even with the full series applied there
> are still a bunch of uses of virDomainPCIAddressReserveAddr()
> and virDomainPCIAddressReserveSlot(), mostly in
> qemuDomainValidateDevicePCISlots{PIIX3,Q35}().

Yeah, my later series turns all of those into 
virDomainPCIAddressReserveAddr().

>
> The attached patch makes all of them go away, except a few
> that actually make sense because they set aside addresses for
> potential future devices and as such don't have access to
> a virDomainDeviceInfo yet.
>
> I'm not saying we should deal with this right away: I'd
> rather go back later to tidy up the whole thing than hold up
> the series or go through another round of reviews for what
> is ultimately a cosmetic issue.
>
> [...]
>> @@ -1878,24 +1795,50 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
>>             */
>>            if (!buses_reserved &&
>>                !qemuDomainMachineIsVirt(def) &&
>> -            qemuDomainPCIAddressReserveNextSlot(addrs, &info, flags) < 0)
>> +            qemuDomainPCIAddressReserveNextSlot(addrs, &info) < 0)
>>                goto cleanup;
>>    
>>            if (qemuDomainAssignDevicePCISlots(def, qemuCaps, addrs) < 0)
>>                goto cleanup;
>>    
>>            for (i = 1; i < addrs->nbuses; i++) {
>> +            virDomainDeviceDef dev;
>> +            int contIndex;
>>                virDomainPCIAddressBusPtr bus = &addrs->buses[i];
>>    
>>                if ((rv = virDomainDefMaybeAddController(
>>                         def, VIR_DOMAIN_CONTROLLER_TYPE_PCI,
>>                         i, bus->model)) < 0)
>>                    goto cleanup;
>> -            /* If we added a new bridge, we will need one more address */
>> -            if (rv > 0 &&
>> -                qemuDomainPCIAddressReserveNextSlot(addrs, &info, flags) < 0)
>> +
>> +            if (rv == 0)
>> +                continue; /* no new controller added */
> Alternatively, you could turn this into
>
>    if (rv > 0) {
>        virDomainDeviceDef dev;
>        int contIndex;
>
>        /* The code below */
>    }
>
> but I leave up to you whether to go one way or the other.

I like the reduced indent level of doing it this way :-)

>
>> +
>> +            /* We did add a new controller, so we will need one more
>> +             * address (and we need to set the new controller's
>> +             * pciConnectFlags)
>> +             */
>> +            contIndex = virDomainControllerFind(def,
>> +                                                VIR_DOMAIN_CONTROLLER_TYPE_PCI,
>> +                                                i);
>> +            if (contIndex < 0) {
>> +                /* this should never happen - we just added it */
>> +                virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                               _("Could not find auto-added %s controller "
>> +                                 "with index %zu"),
>> +                               virDomainControllerModelPCITypeToString(bus->model),
>> +                               i);
>> +                goto cleanup;
>> +            }
>> +            dev.type = VIR_DOMAIN_DEVICE_CONTROLLER;
>> +            dev.data.controller = def->controllers[contIndex];
>> +            /* set connect flags so it will be properly addressed */
>> +            qemuDomainFillDevicePCIConnectFlags(def, &dev, qemuCaps);
>> +            if (qemuDomainPCIAddressReserveNextSlot(addrs,
>> +                                                    &dev.data.controller->info) < 0)
>>                    goto cleanup;
>>            }
>> +
>>            nbuses = addrs->nbuses;
>>            virDomainPCIAddressSetFree(addrs);
>>            addrs = NULL;
> ACK
>
> -- 
> Andrea Bolognani / Red Hat / Virtualization





More information about the libvir-list mailing list