On Wed, 2020-10-07 at 19:09 +0200, Ján Tomko wrote:
> 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.
>
I will check to see what the status was. I have only tested it down to
5.0, but did have earlier versions in my patch before I sent it.
> > 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 <st...@snewbury.org.uk>
> > ---
> > 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.
>
Okay
> > 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.
>
Well it does work, so I guess it does! But I will do as you say and
make sure all thest tests are working.
> 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.
>
Handy
> > 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.
>
But there can in theory be a second video device (non-vga). It
certainly works with passthrough + emulated. I never tried two
emulated devices, that might be because it can't be done...
> > );
> >
> > 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
Okay. As I said, I'll try it and see.
Thanks for the feedback!
