[libvirt] [PATCH v1 10/11] conf: Introduce new <hostdev> attribute 'display'

[John Ferlan]

On 06/27/2018 09:34 AM, Erik Skultety wrote:
> QEMU 2.12 introduced a new type of display for mediated devices using
> vfio-pci backend which allows a mediated device to be used as a VGA
> compatible device as an alternative to an emulated video device. QEMU
> exposes this feature via a vfio device property 'display' with supported
> values 'on/off/auto' (default is 'off').
> 
> This patch adds the necessary bits to domain config handling in order to
> expose this feature. Since there's no convenient way for libvirt to come
> up with usable defaults for the display setting, simply because libvirt
> is not able to figure out which of the display implementations - dma-buf
> which requires OpenGL support vs vfio regions which doesn't need OpenGL
> (works with OpenGL enabled too) - the underlying mdev uses.
> 
> Signed-off-by: Erik Skultety <eskultet at redhat.com>
> ---
>  docs/formatdomain.html.in                          | 19 +++++-
>  docs/schemas/domaincommon.rng                      |  5 ++
>  src/conf/domain_conf.c                             | 18 +++++-
>  src/conf/domain_conf.h                             |  1 +
>  src/qemu/qemu_domain.c                             | 71 +++++++++++++++++++++-
>  .../hostdev-mdev-display-missing-graphics.xml      | 35 +++++++++++
>  tests/qemuxml2argvdata/hostdev-mdev-display.xml    | 39 ++++++++++++
>  .../hostdev-mdev-display-active.xml                | 47 ++++++++++++++
>  .../hostdev-mdev-display-inactive.xml              | 47 ++++++++++++++
>  tests/qemuxml2xmltest.c                            |  2 +
>  10 files changed, 279 insertions(+), 5 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-missing-graphics.xml
>  create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display.xml
>  create mode 100644 tests/qemuxml2xmloutdata/hostdev-mdev-display-active.xml
>  create mode 100644 tests/qemuxml2xmloutdata/hostdev-mdev-display-inactive.xml
> 

[...]

> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 6d399a198e..23d646cb63 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -7651,6 +7651,7 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
>      char *rawio = NULL;
>      char *backendStr = NULL;
>      char *model = NULL;
> +    char *display = NULL;
>      int backend;
>      int ret = -1;
>      virDomainHostdevSubsysPCIPtr pcisrc = &def->source.subsys.u.pci;
> @@ -7670,6 +7671,7 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
>      sgio = virXMLPropString(node, "sgio");
>      rawio = virXMLPropString(node, "rawio");
>      model = virXMLPropString(node, "model");
> +    display = virXMLPropString(node, "display");
>  
>      /* @type is passed in from the caller rather than read from the
>       * xml document, because it is specified in different places for
> @@ -7757,6 +7759,15 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
>                             model);
>              goto error;
>          }
> +
> +        if (display &&
> +            (mdevsrc->display = virTristateSwitchTypeFromString(display)) < 0) {
> +            virReportError(VIR_ERR_XML_ERROR,
> +                           _("unknown value '%s' for <hostdev> attribute "
> +                             "'display'"),
> +                           display);
> +            goto error;
> +        }
>      }
>  
>      switch (def->source.subsys.type) {
> @@ -26574,9 +26585,14 @@ virDomainHostdevDefFormat(virBufferPtr buf,
>                                virTristateBoolTypeToString(scsisrc->rawio));
>          }
>  
> -        if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV)
> +        if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV) {
>              virBufferAsprintf(buf, " model='%s'",
>                                virMediatedDeviceModelTypeToString(mdevsrc->model));
> +            if (mdevsrc->display > VIR_TRISTATE_SWITCH_ABSENT)

Usually just != instead of >, but whatever.

> +                virBufferAsprintf(buf, " display='%s'",
> +                                  virTristateSwitchTypeToString(mdevsrc->display));
> +        }
> +
>      }
>      virBufferAddLit(buf, ">\n");
>      virBufferAdjustIndent(buf, 2);

Similar as before (and sorry if I asked the last time too), any ABI
concerns?

[...]

> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 948b9b7fd0..d624383c61 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -6127,6 +6127,72 @@ qemuDomainVsockDefPostParse(virDomainVsockDefPtr vsock)
>  }
>  
>  
> +static int
> +qemuDomainHostdevMdevDefPostParse(const virDomainHostdevSubsysMediatedDev *mdevsrc,
> +                                  const virDomainDef *def)
> +{
> +    if (mdevsrc->display > VIR_TRISTATE_SWITCH_ABSENT &&

Hmmm... usually this only matters for display == YES (or != ABSENT), but
then in the subsequent patch we generate NO by default.

John

> +        mdevsrc->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("<hostdev> attribute 'display' is only supported"
> +                         " with model='vfio-pci'"));
> +
> +        return -1;
> +    }
> +
> +    if (mdevsrc->display == VIR_TRISTATE_SWITCH_ON) {
> +        virDomainGraphicsDefPtr graphics = def->graphics[0];
> +
> +        if (def->ngraphics == 0) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("graphics device is needed for attribute value "
> +                             "'display=on' in <hostdev>"));
> +            return -1;
> +        }
> +
> +        /* We're not able to tell whether an mdev needs OpenGL or not at the
> +         * moment, so print a warning that an extra <gl> element might be
> +         * necessary to be added.
> +         */
> +        if (!graphics->gl ||
> +            graphics->gl->enable != VIR_TRISTATE_BOOL_YES) {
> +            VIR_WARN("<hostdev> attribute 'display' may need the OpenGL to "
> +                     "be enabled");
> +        }
> +    }
> +
> +    return 0;
> +}
> +


[...]