[libvirt] [PATCH 0/8] qemu: Default to video type=virtio for machvirt
This series aim is to change the qemu machvirt video type default to virtio, but rather than continue to hack things into place in domain_conf.c, this rearranges things to allow drivers to set a video type default. Patches 1-4 are small cleanups/improvements in this area Patches 5-7 are the plumbing to allow drivers to set their own default Patch 8 is the actual default change https://bugzilla.redhat.com/show_bug.cgi?id=1404112 Cole Robinson (8): qemu: parse: drop redundant video config qemu: domain: Move some validation out of DeviceDefPostParse qemu: annotate some VIDEO_TYPE enum switch conf: add virDomainVideoDefNew conf: domain: add VIDEO_TYPE_DEFAULT conf: domain: move video type validation to DeviceDefValidate qemu: Set default video type in qemu PostParse qemu: Default to video type=virtio for machvirt src/conf/domain_conf.c | 54 +++++++++++------- src/conf/domain_conf.h | 2 + src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 7 +-- src/qemu/qemu_domain.c | 66 ++++++++++++++-------- src/qemu/qemu_domain_address.c | 1 + src/qemu/qemu_monitor_json.c | 16 ++---- src/qemu/qemu_parse_command.c | 14 +---- src/qemu/qemu_process.c | 7 +-- src/vz/vz_sdk.c | 3 +- tests/domaincapsschemadata/full.xml | 1 + .../qemuxml2argv-aarch64-video-default.args | 24 ++++++++ .../qemuxml2argv-aarch64-video-default.xml | 17 ++++++ tests/qemuxml2argvtest.c | 6 ++ .../qemuxml2xmlout-aarch64-video-default.xml | 46 +++++++++++++++ tests/qemuxml2xmltest.c | 6 ++ 16 files changed, 191 insertions(+), 80 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-aarch64-video-default.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-aarch64-video-default.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-aarch64-video-default.xml -- 2.13.0
The ram/vram = 0 bits aren't needed, and PostParse will fill in the needed QXL default Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_command.c | 4 ---- src/qemu/qemu_parse_command.c | 11 +---------- 2 files changed, 1 insertion(+), 14 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c53ab97b9..ebf9c63bc 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7264,10 +7264,6 @@ qemuBuildObsoleteAccelArg(virCommandPtr cmd, } break; - case VIR_DOMAIN_VIRT_XEN: - /* XXX better check for xenner */ - break; - default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("the QEMU binary does not support %s"), diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c index af9063c02..60c81f0ca 100644 --- a/src/qemu/qemu_parse_command.c +++ b/src/qemu/qemu_parse_command.c @@ -2608,16 +2608,7 @@ qemuParseCommandLine(virCapsPtr caps, virDomainVideoDefPtr vid; if (VIR_ALLOC(vid) < 0) goto error; - if (def->virtType == VIR_DOMAIN_VIRT_XEN) - vid->type = VIR_DOMAIN_VIDEO_TYPE_XEN; - else - vid->type = video; - if (vid->type == VIR_DOMAIN_VIDEO_TYPE_QXL) { - vid->vgamem = QEMU_QXL_VGAMEM_DEFAULT; - } else { - vid->ram = 0; - vid->vgamem = 0; - } + vid->type = video; vid->heads = 1; if (VIR_APPEND_ELEMENT(def->videos, def->nvideos, vid) < 0) { -- 2.13.0
On 06/28/2017 02:49 PM, Cole Robinson wrote:
The ram/vram = 0 bits aren't needed, and PostParse will fill in the needed QXL default
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_command.c | 4 ---- src/qemu/qemu_parse_command.c | 11 +---------- 2 files changed, 1 insertion(+), 14 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c53ab97b9..ebf9c63bc 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7264,10 +7264,6 @@ qemuBuildObsoleteAccelArg(virCommandPtr cmd, } break;
- case VIR_DOMAIN_VIRT_XEN: - /* XXX better check for xenner */ - break; - default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("the QEMU binary does not support %s"), diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c index af9063c02..60c81f0ca 100644 --- a/src/qemu/qemu_parse_command.c +++ b/src/qemu/qemu_parse_command.c @@ -2608,16 +2608,7 @@ qemuParseCommandLine(virCapsPtr caps, virDomainVideoDefPtr vid; if (VIR_ALLOC(vid) < 0) goto error; - if (def->virtType == VIR_DOMAIN_VIRT_XEN) - vid->type = VIR_DOMAIN_VIDEO_TYPE_XEN; - else - vid->type = video; - if (vid->type == VIR_DOMAIN_VIDEO_TYPE_QXL) { - vid->vgamem = QEMU_QXL_VGAMEM_DEFAULT; - } else { - vid->ram = 0; - vid->vgamem = 0; - } + vid->type = video; vid->heads = 1;
if (VIR_APPEND_ELEMENT(def->videos, def->nvideos, vid) < 0) {
Now that I look closer at this patch, I realize I accidentally squashed together two changes: the XEN removal was supposed to be its own patch. I'll respin after review comes in - Cole
On 06/28/2017 02:49 PM, Cole Robinson wrote:
The ram/vram = 0 bits aren't needed, and PostParse will fill in the needed QXL default
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_command.c | 4 ---- src/qemu/qemu_parse_command.c | 11 +---------- 2 files changed, 1 insertion(+), 14 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c53ab97b9..ebf9c63bc 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7264,10 +7264,6 @@ qemuBuildObsoleteAccelArg(virCommandPtr cmd, } break;
- case VIR_DOMAIN_VIRT_XEN: - /* XXX better check for xenner */ - break; - default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("the QEMU binary does not support %s"), diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c index af9063c02..60c81f0ca 100644 --- a/src/qemu/qemu_parse_command.c +++ b/src/qemu/qemu_parse_command.c @@ -2608,16 +2608,7 @@ qemuParseCommandLine(virCapsPtr caps, virDomainVideoDefPtr vid; if (VIR_ALLOC(vid) < 0) goto error; - if (def->virtType == VIR_DOMAIN_VIRT_XEN) - vid->type = VIR_DOMAIN_VIDEO_TYPE_XEN;
Did your follow-up comment mean to imply this would be done separately too or just the qemu_command.c change? I guess I see video default'ing to VIR_DOMAIN_VIDEO_TYPE_CIRRUS and changing based on the presence of various qualifiers, but this seems to force a specific type based on virtType for virtType == VIRT_XEN that would seemingly be lost. What is actually on the command line I didn't chase down... Still the qemu_parse_command code isn't exactly highly used and I think as long as it can be explained why there's not need to special case VIRT_XEN any more perhaps in the commit message, then you've got my: Reviewed-by: John Ferlan <jferlan@redhat.com> John
- else - vid->type = video; - if (vid->type == VIR_DOMAIN_VIDEO_TYPE_QXL) { - vid->vgamem = QEMU_QXL_VGAMEM_DEFAULT; - } else { - vid->ram = 0; - vid->vgamem = 0; - } + vid->type = video; vid->heads = 1;
if (VIR_APPEND_ELEMENT(def->videos, def->nvideos, vid) < 0) {
On 07/18/2017 06:20 PM, John Ferlan wrote:
On 06/28/2017 02:49 PM, Cole Robinson wrote:
The ram/vram = 0 bits aren't needed, and PostParse will fill in the needed QXL default
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_command.c | 4 ---- src/qemu/qemu_parse_command.c | 11 +---------- 2 files changed, 1 insertion(+), 14 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c53ab97b9..ebf9c63bc 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7264,10 +7264,6 @@ qemuBuildObsoleteAccelArg(virCommandPtr cmd, } break;
- case VIR_DOMAIN_VIRT_XEN: - /* XXX better check for xenner */ - break; - default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("the QEMU binary does not support %s"), diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c index af9063c02..60c81f0ca 100644 --- a/src/qemu/qemu_parse_command.c +++ b/src/qemu/qemu_parse_command.c @@ -2608,16 +2608,7 @@ qemuParseCommandLine(virCapsPtr caps, virDomainVideoDefPtr vid; if (VIR_ALLOC(vid) < 0) goto error; - if (def->virtType == VIR_DOMAIN_VIRT_XEN) - vid->type = VIR_DOMAIN_VIDEO_TYPE_XEN;
Did your follow-up comment mean to imply this would be done separately too or just the qemu_command.c change?
I guess I see video default'ing to VIR_DOMAIN_VIDEO_TYPE_CIRRUS and changing based on the presence of various qualifiers, but this seems to force a specific type based on virtType for virtType == VIRT_XEN that would seemingly be lost. What is actually on the command line I didn't chase down...
Still the qemu_parse_command code isn't exactly highly used and I think as long as it can be explained why there's not need to special case VIRT_XEN any more perhaps in the commit message, then you've got my:
Reviewed-by: John Ferlan <jferlan@redhat.com>
Thanks for the review and sorry for the delayed response. I split out the xen changes into their own commit with this message: qemu: Remove remnants of xenner support Both of these are dead code: qemu_command.c explicitly rejects VIRT_XEN earlier in the call chain, and qemu_parse_command.c will never set VIRT_XEN anymore I pushed it along with patch 1, 2, and 4, since patch 3 is independent and has some comments Thanks, Cole
And into DeviceDefValidate which is the expected place Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_domain.c | 55 +++++++++++++++++++++++++++----------------------- 1 file changed, 30 insertions(+), 25 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 8e7404da6..90f489840 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3191,6 +3191,33 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, } } + /* forbid capabilities mode hostdev in this kind of hypervisor */ + if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV && + dev->data.hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("hostdev mode 'capabilities' is not " + "supported in %s"), + virDomainVirtTypeToString(def->virtType)); + goto cleanup; + } + + if (dev->type == VIR_DOMAIN_DEVICE_VIDEO) { + if (dev->data.video->type == VIR_DOMAIN_VIDEO_TYPE_QXL && + dev->data.video->vgamem) { + if (dev->data.video->vgamem < 1024) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("value for 'vgamem' must be at least 1 MiB " + "(1024 KiB)")); + goto cleanup; + } + if (dev->data.video->vgamem != VIR_ROUND_UP_POWER_OF_TWO(dev->data.video->vgamem)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("value for 'vgamem' must be power of two")); + goto cleanup; + } + } + } + ret = 0; cleanup: virObjectUnref(cfg); @@ -3551,31 +3578,9 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, goto cleanup; } - /* forbid capabilities mode hostdev in this kind of hypervisor */ - if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV && - dev->data.hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("hostdev mode 'capabilities' is not " - "supported in %s"), - virDomainVirtTypeToString(def->virtType)); - goto cleanup; - } - - if (dev->type == VIR_DOMAIN_DEVICE_VIDEO && - dev->data.video->type == VIR_DOMAIN_VIDEO_TYPE_QXL) { - if (dev->data.video->vgamem) { - if (dev->data.video->vgamem < 1024) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("value for 'vgamem' must be at least 1 MiB " - "(1024 KiB)")); - goto cleanup; - } - if (dev->data.video->vgamem != VIR_ROUND_UP_POWER_OF_TWO(dev->data.video->vgamem)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("value for 'vgamem' must be power of two")); - goto cleanup; - } - } else { + if (dev->type == VIR_DOMAIN_DEVICE_VIDEO) { + if (dev->data.video->type == VIR_DOMAIN_VIDEO_TYPE_QXL && + !dev->data.video->vgamem) { dev->data.video->vgamem = QEMU_QXL_VGAMEM_DEFAULT; } } -- 2.13.0
On 06/28/2017 02:49 PM, Cole Robinson wrote:
And into DeviceDefValidate which is the expected place
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_domain.c | 55 +++++++++++++++++++++++++++----------------------- 1 file changed, 30 insertions(+), 25 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
For the ram/vram monitor wrappers, just add a default: clause... seems like it should be rarely extended so this saves every committer from needing to update For the validation switch, fill in the missing values Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_domain.c | 3 ++- src/qemu/qemu_monitor_json.c | 16 ++++------------ src/qemu/qemu_process.c | 7 ++----- 3 files changed, 8 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 90f489840..ac1bc1a1e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2950,10 +2950,11 @@ qemuDomainDefValidateVideo(const virDomainDef *def) for (i = 0; i < def->nvideos; i++) { video = def->videos[i]; - switch (video->type) { + switch ((virDomainVideoType) video->type) { case VIR_DOMAIN_VIDEO_TYPE_XEN: case VIR_DOMAIN_VIDEO_TYPE_VBOX: case VIR_DOMAIN_VIDEO_TYPE_PARALLELS: + case VIR_DOMAIN_VIDEO_TYPE_GOP: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("video type '%s' is not supported with QEMU"), virDomainVideoTypeToString(video->type)); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 5ddc09ca6..2afc03329 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1565,7 +1565,7 @@ qemuMonitorJSONUpdateVideoMemorySize(qemuMonitorPtr mon, {0} }; - switch (video->type) { + switch ((virDomainVideoType) video->type) { case VIR_DOMAIN_VIDEO_TYPE_VGA: if (qemuMonitorJSONGetObjectProperty(mon, path, "vgamem_mb", &prop) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -1608,10 +1608,7 @@ qemuMonitorJSONUpdateVideoMemorySize(qemuMonitorPtr mon, } video->vram = prop.val.ul * 1024; break; - case VIR_DOMAIN_VIDEO_TYPE_CIRRUS: - case VIR_DOMAIN_VIDEO_TYPE_XEN: - case VIR_DOMAIN_VIDEO_TYPE_VBOX: - case VIR_DOMAIN_VIDEO_TYPE_LAST: + default: break; } @@ -1635,7 +1632,7 @@ qemuMonitorJSONUpdateVideoVram64Size(qemuMonitorPtr mon, {0} }; - switch (video->type) { + switch ((virDomainVideoType) video->type) { case VIR_DOMAIN_VIDEO_TYPE_QXL: if (video->vram64 != 0) { if (qemuMonitorJSONGetObjectProperty(mon, path, @@ -1648,12 +1645,7 @@ qemuMonitorJSONUpdateVideoVram64Size(qemuMonitorPtr mon, video->vram64 = prop.val.ul * 1024; } break; - case VIR_DOMAIN_VIDEO_TYPE_VGA: - case VIR_DOMAIN_VIDEO_TYPE_VMVGA: - case VIR_DOMAIN_VIDEO_TYPE_CIRRUS: - case VIR_DOMAIN_VIDEO_TYPE_XEN: - case VIR_DOMAIN_VIDEO_TYPE_VBOX: - case VIR_DOMAIN_VIDEO_TYPE_LAST: + default: break; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index d669dfb32..fb6e2c82b 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2605,7 +2605,7 @@ qemuProcessUpdateVideoRamSize(virQEMUDriverPtr driver, for (i = 0; i < vm->def->nvideos; i++) { video = vm->def->videos[i]; - switch (video->type) { + switch ((virDomainVideoType) video->type) { case VIR_DOMAIN_VIDEO_TYPE_VGA: if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VGA_VGAMEM)) { if (qemuMonitorUpdateVideoMemorySize(priv->mon, video, "VGA") < 0) @@ -2642,10 +2642,7 @@ qemuProcessUpdateVideoRamSize(virQEMUDriverPtr driver, goto error; } break; - case VIR_DOMAIN_VIDEO_TYPE_CIRRUS: - case VIR_DOMAIN_VIDEO_TYPE_XEN: - case VIR_DOMAIN_VIDEO_TYPE_VBOX: - case VIR_DOMAIN_VIDEO_TYPE_LAST: + default: break; } -- 2.13.0
On 06/28/2017 02:49 PM, Cole Robinson wrote:
For the ram/vram monitor wrappers, just add a default: clause... seems like it should be rarely extended so this saves every committer from needing to update
For the validation switch, fill in the missing values
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_domain.c | 3 ++- src/qemu/qemu_monitor_json.c | 16 ++++------------ src/qemu/qemu_process.c | 7 ++----- 3 files changed, 8 insertions(+), 18 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 90f489840..ac1bc1a1e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2950,10 +2950,11 @@ qemuDomainDefValidateVideo(const virDomainDef *def) for (i = 0; i < def->nvideos; i++) { video = def->videos[i];
- switch (video->type) { + switch ((virDomainVideoType) video->type) {
My recollection is when this is typecast or the @type was typed as the enum, then the switch needed every case of the enum to be listed. Whereas, when the @type was an int, then using 'default:' was possible if one didn't want to provide every possible combination. Still, I believe more recent changes have always favored the list every possible case, even if they do nothing rather than using default: Is there any special reason to not list every case option? If not, I'd prefer _virDomainVideoDef change @type from int to virDomainVideoType if only to avoid this particular type situation in the future. We may not add new types that often, but as shown by this particular switch, GOP (added in 3.2) wasn't added into the list and the default here is not an error. I'd almost say in this particular switch/case that this one change could be a separate patch (e.g. bug fix). If it's that rare, then it shouldn't cause too much hassle for new video types, but does force future changes to consider all the places where adding a new video type that need to change. I'd rather see future adjustment be forced to decide where something applies or does not apply. However, I'm willing to be convinced otherwise... John
case VIR_DOMAIN_VIDEO_TYPE_XEN: case VIR_DOMAIN_VIDEO_TYPE_VBOX: case VIR_DOMAIN_VIDEO_TYPE_PARALLELS: + case VIR_DOMAIN_VIDEO_TYPE_GOP: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("video type '%s' is not supported with QEMU"), virDomainVideoTypeToString(video->type)); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 5ddc09ca6..2afc03329 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1565,7 +1565,7 @@ qemuMonitorJSONUpdateVideoMemorySize(qemuMonitorPtr mon, {0} };
- switch (video->type) { + switch ((virDomainVideoType) video->type) { case VIR_DOMAIN_VIDEO_TYPE_VGA: if (qemuMonitorJSONGetObjectProperty(mon, path, "vgamem_mb", &prop) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -1608,10 +1608,7 @@ qemuMonitorJSONUpdateVideoMemorySize(qemuMonitorPtr mon, } video->vram = prop.val.ul * 1024; break; - case VIR_DOMAIN_VIDEO_TYPE_CIRRUS: - case VIR_DOMAIN_VIDEO_TYPE_XEN: - case VIR_DOMAIN_VIDEO_TYPE_VBOX: - case VIR_DOMAIN_VIDEO_TYPE_LAST: + default: break; }
@@ -1635,7 +1632,7 @@ qemuMonitorJSONUpdateVideoVram64Size(qemuMonitorPtr mon, {0} };
- switch (video->type) { + switch ((virDomainVideoType) video->type) { case VIR_DOMAIN_VIDEO_TYPE_QXL: if (video->vram64 != 0) { if (qemuMonitorJSONGetObjectProperty(mon, path, @@ -1648,12 +1645,7 @@ qemuMonitorJSONUpdateVideoVram64Size(qemuMonitorPtr mon, video->vram64 = prop.val.ul * 1024; } break; - case VIR_DOMAIN_VIDEO_TYPE_VGA: - case VIR_DOMAIN_VIDEO_TYPE_VMVGA: - case VIR_DOMAIN_VIDEO_TYPE_CIRRUS: - case VIR_DOMAIN_VIDEO_TYPE_XEN: - case VIR_DOMAIN_VIDEO_TYPE_VBOX: - case VIR_DOMAIN_VIDEO_TYPE_LAST: + default: break; }
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index d669dfb32..fb6e2c82b 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2605,7 +2605,7 @@ qemuProcessUpdateVideoRamSize(virQEMUDriverPtr driver, for (i = 0; i < vm->def->nvideos; i++) { video = vm->def->videos[i];
- switch (video->type) { + switch ((virDomainVideoType) video->type) { case VIR_DOMAIN_VIDEO_TYPE_VGA: if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VGA_VGAMEM)) { if (qemuMonitorUpdateVideoMemorySize(priv->mon, video, "VGA") < 0) @@ -2642,10 +2642,7 @@ qemuProcessUpdateVideoRamSize(virQEMUDriverPtr driver, goto error; } break; - case VIR_DOMAIN_VIDEO_TYPE_CIRRUS: - case VIR_DOMAIN_VIDEO_TYPE_XEN: - case VIR_DOMAIN_VIDEO_TYPE_VBOX: - case VIR_DOMAIN_VIDEO_TYPE_LAST: + default: break; }
On 07/18/2017 06:24 PM, John Ferlan wrote:
On 06/28/2017 02:49 PM, Cole Robinson wrote:
For the ram/vram monitor wrappers, just add a default: clause... seems like it should be rarely extended so this saves every committer from needing to update
For the validation switch, fill in the missing values
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_domain.c | 3 ++- src/qemu/qemu_monitor_json.c | 16 ++++------------ src/qemu/qemu_process.c | 7 ++----- 3 files changed, 8 insertions(+), 18 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 90f489840..ac1bc1a1e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2950,10 +2950,11 @@ qemuDomainDefValidateVideo(const virDomainDef *def) for (i = 0; i < def->nvideos; i++) { video = def->videos[i];
- switch (video->type) { + switch ((virDomainVideoType) video->type) {
My recollection is when this is typecast or the @type was typed as the enum, then the switch needed every case of the enum to be listed.
Whereas, when the @type was an int, then using 'default:' was possible if one didn't want to provide every possible combination.
It seems to work a bit differently: - If @type is int, no checking is done. - If @type is explicit, gcc checks that either all values are listed, or a default: clause is present
Still, I believe more recent changes have always favored the list every possible case, even if they do nothing rather than using default:
Is there any special reason to not list every case option? If not, I'd prefer _virDomainVideoDef change @type from int to virDomainVideoType if only to avoid this particular type situation in the future.
That said I think it is beneficial to make the VideoDef change and adjust all the users to add an explicit 'default:' if it makes sense. There aren't many cases I can think of outside generic domain_conf code where explicitly listing every VIDEO_TYPE makes sense IMO. There's a bunch of similar cases like that in qemu_domain_address.c but I wonder if that is actually preventing bugs from being added or just saving developers a few minutes hunting through the code... Anyways I'll side step this discussion in my v2 by converting the qemu validation switch to a whitelist approach which makes more sense anyways, and just skipping the (virDomainVideoType) annotation Thanks, Cole
To handle setting a default heads value. Convert callers that were doing it by hand Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_conf.c | 21 ++++++++++++++++----- src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 1 + src/qemu/qemu_parse_command.c | 3 +-- src/vz/vz_sdk.c | 3 +-- 5 files changed, 20 insertions(+), 9 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c3149f976..47b668dc1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2351,6 +2351,20 @@ void virDomainShmemDefFree(virDomainShmemDefPtr def) VIR_FREE(def); } + +virDomainVideoDefPtr +virDomainVideoDefNew(void) +{ + virDomainVideoDefPtr def; + + if (VIR_ALLOC(def) < 0) + return NULL; + + def->heads = 1; + return def; +} + + void virDomainVideoDefFree(virDomainVideoDefPtr def) { if (!def) @@ -13660,7 +13674,7 @@ virDomainVideoDefParseXML(xmlNodePtr node, ctxt->node = node; - if (VIR_ALLOC(def) < 0) + if (!(def = virDomainVideoDefNew())) return NULL; cur = node->children; @@ -13754,8 +13768,6 @@ virDomainVideoDefParseXML(xmlNodePtr node, _("cannot parse video heads '%s'"), heads); goto error; } - } else { - def->heads = 1; } if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) @@ -20944,7 +20956,7 @@ virDomainDefAddImplicitVideo(virDomainDefPtr def) if (def->ngraphics == 0 || def->nvideos > 0) return 0; - if (VIR_ALLOC(video) < 0) + if (!(video = virDomainVideoDefNew())) goto cleanup; video->type = virDomainVideoDefaultType(def); if (video->type < 0) { @@ -20952,7 +20964,6 @@ virDomainDefAddImplicitVideo(virDomainDefPtr def) _("cannot determine default video type")); goto cleanup; } - video->heads = 1; if (VIR_APPEND_ELEMENT(def->videos, def->nvideos, video) < 0) goto cleanup; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 964bc02f9..db89ffa97 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2683,6 +2683,7 @@ void virDomainSoundDefFree(virDomainSoundDefPtr def); void virDomainMemballoonDefFree(virDomainMemballoonDefPtr def); void virDomainNVRAMDefFree(virDomainNVRAMDefPtr def); void virDomainWatchdogDefFree(virDomainWatchdogDefPtr def); +virDomainVideoDefPtr virDomainVideoDefNew(void); void virDomainVideoDefFree(virDomainVideoDefPtr def); virDomainHostdevDefPtr virDomainHostdevDefAlloc(virDomainXMLOptionPtr xmlopt); void virDomainHostdevDefClear(virDomainHostdevDefPtr def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 888412ac7..248237c4a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -528,6 +528,7 @@ virDomainUSBDeviceDefForeach; virDomainVideoDefaultRAM; virDomainVideoDefaultType; virDomainVideoDefFree; +virDomainVideoDefNew; virDomainVideoTypeFromString; virDomainVideoTypeToString; virDomainVideoVGAConfTypeFromString; diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c index 60c81f0ca..6751868a6 100644 --- a/src/qemu/qemu_parse_command.c +++ b/src/qemu/qemu_parse_command.c @@ -2606,10 +2606,9 @@ qemuParseCommandLine(virCapsPtr caps, if (def->ngraphics) { virDomainVideoDefPtr vid; - if (VIR_ALLOC(vid) < 0) + if (!(vid = virDomainVideoDefNew())) goto error; vid->type = video; - vid->heads = 1; if (VIR_APPEND_ELEMENT(def->videos, def->nvideos, vid) < 0) { virDomainVideoDefFree(vid); diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index a62b31079..950eeaa34 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -552,12 +552,11 @@ prlsdkAddDomainVideoInfoCt(virDomainDefPtr def) if (def->ngraphics == 0) return 0; - if (VIR_ALLOC(video) < 0) + if (!(video = virDomainVideoDefNew())) goto cleanup; video->type = VIR_DOMAIN_VIDEO_TYPE_PARALLELS; video->vram = 0; - video->heads = 1; if (VIR_APPEND_ELEMENT(def->videos, def->nvideos, video) < 0) goto cleanup; -- 2.13.0
On 06/28/2017 02:49 PM, Cole Robinson wrote:
To handle setting a default heads value. Convert callers that were doing it by hand
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_conf.c | 21 ++++++++++++++++----- src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 1 + src/qemu/qemu_parse_command.c | 3 +-- src/vz/vz_sdk.c | 3 +-- 5 files changed, 20 insertions(+), 9 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Will be needed for future patches to pull the default video type setting out of XML parsing routines. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_conf.c | 1 + src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 3 +++ src/qemu/qemu_domain.c | 1 + src/qemu/qemu_domain_address.c | 1 + tests/domaincapsschemadata/full.xml | 1 + 6 files changed, 8 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 47b668dc1..984d15abf 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -550,6 +550,7 @@ VIR_ENUM_IMPL(virDomainPanicModel, VIR_DOMAIN_PANIC_MODEL_LAST, "s390") VIR_ENUM_IMPL(virDomainVideo, VIR_DOMAIN_VIDEO_TYPE_LAST, + "default", "vga", "cirrus", "vmvga", diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index db89ffa97..dc1516b91 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1345,6 +1345,7 @@ struct _virDomainWatchdogDef { typedef enum { + VIR_DOMAIN_VIDEO_TYPE_DEFAULT, VIR_DOMAIN_VIDEO_TYPE_VGA, VIR_DOMAIN_VIDEO_TYPE_CIRRUS, VIR_DOMAIN_VIDEO_TYPE_VMVGA, diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ebf9c63bc..cf5e9092b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -96,6 +96,7 @@ VIR_ENUM_IMPL(qemuDiskCacheV2, VIR_DOMAIN_DISK_CACHE_LAST, "unsafe"); VIR_ENUM_IMPL(qemuVideo, VIR_DOMAIN_VIDEO_TYPE_LAST, + "", /* default value, we shouldn't see this */ "std", "cirrus", "vmware", @@ -109,6 +110,7 @@ VIR_ENUM_IMPL(qemuVideo, VIR_DOMAIN_VIDEO_TYPE_LAST, VIR_ENUM_DECL(qemuDeviceVideo) VIR_ENUM_IMPL(qemuDeviceVideo, VIR_DOMAIN_VIDEO_TYPE_LAST, + "", /* default value, we shouldn't see this */ "VGA", "cirrus-vga", "vmware-svga", @@ -122,6 +124,7 @@ VIR_ENUM_IMPL(qemuDeviceVideo, VIR_DOMAIN_VIDEO_TYPE_LAST, VIR_ENUM_DECL(qemuDeviceVideoSecondary) VIR_ENUM_IMPL(qemuDeviceVideoSecondary, VIR_DOMAIN_VIDEO_TYPE_LAST, + "", /* default value, we shouldn't see this */ "", /* no secondary device for VGA */ "", /* no secondary device for cirrus-vga */ "", /* no secondary device for vmware-svga */ diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ac1bc1a1e..387dade8f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2955,6 +2955,7 @@ qemuDomainDefValidateVideo(const virDomainDef *def) case VIR_DOMAIN_VIDEO_TYPE_VBOX: case VIR_DOMAIN_VIDEO_TYPE_PARALLELS: case VIR_DOMAIN_VIDEO_TYPE_GOP: + case VIR_DOMAIN_VIDEO_TYPE_DEFAULT: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("video type '%s' is not supported with QEMU"), virDomainVideoTypeToString(video->type)); diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index b5b863fe4..8f19ab824 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -752,6 +752,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev, case VIR_DOMAIN_VIDEO_TYPE_PARALLELS: return pciFlags; + case VIR_DOMAIN_VIDEO_TYPE_DEFAULT: case VIR_DOMAIN_VIDEO_TYPE_GOP: case VIR_DOMAIN_VIDEO_TYPE_LAST: return 0; diff --git a/tests/domaincapsschemadata/full.xml b/tests/domaincapsschemadata/full.xml index 82a92322e..ab6ef9f2e 100644 --- a/tests/domaincapsschemadata/full.xml +++ b/tests/domaincapsschemadata/full.xml @@ -62,6 +62,7 @@ </graphics> <video supported='yes'> <enum name='modelType'> + <value>default</value> <value>vga</value> <value>cirrus</value> <value>vmvga</value> -- 2.13.0
On 06/28/2017 02:49 PM, Cole Robinson wrote:
Will be needed for future patches to pull the default video type setting out of XML parsing routines.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_conf.c | 1 + src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 3 +++ src/qemu/qemu_domain.c | 1 + src/qemu/qemu_domain_address.c | 1 + tests/domaincapsschemadata/full.xml | 1 + 6 files changed, 8 insertions(+)
Well, I see why you added patch 3 now... But I'm still convinced it's better to have all switch/cases handle every case. My only concern here would be related to migration, but since it doesn't appear that "default" can be saved to the XML file I think we're good. I will guarantee someone in qe will find this default and find some oddball path that tries to allow it to be defined in some read XML file. Reviewed-by: John Ferlan <jferlan@redhat.com> John
This allows drivers to set their own default. But if a driver neglects to fill one in, we still error like we previously would at parse time. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_conf.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 984d15abf..bb61f4091 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5075,6 +5075,18 @@ virDomainControllerDefValidate(const virDomainControllerDef *controller) static int +virDomainVideoDefValidate(const virDomainVideoDef *video) +{ + if (video->type == VIR_DOMAIN_VIDEO_TYPE_DEFAULT) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing video model and cannot determine default")); + return -1; + } + return 0; +} + + +static int virDomainDeviceDefValidateInternal(const virDomainDeviceDef *dev, const virDomainDef *def) { @@ -5091,11 +5103,13 @@ virDomainDeviceDefValidateInternal(const virDomainDeviceDef *dev, case VIR_DOMAIN_DEVICE_CONTROLLER: return virDomainControllerDefValidate(dev->data.controller); + case VIR_DOMAIN_DEVICE_VIDEO: + return virDomainVideoDefValidate(dev->data.video); + case VIR_DOMAIN_DEVICE_LEASE: case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_INPUT: case VIR_DOMAIN_DEVICE_SOUND: - case VIR_DOMAIN_DEVICE_VIDEO: case VIR_DOMAIN_DEVICE_HOSTDEV: case VIR_DOMAIN_DEVICE_WATCHDOG: case VIR_DOMAIN_DEVICE_GRAPHICS: @@ -13564,7 +13578,7 @@ virDomainVideoDefaultType(const virDomainDef *def) case VIR_DOMAIN_VIRT_BHYVE: return VIR_DOMAIN_VIDEO_TYPE_GOP; default: - return -1; + return VIR_DOMAIN_VIDEO_TYPE_DEFAULT; } } @@ -13709,11 +13723,7 @@ virDomainVideoDefParseXML(xmlNodePtr node, goto error; } } else { - if ((def->type = virDomainVideoDefaultType(dom)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("missing video model and cannot determine default")); - goto error; - } + def->type = virDomainVideoDefaultType(dom); } if (ram) { @@ -20960,11 +20970,6 @@ virDomainDefAddImplicitVideo(virDomainDefPtr def) if (!(video = virDomainVideoDefNew())) goto cleanup; video->type = virDomainVideoDefaultType(def); - if (video->type < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot determine default video type")); - goto cleanup; - } if (VIR_APPEND_ELEMENT(def->videos, def->nvideos, video) < 0) goto cleanup; -- 2.13.0
On 06/28/2017 02:49 PM, Cole Robinson wrote:
This allows drivers to set their own default. But if a driver neglects to fill one in, we still error like we previously would at parse time.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_conf.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
And not generic domain_conf code. We will need qemu private functions in a bit. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_conf.c | 3 --- src/qemu/qemu_domain.c | 7 +++++++ 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bb61f4091..1fe620dfb 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13551,9 +13551,6 @@ virDomainVideoDefaultType(const virDomainDef *def) { switch (def->virtType) { case VIR_DOMAIN_VIRT_TEST: - case VIR_DOMAIN_VIRT_QEMU: - case VIR_DOMAIN_VIRT_KQEMU: - case VIR_DOMAIN_VIRT_KVM: case VIR_DOMAIN_VIRT_XEN: if (def->os.type == VIR_DOMAIN_OSTYPE_XEN || def->os.type == VIR_DOMAIN_OSTYPE_LINUX) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 387dade8f..b41b3c5cc 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3581,6 +3581,13 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, } if (dev->type == VIR_DOMAIN_DEVICE_VIDEO) { + if (dev->data.video->type == VIR_DOMAIN_VIDEO_TYPE_DEFAULT) { + if ARCH_IS_PPC64(def->os.arch) + dev->data.video->type = VIR_DOMAIN_VIDEO_TYPE_VGA; + else + dev->data.video->type = VIR_DOMAIN_VIDEO_TYPE_CIRRUS; + } + if (dev->data.video->type == VIR_DOMAIN_VIDEO_TYPE_QXL && !dev->data.video->vgamem) { dev->data.video->vgamem = QEMU_QXL_VGAMEM_DEFAULT; -- 2.13.0
On 06/28/2017 02:49 PM, Cole Robinson wrote:
And not generic domain_conf code. We will need qemu private functions in a bit.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_conf.c | 3 --- src/qemu/qemu_domain.c | 7 +++++++ 2 files changed, 7 insertions(+), 3 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
arm/aarch64 -M virt on KVM doesn't and will never work with standard VGA card emulation. The recommended method is to use type=virtio, so let's make it the default for video devices without an explicit type set by the user https://bugzilla.redhat.com/show_bug.cgi?id=1404112 Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_domain.c | 2 + .../qemuxml2argv-aarch64-video-default.args | 24 +++++++++++ .../qemuxml2argv-aarch64-video-default.xml | 17 ++++++++ tests/qemuxml2argvtest.c | 6 +++ .../qemuxml2xmlout-aarch64-video-default.xml | 46 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 6 +++ 6 files changed, 101 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-aarch64-video-default.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-aarch64-video-default.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-aarch64-video-default.xml diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b41b3c5cc..69bf1cb94 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3584,6 +3584,8 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, if (dev->data.video->type == VIR_DOMAIN_VIDEO_TYPE_DEFAULT) { if ARCH_IS_PPC64(def->os.arch) dev->data.video->type = VIR_DOMAIN_VIDEO_TYPE_VGA; + else if (qemuDomainIsVirt(def)) + dev->data.video->type = VIR_DOMAIN_VIDEO_TYPE_VIRTIO; else dev->data.video->type = VIR_DOMAIN_VIDEO_TYPE_CIRRUS; } diff --git a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-video-default.args b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-video-default.args new file mode 100644 index 000000000..8e56c4250 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-video-default.args @@ -0,0 +1,24 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-aarch64 \ +-name aarch64-vgpu \ +-S \ +-M virt \ +-cpu cortex-a57 \ +-m 1024 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid f3197c89-6457-44fe-b26d-897090ba6541 \ +-nodefconfig \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-aarch64-vgpu/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1 \ +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \ +-device ioh3420,port=0x10,chassis=3,id=pci.3,bus=pcie.0,addr=0x2 \ +-vnc 127.0.0.1:0 \ +-device virtio-gpu-pci,id=video0,bus=pci.2,addr=0x1 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-video-default.xml b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-video-default.xml new file mode 100644 index 000000000..bc4ea48f0 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-video-default.xml @@ -0,0 +1,17 @@ +<domain type='qemu'> + <name>aarch64-vgpu</name> + <uuid>f3197c89-6457-44fe-b26d-897090ba6541</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='aarch64' machine='virt'>hvm</type> + </os> + <cpu mode='custom' match='exact'> + <model fallback='allow'>cortex-a57</model> + </cpu> + <devices> + <emulator>/usr/bin/qemu-system-aarch64</emulator> + <graphics type='vnc'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 27eea70ae..ed7b004cf 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2243,6 +2243,12 @@ mymain(void) QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_IOH3420, QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_DEVICE_VIDEO_PRIMARY, QEMU_CAPS_DEVICE_VIRTIO_GPU, QEMU_CAPS_BOOTINDEX); + DO_TEST("aarch64-video-default", + QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_OBJECT_GPEX, + QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_DEVICE_VIDEO_PRIMARY, + QEMU_CAPS_DEVICE_VIRTIO_GPU, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_VNC); DO_TEST("aarch64-aavmf-virtio-mmio", QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_DTB, QEMU_CAPS_DEVICE_VIRTIO_MMIO, diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-aarch64-video-default.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-aarch64-video-default.xml new file mode 100644 index 000000000..47b46d0d0 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-aarch64-video-default.xml @@ -0,0 +1,46 @@ +<domain type='qemu'> + <name>aarch64-vgpu</name> + <uuid>f3197c89-6457-44fe-b26d-897090ba6541</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='aarch64' machine='virt'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <gic version='2'/> + </features> + <cpu mode='custom' match='exact' check='none'> + <model fallback='allow'>cortex-a57</model> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-aarch64</emulator> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <model name='i82801b11-bridge'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='2'/> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </controller> + <controller type='pci' index='3' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='3' port='0x10'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </controller> + <graphics type='vnc' port='-1' autoport='yes'> + <listen type='address'/> + </graphics> + <video> + <model type='virtio' heads='1' primary='yes'/> + <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/> + </video> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 13072996c..ade8d9300 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -1073,6 +1073,12 @@ mymain(void) QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_IOH3420, QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_DEVICE_VIDEO_PRIMARY, QEMU_CAPS_DEVICE_VIRTIO_GPU, QEMU_CAPS_BOOTINDEX); + DO_TEST("aarch64-video-default", + QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_OBJECT_GPEX, + QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_DEVICE_VIDEO_PRIMARY, + QEMU_CAPS_DEVICE_VIRTIO_GPU, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_VNC); DO_TEST_FULL("aarch64-gic-none", WHEN_BOTH, GIC_NONE, NONE); DO_TEST_FULL("aarch64-gic-none-v2", WHEN_BOTH, GIC_V2, NONE); -- 2.13.0
On 06/28/2017 02:49 PM, Cole Robinson wrote:
arm/aarch64 -M virt on KVM doesn't and will never work with standard VGA card emulation. The recommended method is to use type=virtio, so let's make it the default for video devices without an explicit type set by the user
https://bugzilla.redhat.com/show_bug.cgi?id=1404112 Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_domain.c | 2 + .../qemuxml2argv-aarch64-video-default.args | 24 +++++++++++ .../qemuxml2argv-aarch64-video-default.xml | 17 ++++++++ tests/qemuxml2argvtest.c | 6 +++ .../qemuxml2xmlout-aarch64-video-default.xml | 46 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 6 +++ 6 files changed, 101 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-aarch64-video-default.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-aarch64-video-default.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-aarch64-video-default.xml
I could not git am -3 this one, but it's fairly straightforward. Reviewed-by: John Ferlan <jferlan@redhat.com> John
participants (2)
-
Cole Robinson -
John Ferlan