[libvirt] [PATCH 0/3] PCI mess fixes
1/3 should allow domains without a PCI bus to start 2/3 prevents invalid memory access when the domain has no PCI bus 3/3 fixes a mising error message on OOM Ján Tomko (3): qemu: don't always reserve PCI addresses for implicit controllers qemu: prevent invalid reads in qemuAssignDevicePCISlots conf: add missing error on OOM src/conf/domain_conf.c | 1 + src/qemu/qemu_command.c | 42 +++++++++++++++++++++++++----------------- 2 files changed, 26 insertions(+), 17 deletions(-) -- 1.8.1.5
In the past we automatically added a USB controller and assigned it a PCI address (0:0:1.2) even on machines without a PCI bus. This didn't break machines with no PCI bus because the command line for it is just '-usb', with no mention of the PCI bus. The implicit IDE controller (reserved address 0:0:1.1) has no command line at all. Commit b33eb0dc removed the ability to reserve PCI addresses on machines without a PCI bus. This made them stop working, since there would always be the implicit USB controller. Skip the reservation of addresses for these controllers when there is no PCI bus, instead of failing. --- src/qemu/qemu_command.c | 37 +++++++++++++++++++++---------------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6f6b61b..915a8dd 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1325,6 +1325,25 @@ static int qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, return 0; } + /* Ignore implicit controllers on slot 0:0:1.0: + * implicit IDE controller on 0:0:1.1 (no qemu command line) + * implicit USB controller on 0:0:1.2 (-usb) + * + * If the machine does have a PCI bus, they will get reserved + * in qemuAssignDevicePCISlots(). + */ + if (device->type == VIR_DOMAIN_DEVICE_CONTROLLER && addr->domain == 0 && + addr->bus == 0 && addr->slot == 1) { + virDomainControllerDefPtr cont = device->data.controller; + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE && cont->idx == 0 && + addr->function == 1) + return 0; + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && cont->idx == 0 && + (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI || + cont->model == -1) && addr->function == 2) + return 0; + } + if (addrs->dryRun && qemuDomainPCIAddressSetGrow(addrs, addr) < 0) return -1; @@ -1740,12 +1759,9 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, qemuDomainPCIAddressSetPtr addrs) { size_t i, j; - bool reservedIDE = false; - bool reservedUSB = false; bool qemuDeviceVideoUsable = virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY); virDevicePCIAddress tmp_addr; virDevicePCIAddressPtr addrptr; - unsigned int *func = &tmp_addr.function; /* Verify that first IDE and USB controllers (if any) is on the PIIX3, fn 1 */ for (i = 0; i < def->ncontrollers ; i++) { @@ -1761,9 +1777,6 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, _("Primary IDE controller must have PCI address 0:0:1.1")); goto error; } - /* If TYPE==PCI, then qemuCollectPCIAddress() function - * has already reserved the address, so we must skip */ - reservedIDE = true; } else { def->controllers[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; def->controllers[i]->info.addr.pci.domain = 0; @@ -1784,7 +1797,6 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, _("PIIX3 USB controller must have PCI address 0:0:1.2")); goto error; } - reservedUSB = true; } else { def->controllers[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; def->controllers[i]->info.addr.pci.domain = 0; @@ -1801,15 +1813,8 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, if (addrs->nbuses) { memset(&tmp_addr, 0, sizeof(tmp_addr)); tmp_addr.slot = 1; - for (*func = 0; *func < QEMU_PCI_ADDRESS_FUNCTION_LAST; (*func)++) { - if ((*func == 1 && reservedIDE) || - (*func == 2 && reservedUSB)) - /* we have reserved this pci address */ - continue; - - if (qemuDomainPCIAddressReserveAddr(addrs, &tmp_addr) < 0) - goto error; - } + if (qemuDomainPCIAddressReserveSlot(addrs, &tmp_addr) < 0) + goto error; } if (def->nvideos > 0) { -- 1.8.1.5
In the past we automatically added a USB controller and assigned it a PCI address (0:0:1.2) even on machines without a PCI bus. This didn't break machines with no PCI bus because the command line for it is just '-usb', with no mention of the PCI bus. The implicit IDE controller (reserved address 0:0:1.1) has no command line at all. Commit b33eb0dc removed the ability to reserve PCI addresses on machines without a PCI bus. This made them stop working, since there would always be the implicit USB controller. Skip the reservation of addresses for these controllers when there is no PCI bus, instead of failing. --- Diff to v1: added a test src/qemu/qemu_command.c | 37 ++++++++++++---------- .../qemuxml2argv-s390-piix-controllers.args | 11 +++++++ .../qemuxml2argv-s390-piix-controllers.xml | 34 ++++++++++++++++++++ tests/qemuxml2argvtest.c | 5 +++ 4 files changed, 71 insertions(+), 16 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-s390-piix-controllers.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-s390-piix-controllers.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6f6b61b..915a8dd 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1325,6 +1325,25 @@ static int qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, return 0; } + /* Ignore implicit controllers on slot 0:0:1.0: + * implicit IDE controller on 0:0:1.1 (no qemu command line) + * implicit USB controller on 0:0:1.2 (-usb) + * + * If the machine does have a PCI bus, they will get reserved + * in qemuAssignDevicePCISlots(). + */ + if (device->type == VIR_DOMAIN_DEVICE_CONTROLLER && addr->domain == 0 && + addr->bus == 0 && addr->slot == 1) { + virDomainControllerDefPtr cont = device->data.controller; + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE && cont->idx == 0 && + addr->function == 1) + return 0; + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && cont->idx == 0 && + (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI || + cont->model == -1) && addr->function == 2) + return 0; + } + if (addrs->dryRun && qemuDomainPCIAddressSetGrow(addrs, addr) < 0) return -1; @@ -1740,12 +1759,9 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, qemuDomainPCIAddressSetPtr addrs) { size_t i, j; - bool reservedIDE = false; - bool reservedUSB = false; bool qemuDeviceVideoUsable = virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY); virDevicePCIAddress tmp_addr; virDevicePCIAddressPtr addrptr; - unsigned int *func = &tmp_addr.function; /* Verify that first IDE and USB controllers (if any) is on the PIIX3, fn 1 */ for (i = 0; i < def->ncontrollers ; i++) { @@ -1761,9 +1777,6 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, _("Primary IDE controller must have PCI address 0:0:1.1")); goto error; } - /* If TYPE==PCI, then qemuCollectPCIAddress() function - * has already reserved the address, so we must skip */ - reservedIDE = true; } else { def->controllers[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; def->controllers[i]->info.addr.pci.domain = 0; @@ -1784,7 +1797,6 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, _("PIIX3 USB controller must have PCI address 0:0:1.2")); goto error; } - reservedUSB = true; } else { def->controllers[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; def->controllers[i]->info.addr.pci.domain = 0; @@ -1801,15 +1813,8 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, if (addrs->nbuses) { memset(&tmp_addr, 0, sizeof(tmp_addr)); tmp_addr.slot = 1; - for (*func = 0; *func < QEMU_PCI_ADDRESS_FUNCTION_LAST; (*func)++) { - if ((*func == 1 && reservedIDE) || - (*func == 2 && reservedUSB)) - /* we have reserved this pci address */ - continue; - - if (qemuDomainPCIAddressReserveAddr(addrs, &tmp_addr) < 0) - goto error; - } + if (qemuDomainPCIAddressReserveSlot(addrs, &tmp_addr) < 0) + goto error; } if (def->nvideos > 0) { diff --git a/tests/qemuxml2argvdata/qemuxml2argv-s390-piix-controllers.args b/tests/qemuxml2argvdata/qemuxml2argv-s390-piix-controllers.args new file mode 100644 index 0000000..0d53d2b --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-s390-piix-controllers.args @@ -0,0 +1,11 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \ +/usr/bin/qemu-system-s390x -S -M s390-virtio -m 214 -smp 1 -nographic \ +-nodefconfig -nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/test-monitor,server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline -no-acpi \ +-device virtio-serial-s390,id=virtio-serial0 -usb -drive \ +file=/dev/HostVG/QEMUGuest1,if=none,id=drive-virtio-disk0 \ +-device virtio-blk-s390,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 \ +-chardev pty,id=charconsole0 \ +-device virtconsole,chardev=charconsole0,id=console0 \ +-object rng-random,id=rng0,filename=/dev/hwrng -device virtio-rng-s390,rng=rng0 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-s390-piix-controllers.xml b/tests/qemuxml2argvdata/qemuxml2argv-s390-piix-controllers.xml new file mode 100644 index 0000000..a8b72d7 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-s390-piix-controllers.xml @@ -0,0 +1,34 @@ +<domain type='qemu'> + <name>test</name> + <memory>219100</memory> + <currentMemory>219100</currentMemory> + <os> + <type arch='s390x' machine='s390-virtio'>hvm</type> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-s390x</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='virtio'/> + <boot order='1'/> + </disk> + <console type='pty'> + <target type='virtio'/> + </console> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <memballoon model='virtio'> + </memballoon> + <rng model='virtio'> + <backend model='random'>/dev/hwrng</backend> + </rng> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 7c4d1ce..fb6ed75 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -955,6 +955,11 @@ mymain(void) QEMU_CAPS_VIRTIO_S390, QEMU_CAPS_DEVICE_VIRTIO_RNG, QEMU_CAPS_OBJECT_RNG_RANDOM); + DO_TEST("s390-piix-controllers", + QEMU_CAPS_DEVICE, QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG, + QEMU_CAPS_DRIVE, QEMU_CAPS_BOOTINDEX, QEMU_CAPS_VIRTIO_S390, + QEMU_CAPS_DEVICE_VIRTIO_RNG, QEMU_CAPS_OBJECT_RNG_RANDOM); + DO_TEST("ppc-dtb", QEMU_CAPS_KVM, QEMU_CAPS_DTB); DO_TEST("tpm-passthrough", QEMU_CAPS_DEVICE, -- 1.8.1.5
On 04/26/2013 11:50 AM, Ján Tomko wrote:
In the past we automatically added a USB controller and assigned it a PCI address (0:0:1.2) even on machines without a PCI bus. This didn't break machines with no PCI bus because the command line for it is just '-usb', with no mention of the PCI bus.
The implicit IDE controller (reserved address 0:0:1.1) has no command line at all.
Commit b33eb0dc removed the ability to reserve PCI addresses on machines without a PCI bus. This made them stop working, since there would always be the implicit USB controller.
Skip the reservation of addresses for these controllers when there is no PCI bus, instead of failing. --- Diff to v1: added a test
src/qemu/qemu_command.c | 37 ++++++++++++---------- .../qemuxml2argv-s390-piix-controllers.args | 11 +++++++ .../qemuxml2argv-s390-piix-controllers.xml | 34 ++++++++++++++++++++ tests/qemuxml2argvtest.c | 5 +++ 4 files changed, 71 insertions(+), 16 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-s390-piix-controllers.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-s390-piix-controllers.xml
The test helps; thanks. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Don't reserve slot 2 for video if the machine has no PCI buses. Error out when the user specifies a video device without a PCI address when there are no PCI buses. (This wouldn't work on a machine with no PCI bus anyway since we do add PCI addresses for video devices to the command line) --- src/qemu/qemu_command.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 915a8dd..9737609 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1827,6 +1827,9 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, primaryVideo->info.addr.pci.function = 0; addrptr = &primaryVideo->info.addr.pci; + if (!qemuPCIAddressValidate(addrs, addrptr)) + goto error; + if (qemuDomainPCIAddressSlotInUse(addrs, addrptr)) { if (qemuDeviceVideoUsable) { virResetLastError(); @@ -1853,7 +1856,7 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, /* If TYPE==PCI, then qemuCollectPCIAddress() function * has already reserved the address, so we must skip */ } - } else if (!qemuDeviceVideoUsable) { + } else if (addrs->nbuses && !qemuDeviceVideoUsable) { memset(&tmp_addr, 0, sizeof(tmp_addr)); tmp_addr.slot = 2; -- 1.8.1.5
On 04/26/2013 10:31 AM, Ján Tomko wrote:
Don't reserve slot 2 for video if the machine has no PCI buses. Error out when the user specifies a video device without a PCI address when there are no PCI buses.
(This wouldn't work on a machine with no PCI bus anyway since we do add PCI addresses for video devices to the command line) --- src/qemu/qemu_command.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
I removed it in 5c3d5b2 by accident. --- src/conf/domain_conf.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3c6d260..88e3d93 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9924,6 +9924,7 @@ virDomainDefMaybeAddController(virDomainDefPtr def, if (VIR_APPEND_ELEMENT(def->controllers, def->ncontrollers, cont) < 0) { VIR_FREE(cont); + virReportOOMError(); return -1; } -- 1.8.1.5
On 04/26/2013 10:31 AM, Ján Tomko wrote:
I removed it in 5c3d5b2 by accident. --- src/conf/domain_conf.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3c6d260..88e3d93 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9924,6 +9924,7 @@ virDomainDefMaybeAddController(virDomainDefPtr def,
if (VIR_APPEND_ELEMENT(def->controllers, def->ncontrollers, cont) < 0) { VIR_FREE(cont); + virReportOOMError(); return -1;
We may be removing it again once the VIR_STRDUP stuff lands to push OOM reporting into viralloc.c; but for now this patch is right. ACK; series belongs in 1.0.5. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 04/26/2013 06:31 PM, Ján Tomko wrote:
1/3 should allow domains without a PCI bus to start 2/3 prevents invalid memory access when the domain has no PCI bus 3/3 fixes a mising error message on OOM
Ján Tomko (3): qemu: don't always reserve PCI addresses for implicit controllers qemu: prevent invalid reads in qemuAssignDevicePCISlots conf: add missing error on OOM
src/conf/domain_conf.c | 1 + src/qemu/qemu_command.c | 42 +++++++++++++++++++++++++----------------- 2 files changed, 26 insertions(+), 17 deletions(-)
I've pushed the series. Jan
participants (2)
-
Eric Blake -
Ján Tomko