On 06/23/2017 11:03 AM, Andrea Bolognani wrote:
> These rules will make it possible for libvirt to
> automatically assign PCI addresses in a way that
> respects any isolation constraints devices might
> have.
>
> Signed-off-by: Andrea Bolognani <abologna@xxxxxxxxxx>
> ---
> src/conf/domain_addr.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 99 insertions(+), 5 deletions(-)
>
> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
> index 48af1f5..22ff014 100644
> --- a/src/conf/domain_addr.c
> +++ b/src/conf/domain_addr.c
> @@ -534,7 +534,7 @@ static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
> virDomainPCIAddressReserveAddrInternal(virDomainPCIAddressSetPtr addrs,
> virPCIDeviceAddressPtr addr,
> virDomainPCIConnectFlags flags,
> - int isolationGroup ATTRIBUTE_UNUSED,
> + int isolationGroup,
> bool fromConfig)
> {
> int ret = -1;
> @@ -542,6 +542,8 @@ virDomainPCIAddressReserveAddrInternal(virDomainPCIAddressSetPtr addrs,
> virDomainPCIAddressBusPtr bus;
> virErrorNumber errType = (fromConfig
> ? VIR_ERR_XML_ERROR : VIR_ERR_INTERNAL_ERROR);
> + bool firstDevice;
> + size_t slot;
>
> if (!(addrStr = virDomainPCIAddressAsString(addr)))
> goto cleanup;
> @@ -572,6 +574,36 @@ virDomainPCIAddressReserveAddrInternal(virDomainPCIAddressSetPtr addrs,
> bus->slot[addr->slot].aggregate = true;
> }
>
> + /* Figure out whether this is the first device we're plugging
> + * into the bus by looking at all the slots */
> + firstDevice = true;
> + for (slot = bus->minSlot; slot <= bus->maxSlot; slot++) {
> + if (bus->slot[slot].functions) {
> + firstDevice = false;
> + break;
> + }
> + }
You have the same loop 3 times in the code (although in one case the
bool is called "lastDevice" instead of "firstDevice". How about a short
utility function called virDomainPCIAddressBusIsEmpty() (or something
like that)?
> +
> + if (firstDevice && !bus->isolationGroupLocked) {
> + /* The first device decides the isolation group for the
> + * entire bus */
> + bus->isolationGroup = isolationGroup;
> + VIR_DEBUG("PCI bus %.4x:%.2x assigned isolation group %d because of "
> + "first device %s",
> + addr->domain, addr->bus, isolationGroup, addrStr);
I haven't tried out these patches, but it looks like all this stuff
happens for everybody all the time, not just for pSeries, correct? I
suppose it's an effective NOP if everything is group 0 though, right?
> + } else if (bus->isolationGroup != isolationGroup && fromConfig) {
> + /* If this is not the first function and its isolation group
> + * doesn't match the bus', then it should not be using this
> + * address. However, if the address comes from the user then
> + * we comply with the request and change the isolation group
> + * back to the default (because at that point isolation can't
> + * be guaranteed anymore) */
> + bus->isolationGroup = 0;
> + VIR_DEBUG("PCI bus %.4x:%.2x assigned isolation group %d because of "
> + "user assigned address %s",
> + addr->domain, addr->bus, isolationGroup, addrStr);
Is this what we want to do? Or is it a bad enough situation that we want
to log an error and fail? (I don't have any idea, that's why I'm asking :-)
> + }
> +
> /* mark the requested function as reserved */
> bus->slot[addr->slot].functions |= (1 << addr->function);
> VIR_DEBUG("Reserving PCI address %s (aggregate='%s')", addrStr,
> @@ -643,7 +675,28 @@ int
> virDomainPCIAddressReleaseAddr(virDomainPCIAddressSetPtr addrs,
> virPCIDeviceAddressPtr addr)
> {
> - addrs->buses[addr->bus].slot[addr->slot].functions &= ~(1 << addr->function);
> + virDomainPCIAddressBusPtr bus = &addrs->buses[addr->bus];
> + bool lastDevice;
> + size_t slot;
> +
> + bus->slot[addr->slot].functions &= ~(1 << addr->function);
> +
> + /* Figure out whether this is the first device we're plugging
> + * into the bus by looking at all the slots */
> + lastDevice = true;
Here's instance 2 of the "check for an empty bus" loop.
> + for (slot = bus->minSlot; slot <= bus->maxSlot; slot++) {
> + if (bus->slot[slot].functions) {
> + lastDevice = false;
> + break;
> + }
> + }
> +
> + /* If the one we just unplugged was the last device on the bus,
> + * we can reset the isolation group for the bus so that any
> + * device we'll try to plug in from now on will be accepted */
> + if (lastDevice && !bus->isolationGroupLocked)
> + bus->isolationGroup = 0;
Ah, for awhile I had been thinking that isolationGroupLocked would be
set as soon as an isolationGroup was set for a bus. But I guess instead
it's only set when specifically requested as the bus is added; otherwise
an isolationGroup of 0 means "none selected". (Or is it that "BusIsEmpty
== true && !isolationGroupLocked" means "none selected"?)
> +
> return 0;
> }
>
> @@ -749,7 +802,7 @@ static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
> virDomainPCIAddressGetNextAddr(virDomainPCIAddressSetPtr addrs,
> virPCIDeviceAddressPtr next_addr,
> virDomainPCIConnectFlags flags,
> - int isolationGroup ATTRIBUTE_UNUSED,
> + int isolationGroup,
> int function)
> {
> virPCIDeviceAddress a = { 0 };
> @@ -765,11 +818,50 @@ virDomainPCIAddressGetNextAddr(virDomainPCIAddressSetPtr addrs,
> else
> a.function = function;
>
> - /* "Begin at the beginning," the King said, very gravely, "and go on
> - * till you come to the end: then stop." */
> + /* When looking for a suitable bus for the device, start by being
> + * very strict and ignoring all those where the isolation groups
> + * don't match. This ensures all devices sharing the same isolation
> + * group will end up on the same group */
I think you wanted to say "on the same bus"
> + for (a.bus = 0; a.bus < addrs->nbuses; a.bus++) {
> + virDomainPCIAddressBusPtr bus = &addrs->buses[a.bus];
> + bool found = false;
> +
> + if (bus->isolationGroup != isolationGroup)
> + continue;
> +
> + a.slot = bus->minSlot;
> +
> + if (virDomainPCIAddressFindUnusedFunctionOnBus(bus, &a, function,
> + flags, &found) < 0) {
> + goto error;
> + }
> +
> + if (found)
> + goto success;
> + }
> +
> + /* We haven't been able to find a perfectly matching bus, but we
> + * might still be able to make this work by altering the isolation
> + * group for a bus that's currently empty. So let's try that */
> for (a.bus = 0; a.bus < addrs->nbuses; a.bus++) {
> virDomainPCIAddressBusPtr bus = &addrs->buses[a.bus];
> bool found = false;
> + bool firstDevice = true;
> + size_t slot;
> +
> + /* Go through all the slots and see whether they are empty */
> + for (slot = bus->minSlot; slot <= bus->maxSlot; slot++) {
> + if (bus->slot[slot].functions) {
> + firstDevice = false;
> + break;
> + }
> + }
...and here's the 3rd place you have the "check if the bus is empty" loop.
> +
> + /* We can only change the isolation group for a bus when
> + * plugging in the first device; moreover, some buses are
> + * prevented from ever changing it */
> + if (!firstDevice || bus->isolationGroupLocked)
Yeah, how does that 2nd thing happen? (setting isolationGroupLocked on
an empty bus) I see that happens in the next patch for the default
(first) PHB, but will it ever happen otherwise?
> + continue;
>
> a.slot = bus->minSlot;
>
> @@ -778,6 +870,8 @@ virDomainPCIAddressGetNextAddr(virDomainPCIAddressSetPtr addrs,
> goto error;
> }
>
> + /* The isolation group for the bus will actually be changed
> + * later, in virDomainPCIAddressReserveAddrInternal() */
> if (found)
> goto success;
> }
>
I think this just needs the one typo fixed and the utility "Is the bus
empty" function. The rest is just me asking for out-of-band explanations
of a couple things. I think I can trust your copy-paste sk1llz enough to
give
Reviewed-by: Laine Stump <laine@xxxxxxxxx>
assuming the function is added and typo fixed.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
