On Thu, Jun 27, 2019 at 14:03:18 -0500, Jonathon Jongsma wrote: > qemu provides the bochs-display video device since 3.0. This patch adds > support for this device in libvirt. See Gerd's post for more details: > https://www.kraxel.org/blog/2018/10/qemu-vga-emulation-and-bochs-display/ > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1643404 > > Signed-off-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx> > --- > Note that the documentation may need to be changed depending on which version the patch makes it > into. I suppose it'll miss 5.5.0 since we're in freeze right now. > > Note: depending on which distribution you're using, you may need to copy the vgabios into place in > order to test. For example: > $ sudo ln -s /path/to/qemu/pc-bios/vgabios-bochs-display.bin /usr/share/qemu/ > > docs/formatdomain.html.in | 5 +-- > docs/schemas/domaincommon.rng | 1 + > src/conf/domain_conf.c | 2 ++ > src/conf/domain_conf.h | 1 + > src/qemu/qemu_capabilities.c | 4 +++ > src/qemu/qemu_capabilities.h | 3 ++ > src/qemu/qemu_command.c | 18 +++++++---- > src/qemu/qemu_domain.c | 1 + > src/qemu/qemu_domain_address.c | 1 + > .../qemucapabilitiesdata/caps_3.0.0.ppc64.xml | 1 + > .../caps_3.0.0.x86_64.xml | 1 + > .../qemucapabilitiesdata/caps_3.1.0.ppc64.xml | 1 + > .../caps_3.1.0.x86_64.xml | 1 + > .../caps_4.0.0.aarch64.xml | 1 + > .../qemucapabilitiesdata/caps_4.0.0.ppc64.xml | 1 + > .../caps_4.0.0.riscv32.xml | 1 + > .../caps_4.0.0.riscv64.xml | 1 + > .../caps_4.0.0.x86_64.xml | 1 + > .../caps_4.1.0.x86_64.xml | 1 + > .../video-bochs-display-device.args | 32 +++++++++++++++++++ > .../video-bochs-display-device.xml | 29 +++++++++++++++++ > tests/qemuxml2argvtest.c | 3 ++ > 22 files changed, 102 insertions(+), 8 deletions(-) > create mode 100644 tests/qemuxml2argvdata/video-bochs-display-device.args > create mode 100644 tests/qemuxml2argvdata/video-bochs-display-device.xml We usually split out config/schema, and capability changes into separate patches. > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index a7a6ec32a5..9298ee7b16 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -6990,8 +6990,9 @@ qemu-kvm -net nic,model=? /dev/null > attribute which takes the value "vga", "cirrus", "vmvga", "xen", > "vbox", "qxl" (<span class="since">since 0.8.6</span>), > "virtio" (<span class="since">since 1.3.0</span>), > - "gop" (<span class="since">since 3.2.0</span>), or > - "none" (<span class="since">since 4.6.0</span>) > + "gop" (<span class="since">since 3.2.0</span>), > + "none" (<span class="since">since 4.6.0</span>, or "bochs-display" > + (<span class="since">since 5.5.0</span>) > depending on the hypervisor features available. > The purpose of the type <code>none</code> is to instruct libvirt not > to add a default video device in the guest (see the paragraph above). I'm wondering whether we need to use the '-display' suffix here. While we try to model stuff universally, in this case keeping the qemu name verbatim probably makes sense. [...] > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c > index 02e84edc15..ec68d05112 100644 > --- a/src/qemu/qemu_capabilities.c > +++ b/src/qemu/qemu_capabilities.c > @@ -533,6 +533,9 @@ VIR_ENUM_IMPL(virQEMUCaps, > "x86-max-cpu", > "cpu-unavailable-features", > "canonical-cpu-features", > + > + /* 330 */ > + "bochs-display", > ); [...] > static struct virQEMUCapsStringFlags virQEMUCapsDevicePropsVirtioBalloon[] = { > diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h > index 915ba6cb2e..3cb56e63f4 100644 > --- a/src/qemu/qemu_capabilities.h > +++ b/src/qemu/qemu_capabilities.h > @@ -515,6 +515,9 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ > QEMU_CAPS_CPU_UNAVAILABLE_FEATURES, /* "unavailable-features" CPU property */ > QEMU_CAPS_CANONICAL_CPU_FEATURES, /* avoid CPU feature aliases */ > > + /* 335 */ > + QEMU_CAPS_DEVICE_BOCHS_DISPLAY, /* -device bochs-display */ The section numbers don't match with the above one. > + > 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 688dc324c6..5455b42f4a 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c [...] > @@ -145,6 +147,7 @@ VIR_ENUM_IMPL(qemuDeviceVideoSecondary, > "virtio-gpu", > "" /* don't support gop */, > "" /* 'none' doesn't make sense here */, > + "bochs-display", You are using it with the 'qemuDeviceVideoSecondary' array, but qemuDomainDeviceDefValidateVideo allows only QXL and VIRTIO as secondary graphics, so one of them needs to change. > ); > > VIR_ENUM_DECL(qemuSoundCodec); > @@ -4748,13 +4751,16 @@ qemuBuildDeviceVideoStr(const virDomainDef *def, > if (video->heads) > virBufferAsprintf(&buf, ",max_outputs=%u", video->heads); > } > - } else if (video->vram && > - ((video->type == VIR_DOMAIN_VIDEO_TYPE_VGA && > - virQEMUCapsGet(qemuCaps, QEMU_CAPS_VGA_VGAMEM)) || > - (video->type == VIR_DOMAIN_VIDEO_TYPE_VMVGA && > - virQEMUCapsGet(qemuCaps, QEMU_CAPS_VMWARE_SVGA_VGAMEM)))) { > + } else if (video->vram) { > + if ((video->type == VIR_DOMAIN_VIDEO_TYPE_VGA && > + virQEMUCapsGet(qemuCaps, QEMU_CAPS_VGA_VGAMEM)) || > + (video->type == VIR_DOMAIN_VIDEO_TYPE_VMVGA && > + virQEMUCapsGet(qemuCaps, QEMU_CAPS_VMWARE_SVGA_VGAMEM))) { > > - virBufferAsprintf(&buf, ",vgamem_mb=%u", video->vram / 1024); > + virBufferAsprintf(&buf, ",vgamem_mb=%u", video->vram / 1024); > + } else if (video->type == VIR_DOMAIN_VIDEO_TYPE_BOCHS_DISPLAY) { > + virBufferAsprintf(&buf, ",vgamem=%uk", video->vram); > + } This logic would probably benefit of some separate refactoring first before adding actual functionality. > } > > if (qemuBuildDeviceAddressStr(&buf, def, &video->info, qemuCaps) < 0) [...] > diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c > index 4b99e8ca93..677a3f0499 100644 > --- a/src/qemu/qemu_domain_address.c > +++ b/src/qemu/qemu_domain_address.c > @@ -929,6 +929,7 @@ 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_BOCHS_DISPLAY: > return pciFlags; The blog article you linked above says: "No I/O ports needed. You can plug it into an PCIe slot." Since the idea is to use it with modern machine types (which usually use PCIe) as a replacement for VGA I think that fact needs to be taken into account.
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
