Somewhere around commit 9ff9d9f reserving entire PCI slots was eliminated, as demonstrated by commit 6cc2014. Reserve the functions required by the implicit devices: 00:01.0 ISA Bridge 00:01.1 IDE Controller 00:01.2 USB Controller (unless USB is disabled) 00:01.3 Bridge https://bugzilla.redhat.com/show_bug.cgi?id=1460143 --- src/qemu/qemu_domain_address.c | 15 ++++++++++--- .../qemuxml2argv-440fx-ide-address-conflict.xml | 25 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 38 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-440fx-ide-address-conflict.xml diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 69c0c8bf2..f35e01e5a 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1429,15 +1429,24 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr def, cont->info.addr.pci.slot = 1; cont->info.addr.pci.function = 2; } + } else { + /* this controller is not skipped in qemuDomainCollectPCIAddress */ + continue; } + if (addrs->nbuses && + virDomainPCIAddressReserveAddr(addrs, &cont->info.addr.pci, flags, 0) < 0) + goto cleanup; } - /* PIIX3 (ISA bridge, IDE controller, something else unknown, USB controller) - * hardcoded slot=1, multifunction device - */ + /* Implicit PIIX3 devices living on slot 1 not handled above */ if (addrs->nbuses) { memset(&tmp_addr, 0, sizeof(tmp_addr)); tmp_addr.slot = 1; + /* ISA Bridge at 00:01.0 */ + if (virDomainPCIAddressReserveAddr(addrs, &tmp_addr, flags, 0) < 0) + goto cleanup; + /* Bridge at 00:01.3 */ + tmp_addr.function = 3; if (virDomainPCIAddressReserveAddr(addrs, &tmp_addr, flags, 0) < 0) goto cleanup; } diff --git a/tests/qemuxml2argvdata/qemuxml2argv-440fx-ide-address-conflict.xml b/tests/qemuxml2argvdata/qemuxml2argv-440fx-ide-address-conflict.xml new file mode 100644 index 000000000..4195880aa --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-440fx-ide-address-conflict.xml @@ -0,0 +1,25 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </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-i686</emulator> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' slot='1' function='1'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 70be0c32d..5a16760cc 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2249,6 +2249,7 @@ mymain(void) QEMU_CAPS_DEVICE_VIDEO_PRIMARY, QEMU_CAPS_DEVICE_QXL); DO_TEST_PARSE_ERROR("440fx-wrong-root", NONE); + DO_TEST_PARSE_ERROR("440fx-ide-address-conflict", NONE); DO_TEST_PARSE_ERROR("pcie-root-port-too-many", QEMU_CAPS_DEVICE_IOH3420, -- 2.13.0

On Tue, 2017-09-26 at 13:05 +0200, Ján Tomko wrote: I think it would make more sense to reserve the address for these devices in qemuDomainCollectPCIAddress() directly, along with the other ones, eg. squash in the following diff: ---------- 8< ---------- 8< ---------- 8< ---------- 8< ---------- diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index f35e01e5a..ecb91c565 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1265,13 +1265,12 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, _("Bus 0 must be PCI for integrated PIIX3 " "USB or IDE controllers")); return -1; - } else { - return 0; } } } - if (virDomainPCIAddressReserveAddr(addrs, addr, + if (addrs->nbuses && + virDomainPCIAddressReserveAddr(addrs, addr, info->pciConnectFlags, info->isolationGroup) < 0) { goto cleanup; @@ -1429,13 +1428,7 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr def, cont->info.addr.pci.slot = 1; cont->info.addr.pci.function = 2; } - } else { - /* this controller is not skipped in qemuDomainCollectPCIAddress */ - continue; } - if (addrs->nbuses && - virDomainPCIAddressReserveAddr(addrs, &cont->info.addr.pci, flags, 0) < 0) - goto cleanup; } /* Implicit PIIX3 devices living on slot 1 not handled above */ ---------- >8 ---------- >8 ---------- >8 ---------- >8 ---------- What do you think about it? -- Andrea Bolognani / Red Hat / Virtualization

On Wed, 2017-10-11 at 14:45 +0200, Ján Tomko wrote: qemuDomainCollectPCIAddress() is called by qemuDomainPCIAddressSetCreate(), which is called both before and after calling qemuDomainValidateDevicePCISlotsChipsets() - which in turn calls qemuDomainValidateDevicePCISlotsPIIX3(). So if the controllers already had been assigned a PCI address, then it was stored in the domain XML and it will be picked up during the first loop, reserved in qemuDomainCollectPCIAddress() and validated by qemuDomainValidateDevicePCISlotsPIIX3(); if it has never been assigned a PCI address, then it will be assigned the correct one by qemuDomainValidateDevicePCISlotsPIIX3() and that address will be reserved the second time qemuDomainCollectPCIAddress() is called. So it looks to me like it would work. Or did I miss something? -- Andrea Bolognani / Red Hat / Virtualization

On 09/26/2017 07:05 AM, Ján Tomko wrote: Consider: PIIX3 USB controller using index 0 must use PCI address 0:0:1.2" or PIIX3 USB controller at index 0 must use PCI address 0:0:1.2" ? Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John