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
