[libvirt] [PATCH v2 11/11] qemu: auto-add pci-root controller for pc machine types
Laine Stump
laine at laine.org
Thu Apr 18 05:22:09 UTC 2013
On 04/17/2013 03:00 PM, Ján Tomko wrote:
> <controller type='pci' index='0' model='pci-root'/>
> is auto-added to pc* machine types.
> Without this controller PCI bus 0 is not available and
> no PCI addresses are assigned by default.
>
> Since older libvirt supported PCI bus 0 even without
> this controller, it is removed from the XML when migrating.
> ---
> src/conf/domain_conf.c | 8 ++++-
> src/qemu/qemu_command.c | 18 ++++++++---
> src/qemu/qemu_domain.c | 37 ++++++++++++++++++++++
> tests/domainsnapshotxml2xmlout/disk_snapshot.xml | 1 +
> tests/domainsnapshotxml2xmlout/external_vm.xml | 1 +
> tests/domainsnapshotxml2xmlout/full_domain.xml | 1 +
> tests/domainsnapshotxml2xmlout/metadata.xml | 1 +
> .../qemuxml2argvdata/qemuxml2argv-blkdeviotune.xml | 1 +
> .../qemuxml2argv-blkiotune-device.xml | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-blkiotune.xml | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-boot-cdrom.xml | 1 +
> .../qemuxml2argvdata/qemuxml2argv-boot-floppy.xml | 1 +
> .../qemuxml2argv-boot-menu-disable.xml | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-boot-multi.xml | 1 +
> .../qemuxml2argvdata/qemuxml2argv-boot-network.xml | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-boot-order.xml | 1 +
> .../qemuxml2argv-channel-guestfwd.xml | 1 +
> .../qemuxml2argv-channel-virtio.xml | 1 +
> .../qemuxml2argv-clock-localtime.xml | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-clock-utc.xml | 1 +
> .../qemuxml2argv-console-compat.xml | 1 +
> .../qemuxml2argv-console-virtio-many.xml | 1 +
> .../qemuxml2argv-controller-order.xml | 1 +
> .../qemuxml2argv-cpu-eoi-disabled.xml | 1 +
> .../qemuxml2argv-cpu-eoi-enabled.xml | 1 +
> .../qemuxml2argv-cpu-host-kvmclock.xml | 1 +
> .../qemuxml2argvdata/qemuxml2argv-cpu-kvmclock.xml | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-cputune.xml | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-disk-aio.xml | 1 +
> .../qemuxml2argv-disk-cdrom-empty.xml | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-disk-cdrom.xml | 1 +
> .../qemuxml2argv-disk-drive-boot-cdrom.xml | 1 +
> .../qemuxml2argv-disk-drive-boot-disk.xml | 1 +
> .../qemuxml2argv-disk-drive-cache-directsync.xml | 1 +
> .../qemuxml2argv-disk-drive-cache-unsafe.xml | 1 +
> .../qemuxml2argv-disk-drive-cache-v1-none.xml | 1 +
> .../qemuxml2argv-disk-drive-cache-v1-wb.xml | 1 +
> .../qemuxml2argv-disk-drive-cache-v1-wt.xml | 1 +
> .../qemuxml2argv-disk-drive-cache-v2-none.xml | 1 +
> .../qemuxml2argv-disk-drive-cache-v2-wb.xml | 1 +
> .../qemuxml2argv-disk-drive-cache-v2-wt.xml | 1 +
> ...muxml2argv-disk-drive-error-policy-enospace.xml | 1 +
> .../qemuxml2argv-disk-drive-error-policy-stop.xml | 1 +
> ...rgv-disk-drive-error-policy-wreport-rignore.xml | 1 +
> .../qemuxml2argv-disk-drive-fat.xml | 1 +
> .../qemuxml2argv-disk-drive-fmt-qcow.xml | 1 +
> .../qemuxml2argv-disk-drive-network-gluster.xml | 1 +
> .../qemuxml2argv-disk-drive-network-iscsi-auth.xml | 1 +
> .../qemuxml2argv-disk-drive-network-iscsi.xml | 1 +
> .../qemuxml2argv-disk-drive-network-nbd-export.xml | 1 +
> ...xml2argv-disk-drive-network-nbd-ipv6-export.xml | 1 +
> .../qemuxml2argv-disk-drive-network-nbd-ipv6.xml | 1 +
> .../qemuxml2argv-disk-drive-network-nbd-unix.xml | 1 +
> .../qemuxml2argv-disk-drive-network-nbd.xml | 1 +
> ...emuxml2argv-disk-drive-network-rbd-ceph-env.xml | 1 +
> .../qemuxml2argv-disk-drive-network-rbd-ipv6.xml | 1 +
> .../qemuxml2argv-disk-drive-network-rbd.xml | 1 +
> .../qemuxml2argv-disk-drive-network-sheepdog.xml | 1 +
> .../qemuxml2argvdata/qemuxml2argv-disk-floppy.xml | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-disk-many.xml | 1 +
> .../qemuxml2argvdata/qemuxml2argv-disk-mirror.xml | 1 +
> .../qemuxml2argv-disk-scsi-device.xml | 1 +
> .../qemuxml2argv-disk-scsi-disk-vpd.xml | 1 +
> ...qemuxml2argv-disk-scsi-lun-passthrough-sgio.xml | 1 +
> .../qemuxml2argv-disk-scsi-megasas.xml | 1 +
> .../qemuxml2argv-disk-scsi-virtio-scsi.xml | 1 +
> .../qemuxml2argv-disk-scsi-vscsi.xml | 1 +
> .../qemuxml2argv-disk-source-pool.xml | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-disk-usb.xml | 1 +
> .../qemuxml2argv-disk-virtio-scsi-num_queues.xml | 1 +
> .../qemuxml2argvdata/qemuxml2argv-disk-virtio.xml | 1 +
> .../qemuxml2argvdata/qemuxml2argv-disk-xenvbd.xml | 1 +
> .../qemuxml2argv-encrypted-disk.xml | 1 +
> .../qemuxml2argvdata/qemuxml2argv-eoi-disabled.xml | 1 +
> .../qemuxml2argvdata/qemuxml2argv-eoi-enabled.xml | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-event_idx.xml | 1 +
> .../qemuxml2argv-floppy-drive-fat.xml | 1 +
> .../qemuxml2argv-graphics-listen-network.xml | 1 +
> .../qemuxml2argv-graphics-sdl-fullscreen.xml | 1 +
> .../qemuxml2argvdata/qemuxml2argv-graphics-sdl.xml | 1 +
> .../qemuxml2argv-graphics-spice-compression.xml | 1 +
> .../qemuxml2argv-graphics-spice-qxl-vga.xml | 1 +
> .../qemuxml2argv-graphics-spice.xml | 1 +
> .../qemuxml2argv-graphics-vnc-sasl.xml | 1 +
> .../qemuxml2argv-graphics-vnc-socket.xml | 1 +
> .../qemuxml2argv-graphics-vnc-tls.xml | 1 +
> .../qemuxml2argvdata/qemuxml2argv-graphics-vnc.xml | 1 +
> .../qemuxml2argv-hostdev-pci-address.xml | 1 +
> .../qemuxml2argv-hostdev-usb-address.xml | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-hugepages.xml | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-hyperv.xml | 1 +
> .../qemuxml2argv-input-usbmouse.xml | 1 +
> .../qemuxml2argv-input-usbtablet.xml | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-kvmclock.xml | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-lease.xml | 1 +
> .../qemuxml2argv-machine-core-off.xml | 1 +
> .../qemuxml2argv-machine-core-on.xml | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-migrate.xml | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-minimal.xml | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-misc-acpi.xml | 1 +
> .../qemuxml2argv-misc-disable-s3.xml | 1 +
> .../qemuxml2argv-misc-disable-suspends.xml | 1 +
> .../qemuxml2argv-misc-enable-s4.xml | 1 +
> .../qemuxml2argv-misc-no-reboot.xml | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-misc-uuid.xml | 1 +
> .../qemuxml2argv-net-bandwidth.xml | 1 +
> .../qemuxml2argv-net-eth-ifname.xml | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-net-eth.xml | 1 +
> .../qemuxml2argvdata/qemuxml2argv-net-hostdev.xml | 1 +
> .../qemuxml2argv-net-openvswitch.xml | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-net-user.xml | 1 +
> .../qemuxml2argv-net-virtio-device.xml | 1 +
> .../qemuxml2argv-net-virtio-network-portgroup.xml | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-net-virtio.xml | 1 +
> .../qemuxml2argv-nographics-vga.xml | 1 +
> .../qemuxml2argv-numad-static-vcpu-no-numatune.xml | 1 +
> .../qemuxml2argvdata/qemuxml2argv-parallel-tcp.xml | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-pci-rom.xml | 1 +
> .../qemuxml2argv-qemu-ns-no-env.xml | 1 +
> .../qemuxml2argv-reboot-timeout-disabled.xml | 1 +
> .../qemuxml2argv-reboot-timeout-enabled.xml | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-restore-v1.xml | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-restore-v2.xml | 1 +
> .../qemuxml2argv-seclabel-dynamic-baselabel.xml | 1 +
> .../qemuxml2argv-seclabel-dynamic-override.xml | 1 +
> .../qemuxml2argv-seclabel-none.xml | 1 +
> .../qemuxml2argv-seclabel-static.xml | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-serial-dev.xml | 1 +
> .../qemuxml2argvdata/qemuxml2argv-serial-file.xml | 1 +
> .../qemuxml2argvdata/qemuxml2argv-serial-many.xml | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-serial-pty.xml | 1 +
> .../qemuxml2argv-serial-tcp-telnet.xml | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-serial-tcp.xml | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-serial-udp.xml | 1 +
> .../qemuxml2argvdata/qemuxml2argv-serial-unix.xml | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-serial-vc.xml | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-smp.xml | 1 +
> .../qemuxml2argvdata/qemuxml2argv-sound-device.xml | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-sound.xml | 1 +
> .../qemuxml2argv-tpm-passthrough.xml | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-usb-redir.xml | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml | 1 +
> .../qemuxml2argv-virtio-rng-egd.xml | 1 +
> .../qemuxml2argv-virtio-rng-random.xml | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-watchdog.xml | 1 +
> .../qemuxml2xmlout-balloon-device-auto.xml | 1 +
> .../qemuxml2xmlout-channel-virtio-auto.xml | 1 +
> .../qemuxml2xmlout-console-compat-auto.xml | 1 +
> .../qemuxml2xmlout-console-virtio.xml | 1 +
> .../qemuxml2xmlout-disk-mirror.xml | 1 +
> .../qemuxml2xmlout-disk-scsi-device-auto.xml | 1 +
> .../qemuxml2xmlout-graphics-listen-network2.xml | 1 +
> .../qemuxml2xmlout-graphics-spice-timeout.xml | 1 +
> .../qemuxml2xmloutdata/qemuxml2xmlout-memtune.xml | 1 +
> .../qemuxml2xmloutdata/qemuxml2xmlout-metadata.xml | 1 +
> ...emuxml2xmlout-numad-auto-memory-vcpu-cpuset.xml | 1 +
> ...ad-auto-memory-vcpu-no-cpuset-and-placement.xml | 1 +
> .../qemuxml2xmlout-numad-auto-vcpu-no-numatune.xml | 1 +
> .../qemuxml2xmlout-serial-target-port-auto.xml | 1 +
> .../qemuxml2xmlout-usb-ich9-ehci-addr.xml | 1 +
> 160 files changed, 215 insertions(+), 5 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 68518a7..a2179aa 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -10849,9 +10849,15 @@ virDomainDefParseXML(xmlDocPtr xml,
>
> if (def->virtType == VIR_DOMAIN_VIRT_QEMU ||
> def->virtType == VIR_DOMAIN_VIRT_KQEMU ||
> - def->virtType == VIR_DOMAIN_VIRT_KVM)
> + def->virtType == VIR_DOMAIN_VIRT_KVM) {
> if (virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_USB, 0, -1) < 0)
> goto error;
> + if (STRPREFIX(def->os.machine,"pc")) {
You also need to do this for all rhel-* machine types. Even though that
machine type only shows up in RHEL-specific versions of qemu, it is
possible for someone with a stock RHEL qemu to install upstream libvirt,
and we don't want to break that.
(side note - we need to make another patch that moves *all* of these
implicit controller additions to a hypervisor-specific post-parse
callback and only for certain machinetypes; of course this could break
non-qemu hypervisors if you move the MaybeAddController() calls out of
here and into a qemu-only callback, so I guess we should add a callback
for all of them, then let the maintainers remove that which is
inappropriate).
> + if (virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0,
> + VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) < 0)
> + goto error;
> + }
> + }
>
> /* analysis of the resource leases */
> if ((n = virXPathNodeSet("./devices/lease", ctxt, &nodes)) < 0) {
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index df0077a..21da6b6 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1209,6 +1209,10 @@ struct _qemuDomainPCIAddressSet {
> static bool qemuPCIAddressValidate(qemuDomainPCIAddressSetPtr addrs ATTRIBUTE_UNUSED,
> virDevicePCIAddressPtr addr)
> {
> + if (addrs->nbuses == 0) {
> + virReportError(VIR_ERR_XML_ERROR, "%s", _("No PCI buses available"));
> + return false;
> + }
> if (addr->domain != 0) {
> virReportError(VIR_ERR_XML_ERROR, "%s",
> _("Only PCI domain 0 is available"));
> @@ -1363,15 +1367,15 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
> qemuDomainObjPrivatePtr priv = NULL;
>
> if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
> - int nbuses = 1;
> + int nbuses = 0;
> int i;
> int rv;
>
> for (i = 0; i < def->ncontrollers; i++) {
> - if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
> - def->controllers[i]->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE)
> + if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
> nbuses++;
> }
> +
> if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) {
> virDomainDeviceInfo info;
> /* 1st pass to figure out how many PCI bridges we need */
> @@ -1398,7 +1402,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
> if (!(addrs = qemuDomainPCIAddressSetCreate(def, addrs->nbuses, false)))
> goto cleanup;
>
> - if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0)
> + if (nbuses && qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0)
> goto cleanup;
Is it okay to simply not call qemuAssignDevicePCISlots() if there are no
PCI buses? Does this properly return success if there are no PCI devices
and no PCI buses? Does it properly return a failure when there is at
least one PCI device in the config but no PCI buses?
> }
>
> @@ -10146,6 +10150,12 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr qemuCaps,
> if (virDomainDefAddImplicitControllers(def) < 0)
> goto error;
>
> + if (STRPREFIX(def->os.machine,"pc")) {
> + if (virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0,
> + VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) < 0)
> + goto error;
> + }
> +
Didn't you already do this in virDomainDefParseXML?
> if (cmd->num_args || cmd->num_env) {
> def->ns = *virDomainXMLOptionGetNamespace(xmlopt);
> def->namespaceData = cmd;
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index a7aabdf..9d6db05 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1255,6 +1255,7 @@ qemuDomainDefFormatBuf(virQEMUDriverPtr driver,
>
> if ((flags & VIR_DOMAIN_XML_MIGRATABLE)) {
> int i;
> + int idx = -1;
> virDomainControllerDefPtr usb = NULL;
>
> /* If only the default USB controller is present, we can remove it
> @@ -1288,6 +1289,42 @@ qemuDomainDefFormatBuf(virQEMUDriverPtr driver,
> def->controllers[def->ncontrollers++] = controllers[i];
> }
> }
> +
> + /* Remove the default PCI controller if there is only one present
> + * and its model is pci-root */
> + for (i = 0; i < def->ncontrollers; i++) {
> + if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) {
> + if (idx >= 0) {
> + idx = -1;
> + break;
> + }
> + idx = i;
> + }
> + }
> + if (idx >= 0 && def->controllers[idx]->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) {
> + VIR_DEBUG("Removing default 'pci-root' from domain '%s'"
> + " for migration compatibility", def->name);
> + if (usb) {
> + if (VIR_DELETE_ELEMENT(def->controllers, idx, def->ncontrollers) < 0) {
> + virReportOOMError();
> + goto cleanup;
> + }
> + } else {
> + controllers = def->controllers;
> + ncontrollers = def->ncontrollers;
> + if (VIR_ALLOC_N(def->controllers, ncontrollers - 1) < 0) {
> + controllers = NULL;
> + virReportOOMError();
> + goto cleanup;
> + }
> +
> + def->ncontrollers = 0;
> + for (i = 0; i < ncontrollers; i++) {
> + if (i != idx)
> + def->controllers[def->ncontrollers++] = controllers[i];
> + }
> + }
> + }
> }
This code seems a bit.... "icky". How about if you change the whole
thing to do something like this (or some other method of eliminating the
duplicated VIR_ALLOC_N() code and ensuing loop):
virDomainControllerDefPtr usb = NULL;
virDomainControllerDefPtr pci = NULL;
int removeCt = 0;
for (ii = 0; ii < def->ncontrollers; ii++) {
if (def->controllers[ii]->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) {
if (usb) {
usb = NULL;
break;
}
usb = def->controllers[ii];
}
}
if (usb && usb->idx == 0 && usb->model == -1) {
VIR_DEBUG("Removing default USB controller from domain '%s'"
" for migration compatibility", def->name);
removeCt++;
} else {
usb = NULL;
}
for (ii = 0; ii < def->ncontrollers; ii++) {
if (def->controllers[ii]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) {
if (pci) {
pci = NULL;
break;
}
pci = def->controllers[ii];
}
}
if (pci && && pci->idx == 0 && pci->model ==
VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) {
VIR_DEBUG("Removing default 'pci-root' from domain '%s'"
" for migration compatibility", def->name);
removeCt++;
} else {
pci = NULL;
}
if (removeCt) {
controllers = def->controllers;
ncontrollers = def->ncontrollers;
if (VIR_ALLOC_N(def->controllers, ncontrollers - removeCt) < 0) {
controllers = NULL;
virReportOOMError();
goto cleanup;
}
def->ncontrollers = 0;
for (ii = 0; ii < ncontrollers; ii++) {
if (controllers[i] != usb && controllers[ii] != pci)
def->controllers[def->ncontrollers++] = controllers[ii];
}
}
>
> ret = virDomainDefFormatInternal(def, flags, buf);
> diff --git a/tests/domainsnapshotxml2xmlout/disk_snapshot.xml b/tests/domainsnapshotxml2xmlout/disk_snapshot.xml
> index 57aef16..5f42bf5 100644
> --- a/tests/domainsnapshotxml2xmlout/disk_snapshot.xml
> +++ b/tests/domainsnapshotxml2xmlout/disk_snapshot.xml
> @@ -72,6 +72,7 @@
> </disk>
> <controller type='usb' index='0'/>
> <controller type='ide' index='0'/>
> + <controller type='pci' index='0' model='pci-root'/>
yada yada yada :-)
This needs a bit of touchup, but it's close!
(BTW, how difficult would it be to put this patch *before* 10/11? I
think logically that's where it should go, but the result will be the
same anyway, so it's not a big deal)
More information about the libvir-list
mailing list