[PATCH] qemu: Add virtio related options to vsock
Michal Privoznik
mprivozn at redhat.com
Thu Jan 28 12:42:27 UTC 2021
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 at linux.ibm.com>
> ---
> 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 at redhat.com>
Michal
More information about the libvir-list
mailing list