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@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