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

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(a)redhat.com&gt; --- 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; + } + }

- /* 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;

+ } 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 */);

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 */);

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; +

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' */

+ 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; + }

+ 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;

if (!virDeviceInfoPCIAddressWanted(&def->videos[i]->info)) continue;