Re: [libvirt] [PATCH v2 2/3] conf: Introduce new video type 'none'

2018-07-18 Thread Erik Skultety 
On Fri, Jul 13, 2018 at 01:56:32PM +0200, Ján Tomko wrote:
> On Thu, Jul 12, 2018 at 05:08:47PM +0200, Erik Skultety wrote:
> > Historically, we've always enabled an emulated video device every time we
> > see that graphics should be supported with a guest. With the appearance
> > of mediated devices which can support QEMU's vfio-display capability,
> > users might want to use such a device as the only video device.
> > Therefore introduce a new, effectively a 'disable', type for video
> > device.
> >
> > Signed-off-by: Erik Skultety 
> > ---
> > docs/formatdomain.html.in  | 13 -
> > docs/schemas/domaincommon.rng  |  1 +
> > src/conf/domain_conf.c | 55 
> > --
> > src/conf/domain_conf.h |  1 +
> > src/qemu/qemu_command.c| 14 --
> > src/qemu/qemu_domain.c |  3 ++
> > src/qemu/qemu_domain_address.c | 10 
> > tests/domaincapsschemadata/full.xml|  1 +
> > .../video-invalid-multiple-devices.xml | 33 +
> > tests/qemuxml2argvdata/video-none-device.args  | 27 +++
> > tests/qemuxml2argvdata/video-none-device.xml   | 39 +++
> > tests/qemuxml2argvtest.c   |  4 +-
> > tests/qemuxml2xmloutdata/video-none-device.xml | 42 +
> > tests/qemuxml2xmltest.c|  1 +
> > 14 files changed, 223 insertions(+), 21 deletions(-)
> > create mode 100644 tests/qemuxml2argvdata/video-invalid-multiple-devices.xml
> > create mode 100644 tests/qemuxml2argvdata/video-none-device.args
> > create mode 100644 tests/qemuxml2argvdata/video-none-device.xml
> > create mode 100644 tests/qemuxml2xmloutdata/video-none-device.xml
> >
> > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> > index 84ab5a9d12..03d94f0533 100644
> > --- a/docs/formatdomain.html.in
> > +++ b/docs/formatdomain.html.in
> > @@ -6622,9 +6622,18 @@ qemu-kvm -net nic,model=? /dev/null
> >   The model element has a mandatory type
> >   attribute which takes the value "vga", "cirrus", "vmvga", "xen",
> >   "vbox", "qxl" (since 0.8.6),
> > -  "virtio" (since 1.3.0)
> > -  or "gop" (since 3.2.0)
> > +  "virtio" (since 1.3.0),
> > +  "gop" (since 3.2.0), or
> > +  "none" (since 4.6.0)
> >   depending on the hypervisor features available.
> > +  The purpose of the type none is to instruct libvirt 
> > not
> > +  to add a default video device in the guest (see the paragraph 
> > above).
> > +  This legacy behaviour can be inconvenient in cases where GPU 
> > mediated
> > +  devices are meant to be the only rendering device within a guest 
> > and
> > +  so specifying another video device along with type
> > +  none.
> > +  Refer to Host device assignment to 
> > see
> > +  how to add a mediated device into a guest.
> > 
> > 
> >   You can provide the amount of video memory in kibibytes (blocks of
> > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> > index bd687ce9d3..ed989c000f 100644
> > --- a/docs/schemas/domaincommon.rng
> > +++ b/docs/schemas/domaincommon.rng
> > @@ -3451,6 +3451,7 @@
> > vbox
> > virtio
> > gop
> > +none
> >   
> > 
> > 
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 7396616eda..d4927f0226 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -590,7 +590,8 @@ VIR_ENUM_IMPL(virDomainVideo, 
> > VIR_DOMAIN_VIDEO_TYPE_LAST,
> >   "qxl",
> >   "parallels",
> >   "virtio",
> > -  "gop")
> > +  "gop",
> > +  "none")
> >
> > VIR_ENUM_IMPL(virDomainVideoVGAConf, VIR_DOMAIN_VIDEO_VGACONF_LAST,
> >   "io",
> > @@ -5091,25 +5092,48 @@ static int
> > virDomainDefPostParseVideo(virDomainDefPtr def,
> >void *opaque)
> > {
> > +size_t i;
> > +
> > if (def->nvideos == 0)
> > return 0;
> >
> > -virDomainDeviceDef device = {
> > -.type = VIR_DOMAIN_DEVICE_VIDEO,
> > -.data.video = def->videos[0],
> > -};
> > -
> > -/* Mark the first video as primary. If the user specified
> > - * primary="yes", the parser already inserted the device at
> > - * def->videos[0]
> > +/* it doesn't make sense to pair video device type 'none' with any 
> > other
> > + * types, there can be only a single video device in such case
> >  */
> > -def->videos[0]->primary = true;
> > +for (i = 0; i < def->nvideos; i++) {
> > +if (def->videos[i]->type == VIR_DOMAIN_VIDEO_TYPE_NONE &&
> > +def->nvideos > 1) {
> > +

Re: [libvirt] [PATCH v2 2/3] conf: Introduce new video type 'none'

2018-07-13 Thread Ján Tomko 

On Thu, Jul 12, 2018 at 05:08:47PM +0200, Erik Skultety wrote:

Historically, we've always enabled an emulated video device every time we
see that graphics should be supported with a guest. With the appearance
of mediated devices which can support QEMU's vfio-display capability,
users might want to use such a device as the only video device.
Therefore introduce a new, effectively a 'disable', type for video
device.

Signed-off-by: Erik Skultety 
---
docs/formatdomain.html.in  | 13 -
docs/schemas/domaincommon.rng  |  1 +
src/conf/domain_conf.c | 55 --
src/conf/domain_conf.h |  1 +
src/qemu/qemu_command.c| 14 --
src/qemu/qemu_domain.c |  3 ++
src/qemu/qemu_domain_address.c | 10 
tests/domaincapsschemadata/full.xml|  1 +
.../video-invalid-multiple-devices.xml | 33 +
tests/qemuxml2argvdata/video-none-device.args  | 27 +++
tests/qemuxml2argvdata/video-none-device.xml   | 39 +++
tests/qemuxml2argvtest.c   |  4 +-
tests/qemuxml2xmloutdata/video-none-device.xml | 42 +
tests/qemuxml2xmltest.c|  1 +
14 files changed, 223 insertions(+), 21 deletions(-)
create mode 100644 tests/qemuxml2argvdata/video-invalid-multiple-devices.xml
create mode 100644 tests/qemuxml2argvdata/video-none-device.args
create mode 100644 tests/qemuxml2argvdata/video-none-device.xml
create mode 100644 tests/qemuxml2xmloutdata/video-none-device.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 84ab5a9d12..03d94f0533 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -6622,9 +6622,18 @@ qemu-kvm -net nic,model=? /dev/null
  The model element has a mandatory type
  attribute which takes the value "vga", "cirrus", "vmvga", "xen",
  "vbox", "qxl" (since 0.8.6),
-  "virtio" (since 1.3.0)
-  or "gop" (since 3.2.0)
+  "virtio" (since 1.3.0),
+  "gop" (since 3.2.0), or
+  "none" (since 4.6.0)
  depending on the hypervisor features available.
+  The purpose of the type none is to instruct libvirt not
+  to add a default video device in the guest (see the paragraph above).
+  This legacy behaviour can be inconvenient in cases where GPU mediated
+  devices are meant to be the only rendering device within a guest and
+  so specifying another video device along with type
+  none.
+  Refer to Host device assignment to see
+  how to add a mediated device into a guest.


  You can provide the amount of video memory in kibibytes (blocks of
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index bd687ce9d3..ed989c000f 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -3451,6 +3451,7 @@
vbox
virtio
gop
+none
  


diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 7396616eda..d4927f0226 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -590,7 +590,8 @@ VIR_ENUM_IMPL(virDomainVideo, VIR_DOMAIN_VIDEO_TYPE_LAST,
  "qxl",
  "parallels",
  "virtio",
-  "gop")
+  "gop",
+  "none")

VIR_ENUM_IMPL(virDomainVideoVGAConf, VIR_DOMAIN_VIDEO_VGACONF_LAST,
  "io",
@@ -5091,25 +5092,48 @@ static int
virDomainDefPostParseVideo(virDomainDefPtr def,
   void *opaque)
{
+size_t i;
+
if (def->nvideos == 0)
return 0;

-virDomainDeviceDef device = {
-.type = VIR_DOMAIN_DEVICE_VIDEO,
-.data.video = def->videos[0],
-};
-
-/* Mark the first video as primary. If the user specified
- * primary="yes", the parser already inserted the device at
- * def->videos[0]
+/* it doesn't make sense to pair video device type 'none' with any other
+ * types, there can be only a single video device in such case
 */
-def->videos[0]->primary = true;
+for (i = 0; i < def->nvideos; i++) {
+if (def->videos[i]->type == VIR_DOMAIN_VIDEO_TYPE_NONE &&
+def->nvideos > 1) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("a '%s' video type must be the only video device "
+ "defined for the domain"));
+return -1;
+}
+}


This looks like something virDomainDefValidate should do.



-/* videos[0] might have been added in AddImplicitDevices, after we've
- * done the per-device post-parse */
-if (virDomainDefPostParseDeviceIterator(def, ,
-

[libvirt] [PATCH v2 2/3] conf: Introduce new video type 'none'

2018-07-12 Thread Erik Skultety 
Historically, we've always enabled an emulated video device every time we
see that graphics should be supported with a guest. With the appearance
of mediated devices which can support QEMU's vfio-display capability,
users might want to use such a device as the only video device.
Therefore introduce a new, effectively a 'disable', type for video
device.

Signed-off-by: Erik Skultety 
---
 docs/formatdomain.html.in  | 13 -
 docs/schemas/domaincommon.rng  |  1 +
 src/conf/domain_conf.c | 55 --
 src/conf/domain_conf.h |  1 +
 src/qemu/qemu_command.c| 14 --
 src/qemu/qemu_domain.c |  3 ++
 src/qemu/qemu_domain_address.c | 10 
 tests/domaincapsschemadata/full.xml|  1 +
 .../video-invalid-multiple-devices.xml | 33 +
 tests/qemuxml2argvdata/video-none-device.args  | 27 +++
 tests/qemuxml2argvdata/video-none-device.xml   | 39 +++
 tests/qemuxml2argvtest.c   |  4 +-
 tests/qemuxml2xmloutdata/video-none-device.xml | 42 +
 tests/qemuxml2xmltest.c|  1 +
 14 files changed, 223 insertions(+), 21 deletions(-)
 create mode 100644 tests/qemuxml2argvdata/video-invalid-multiple-devices.xml
 create mode 100644 tests/qemuxml2argvdata/video-none-device.args
 create mode 100644 tests/qemuxml2argvdata/video-none-device.xml
 create mode 100644 tests/qemuxml2xmloutdata/video-none-device.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 84ab5a9d12..03d94f0533 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -6622,9 +6622,18 @@ qemu-kvm -net nic,model=? /dev/null
   The model element has a mandatory type
   attribute which takes the value "vga", "cirrus", "vmvga", "xen",
   "vbox", "qxl" (since 0.8.6),
-  "virtio" (since 1.3.0)
-  or "gop" (since 3.2.0)
+  "virtio" (since 1.3.0),
+  "gop" (since 3.2.0), or
+  "none" (since 4.6.0)
   depending on the hypervisor features available.
+  The purpose of the type none is to instruct libvirt not
+  to add a default video device in the guest (see the paragraph above).
+  This legacy behaviour can be inconvenient in cases where GPU mediated
+  devices are meant to be the only rendering device within a guest and
+  so specifying another video device along with type
+  none.
+  Refer to Host device assignment to see
+  how to add a mediated device into a guest.
 
 
   You can provide the amount of video memory in kibibytes (blocks of
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index bd687ce9d3..ed989c000f 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -3451,6 +3451,7 @@
 vbox
 virtio
 gop
+none
   
 
 
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 7396616eda..d4927f0226 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -590,7 +590,8 @@ VIR_ENUM_IMPL(virDomainVideo, VIR_DOMAIN_VIDEO_TYPE_LAST,
   "qxl",
   "parallels",
   "virtio",
-  "gop")
+  "gop",
+  "none")
 
 VIR_ENUM_IMPL(virDomainVideoVGAConf, VIR_DOMAIN_VIDEO_VGACONF_LAST,
   "io",
@@ -5091,25 +5092,48 @@ static int
 virDomainDefPostParseVideo(virDomainDefPtr def,
void *opaque)
 {
+size_t i;
+
 if (def->nvideos == 0)
 return 0;
 
-virDomainDeviceDef device = {
-.type = VIR_DOMAIN_DEVICE_VIDEO,
-.data.video = def->videos[0],
-};
-
-/* Mark the first video as primary. If the user specified
- * primary="yes", the parser already inserted the device at
- * def->videos[0]
+/* it doesn't make sense to pair video device type 'none' with any other
+ * types, there can be only a single video device in such case
  */
-def->videos[0]->primary = true;
+for (i = 0; i < def->nvideos; i++) {
+if (def->videos[i]->type == VIR_DOMAIN_VIDEO_TYPE_NONE &&
+def->nvideos > 1) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("a '%s' video type must be the only video device "
+ "defined for the domain"));
+return -1;
+}
+}
 
-/* videos[0] might have been added in AddImplicitDevices, after we've
- * done the per-device post-parse */
-if (virDomainDefPostParseDeviceIterator(def, ,
-NULL, opaque) < 0)
-return -1;
+if (def->videos[0]->type ==