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;
+ }
+ }
+
+ 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);
+ } 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);
+ }
+
/* 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;
+ 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;
+
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 */
+ 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;
+ }
+ }
+
+ /* 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)
+ 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;
}
--
2.7.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, 2017-06-28 at 14:12 -0400, Laine Stump wrote:
> > + /* 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)?
That makes perfect sense, and I'm not sure why I didn't
think of it myself :/
> > + 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?
That's the idea :)
For all guests except pSeries, the isolation group for both
devices and controllers will never change from the default,
so it will not influence address allocation.
If it did, either this patch or the next one would cause a
massive test suite churn, which they don't ;)
> > + } 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 :-)
If we did that, all existing pSeries guests using hostdevs
would suddenly fail to start :(
Degraded isolation (multiple indipendent hostdevs on the
same PHB) is something we want to avoid but doesn't cause
any harm in practice, you just don't get the advantages of
isolation. It's worked fine until now.
Splitting isolation groups (devices that are in the same
host IOMMU group being assigned to separate PHBs), on the
other hand, is problematic. Maybe I can add try to check
for that situation and error out when it happens.
> > + /* 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"?)
Yes, isolationGroupLocked is set when the controller is
created and never changed afterwards.
There's not really a notion of "none selected" for isolation
groups: 0 is not special in that regard, it just happens to
be the default for both devices and controllers.
... Now what I look at that code again, I'm pretty sure
resetting the isolation group for the bus when the last
device is removed is not even needed: it will be changed
back whenever another device is plugged in anyway. I'll
try getting rid of that hunk and verify everything still
works.
> > + /* 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?
That's the only use case at the moment.
We want to assing all emulated devices to the default PHB,
and locking its isolation group to the default one is a way
to achieve that.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, 2017-06-29 at 08:34 +0200, Andrea Bolognani wrote: > ... Now what I look at that code again, I'm pretty sure > resetting the isolation group for the bus when the last > device is removed is not even needed: it will be changed > back whenever another device is plugged in anyway. I'll > try getting rid of that hunk and verify everything still > works. Yup yup yup. I was right. Consider that hunk gone. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.