[libvirt] [PATCH 1/2] conf: new function virDomainPCIAddressSetAllMulti()

[Andrea Bolognani]

On Tue, 2017-01-10 at 03:23 -0500, Laine Stump wrote:
> This utility function looks through a virPCIAddressSet for any slot of
> any bus that has multiple functions in use, and sets the "multi" flag
> in the virDomainDeviceInfo for the device that is assigned to function
> 0 of that slot (as long as it hasn't already been set explicitly by
> someone who presumably has better information than we do).
> 
> It isn't yet called from anywhere, so will have no functional effect.

[...]
> +static int
> +virDomainPCIAddressSetMultiIter(virDomainDefPtr def ATTRIBUTE_UNUSED,
> +                                virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED,
> +                                virDomainDeviceInfoPtr info,
> +                                void *data)

virDomainPCIAddressSetAllMultiIter()?

Or not :)

[...]
> +/**
> + * virDomainPCIAddressSetAllMulti():
> + *
> + * @def: the domain definition whose devices may need adjusting
> + * @addrs: address set keeping track of all addresses in use.
> + *
> + * Look for any PCI slots that have multiple functions assigned, and
> + * set multi to YES in the address for the device at function 0

s/YES/ON/

> + * (unless it has been explicitly set to NO).

s/NO/OFF/

> + *
> + * No return code, since there is no possibility of failure.
> + */
> +void
> +virDomainPCIAddressSetAllMulti(virDomainDefPtr def,
> +                               virDomainPCIAddressSetPtr addrs)

I also just realized that virDomainPCIAddressSetAllMulti()
might be mistaken for vir::DomainPCIAddressSet::AllMulti()
instead of vir::Domain::PCIAddressSetAllMulti().

The joys of pretending to use an object-oriented language
when actually all you have is plain old C :)

> +{
> +    /* Scan through all the slots in @addrs looking for any that have
> +     * more than just function 0 marked as in use, then use an
> +     * iterator to find the DeviceInfo that uses function 0 on that
> +     * slot and mark it as multi = YES

s/YES/ON/

> +     */
> +    size_t busIdx, slotIdx;
> +
> +    for (busIdx = 0; busIdx < addrs->nbuses; busIdx++) {
> +        virDomainPCIAddressBusPtr bus = &addrs->buses[busIdx];
> +
> +        for (slotIdx = bus->minSlot; slotIdx <= bus->maxSlot; slotIdx++) {
> +            if (bus->slot[slotIdx].functions > 1) {
> +                virPCIDeviceAddress addr = { .domain = 0,

This doesn't look right. What happens if the multifunction
device is at eg. 0001:00:04.0?

...

Oh, never mind, apparently virDomainPCIAddressSet has no
concept of PCI domains anyways... So that's an existing
limitation - one that I assume we're going to have to
address, especially now that ppc64 guests are going to
want to use *lots* of separate PCI domains.


On the other hand, at the point where you're going to call
this function haven't PCI addresses already been assigned
and stored in @def? Why not iterate over it twice to make
the comparison future-proof? The computational cost would
be the same AFAICT.

-- 
Andrea Bolognani / Red Hat / Virtualization