[v5,03/15] qemu: set/use info->pciConnectFlags when validating/assigning PCI addresses

Message ID 1477414421.3434.14.camel@redhat.com
State New
Headers show

Commit Message

Andrea Bolognani Oct. 25, 2016, 4:53 p.m.
On Tue, 2016-10-25 at 09:49 -0400, Laine Stump wrote:
> Set pciConnectFlags in each device's DeviceInfo and then use those

> flags later when validating existing addresses in

> qemuDomainCollectPCIAddress() and when assigning new addresses with

> qemuDomainPCIAddressReserveNextAddr() (rather than scattering the

> logic about which devices need which type of slot all over the place).


> Note that the exact flags set by

> qemuDomainDeviceCalculatePCIConnectFlags() are different from the

> flags previously set manually in qemuDomainCollectPCIAddress(), but

> this doesn't matter because all validation of addresses in that case

> ignores the setting of the HOTPLUGGABLE flag, and treats PCIE_DEVICE

> and PCI_DEVICE the same (this lax checking was done on purpose,

> because there are some things that we want to allow the user to

> specify manually, e.g. assigning a PCIe device to a PCI slot, that we

> *don't* ever want libvirt to do automatically. The flag settings that

> we *really* want to match are 1) the old flag settings in

> qemuDomainAssignDevicePCISlots() (which is HOTPLUGGABLE | PCI_DEVICE

> for everything except PCI controllers) and the new flag settings done


[...] and 2) the new [...]

> by qemuDomainDeviceCalculatePCIConnectFlags() (which are currently

> exactly that - HOTPLUGGABLE | PCI_DEVICE for everything except PCI

> controllers).

> ---

>  src/qemu/qemu_domain_address.c | 205 +++++++++++++++--------------------------

>  1 file changed, 74 insertions(+), 131 deletions(-)


> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c

> index 602179c..d731b19 100644

> --- a/src/qemu/qemu_domain_address.c

> +++ b/src/qemu/qemu_domain_address.c

> @@ -721,7 +721,7 @@ qemuDomainFillDevicePCIConnectFlagsIter(virDomainDefPtr def ATTRIBUTE_UNUSED,

>   * failure would be some internal problem with

>   * virDomainDeviceInfoIterate())

>   */

> -static int ATTRIBUTE_UNUSED

> +static int

>  qemuDomainFillAllPCIConnectFlags(virDomainDefPtr def,

>                                   virQEMUCapsPtr qemuCaps)

>  {


You should remove ATTRIBUTE_UNUSED from
qemuDomainFillDevicePCIConnectFlags() as well.

[...]
> @@ -926,7 +857,7 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,

>      entireSlot = (addr->function == 0 &&

>                    addr->multi != VIR_TRISTATE_SWITCH_ON);

>  

> -    if (virDomainPCIAddressReserveAddr(addrs, addr, flags,

> +    if (virDomainPCIAddressReserveAddr(addrs, addr, info->pciConnectFlags,

>                                         entireSlot, true) < 0)


Would it be cleaner to have a qemuDomainPCIAddressReserveAddr()
function that takes @info directly?

It would be used only a single time, so it could very well be
argued that it would be overkill... On the other hand, it would
be neat not to use virDomainPCIAddressReserve*() functions at
all in the qemu driver and rely solely on the wrappers instead.

Speaking of which, even with the full series applied there
are still a bunch of uses of virDomainPCIAddressReserveAddr()
and virDomainPCIAddressReserveSlot(), mostly in
qemuDomainValidateDevicePCISlots{PIIX3,Q35}().

The attached patch makes all of them go away, except a few
that actually make sense because they set aside addresses for
potential future devices and as such don't have access to
a virDomainDeviceInfo yet.

I'm not saying we should deal with this right away: I'd
rather go back later to tidy up the whole thing than hold up
the series or go through another round of reviews for what
is ultimately a cosmetic issue.

[...]
> @@ -1878,24 +1795,50 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,

>           */

>          if (!buses_reserved &&

>              !qemuDomainMachineIsVirt(def) &&

> -            qemuDomainPCIAddressReserveNextSlot(addrs, &info, flags) < 0)

> +            qemuDomainPCIAddressReserveNextSlot(addrs, &info) < 0)

>              goto cleanup;

>  

>          if (qemuDomainAssignDevicePCISlots(def, qemuCaps, addrs) < 0)

>              goto cleanup;

>  

>          for (i = 1; i < addrs->nbuses; i++) {

> +            virDomainDeviceDef dev;

> +            int contIndex;

>              virDomainPCIAddressBusPtr bus = &addrs->buses[i];

>  

>              if ((rv = virDomainDefMaybeAddController(

>                       def, VIR_DOMAIN_CONTROLLER_TYPE_PCI,

>                       i, bus->model)) < 0)

>                  goto cleanup;

> -            /* If we added a new bridge, we will need one more address */

> -            if (rv > 0 &&

> -                qemuDomainPCIAddressReserveNextSlot(addrs, &info, flags) < 0)

> +

> +            if (rv == 0)

> +                continue; /* no new controller added */


Alternatively, you could turn this into

  if (rv > 0) {
      virDomainDeviceDef dev;
      int contIndex;

      /* The code below */
  }

but I leave up to you whether to go one way or the other.

> +

> +            /* We did add a new controller, so we will need one more

> +             * address (and we need to set the new controller's

> +             * pciConnectFlags)

> +             */

> +            contIndex = virDomainControllerFind(def,

> +                                                VIR_DOMAIN_CONTROLLER_TYPE_PCI,

> +                                                i);

> +            if (contIndex < 0) {

> +                /* this should never happen - we just added it */

> +                virReportError(VIR_ERR_INTERNAL_ERROR,

> +                               _("Could not find auto-added %s controller "

> +                                 "with index %zu"),

> +                               virDomainControllerModelPCITypeToString(bus->model),

> +                               i);

> +                goto cleanup;

> +            }

> +            dev.type = VIR_DOMAIN_DEVICE_CONTROLLER;

> +            dev.data.controller = def->controllers[contIndex];

> +            /* set connect flags so it will be properly addressed */

> +            qemuDomainFillDevicePCIConnectFlags(def, &dev, qemuCaps);

> +            if (qemuDomainPCIAddressReserveNextSlot(addrs,

> +                                                    &dev.data.controller->info) < 0)

>                  goto cleanup;

>          }

> +

>          nbuses = addrs->nbuses;

>          virDomainPCIAddressSetFree(addrs);

>          addrs = NULL;


ACK

-- 
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Comments

Laine Stump Oct. 25, 2016, 6:08 p.m. | #1
On 10/25/2016 12:53 PM, Andrea Bolognani wrote:
> On Tue, 2016-10-25 at 09:49 -0400, Laine Stump wrote:

>> Set pciConnectFlags in each device's DeviceInfo and then use those

>> flags later when validating existing addresses in

>> qemuDomainCollectPCIAddress() and when assigning new addresses with

>> qemuDomainPCIAddressReserveNextAddr() (rather than scattering the

>> logic about which devices need which type of slot all over the place).

>>   

>> Note that the exact flags set by

>> qemuDomainDeviceCalculatePCIConnectFlags() are different from the

>> flags previously set manually in qemuDomainCollectPCIAddress(), but

>> this doesn't matter because all validation of addresses in that case

>> ignores the setting of the HOTPLUGGABLE flag, and treats PCIE_DEVICE

>> and PCI_DEVICE the same (this lax checking was done on purpose,

>> because there are some things that we want to allow the user to

>> specify manually, e.g. assigning a PCIe device to a PCI slot, that we

>> *don't* ever want libvirt to do automatically. The flag settings that

>> we *really* want to match are 1) the old flag settings in

>> qemuDomainAssignDevicePCISlots() (which is HOTPLUGGABLE | PCI_DEVICE

>> for everything except PCI controllers) and the new flag settings done

> [...] and 2) the new [...]

>

>> by qemuDomainDeviceCalculatePCIConnectFlags() (which are currently

>> exactly that - HOTPLUGGABLE | PCI_DEVICE for everything except PCI

>> controllers).

>> ---

>>    src/qemu/qemu_domain_address.c | 205 +++++++++++++++--------------------------

>>    1 file changed, 74 insertions(+), 131 deletions(-)

>>   

>> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c

>> index 602179c..d731b19 100644

>> --- a/src/qemu/qemu_domain_address.c

>> +++ b/src/qemu/qemu_domain_address.c

>> @@ -721,7 +721,7 @@ qemuDomainFillDevicePCIConnectFlagsIter(virDomainDefPtr def ATTRIBUTE_UNUSED,

>>     * failure would be some internal problem with

>>     * virDomainDeviceInfoIterate())

>>     */

>> -static int ATTRIBUTE_UNUSED

>> +static int

>>    qemuDomainFillAllPCIConnectFlags(virDomainDefPtr def,

>>                                     virQEMUCapsPtr qemuCaps)

>>    {

> You should remove ATTRIBUTE_UNUSED from

> qemuDomainFillDevicePCIConnectFlags() as well.

>

> [...]

>> @@ -926,7 +857,7 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,

>>        entireSlot = (addr->function == 0 &&

>>                      addr->multi != VIR_TRISTATE_SWITCH_ON);

>>    

>> -    if (virDomainPCIAddressReserveAddr(addrs, addr, flags,

>> +    if (virDomainPCIAddressReserveAddr(addrs, addr, info->pciConnectFlags,

>>                                           entireSlot, true) < 0)

> Would it be cleaner to have a qemuDomainPCIAddressReserveAddr()

> function that takes @info directly?


Actually in a later series (the one that cleans up the *Slot() vs 
*Addr() naming), I eliminated all but one of the 
qemuDomainPCIAddressReserve*() functions anyway. After that series, 
there are only two *PCIAddressReserve*() functions used in this file: 
qemuDomainPCIAddressReserveNextAddr() (21 times), and 
virDomainPCIAddressReserveAddr() (12 times). The latter can't have a 
nice flags-removing wrapper added in qemu_domain_address.c (like the 
former does) because it often is called with a bare address - no DeviceInfo

(Well, I don't know, maybe it could be done by reorganizing some of the 
calls, I'll have to look at it).




>

> It would be used only a single time, so it could very well be

> argued that it would be overkill... On the other hand, it would

> be neat not to use virDomainPCIAddressReserve*() functions at

> all in the qemu driver and rely solely on the wrappers instead.

>

> Speaking of which, even with the full series applied there

> are still a bunch of uses of virDomainPCIAddressReserveAddr()

> and virDomainPCIAddressReserveSlot(), mostly in

> qemuDomainValidateDevicePCISlots{PIIX3,Q35}().


Yeah, my later series turns all of those into 
virDomainPCIAddressReserveAddr().

>

> The attached patch makes all of them go away, except a few

> that actually make sense because they set aside addresses for

> potential future devices and as such don't have access to

> a virDomainDeviceInfo yet.

>

> I'm not saying we should deal with this right away: I'd

> rather go back later to tidy up the whole thing than hold up

> the series or go through another round of reviews for what

> is ultimately a cosmetic issue.

>

> [...]

>> @@ -1878,24 +1795,50 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,

>>             */

>>            if (!buses_reserved &&

>>                !qemuDomainMachineIsVirt(def) &&

>> -            qemuDomainPCIAddressReserveNextSlot(addrs, &info, flags) < 0)

>> +            qemuDomainPCIAddressReserveNextSlot(addrs, &info) < 0)

>>                goto cleanup;

>>    

>>            if (qemuDomainAssignDevicePCISlots(def, qemuCaps, addrs) < 0)

>>                goto cleanup;

>>    

>>            for (i = 1; i < addrs->nbuses; i++) {

>> +            virDomainDeviceDef dev;

>> +            int contIndex;

>>                virDomainPCIAddressBusPtr bus = &addrs->buses[i];

>>    

>>                if ((rv = virDomainDefMaybeAddController(

>>                         def, VIR_DOMAIN_CONTROLLER_TYPE_PCI,

>>                         i, bus->model)) < 0)

>>                    goto cleanup;

>> -            /* If we added a new bridge, we will need one more address */

>> -            if (rv > 0 &&

>> -                qemuDomainPCIAddressReserveNextSlot(addrs, &info, flags) < 0)

>> +

>> +            if (rv == 0)

>> +                continue; /* no new controller added */

> Alternatively, you could turn this into

>

>    if (rv > 0) {

>        virDomainDeviceDef dev;

>        int contIndex;

>

>        /* The code below */

>    }

>

> but I leave up to you whether to go one way or the other.


I like the reduced indent level of doing it this way :-)

>

>> +

>> +            /* We did add a new controller, so we will need one more

>> +             * address (and we need to set the new controller's

>> +             * pciConnectFlags)

>> +             */

>> +            contIndex = virDomainControllerFind(def,

>> +                                                VIR_DOMAIN_CONTROLLER_TYPE_PCI,

>> +                                                i);

>> +            if (contIndex < 0) {

>> +                /* this should never happen - we just added it */

>> +                virReportError(VIR_ERR_INTERNAL_ERROR,

>> +                               _("Could not find auto-added %s controller "

>> +                                 "with index %zu"),

>> +                               virDomainControllerModelPCITypeToString(bus->model),

>> +                               i);

>> +                goto cleanup;

>> +            }

>> +            dev.type = VIR_DOMAIN_DEVICE_CONTROLLER;

>> +            dev.data.controller = def->controllers[contIndex];

>> +            /* set connect flags so it will be properly addressed */

>> +            qemuDomainFillDevicePCIConnectFlags(def, &dev, qemuCaps);

>> +            if (qemuDomainPCIAddressReserveNextSlot(addrs,

>> +                                                    &dev.data.controller->info) < 0)

>>                    goto cleanup;

>>            }

>> +

>>            nbuses = addrs->nbuses;

>>            virDomainPCIAddressSetFree(addrs);

>>            addrs = NULL;

> ACK

>

> -- 

> Andrea Bolognani / Red Hat / Virtualization



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Andrea Bolognani Oct. 26, 2016, 7:55 a.m. | #2
On Tue, 2016-10-25 at 14:08 -0400, Laine Stump wrote:
> > [...]
> > > @@ -926,7 +857,7 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,
> > >        entireSlot = (addr->function == 0 &&
> > >                      addr->multi != VIR_TRISTATE_SWITCH_ON);
> > >    
> > > -    if (virDomainPCIAddressReserveAddr(addrs, addr, flags,
> > > +    if (virDomainPCIAddressReserveAddr(addrs, addr, info->pciConnectFlags,
> > >                                           entireSlot, true) < 0)
> > Would it be cleaner to have a qemuDomainPCIAddressReserveAddr()
> > function that takes @info directly?

> Actually in a later series (the one that cleans up the *Slot() vs 
> *Addr() naming), I eliminated all but one of the 
> qemuDomainPCIAddressReserve*() functions anyway. After that series, 
> there are only two *PCIAddressReserve*() functions used in this file: 
> qemuDomainPCIAddressReserveNextAddr() (21 times), and 
> virDomainPCIAddressReserveAddr() (12 times). The latter can't have a 
> nice flags-removing wrapper added in qemu_domain_address.c (like the 
> former does) because it often is called with a bare address - no DeviceInfo

> (Well, I don't know, maybe it could be done by reorganizing some of the 
> calls, I'll have to look at it).

> > It would be used only a single time, so it could very well be
> > argued that it would be overkill... On the other hand, it would
> > be neat not to use virDomainPCIAddressReserve*() functions at
> > all in the qemu driver and rely solely on the wrappers instead.
> > 
> > Speaking of which, even with the full series applied there
> > are still a bunch of uses of virDomainPCIAddressReserveAddr()
> > and virDomainPCIAddressReserveSlot(), mostly in
> > qemuDomainValidateDevicePCISlots{PIIX3,Q35}().

> Yeah, my later series turns all of those into 
> virDomainPCIAddressReserveAddr().

Sorry, I haven't looked at any of your follow-up series at
all yet. Disregard my comments then :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Patch

From 2ce5a69b762fe6767f3289c200cf2ce3acc69a52 Mon Sep 17 00:00:00 2001
From: Andrea Bolognani <abologna@redhat.com>
Date: Tue, 25 Oct 2016 18:37:47 +0200
Subject: [PATCH] qemu: Introduce qemuDomainPCIAddressReserve{Addr,Slot}()

---
 src/qemu/qemu_domain_address.c | 54 ++++++++++++++++++++++++++++++++----------
 1 file changed, 42 insertions(+), 12 deletions(-)

diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 42af435..f17eaa1 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -789,6 +789,30 @@  qemuDomainFillDevicePCIConnectFlags(virDomainDefPtr def,
 
 
 static int
+qemuDomainPCIAddressReserveAddr(virDomainPCIAddressSetPtr addrs,
+                                virPCIDeviceAddressPtr addr,
+                                virDomainDeviceInfoPtr info,
+                                bool reserveEntireSlot,
+                                bool fromConfig)
+{
+    return virDomainPCIAddressReserveAddr(addrs, addr,
+                                          info->pciConnectFlags,
+                                          reserveEntireSlot, fromConfig);
+}
+
+
+static int
+qemuDomainPCIAddressReserveSlot(virDomainPCIAddressSetPtr addrs,
+                                virPCIDeviceAddressPtr addr,
+                                virDomainDeviceInfoPtr info)
+{
+    return qemuDomainPCIAddressReserveAddr(addrs, addr,
+                                           info,
+                                           true, false);
+}
+
+
+static int
 qemuDomainPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs,
                                     virDomainDeviceInfoPtr dev,
                                     unsigned int function,
@@ -892,8 +916,8 @@  qemuDomainCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,
     entireSlot = (addr->function == 0 &&
                   addr->multi != VIR_TRISTATE_SWITCH_ON);
 
-    if (virDomainPCIAddressReserveAddr(addrs, addr, info->pciConnectFlags,
-                                       entireSlot, true) < 0)
+    if (qemuDomainPCIAddressReserveAddr(addrs, addr, info,
+                                        entireSlot, true) < 0)
         goto cleanup;
 
     ret = 0;
@@ -1113,7 +1137,8 @@  qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr def,
                     goto cleanup;
                 }
             } else {
-                if (virDomainPCIAddressReserveSlot(addrs, &tmp_addr, flags) < 0)
+                if (qemuDomainPCIAddressReserveSlot(addrs, &tmp_addr,
+                                                    &primaryVideo->info) < 0)
                     goto cleanup;
                 primaryVideo->info.addr.pci = tmp_addr;
                 primaryVideo->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
@@ -1214,8 +1239,9 @@  qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def,
                         assign = true;
                 }
                 if (assign) {
-                    if (virDomainPCIAddressReserveAddr(addrs, &tmp_addr,
-                                                       flags, false, true) < 0)
+                    if (qemuDomainPCIAddressReserveAddr(addrs, &tmp_addr,
+                                                        &cont->info,
+                                                        false, true) < 0)
                         goto cleanup;
                     cont->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
                     cont->info.addr.pci.domain = 0;
@@ -1237,8 +1263,8 @@  qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def,
                 memset(&tmp_addr, 0, sizeof(tmp_addr));
                 tmp_addr.slot = 0x1E;
                 if (!virDomainPCIAddressSlotInUse(addrs, &tmp_addr)) {
-                    if (virDomainPCIAddressReserveAddr(addrs, &tmp_addr,
-                                                       flags, true, false) < 0)
+                    if (qemuDomainPCIAddressReserveSlot(addrs, &tmp_addr,
+                                                        &cont->info) < 0)
                         goto cleanup;
                     cont->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
                     cont->info.addr.pci.domain = 0;
@@ -1301,7 +1327,9 @@  qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def,
                     goto cleanup;
                 }
             } else {
-                if (virDomainPCIAddressReserveSlot(addrs, &tmp_addr, flags) < 0)
+                if (qemuDomainPCIAddressReserveAddr(addrs, &tmp_addr,
+                                                    &primaryVideo->info,
+                                                    true, true) < 0)
                     goto cleanup;
                 primaryVideo->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
                 primaryVideo->info.addr.pci = tmp_addr;
@@ -1349,7 +1377,9 @@  qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def,
                 !virDeviceInfoPCIAddressWanted(&sound->info)) {
                 continue;
             }
-            if (virDomainPCIAddressReserveSlot(addrs, &tmp_addr, flags) < 0)
+            if (qemuDomainPCIAddressReserveAddr(addrs, &tmp_addr,
+                                                &sound->info,
+                                                true, true) < 0)
                 goto cleanup;
 
             sound->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
@@ -1565,9 +1595,9 @@  qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
 
             if (foundAddr) {
                 /* Reserve this function on the slot we found */
-                if (virDomainPCIAddressReserveAddr(addrs, &addr,
-                                                   cont->info.pciConnectFlags,
-                                                   false, true) < 0)
+                if (qemuDomainPCIAddressReserveAddr(addrs, &addr,
+                                                    &cont->info,
+                                                    false, true) < 0)
                     goto error;
 
                 cont->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
-- 
2.7.4