[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