Re: [PATCH] qemu: Add support for -device ati-vga (models: rage128p and rv100)

On a Wednesday in 2020, Steven Newbury wrote: > QEMU since 5.0 has two new video devices rage128p and rv100. Are you sure about the QEMU version? The way you check it (presence of 'ati-vga') says it's available since 4.0.0. commit 862b4a291dcf143fdb227e97feb7fd45e6466aca hw/display: Add basic ATI VGA emulation git describe: v3.1.0-2677-g862b4a291d contains: v4.0.0-rc0~39^2~1 If they were present in QEMU before 5.0, but unusable, we need to find a different way to set the capability.

> These emulate ATI Rage128 Pro and Radeon GPUs respectively, > at least to some extent. This can be useful for OS without > virtualisation drivers or support for VBE or the old Cirrus > emulation. > > Signed-off-by: Steven Newbury <steve(a)snewbury.org.uk&gt; > --- > docs/schemas/domaincommon.rng | 2 ++ > src/conf/domain_conf.c | 7 +++++++ > src/conf/domain_conf.h | 2 ++ Ideally, this would be split in multiple patches: * The QEMU capablity addition: src/qemu/qemu_capabilities* tests/qemucapabilitiesdata* * XML parser/formatter: docs/schemas/ src/conf/* tests/*xml2xml* * QEMU command line formatter the rest But this is an enum addition, so our compile warnings require that the two last steps are squashed together. But having the capability separate means less changes in the main patch and easier rebasing if multiple people add a capability.

> src/qemu/qemu_capabilities.c | 6 ++++++ > src/qemu/qemu_capabilities.h | 1 + > src/qemu/qemu_command.c | 6 ++++++ > src/qemu/qemu_domain_address.c | 2 ++ > src/qemu/qemu_process.c | 2 ++ > tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml | 1 + > tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml | 1 + > tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml | 1 + > tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml | 1 + Missing xml2xml test is not such a big deal for just another enum value, but please take a look at qemuxml2argvtest and add a test case to verify that it actually creates the commandline you want.

Once you create a domain XML using the device in qemuxml2argvdata, it will automatically be checked against the schema thans to virschematest. By setting VIR_TEST_REGENERATE_OUTPUT=1 before running the test, it will overwrite the expected output file with what it actually expects, so you don't have to write the output by hand.

> 12 files changed, 32 insertions(+) > > diff --git a/docs/schemas/domaincommon.rng > b/docs/schemas/domaincommon.rng > index 7d4b105981..d52eb293a8 100644 > --- a/docs/schemas/domaincommon.rng > +++ b/docs/schemas/domaincommon.rng > @@ -3915,6 +3915,8 @@ > <value>none</value> > <value>bochs</value> > <value>ramfb</value> > + <value>rage128p</value> > + <value>rv100</value> > </choice> > </attribute> > <group> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index c003b5c030..5d8b6a7611 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -811,6 +811,8 @@ VIR_ENUM_IMPL(virDomainVideo, > "none", > "bochs", > "ramfb", > + "rage128p", > + "rv100", > ); > > VIR_ENUM_IMPL(virDomainVideoVGAConf, > @@ -16016,6 +16018,11 @@ virDomainVideoDefaultRAM(const > virDomainDef *def, > /* QEMU use 64M as the minimal video memory for qxl device > */ > return 64 * 1024; > > + case VIR_DOMAIN_VIDEO_TYPE_RAGE128P: > + case VIR_DOMAIN_VIDEO_TYPE_RV100: > + /* The real world devices started at 16M */ > + return 16 * 1024; > + > case VIR_DOMAIN_VIDEO_TYPE_DEFAULT: > case VIR_DOMAIN_VIDEO_TYPE_VBOX: > case VIR_DOMAIN_VIDEO_TYPE_PARALLELS: > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h > index 450686dfb5..b12c7ab00d 100644 > --- a/src/conf/domain_conf.h > +++ b/src/conf/domain_conf.h > @@ -1502,6 +1502,8 @@ typedef enum { > VIR_DOMAIN_VIDEO_TYPE_NONE, > VIR_DOMAIN_VIDEO_TYPE_BOCHS, > VIR_DOMAIN_VIDEO_TYPE_RAMFB, > + VIR_DOMAIN_VIDEO_TYPE_RAGE128P, > + VIR_DOMAIN_VIDEO_TYPE_RV100, > > VIR_DOMAIN_VIDEO_TYPE_LAST > } virDomainVideoType; > diff --git a/src/qemu/qemu_capabilities.c > b/src/qemu/qemu_capabilities.c > index 38b901a6c4..0b2bd24619 100644 > --- a/src/qemu/qemu_capabilities.c > +++ b/src/qemu/qemu_capabilities.c > @@ -600,6 +600,7 @@ VIR_ENUM_IMPL(virQEMUCaps, > > /* 380 */ > "usb-host.hostdevice", > + "ati-vga", > ); > > > @@ -1325,6 +1326,7 @@ struct virQEMUCapsStringFlags > virQEMUCapsObjectTypes[] = { > { "tcg-accel", QEMU_CAPS_TCG }, > { "pvscsi", QEMU_CAPS_SCSI_PVSCSI }, > { "spapr-tpm-proxy", QEMU_CAPS_DEVICE_SPAPR_TPM_PROXY }, > + { "ati-vga", QEMU_CAPS_DEVICE_ATI_VGA }, > }; > > > @@ -6127,6 +6129,10 @@ > virQEMUCapsFillDomainDeviceVideoCaps(virQEMUCapsPtr qemuCaps, > VIR_DOMAIN_CAPS_ENUM_SET(dev->modelType, > VIR_DOMAIN_VIDEO_TYPE_BOCHS); > if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_RAMFB)) > VIR_DOMAIN_CAPS_ENUM_SET(dev->modelType, > VIR_DOMAIN_VIDEO_TYPE_RAMFB); > + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_ATI_VGA)) { > + VIR_DOMAIN_CAPS_ENUM_SET(dev->modelType, > VIR_DOMAIN_VIDEO_TYPE_RAGE128P); > + VIR_DOMAIN_CAPS_ENUM_SET(dev->modelType, > VIR_DOMAIN_VIDEO_TYPE_RV100); > + } > } > > > diff --git a/src/qemu/qemu_capabilities.h > b/src/qemu/qemu_capabilities.h > index 107056ba17..e316bd1532 100644 > --- a/src/qemu/qemu_capabilities.h > +++ b/src/qemu/qemu_capabilities.h > @@ -580,6 +580,7 @@ typedef enum { /* virQEMUCapsFlags grouping > marker for syntax-check */ > > /* 380 */ > QEMU_CAPS_USB_HOST_HOSTDEVICE, /* -device usb-host.hostdevice > */ > + QEMU_CAPS_DEVICE_ATI_VGA, /* -device ati-vga */ > > QEMU_CAPS_LAST /* this must always be the last item */ > } virQEMUCapsFlags; > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index 476cf6972e..b501b8e3e2 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -101,6 +101,8 @@ VIR_ENUM_IMPL(qemuVideo, > "" /* 'none' doesn't make sense here */, > "bochs-display", > "", /* ramfb can't be used with -vga */ > + "rage128p", > + "rv100", > ); > > VIR_ENUM_DECL(qemuDeviceVideo); > @@ -120,6 +122,8 @@ VIR_ENUM_IMPL(qemuDeviceVideo, > "" /* 'none' doesn't make sense here */, > "bochs-display", > "ramfb", > + "ati-vga" /* (rage128p) */, > + "ati-vga" /* (rv100) */, > ); > > VIR_ENUM_DECL(qemuDeviceVideoSecondary); > @@ -139,6 +143,8 @@ VIR_ENUM_IMPL(qemuDeviceVideoSecondary, > "" /* 'none' doesn't make sense here */, > "" /* no secondary device for bochs */, > "" /* no secondary device for ramfb */, > + "" /* no secondary device for ati-vga? (rage128p) > */, > + "" /* no secondary device for ati-vga? (rv100) */, IIUC there can only be one vga device, so this seems correct.

> ); > > VIR_ENUM_IMPL(qemuSoundCodec, > diff --git a/src/qemu/qemu_domain_address.c > b/src/qemu/qemu_domain_address.c > index dd87915a97..d8273af92f 100644 > --- a/src/qemu/qemu_domain_address.c > +++ b/src/qemu/qemu_domain_address.c > @@ -964,6 +964,8 @@ > qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev, > case VIR_DOMAIN_VIDEO_TYPE_VBOX: > case VIR_DOMAIN_VIDEO_TYPE_QXL: > case VIR_DOMAIN_VIDEO_TYPE_PARALLELS: > + case VIR_DOMAIN_VIDEO_TYPE_RAGE128P: > + case VIR_DOMAIN_VIDEO_TYPE_RV100: > return pciFlags; > > case VIR_DOMAIN_VIDEO_TYPE_BOCHS: > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index 6b5de29fdb..09aac14430 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -3111,6 +3111,8 @@ > qemuProcessUpdateVideoRamSize(virQEMUDriverPtr driver, > case VIR_DOMAIN_VIDEO_TYPE_CIRRUS: > case VIR_DOMAIN_VIDEO_TYPE_XEN: > case VIR_DOMAIN_VIDEO_TYPE_VBOX: > + case VIR_DOMAIN_VIDEO_TYPE_RAGE128P: > + case VIR_DOMAIN_VIDEO_TYPE_RV100: > case VIR_DOMAIN_VIDEO_TYPE_LAST: > break; > } > diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml > b/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml > index 928af2a01c..30609085e1 100644 > --- a/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml > +++ b/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml [...] These capabilities changes are incomplete. VIR_TEST_REGENERATE_OUTPUT=1 build/tests/qemucapabilitiestest assumes this is supported since QEMU 4.0