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

Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx> · Fri, 29 Jan 2021 11:08:50 +0100

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@xxxxxxxxxxxxx>
---
  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@xxxxxxxxxx>

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








