1:49 p.m.
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
1:49 p.m.
New subject: [libvirt] [PATCH 1/8] qemu: parse: drop redundant video config
The ram/vram = 0 bits aren't needed, and PostParse will fill in the
needed QXL default
Signed-off-by: Cole Robinson <crobinso(a)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
7:03 p.m.
New subject: [libvirt] [PATCH 1/8] qemu: parse: drop redundant video config
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(a)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
5:20 p.m.
New subject: [libvirt] [PATCH 1/8] qemu: parse: drop redundant video config
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(a)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(a)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) {
8:45 a.m.
New subject: [libvirt] [PATCH 1/8] qemu: parse: drop redundant video config
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(a)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(a)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
1:49 p.m.
New subject: [libvirt] [PATCH 2/8] qemu: domain: Move some validation out of DeviceDefPostParse
And into DeviceDefValidate which is the expected place
Signed-off-by: Cole Robinson <crobinso(a)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
5:20 p.m.
New subject: [libvirt] [PATCH 2/8] qemu: domain: Move some validation out of DeviceDefPostParse
On 06/28/2017 02:49 PM, Cole Robinson wrote:
And into DeviceDefValidate which is the expected place
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
src/qemu/qemu_domain.c | 55 +++++++++++++++++++++++++++-----------------------
1 file changed, 30 insertions(+), 25 deletions(-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
1:49 p.m.
New subject: [libvirt] [PATCH 3/8] qemu: annotate some VIDEO_TYPE enum switch
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(a)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
5:24 p.m.
New subject: [libvirt] [PATCH 3/8] qemu: annotate some VIDEO_TYPE enum switch
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(a)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;
}
9:51 a.m.
New subject: [libvirt] [PATCH 3/8] qemu: annotate some VIDEO_TYPE enum switch
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(a)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
1:49 p.m.
New subject: [libvirt] [PATCH 4/8] conf: add virDomainVideoDefNew
To handle setting a default heads value. Convert callers that were
doing it by hand
Signed-off-by: Cole Robinson <crobinso(a)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
5:24 p.m.
New subject: [libvirt] [PATCH 4/8] conf: add virDomainVideoDefNew
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(a)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(a)redhat.com>
John
1:49 p.m.
New subject: [libvirt] [PATCH 5/8] conf: domain: add VIDEO_TYPE_DEFAULT
Will be needed for future patches to pull the default video type
setting out of XML parsing routines.
Signed-off-by: Cole Robinson <crobinso(a)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
5:24 p.m.
New subject: [libvirt] [PATCH 5/8] conf: domain: add VIDEO_TYPE_DEFAULT
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(a)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(a)redhat.com>
John
1:49 p.m.
New subject: [libvirt] [PATCH 6/8] conf: domain: move video type validation to DeviceDefValidate
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(a)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
5:25 p.m.
New subject: [libvirt] [PATCH 6/8] conf: domain: move video type validation to DeviceDefValidate
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(a)redhat.com>
---
src/conf/domain_conf.c | 29 +++++++++++++++++------------
1 file changed, 17 insertions(+), 12 deletions(-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
1:49 p.m.
New subject: [libvirt] [PATCH 7/8] qemu: Set default video type in qemu PostParse
And not generic domain_conf code. We will need qemu private functions
in a bit.
Signed-off-by: Cole Robinson <crobinso(a)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
5:25 p.m.
New subject: [libvirt] [PATCH 7/8] qemu: Set default video type in qemu PostParse
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(a)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(a)redhat.com>
John
1:49 p.m.
New subject: [libvirt] [PATCH 8/8] qemu: Default to video type=virtio for machvirt
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(a)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
5:25 p.m.
New subject: [libvirt] [PATCH 8/8] qemu: Default to video type=virtio for machvirt
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(a)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(a)redhat.com>
John
2457
days inactive
2517
days old
19 comments
2 participants
participants (2)
-
Cole Robinson -
John Ferlan