[libvirt] [PATCH] Fix explicit usage of default video PCI slots
Don't set the PCI address of primary video to the default if it's already occupied by another device. Without this, a primary video card that doesn't have an address gets the default one on the 1st pass of PCI address allocation, even if it's occupied. This address gets picked up by the second pass, and a conflict occurs: XML error: Attempted double use of PCI slot 0000:00:02.0 (may need "multifunction='on'" for device on function 0) Also fix the test that was supposed to catch this. --- src/qemu/qemu_command.c | 50 +++++++++++++++++++++--------------------------- tests/qemuxml2argvtest.c | 3 ++- 2 files changed, 24 insertions(+), 29 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d723dc8..2a64cd1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2379,7 +2379,6 @@ qemuValidateDevicePCISlotsPIIX3(virDomainDefPtr def, size_t i; virDevicePCIAddress tmp_addr; bool qemuDeviceVideoUsable = virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY); - virDevicePCIAddressPtr addrptr; char *addrStr = NULL; qemuDomainPCIConnectFlags flags = QEMU_PCI_CONNECT_HOTPLUGGABLE | QEMU_PCI_CONNECT_TYPE_PCI; @@ -2446,22 +2445,17 @@ qemuValidateDevicePCISlotsPIIX3(virDomainDefPtr def, */ virDomainVideoDefPtr primaryVideo = def->videos[0]; if (primaryVideo->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { - primaryVideo->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; - primaryVideo->info.addr.pci.domain = 0; - primaryVideo->info.addr.pci.bus = 0; - primaryVideo->info.addr.pci.slot = 2; - primaryVideo->info.addr.pci.function = 0; - addrptr = &primaryVideo->info.addr.pci; - - if (!(addrStr = qemuDomainPCIAddressAsString(addrptr))) + memset(&tmp_addr, 0, sizeof(tmp_addr)); + tmp_addr.slot = 2; + + if (!(addrStr = qemuDomainPCIAddressAsString(&tmp_addr))) goto cleanup; - if (!qemuDomainPCIAddressValidate(addrs, addrptr, + if (!qemuDomainPCIAddressValidate(addrs, &tmp_addr, addrStr, flags, false)) goto cleanup; - if (qemuDomainPCIAddressSlotInUse(addrs, addrptr)) { + if (qemuDomainPCIAddressSlotInUse(addrs, &tmp_addr)) { if (qemuDeviceVideoUsable) { - virResetLastError(); if (qemuDomainPCIAddressReserveNextSlot(addrs, &primaryVideo->info, flags) < 0) @@ -2472,8 +2466,11 @@ qemuValidateDevicePCISlotsPIIX3(virDomainDefPtr def, "QEMU needs it for primary video")); goto cleanup; } - } else if (qemuDomainPCIAddressReserveSlot(addrs, addrptr, flags) < 0) { - goto cleanup; + } else { + if (qemuDomainPCIAddressReserveSlot(addrs, &tmp_addr, flags) < 0) + goto cleanup; + primaryVideo->info.addr.pci = tmp_addr; + primaryVideo->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; } } else if (!qemuDeviceVideoUsable) { if (primaryVideo->info.addr.pci.domain != 0 || @@ -2535,7 +2532,6 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, size_t i; virDevicePCIAddress tmp_addr; bool qemuDeviceVideoUsable = virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY); - virDevicePCIAddressPtr addrptr; char *addrStr = NULL; qemuDomainPCIConnectFlags flags = QEMU_PCI_CONNECT_TYPE_PCIE; @@ -2619,22 +2615,17 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, */ virDomainVideoDefPtr primaryVideo = def->videos[0]; if (primaryVideo->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { - primaryVideo->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; - primaryVideo->info.addr.pci.domain = 0; - primaryVideo->info.addr.pci.bus = 0; - primaryVideo->info.addr.pci.slot = 1; - primaryVideo->info.addr.pci.function = 0; - addrptr = &primaryVideo->info.addr.pci; - - if (!(addrStr = qemuDomainPCIAddressAsString(addrptr))) + memset(&tmp_addr, 0, sizeof(tmp_addr)); + tmp_addr.slot = 1; + + if (!(addrStr = qemuDomainPCIAddressAsString(&tmp_addr))) goto cleanup; - if (!qemuDomainPCIAddressValidate(addrs, addrptr, + if (!qemuDomainPCIAddressValidate(addrs, &tmp_addr, addrStr, flags, false)) goto cleanup; - if (qemuDomainPCIAddressSlotInUse(addrs, addrptr)) { + if (qemuDomainPCIAddressSlotInUse(addrs, &tmp_addr)) { if (qemuDeviceVideoUsable) { - virResetLastError(); if (qemuDomainPCIAddressReserveNextSlot(addrs, &primaryVideo->info, flags) < 0) @@ -2645,8 +2636,11 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, "QEMU needs it for primary video")); goto cleanup; } - } else if (qemuDomainPCIAddressReserveSlot(addrs, addrptr, flags) < 0) { - goto cleanup; + } else { + if (qemuDomainPCIAddressReserveSlot(addrs, &tmp_addr, flags) < 0) + goto cleanup; + primaryVideo->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; + primaryVideo->info.addr.pci = tmp_addr; } } else if (!qemuDeviceVideoUsable) { if (primaryVideo->info.addr.pci.domain != 0 || diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 847e96f..5a2bfcc 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1193,7 +1193,8 @@ mymain(void) DO_TEST("video-device-pciaddr-default", QEMU_CAPS_KVM, QEMU_CAPS_VNC, QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_VIDEO_PRIMARY, - QEMU_CAPS_DEVICE_QXL, QEMU_CAPS_DEVICE_QXL_VGA); + QEMU_CAPS_DEVICE_QXL, QEMU_CAPS_DEVICE_QXL_VGA, + QEMU_CAPS_DEVICE_PCI_BRIDGE); DO_TEST("virtio-rng-default", QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_VIRTIO_RNG, QEMU_CAPS_OBJECT_RNG_RANDOM); -- 1.8.3.2
On Thu, Dec 19, 2013 at 17:43:33 +0100, Jano Tomko wrote:
Don't set the PCI address of primary video to the default if it's already occupied by another device.
Without this, a primary video card that doesn't have an address gets the default one on the 1st pass of PCI address allocation, even if it's occupied. This address gets picked up by the second pass, and a conflict occurs:
XML error: Attempted double use of PCI slot 0000:00:02.0 (may need "multifunction='on'" for device on function 0)
Also fix the test that was supposed to catch this.
And does it actually work after this patch? IIRC, the primary video card is forced to be at 0000:00:02.0 by QEMU. That is, if this address is occupied by another device, there should be no way to add a video card. Jirka
On 12/19/2013 06:18 PM, Jiri Denemark wrote:
On Thu, Dec 19, 2013 at 17:43:33 +0100, Jano Tomko wrote:
Don't set the PCI address of primary video to the default if it's already occupied by another device.
Without this, a primary video card that doesn't have an address gets the default one on the 1st pass of PCI address allocation, even if it's occupied. This address gets picked up by the second pass, and a conflict occurs:
XML error: Attempted double use of PCI slot 0000:00:02.0 (may need "multifunction='on'" for device on function 0)
Also fix the test that was supposed to catch this.
And does it actually work after this patch? IIRC, the primary video card is forced to be at 0000:00:02.0 by QEMU. That is, if this address is occupied by another device, there should be no way to add a video card.
It works without this patch too, if you explicitly specify the PCI address of the video card (and you have QEMU >= 1.6, because that's when we use -device instead of -vga - (the QEMU_CAPS_DEVICE_VIDEO_PRIMARY capability)). Jan
On Thu, Dec 19, 2013 at 18:41:25 +0100, Jano Tomko wrote:
On 12/19/2013 06:18 PM, Jiri Denemark wrote:
On Thu, Dec 19, 2013 at 17:43:33 +0100, Jano Tomko wrote:
Don't set the PCI address of primary video to the default if it's already occupied by another device.
Without this, a primary video card that doesn't have an address gets the default one on the 1st pass of PCI address allocation, even if it's occupied. This address gets picked up by the second pass, and a conflict occurs:
XML error: Attempted double use of PCI slot 0000:00:02.0 (may need "multifunction='on'" for device on function 0)
Also fix the test that was supposed to catch this.
And does it actually work after this patch? IIRC, the primary video card is forced to be at 0000:00:02.0 by QEMU. That is, if this address is occupied by another device, there should be no way to add a video card.
It works without this patch too, if you explicitly specify the PCI address of the video card (and you have QEMU >= 1.6, because that's when we use -device instead of -vga - (the QEMU_CAPS_DEVICE_VIDEO_PRIMARY capability)).
Oh so this is only changing the situation for QEMU >= 1.6? This makes more sense now although it wasn't exactly obvious when looking at the patch without bigger context. It could be a good idea to explicitly mention this in the commit message. Jirka
On 12/19/2013 07:41 PM, Ján Tomko wrote:
On 12/19/2013 06:18 PM, Jiri Denemark wrote:
On Thu, Dec 19, 2013 at 17:43:33 +0100, Jano Tomko wrote:
Don't set the PCI address of primary video to the default if it's already occupied by another device.
Without this, a primary video card that doesn't have an address gets the default one on the 1st pass of PCI address allocation, even if it's occupied. This address gets picked up by the second pass, and a conflict occurs:
XML error: Attempted double use of PCI slot 0000:00:02.0 (may need "multifunction='on'" for device on function 0)
Also fix the test that was supposed to catch this. And does it actually work after this patch? IIRC, the primary video card is forced to be at 0000:00:02.0 by QEMU. That is, if this address is occupied by another device, there should be no way to add a video card.
It works without this patch too, if you explicitly specify the PCI address of the video card (and you have QEMU >= 1.6, because that's when we use -device instead of -vga - (the QEMU_CAPS_DEVICE_VIDEO_PRIMARY capability)).
Isn't there also something about multi-display support that requires the primary video to be at a particular address?
On Fri, Dec 20, 2013 at 10:17:22AM +0200, Laine Stump wrote:
On 12/19/2013 07:41 PM, Ján Tomko wrote:
On 12/19/2013 06:18 PM, Jiri Denemark wrote:
On Thu, Dec 19, 2013 at 17:43:33 +0100, Jano Tomko wrote:
Don't set the PCI address of primary video to the default if it's already occupied by another device.
Without this, a primary video card that doesn't have an address gets the default one on the 1st pass of PCI address allocation, even if it's occupied. This address gets picked up by the second pass, and a conflict occurs:
XML error: Attempted double use of PCI slot 0000:00:02.0 (may need "multifunction='on'" for device on function 0)
Also fix the test that was supposed to catch this. And does it actually work after this patch? IIRC, the primary video card is forced to be at 0000:00:02.0 by QEMU. That is, if this address is occupied by another device, there should be no way to add a video card.
It works without this patch too, if you explicitly specify the PCI address of the video card (and you have QEMU >= 1.6, because that's when we use -device instead of -vga - (the QEMU_CAPS_DEVICE_VIDEO_PRIMARY capability)).
Isn't there also something about multi-display support that requires the primary video to be at a particular address?
FWIW, the only reason why I had hardcoded display originally was because we were forced to use -vga instead of -device, and so could't control the PCI port. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (4)
-
Daniel P. Berrange -
Jiri Denemark -
Ján Tomko -
Laine Stump