From patchwork Tue Oct 25 16:53:41 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Andrea Bolognani X-Patchwork-Id: 79241 Delivered-To: patch@linaro.org Received: by 10.140.97.247 with SMTP id m110csp3220759qge; Tue, 25 Oct 2016 09:57:58 -0700 (PDT) X-Received: by 10.194.176.6 with SMTP id ce6mr20934524wjc.43.1477414678241; Tue, 25 Oct 2016 09:57:58 -0700 (PDT) Return-Path: Received: from mx5-phx2.redhat.com (mx5-phx2.redhat.com. [209.132.183.37]) by mx.google.com with ESMTPS id ev6si23202360wjd.116.2016.10.25.09.57.57 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 25 Oct 2016 09:57:58 -0700 (PDT) Received-SPF: permerror (google.com: permanent error in processing during lookup of libvir-list-bounces@redhat.com: _spf1.redhat.com not found) client-ip=209.132.183.37; Authentication-Results: mx.google.com; spf=permerror (google.com: permanent error in processing during lookup of libvir-list-bounces@redhat.com: _spf1.redhat.com not found) smtp.mailfrom=libvir-list-bounces@redhat.com Received: from lists01.pubmisc.prod.ext.phx2.redhat.com (lists01.pubmisc.prod.ext.phx2.redhat.com [10.5.19.33]) by mx5-phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u9PGrku8012052; Tue, 25 Oct 2016 12:53:46 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by lists01.pubmisc.prod.ext.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id u9PGriJN031295 for ; Tue, 25 Oct 2016 12:53:44 -0400 Received: from ovpn-200-18.brq.redhat.com (ovpn-200-18.brq.redhat.com [10.40.200.18]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u9PGrfwC015712 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Tue, 25 Oct 2016 12:53:43 -0400 Message-ID: <1477414421.3434.14.camel@redhat.com> From: Andrea Bolognani To: Laine Stump , libvir-list@redhat.com Date: Tue, 25 Oct 2016 18:53:41 +0200 In-Reply-To: <1477403399-26157-4-git-send-email-laine@laine.org> References: <1477403399-26157-1-git-send-email-laine@laine.org> <1477403399-26157-4-git-send-email-laine@laine.org> Mime-Version: 1.0 X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-loop: libvir-list@redhat.com Subject: Re: [libvirt] [PATCH v5 03/15] qemu: set/use info->pciConnectFlags when validating/assigning PCI addresses X-BeenThere: libvir-list@redhat.com X-Mailman-Version: 2.1.12 Precedence: junk List-Id: Development discussions about the libvirt library & tools List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: libvir-list-bounces@redhat.com Errors-To: libvir-list-bounces@redhat.com 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 >From 2ce5a69b762fe6767f3289c200cf2ce3acc69a52 Mon Sep 17 00:00:00 2001 From: Andrea Bolognani 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