[libvirt] [PATCH v2 0/5] 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 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
Rather than require an explicit blacklist that needs to be extended for every new VIDEO_TYPE Signed-off-by: Cole Robinson <crobinso@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
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@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
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@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
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@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
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@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
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@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
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_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
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@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@redhat.com>
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@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
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 | 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
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@redhat.com> --- src/conf/domain_conf.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
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 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
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@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@redhat.com>
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 | 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
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@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@redhat.com>
participants (2)
-
Cole Robinson -
Pavel Hrdina