10:04 a.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 is a small prep change in the qemu code
Patches 2-4 are the plumbing to allow drivers to set their own default
Patch 5 is the actual default change
https://bugzilla.redhat.com/show_bug.cgi?id=1404112
v2:
Rebase
Push a few prep patches
Drop the (virDomainVideoType) annotations
Add patch #1
Cole Robinson (5):
qemu: whitelist valid video types
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 | 32 ++++++++-------
src/conf/domain_conf.h | 1 +
src/qemu/qemu_command.c | 3 ++
src/qemu/qemu_domain.c | 22 +++++++----
src/qemu/qemu_domain_address.c | 1 +
tests/domaincapsschemadata/full.xml | 1 +
.../qemuxml2argv-aarch64-video-default.args | 26 ++++++++++++
.../qemuxml2argv-aarch64-video-default.xml | 17 ++++++++
tests/qemuxml2argvtest.c | 6 +++
.../qemuxml2xmlout-aarch64-video-default.xml | 46 ++++++++++++++++++++++
tests/qemuxml2xmltest.c | 10 +++++
11 files changed, 142 insertions(+), 23 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.5
10:04 a.m.
New subject: [libvirt] [PATCH v2 1/5] qemu: whitelist valid video types
Rather than require an explicit blacklist that needs to be extended
for every new VIDEO_TYPE
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
src/qemu/qemu_domain.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 2c77a6442..1c6b1ba79 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3014,20 +3014,17 @@ qemuDomainDefValidateVideo(const virDomainDef *def)
video = def->videos[i];
switch (video->type) {
- case VIR_DOMAIN_VIDEO_TYPE_XEN:
- case VIR_DOMAIN_VIDEO_TYPE_VBOX:
- case VIR_DOMAIN_VIDEO_TYPE_PARALLELS:
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("video type '%s' is not supported with
QEMU"),
- virDomainVideoTypeToString(video->type));
- return -1;
case VIR_DOMAIN_VIDEO_TYPE_VGA:
case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
case VIR_DOMAIN_VIDEO_TYPE_VMVGA:
case VIR_DOMAIN_VIDEO_TYPE_QXL:
case VIR_DOMAIN_VIDEO_TYPE_VIRTIO:
- case VIR_DOMAIN_VIDEO_TYPE_LAST:
break;
+ default:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("video type '%s' is not supported with
QEMU"),
+ virDomainVideoTypeToString(video->type));
+ return -1;
}
if (!video->primary &&
--
2.13.5
2:56 a.m.
New subject: [libvirt] [PATCH v2 1/5] qemu: whitelist valid video types
On Sun, Aug 27, 2017 at 11:04:38AM -0400, Cole Robinson wrote:
Rather than require an explicit blacklist that needs to be extended
for every new VIDEO_TYPE
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
src/qemu/qemu_domain.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
NACK, we prefer to listing all possible values for typed switch. It
forces a contributor to look at all places where that switch is used
in order to consider whether that place should be updated or not.
Pavel
9:44 a.m.
New subject: [libvirt] [PATCH v2 1/5] qemu: whitelist valid video types
On 08/28/2017 03:56 AM, Pavel Hrdina wrote:
On Sun, Aug 27, 2017 at 11:04:38AM -0400, Cole Robinson wrote:
> Rather than require an explicit blacklist that needs to be extended
> for every new VIDEO_TYPE
>
> Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
> ---
> src/qemu/qemu_domain.c | 13 +++++--------
> 1 file changed, 5 insertions(+), 8 deletions(-)
NACK, we prefer to listing all possible values for typed switch. It
forces a contributor to look at all places where that switch is used
in order to consider whether that place should be updated or not.
Thanks for the review. I don't really see the benefit of listing all
VIDEO_TYPE in this case. If a new TYPE is added, either it's:
1) Something qemu supports and the developer is adding qemu support for it.
There's no way they can miss extending this switch to whitelist the new type,
since otherwise their qemu XML will be rejected at parse time. It's the first
functional thing in src/qemu they have to change
2) Something being added for a non-qemu driver. Maybe qemu supports it, maybe
it doesn't, but regardless the developer isn't on the hook for implementing it
for qemu. In this case, adding the new VIDEO_TYPE to the blacklist is slightly
better code documentation, but it adds no runtime benefits over the 'default:'
case. Plus there's potential issues if the user forgets to add the new TYPE
(like TYPE_GOP currently but the impact is small, start time vs parse time
failure). We could enforce the switch type checking with (virDomainVideoType)
cast but that could lead to build breakage if the dev isn't compiling the qemu
driver.
In some cases I think it makes sense to list all enum values but IMO this
isn't one of them.
Thanks,
Cole
2:42 p.m.
New subject: [libvirt] [PATCH v2 1/5] qemu: whitelist valid video types
On Mon, Aug 28, 2017 at 10:44:43AM -0400, Cole Robinson wrote:
On 08/28/2017 03:56 AM, Pavel Hrdina wrote:
> On Sun, Aug 27, 2017 at 11:04:38AM -0400, Cole Robinson wrote:
>> Rather than require an explicit blacklist that needs to be extended
>> for every new VIDEO_TYPE
>>
>> Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
>> ---
>> src/qemu/qemu_domain.c | 13 +++++--------
>> 1 file changed, 5 insertions(+), 8 deletions(-)
>
> NACK, we prefer to listing all possible values for typed switch. It
> forces a contributor to look at all places where that switch is used
> in order to consider whether that place should be updated or not.
>
Thanks for the review. I don't really see the benefit of listing all
VIDEO_TYPE in this case. If a new TYPE is added, either it's:
Sorry for the late reply, I was solving the chardev reconnect issues.
1) Something qemu supports and the developer is adding qemu support
for it.
There's no way they can miss extending this switch to whitelist the new type,
since otherwise their qemu XML will be rejected at parse time. It's the first
functional thing in src/qemu they have to change
That's true but having the typed switch that forces you to cover all
values without default would save you the trouble of building libvirt,
starting the daemon and defining new domain. It would fail while
building. Yes, most of the developers adding such feature would
probably extend that switch but still if you forget to do it this would
help you realize that right away.
2) Something being added for a non-qemu driver. Maybe qemu supports
it, maybe
it doesn't, but regardless the developer isn't on the hook for implementing it
for qemu. In this case, adding the new VIDEO_TYPE to the blacklist is slightly
better code documentation, but it adds no runtime benefits over the 'default:'
case. Plus there's potential issues if the user forgets to add the new TYPE
(like TYPE_GOP currently but the impact is small, start time vs parse time
failure). We could enforce the switch type checking with (virDomainVideoType)
cast but that could lead to build breakage if the dev isn't compiling the qemu
driver.
So instead of using default I would actually prefer casting the
video->type to virDomainVideoType so if forces you to always consider
this place.
Pavel
2:14 p.m.
New subject: [libvirt] [PATCH v2 1/5] qemu: whitelist valid video types
On 08/28/2017 03:56 AM, Pavel Hrdina wrote:
On Sun, Aug 27, 2017 at 11:04:38AM -0400, Cole Robinson wrote:
> Rather than require an explicit blacklist that needs to be extended
> for every new VIDEO_TYPE
>
> Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
> ---
> src/qemu/qemu_domain.c | 13 +++++--------
> 1 file changed, 5 insertions(+), 8 deletions(-)
NACK, we prefer to listing all possible values for typed switch. It
forces a contributor to look at all places where that switch is used
in order to consider whether that place should be updated or not.
This patch isn't necessary anyways, I can drop it and squash this into patch
#2, ACK to that? (if so I'll push after the release)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 8f93018d8..eb058c57c 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3017,6 +3017,7 @@ qemuDomainDefValidateVideo(const virDomainDef *def)
case VIR_DOMAIN_VIDEO_TYPE_XEN:
case VIR_DOMAIN_VIDEO_TYPE_VBOX:
case VIR_DOMAIN_VIDEO_TYPE_PARALLELS:
+ case VIR_DOMAIN_VIDEO_TYPE_DEFAULT:
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("video type '%s' is not supported with
QEMU"),
virDomainVideoTypeToString(video->type));
Thanks,
Cole
2:46 p.m.
New subject: [libvirt] [PATCH v2 1/5] qemu: whitelist valid video types
On Wed, Aug 30, 2017 at 03:14:40PM -0400, Cole Robinson wrote:
On 08/28/2017 03:56 AM, Pavel Hrdina wrote:
> On Sun, Aug 27, 2017 at 11:04:38AM -0400, Cole Robinson wrote:
>> Rather than require an explicit blacklist that needs to be extended
>> for every new VIDEO_TYPE
>>
>> Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
>> ---
>> src/qemu/qemu_domain.c | 13 +++++--------
>> 1 file changed, 5 insertions(+), 8 deletions(-)
>
> NACK, we prefer to listing all possible values for typed switch. It
> forces a contributor to look at all places where that switch is used
> in order to consider whether that place should be updated or not.
>
This patch isn't necessary anyways, I can drop it and squash this into patch
#2, ACK to that? (if so I'll push after the release)
ACK to that, I was about to suggest it :).
Pavel
10:04 a.m.
New subject: [libvirt] [PATCH v2 2/5] 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_address.c | 1 +
tests/domaincapsschemadata/full.xml | 1 +
5 files changed, 7 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 646b60a83..a6b793075 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -551,6 +551,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 c3d684503..0c1660056 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1348,6 +1348,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 a68ff717f..bc027f85a 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -97,6 +97,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",
@@ -110,6 +111,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",
@@ -123,6 +125,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_address.c b/src/qemu/qemu_domain_address.c
index 16bf0cdf9..8afaa5209 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -753,6 +753,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.5
5:23 a.m.
New subject: [libvirt] [PATCH v2 2/5] conf: domain: add VIDEO_TYPE_DEFAULT
On Sun, Aug 27, 2017 at 11:04:39AM -0400, 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_address.c | 1 +
tests/domaincapsschemadata/full.xml | 1 +
5 files changed, 7 insertions(+)
[...]
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>
This hunk should not be required, but I've checked the code that fills
in domain capabilities and we claim that every video model is supported.
Well that's wrong and should be fixed.
Anyway, the patch itself is good.
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
5:39 a.m.
New subject: [libvirt] [PATCH v2 2/5] conf: domain: add VIDEO_TYPE_DEFAULT
On Mon, Aug 28, 2017 at 12:23:19PM +0200, Pavel Hrdina wrote:
On Sun, Aug 27, 2017 at 11:04:39AM -0400, 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_address.c | 1 +
> tests/domaincapsschemadata/full.xml | 1 +
> 5 files changed, 7 insertions(+)
[...]
> 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>
This hunk should not be required, but I've checked the code that fills
in domain capabilities and we claim that every video model is supported.
Well that's wrong and should be fixed.
Sigh :) Ignore this comment, the code works correctly, this is a test
where we format all capabilities.
Pavel
10:04 a.m.
New subject: [libvirt] [PATCH v2 3/5] 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 | 28 ++++++++++++++++------------
1 file changed, 16 insertions(+), 12 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index a6b793075..e902ea9a8 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5314,7 +5314,18 @@ virDomainHostdevDefValidate(const virDomainHostdevDef *hostdev)
break;
}
}
+ return 0;
+}
+
+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;
}
@@ -5348,11 +5359,13 @@ virDomainDeviceDefValidateInternal(const virDomainDeviceDef *dev,
case VIR_DOMAIN_DEVICE_HOSTDEV:
return virDomainHostdevDefValidate(dev->data.hostdev);
+ 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_WATCHDOG:
case VIR_DOMAIN_DEVICE_GRAPHICS:
case VIR_DOMAIN_DEVICE_HUB:
@@ -13866,7 +13879,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;
}
}
@@ -14015,11 +14028,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) {
@@ -21223,11 +21232,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.5
5:24 a.m.
New subject: [libvirt] [PATCH v2 3/5] conf: domain: move video type validation to DeviceDefValidate
On Sun, Aug 27, 2017 at 11:04:40AM -0400, 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 | 28 ++++++++++++++++------------
1 file changed, 16 insertions(+), 12 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
10:04 a.m.
New subject: [libvirt] [PATCH v2 4/5] 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 e902ea9a8..c0515bb2b 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -13852,9 +13852,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 1c6b1ba79..7d8c2c1ce 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3667,6 +3667,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.5
5:39 a.m.
New subject: [libvirt] [PATCH v2 4/5] qemu: Set default video type in qemu PostParse
On Sun, Aug 27, 2017 at 11:04:41AM -0400, 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: Pavel Hrdina <phrdina(a)redhat.com>
10:04 a.m.
New subject: [libvirt] [PATCH v2 5/5] 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 | 26 ++++++++++++
.../qemuxml2argv-aarch64-video-default.xml | 17 ++++++++
tests/qemuxml2argvtest.c | 6 +++
.../qemuxml2xmlout-aarch64-video-default.xml | 46 ++++++++++++++++++++++
tests/qemuxml2xmltest.c | 10 +++++
6 files changed, 107 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 7d8c2c1ce..6f9fafb6e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3670,6 +3670,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..359c3875d
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-video-default.args
@@ -0,0 +1,26 @@
+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 \
+-chardev socket,id=charmonitor,\
+path=/tmp/lib/domain--1-aarch64-vgpu/monitor.sock,server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=readline \
+-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 5cdbc78eb..a536e9397 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2371,6 +2371,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 311b71356..97ff36cb1 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -1128,6 +1128,16 @@ mymain(void)
QEMU_CAPS_DEVICE_PCI_BRIDGE,
QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
QEMU_CAPS_DEVICE_PCIE_ROOT_PORT);
+ 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.5
5:53 a.m.
New subject: [libvirt] [PATCH v2 5/5] qemu: Default to video type=virtio for machvirt
On Sun, Aug 27, 2017 at 11:04:42AM -0400, 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
Missing a period at the end of sentence.
I would put an empty line after the BZ link.
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
src/qemu/qemu_domain.c | 2 +
.../qemuxml2argv-aarch64-video-default.args | 26 ++++++++++++
.../qemuxml2argv-aarch64-video-default.xml | 17 ++++++++
tests/qemuxml2argvtest.c | 6 +++
.../qemuxml2xmlout-aarch64-video-default.xml | 46 ++++++++++++++++++++++
tests/qemuxml2xmltest.c | 10 +++++
6 files changed, 107 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 7d8c2c1ce..6f9fafb6e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3670,6 +3670,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..359c3875d
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-video-default.args
@@ -0,0 +1,26 @@
+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 \
+-chardev socket,id=charmonitor,\
+path=/tmp/lib/domain--1-aarch64-vgpu/monitor.sock,server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=readline \
+-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'/>
It is possible to skip the graphics and use <video/> instead. This
would make the test only about video device but I'm fine with having
the graphics in the test as well.
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
2641
days inactive
2644
days old
15 comments
2 participants
participants (2)
-
Cole Robinson -
Pavel Hrdina