From 02f80e36cf982c2397cb6efc347106b023bebf75 Mon Sep 17 00:00:00 2001 From: Adam Williamson Date: Tue, 13 Sep 2022 11:09:15 -0700 Subject: [PATCH] Consolidate qemu video device setting, deprecate QEMUVGA As discussed in #2170, this code has progressively become a bit of a mess as special cases were added for different arches and things changed in qemu. This rewrites it to at least consolidate the mess in one place, and deprecate some things which will allow us to simplify it in future. Always setting `-device` rather than sometimes `-vga` and sometimes `-device` allows us to be much more consistent. We 'translate' `QEMUVGA` values into `-device` values after warning that it is deprecated. Eventually we can hopefully get rid of the handling of the deprecated vars, and the code will then be much clearer and simpler. References for this: https://www.kraxel.org/blog/2019/09/display-devices-in-qemu/ https://www.kraxel.org/blog/2019/03/edid-support-for-qemu/ Signed-off-by: Adam Williamson --- backend/qemu.pm | 59 ++++++++++++++++++++++++++------------- doc/backend_vars.asciidoc | 11 ++++---- t/18-backend-qemu.t | 29 +++++++++++++++---- 3 files changed, 68 insertions(+), 31 deletions(-) diff --git a/backend/qemu.pm b/backend/qemu.pm index 36124879..35f2fb32 100644 --- a/backend/qemu.pm +++ b/backend/qemu.pm @@ -501,7 +501,6 @@ sub delete_virtio_console_fifo () { unlink $_ or bmwqemu::fctwarn("Could not unl sub qemu_params_ofw ($self) { my $vars = \%bmwqemu::vars; - $vars->{QEMUVGA} ||= "std"; $vars->{QEMUMACHINE} //= "usb=off"; # set the initial resolution on PCC and SPARC sp('g', "$self->{xres}x$self->{yres}"); @@ -547,18 +546,38 @@ sub setup_tpm ($self, $arch) { } } -sub _set_graphics_backend ($self) { - # note: Specifying EDID information explicitly for std/virtio backends to get consistent behavior across different QEMU - # versions (as of QEMU 7.0.0 the default resolution is no longer 1024x768). - - my $qemu_vga = $bmwqemu::vars{QEMUVGA}; - if (!$qemu_vga || $qemu_vga eq 'std') { - sp('device', "VGA,edid=on,xres=$self->{xres},yres=$self->{yres}"); - } elsif ($qemu_vga eq 'virtio') { - sp('device', "virtio-vga,edid=on,xres=$self->{xres},yres=$self->{yres}"); - } else { - sp('vga', $qemu_vga); # adding this only if not specifying a device; otherwise we'd end up with two graphic cards +sub _set_graphics_backend ($self, $is_arm) { + my $vars = \%bmwqemu::vars; + my $device = "VGA"; + my $options = ""; + if ($vars->{QEMU_OVERRIDE_VIDEO_DEVICE_AARCH64}) { + bmwqemu::fctwarn("QEMU_OVERRIDE_VIDEO_DEVICE_AARCH64 is deprecated, please set QEMU_VIDEO_DEVICE=VGA instead"); + } + else { + # annoying pre-existing special-case default for ARM + $device = "virtio-gpu-pci" if ($is_arm); + } + if ($vars->{QEMU_VIDEO_DEVICE}) { + bmwqemu::fctwarn("Both QEMUVGA and QEMU_VIDEO_DEVICE set, ignoring deprecated QEMUVGA!") if $vars->{QEMUVGA}; + $device = $vars->{QEMU_VIDEO_DEVICE}; + } + elsif ($vars->{QEMUVGA}) { + my $vga = $vars->{QEMUVGA}; + bmwqemu::fctwarn("QEMUVGA is deprecated, please set QEMU_VIDEO_DEVICE"); + $device = "virtio-vga" if ($vga eq "virtio"); + $device = "qxl-vga" if ($vga eq "qxl"); + $device = "cirrus-vga" if ($vga eq "cirrus"); + $device = "VGA" if ($vga eq "std"); + } + my @edids = ("VGA", "virtio-vga", "virtio-gpu-pci", "bochs-display"); + if (grep { $device eq $_ } @edids) { + # these devices support EDID + $options = ",edid=on,xres=$self->{xres},yres=$self->{yres}"; + } + if ($vars->{QEMU_VIDEO_DEVICE_OPTIONS}) { + $options .= "," . $vars->{QEMU_VIDEO_DEVICE_OPTIONS}; } + sp('device', "${device}${options}"); } sub start_qemu ($self) { @@ -678,24 +697,24 @@ sub start_qemu ($self) { $vars->{VDE_PORT} ||= ($vars->{WORKER_ID} // 0) * 2 + 2; } - # misc - my $arch_supports_boot_order = $vars->{UEFI} ? 0 : 1; # UEFI/OVMF supports ",bootindex=N", but not "-boot order=X" - my $use_usb_kbd; + # arch discovery my $arch = $vars->{ARCH} // ''; $arch = 'arm' if ($arch =~ /armv6|armv7/); my $is_arm = $arch eq 'aarch64' || $arch eq 'arm'; - my $custom_video_device = $vars->{QEMU_VIDEO_DEVICE} // 'virtio-gpu-pci'; + + $self->_set_graphics_backend($is_arm); + + # misc + my $arch_supports_boot_order = $vars->{UEFI} ? 0 : 1; # UEFI/OVMF supports ",bootindex=N", but not "-boot order=X" + my $use_usb_kbd; if ($is_arm) { - my $video_device = ($vars->{QEMU_OVERRIDE_VIDEO_DEVICE_AARCH64}) ? 'VGA' : "${custom_video_device},xres=$self->{xres},yres=$self->{yres}"; - sp('device', $video_device); $arch_supports_boot_order = 0; $use_usb_kbd = 1; } elsif ($vars->{OFW}) { $use_usb_kbd = $self->qemu_params_ofw; } - $self->_set_graphics_backend unless $is_arm; my @nicmac; my @nicvlan; @@ -798,7 +817,7 @@ sub start_qemu ($self) { } { # Remove floppy drive device on architectures - sp('global', 'isa-fdc.fdtypeA=none') unless ($arch eq 'aarch64' || $arch eq 'arm' || $vars->{QEMU_NO_FDC_SET}); + sp('global', 'isa-fdc.fdtypeA=none') unless ($is_arm || $vars->{QEMU_NO_FDC_SET}); sp('m', $vars->{QEMURAM}) if $vars->{QEMURAM}; sp('machine', $vars->{QEMUMACHINE}) if $vars->{QEMUMACHINE}; diff --git a/doc/backend_vars.asciidoc b/doc/backend_vars.asciidoc index b5854918..23766252 100644 --- a/doc/backend_vars.asciidoc +++ b/doc/backend_vars.asciidoc @@ -44,8 +44,8 @@ UPLOAD_METER;boolean;0;Display curl progress meter in `upload_logs()` and `uploa UPLOAD_MAX_MESSAGE_SIZE_GB;integer;0;Specifies the max. upload size in GiB for the test API functions `upload_logs()` and `upload_assets()` and the underlying command server API. Zero denotes infinity. UPLOAD_INACTIVITY_TIMEOUT;integer;300;Specifies the inactivity timeout in seconds for the test API functions `upload_logs()` and `upload_assets()` and underlying the command server API. NO_DEPRECATE_BACKEND_$backend;boolean;0;Only warn about deprecated backends instead of aborting -XRES;integer;1024;Resolution of display on x axis -YRES;integer;768;Resolution of display on y axis +XRES;integer;1024;Resolution of display on x axis. Sets the resolution of the video encoder, and in qemu, the initial console resolution when OFW is set (Power and SPARC), and the EDID resolution for devices that support EDID +YRES;integer;768;Resolution of display on y axis. Sets the resolution of the video encoder, and in qemu, the initial console resolution when OFW is set (Power and SPARC), and the EDID resolution for devices that support EDID |==================== @@ -129,7 +129,7 @@ OFW;boolean;0;QEMU Open Firmware is in use OVS_DEBUG;integer;undef;Set debug mode if value is 1 QEMU_ONLY_EXEC;boolean;undef;If set, only execute the qemu process but return early before connecting to the process. This can be helpful for cutting testing time or to connect to the qemu process manually. QEMU_WAIT_FINISH;boolean;undef;Only used for internal testing, see comment in t/18-qemu-options.t for details. -QEMU_OVERRIDE_VIDEO_DEVICE_AARCH64;boolean;undef;If set, for aarch64 systems use VGA as video adapter +QEMU_OVERRIDE_VIDEO_DEVICE_AARCH64;boolean;undef;(Deprecated, set QEMU_VIDEO_DEVICE=VGA) If set, and QEMU_VIDEO_DEVICE is not set, for arm systems use VGA as video adapter instead of virtio-gpu-pci QEMU_DISABLE_SNAPSHOTS;boolean;undef;If set, disable snapshots in QEMU. This needs to be set when using vmdk disk images or in case the worker has slow disks to avoid save_vm calls failing due to timeouts (See https://bugzilla.suse.com/show_bug.cgi?id=1035453[bsc#1035453]) QEMU_QMP_CONNECT_ATTEMPTS;integer;20;The number of attempts to connect to qemu qmp. Usually used for internal testing PATHCNT;integer;2;Number of paths in MULTIPATH scenario @@ -148,7 +148,7 @@ QEMUTHREADS;integer;undef;Number of CPU threads used by VM QEMUTPM;'instance' or appropriate value for local swtpm config;undef;Configure VM to use a TPM emulator device, with appropriate args for the arch. If a TPM device is available at QEMUTPM_PATH_PREFIX + X, where X is the value of QEMUTPM or the worker instance number if QEMUTPM is set to 'instance', it will be used. Otherwise it will be created at test startup. See QEMUTPM_VER in the latter case. QEMUTPM_VER;'1.2' or '2.0' depending on which TPM chip should be emulated;'2.0';If no TPM device has been setup otherwise, swtpm will be used internally to create one with a socket at /tmp/mytpmX QEMUTPM_PATH_PREFIX;string;'/tmp/mytpm';Path prefix to use or create TPM emulator device in -QEMUVGA;see qemu -device ?;QEMU's default;VGA device to use with VM +QEMUVGA;virtio,qxl,cirrus,std;See QEMU_VIDEO_DEVICE;(Deprecated, use QEMU_VIDEO_DEVICE instead) VGA device to use with VM (will be converted to a matching -device parameter) QEMU_COMPRESS_QCOW2;boolean;1;compress qcow2 images intended for upload QEMU_IMG_CREATE_TRIES;integer;3;Define number of tries for qemu-img commands QEMU_HUGE_PAGES_PATH;string;undef;Define a path to use huge pages (e.g. /dev/hugepages/) @@ -195,7 +195,8 @@ VNC;integer;worker instance + 90;Display on which VNC server is running. Actual VNCKB;;;Set the keyboard layout if you are not using en-us WORKER_CLASS;string;undef;qemu system types WORKER_HOSTNAME;string;undef;Worker hostname -QEMU_VIDEO_DEVICE;string;virtio-gpu-pci;Video device to use for ARM architectures (can have options appended e.g. virtio-gpu-gl,edid=on) +QEMU_VIDEO_DEVICE;string;virtio-gpu-pci on ARM, VGA otherwise;Video device to use with VM (using -device, not -vga). Can have options appended e.g. "virtio-gpu-gl,edid=on", but it is better to set QEMU_VIDEO_DEVICE_OPTIONS. See qemu docs and https://www.kraxel.org/blog/2019/09/display-devices-in-qemu/ for valid choices +QEMU_VIDEO_DEVICE_OPTIONS;string;none;Additional options for QEMU_VIDEO_DEVICE (comma-separated). Will be appended after automatically-generated resolution setting options on devices that support EDID |==================== .SVIRT backend diff --git a/t/18-backend-qemu.t b/t/18-backend-qemu.t index 854649ca..41f1cebf 100755 --- a/t/18-backend-qemu.t +++ b/t/18-backend-qemu.t @@ -134,16 +134,33 @@ subtest 'switch_network' => sub { subtest 'setting graphics backend' => sub { my @params; local *backend::qemu::sp = sub (@args) { @params = @args }; - $backend->_set_graphics_backend; - is_deeply \@params, [device => 'VGA,edid=on,xres=1024,yres=768'], 'consistent EDID info set for std backend (no QEMUVGA set)' or diag explain \@params; + $backend->_set_graphics_backend(0); + is_deeply \@params, [device => 'VGA,edid=on,xres=1024,yres=768'], 'default backend is VGA with EDID info (no QEMUVGA or QEMU_VIDEO_DEVICE set)' or diag explain \@params; + + $backend->_set_graphics_backend(1); + is_deeply \@params, [device => 'virtio-gpu-pci,edid=on,xres=1024,yres=768'], 'default backend for ARM is virtio with EDID info (no QEMUVGA or QEMU_VIDEO_DEVICE set)' or diag explain \@params; + + $bmwqemu::vars{QEMU_OVERRIDE_VIDEO_DEVICE_AARCH64} = '1'; + $backend->_set_graphics_backend(1); + is_deeply \@params, [device => 'VGA,edid=on,xres=1024,yres=768'], 'QEMU_OVERRIDE_VIDEO_DEVICE_AARCH64 changes ARM default to VGA' or diag explain \@params; + delete $bmwqemu::vars{QEMU_OVERRIDE_VIDEO_DEVICE_AARCH64}; $bmwqemu::vars{QEMUVGA} = 'virtio'; - $backend->_set_graphics_backend; - is_deeply \@params, [device => 'virtio-vga,edid=on,xres=1024,yres=768'], 'consistent EDID info set for virtio backend' or diag explain \@params; + $backend->_set_graphics_backend(0); + is_deeply \@params, [device => 'virtio-vga,edid=on,xres=1024,yres=768'], 'QEMUVGA=virtio results in device virtio-vga with EDID info' or diag explain \@params; $bmwqemu::vars{QEMUVGA} = 'cirrus'; - $backend->_set_graphics_backend; - is_deeply \@params, [vga => 'cirrus'], 'other backends passes as-is via "-vga" parameter' or diag explain \@params; + $backend->_set_graphics_backend(0); + is_deeply \@params, [device => 'cirrus-vga'], 'QEMUVGA=cirrus results in device cirrus-vga with no EDID info' or diag explain \@params; + + $bmwqemu::vars{QEMU_VIDEO_DEVICE} = 'virtio-vga'; + $backend->_set_graphics_backend(0); + is_deeply \@params, [device => 'virtio-vga,edid=on,xres=1024,yres=768'], 'QEMU_VIDEO_DEVICE wins if both it and QEMUVGA are set' or diag explain \@params; + + delete $bmwqemu::vars{QEMUVGA}; + $bmwqemu::vars{QEMU_VIDEO_DEVICE_OPTIONS} = 'foo=bar'; + $backend->_set_graphics_backend(0); + is_deeply \@params, [device => 'virtio-vga,edid=on,xres=1024,yres=768,foo=bar'], 'QEMU_VIDEO_DEVICE_OPTIONS gets appended to EDID values' or diag explain \@params; }; subtest 'execute arbitrary QMP command' => sub { -- 2.37.1