10:43 a.m.
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
11:18 a.m.
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
11:41 a.m.
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
1:50 p.m.
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
2:17 a.m.
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?
4:17 a.m.
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 :|
3990
days inactive
3991
days old
5 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Jiri Denemark -
Ján Tomko -
Laine Stump