[libvirt] [PATCH 1/1] qemu: fix type of default video device

[Cole Robinson]

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