Re: [PATCH 3/4] qemu: move virtio capability validation

Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx> · Mon, 27 Apr 2020 12:10:13 +0200

Looks OK to me.
Reviewed-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx>

On 4/23/20 3:15 PM, Bjoern Walk wrote:
Move capability validation of virtio options from command line
generation to post-parse device validation where it belongs.

Signed-off-by: Bjoern Walk <bwalk@xxxxxxxxxxxxx>
---
  src/qemu/qemu_command.c  | 41 ++++++--------------
  src/qemu/qemu_validate.c | 70 +++++++++++++++++++++++++++++++--
  tests/qemuxml2argvtest.c | 84 ++++++++++++++++++++--------------------
  3 files changed, 120 insertions(+), 75 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 95402fc4..ca9d3f10 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -588,39 +588,20 @@ qemuBuildVirtioDevStr(virBufferPtr buf,
  
  static int
  qemuBuildVirtioOptionsStr(virBufferPtr buf,
-                          virDomainVirtioOptionsPtr virtio,
-                          virQEMUCapsPtr qemuCaps)
+                          virDomainVirtioOptionsPtr virtio)
  {
      if (!virtio)
          return 0;
  
      if (virtio->iommu != VIR_TRISTATE_SWITCH_ABSENT) {
-        if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_PCI_IOMMU_PLATFORM)) {
-            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                           _("the iommu setting is not supported "
-                             "with this QEMU binary"));
-            return -1;
-        }
          virBufferAsprintf(buf, ",iommu_platform=%s",
                            virTristateSwitchTypeToString(virtio->iommu));
      }
      if (virtio->ats != VIR_TRISTATE_SWITCH_ABSENT) {
-        if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_PCI_ATS)) {
-            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                           _("the ats setting is not supported with this "
-                             "QEMU binary"));
-            return -1;
-        }
          virBufferAsprintf(buf, ",ats=%s",
                            virTristateSwitchTypeToString(virtio->ats));
      }
      if (virtio->packed != VIR_TRISTATE_SWITCH_ABSENT) {
-        if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_PACKED_QUEUES)) {
-            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                           _("the packed setting is not supported with this "
-                             "QEMU binary"));
-            return -1;
-        }
          virBufferAsprintf(buf, ",packed=%s",
                            virTristateSwitchTypeToString(virtio->packed));
      }
@@ -2150,7 +2131,7 @@ qemuBuildDiskDeviceStr(const virDomainDef *def,
              virBufferAsprintf(&opt, ",num-queues=%u", disk->queues);
          }
  
-        if (qemuBuildVirtioOptionsStr(&opt, disk->virtio, qemuCaps) < 0)
+        if (qemuBuildVirtioOptionsStr(&opt, disk->virtio) < 0)
              return NULL;
  
          if (qemuBuildDeviceAddressStr(&opt, def, &disk->info, qemuCaps) < 0)
@@ -2615,7 +2596,7 @@ qemuBuildVHostUserFsCommandLine(virCommandPtr cmd,
          virBufferAsprintf(&opt, ",queue-size=%llu", fs->queue_size);
      virBufferAddLit(&opt, ",tag=");
      virQEMUBuildBufferEscapeComma(&opt, fs->dst);
-    if (qemuBuildVirtioOptionsStr(&opt, fs->virtio, priv->qemuCaps) < 0)
+    if (qemuBuildVirtioOptionsStr(&opt, fs->virtio) < 0)
          return -1;
  
      if (qemuBuildDeviceAddressStr(&opt, def, &fs->info, priv->qemuCaps) < 0)
@@ -2685,7 +2666,7 @@ qemuBuildFSDevStr(const virDomainDef *def,
      virBufferAddLit(&opt, ",mount_tag=");
      virQEMUBuildBufferEscapeComma(&opt, fs->dst);
  
-    if (qemuBuildVirtioOptionsStr(&opt, fs->virtio, qemuCaps) < 0)
+    if (qemuBuildVirtioOptionsStr(&opt, fs->virtio) < 0)
          return NULL;
  
      if (qemuBuildDeviceAddressStr(&opt, def, &fs->info, qemuCaps) < 0)
@@ -2917,7 +2898,7 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
                                    def->iothread);
              }
  
-            if (qemuBuildVirtioOptionsStr(&buf, def->virtio, qemuCaps) < 0)
+            if (qemuBuildVirtioOptionsStr(&buf, def->virtio) < 0)
                  return -1;
              break;
          case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC:
@@ -2964,7 +2945,7 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
              virBufferAsprintf(&buf, ",vectors=%d",
                                def->opts.vioserial.vectors);
          }
-        if (qemuBuildVirtioOptionsStr(&buf, def->virtio, qemuCaps) < 0)
+        if (qemuBuildVirtioOptionsStr(&buf, def->virtio) < 0)
              return -1;
          break;
  
@@ -3916,7 +3897,7 @@ qemuBuildNicDevStr(virDomainDefPtr def,
      if (bootindex)
          virBufferAsprintf(&buf, ",bootindex=%u", bootindex);
      if (usingVirtio &&
-        qemuBuildVirtioOptionsStr(&buf, net->virtio, qemuCaps) < 0)
+        qemuBuildVirtioOptionsStr(&buf, net->virtio) < 0)
          return NULL;
  
      return virBufferContentAndReset(&buf);
@@ -4158,7 +4139,7 @@ qemuBuildMemballoonCommandLine(virCommandPtr cmd,
                            virTristateSwitchTypeToString(def->memballoon->autodeflate));
      }
  
-    if (qemuBuildVirtioOptionsStr(&buf, def->memballoon->virtio, qemuCaps) < 0)
+    if (qemuBuildVirtioOptionsStr(&buf, def->memballoon->virtio) < 0)
          return -1;
  
      if (qemuCommandAddExtDevice(cmd, &def->memballoon->info) < 0)
@@ -4250,7 +4231,7 @@ qemuBuildVirtioInputDevStr(const virDomainDef *def,
      if (qemuBuildDeviceAddressStr(&buf, def, &dev->info, qemuCaps) < 0)
          return NULL;
  
-    if (qemuBuildVirtioOptionsStr(&buf, dev->virtio, qemuCaps) < 0)
+    if (qemuBuildVirtioOptionsStr(&buf, dev->virtio) < 0)
          return NULL;
  
      return virBufferContentAndReset(&buf);
@@ -4561,7 +4542,7 @@ qemuBuildDeviceVideoStr(const virDomainDef *def,
      if (qemuBuildDeviceAddressStr(&buf, def, &video->info, qemuCaps) < 0)
          return NULL;
  
-    if (qemuBuildVirtioOptionsStr(&buf, video->virtio, qemuCaps) < 0)
+    if (qemuBuildVirtioOptionsStr(&buf, video->virtio) < 0)
          return NULL;
  
      return virBufferContentAndReset(&buf);
@@ -5777,7 +5758,7 @@ qemuBuildRNGDevStr(const virDomainDef *def,
              virBufferAddLit(&buf, ",period=1000");
      }
  
-    if (qemuBuildVirtioOptionsStr(&buf, dev->virtio, qemuCaps) < 0)
+    if (qemuBuildVirtioOptionsStr(&buf, dev->virtio) < 0)
          return NULL;
  
      if (qemuBuildDeviceAddressStr(&buf, def, &dev->info, qemuCaps) < 0)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index cb0ff8d6..76691a5b 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -1045,6 +1045,40 @@ qemuValidateNetSupportsCoalesce(virDomainNetType type)
  }
  
  
+static int
+qemuValidateDomainVirtioOptions(const virDomainVirtioOptions *virtio,
+                                virQEMUCapsPtr qemuCaps)
+{
+    if (!virtio)
+        return 0;
+
+    if (virtio->iommu != VIR_TRISTATE_SWITCH_ABSENT &&
+        !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_PCI_IOMMU_PLATFORM)) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                       _("the iommu setting is not supported "
+                         "with this QEMU binary"));
+        return -1;
+    }
+
+    if (virtio->ats != VIR_TRISTATE_SWITCH_ABSENT &&
+        !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_PCI_ATS)) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                       _("the ats setting is not supported with this "
+                         "QEMU binary"));
+        return -1;
+    }
+
+    if (virtio->packed != VIR_TRISTATE_SWITCH_ABSENT &&
+        !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_PACKED_QUEUES)) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                           _("the packed setting is not supported with this "
+                             "QEMU binary"));
+            return -1;
+        }
+    return 0;
+}
+
+
  static int
  qemuValidateDomainDeviceDefNetwork(const virDomainNetDef *net,
                                     virQEMUCapsPtr qemuCaps)
@@ -1122,6 +1156,8 @@ qemuValidateDomainDeviceDefNetwork(const virDomainNetDef *net,
                             _("tx_queue_size has to be a power of two"));
              return -1;
          }
+        if (qemuValidateDomainVirtioOptions(net->virtio, qemuCaps) < 0)
+            return -1;
      }
  
      if (net->mtu &&
@@ -1469,12 +1505,15 @@ qemuValidateDomainSmartcardDef(const virDomainSmartcardDef *def,
  
  static int
  qemuValidateDomainRNGDef(const virDomainRNGDef *def,
-                         virQEMUCapsPtr qemuCaps G_GNUC_UNUSED)
+                         virQEMUCapsPtr qemuCaps)
  {
      if (def->backend == VIR_DOMAIN_RNG_BACKEND_EGD &&
          qemuValidateDomainChrSourceDef(def->source.chardev, qemuCaps) < 0)
          return -1;
  
+    if (qemuValidateDomainVirtioOptions(def->virtio, qemuCaps) < 0)
+        return -1;
+
      return 0;
  }
  
@@ -1818,6 +1857,9 @@ qemuValidateDomainDeviceDefVideo(const virDomainVideoDef *video,
          }
      }
  
+    if (qemuValidateDomainVirtioOptions(video->virtio, qemuCaps) < 0)
+        return -1;
+
      return 0;
  }
  
@@ -1916,6 +1958,11 @@ qemuValidateDomainDeviceDefDisk(const virDomainDiskDef *disk,
              return -1;
      }
  
+    if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO &&
+        qemuValidateDomainVirtioOptions(disk->virtio, qemuCaps) < 0) {
+        return -1;
+    }
+
      return 0;
  }
  
@@ -2146,7 +2193,8 @@ virValidateControllerPCIModelNameToQEMUCaps(int modelName)
  
  
  static int
-qemuValidateDomainDeviceDefControllerAttributes(const virDomainControllerDef *controller)
+qemuValidateDomainDeviceDefControllerAttributes(const virDomainControllerDef *controller,
+                                                virQEMUCapsPtr qemuCaps)
  {
      if (!(controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI &&
            (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI ||
@@ -2177,6 +2225,13 @@ qemuValidateDomainDeviceDefControllerAttributes(const virDomainControllerDef *co
                             _("'iothread' is only supported for virtio-scsi controller"));
              return -1;
          }
+        if (qemuValidateDomainVirtioOptions(controller->virtio, qemuCaps) < 0)
+            return -1;
+    }
+
+    if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL &&
+        qemuValidateDomainVirtioOptions(controller->virtio, qemuCaps) < 0) {
+        return -1;
      }
  
      return 0;
@@ -2719,7 +2774,7 @@ qemuValidateDomainDeviceDefController(const virDomainControllerDef *controller,
          !qemuValidateCheckSCSIControllerModel(qemuCaps, controller->model))
          return -1;
  
-    if (qemuValidateDomainDeviceDefControllerAttributes(controller) < 0)
+    if (qemuValidateDomainDeviceDefControllerAttributes(controller, qemuCaps) < 0)
          return -1;
  
      switch ((virDomainControllerType)controller->type) {
@@ -3056,6 +3111,9 @@ qemuValidateDomainDeviceDefFS(virDomainFSDefPtr fs,
          return -1;
      }
  
+    if (qemuValidateDomainVirtioOptions(fs->virtio, qemuCaps) < 0)
+        return -1;
+
      return 0;
  }
  
@@ -3324,6 +3382,9 @@ qemuValidateDomainDeviceDefInput(const virDomainInputDef *input,
          return -1;
      }
  
+    if (qemuValidateDomainVirtioOptions(input->virtio, qemuCaps) < 0)
+        return -1;
+
      return 0;
  }
  
@@ -3353,6 +3414,9 @@ qemuValidateDomainDeviceDefMemballoon(const virDomainMemballoonDef *memballoon,
          return -1;
      }
  
+    if (qemuValidateDomainVirtioOptions(memballoon->virtio, qemuCaps) < 0)
+        return -1;
+
      return 0;
  }
  
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 7ceb3aee..7a9c65f0 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -3039,48 +3039,48 @@ mymain(void)
              QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
              QEMU_CAPS_DEVICE_VHOST_USER_GPU,
              QEMU_CAPS_VIRTIO_PACKED_QUEUES);
-    DO_TEST_FAILURE("virtio-options-controller-iommu", QEMU_CAPS_VIRTIO_SCSI);
-    DO_TEST_FAILURE("virtio-options-disk-iommu", NONE);
-    DO_TEST_FAILURE("virtio-options-fs-iommu", NONE);
-    DO_TEST_FAILURE("virtio-options-input-iommu", QEMU_CAPS_VIRTIO_MOUSE,
-                    QEMU_CAPS_VIRTIO_KEYBOARD);
-    DO_TEST_FAILURE("virtio-options-memballoon-iommu", NONE);
-    DO_TEST_FAILURE("virtio-options-net-iommu", NONE);
-    DO_TEST_FAILURE("virtio-options-rng-iommu", QEMU_CAPS_DEVICE_VIRTIO_RNG,
-                    QEMU_CAPS_OBJECT_RNG_RANDOM);
-    DO_TEST_FAILURE("virtio-options-video-iommu", QEMU_CAPS_DEVICE_VIRTIO_GPU,
-                    QEMU_CAPS_DEVICE_VIRTIO_GPU,
-                    QEMU_CAPS_VIRTIO_GPU_VIRGL,
-                    QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
-                    QEMU_CAPS_DEVICE_VHOST_USER_GPU);
-    DO_TEST_FAILURE("virtio-options-controller-ats", QEMU_CAPS_VIRTIO_SCSI);
-    DO_TEST_FAILURE("virtio-options-disk-ats", NONE);
-    DO_TEST_FAILURE("virtio-options-fs-ats", NONE);
-    DO_TEST_FAILURE("virtio-options-input-ats", QEMU_CAPS_VIRTIO_MOUSE,
-                    QEMU_CAPS_VIRTIO_KEYBOARD);
-    DO_TEST_FAILURE("virtio-options-memballoon-ats", NONE);
-    DO_TEST_FAILURE("virtio-options-net-ats", NONE);
-    DO_TEST_FAILURE("virtio-options-rng-ats", QEMU_CAPS_DEVICE_VIRTIO_RNG,
-                    QEMU_CAPS_OBJECT_RNG_RANDOM);
-    DO_TEST_FAILURE("virtio-options-video-ats", QEMU_CAPS_DEVICE_VIRTIO_GPU,
-                    QEMU_CAPS_DEVICE_VIRTIO_GPU,
-                    QEMU_CAPS_VIRTIO_GPU_VIRGL,
-                    QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
-                    QEMU_CAPS_DEVICE_VHOST_USER_GPU);
-    DO_TEST_FAILURE("virtio-options-controller-packed", QEMU_CAPS_VIRTIO_SCSI);
-    DO_TEST_FAILURE("virtio-options-disk-packed", NONE);
-    DO_TEST_FAILURE("virtio-options-fs-packed", NONE);
-    DO_TEST_FAILURE("virtio-options-input-packed", QEMU_CAPS_VIRTIO_MOUSE,
-                    QEMU_CAPS_VIRTIO_KEYBOARD);
-    DO_TEST_FAILURE("virtio-options-memballoon-packed", NONE);
-    DO_TEST_FAILURE("virtio-options-net-packed", NONE);
-    DO_TEST_FAILURE("virtio-options-rng-packed", QEMU_CAPS_DEVICE_VIRTIO_RNG,
-                    QEMU_CAPS_OBJECT_RNG_RANDOM);
-    DO_TEST_FAILURE("virtio-options-video-packed", QEMU_CAPS_DEVICE_VIRTIO_GPU,
-                    QEMU_CAPS_DEVICE_VIRTIO_GPU,
-                    QEMU_CAPS_VIRTIO_GPU_VIRGL,
-                    QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
-                    QEMU_CAPS_DEVICE_VHOST_USER_GPU);
+    DO_TEST_PARSE_ERROR("virtio-options-controller-iommu", QEMU_CAPS_VIRTIO_SCSI);
+    DO_TEST_PARSE_ERROR("virtio-options-disk-iommu", NONE);
+    DO_TEST_PARSE_ERROR("virtio-options-fs-iommu", NONE);
+    DO_TEST_PARSE_ERROR("virtio-options-input-iommu", QEMU_CAPS_VIRTIO_MOUSE,
+                        QEMU_CAPS_VIRTIO_KEYBOARD);
+    DO_TEST_PARSE_ERROR("virtio-options-net-iommu", NONE);
+    DO_TEST_PARSE_ERROR("virtio-options-memballoon-iommu", NONE);
+    DO_TEST_PARSE_ERROR("virtio-options-rng-iommu", QEMU_CAPS_DEVICE_VIRTIO_RNG,
+                        QEMU_CAPS_OBJECT_RNG_RANDOM);
+    DO_TEST_PARSE_ERROR("virtio-options-video-iommu", QEMU_CAPS_DEVICE_VIRTIO_GPU,
+                        QEMU_CAPS_DEVICE_VIRTIO_GPU,
+                        QEMU_CAPS_VIRTIO_GPU_VIRGL,
+                        QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
+                        QEMU_CAPS_DEVICE_VHOST_USER_GPU);
+    DO_TEST_PARSE_ERROR("virtio-options-controller-ats", QEMU_CAPS_VIRTIO_SCSI);
+    DO_TEST_PARSE_ERROR("virtio-options-disk-ats", NONE);
+    DO_TEST_PARSE_ERROR("virtio-options-fs-ats", NONE);
+    DO_TEST_PARSE_ERROR("virtio-options-input-ats", QEMU_CAPS_VIRTIO_MOUSE,
+                        QEMU_CAPS_VIRTIO_KEYBOARD);
+    DO_TEST_PARSE_ERROR("virtio-options-memballoon-ats", NONE);
+    DO_TEST_PARSE_ERROR("virtio-options-net-ats", NONE);
+    DO_TEST_PARSE_ERROR("virtio-options-rng-ats", QEMU_CAPS_DEVICE_VIRTIO_RNG,
+                        QEMU_CAPS_OBJECT_RNG_RANDOM);
+    DO_TEST_PARSE_ERROR("virtio-options-video-ats", QEMU_CAPS_DEVICE_VIRTIO_GPU,
+                        QEMU_CAPS_DEVICE_VIRTIO_GPU,
+                        QEMU_CAPS_VIRTIO_GPU_VIRGL,
+                        QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
+                        QEMU_CAPS_DEVICE_VHOST_USER_GPU);
+    DO_TEST_PARSE_ERROR("virtio-options-controller-packed", QEMU_CAPS_VIRTIO_SCSI);
+    DO_TEST_PARSE_ERROR("virtio-options-disk-packed", NONE);
+    DO_TEST_PARSE_ERROR("virtio-options-fs-packed", NONE);
+    DO_TEST_PARSE_ERROR("virtio-options-input-packed", QEMU_CAPS_VIRTIO_MOUSE,
+                        QEMU_CAPS_VIRTIO_KEYBOARD);
+    DO_TEST_PARSE_ERROR("virtio-options-memballoon-packed", NONE);
+    DO_TEST_PARSE_ERROR("virtio-options-net-packed", NONE);
+    DO_TEST_PARSE_ERROR("virtio-options-rng-packed", QEMU_CAPS_DEVICE_VIRTIO_RNG,
+                        QEMU_CAPS_OBJECT_RNG_RANDOM);
+    DO_TEST_PARSE_ERROR("virtio-options-video-packed", QEMU_CAPS_DEVICE_VIRTIO_GPU,
+                        QEMU_CAPS_DEVICE_VIRTIO_GPU,
+                        QEMU_CAPS_VIRTIO_GPU_VIRGL,
+                        QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
+                        QEMU_CAPS_DEVICE_VHOST_USER_GPU);
  
      DO_TEST("fd-memory-numa-topology", QEMU_CAPS_OBJECT_MEMORY_FILE,
              QEMU_CAPS_KVM);



--
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









