[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 24/26] conf: Implement isolation rules



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 redhat com>
> ---
>  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 laine org>

assuming the function is added and typo fixed.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]