Re: [PATCH] qemu: add bochs-display device

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux