[libvirt] [PATCH v2 1/4] qemu: add qemu various VGA devices Caps flag
QEMU_CAPS_DEVICE_QXL -device qxl QEMU_CAPS_DEVICE_VGA -device VGA QEMU_CAPS_DEVICE_CIRRUS_VGA -device cirrus-vga QEMU_CAPS_DEVICE_VMWARE_SVGA -device vmware-svga Fix a typo in qemuCapsObjectTypes, the string 'qxl' here should be -device qxl rather than -vga [...|qxl|..] --- src/qemu/qemu_capabilities.c | 10 ++++++++- src/qemu/qemu_capabilities.h | 4 ++++ tests/qemuhelptest.c | 48 +++++++++++++++++++++++++++++++++++--------- 3 files changed, 52 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 01a1b98..97bc968 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -194,6 +194,11 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST, "usb-redir.bootindex", "usb-host.bootindex", "blockdev-snapshot-sync", + "qxl", + + "VGA", /* 200 */ + "cirrus-vga", + "vmware-svga", ); struct _qemuCaps { @@ -1322,11 +1327,14 @@ struct qemuCapsStringFlags qemuCapsObjectTypes[] = { { "virtio-scsi-pci", QEMU_CAPS_VIRTIO_SCSI_PCI }, { "spicevmc", QEMU_CAPS_DEVICE_SPICEVMC }, { "qxl-vga", QEMU_CAPS_DEVICE_QXL_VGA }, - { "qxl", QEMU_CAPS_VGA_QXL }, + { "qxl", QEMU_CAPS_DEVICE_QXL }, { "sga", QEMU_CAPS_SGA }, { "scsi-block", QEMU_CAPS_SCSI_BLOCK }, { "scsi-cd", QEMU_CAPS_SCSI_CD }, { "ide-cd", QEMU_CAPS_IDE_CD }, + { "VGA", QEMU_CAPS_DEVICE_VGA }, + { "cirrus-vga", QEMU_CAPS_DEVICE_CIRRUS_VGA }, + { "vmware-svga", QEMU_CAPS_DEVICE_VMWARE_SVGA }, }; diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 3da8672..28604ce 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -156,6 +156,10 @@ enum qemuCapsFlags { QEMU_CAPS_USB_REDIR_BOOTINDEX = 116, /* usb-redir.bootindex */ QEMU_CAPS_USB_HOST_BOOTINDEX = 117, /* usb-host.bootindex */ QEMU_CAPS_DISK_SNAPSHOT = 118, /* blockdev-snapshot-sync command */ + QEMU_CAPS_DEVICE_QXL = 119, /* -device qxl */ + QEMU_CAPS_DEVICE_VGA = 120, /* -device VGA */ + QEMU_CAPS_DEVICE_CIRRUS_VGA = 121, /* -device cirrus-vga */ + QEMU_CAPS_DEVICE_VMWARE_SVGA = 122, /* -device vmware-svga */ QEMU_CAPS_LAST, /* this must always be the last item */ }; diff --git a/tests/qemuhelptest.c b/tests/qemuhelptest.c index 021b3dc..2280f2d 100644 --- a/tests/qemuhelptest.c +++ b/tests/qemuhelptest.c @@ -391,7 +391,11 @@ mymain(void) QEMU_CAPS_NO_ACPI, QEMU_CAPS_VIRTIO_BLK_SG_IO, QEMU_CAPS_CPU_HOST, - QEMU_CAPS_VNC); + QEMU_CAPS_VNC, + QEMU_CAPS_DEVICE_QXL, + QEMU_CAPS_DEVICE_VGA, + QEMU_CAPS_DEVICE_CIRRUS_VGA, + QEMU_CAPS_DEVICE_VMWARE_SVGA); DO_TEST("qemu-kvm-0.12.3", 12003, 1, 0, QEMU_CAPS_VNC_COLON, QEMU_CAPS_NO_REBOOT, @@ -477,7 +481,6 @@ mymain(void) QEMU_CAPS_NESTING, QEMU_CAPS_NAME_PROCESS, QEMU_CAPS_SMBIOS_TYPE, - QEMU_CAPS_VGA_QXL, QEMU_CAPS_SPICE, QEMU_CAPS_VGA_NONE, QEMU_CAPS_MIGRATE_QEMU_FD, @@ -495,7 +498,11 @@ mymain(void) QEMU_CAPS_VIRTIO_BLK_SG_IO, QEMU_CAPS_CPU_HOST, QEMU_CAPS_SCSI_LSI, - QEMU_CAPS_VNC); + QEMU_CAPS_VNC, + QEMU_CAPS_DEVICE_QXL, + QEMU_CAPS_DEVICE_VGA, + QEMU_CAPS_DEVICE_CIRRUS_VGA, + QEMU_CAPS_DEVICE_VMWARE_SVGA); DO_TEST("qemu-kvm-0.12.1.2-rhel61", 12001, 1, 0, QEMU_CAPS_VNC_COLON, QEMU_CAPS_NO_REBOOT, @@ -554,7 +561,11 @@ mymain(void) QEMU_CAPS_VIRTIO_BLK_SG_IO, QEMU_CAPS_CPU_HOST, QEMU_CAPS_BLOCKIO, - QEMU_CAPS_VNC); + QEMU_CAPS_VNC, + QEMU_CAPS_DEVICE_QXL, + QEMU_CAPS_DEVICE_VGA, + QEMU_CAPS_DEVICE_CIRRUS_VGA, + QEMU_CAPS_DEVICE_VMWARE_SVGA); DO_TEST("qemu-kvm-0.12.1.2-rhel62-beta", 12001, 1, 0, QEMU_CAPS_VNC_COLON, QEMU_CAPS_NO_REBOOT, @@ -623,7 +634,10 @@ mymain(void) QEMU_CAPS_CPU_HOST, QEMU_CAPS_SCSI_CD, QEMU_CAPS_BLOCKIO, - QEMU_CAPS_VNC); + QEMU_CAPS_VNC, + QEMU_CAPS_DEVICE_QXL, + QEMU_CAPS_DEVICE_VGA, + QEMU_CAPS_DEVICE_CIRRUS_VGA); DO_TEST("qemu-1.0", 1000000, 0, 0, QEMU_CAPS_VNC_COLON, QEMU_CAPS_NO_REBOOT, @@ -697,7 +711,11 @@ mymain(void) QEMU_CAPS_IDE_CD, QEMU_CAPS_SCSI_LSI, QEMU_CAPS_BLOCKIO, - QEMU_CAPS_VNC); + QEMU_CAPS_VNC, + QEMU_CAPS_DEVICE_QXL, + QEMU_CAPS_DEVICE_VGA, + QEMU_CAPS_DEVICE_CIRRUS_VGA, + QEMU_CAPS_DEVICE_VMWARE_SVGA); DO_TEST("qemu-1.1.0", 1001000, 0, 0, QEMU_CAPS_VNC_COLON, QEMU_CAPS_NO_REBOOT, @@ -779,7 +797,11 @@ mymain(void) QEMU_CAPS_SCSI_LSI, QEMU_CAPS_VIRTIO_SCSI_PCI, QEMU_CAPS_BLOCKIO, - QEMU_CAPS_VNC); + QEMU_CAPS_VNC, + QEMU_CAPS_DEVICE_QXL, + QEMU_CAPS_DEVICE_VGA, + QEMU_CAPS_DEVICE_CIRRUS_VGA, + QEMU_CAPS_DEVICE_VMWARE_SVGA); DO_TEST("qemu-1.2.0", 1002000, 0, 0, QEMU_CAPS_VNC_COLON, QEMU_CAPS_NO_REBOOT, @@ -871,7 +893,11 @@ mymain(void) QEMU_CAPS_DUMP_GUEST_CORE, QEMU_CAPS_VNC, QEMU_CAPS_USB_REDIR_BOOTINDEX, - QEMU_CAPS_USB_HOST_BOOTINDEX); + QEMU_CAPS_USB_HOST_BOOTINDEX, + QEMU_CAPS_DEVICE_QXL, + QEMU_CAPS_DEVICE_VGA, + QEMU_CAPS_DEVICE_CIRRUS_VGA, + QEMU_CAPS_DEVICE_VMWARE_SVGA); DO_TEST("qemu-kvm-1.2.0", 1002000, 1, 0, QEMU_CAPS_VNC_COLON, QEMU_CAPS_NO_REBOOT, @@ -968,7 +994,11 @@ mymain(void) QEMU_CAPS_DUMP_GUEST_CORE, QEMU_CAPS_VNC, QEMU_CAPS_USB_REDIR_BOOTINDEX, - QEMU_CAPS_USB_HOST_BOOTINDEX); + QEMU_CAPS_USB_HOST_BOOTINDEX, + QEMU_CAPS_DEVICE_QXL, + QEMU_CAPS_DEVICE_VGA, + QEMU_CAPS_DEVICE_CIRRUS_VGA, + QEMU_CAPS_DEVICE_VMWARE_SVGA); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 1.7.11.2
If there are multiple video devices primary = 'yes' marks this video device as the primary one. The rest are secondary video devices. No more than one could be mark as primary. If none of them has primary attribute, the first one will be the primary by default like what it was. The reason of this changing is that for qemu, only one primary video device is permitted which can be of any type. For secondary video devices, only qxl is allowd. Primary attribute removes the restriction that the first have to be the primary one. We always put the primary video device into the first position of video device structure array after parsing. --- src/conf/domain_conf.c | 25 +++++++++++++++++++++++++ src/conf/domain_conf.h | 1 + 2 files changed, 26 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6aa5f79..862ab0f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7275,6 +7275,7 @@ virDomainVideoDefParseXML(const xmlNodePtr node, char *type = NULL; char *heads = NULL; char *vram = NULL; + char *primary = NULL; if (VIR_ALLOC(def) < 0) { virReportOOMError(); @@ -7289,6 +7290,11 @@ virDomainVideoDefParseXML(const xmlNodePtr node, type = virXMLPropString(cur, "type"); vram = virXMLPropString(cur, "vram"); heads = virXMLPropString(cur, "heads"); + + if ((primary = virXMLPropString(cur, "primary"))!= NULL) + if (STREQ(primary, "yes")) + def->primary = 1; + def->accel = virDomainVideoAccelDefParseXML(cur); } } @@ -8669,6 +8675,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, xmlNodePtr cur; bool usb_none = false; bool usb_other = false; + bool primaryVideo = false; if (VIR_ALLOC(def) < 0) { virReportOOMError(); @@ -9961,6 +9968,22 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, flags); if (!video) goto error; + + if (!primaryVideo && video->primary) { + if (def->nvideos != 0) + memmove(def->videos + 1, + def->videos, + (sizeof(def->videos[0]) * def->nvideos)); + + def->videos[0] = video; + def->nvideos++; + primaryVideo = true; + continue; + } else if (primaryVideo && video->primary) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Only one primary video device is supported")); + goto error; + } def->videos[def->nvideos++] = video; } VIR_FREE(nodes); @@ -13109,6 +13132,8 @@ virDomainVideoDefFormat(virBufferPtr buf, virBufferAsprintf(buf, " vram='%u'", def->vram); if (def->heads) virBufferAsprintf(buf, " heads='%u'", def->heads); + if (def->primary) + virBufferAddLit(buf, " primary='yes'"); if (def->accel) { virBufferAddLit(buf, ">\n"); virDomainVideoAccelDefFormat(buf, def->accel); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 7ad5377..bf6b532 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1120,6 +1120,7 @@ struct _virDomainVideoDef { int type; unsigned int vram; unsigned int heads; + unsigned int primary : 1; virDomainVideoAccelDefPtr accel; virDomainDeviceInfo info; }; -- 1.7.11.2
On 12/11/2012 07:14 AM, Guannan Ren wrote:
If there are multiple video devices primary = 'yes' marks this video device as the primary one. The rest are secondary video devices. No more than one could be mark as primary. If none of them has primary attribute, the first one will be the primary by default like what it was. The reason of this changing is that for qemu, only one primary video device is permitted which can be of any type. For secondary video devices, only qxl is allowd. Primary attribute removes the restriction that the first have to be the primary one.
We always put the primary video device into the first position of video device structure array after parsing. --- src/conf/domain_conf.c | 25 +++++++++++++++++++++++++ src/conf/domain_conf.h | 1 + 2 files changed, 26 insertions(+)
Incomplete. I can't approve this without a change to docs/schemas/domaincommon.rng to parse the new attribute, and to docs/formatdomain.html.in to document it.
@@ -7289,6 +7290,11 @@ virDomainVideoDefParseXML(const xmlNodePtr node, type = virXMLPropString(cur, "type"); vram = virXMLPropString(cur, "vram"); heads = virXMLPropString(cur, "heads"); + + if ((primary = virXMLPropString(cur, "primary"))!= NULL)
Space before !=.
@@ -9961,6 +9968,22 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, flags); if (!video) goto error; + + if (!primaryVideo && video->primary) { + if (def->nvideos != 0) + memmove(def->videos + 1, + def->videos, + (sizeof(def->videos[0]) * def->nvideos)); + + def->videos[0] = video; + def->nvideos++; + primaryVideo = true;
This would be a good candidate to use Laine's new VIR_INSERT_ELEMENT macro.
+++ b/src/conf/domain_conf.h @@ -1120,6 +1120,7 @@ struct _virDomainVideoDef { int type; unsigned int vram; unsigned int heads; + unsigned int primary : 1;
It's probably easier to use 'bool primary' instead of a bitfield here. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 12/11/2012 01:11 PM, Eric Blake wrote:
On 12/11/2012 07:14 AM, Guannan Ren wrote:
If there are multiple video devices primary = 'yes' marks this video device as the primary one. The rest are secondary video devices. No more than one could be mark as primary. If none of them has primary attribute, the first one will be the primary by default like what it was. The reason of this changing is that for qemu, only one primary video device is permitted which can be of any type. For secondary video devices, only qxl is allowd. Primary attribute removes the restriction that the first have to be the primary one.
We always put the primary video device into the first position of video device structure array after parsing. --- src/conf/domain_conf.c | 25 +++++++++++++++++++++++++ src/conf/domain_conf.h | 1 + 2 files changed, 26 insertions(+) Incomplete. I can't approve this without a change to docs/schemas/domaincommon.rng to parse the new attribute, and to docs/formatdomain.html.in to document it.
@@ -7289,6 +7290,11 @@ virDomainVideoDefParseXML(const xmlNodePtr node, type = virXMLPropString(cur, "type"); vram = virXMLPropString(cur, "vram"); heads = virXMLPropString(cur, "heads"); + + if ((primary = virXMLPropString(cur, "primary"))!= NULL) Space before !=.
@@ -9961,6 +9968,22 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, flags); if (!video) goto error; + + if (!primaryVideo && video->primary) { + if (def->nvideos != 0) + memmove(def->videos + 1, + def->videos, + (sizeof(def->videos[0]) * def->nvideos)); + + def->videos[0] = video; + def->nvideos++; + primaryVideo = true; This would be a good candidate to use Laine's new VIR_INSERT_ELEMENT macro.
Heh. Yep. Even in my patches that I didn't push, I hadn't done the substitution here because it was only appending and only preallocated memory (so there was no real gain). But since you're optionally inserting at the beginning, you could do this: size_t ii = def->nvideos; if (video->primary) { if (primaryVideo) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Only one primary video device is supported")); goto error; } ii = 0; primaryVideo = true; } VIR_INSERT_ELEMENT_INPLACE(def->videos, ii, def->nvideos, video); (then you don't need the "def->videos[def->nvideos++] = video" that follows the chunk from your original patch)
'-device VGA' maps to '-vga std' '-device cirrus-vga' maps to '-vga cirrus' '-device qxl-vga' maps to '-vga qxl' (there is also '-device qxl' for secondary devices) '-device vmware-svga' maps to '-vga vmware' For qemu(>=1.2), we can use -device to replace -vga for video device. For the primary video device, the patch trys to use 0x2 slot for matching old qemu. If the 0x2 slot is allocated already, the addr property could help for using any available slot. For qemu(< 1.2), we keep using -vga for primary device. --- src/qemu/qemu_command.c | 132 ++++++++++++++++++++++++++++++++++++----------- src/qemu/qemu_command.h | 4 +- tests/qemuxml2argvtest.c | 9 ++-- 3 files changed, 110 insertions(+), 35 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 5335dcf..7f1f3ee 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -93,6 +93,16 @@ VIR_ENUM_IMPL(qemuVideo, VIR_DOMAIN_VIDEO_TYPE_LAST, "", /* don't support vbox */ "qxl"); +VIR_ENUM_DECL(qemuDeviceVideo) + +VIR_ENUM_IMPL(qemuDeviceVideo, VIR_DOMAIN_VIDEO_TYPE_LAST, + "VGA", + "cirrus-vga", + "vmware-svga", + "", /* no device for xen */ + "", /* don't support vbox */ + "qxl-vga"); + VIR_ENUM_DECL(qemuSoundCodec) VIR_ENUM_IMPL(qemuSoundCodec, VIR_DOMAIN_SOUND_CODEC_TYPE_LAST, @@ -1057,7 +1067,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, if (!(addrs = qemuDomainPCIAddressSetCreate(def))) goto cleanup; - if (qemuAssignDevicePCISlots(def, addrs) < 0) + if (qemuAssignDevicePCISlots(def, caps, addrs) < 0) goto cleanup; } @@ -1415,12 +1425,15 @@ int qemuDomainPCIAddressSetNextAddr(qemuDomainPCIAddressSetPtr addrs, * skip over info.type == PCI */ int -qemuAssignDevicePCISlots(virDomainDefPtr def, qemuDomainPCIAddressSetPtr addrs) +qemuAssignDevicePCISlots(virDomainDefPtr def, + qemuCapsPtr caps, + qemuDomainPCIAddressSetPtr addrs) { size_t i, j; bool reservedIDE = false; bool reservedUSB = false; int function; + bool qemu1dot2plus = qemuCapsGetVersion(caps) >= 1002000; /* Host bridge */ if (qemuDomainPCIAddressReserveSlot(addrs, 0) < 0) @@ -1487,29 +1500,42 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, qemuDomainPCIAddressSetPtr addrs) goto error; } - /* First VGA is hardcoded slot=2 */ if (def->nvideos > 0) { - if (def->videos[0]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { - if (def->videos[0]->info.addr.pci.domain != 0 || - def->videos[0]->info.addr.pci.bus != 0 || - def->videos[0]->info.addr.pci.slot != 2 || - def->videos[0]->info.addr.pci.function != 0) { + 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; + + if (qemuDomainPCIAddressCheckSlot(addrs, &primaryVideo->info) < 0) { + if (qemu1dot2plus) { + virResetLastError(); + if (qemuDomainPCIAddressSetNextAddr(addrs, &primaryVideo->info) < 0) + goto error; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("PCI address 0:0:2.0 is in use, " + "QEMU needs it for primary video")); + goto error; + } + } else if (qemuDomainPCIAddressReserveSlot(addrs, 2) < 0) { + goto error; + } + } else if (!qemu1dot2plus) { + if (primaryVideo->info.addr.pci.domain != 0 || + primaryVideo->info.addr.pci.bus != 0 || + primaryVideo->info.addr.pci.slot != 2 || + primaryVideo->info.addr.pci.function != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Primary video card must have PCI address 0:0:2.0")); goto error; } /* If TYPE==PCI, then qemuCollectPCIAddress() function * has already reserved the address, so we must skip */ - } else { - def->videos[0]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; - def->videos[0]->info.addr.pci.domain = 0; - def->videos[0]->info.addr.pci.bus = 0; - def->videos[0]->info.addr.pci.slot = 2; - def->videos[0]->info.addr.pci.function = 0; - if (qemuDomainPCIAddressReserveSlot(addrs, 2) < 0) - goto error; } - } else { + } else if (!qemu1dot2plus) { virDomainDeviceInfo dev; memset(&dev, 0, sizeof(dev)); dev.addr.pci.slot = 2; @@ -1682,8 +1708,13 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, qemuDomainPCIAddressSetPtr addrs) goto error; } - /* Further non-primary video cards */ + /* Further non-primary video cards which have to be qxl type */ for (i = 1; i < def->nvideos ; i++) { + if (def->videos[i]->type != VIR_DOMAIN_VIDEO_TYPE_QXL) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("non-primary video device must be type of 'qxl'")); + goto error; + } if (def->videos[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) continue; if (qemuDomainPCIAddressSetNextAddr(addrs, &def->videos[i]->info) < 0) @@ -3501,16 +3532,35 @@ error: } static char * -qemuBuildVideoDevStr(virDomainVideoDefPtr video, - qemuCapsPtr caps) +qemuBuildDeviceVideoStr(virDomainVideoDefPtr video, + qemuCapsPtr caps, + bool primary) { virBuffer buf = VIR_BUFFER_INITIALIZER; - const char *model = qemuVideoTypeToString(video->type); + const char *model; - if (!model) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("invalid video model")); - goto error; + if (primary) { + model = qemuDeviceVideoTypeToString(video->type); + if (!model || STREQ(model, "")) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("video type %s is not supported with QEMU"), + virDomainVideoTypeToString(video->type)); + goto error; + } + } else { + if (video->type != VIR_DOMAIN_VIDEO_TYPE_QXL) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", _("non-primary video device must be type of 'qxl'")); + goto error; + } + + if (!qemuCapsGet(caps, QEMU_CAPS_DEVICE_QXL)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", _("only one video card is currently supported")); + goto error; + } + + model = "qxl"; } virBufferAsprintf(&buf, "%s,id=%s", model, video->info.alias); @@ -6427,22 +6477,42 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; } if (def->nvideos > 0) { - if (qemuCapsGet(caps, QEMU_CAPS_VGA)) { - if (def->videos[0]->type == VIR_DOMAIN_VIDEO_TYPE_XEN) { + int primaryVideoType = def->videos[0]->type; + if (qemuCapsGetVersion(caps) >= 1002000 && + ((primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_VGA && + qemuCapsGet(caps, QEMU_CAPS_DEVICE_VGA)) || + (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_CIRRUS && + qemuCapsGet(caps, QEMU_CAPS_DEVICE_CIRRUS_VGA)) || + (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_VMVGA && + qemuCapsGet(caps, QEMU_CAPS_DEVICE_VMWARE_SVGA)) || + (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_QXL && + qemuCapsGet(caps, QEMU_CAPS_DEVICE_QXL_VGA))) + ) { + for (i = 0 ; i < def->nvideos ; i++) { + char *str; + virCommandAddArg(cmd, "-device"); + if (!(str = qemuBuildDeviceVideoStr(def->videos[i], caps, !i))) + goto error; + + virCommandAddArg(cmd, str); + VIR_FREE(str); + } + } else if (qemuCapsGet(caps, QEMU_CAPS_VGA)) { + if (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_XEN) { /* nothing - vga has no effect on Xen pvfb */ } else { - if ((def->videos[0]->type == VIR_DOMAIN_VIDEO_TYPE_QXL) && + if ((primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_QXL) && !qemuCapsGet(caps, QEMU_CAPS_VGA_QXL)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("This QEMU does not support QXL graphics adapters")); goto error; } - const char *vgastr = qemuVideoTypeToString(def->videos[0]->type); + const char *vgastr = qemuVideoTypeToString(primaryVideoType); if (!vgastr || STREQ(vgastr, "")) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("video type %s is not supported with QEMU"), - virDomainVideoTypeToString(def->videos[0]->type)); + virDomainVideoTypeToString(primaryVideoType)); goto error; } @@ -6506,7 +6576,7 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, "-device"); - if (!(str = qemuBuildVideoDevStr(def->videos[i], caps))) + if (!(str = qemuBuildDeviceVideoStr(def->videos[i], caps, false))) goto error; virCommandAddArg(cmd, str); diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 6556e6e..0dde4be 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -211,7 +211,9 @@ int qemuDomainPCIAddressReleaseFunction(qemuDomainPCIAddressSetPtr addrs, int qemuDomainPCIAddressReleaseSlot(qemuDomainPCIAddressSetPtr addrs, int slot); void qemuDomainPCIAddressSetFree(qemuDomainPCIAddressSetPtr addrs); -int qemuAssignDevicePCISlots(virDomainDefPtr def, qemuDomainPCIAddressSetPtr addrs); +int qemuAssignDevicePCISlots(virDomainDefPtr def, + qemuCapsPtr caps, + qemuDomainPCIAddressSetPtr addrs); int qemuAssignDeviceAliases(virDomainDefPtr def, qemuCapsPtr caps); int qemuDomainNetVLAN(virDomainNetDefPtr def); diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index bb233ed..ae6c2a8 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -574,7 +574,8 @@ mymain(void) QEMU_CAPS_VGA, QEMU_CAPS_VGA_NONE); DO_TEST("graphics-spice", QEMU_CAPS_VGA, QEMU_CAPS_VGA_QXL, - QEMU_CAPS_DEVICE, QEMU_CAPS_SPICE); + QEMU_CAPS_DEVICE, QEMU_CAPS_SPICE, + QEMU_CAPS_DEVICE_QXL); DO_TEST("graphics-spice-agentmouse", QEMU_CAPS_VGA, QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE, QEMU_CAPS_SPICE, @@ -582,7 +583,8 @@ mymain(void) QEMU_CAPS_NODEFCONFIG); DO_TEST("graphics-spice-compression", QEMU_CAPS_VGA, QEMU_CAPS_VGA_QXL, - QEMU_CAPS_DEVICE, QEMU_CAPS_SPICE); + QEMU_CAPS_DEVICE, QEMU_CAPS_SPICE, + QEMU_CAPS_DEVICE_QXL); DO_TEST("graphics-spice-timeout", QEMU_CAPS_DRIVE, QEMU_CAPS_VGA, QEMU_CAPS_VGA_QXL, @@ -591,7 +593,8 @@ mymain(void) DO_TEST("graphics-spice-qxl-vga", QEMU_CAPS_VGA, QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE, QEMU_CAPS_SPICE, - QEMU_CAPS_DEVICE_QXL_VGA); + QEMU_CAPS_DEVICE_QXL_VGA, + QEMU_CAPS_DEVICE_QXL); DO_TEST("graphics-spice-usb-redir", QEMU_CAPS_VGA, QEMU_CAPS_SPICE, QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG, -- 1.7.11.2
On 12/11/2012 07:14 AM, Guannan Ren wrote:
'-device VGA' maps to '-vga std' '-device cirrus-vga' maps to '-vga cirrus' '-device qxl-vga' maps to '-vga qxl' (there is also '-device qxl' for secondary devices) '-device vmware-svga' maps to '-vga vmware'
For qemu(>=1.2), we can use -device to replace -vga for video device. For the primary video device, the patch trys to use 0x2 slot for matching old qemu. If the 0x2 slot is allocated already, the addr property could help for using any available slot. For qemu(< 1.2), we keep using -vga for primary device. --- src/qemu/qemu_command.c | 132 ++++++++++++++++++++++++++++++++++++----------- src/qemu/qemu_command.h | 4 +- tests/qemuxml2argvtest.c | 9 ++-- 3 files changed, 110 insertions(+), 35 deletions(-)
{ size_t i, j; bool reservedIDE = false; bool reservedUSB = false; int function; + bool qemu1dot2plus = qemuCapsGetVersion(caps) >= 1002000;
Eww. Version string comparison is wrong, as it is feasible that someone could backport the improved command line to older qemu. We should instead be going off of whether specific '-device XXX' is supported; probably by checking for QEMU_CAPS_DEVICE_QXL.
@@ -6427,22 +6477,42 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; } if (def->nvideos > 0) { - if (qemuCapsGet(caps, QEMU_CAPS_VGA)) { - if (def->videos[0]->type == VIR_DOMAIN_VIDEO_TYPE_XEN) { + int primaryVideoType = def->videos[0]->type; + if (qemuCapsGetVersion(caps) >= 1002000 &&
Again, version comparison feels wrong. What are you really gating on, whether qemu is new enough to support primary video on a non-default address? If so, set up the right capability bit for that.
+ ((primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_VGA && + qemuCapsGet(caps, QEMU_CAPS_DEVICE_VGA)) || + (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_CIRRUS && + qemuCapsGet(caps, QEMU_CAPS_DEVICE_CIRRUS_VGA)) || + (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_VMVGA && + qemuCapsGet(caps, QEMU_CAPS_DEVICE_VMWARE_SVGA)) || + (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_QXL && + qemuCapsGet(caps, QEMU_CAPS_DEVICE_QXL_VGA)))
Or is the existence of these four device capability bits sufficient?
+++ b/tests/qemuxml2argvtest.c @@ -574,7 +574,8 @@ mymain(void) QEMU_CAPS_VGA, QEMU_CAPS_VGA_NONE); DO_TEST("graphics-spice", QEMU_CAPS_VGA, QEMU_CAPS_VGA_QXL, - QEMU_CAPS_DEVICE, QEMU_CAPS_SPICE); + QEMU_CAPS_DEVICE, QEMU_CAPS_SPICE, + QEMU_CAPS_DEVICE_QXL);
Where are the tests for the new command line possible with newer qemu? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 12/12/2012 02:25 AM, Eric Blake wrote:
'-device VGA' maps to '-vga std' '-device cirrus-vga' maps to '-vga cirrus' '-device qxl-vga' maps to '-vga qxl' (there is also '-device qxl' for secondary devices) '-device vmware-svga' maps to '-vga vmware'
For qemu(>=1.2), we can use -device to replace -vga for video device. For the primary video device, the patch trys to use 0x2 slot for matching old qemu. If the 0x2 slot is allocated already, the addr property could help for using any available slot. For qemu(< 1.2), we keep using -vga for primary device. --- src/qemu/qemu_command.c | 132 ++++++++++++++++++++++++++++++++++++----------- src/qemu/qemu_command.h | 4 +- tests/qemuxml2argvtest.c | 9 ++-- 3 files changed, 110 insertions(+), 35 deletions(-)
{ size_t i, j; bool reservedIDE = false; bool reservedUSB = false; int function; + bool qemu1dot2plus = qemuCapsGetVersion(caps) >= 1002000; Eww. Version string comparison is wrong, as it is feasible that someone could backport the improved command line to older qemu. We should instead be going off of whether specific '-device XXX' is supported;
On 12/11/2012 07:14 AM, Guannan Ren wrote: probably by checking for QEMU_CAPS_DEVICE_QXL.
@@ -6427,22 +6477,42 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; } if (def->nvideos > 0) { - if (qemuCapsGet(caps, QEMU_CAPS_VGA)) { - if (def->videos[0]->type == VIR_DOMAIN_VIDEO_TYPE_XEN) { + int primaryVideoType = def->videos[0]->type; + if (qemuCapsGetVersion(caps) >= 1002000 && Again, version comparison feels wrong. What are you really gating on, whether qemu is new enough to support primary video on a non-default address? If so, set up the right capability bit for that.
Although '-device XXX' is there but it probably doesn't work for qemu(<1.2) the following is the comments from Gerd Hoffmann in v1 "That will not work. You must check the qemu version. 1.2+ is safe, IIRC 1.1 will work too. Technical background: '-device VGA' (+variants) exists for a looooooong time, pretty much day one of the -device switch. Actually using it did not work though due to a nasty initialization order issue in qemu. The qemu memory api created by avi fixed it."
+ ((primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_VGA && + qemuCapsGet(caps, QEMU_CAPS_DEVICE_VGA)) || + (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_CIRRUS && + qemuCapsGet(caps, QEMU_CAPS_DEVICE_CIRRUS_VGA)) || + (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_VMVGA && + qemuCapsGet(caps, QEMU_CAPS_DEVICE_VMWARE_SVGA)) || + (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_QXL && + qemuCapsGet(caps, QEMU_CAPS_DEVICE_QXL_VGA))) Or is the existence of these four device capability bits sufficient?
device capability bits checking here is in case that one of four video device is missing on some qemu(>=1.2) though.
On 12/11/2012 08:32 PM, Guannan Ren wrote:
+ bool qemu1dot2plus = qemuCapsGetVersion(caps) >= 1002000; Eww. Version string comparison is wrong, as it is feasible that someone could backport the improved command line to older qemu. We should instead be going off of whether specific '-device XXX' is supported; probably by checking for QEMU_CAPS_DEVICE_QXL.
@@ -6427,22 +6477,42 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; } if (def->nvideos > 0) { - if (qemuCapsGet(caps, QEMU_CAPS_VGA)) { - if (def->videos[0]->type == VIR_DOMAIN_VIDEO_TYPE_XEN) { + int primaryVideoType = def->videos[0]->type; + if (qemuCapsGetVersion(caps) >= 1002000 && Again, version comparison feels wrong. What are you really gating on, whether qemu is new enough to support primary video on a non-default address? If so, set up the right capability bit for that.
Although '-device XXX' is there but it probably doesn't work for qemu(<1.2) the following is the comments from Gerd Hoffmann in v1
"That will not work. You must check the qemu version. 1.2+ is safe, IIRC 1.1 will work too.
Still, I'd rather see us create a capability bit, even if that bit is initially set according to a version check in qemu_capabilities.c, and use that bit rather than polluting the rest of the qemu code with additional version checks. That way, if someone manages to backport fixed device handling even to RHEL qemu 0.12.x, as well as advertise that backport, we can set the bit when the backport is detected. It may even be as simple as a RHEL-specific patch that merely blindly sets the capability bit at the one place where upstream sets the bit according to a version check. But no matter how we approach it, having a dedicated capability bit rather than multiple version checks will make backporting much easier. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 12/12/2012 09:22 PM, Eric Blake wrote:
On 12/11/2012 08:32 PM, Guannan Ren wrote:
+ bool qemu1dot2plus = qemuCapsGetVersion(caps) >= 1002000; Eww. Version string comparison is wrong, as it is feasible that someone could backport the improved command line to older qemu. We should instead be going off of whether specific '-device XXX' is supported; probably by checking for QEMU_CAPS_DEVICE_QXL.
@@ -6427,22 +6477,42 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; } if (def->nvideos > 0) { - if (qemuCapsGet(caps, QEMU_CAPS_VGA)) { - if (def->videos[0]->type == VIR_DOMAIN_VIDEO_TYPE_XEN) { + int primaryVideoType = def->videos[0]->type; + if (qemuCapsGetVersion(caps) >= 1002000 && Again, version comparison feels wrong. What are you really gating on, whether qemu is new enough to support primary video on a non-default address? If so, set up the right capability bit for that. Although '-device XXX' is there but it probably doesn't work for qemu(<1.2) the following is the comments from Gerd Hoffmann in v1
"That will not work. You must check the qemu version. 1.2+ is safe, IIRC 1.1 will work too. Still, I'd rather see us create a capability bit, even if that bit is initially set according to a version check in qemu_capabilities.c, and use that bit rather than polluting the rest of the qemu code with additional version checks. That way, if someone manages to backport fixed device handling even to RHEL qemu 0.12.x, as well as advertise that backport, we can set the bit when the backport is detected. It may even be as simple as a RHEL-specific patch that merely blindly sets the capability bit at the one place where upstream sets the bit according to a version check. But no matter how we approach it, having a dedicated capability bit rather than multiple version checks will make backporting much easier.
Agree, I will do that in v3. and setting a specific caps flags also make writing new testcase easier than checking qemu version directly. Guannan
On 12/11/12 19:25, Eric Blake wrote:
+ bool qemu1dot2plus = qemuCapsGetVersion(caps) >= 1002000; Eww. Version string comparison is wrong, as it is feasible that someone could backport the improved command line to older qemu. We should instead be going off of whether specific '-device XXX' is supported; probably by checking for QEMU_CAPS_DEVICE_QXL.
Isn't going to fly. -device VGA is present for a loooooooong time already, but only very recently it started to actually work. So checking for the devices being present isn't an option. The change which made it work is avi's memory api, which fixed a nasty initialization order issue. That change is invasive enougth that you don't have to worry about backports at all. cheers, Gerd
--- docs/formatdomain.html.in | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index c81af8a..3d1fdf4 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3485,7 +3485,11 @@ qemu-kvm -net nic,model=? /dev/null will add a default <code>video</code> according to the guest type. For a guest of type "kvm", the default <code>video</code> for it is: <code>type</code> with value "cirrus", <code>vram</code> with value - "9216", and <code>heads</code> with value "1". + "9216", and <code>heads</code> with value "1". By default, the first + video device in domain xml is the primary one, but the optional + attribute <code>primary</code> with value 'yes' can be used to mark the + primary in cases of mutiple video device. The non-primary must be + type of "qxl". </dd> <dt><code>model</code></dt> -- 1.7.11.2
On 12/11/2012 07:14 AM, Guannan Ren wrote:
--- docs/formatdomain.html.in | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
Squash this into 2/4. But still missing the RNG schema update, and preferably also an addition to tests/ that proves we can parse the new XML correctly.
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index c81af8a..3d1fdf4 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3485,7 +3485,11 @@ qemu-kvm -net nic,model=? /dev/null will add a default <code>video</code> according to the guest type. For a guest of type "kvm", the default <code>video</code> for it is: <code>type</code> with value "cirrus", <code>vram</code> with value - "9216", and <code>heads</code> with value "1". + "9216", and <code>heads</code> with value "1". By default, the first + video device in domain xml is the primary one, but the optional + attribute <code>primary</code> with value 'yes' can be used to mark the + primary in cases of mutiple video device. The non-primary must be + type of "qxl".
Mention that the 'primary' attribute is since 1.0.2 (unless someone else can argue that this is still worth including in 1.0.1). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
v2: check qemu version, it has to be qemu >= 1.2 for using new video device with -device qemu option This patch set aims to use qemu VGA device with -device option to replace -vga option. The mapping is as follows. '-device VGA' maps to '-vga std' '-device cirrus-vga' maps to '-vga cirrus' '-device qxl-vga' maps to '-vga qxl' (there is also '-device qxl' for secondary devices) '-device vmware-svga' maps to '-vga vmware' Through this change, guests will be able to use any available PCI slot number rather than fixed 0x2 slot for primary video device. Guannan Ren(4) [PATCH v2 1/4] qemu: add qemu various VGA devices Caps flag [PATCH v2 2/4] conf: add optional attribte primary to video <model> [PATCH v2 3/4] qemu: use newer -device video device in qemu commandline [PATCH v2 4/4] doc: add attribute primary doc to video model element docs/formatdomain.html.in | 6 ++++- src/conf/domain_conf.c | 25 +++++++++++++++++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_capabilities.c | 10 ++++++++- src/qemu/qemu_capabilities.h | 4 ++++ src/qemu/qemu_command.c | 132 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------- src/qemu/qemu_command.h | 4 +++- tests/qemuhelptest.c | 48 +++++++++++++++++++++++++++++++-------- tests/qemuxml2argvtest.c | 9 +++++--- 9 files changed, 193 insertions(+), 46 deletions(-)
On 12/11/2012 07:14 AM, Guannan Ren wrote:
QEMU_CAPS_DEVICE_QXL -device qxl QEMU_CAPS_DEVICE_VGA -device VGA QEMU_CAPS_DEVICE_CIRRUS_VGA -device cirrus-vga QEMU_CAPS_DEVICE_VMWARE_SVGA -device vmware-svga
Fix a typo in qemuCapsObjectTypes, the string 'qxl' here should be -device qxl rather than -vga [...|qxl|..] --- src/qemu/qemu_capabilities.c | 10 ++++++++- src/qemu/qemu_capabilities.h | 4 ++++ tests/qemuhelptest.c | 48 +++++++++++++++++++++++++++++++++++--------- 3 files changed, 52 insertions(+), 10 deletions(-)
@@ -477,7 +481,6 @@ mymain(void) QEMU_CAPS_NESTING, QEMU_CAPS_NAME_PROCESS, QEMU_CAPS_SMBIOS_TYPE, - QEMU_CAPS_VGA_QXL, QEMU_CAPS_SPICE, QEMU_CAPS_VGA_NONE, QEMU_CAPS_MIGRATE_QEMU_FD,
I'm slightly worried about this one - if someone is running this version of qemu, and then upgrades and restarts libvirtd, the fact that the capability disappears may make the running guest disappear from the new libvirtd. But I don't have any suggestions on how to make it better; I guess we'll just have to wait and see if we have any bug reports. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (4)
-
Eric Blake -
Gerd Hoffmann -
Guannan Ren -
Laine Stump