[libvirt] [PATCH 0/2] PIIX3 implicit controller address handling fixes
Ján Tomko (2): qemu: reserve PCI addresses for implicit i440fx devices qemu: clarify error message for index 0 PIIX3 USB controller src/qemu/qemu_domain_address.c | 17 +++++++++++---- .../qemuxml2argv-440fx-ide-address-conflict.xml | 25 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 39 insertions(+), 4 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-440fx-ide-address-conflict.xml -- 2.13.0
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 09/26/2017 07:05 AM, Ján Tomko wrote:
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
Reviewed-by: John Ferlan <jferlan@redhat.com> John
On Tue, 2017-09-26 at 13:05 +0200, Ján Tomko wrote:
@@ -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;
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, Oct 11, 2017 at 01:14:58PM +0200, Andrea Bolognani wrote:
On Tue, 2017-09-26 at 13:05 +0200, Ján Tomko wrote:
@@ -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;
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?
I think I cannot possibly reserve the address before setting it. Jan
On Wed, 2017-10-11 at 14:45 +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?
I think I cannot possibly reserve the address before setting it.
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
The address is restricted to 0:0:1.2 only for the piix3-uhci controller with index 0. https://bugzilla.redhat.com/show_bug.cgi?id=1460602 --- src/qemu/qemu_domain_address.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index f35e01e5a..17ef93a3b 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1419,7 +1419,7 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr def, cont->info.addr.pci.slot != 1 || cont->info.addr.pci.function != 2) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("PIIX3 USB controller must have PCI address 0:0:1.2")); + _("PIIX3 USB controller with index 0 must have PCI address 0:0:1.2")); goto cleanup; } } else { -- 2.13.0
On 09/26/2017 07:05 AM, Ján Tomko wrote:
The address is restricted to 0:0:1.2 only for the piix3-uhci controller with index 0.
https://bugzilla.redhat.com/show_bug.cgi?id=1460602 --- src/qemu/qemu_domain_address.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index f35e01e5a..17ef93a3b 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1419,7 +1419,7 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr def, cont->info.addr.pci.slot != 1 || cont->info.addr.pci.function != 2) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("PIIX3 USB controller must have PCI address 0:0:1.2")); + _("PIIX3 USB controller with index 0 must have PCI address 0:0:1.2"));
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" ?
goto cleanup; } } else {
Reviewed-by: John Ferlan <jferlan@redhat.com> John
participants (3)
-
Andrea Bolognani -
John Ferlan -
Ján Tomko