Signed-off-by: Erik Skultety <eskultet@redhat.com>
---
src/qemu/qemu_command.c | 24 +++++++++++++++-
.../hostdev-mdev-display-spice-no-opengl.args | 32 ++++++++++++++++++++++
.../hostdev-mdev-display-spice-opengl.args | 31 +++++++++++++++++++++
.../qemuxml2argvdata/hostdev-mdev-display-vnc.args | 32 ++++++++++++++++++++++
tests/qemuxml2argvtest.c | 23 ++++++++++++++++
5 files changed, 141 insertions(+), 1 deletion(-)
create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-no-opengl.args
create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-opengl.args
create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-vnc.args
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 1667b09a8b..8a1a4dc72b 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5088,6 +5088,10 @@ qemuBuildHostdevMediatedDevStr(const virDomainDef *def,
virBufferAdd(&buf, dev_str, -1);
virBufferAsprintf(&buf, ",id=%s,sysfsdev=%s", dev->info->alias, mdevPath);
+ if (mdevsrc->display)
+ virBufferAsprintf(&buf, ",display=%s",
+ virTristateSwitchTypeToString(mdevsrc->display));
+
if (qemuBuildDeviceAddressStr(&buf, def, dev->info, qemuCaps) < 0)
goto cleanup;
@@ -5305,7 +5309,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",
@@ -5313,6 +5319,15 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
"supported by this version of QEMU"));
return -1;
}
+
+ if (mdevsrc->display &&
+ !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;
+ }
+
break;
case VIR_MDEV_MODEL_TYPE_VFIO_CCW:
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_CCW)) {
@@ -5335,6 +5350,13 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
return -1;
virCommandAddArg(cmd, devstr);
VIR_FREE(devstr);
+
+ /* we need to add '-display egl-headless' if 'display=on' for
+ * vfio-pci and OpenGL is disabled (either not supported or user
+ * forgot to add 'gl=on' to SPICE or simply wants to use VNC...)
+ */
+ if (mdevsrc->display && !virDomainDefHasSpiceGL(def))
+ virCommandAddArgList(cmd, "-display", "egl-headless", NULL);
}
}
diff --git a/tests/qemuxml2argvdata/hostdev-mdev-display-spice-no-opengl.args b/tests/qemuxml2argvdata/hostdev-mdev-display-spice-no-opengl.args
new file mode 100644
index 0000000000..b3fe29cbf4
--- /dev/null
+++ b/tests/qemuxml2argvdata/hostdev-mdev-display-spice-no-opengl.args
@@ -0,0 +1,32 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=spice \
+/usr/bin/qemu-system-i686 \
+-name QEMUGuest2 \
+-S \
+-machine pc,accel=tcg,usb=off,dump-guest-core=off \
+-m 214 \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest2/monitor.sock,\
+server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-no-acpi \
+-boot c \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest2,format=raw,if=none,id=drive-ide0-0-0 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
+-spice port=0 \
+-device qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,bus=pci.0,\
+addr=0x2 \
+-device vfio-pci,id=hostdev0,\
+sysfsdev=/sys/bus/mdev/devices/53764d0e-85a0-42b4-af5c-2046b460b1dc,display=on,\
+bus=pci.0,addr=0x3 \
+-display egl-headless
diff --git a/tests/qemuxml2argvdata/hostdev-mdev-display-spice-opengl.args b/tests/qemuxml2argvdata/hostdev-mdev-display-spice-opengl.args
new file mode 100644
index 0000000000..170b4e4ed0
--- /dev/null
+++ b/tests/qemuxml2argvdata/hostdev-mdev-display-spice-opengl.args
@@ -0,0 +1,31 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=spice \
+/usr/bin/qemu-system-i686 \
+-name QEMUGuest2 \
+-S \
+-machine pc,accel=tcg,usb=off,dump-guest-core=off \
+-m 214 \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest2/monitor.sock,\
+server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-no-acpi \
+-boot c \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest2,format=raw,if=none,id=drive-ide0-0-0 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
+-spice port=0,gl=on,rendernode=/dev/dri/foo \
+-device qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,bus=pci.0,\
+addr=0x2 \
+-device vfio-pci,id=hostdev0,\
+sysfsdev=/sys/bus/mdev/devices/53764d0e-85a0-42b4-af5c-2046b460b1dc,display=on,\
+bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/hostdev-mdev-display-vnc.args b/tests/qemuxml2argvdata/hostdev-mdev-display-vnc.args
new file mode 100644
index 0000000000..126ac8e6a6
--- /dev/null
+++ b/tests/qemuxml2argvdata/hostdev-mdev-display-vnc.args
@@ -0,0 +1,32 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-i686 \
+-name QEMUGuest2 \
+-S \
+-machine pc,accel=tcg,usb=off,dump-guest-core=off \
+-m 214 \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest2/monitor.sock,\
+server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-no-acpi \
+-boot c \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest2,format=raw,if=none,id=drive-ide0-0-0 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
+-vnc 127.0.0.1:0 \
+-device qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,bus=pci.0,\
+addr=0x2 \
+-device vfio-pci,id=hostdev0,\
+sysfsdev=/sys/bus/mdev/devices/53764d0e-85a0-42b4-af5c-2046b460b1dc,display=on,\
+bus=pci.0,addr=0x3 \
+-display egl-headless
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index ddd2b88c0a..9cd92fbfbe 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1557,6 +1557,29 @@ mymain(void)
QEMU_CAPS_DEVICE_VFIO_PCI);
DO_TEST_PARSE_ERROR("hostdev-mdev-invalid-target-address",
QEMU_CAPS_DEVICE_VFIO_PCI);
+ DO_TEST("hostdev-mdev-display-spice-opengl",
+ QEMU_CAPS_SPICE,
+ QEMU_CAPS_SPICE_GL,
+ QEMU_CAPS_SPICE_RENDERNODE,
+ QEMU_CAPS_DEVICE_QXL,
+ QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
+ QEMU_CAPS_DEVICE_VFIO_PCI,
+ QEMU_CAPS_VFIO_PCI_DISPLAY);
+ DO_TEST("hostdev-mdev-display-spice-no-opengl",
+ QEMU_CAPS_SPICE,
+ QEMU_CAPS_DEVICE_QXL,
+ QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
+ QEMU_CAPS_DEVICE_VFIO_PCI,
+ QEMU_CAPS_VFIO_PCI_DISPLAY);
+ DO_TEST("hostdev-mdev-display-vnc",
+ QEMU_CAPS_VNC,
+ QEMU_CAPS_DEVICE_QXL,
+ QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
+ QEMU_CAPS_DEVICE_VFIO_PCI,
+ QEMU_CAPS_VFIO_PCI_DISPLAY);
+ DO_TEST_PARSE_ERROR("hostdev-mdev-display-missing-graphics",
+ QEMU_CAPS_DEVICE_VFIO_PCI,
+ QEMU_CAPS_VFIO_PCI_DISPLAY);
DO_TEST("pci-rom", NONE);
DO_TEST("pci-rom-disabled", NONE);
DO_TEST("pci-rom-disabled-invalid", NONE);
--
2.14.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 05/30/2018 09:43 AM, Erik Skultety wrote:
> Signed-off-by: Erik Skultety <eskultet@redhat.com>
> ---
> src/qemu/qemu_command.c | 24 +++++++++++++++-
> .../hostdev-mdev-display-spice-no-opengl.args | 32 ++++++++++++++++++++++
> .../hostdev-mdev-display-spice-opengl.args | 31 +++++++++++++++++++++
> .../qemuxml2argvdata/hostdev-mdev-display-vnc.args | 32 ++++++++++++++++++++++
> tests/qemuxml2argvtest.c | 23 ++++++++++++++++
> 5 files changed, 141 insertions(+), 1 deletion(-)
> create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-no-opengl.args
> create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-opengl.args
> create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-vnc.args
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 1667b09a8b..8a1a4dc72b 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -5088,6 +5088,10 @@ qemuBuildHostdevMediatedDevStr(const virDomainDef *def,
> virBufferAdd(&buf, dev_str, -1);
> virBufferAsprintf(&buf, ",id=%s,sysfsdev=%s", dev->info->alias, mdevPath);
>
> + if (mdevsrc->display)
> VIR_TRISTATE_SWITCH_ABSENT
> + virBufferAsprintf(&buf, ",display=%s",
> + virTristateSwitchTypeToString(mdevsrc->display));
> +
> if (qemuBuildDeviceAddressStr(&buf, def, dev->info, qemuCaps) < 0)
> goto cleanup;
>
> @@ -5305,7 +5309,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",
> @@ -5313,6 +5319,15 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
> "supported by this version of QEMU"));
> return -1;
> }
> +
> + if (mdevsrc->display &&
!= 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;
> + }
> +
After reading this, I thought perhaps patch 6 wasn't necessary at least
w/r/t that by setting off we write out to the config file which may not
be desired.
So as a way to avoid this, can we set a "local" @mdev_display value and
pass that qemuBuildHostdevMediatedDevStr which then would then either
ignore or write on/off on the command line.
Thus:
if (mdevsrc->display > SWITCH_ABSENT && !VFIO_PCI_DISPLAY)
error as is
mdev_display = mdevsrc->display
if (mdev_display == SWITCH_ABSENT && VFIO_PCI_DISPLAY)
mdev_display == SWITCH_OFF;
And passing mdev_display to building the command line will print "off"
when mdevsrc->display == ABSENT, but we don't write "off" to the config
file.
??? thoughts
> break;
> case VIR_MDEV_MODEL_TYPE_VFIO_CCW:
> if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_CCW)) {
> @@ -5335,6 +5350,13 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
> return -1;
> virCommandAddArg(cmd, devstr);
> VIR_FREE(devstr);
> +
> + /* we need to add '-display egl-headless' if 'display=on' for
> + * vfio-pci and OpenGL is disabled (either not supported or user
> + * forgot to add 'gl=on' to SPICE or simply wants to use VNC...)
> + */
> + if (mdevsrc->display && !virDomainDefHasSpiceGL(def))
Doesn't this only matter if display == SWITCH_ON ?
Hence the OCD on the "if (mdevsrc->display)" type checks. ;-)
John
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Jun 04, 2018 at 07:40:42PM -0400, John Ferlan wrote:
>
>
> On 05/30/2018 09:43 AM, Erik Skultety wrote:
> > Signed-off-by: Erik Skultety <eskultet@redhat.com>
> > ---
> > src/qemu/qemu_command.c | 24 +++++++++++++++-
> > .../hostdev-mdev-display-spice-no-opengl.args | 32 ++++++++++++++++++++++
> > .../hostdev-mdev-display-spice-opengl.args | 31 +++++++++++++++++++++
> > .../qemuxml2argvdata/hostdev-mdev-display-vnc.args | 32 ++++++++++++++++++++++
> > tests/qemuxml2argvtest.c | 23 ++++++++++++++++
> > 5 files changed, 141 insertions(+), 1 deletion(-)
> > create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-no-opengl.args
> > create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-opengl.args
> > create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-vnc.args
> >
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index 1667b09a8b..8a1a4dc72b 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -5088,6 +5088,10 @@ qemuBuildHostdevMediatedDevStr(const virDomainDef *def,
> > virBufferAdd(&buf, dev_str, -1);
> > virBufferAsprintf(&buf, ",id=%s,sysfsdev=%s", dev->info->alias, mdevPath);
> >
> > + if (mdevsrc->display)
>
> > VIR_TRISTATE_SWITCH_ABSENT
>
> > + virBufferAsprintf(&buf, ",display=%s",
> > + virTristateSwitchTypeToString(mdevsrc->display));
> > +
> > if (qemuBuildDeviceAddressStr(&buf, def, dev->info, qemuCaps) < 0)
> > goto cleanup;
> >
> > @@ -5305,7 +5309,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",
> > @@ -5313,6 +5319,15 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
> > "supported by this version of QEMU"));
> > return -1;
> > }
> > +
> > + if (mdevsrc->display &&
>
> != 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;
> > + }
> > +
>
> After reading this, I thought perhaps patch 6 wasn't necessary at least
> w/r/t that by setting off we write out to the config file which may not
> be desired.
>
> So as a way to avoid this, can we set a "local" @mdev_display value and
> pass that qemuBuildHostdevMediatedDevStr which then would then either
> ignore or write on/off on the command line.
>
> Thus:
>
> if (mdevsrc->display > SWITCH_ABSENT && !VFIO_PCI_DISPLAY)
> error as is
> mdev_display = mdevsrc->display
> if (mdev_display == SWITCH_ABSENT && VFIO_PCI_DISPLAY)
> mdev_display == SWITCH_OFF;
>
> And passing mdev_display to building the command line will print "off"
> when mdevsrc->display == ABSENT, but we don't write "off" to the config
> file.
>
> ??? thoughts
So, w/r/t comments in patch 6, I understand that we'd keep an open door to
changing the default (no display attr means use 'auto' as it would in most
cases in libvirt). However, I don't believe that we'd want to make 'auto' a
default ever because you can't predict whether the user wants to use the device
as GPGPU or graphics, if they leave the display attr unset and we'd default to
auto in the future, we might risk that they know they can't use it for graphics
and they didn't even intend to, but we tried and failed because of some
underlying reasons. And I'm not even sure we'd ever introduce 'auto' to libvirt
for reasons I just mentioned + the cover letter and the discussion that
happened upstream regarding the VNC console. What I know though, is that
libvirt is never going to use QEMU's 'auto', since it doesn't make sense,
either we take care of everything or the user does, regardless of QEMU.
Anyway, the way I imagine 'auto' could work in libvirt is that user references
an mdev, but doesn't care (or doesn't know) what technology it uses underneath
and explicitly says "libvirt please take care of that on my behalf, I want to
use the display, but have 0 clue on what to enable". That's a different kind of
'auto' compared to when we'd try to read user's mind. I tried to play it very
safe here, as the vgpu display thing can be very tricky, not even thinking about
future migrations. But if there's a strong disagreement with this kind of
approach, then I'll of course need to incorporate your comments in patch 6 and
this one to get this successfully merged.
Erik
>
> > break;
> > case VIR_MDEV_MODEL_TYPE_VFIO_CCW:
> > if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_CCW)) {
> > @@ -5335,6 +5350,13 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
> > return -1;
> > virCommandAddArg(cmd, devstr);
> > VIR_FREE(devstr);
> > +
> > + /* we need to add '-display egl-headless' if 'display=on' for
> > + * vfio-pci and OpenGL is disabled (either not supported or user
> > + * forgot to add 'gl=on' to SPICE or simply wants to use VNC...)
> > + */
> > + if (mdevsrc->display && !virDomainDefHasSpiceGL(def))
>
> Doesn't this only matter if display == SWITCH_ON ?
>
> Hence the OCD on the "if (mdevsrc->display)" type checks. ;-)
Will fix this.
Thanks,
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 06/08/2018 07:09 AM, Erik Skultety wrote:
> On Mon, Jun 04, 2018 at 07:40:42PM -0400, John Ferlan wrote:
>>
>>
>> On 05/30/2018 09:43 AM, Erik Skultety wrote:
>>> Signed-off-by: Erik Skultety <eskultet@redhat.com>
>>> ---
>>> src/qemu/qemu_command.c | 24 +++++++++++++++-
>>> .../hostdev-mdev-display-spice-no-opengl.args | 32 ++++++++++++++++++++++
>>> .../hostdev-mdev-display-spice-opengl.args | 31 +++++++++++++++++++++
>>> .../qemuxml2argvdata/hostdev-mdev-display-vnc.args | 32 ++++++++++++++++++++++
>>> tests/qemuxml2argvtest.c | 23 ++++++++++++++++
>>> 5 files changed, 141 insertions(+), 1 deletion(-)
>>> create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-no-opengl.args
>>> create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-opengl.args
>>> create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-vnc.args
>>>
>>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>>> index 1667b09a8b..8a1a4dc72b 100644
>>> --- a/src/qemu/qemu_command.c
>>> +++ b/src/qemu/qemu_command.c
>>> @@ -5088,6 +5088,10 @@ qemuBuildHostdevMediatedDevStr(const virDomainDef *def,
>>> virBufferAdd(&buf, dev_str, -1);
>>> virBufferAsprintf(&buf, ",id=%s,sysfsdev=%s", dev->info->alias, mdevPath);
>>>
>>> + if (mdevsrc->display)
>>
>>> VIR_TRISTATE_SWITCH_ABSENT
>>
>>> + virBufferAsprintf(&buf, ",display=%s",
>>> + virTristateSwitchTypeToString(mdevsrc->display));
>>> +
>>> if (qemuBuildDeviceAddressStr(&buf, def, dev->info, qemuCaps) < 0)
>>> goto cleanup;
>>>
>>> @@ -5305,7 +5309,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",
>>> @@ -5313,6 +5319,15 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
>>> "supported by this version of QEMU"));
>>> return -1;
>>> }
>>> +
>>> + if (mdevsrc->display &&
>>
>> != 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;
>>> + }
>>> +
>>
>> After reading this, I thought perhaps patch 6 wasn't necessary at least
>> w/r/t that by setting off we write out to the config file which may not
>> be desired.
>>
>> So as a way to avoid this, can we set a "local" @mdev_display value and
>> pass that qemuBuildHostdevMediatedDevStr which then would then either
>> ignore or write on/off on the command line.
>>
>> Thus:
>>
>> if (mdevsrc->display > SWITCH_ABSENT && !VFIO_PCI_DISPLAY)
>> error as is
>> mdev_display = mdevsrc->display
>> if (mdev_display == SWITCH_ABSENT && VFIO_PCI_DISPLAY)
>> mdev_display == SWITCH_OFF;
>>
>> And passing mdev_display to building the command line will print "off"
>> when mdevsrc->display == ABSENT, but we don't write "off" to the config
>> file.
>>
>> ??? thoughts
>
> So, w/r/t comments in patch 6, I understand that we'd keep an open door to
> changing the default (no display attr means use 'auto' as it would in most
> cases in libvirt). However, I don't believe that we'd want to make 'auto' a
> default ever because you can't predict whether the user wants to use the device
> as GPGPU or graphics, if they leave the display attr unset and we'd default to
> auto in the future, we might risk that they know they can't use it for graphics
> and they didn't even intend to, but we tried and failed because of some
> underlying reasons. And I'm not even sure we'd ever introduce 'auto' to libvirt
> for reasons I just mentioned + the cover letter and the discussion that
> happened upstream regarding the VNC console. What I know though, is that
> libvirt is never going to use QEMU's 'auto', since it doesn't make sense,
> either we take care of everything or the user does, regardless of QEMU.
>
> Anyway, the way I imagine 'auto' could work in libvirt is that user references
> an mdev, but doesn't care (or doesn't know) what technology it uses underneath
> and explicitly says "libvirt please take care of that on my behalf, I want to
> use the display, but have 0 clue on what to enable". That's a different kind of
> 'auto' compared to when we'd try to read user's mind. I tried to play it very
> safe here, as the vgpu display thing can be very tricky, not even thinking about
> future migrations. But if there's a strong disagreement with this kind of
> approach, then I'll of course need to incorporate your comments in patch 6 and
> this one to get this successfully merged.
>
> Erik
>
I didn't think I was advocating for adding AUTO - just blabbering how
insane the code is ;-). IIRC, if "off" isn't written then auto is
assumed. What I thought I was suggesting here though was to not write
the "off" into the libvirt XML, but rather only into the .args. IOW: If
the option the option is available in qemu, but not supplied in libvirt
XML, then write "off". If someone changes the XML to have "off" or "on",
then that's fine - go with what they have and keep it. I just was trying
to be cautious about writing "off" into the output XML (especially the
*config* one).
John
>>
>>> break;
>>> case VIR_MDEV_MODEL_TYPE_VFIO_CCW:
>>> if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_CCW)) {
>>> @@ -5335,6 +5350,13 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
>>> return -1;
>>> virCommandAddArg(cmd, devstr);
>>> VIR_FREE(devstr);
>>> +
>>> + /* we need to add '-display egl-headless' if 'display=on' for
>>> + * vfio-pci and OpenGL is disabled (either not supported or user
>>> + * forgot to add 'gl=on' to SPICE or simply wants to use VNC...)
>>> + */
>>> + if (mdevsrc->display && !virDomainDefHasSpiceGL(def))
>>
>> Doesn't this only matter if display == SWITCH_ON ?
>>
>> Hence the OCD on the "if (mdevsrc->display)" type checks. ;-)
>
> Will fix this.
>
> Thanks,
> Erik
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Jun 08, 2018 at 12:00:50PM -0400, John Ferlan wrote:
>
>
> On 06/08/2018 07:09 AM, Erik Skultety wrote:
> > On Mon, Jun 04, 2018 at 07:40:42PM -0400, John Ferlan wrote:
> >>
> >>
> >> On 05/30/2018 09:43 AM, Erik Skultety wrote:
> >>> Signed-off-by: Erik Skultety <eskultet@redhat.com>
> >>> ---
> >>> src/qemu/qemu_command.c | 24 +++++++++++++++-
> >>> .../hostdev-mdev-display-spice-no-opengl.args | 32 ++++++++++++++++++++++
> >>> .../hostdev-mdev-display-spice-opengl.args | 31 +++++++++++++++++++++
> >>> .../qemuxml2argvdata/hostdev-mdev-display-vnc.args | 32 ++++++++++++++++++++++
> >>> tests/qemuxml2argvtest.c | 23 ++++++++++++++++
> >>> 5 files changed, 141 insertions(+), 1 deletion(-)
> >>> create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-no-opengl.args
> >>> create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-opengl.args
> >>> create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-vnc.args
> >>>
> >>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> >>> index 1667b09a8b..8a1a4dc72b 100644
> >>> --- a/src/qemu/qemu_command.c
> >>> +++ b/src/qemu/qemu_command.c
> >>> @@ -5088,6 +5088,10 @@ qemuBuildHostdevMediatedDevStr(const virDomainDef *def,
> >>> virBufferAdd(&buf, dev_str, -1);
> >>> virBufferAsprintf(&buf, ",id=%s,sysfsdev=%s", dev->info->alias, mdevPath);
> >>>
> >>> + if (mdevsrc->display)
> >>
> >>> VIR_TRISTATE_SWITCH_ABSENT
> >>
> >>> + virBufferAsprintf(&buf, ",display=%s",
> >>> + virTristateSwitchTypeToString(mdevsrc->display));
> >>> +
> >>> if (qemuBuildDeviceAddressStr(&buf, def, dev->info, qemuCaps) < 0)
> >>> goto cleanup;
> >>>
> >>> @@ -5305,7 +5309,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",
> >>> @@ -5313,6 +5319,15 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
> >>> "supported by this version of QEMU"));
> >>> return -1;
> >>> }
> >>> +
> >>> + if (mdevsrc->display &&
> >>
> >> != 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;
> >>> + }
> >>> +
> >>
> >> After reading this, I thought perhaps patch 6 wasn't necessary at least
> >> w/r/t that by setting off we write out to the config file which may not
> >> be desired.
> >>
> >> So as a way to avoid this, can we set a "local" @mdev_display value and
> >> pass that qemuBuildHostdevMediatedDevStr which then would then either
> >> ignore or write on/off on the command line.
> >>
> >> Thus:
> >>
> >> if (mdevsrc->display > SWITCH_ABSENT && !VFIO_PCI_DISPLAY)
> >> error as is
> >> mdev_display = mdevsrc->display
> >> if (mdev_display == SWITCH_ABSENT && VFIO_PCI_DISPLAY)
> >> mdev_display == SWITCH_OFF;
> >>
> >> And passing mdev_display to building the command line will print "off"
> >> when mdevsrc->display == ABSENT, but we don't write "off" to the config
> >> file.
> >>
> >> ??? thoughts
> >
> > So, w/r/t comments in patch 6, I understand that we'd keep an open door to
> > changing the default (no display attr means use 'auto' as it would in most
> > cases in libvirt). However, I don't believe that we'd want to make 'auto' a
> > default ever because you can't predict whether the user wants to use the device
> > as GPGPU or graphics, if they leave the display attr unset and we'd default to
> > auto in the future, we might risk that they know they can't use it for graphics
> > and they didn't even intend to, but we tried and failed because of some
> > underlying reasons. And I'm not even sure we'd ever introduce 'auto' to libvirt
> > for reasons I just mentioned + the cover letter and the discussion that
> > happened upstream regarding the VNC console. What I know though, is that
> > libvirt is never going to use QEMU's 'auto', since it doesn't make sense,
> > either we take care of everything or the user does, regardless of QEMU.
> >
> > Anyway, the way I imagine 'auto' could work in libvirt is that user references
> > an mdev, but doesn't care (or doesn't know) what technology it uses underneath
> > and explicitly says "libvirt please take care of that on my behalf, I want to
> > use the display, but have 0 clue on what to enable". That's a different kind of
> > 'auto' compared to when we'd try to read user's mind. I tried to play it very
> > safe here, as the vgpu display thing can be very tricky, not even thinking about
> > future migrations. But if there's a strong disagreement with this kind of
> > approach, then I'll of course need to incorporate your comments in patch 6 and
> > this one to get this successfully merged.
> >
> > Erik
> >
>
> I didn't think I was advocating for adding AUTO - just blabbering how
> insane the code is ;-). IIRC, if "off" isn't written then auto is
> assumed. What I thought I was suggesting here though was to not write
> the "off" into the libvirt XML, but rather only into the .args. IOW: If
> the option the option is available in qemu, but not supplied in libvirt
> XML, then write "off". If someone changes the XML to have "off" or "on",
> then that's fine - go with what they have and keep it. I just was trying
> to be cautious about writing "off" into the output XML (especially the
> *config* one).
Alright, we could skip the "offline" XML that's a fair argument, but we should
format something into the "live" one, as that one serves as the base for the
"migration" XML, I'd rather us not sending a missing display argument to a newer
libvirt on the destination for safety reasons, I believe that turning something
'off' rather than 'on' by default is good.
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.