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>
Reviewed-by: Laine Stump <laine@laine.org>
---
 src/conf/domain_addr.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++---
 src/conf/domain_addr.h |  3 +++
 2 files changed, 72 insertions(+), 4 deletions(-)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index bb095a3..531fc68 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -369,6 +369,20 @@ virDomainPCIAddressBusIsFullyReserved(virDomainPCIAddressBusPtr bus)
 }
 
 
+bool
+virDomainPCIAddressBusIsEmpty(virDomainPCIAddressBusPtr bus)
+{
+    size_t i;
+
+    for (i = bus->minSlot; i <= bus->maxSlot; i++) {
+        if (bus->slot[i].functions)
+            return false;
+    }
+
+    return true;
+}
+
+
 /* Ensure addr fits in the address set, by expanding it if needed
  *
  * Return value:
@@ -548,7 +562,7 @@ static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
 virDomainPCIAddressReserveAddrInternal(virDomainPCIAddressSetPtr addrs,
                                        virPCIDeviceAddressPtr addr,
                                        virDomainPCIConnectFlags flags,
-                                       unsigned int isolationGroup ATTRIBUTE_UNUSED,
+                                       unsigned int isolationGroup,
                                        bool fromConfig)
 {
     int ret = -1;
@@ -586,6 +600,26 @@ virDomainPCIAddressReserveAddrInternal(virDomainPCIAddressSetPtr addrs,
         bus->slot[addr->slot].aggregate = true;
     }
 
+    if (virDomainPCIAddressBusIsEmpty(bus) && !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 %u because of "
+                  "first device %s",
+                  addr->domain, addr->bus, isolationGroup, addrStr);
+    } 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 %u because of "
+                  "user assigned address %s",
+                  addr->domain, addr->bus, isolationGroup, addrStr);
+    }
+
     /* mark the requested function as reserved */
     bus->slot[addr->slot].functions |= (1 << addr->function);
     VIR_DEBUG("Reserving PCI address %s (aggregate='%s')", addrStr,
@@ -763,7 +797,7 @@ static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
 virDomainPCIAddressGetNextAddr(virDomainPCIAddressSetPtr addrs,
                                virPCIDeviceAddressPtr next_addr,
                                virDomainPCIConnectFlags flags,
-                               unsigned int isolationGroup ATTRIBUTE_UNUSED,
+                               unsigned int isolationGroup,
                                int function)
 {
     virPCIDeviceAddress a = { 0 };
@@ -779,12 +813,41 @@ 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 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;
+
+        /* 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 (!virDomainPCIAddressBusIsEmpty(bus) || bus->isolationGroupLocked)
+            continue;
+
         a.slot = bus->minSlot;
 
         if (virDomainPCIAddressFindUnusedFunctionOnBus(bus, &a, function,
@@ -792,6 +855,8 @@ virDomainPCIAddressGetNextAddr(virDomainPCIAddressSetPtr addrs,
             goto error;
         }
 
+        /* The isolation group for the bus will actually be changed
+         * later, in virDomainPCIAddressReserveAddrInternal() */
         if (found)
             goto success;
     }
diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
index ac6d64f..205e7cf 100644
--- a/src/conf/domain_addr.h
+++ b/src/conf/domain_addr.h
@@ -142,6 +142,9 @@ int virDomainPCIAddressBusSetModel(virDomainPCIAddressBusPtr bus,
 bool virDomainPCIAddressBusIsFullyReserved(virDomainPCIAddressBusPtr bus)
     ATTRIBUTE_NONNULL(1);
 
+bool virDomainPCIAddressBusIsEmpty(virDomainPCIAddressBusPtr bus)
+    ATTRIBUTE_NONNULL(1);
+
 bool virDomainPCIAddressSlotInUse(virDomainPCIAddressSetPtr addrs,
                                   virPCIDeviceAddressPtr addr)
     ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
-- 
2.7.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 07/15/2017 11:30 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>
> Reviewed-by: Laine Stump <laine@laine.org>
> ---
>  src/conf/domain_addr.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++---
>  src/conf/domain_addr.h |  3 +++
>  2 files changed, 72 insertions(+), 4 deletions(-)

[...]

>
> @@ -763,7 +797,7 @@ static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
>  virDomainPCIAddressGetNextAddr(virDomainPCIAddressSetPtr addrs,
>                                 virPCIDeviceAddressPtr next_addr,
>                                 virDomainPCIConnectFlags flags,
> -                               unsigned int isolationGroup ATTRIBUTE_UNUSED,
> +                               unsigned int isolationGroup,
>                                 int function)
>  {
>      virPCIDeviceAddress a = { 0 };
> @@ -779,12 +813,41 @@ 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 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;

Something for a followup patch - it occurred to me while looking at this
that due to your earlier patch changing the "find next" algorithm to
always start over at bus='0' slot='0',
virDomainPCIAddressFindUnusedFunctionOnBus() is now always called with
a.slot == bus->minSlot. We should change that function internally to
start at bus->minSlot rather than starting at a.slot - then we can
remove the "pre-load" of a.slot = bus->minSlot in this function.
Behavior will be identical, but the code will be easier for an outsider
to follow (and look less like it has been hacked up multiple times from
its original form :-P)

My Reviewed-By still stands BTW.


> +
> +        if (virDomainPCIAddressFindUnusedFunctionOnBus(bus, &a, function,
> +                                                       flags, &found) < 0) {
> +            goto error;
> +        }
> +
> +        if (found)

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list