[libvirt] [PATCHv2 06/17] qemu: implement <model> subelement to <controller>

Martin Kletzander mkletzan at redhat.com
Fri Jul 24 13:01:25 UTC 2015


On Fri, Jul 17, 2015 at 02:43:33PM -0400, Laine Stump wrote:
>This patch provides qemu support for the contents of <model> in
><controller> for the two existing PCI controller types that need it
>(i.e. the two controller types that are backed by a device that must
>be specified on the qemu commandline):
>
>1) pci-bridge - sets <model> type attribute default as "pci-bridge"
>
>2) dmi-to-pci-bridge - sets <model> type attribute default as
>   "i82801b11-bridge".
>
>These both match current hardcoded practice.
>
>The defaults are set at the end of qemuDomainAssignPCIAddresses(), It
>can't be done earlier because some of the options that will be
>autogenerated need full PCI address info for the controller and
>because qemuDomainAssignPCIAddresses() might create extra controllers
>which would need default settings added. This is still prior to the
>XML being written to disk, though, so the autogenerated defaults are
>persistent.
>
>qemu capabilities bits aren't checked until the commandline is
>actually created (so the domain can possibly be defined on a host that
>doesn't yet have support for the give n device, or a host different
>from the one where it will eventually be run). At that time we compare
>the type strings to known qemu device names and check for the
>capabilities bit for that device.
>---
>new in V2 (previously was a part of the patch to add pcie-root-port)
>
> src/qemu/qemu_command.c | 70 ++++++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 64 insertions(+), 6 deletions(-)
>
>diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>index 74f02f5..8868e18 100644
>--- a/src/qemu/qemu_command.c
>+++ b/src/qemu/qemu_command.c
>@@ -2243,11 +2243,37 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,

Pity it has to be done in this function, but I understand we need to
have all controllers in and all of them have to have an address
assigned already.

>                 virDomainControllerDefPtr cont = def->controllers[i];
>                 int idx = cont->idx;
>                 virDevicePCIAddressPtr addr;
>+                virDomainPCIControllerOptsPtr options;
>+                const char *deviceName = NULL;
>
>                 if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_PCI)
>                     continue;
>
>                 addr = &cont->info.addr.pci;
>+                options = &cont->opts.pciopts;
>+
>+                /* set defaults for any other auto-generated config
>+                 * options for this controller that haven't been
>+                 * specified in config.
>+                 */
>+                switch ((virDomainControllerModelPCI)cont->model) {

I'd rather make sure that cont->model >= -1 as you can't know how the
cast will handle the '-1'.  Why didn't we go with MODEL_UNDEFINED == 0
in the first place?  And it's a fairly recent change, only 5 years back.

>+                case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
>+                    if (!options->type)
>+                        deviceName = "pci-bridge";
>+                    break;
>+                case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
>+                    if (!options->type)
>+                        deviceName = "i82801b11-bridge";
>+                    break;
>+                case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
>+                case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
>+                case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
>+                    break;
>+                }
>+                if (deviceName &&
>+                    VIR_STRDUP(options->type, deviceName) < 0)
>+                    goto cleanup;
>+
>                 /* check if every PCI bridge controller's ID is greater than
>                  * the bus it is placed onto
>                  */
>@@ -4614,17 +4640,49 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef,
>         }
>         switch (def->model) {
>         case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
>-            virBufferAsprintf(&buf, "pci-bridge,chassis_nr=%d,id=%s",
>+            if (!def->opts.pciopts.type) {
>+                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>+                               _("autogenerated pci-bridge options not set"));
>+                goto error;
>+            }
>+            if (STREQ(def->opts.pciopts.type, "pci-bridge")) {

When the type gets changed to enum, I guess these could be simplified
to another function, e.g. qemuControllerModelPCIToCaps(), similarly to
qemuControllerModelUSBToCaps() or qemuSoundCodecTypeToCaps().  It
could even do the check and error setting if unsupported.  But that's
just a readability hint.

>+                if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) {
>+                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>+                                   _("the pci-bridge controller "
>+                                     "is not supported in this QEMU binary"));
>+                goto error;
>+                }
>+            } else {
>+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>+                               _("unknown pci-bridge device '%s'"),
>+                               def->opts.pciopts.type);
>+                goto error;
>+            }
>+            virBufferAsprintf(&buf, "%s,chassis_nr=%d,id=%s",
>+                              def->opts.pciopts.type,
>                               def->idx, def->info.alias);
>             break;
>         case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
>-            if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE)) {
>-                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>-                               _("The dmi-to-pci-bridge (i82801b11-bridge) "
>-                                 "controller is not supported in this QEMU binary"));
>+            if (!def->opts.pciopts.type) {
>+                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>+                               _("autogenerated dmi-to-pci-bridge options not set"));
>+                goto error;
>+            }
>+            if (STREQ(def->opts.pciopts.type, "i82801b11-bridge")) {
>+                if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE)) {
>+                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>+                                   _("the dmi-to-pci-bridge (i82801b11-bridge) "
>+                                     "controller is not supported in this QEMU binary"));
>+                goto error;
>+                }
>+            } else {
>+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>+                               _("unknown dmi-to-pci-bridge device '%s'"),
>+                               def->opts.pciopts.type);
>                 goto error;
>             }
>-            virBufferAsprintf(&buf, "i82801b11-bridge,id=%s", def->info.alias);
>+            virBufferAsprintf(&buf, "%s,id=%s",
>+                              def->opts.pciopts.type, def->info.alias);
>             break;
>         }
>         break;
>--
>2.1.0
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150724/a613aa04/attachment-0001.sig>


More information about the libvir-list mailing list