6:16 a.m.
This patch does seem to fix the original bug as it was reported, however it
also raises the broader question of how exactly to decide under various
circumstances what the type of the video device we're silently adding
should be.
This patch is meant to be an initial suggestion to kick off discussion. If
you don't like anything about it please comment. :-)
Pavel Mores (1):
qemu: fix type of default video device
src/qemu/qemu_domain.c | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
--
2.21.0
6:16 a.m.
New subject: [libvirt] [PATCH 1/1] qemu: fix type of default video device
If a graphics device is added to XML that has no video device, libvirt
automatically adds a video device which is always of type 'cirrus', even if
the underlying qemu doesn't support cirrus.
This patch refines a bit the decision about the type of the video device.
Based on QEMU capabilities, cirrus is still preferred but only added if QEMU
supports it, otherwise QXL is preferred for SPICE and Bochs for UEFI guests.
VGA is used as a fallback.
(For some info about QEMU display devices see e.g.
https://www.kraxel.org/blog/2019/09/display-devices-in-qemu/ .)
https://bugzilla.redhat.com/show_bug.cgi?id=1668141
Signed-off-by: Pavel Mores <pmores(a)redhat.com>
---
src/qemu/qemu_domain.c | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index b4175a846e..a37ff1d384 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7821,7 +7821,8 @@ qemuDomainDeviceNetDefPostParse(virDomainNetDefPtr net,
static int
qemuDomainDeviceVideoDefPostParse(virDomainVideoDefPtr video,
- const virDomainDef *def)
+ const virDomainDef *def,
+ virQEMUCapsPtr qemuCaps)
{
if (video->type == VIR_DOMAIN_VIDEO_TYPE_DEFAULT) {
if (ARCH_IS_PPC64(def->os.arch))
@@ -7830,8 +7831,20 @@ qemuDomainDeviceVideoDefPostParse(virDomainVideoDefPtr video,
qemuDomainIsRISCVVirt(def) ||
ARCH_IS_S390(def->os.arch))
video->type = VIR_DOMAIN_VIDEO_TYPE_VIRTIO;
- else
- video->type = VIR_DOMAIN_VIDEO_TYPE_CIRRUS;
+ else {
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_CIRRUS_VGA)) {
+ video->type = VIR_DOMAIN_VIDEO_TYPE_CIRRUS;
+ } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE)
+ && virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QXL)) {
+ video->type = VIR_DOMAIN_VIDEO_TYPE_QXL;
+ video->vgamem = QEMU_QXL_VGAMEM_DEFAULT;
+ } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_BOCHS_DISPLAY)
+ && def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_EFI) {
+ video->type = VIR_DOMAIN_VIDEO_TYPE_BOCHS;
+ } else {
+ video->type = VIR_DOMAIN_VIDEO_TYPE_VGA;
+ }
+ }
}
if (video->type == VIR_DOMAIN_VIDEO_TYPE_QXL &&
@@ -7926,7 +7939,7 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
break;
case VIR_DOMAIN_DEVICE_VIDEO:
- ret = qemuDomainDeviceVideoDefPostParse(dev->data.video, def);
+ ret = qemuDomainDeviceVideoDefPostParse(dev->data.video, def, qemuCaps);
break;
case VIR_DOMAIN_DEVICE_PANIC:
--
2.21.0
2:37 p.m.
New subject: [libvirt] [PATCH 1/1] qemu: fix type of default video device
On 10/8/19 7:16 AM, Pavel Mores wrote:
If a graphics device is added to XML that has no video device,
libvirt
automatically adds a video device which is always of type 'cirrus', even if
the underlying qemu doesn't support cirrus.
This patch refines a bit the decision about the type of the video device.
Based on QEMU capabilities, cirrus is still preferred but only added if QEMU
supports it, otherwise QXL is preferred for SPICE and Bochs for UEFI guests.
VGA is used as a fallback.
(For some info about QEMU display devices see e.g.
https://www.kraxel.org/blog/2019/09/display-devices-in-qemu/ .)
https://bugzilla.redhat.com/show_bug.cgi?id=1668141
Signed-off-by: Pavel Mores <pmores(a)redhat.com>
---
In response to your RFE message earlier, yup this is the right place to
do it.
I think for starters, breaking the existing logic out into its own
function is a nice cleanup. Then every 'video->type = X' can be a
'return', which will make the function flow nicely IMO. A good model to
follow is qemuDomainDefaultNetModel
Then you can change the log, ideally adding a test case for each one.
The test cases will need custom qemu caps lists to hit certain cases, or
you can just add a test with RHEL8 qemu caps to catch the 'no cirrus'
default case.
As for getting into the business of trying to choose an optimal default,
I have mixed feelings. Trying to abide opinionated decisions like 'bochs
is preferred if firmware=efi' and 'qxl is preferred if type=spice' is a
bit of a slippery slope. I think libvirt should just stick with trying
to generate any working config, and leave the higher level desires up to
management tools.
So IMO for this, I think libvirt could just get away with
if cirrus supported:
use cirrus
elif vga supported:
use vga
else
do nothing. domain_conf.c will error that a video default wasn't
filled in
That will at least inform the user that they need to explicitly specify
a video model. If we wanted to extend that matrix to cover qxl and
virtio and bochs then that's an option too but I'd rather see some
smarter way of dealing with it, like a function to say 'give me all the
supported VIR_DOMAIN_VIDEO_TYPEs' and this function just pops one off
the top. But IMO that's kinda overkill, just hitting cirrus and vga and
maybe virtio for non-x86 virt arches is probably sufficient
One comment below
src/qemu/qemu_domain.c | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index b4175a846e..a37ff1d384 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7821,7 +7821,8 @@ qemuDomainDeviceNetDefPostParse(virDomainNetDefPtr net,
static int
qemuDomainDeviceVideoDefPostParse(virDomainVideoDefPtr video,
- const virDomainDef *def)
+ const virDomainDef *def,
+ virQEMUCapsPtr qemuCaps)
{
if (video->type == VIR_DOMAIN_VIDEO_TYPE_DEFAULT) {
if (ARCH_IS_PPC64(def->os.arch))
@@ -7830,8 +7831,20 @@ qemuDomainDeviceVideoDefPostParse(virDomainVideoDefPtr video,
qemuDomainIsRISCVVirt(def) ||
ARCH_IS_S390(def->os.arch))
video->type = VIR_DOMAIN_VIDEO_TYPE_VIRTIO;
- else
- video->type = VIR_DOMAIN_VIDEO_TYPE_CIRRUS;
+ else {
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_CIRRUS_VGA)) {
+ video->type = VIR_DOMAIN_VIDEO_TYPE_CIRRUS;
+ } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE)
+ && virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QXL)) {
+ video->type = VIR_DOMAIN_VIDEO_TYPE_QXL;
+ video->vgamem = QEMU_QXL_VGAMEM_DEFAULT;
If you decide to keep the qxl bit here, you don't need to set this
VGAMEM value, it's set in the if block below
- Cole
+ } else if (virQEMUCapsGet(qemuCaps,
QEMU_CAPS_DEVICE_BOCHS_DISPLAY)
+ && def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_EFI) {
+ video->type = VIR_DOMAIN_VIDEO_TYPE_BOCHS;
+ } else {
+ video->type = VIR_DOMAIN_VIDEO_TYPE_VGA;
+ }
+ }
}
if (video->type == VIR_DOMAIN_VIDEO_TYPE_QXL &&
@@ -7926,7 +7939,7 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
break;
case VIR_DOMAIN_DEVICE_VIDEO:
- ret = qemuDomainDeviceVideoDefPostParse(dev->data.video, def);
+ ret = qemuDomainDeviceVideoDefPostParse(dev->data.video, def, qemuCaps);
break;
case VIR_DOMAIN_DEVICE_PANIC:
- Cole
1906
days inactive
1907
days old
2 comments
2 participants
participants (2)
-
Cole Robinson -
Pavel Mores