[libvirt] [PATCH v2 2/3] conf: Introduce new video type 'none'
Ján Tomko
jtomko at redhat.com
Fri Jul 13 11:56:32 UTC 2018
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 at redhat.com>
>---
> 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 at redhat.com>
Jano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180713/71c9fa34/attachment-0001.sig>
More information about the libvir-list
mailing list