Re: [PATCH] qemu: Add virtio related options to vsock

On 1/27/21 7:46 PM, Boris Fiuczynski wrote: > Add virtio related options iommu, ats and packed as driver element > attributes > to vsock devices. Ex: > >   <vsock model='virtio'> >     <cid auto='no' address='3'/> >     <driver iommu='on'/> >   </vsock> > > Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; > --- >   docs/formatdomain.rst                         |  2 + >   docs/schemas/domaincommon.rng                 |  5 +++ >   src/conf/domain_conf.c                        | 34 +++++++++++++-- >   src/conf/domain_conf.h                        |  1 + >   src/qemu/qemu_command.c                       |  3 ++ >   src/qemu/qemu_validate.c                      |  3 ++ >   .../vhost-vsock-ccw-iommu.s390x-latest.args   | 42 +++++++++++++++++++ >   .../vhost-vsock-ccw-iommu.xml                 | 33 +++++++++++++++ >   tests/qemuxml2argvtest.c                      |  1 + >   .../vhost-vsock-ccw-iommu.s390x-latest.xml    | 37 ++++++++++++++++ >   tests/qemuxml2xmltest.c                       |  2 + >   11 files changed, 160 insertions(+), 3 deletions(-) >   create mode 100644 > tests/qemuxml2argvdata/vhost-vsock-ccw-iommu.s390x-latest.args >   create mode 100644 tests/qemuxml2argvdata/vhost-vsock-ccw-iommu.xml >   create mode 100644 > tests/qemuxml2xmloutdata/vhost-vsock-ccw-iommu.s390x-latest.xml > > diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst > index c738078b90..a09868bed5 100644 > --- a/docs/formatdomain.rst > +++ b/docs/formatdomain.rst > @@ -7433,6 +7433,8 @@ devices <#elementsVirtioTransitional>`__ for > more details. The optional >   attribute ``address`` of the ``cid`` element specifies the CID > assigned to the >   guest. If the attribute ``auto`` is set to ``yes``, libvirt will > assign a free >   CID automatically on domain startup. :since:`Since 4.4.0` > +The optional ``driver`` element allows to specify virtio options, see > +`Virtio-specific options <#elementsVirtio>`__  for more details. > :since:`Since 7.1.0` >   :: > diff --git a/docs/schemas/domaincommon.rng > b/docs/schemas/domaincommon.rng > index a4bddcf132..232587e690 100644 > --- a/docs/schemas/domaincommon.rng > +++ b/docs/schemas/domaincommon.rng > @@ -4883,6 +4883,11 @@ >           <optional> >             <ref name="alias"/> >           </optional> > +        <optional> > +          <element name="driver"> > +            <ref name="virtioOptions"/> > +          </element> > +        </optional> >         </interleave> >       </element> >     </define> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index dab4f10326..b94204cb4f 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -2457,6 +2457,7 @@ virDomainVsockDefFree(virDomainVsockDefPtr vsock) >       virObjectUnref(vsock->privateData); >       virDomainDeviceInfoClear(&vsock->info); > +    VIR_FREE(vsock->virtio); >       VIR_FREE(vsock); >   } > @@ -5321,7 +5322,16 @@ virDomainNetDefPostParse(virDomainNetDefPtr net) >   } > -static void > +static bool > +virDomainVsockIsVirtioModel(const virDomainVsockDef *vsock) > +{ > +    return (vsock->model == VIR_DOMAIN_VSOCK_MODEL_VIRTIO || > +            vsock->model == > VIR_DOMAIN_VSOCK_MODEL_VIRTIO_TRANSITIONAL || > +            vsock->model == > VIR_DOMAIN_VSOCK_MODEL_VIRTIO_NON_TRANSITIONAL); > +} > + > + > +static int >   virDomainVsockDefPostParse(virDomainVsockDefPtr vsock) >   { >       if (vsock->auto_cid == VIR_TRISTATE_BOOL_ABSENT) { > @@ -5330,6 +5340,12 @@ virDomainVsockDefPostParse(virDomainVsockDefPtr > vsock) >           else >               vsock->auto_cid = VIR_TRISTATE_BOOL_YES; >       } > + > +    if (!virDomainVsockIsVirtioModel(vsock) && > +        virDomainCheckVirtioOptions(vsock->virtio) < 0) > +        return -1; This check should go into validator (virDomainVsockDefValidate()); The idea is that in post parse callbacks we fill in missing info (e.g. generate MAC for an <interface/>), and then in validate callbacks we check whether parsed (and at that point autofilled) configuration makes sense (e.g. if virtio options are set only if model is virtio). But this is pre-existing and I'll post a patch that cleans that up. > + > +    return 0; >   } > @@ -5410,8 +5426,7 @@ > virDomainDeviceDefPostParseCommon(virDomainDeviceDefPtr dev, >           break; >       case VIR_DOMAIN_DEVICE_VSOCK: > -        virDomainVsockDefPostParse(dev->data.vsock); > -        ret = 0; > +        ret = virDomainVsockDefPostParse(dev->data.vsock); >           break; >       case VIR_DOMAIN_DEVICE_MEMORY: > @@ -15711,6 +15726,11 @@ > virDomainVsockDefParseXML(virDomainXMLOptionPtr xmlopt, >       if (virDomainDeviceInfoParseXML(xmlopt, node, &vsock->info, > flags) < 0) >           return NULL; > +    if (virDomainVirtioOptionsParseXML(virXPathNode("./driver", ctxt), > +                                       &vsock->virtio) < 0) > +        return NULL; > + > + >       return g_steal_pointer(&vsock); >   } > @@ -22897,6 +22917,10 @@ > virDomainVsockDefCheckABIStability(virDomainVsockDefPtr src, >           return false; >       } > +    if (src->virtio && dst->virtio && > +        !virDomainVirtioOptionsCheckABIStability(src->virtio, > dst->virtio)) Again, pre-existing, but what if only one from the pair of definitions has ->virtio set? Another item on my cleanup list - move these checks into virDomainVirtioOptionsCheckABIStability() so that it can be called with either (or even both) args == NULL. > +        return false; > + >       if (!virDomainDeviceInfoCheckABIStability(&src->info, &dst->info)) >           return false; > @@ -28087,6 +28111,7 @@ virDomainVsockDefFormat(virBufferPtr buf, >       g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf); >       g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; >       g_auto(virBuffer) cidAttrBuf = VIR_BUFFER_INITIALIZER; > +    g_auto(virBuffer) drvAttrBuf = VIR_BUFFER_INITIALIZER; >       if (vsock->model) { >           virBufferAsprintf(&attrBuf, " model='%s'", > @@ -28103,6 +28128,9 @@ virDomainVsockDefFormat(virBufferPtr buf, >       virDomainDeviceInfoFormat(&childBuf, &vsock->info, 0); > +    virDomainVirtioOptionsFormat(&drvAttrBuf, vsock->virtio); > + > +    virXMLFormatElement(&childBuf, "driver", &drvAttrBuf, NULL); >       virXMLFormatElement(buf, "vsock", &attrBuf, &childBuf); >   } > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h > index 95ad052891..0a5d151150 100644 > --- a/src/conf/domain_conf.h > +++ b/src/conf/domain_conf.h > @@ -2543,6 +2543,7 @@ struct _virDomainVsockDef { >       virTristateBool auto_cid; >       virDomainDeviceInfo info; > +    virDomainVirtioOptionsPtr virtio; >   }; >   struct _virDomainVirtioOptions { > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index 1ec302d4ac..4986ca8b08 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -9733,6 +9733,9 @@ qemuBuildVsockDevStr(virDomainDefPtr def, >       virBufferAsprintf(&buf, ",id=%s", vsock->info.alias); >       virBufferAsprintf(&buf, ",guest-cid=%u", vsock->guest_cid); >       virBufferAsprintf(&buf, ",vhostfd=%s%u", fdprefix, priv->vhostfd); > + > +    qemuBuildVirtioOptionsStr(&buf, vsock->virtio); > + >       if (qemuBuildDeviceAddressStr(&buf, def, &vsock->info, qemuCaps) > < 0) >           return NULL; > diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c > index a060bd98ba..cb9311cb9c 100644 > --- a/src/qemu/qemu_validate.c > +++ b/src/qemu/qemu_validate.c > @@ -4200,6 +4200,9 @@ qemuValidateDomainDeviceDefVsock(const > virDomainVsockDef *vsock, >                                                 "vsock")) >           return -1; > +    if (qemuValidateDomainVirtioOptions(vsock->virtio, qemuCaps) < 0) > +        return -1; > + >       return 0; >   } > diff --git > a/tests/qemuxml2argvdata/vhost-vsock-ccw-iommu.s390x-latest.args > b/tests/qemuxml2argvdata/vhost-vsock-ccw-iommu.s390x-latest.args > new file mode 100644 > index 0000000000..aed32eef25 > --- /dev/null > +++ b/tests/qemuxml2argvdata/vhost-vsock-ccw-iommu.s390x-latest.args > @@ -0,0 +1,42 @@ > +LC_ALL=C \ > +PATH=/bin \ > +HOME=/tmp/lib/domain--1-QEMUGuest1 \ > +USER=test \ > +LOGNAME=test \ > +XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \ > +XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \ > +XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \ > +QEMU_AUDIO_DRV=none \ > +/usr/bin/qemu-system-s390x \ > +-name guest=QEMUGuest1,debug-threads=on \ > +-S \ > +-object secret,id=masterKey0,format=raw,\ > +file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ > +-machine s390-ccw-virtio,accel=tcg,usb=off,dump-guest-core=off,\ > +memory-backend=s390.ram \ > +-cpu qemu \ > +-m 214 \ > +-object memory-backend-ram,id=s390.ram,size=224395264 \ > +-overcommit mem-lock=off \ > +-smp 1,sockets=1,cores=1,threads=1 \ > +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ > +-display none \ > +-no-user-config \ > +-nodefaults \ > +-chardev socket,id=charmonitor,fd=1729,server,nowait \ > +-mon chardev=charmonitor,id=monitor,mode=control \ > +-rtc base=utc \ > +-no-shutdown \ > +-boot strict=on \ > +-blockdev '{"driver":"host_device","filename":"/dev/HostVG/QEMUGuest1",\ > +"node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' > \ > +-blockdev > '{"node-name":"libvirt-1-format","read-only":false,"driver":"raw",\ > +"file":"libvirt-1-storage"}' \ > +-device > virtio-blk-ccw,devno=fe.0.0000,drive=libvirt-1-format,id=virtio-disk0,\ > +bootindex=1 \ > +-device virtio-balloon-ccw,id=balloon0,devno=fe.0.0001 \ > +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ > +resourcecontrol=deny \ > +-device > vhost-vsock-ccw,id=vsock0,guest-cid=4,vhostfd=6789,iommu_platform=on,\ > +devno=fe.0.0002 \ > +-msg timestamp=on > diff --git a/tests/qemuxml2argvdata/vhost-vsock-ccw-iommu.xml > b/tests/qemuxml2argvdata/vhost-vsock-ccw-iommu.xml > new file mode 100644 > index 0000000000..ba9cdc82bf > --- /dev/null > +++ b/tests/qemuxml2argvdata/vhost-vsock-ccw-iommu.xml > @@ -0,0 +1,33 @@ > +<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='s390x' machine='s390-ccw-virtio'>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-s390x</emulator> > +    <disk type='block' device='disk'> > +      <driver name='qemu' type='raw'/> > +      <source dev='/dev/HostVG/QEMUGuest1'/> > +      <target dev='hda' bus='virtio'/> > +      <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0000'/> > +    </disk> > +    <memballoon model='virtio'> > +      <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0001'/> > +    </memballoon> > +    <panic model='s390'/> > +    <vsock model='virtio'> > +      <cid auto='no' address='4'/> > +      <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0002'/> > +      <driver iommu='on'/> > +    </vsock> > +  </devices> > +</domain> > diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c > index cf77224fc3..c5d82ac72e 100644 > --- a/tests/qemuxml2argvtest.c > +++ b/tests/qemuxml2argvtest.c > @@ -3400,6 +3400,7 @@ mymain(void) >       DO_TEST_CAPS_LATEST("vhost-vsock-auto"); >       DO_TEST_CAPS_ARCH_LATEST("vhost-vsock-ccw", "s390x"); >       DO_TEST_CAPS_ARCH_LATEST("vhost-vsock-ccw-auto", "s390x"); > +    DO_TEST_CAPS_ARCH_LATEST("vhost-vsock-ccw-iommu", "s390x"); >       DO_TEST_CAPS_VER("launch-security-sev", "2.12.0"); >       DO_TEST_CAPS_VER("launch-security-sev-missing-platform-info", > "2.12.0"); > diff --git > a/tests/qemuxml2xmloutdata/vhost-vsock-ccw-iommu.s390x-latest.xml > b/tests/qemuxml2xmloutdata/vhost-vsock-ccw-iommu.s390x-latest.xml > new file mode 100644 > index 0000000000..dbfe082a6f > --- /dev/null > +++ b/tests/qemuxml2xmloutdata/vhost-vsock-ccw-iommu.s390x-latest.xml > @@ -0,0 +1,37 @@ > +<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='s390x' machine='s390-ccw-virtio'>hvm</type> > +    <boot dev='hd'/> > +  </os> > +  <cpu mode='custom' match='exact' check='none'> > +    <model fallback='forbid'>qemu</model> > +  </cpu> > +  <clock offset='utc'/> > +  <on_poweroff>destroy</on_poweroff> > +  <on_reboot>restart</on_reboot> > +  <on_crash>destroy</on_crash> > +  <devices> > +    <emulator>/usr/bin/qemu-system-s390x</emulator> > +    <disk type='block' device='disk'> > +      <driver name='qemu' type='raw'/> > +      <source dev='/dev/HostVG/QEMUGuest1'/> > +      <target dev='hda' bus='virtio'/> > +      <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0000'/> > +    </disk> > +    <controller type='pci' index='0' model='pci-root'/> > +    <memballoon model='virtio'> > +      <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0001'/> > +    </memballoon> > +    <panic model='s390'/> > +    <vsock model='virtio'> > +      <cid auto='no' address='4'/> > +      <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0002'/> > +      <driver iommu='on'/> > +    </vsock> > +  </devices> > +</domain> So the difference between tests/qemuxml2argvdata/vhost-vsock-ccw-iommu.xml and tests/qemuxml2xmloutdata/vhost-vsock-ccw-iommu.s390x-latest.xml is very minimal: @@ -10,0 +11,3 @@ +  <cpu mode='custom' match='exact' check='none'> +    <model fallback='forbid'>qemu</model> +  </cpu> @@ -22,0 +26 @@ +    <controller type='pci' index='0' model='pci-root'/> Are okay with me adding these lines into the former .xml and making the latter (-latest.xml) just a symlink to the original .xml? That way we can avoid having two almost identical files stored in git. After all, this feature is not about pci-root or <cpu/>. If so, you can count on my: Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com&gt; Michal