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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux