Re: [libvirt] [PATCH 11/11] qemu_command: add support to use virtio as secondary video device

Nothing else to say?! Is this fixing an existing bug making some other patch/fix "more correct".

On 09/30/2016 12:02 PM, Pavel Hrdina wrote: > Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; > --- > docs/formatdomain.html.in | 3 +- > src/qemu/qemu_command.c | 32 ++++++++++++------- > src/qemu/qemu_domain.c | 3 +- > .../qemuxml2argv-video-virtio-gpu-sec.args | 25 +++++++++++++++ > .../qemuxml2argv-video-virtio-gpu-sec.xml | 36 ++++++++++++++++++++++ > tests/qemuxml2argvtest.c | 3 ++ > 6 files changed, 89 insertions(+), 13 deletions(-) > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-sec.args > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-sec.xml > Adding a new .xml without also adding a qemuxml2xmltest for the same xml file? Note that you can "save" space if you make the xml2xml output file be the same as the input file and just create a link to it (like a number of other more recent changes are doing). Also it seems there are two changes happening here - thus separate patches. > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index 1266e9d..f08cd01 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -5641,7 +5641,8 @@ qemu-kvm -net nic,model=? /dev/null > video device in domain xml is the primary one, but the optional > attribute <code>primary</code> (<span class="since">since 1.0.2</span>) > with value 'yes' can be used to mark the primary in cases of multiple > - video device. The non-primary must be type of "qxl". > + video device. The non-primary must be type of "qxl" or > + "virtio" (<span class="since">since 2.4.0</span>). > </p> > </dd> > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index bb8128e..2314b2df 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -114,6 +114,18 @@ VIR_ENUM_IMPL(qemuDeviceVideo, VIR_DOMAIN_VIDEO_TYPE_LAST, > "", /* don't support parallels */ > "virtio-vga"); > > +VIR_ENUM_DECL(qemuDeviceVideoSec) > + > +VIR_ENUM_IMPL(qemuDeviceVideoSec, VIR_DOMAIN_VIDEO_TYPE_LAST, > + "", /* no secondary device for VGA*/ > + "", /* no secondary device for cirrus-vga*/ > + "", /* no secondary device for vmware-svga*/ > + "", /* no device for xen */ > + "", /* don't support vbox */ > + "qxl", > + "", /* don't support parallels */ > + "virtio-gpu-pci"); > + I know "Sec" is shorthand for secondary, but I've also seen it used for "second"... being verbose isn't bad especially when you read the code months later.

> VIR_ENUM_DECL(qemuSoundCodec) > > VIR_ENUM_IMPL(qemuSoundCodec, VIR_DOMAIN_SOUND_CODEC_TYPE_LAST, > @@ -4299,18 +4311,16 @@ qemuBuildDeviceVideoStr(const virDomainDef *def, > virBuffer buf = VIR_BUFFER_INITIALIZER; > const char *model; > > - if (video->primary) { > + if (video->primary && qemuDomainSupportVideoVga(video, qemuCaps)) This alters your logic, but the else condition is assuming secondary and in fact digging into that table... Shouldn't the logic be: if (video->primary) { if (qemuDomainSupportVideoVga() model = qemuDeviceVideoTypeToString(video->type); else model = "virtio-gpu-pci"; } else { model = qemuDeviceVideoSecTypeToString(video->type); }

> model = qemuDeviceVideoTypeToString(video->type); > - if (!model || STREQ(model, "")) { > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > - _("video type %s is not supported with QEMU"), > - virDomainVideoTypeToString(video->type)); > - goto error; > - } > - if (!qemuDomainSupportVideoVga(video, qemuCaps)) > - model = "virtio-gpu-pci"; > - } else { > - model = "qxl"; > + else > + model = qemuDeviceVideoSecTypeToString(video->type); > + > + if (!model || STREQ(model, "")) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("invalid model for video type '%s'"), > + virDomainVideoTypeToString(video->type)); > + goto error; > } > > virBufferAsprintf(&buf, "%s,id=%s", model, video->info.alias); > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > index 79c945a..8218609 100644 > --- a/src/qemu/qemu_domain.c > +++ b/src/qemu/qemu_domain.c > @@ -2404,7 +2404,8 @@ qemuDomainDefValidateVideo(const virDomainDef *def) > video = def->videos[i]; > > if (!video->primary && > - video->type != VIR_DOMAIN_VIDEO_TYPE_QXL) { > + video->type != VIR_DOMAIN_VIDEO_TYPE_QXL && > + video->type != VIR_DOMAIN_VIDEO_TYPE_VIRTIO) { This would seemingly be a different/separate patch wouldn't it?

And I'm not sure which of the remaining checks this belongs with, but I'm assuming it's this change. John > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > _("video type '%s' is only valid as primary " > "video device"), > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-sec.args b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-sec.args > new file mode 100644 > index 0000000..a87c37b > --- /dev/null > +++ b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-sec.args > @@ -0,0 +1,25 @@ > +LC_ALL=C \ > +PATH=/bin \ > +HOME=/home/test \ > +USER=test \ > +LOGNAME=test \ > +QEMU_AUDIO_DRV=none \ > +/usr/bin/qemu \ > +-name QEMUGuest1 \ > +-S \ > +-M pc \ > +-m 1024 \ > +-smp 1,sockets=1,cores=1,threads=1 \ > +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ > +-nographic \ > +-nodefaults \ > +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \ > +-no-acpi \ > +-boot c \ > +-usb \ > +-drive file=/var/lib/libvirt/images/QEMUGuest1,format=qcow2,if=none,\ > +id=drive-ide0-0-0,cache=none \ > +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ > +-device virtio-gpu-pci,id=video0,bus=pci.0,addr=0x2 \ > +-device virtio-gpu-pci,id=video1,bus=pci.0,addr=0x4 \ > +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-sec.xml b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-sec.xml > new file mode 100644 > index 0000000..feba717 > --- /dev/null > +++ b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-sec.xml > @@ -0,0 +1,36 @@ > +<domain type='qemu'> > + <name>QEMUGuest1</name> > + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> > + <memory unit='KiB'>1048576</memory> > + <currentMemory unit='KiB'>1048576</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</emulator> > + <disk type='file' device='disk'> > + <driver name='qemu' type='qcow2' cache='none'/> > + <source file='/var/lib/libvirt/images/QEMUGuest1'/> > + <target dev='hda' bus='ide'/> > + <address type='drive' controller='0' bus='0' target='0' unit='0'/> > + </disk> > + <controller type='ide' index='0'/> > + <controller type='usb' index='0'/> > + <controller type='pci' index='0' model='pci-root'/> > + <input type='mouse' bus='ps2'/> > + <input type='keyboard' bus='ps2'/> > + <video> > + <model type='virtio' heads='1' primary='yes'/> > + </video> > + <video> > + <model type='virtio' heads='1'/> > + </video> > + <memballoon model='virtio'/> > + </devices> > +</domain> > diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c > index 4cbe57e..d8bcf93 100644 > --- a/tests/qemuxml2argvtest.c > +++ b/tests/qemuxml2argvtest.c > @@ -1616,6 +1616,9 @@ mymain(void) > QEMU_CAPS_SPICE, > QEMU_CAPS_SPICE_GL, > QEMU_CAPS_DEVICE_VIDEO_PRIMARY); > + DO_TEST("video-virtio-gpu-sec", > + QEMU_CAPS_DEVICE_VIRTIO_GPU, > + QEMU_CAPS_DEVICE_VIDEO_PRIMARY); > DO_TEST("video-virtio-vga", > QEMU_CAPS_DEVICE_VIRTIO_GPU, > QEMU_CAPS_DEVICE_VIRTIO_VGA, > -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list