Re: [PATCH 5/5] conf: Introduce new video type 'none'

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 06/28/2018 08:15 AM, 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                          | 10 +++-
>  docs/schemas/domaincommon.rng                      |  1 +
>  src/conf/domain_conf.c                             | 55 ++++++++++++++++------
>  src/conf/domain_conf.h                             |  1 +
>  src/qemu/qemu_command.c                            | 13 +++--
>  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, 219 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
> 

I assume this has to do with the egl-headless discussion from the other
series, so rather than "none", why not go with headless?  The problem I
see w/ NONE is it's usage/meaning typically as, well, NONE or not
defined; whereas, for graphics it would then mean, well it's defined,
but it's something else.

If this isn't a headless device, then I'll need to revisit my comments
below...

Also please split domain/docs/xml2ml from qemu/xml2argv

> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index f45eee6812..2e8196c21f 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -6639,9 +6639,15 @@ 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.
> +          Note that type <code>"none"</code> is currently only available for
> +          QEMU and the intended use case is to prevent libvirt from adding a
> +          default emulated video card in case a host device like mdev should
> +          handle the rendering, see also
> +          <a href="#elementsGraphics">Graphical framebuffers</a>.

"host device like mdev" - I assume you meant "type"... In any case, not
sure it's possible from the domain/xml2xml viewpoint to say QEMU only -
having it in domain. Perhaps consider something like:

"The <code>headless</code> type is designed to be used in coordination
with a host device graphics co-processor such as is provided via
Mediated Device vGPUs. When using a <code>headless</code> type, it must
be the only graphics device defined for the domain.

You could add a link from mdev to
https://libvirt.org/drvnodedev.html#MDEV just like the host device mdev
description did or a link to the hostdev section. It's too bad there
wasn't an anchor for that directly...



>          </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 1df479cda2..55da8c079b 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -3468,6 +3468,7 @@
>                  <value>vbox</value>
>                  <value>virtio</value>
>                  <value>gop</value>
> +                <value>none</value>

headless

>                </choice>
>              </attribute>
>              <group>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 96ab6cf520..cd2a6c991c 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -589,7 +589,8 @@ VIR_ENUM_IMPL(virDomainVideo, VIR_DOMAIN_VIDEO_TYPE_LAST,
>                "qxl",
>                "parallels",
>                "virtio",
> -              "gop")
> +              "gop",
> +              "none")

"headless"

>  
>  VIR_ENUM_IMPL(virDomainVideoVGAConf, VIR_DOMAIN_VIDEO_VGACONF_LAST,
>                "io",
> @@ -5125,25 +5126,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 &&

_HEADLESS

> +            def->nvideos > 1) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("video device type='none' cannot be paired "
> +                             "with any other video device types"));

Consider, "A '%s' video type must be the only video device defined for
the domain."  virDomainVideoTypeToString(VIR_DOMAIN_VIDEO_TYPE_HEADLESS);


> +            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;
>  }
> @@ -15093,6 +15117,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 6d73a0b5d3..c1970e0f62 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1424,6 +1424,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 a2a27b5b9b..bc798f9d2d 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 */);
>  

I assume these become egl-headless, although I'm not 100% sure based on
Gerd's comment from the other series today.

>  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)
>  
> @@ -4421,6 +4424,10 @@ qemuBuildVideoCommandLine(virCommandPtr cmd,
>          char *str = NULL;
>          virDomainVideoDefPtr video = def->videos[i];
>  
> +        /* no cmdline needed for type 'none' */
> +        if (video->type == VIR_DOMAIN_VIDEO_TYPE_NONE)
> +            return 0;
> +

This could go outside the loop and a direct def->videos[0].type == type
logic, although it's technically fine as is, just makes it "appear" as
if we quit when we find this rather than knowing that it only can occur
at [0]. Of course that's true in qemuDomainAssignDevicePCISlots too I
suppose, so whatever. Just looks strange.

John

>          if (video->primary) {
>              if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY)) {
>  
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 42b7635ef4..51aba8d527 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4444,6 +4444,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 ab2ac022f1..bc9786dd60 100644
> --- a/src/qemu/qemu_domain_address.c
> +++ b/src/qemu/qemu_domain_address.c
> @@ -821,6 +821,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;
>          }
> @@ -1540,6 +1541,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;
> @@ -2105,6 +2113,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;
> diff --git a/tests/domaincapsschemadata/full.xml b/tests/domaincapsschemadata/full.xml
> index d3faf38da0..474df90283 100644
> --- a/tests/domaincapsschemadata/full.xml
> +++ b/tests/domaincapsschemadata/full.xml
> @@ -73,6 +73,7 @@
>          <value>parallels</value>
>          <value>virtio</value>
>          <value>gop</value>
> +        <value>none</value>
>        </enum>
>      </video>
>      <hostdev supported='yes'>
> diff --git a/tests/qemuxml2argvdata/video-invalid-multiple-devices.xml b/tests/qemuxml2argvdata/video-invalid-multiple-devices.xml
> new file mode 100644
> index 0000000000..3f105efaae
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/video-invalid-multiple-devices.xml
> @@ -0,0 +1,33 @@
> +<domain type='qemu'>
> +  <name>QEMUGuest1</name>
> +  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> +  <memory unit='KiB'>219100</memory>
> +  <currentMemory unit='KiB'>219100</currentMemory>
> +  <vcpu placement='static' cpuset='1-4,8-20,525'>1</vcpu>
> +  <os>
> +    <type arch='i686' machine='pc'>hvm</type>
> +    <boot dev='hd'/>
> +  </os>
> +  <clock offset='utc'/>
> +  <on_poweroff>destroy</on_poweroff>
> +  <on_reboot>restart</on_reboot>
> +  <on_crash>destroy</on_crash>
> +  <devices>
> +    <emulator>/usr/bin/qemu-system-i686</emulator>
> +    <disk type='block' device='disk'>
> +      <source dev='/dev/HostVG/QEMUGuest1'/>
> +      <target dev='hda' bus='ide'/>
> +      <address type='drive' controller='0' bus='0' target='0' unit='0'/>
> +    </disk>
> +    <controller type='usb' index='0'/>
> +    <controller type='ide' index='0'/>
> +    <controller type='pci' index='0' model='pci-root'/>
> +    <video>
> +      <model type='qxl'/>
> +    </video>
> +    <video>
> +      <model type='none'/>
> +    </video>
> +    <memballoon model='virtio'/>
> +  </devices>
> +</domain>
> diff --git a/tests/qemuxml2argvdata/video-none-device.args b/tests/qemuxml2argvdata/video-none-device.args
> new file mode 100644
> index 0000000000..1b03c0cb97
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/video-none-device.args
> @@ -0,0 +1,27 @@
> +LC_ALL=C \
> +PATH=/bin \
> +HOME=/home/test \
> +USER=test \
> +LOGNAME=test \
> +QEMU_AUDIO_DRV=none \
> +/usr/bin/qemu-system-i686 \
> +-name QEMUGuest1 \
> +-S \
> +-machine pc,accel=tcg,usb=off,dump-guest-core=off \
> +-m 214 \
> +-smp 1,sockets=1,cores=1,threads=1 \
> +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
> +-no-user-config \
> +-nodefaults \
> +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\
> +server,nowait \
> +-mon chardev=charmonitor,id=monitor,mode=control \
> +-rtc base=utc \
> +-no-shutdown \
> +-no-acpi \
> +-boot c \
> +-usb \
> +-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
> +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
> +-vnc 127.0.0.1:0 \
> +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
> diff --git a/tests/qemuxml2argvdata/video-none-device.xml b/tests/qemuxml2argvdata/video-none-device.xml
> new file mode 100644
> index 0000000000..4b591562b7
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/video-none-device.xml
> @@ -0,0 +1,39 @@
> +<domain type='qemu'>
> +  <name>QEMUGuest1</name>
> +  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> +  <memory unit='KiB'>219136</memory>
> +  <currentMemory unit='KiB'>219136</currentMemory>
> +  <vcpu placement='static'>1</vcpu>
> +  <os>
> +    <type arch='i686' machine='pc'>hvm</type>
> +    <boot dev='hd'/>
> +  </os>
> +  <clock offset='utc'/>
> +  <on_poweroff>destroy</on_poweroff>
> +  <on_reboot>restart</on_reboot>
> +  <on_crash>destroy</on_crash>
> +  <devices>
> +    <emulator>/usr/bin/qemu-system-i686</emulator>
> +    <disk type='block' device='disk'>
> +      <source dev='/dev/HostVG/QEMUGuest1'/>
> +      <target dev='hda' bus='ide'/>
> +      <address type='drive' controller='0' bus='0' target='0' unit='0'/>
> +    </disk>
> +    <controller type='usb' index='0'>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
> +    </controller>
> +    <controller type='ide' index='0'>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
> +    </controller>
> +    <controller type='pci' index='0' model='pci-root'/>
> +    <input type='mouse' bus='ps2'/>
> +    <input type='keyboard' bus='ps2'/>
> +    <graphics type='vnc'/>
> +    <video>
> +      <model type='none'/>
> +    </video>
> +    <memballoon model='virtio'>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
> +    </memballoon>
> +  </devices>
> +</domain>
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 8293be949d..4ed165d76b 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -2010,7 +2010,9 @@ mymain(void)
>              QEMU_CAPS_DEVICE_VIRTIO_VGA,
>              QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
>              QEMU_CAPS_VIRTIO_GPU_MAX_OUTPUTS);
> -    DO_TEST_PARSE_ERROR("video-invalid", NONE);
> +    DO_TEST("video-none-device",
> +            QEMU_CAPS_VNC);
> +    DO_TEST_PARSE_ERROR("video-invalid-multiple-devices", NONE);
>  
>      DO_TEST("virtio-rng-default",
>              QEMU_CAPS_DEVICE_VIRTIO_RNG,
> diff --git a/tests/qemuxml2xmloutdata/video-none-device.xml b/tests/qemuxml2xmloutdata/video-none-device.xml
> new file mode 100644
> index 0000000000..6e76b394fe
> --- /dev/null
> +++ b/tests/qemuxml2xmloutdata/video-none-device.xml
> @@ -0,0 +1,42 @@
> +<domain type='qemu'>
> +  <name>QEMUGuest1</name>
> +  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> +  <memory unit='KiB'>219136</memory>
> +  <currentMemory unit='KiB'>219136</currentMemory>
> +  <vcpu placement='static'>1</vcpu>
> +  <os>
> +    <type arch='i686' machine='pc'>hvm</type>
> +    <boot dev='hd'/>
> +  </os>
> +  <clock offset='utc'/>
> +  <on_poweroff>destroy</on_poweroff>
> +  <on_reboot>restart</on_reboot>
> +  <on_crash>destroy</on_crash>
> +  <devices>
> +    <emulator>/usr/bin/qemu-system-i686</emulator>
> +    <disk type='block' device='disk'>
> +      <driver name='qemu' type='raw'/>
> +      <source dev='/dev/HostVG/QEMUGuest1'/>
> +      <target dev='hda' bus='ide'/>
> +      <address type='drive' controller='0' bus='0' target='0' unit='0'/>
> +    </disk>
> +    <controller type='usb' index='0'>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
> +    </controller>
> +    <controller type='ide' index='0'>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
> +    </controller>
> +    <controller type='pci' index='0' model='pci-root'/>
> +    <input type='mouse' bus='ps2'/>
> +    <input type='keyboard' bus='ps2'/>
> +    <graphics type='vnc' port='-1' autoport='yes'>
> +      <listen type='address'/>
> +    </graphics>
> +    <video>
> +      <model type='none'/>
> +    </video>
> +    <memballoon model='virtio'>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
> +    </memballoon>
> +  </devices>
> +</domain>
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index 0095c27cf6..e482705f0e 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -1143,6 +1143,7 @@ mymain(void)
>              QEMU_CAPS_VIRTIO_GPU_MAX_OUTPUTS,
>              QEMU_CAPS_VNC,
>              QEMU_CAPS_DEVICE_VIRTIO_GPU_CCW);
> +    DO_TEST("video-none-device", NONE);
>  
>      DO_TEST("intel-iommu",
>              QEMU_CAPS_DEVICE_INTEL_IOMMU);
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux