1. Patchew
  2. Libvirt
  3. View series

[PATCH] qemu: Add virtio related options to vsock

Boris Fiuczynski posted 1 patch 3 years, 2 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20210127184659.30126-1-fiuczy@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
[PATCH] qemu: Add virtio related options to vsock
Posted by Boris Fiuczynski 3 years, 2 months ago 
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@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;
+
+    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))
+        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>
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 9553a8a4f8..50dd970789 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -1403,6 +1403,8 @@ mymain(void)
             QEMU_CAPS_CCW);
     DO_TEST("vhost-vsock-ccw-auto", QEMU_CAPS_DEVICE_VHOST_VSOCK,
             QEMU_CAPS_CCW);
+    DO_TEST_CAPS_ARCH_LATEST("vhost-vsock-ccw-iommu", "s390x");
+
 
     DO_TEST_CAPS_LATEST("vhost-user-fs-fd-memory");
     DO_TEST_CAPS_LATEST("vhost-user-fs-hugepages");
-- 
2.26.2
Re: [PATCH] qemu: Add virtio related options to vsock
Posted by Michal Privoznik 3 years, 2 months ago 
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@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@redhat.com>

Michal
Re: [PATCH] qemu: Add virtio related options to vsock
Posted by Boris Fiuczynski 3 years, 2 months ago 
On 1/28/21 1:42 PM, Michal Privoznik wrote:
> 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@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@redhat.com>
> 
> Michal
> 

Hi Michal,
thanks for your review.
The elements cpu and controller get autogenerated.
You are right that this does create a cross feature test.
Therefore your proposed change is a the correct thing to do.

Regarding your other two comments:
Do I understand you correctly that you accept the two changes as 
pre-existing 'style' and will refactor these validations with a follow 
up cleanup patch?

-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [PATCH] qemu: Add virtio related options to vsock
Posted by Michal Privoznik 3 years, 2 months ago 
On 1/29/21 11:08 AM, Boris Fiuczynski wrote:
> On 1/28/21 1:42 PM, Michal Privoznik wrote:
>> 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@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

> 
> Hi Michal,
> thanks for your review.
> The elements cpu and controller get autogenerated.
> You are right that this does create a cross feature test.
> Therefore your proposed change is a the correct thing to do.
> 
> Regarding your other two comments:
> Do I understand you correctly that you accept the two changes as 
> pre-existing 'style' and will refactor these validations with a follow 
> up cleanup patch?
> 

Yes, I've posted cleanup patches here:

https://www.redhat.com/archives/libvir-list/2021-January/msg01182.html

Now the only question is which patch is merged first :-) Anyway, I can 
do the change locally before pushing your patch (if mine gets merged 
earlier).

Basically, I only wanted you to confirm that you're okay with changes 
I'm proposing.

Michal
Re: [PATCH] qemu: Add virtio related options to vsock
Posted by Michal Privoznik 3 years, 2 months ago 
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@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

Alright, I've made all the changes as discussed and pushed.

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal
Re: [PATCH] qemu: Add virtio related options to vsock
Posted by Boris Fiuczynski 3 years, 2 months ago 
On 1/29/21 12:27 PM, Michal Privoznik wrote:
> 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@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
> 
> Alright, I've made all the changes as discussed and pushed.
> 
> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
> 
> Michal
> 

Thanks, Michal!

-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.