Re: [libvirt] [PATCH v3 7/8] qemu: command: Enable formatting vfio-pci.display option onto cmdline

2018-07-17 Thread Ján Tomko 

On Mon, Jul 16, 2018 at 03:51:40PM +0200, Erik Skultety wrote:

On Fri, Jul 13, 2018 at 03:06:05PM +0200, Ján Tomko wrote:

On Wed, Jul 11, 2018 at 03:58:27PM +0200, Erik Skultety wrote:
> Since QEMU 2.12, QEMU understands a new vfio-pci device option 'display'
> which can be used to turn on display capabilities on vgpu-enabled
> mediated devices, IOW emulated GPU devices like QXL will no longer be
> needed with vgpu-enable mdevs.
> QEMU defaults to 'auto' for the 'display' attribute, which is not
> foolproof, so we need to play it safe here and explicitly format
> display='off' if this attribute wasn't provided in the XML explicitly.
>
> Signed-off-by: Erik Skultety 
> ---
> src/qemu/qemu_command.c| 25 -
> .../hostdev-mdev-display-missing-graphics.xml  | 35 ++
> .../hostdev-mdev-display-spice-egl-headless.args   | 32 +
> .../hostdev-mdev-display-spice-egl-headless.xml| 40 +
> .../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  | 40 +
> .../qemuxml2argvdata/hostdev-mdev-display-vnc.args | 31 
> .../qemuxml2argvdata/hostdev-mdev-display-vnc.xml  | 39 
> tests/qemuxml2argvtest.c   | 31 
> 11 files changed, 376 insertions(+), 1 deletion(-)
> create mode 100644 
tests/qemuxml2argvdata/hostdev-mdev-display-missing-graphics.xml
> 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 48e224cabc..5f6b340f8f 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -5207,6 +5207,26 @@ 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 need to perform
> + * some additional checks here */
> +if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_PCI_DISPLAY)) {
> +if (mdevsrc->display != VIR_TRISTATE_SWITCH_ABSENT) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +   _("display property of device vfio-pci is "
> + "not supported by this version of QEMU"));
> +goto cleanup;
> +}

qemuCaps checks belong to *Validate functions.


Well, we still need to come up with the default value, but only in case QEMU
has the cap and we already agreed upon not doing this in postparse,


By putting it in postParse you get the benefit of preserving this
default for future starts (if we ever can and decide to change the
default) and more importantly it gets displayed in the domain XML.


otherwise
we'd format something into the offline config even if there was no input from
the user, so I'd like to keep it this way, otherwise the capability check would
have to be at 2 places instead of 1.


You can create a domain with pretty minimal XML and we will fill in all
the sensible defaults, there's no reason to treat this attribute
differently.

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 7/8] qemu: command: Enable formatting vfio-pci.display option onto cmdline

2018-07-16 Thread Erik Skultety 
On Fri, Jul 13, 2018 at 03:06:05PM +0200, Ján Tomko wrote:
> On Wed, Jul 11, 2018 at 03:58:27PM +0200, Erik Skultety wrote:
> > Since QEMU 2.12, QEMU understands a new vfio-pci device option 'display'
> > which can be used to turn on display capabilities on vgpu-enabled
> > mediated devices, IOW emulated GPU devices like QXL will no longer be
> > needed with vgpu-enable mdevs.
> > QEMU defaults to 'auto' for the 'display' attribute, which is not
> > foolproof, so we need to play it safe here and explicitly format
> > display='off' if this attribute wasn't provided in the XML explicitly.
> >
> > Signed-off-by: Erik Skultety 
> > ---
> > src/qemu/qemu_command.c| 25 -
> > .../hostdev-mdev-display-missing-graphics.xml  | 35 ++
> > .../hostdev-mdev-display-spice-egl-headless.args   | 32 +
> > .../hostdev-mdev-display-spice-egl-headless.xml| 40 
> > +
> > .../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  | 40 
> > +
> > .../qemuxml2argvdata/hostdev-mdev-display-vnc.args | 31 
> > .../qemuxml2argvdata/hostdev-mdev-display-vnc.xml  | 39 
> > tests/qemuxml2argvtest.c   | 31 
> > 11 files changed, 376 insertions(+), 1 deletion(-)
> > create mode 100644 
> > tests/qemuxml2argvdata/hostdev-mdev-display-missing-graphics.xml
> > 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 48e224cabc..5f6b340f8f 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -5207,6 +5207,26 @@ 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 need to 
> > perform
> > + * some additional checks here */
> > +if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_PCI_DISPLAY)) {
> > +if (mdevsrc->display != VIR_TRISTATE_SWITCH_ABSENT) {
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > +   _("display property of device vfio-pci is "
> > + "not supported by this version of QEMU"));
> > +goto cleanup;
> > +}
>
> qemuCaps checks belong to *Validate functions.

Well, we still need to come up with the default value, but only in case QEMU
has the cap and we already agreed upon not doing this in postparse, otherwise
we'd format something into the offline config even if there was no input from
the user, so I'd like to keep it this way, otherwise the capability check would
have to be at 2 places instead of 1.

>
> > +} else {
> > + /* we default to 'display=off', since QEMU defaults to 'auto' 
> > which is
> > +  * unreliable and we don't want to risk any breakages */
> > +if (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;
> >
>
> > @@ -5424,7 +5444,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",
> > @@ -5432,6 +5454,7 @@ qemuBuildHostdevCommandLine(virComma

Re: [libvirt] [PATCH v3 7/8] qemu: command: Enable formatting vfio-pci.display option onto cmdline

2018-07-13 Thread Ján Tomko 

On Wed, Jul 11, 2018 at 03:58:27PM +0200, Erik Skultety wrote:

Since QEMU 2.12, QEMU understands a new vfio-pci device option 'display'
which can be used to turn on display capabilities on vgpu-enabled
mediated devices, IOW emulated GPU devices like QXL will no longer be
needed with vgpu-enable mdevs.
QEMU defaults to 'auto' for the 'display' attribute, which is not
foolproof, so we need to play it safe here and explicitly format
display='off' if this attribute wasn't provided in the XML explicitly.

Signed-off-by: Erik Skultety 
---
src/qemu/qemu_command.c| 25 -
.../hostdev-mdev-display-missing-graphics.xml  | 35 ++
.../hostdev-mdev-display-spice-egl-headless.args   | 32 +
.../hostdev-mdev-display-spice-egl-headless.xml| 40 +
.../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  | 40 +
.../qemuxml2argvdata/hostdev-mdev-display-vnc.args | 31 
.../qemuxml2argvdata/hostdev-mdev-display-vnc.xml  | 39 
tests/qemuxml2argvtest.c   | 31 
11 files changed, 376 insertions(+), 1 deletion(-)
create mode 100644 
tests/qemuxml2argvdata/hostdev-mdev-display-missing-graphics.xml
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 48e224cabc..5f6b340f8f 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5207,6 +5207,26 @@ 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 need to perform
+ * some additional checks here */
+if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_PCI_DISPLAY)) {
+if (mdevsrc->display != VIR_TRISTATE_SWITCH_ABSENT) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("display property of device vfio-pci is "
+ "not supported by this version of QEMU"));
+goto cleanup;
+}


qemuCaps checks belong to *Validate functions.


+} else {
+ /* we default to 'display=off', since QEMU defaults to 'auto' which is
+  * unreliable and we don't want to risk any breakages */
+if (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;




@@ -5424,7 +5444,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",
@@ -5432,6 +5454,7 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
 "supported by this version of QEMU"));
return -1;
}
+
break;
case VIR_MDEV_MODEL_TYPE_VFIO_CCW:
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_CCW)) {


These two hunks are unrelated. Feel free to push them as trivial in a
separate commit.


diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 6014c81802..a0ab824038 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1590,6 +1590,37 @@ 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",
+

[libvirt] [PATCH v3 7/8] qemu: command: Enable formatting vfio-pci.display option onto cmdline

2018-07-11 Thread Erik Skultety 
Since QEMU 2.12, QEMU understands a new vfio-pci device option 'display'
which can be used to turn on display capabilities on vgpu-enabled
mediated devices, IOW emulated GPU devices like QXL will no longer be
needed with vgpu-enable mdevs.
QEMU defaults to 'auto' for the 'display' attribute, which is not
foolproof, so we need to play it safe here and explicitly format
display='off' if this attribute wasn't provided in the XML explicitly.

Signed-off-by: Erik Skultety 
---
 src/qemu/qemu_command.c| 25 -
 .../hostdev-mdev-display-missing-graphics.xml  | 35 ++
 .../hostdev-mdev-display-spice-egl-headless.args   | 32 +
 .../hostdev-mdev-display-spice-egl-headless.xml| 40 +
 .../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  | 40 +
 .../qemuxml2argvdata/hostdev-mdev-display-vnc.args | 31 
 .../qemuxml2argvdata/hostdev-mdev-display-vnc.xml  | 39 
 tests/qemuxml2argvtest.c   | 31 
 11 files changed, 376 insertions(+), 1 deletion(-)
 create mode 100644 
tests/qemuxml2argvdata/hostdev-mdev-display-missing-graphics.xml
 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 48e224cabc..5f6b340f8f 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5207,6 +5207,26 @@ 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 need to perform
+ * some additional checks here */
+if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_PCI_DISPLAY)) {
+if (mdevsrc->display != VIR_TRISTATE_SWITCH_ABSENT) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("display property of device vfio-pci is "
+ "not supported by this version of QEMU"));
+goto cleanup;
+}
+} else {
+ /* we default to 'display=off', since QEMU defaults to 'auto' which is
+  * unreliable and we don't want to risk any breakages */
+if (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;
 
@@ -5424,7 +5444,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",
@@ -5432,6 +5454,7 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
  "supported by this version of QEMU"));
 return -1;
 }
+
 break;
 case VIR_MDEV_MODEL_TYPE_VFIO_CCW:
 if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_CCW)) {
diff --git a/tests/qemuxml2argvdata/hostdev-mdev-display-missing-graphics.xml 
b/tests/qemuxml2argvdata/hostdev-mdev-display-missing-graphics.xml
new file mode 100644
index 00..ea559a6444
--- /dev/null
+++ b/tests/qemuxml2argvdata/hostdev-mdev-display-missing-graphics.xml
@@ -0,0 +1,35 @@
+
+  QEMUGuest2
+  c7a5fdbd-edaf-9455-926a-d65c16db1809
+  219136
+  219136
+  1
+  
+hvm
+
+  
+  
+  destroy
+  restart
+  destroy
+  
+/usr/bin/qemu-system-i686
+
+  
+  
+  
+  
+
+
+
+
+
+
+
+  
+