Re: [PATCH v1 11/11] qemu: command: Enable formatting vfio-pci.display option onto cmdline

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

 




On 06/27/2018 09:34 AM, Erik Skultety wrote:

Kinda sparse for what I assume is a new "feature" (and I won't comment
about you're missing a news.xml change :-P)

> Signed-off-by: Erik Skultety <eskultet@xxxxxxxxxx>
> ---
>  src/qemu/qemu_command.c                            | 23 +++++++++++-
>  src/qemu/qemu_domain.c                             |  3 +-
>  .../hostdev-mdev-display-spice-egl-headless.args   | 32 +++++++++++++++++
>  .../hostdev-mdev-display-spice-egl-headless.xml    | 41 ++++++++++++++++++++++
>  .../hostdev-mdev-display-spice-opengl.args         | 31 ++++++++++++++++
>  .../hostdev-mdev-display-spice-opengl.xml          | 41 ++++++++++++++++++++++
>  .../hostdev-mdev-display-vnc-egl-headless.args     | 32 +++++++++++++++++
>  .../hostdev-mdev-display-vnc-egl-headless.xml      | 41 ++++++++++++++++++++++
>  .../qemuxml2argvdata/hostdev-mdev-display-vnc.args | 31 ++++++++++++++++
>  .../qemuxml2argvdata/hostdev-mdev-display-vnc.xml  | 39 ++++++++++++++++++++
>  tests/qemuxml2argvtest.c                           | 29 +++++++++++++++
>  11 files changed, 341 insertions(+), 2 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-egl-headless.args
>  create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-egl-headless.xml
>  create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-opengl.args
>  create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-opengl.xml
>  create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-vnc-egl-headless.args
>  create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-vnc-egl-headless.xml
>  create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-vnc.args
>  create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-vnc.xml
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index fc80a6c3a6..a2a27b5b9b 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -5162,6 +5162,16 @@ qemuBuildHostdevMediatedDevStr(const virDomainDef *def,
>      virBufferAdd(&buf, dev_str, -1);
>      virBufferAsprintf(&buf, ",id=%s,sysfsdev=%s", dev->info->alias, mdevPath);
>  
> +    /* QEMU 2.12 added support for vfio-pci display type, we default to
> +     * 'display=off' to stay safe from future changes */

I have a vague recollection of a longer explanation for this during the
previous review - maybe put some more details here why we had to use
display=off, especially since the commit message is sparse.

> +    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_PCI_DISPLAY) &&
> +        mdevsrc->display == VIR_TRISTATE_SWITCH_ABSENT)
> +        mdevsrc->display = VIR_TRISTATE_SWITCH_OFF;
> +
> +    if (mdevsrc->display > VIR_TRISTATE_SWITCH_ABSENT)
> +        virBufferAsprintf(&buf, ",display=%s",
> +                          virTristateSwitchTypeToString(mdevsrc->display));
> +
>      if (qemuBuildDeviceAddressStr(&buf, def, dev->info, qemuCaps) < 0)
>          goto cleanup;
>  
> @@ -5379,7 +5389,9 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
>  
>          /* MDEV */
>          if (virHostdevIsMdevDevice(hostdev)) {
> -            switch ((virMediatedDeviceModelType) subsys->u.mdev.model) {
> +            virDomainHostdevSubsysMediatedDevPtr mdevsrc = &subsys->u.mdev;
> +
> +            switch ((virMediatedDeviceModelType) mdevsrc->model) {
>              case VIR_MDEV_MODEL_TYPE_VFIO_PCI:
>                  if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_PCI)) {
>                      virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> @@ -5387,6 +5399,15 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
>                                       "supported by this version of QEMU"));
>                      return -1;
>                  }
> +
> +                if (mdevsrc->display != VIR_TRISTATE_SWITCH_ABSENT &&
> +                    !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_PCI_DISPLAY)) {
> +                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                                   _("display property of device vfio-pci is "
> +                                     "not supported by this version of QEMU"));
> +                        return -1;

s/    //

Too many spaces for return -1

> +                }
> +
>                  break;
>              case VIR_MDEV_MODEL_TYPE_VFIO_CCW:
>                  if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_CCW)) {
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index d624383c61..39920da442 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -6141,7 +6141,7 @@ qemuDomainHostdevMdevDefPostParse(const virDomainHostdevSubsysMediatedDev *mdevs
>      }
>  
>      if (mdevsrc->display == VIR_TRISTATE_SWITCH_ON) {
> -        virDomainGraphicsDefPtr graphics = def->graphics[0];
> +        virDomainGraphicsDefPtr graphics = NULL;
>  
>          if (def->ngraphics == 0) {
>              virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> @@ -6154,6 +6154,7 @@ qemuDomainHostdevMdevDefPostParse(const virDomainHostdevSubsysMediatedDev *mdevs
>           * moment, so print a warning that an extra <gl> element might be
>           * necessary to be added.
>           */
> +        graphics = def->graphics[0];
>          if (!graphics->gl ||
>              graphics->gl->enable != VIR_TRISTATE_BOOL_YES) {
>              VIR_WARN("<hostdev> attribute 'display' may need the OpenGL to "

Thus hunk belongs in the previous patch...

John

[...]

FYI: The examples certainly help point out usage of gl enable=yes
conditions and when egl-headless comes into play, I'm still confused
over the RFC comments, but that shouldn't really surprise anyone ;-)

--
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