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 <eskultet@xxxxxxxxxx>
> > ---
> > 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 <code>model</code> element has a mandatory <code>type</code>
> > attribute which takes the value "vga", "cirrus", "vmvga", "xen",
> > "vbox", "qxl" (<span class="since">since 0.8.6</span>),
> > - "virtio" (<span class="since">since 1.3.0</span>)
> > - or "gop" (<span class="since">since 3.2.0</span>)
> > + "virtio" (<span class="since">since 1.3.0</span>),
> > + "gop" (<span class="since">since 3.2.0</span>), or
> > + "none" (<span class="since">since 4.6.0</span>)
> > depending on the hypervisor features available.
> > + The purpose of the type <code>none</code> 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 <code>video</code> device along with type
> > + <code>none</code>.
> > + Refer to <a id="elementsHostDev">Host device assignment</a> to see
> > + how to add a mediated device into a guest.
> > </p>
> > <p>
> > 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 @@
> > <value>vbox</value>
> > <value>virtio</value>
> > <value>gop</value>
> > + <value>none</value>
> > </choice>
> > </attribute>
> > <group>
> > 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, &device,
> > - NULL, opaque) < 0)
> > - return -1;
> > + if (def->videos[0]->type == VIR_DOMAIN_VIDEO_TYPE_NONE) {
> > + /* we don't want to format any values we automatically fill in for
> > + * videos into the XML, so clear them
> > + */
> > + virDomainVideoDefClear(def->videos[0]);
> > + def->videos[0]->type = VIR_DOMAIN_VIDEO_TYPE_NONE;
>
> Is there anything else than 'heads = 1' we fill in by default?
> If we clear other data here, it's because the user specified it, isn't
> it?
>
> Ideally, the parser would only fill in the data that was explicitly
> specified in the XML input and defaults would be filled in here, but
> this will do until we fix the parser.
>
> > + } else {
> > + 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]
> > + */
> > + def->videos[0]->primary = true;
> > +
> > + /* videos[0] might have been added in AddImplicitDevices, after we've
> > + * done the per-device post-parse */
> > + if (virDomainDefPostParseDeviceIterator(def, &device,
> > + NULL, opaque) < 0)
> > + return -1;
> > + }
> >
> > return 0;
> > }
> > @@ -15023,6 +15047,7 @@ virDomainVideoDefaultRAM(const virDomainDef *def,
> > case VIR_DOMAIN_VIDEO_TYPE_PARALLELS:
> > case VIR_DOMAIN_VIDEO_TYPE_VIRTIO:
> > case VIR_DOMAIN_VIDEO_TYPE_GOP:
> > + case VIR_DOMAIN_VIDEO_TYPE_NONE:
> > case VIR_DOMAIN_VIDEO_TYPE_LAST:
> > default:
> > return 0;
> > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> > index 0f10e242fd..fe2fcdc081 100644
> > --- a/src/conf/domain_conf.h
> > +++ b/src/conf/domain_conf.h
> > @@ -1423,6 +1423,7 @@ typedef enum {
> > VIR_DOMAIN_VIDEO_TYPE_PARALLELS, /* pseudo device for VNC in containers */
> > VIR_DOMAIN_VIDEO_TYPE_VIRTIO,
> > VIR_DOMAIN_VIDEO_TYPE_GOP,
> > + VIR_DOMAIN_VIDEO_TYPE_NONE,
> >
> > VIR_DOMAIN_VIDEO_TYPE_LAST
> > } virDomainVideoType;
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index 44ae8dcef7..545b979a6f 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -105,7 +105,8 @@ VIR_ENUM_IMPL(qemuVideo, VIR_DOMAIN_VIDEO_TYPE_LAST,
> > "qxl",
> > "", /* don't support parallels */
> > "", /* no need for virtio */
> > - "" /* don't support gop */);
> > + "" /* don't support gop */,
> > + "none" /* no display */);
>
> This device is not formatted on the command line, so "" should be
> enough.
>
> >
> > VIR_ENUM_DECL(qemuDeviceVideo)
> >
> > @@ -119,7 +120,8 @@ VIR_ENUM_IMPL(qemuDeviceVideo, VIR_DOMAIN_VIDEO_TYPE_LAST,
> > "qxl-vga",
> > "", /* don't support parallels */
> > "virtio-vga",
> > - "" /* don't support gop */);
> > + "" /* don't support gop */,
> > + "none" /* no display */);
>
> Same here.
>
> >
> > VIR_ENUM_DECL(qemuDeviceVideoSecondary)
> >
> > @@ -133,7 +135,8 @@ VIR_ENUM_IMPL(qemuDeviceVideoSecondary, VIR_DOMAIN_VIDEO_TYPE_LAST,
> > "qxl",
> > "", /* don't support parallels */
> > "virtio-gpu",
> > - "" /* don't support gop */);
> > + "" /* don't support gop */,
> > + "" /* 'none' doesn't make sense here */);
> >
> > VIR_ENUM_DECL(qemuSoundCodec)
> >
> > @@ -4447,6 +4450,11 @@ qemuBuildVideoCommandLine(virCommandPtr cmd,
> > {
> > size_t i;
> >
> > + /* no cmdline needed for type 'none' */
> > + if (def->nvideos > 0 &&
> > + def->videos[0]->type == VIR_DOMAIN_VIDEO_TYPE_NONE)
> > + return 0;
> > +
>
> I'd just do:
>
> if (video->type == VIR_DOMAIN_VIDEO_TYPE_NONE)
> continue;
>
> in the loop below - we already verified that this video can only appear
> once and as the only device in the XML.
>
> > for (i = 0; i < def->nvideos; i++) {
> > char *str = NULL;
> > virDomainVideoDefPtr video = def->videos[i];
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index ed76495309..af8c5f8b0b 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -4471,6 +4471,9 @@ static int
> > qemuDomainDeviceDefValidateVideo(const virDomainVideoDef *video)
> > {
> > switch ((virDomainVideoType) video->type) {
> > + case VIR_DOMAIN_VIDEO_TYPE_NONE:
> > + /* nothing to be validated for 'none' */
>
> Strictly speaking this is not true, specifying any attributes for type
> none is nonsense. But we cleared the device definition in postParse already.
>
> > + return 0;
> > case VIR_DOMAIN_VIDEO_TYPE_XEN:
> > case VIR_DOMAIN_VIDEO_TYPE_VBOX:
> > case VIR_DOMAIN_VIDEO_TYPE_PARALLELS:
> > diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> > index 6ea80616af..e045b2627e 100644
> > --- a/src/qemu/qemu_domain_address.c
> > +++ b/src/qemu/qemu_domain_address.c
> > @@ -799,6 +799,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
> >
> > case VIR_DOMAIN_VIDEO_TYPE_DEFAULT:
> > case VIR_DOMAIN_VIDEO_TYPE_GOP:
> > + case VIR_DOMAIN_VIDEO_TYPE_NONE:
> > case VIR_DOMAIN_VIDEO_TYPE_LAST:
> > return 0;
> > }
> > @@ -1518,6 +1519,13 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr def,
> > * at slot 2.
> > */
> > virDomainVideoDefPtr primaryVideo = def->videos[0];
> > +
> > + /* for video type 'none' skip this whole procedure */
> > + if (primaryVideo->type == VIR_DOMAIN_VIDEO_TYPE_NONE) {
> > + ret = 0;
> > + goto cleanup;
> > + }
>
> I'd rather adjust the above condition to:
>
> if (def->nvideos > 0 &&
> def->videos[0]->type != VIR_DOMAIN_VIDEO_TYPE_NONE)
>
> to prevent our auto-assignment code to use 0:0:2.0 unless requested,
> but I cannot estimate the likelihood of someone using <video
> type='none'/> wanting to add one in the future.
>
> > +
> > if (virDeviceInfoPCIAddressWanted(&primaryVideo->info)) {
> > memset(&tmp_addr, 0, sizeof(tmp_addr));
> > tmp_addr.slot = 2;
> > @@ -2083,6 +2091,8 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
> >
> > /* Video devices */
> > for (i = 0; i < def->nvideos; i++) {
> > + if (def->videos[i]->type == VIR_DOMAIN_VIDEO_TYPE_NONE)
> > + break;
>
> continue;
>
> for consistency.
>
> >
> > if (!virDeviceInfoPCIAddressWanted(&def->videos[i]->info))
> > continue;
>
> Reviewed-by: Ján Tomko <jtomko@xxxxxxxxxx>
Thanks, I incorporated your suggestions and pushed the series.
Erik
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
