All current IOMMU features are specific to Intel IOMMU, so
understandably we check for the corresponding capabilities
inside the Intel-specific switch() branch; however, we want
to make sure SMMUv3 IOMMU users get an error if they try to
enable any of those features in their guest, and performing
the capability checks unconditionally is both the easiest
way to achieve that, as well as the one least likely to
result in us inadvertently letting users enable some new
Intel-specific IOMMU feature for ARM guests later on.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 src/qemu/qemu_domain.c | 63 +++++++++++++++++++++++-------------------
 1 file changed, 35 insertions(+), 28 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 3290cdae5f..195124daa3 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -6105,34 +6105,6 @@ qemuDomainDeviceDefValidateIOMMU(const virDomainIOMMUDef *iommu,
                            virDomainIOMMUModelTypeToString(iommu->model));
             return -1;
         }
-        if (iommu->intremap != VIR_TRISTATE_SWITCH_ABSENT &&
-            !virQEMUCapsGet(qemuCaps, QEMU_CAPS_INTEL_IOMMU_INTREMAP)) {
-            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                           _("iommu: interrupt remapping is not supported "
-                             "with this QEMU binary"));
-            return -1;
-        }
-        if (iommu->caching_mode != VIR_TRISTATE_SWITCH_ABSENT &&
-            !virQEMUCapsGet(qemuCaps, QEMU_CAPS_INTEL_IOMMU_CACHING_MODE))  {
-            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                           _("iommu: caching mode is not supported "
-                             "with this QEMU binary"));
-            return -1;
-        }
-        if (iommu->eim != VIR_TRISTATE_SWITCH_ABSENT &&
-            !virQEMUCapsGet(qemuCaps, QEMU_CAPS_INTEL_IOMMU_EIM))  {
-            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                           _("iommu: eim is not supported "
-                             "with this QEMU binary"));
-            return -1;
-        }
-        if (iommu->iotlb != VIR_TRISTATE_SWITCH_ABSENT &&
-            !virQEMUCapsGet(qemuCaps, QEMU_CAPS_INTEL_IOMMU_DEVICE_IOTLB)) {
-            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                           _("iommu: device IOTLB is not supported "
-                             "with this QEMU binary"));
-            return -1;
-        }
         break;
 
     case VIR_DOMAIN_IOMMU_MODEL_SMMUV3:
@@ -6158,6 +6130,41 @@ qemuDomainDeviceDefValidateIOMMU(const virDomainIOMMUDef *iommu,
         return -1;
     }
 
+    /* These capability checks ensure we're not trying to use features
+     * of Intel IOMMU that the QEMU binary does not support, but they
+     * also make sure we report an error when trying to use features
+     * that are not implemented by SMMUv3, so they must be here instead
+     * of inside the switch() statement above */
+
+    if (iommu->intremap != VIR_TRISTATE_SWITCH_ABSENT &&
+        !virQEMUCapsGet(qemuCaps, QEMU_CAPS_INTEL_IOMMU_INTREMAP)) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                       _("iommu: interrupt remapping is not supported "
+                         "with this QEMU binary"));
+        return -1;
+    }
+    if (iommu->caching_mode != VIR_TRISTATE_SWITCH_ABSENT &&
+        !virQEMUCapsGet(qemuCaps, QEMU_CAPS_INTEL_IOMMU_CACHING_MODE))  {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                       _("iommu: caching mode is not supported "
+                         "with this QEMU binary"));
+        return -1;
+    }
+    if (iommu->eim != VIR_TRISTATE_SWITCH_ABSENT &&
+        !virQEMUCapsGet(qemuCaps, QEMU_CAPS_INTEL_IOMMU_EIM))  {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                       _("iommu: eim is not supported "
+                         "with this QEMU binary"));
+        return -1;
+    }
+    if (iommu->iotlb != VIR_TRISTATE_SWITCH_ABSENT &&
+        !virQEMUCapsGet(qemuCaps, QEMU_CAPS_INTEL_IOMMU_DEVICE_IOTLB)) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                       _("iommu: device IOTLB is not supported "
+                         "with this QEMU binary"));
+        return -1;
+    }
+
     return 0;
 }
 
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, May 28, 2019 at 05:29:02PM +0200, Andrea Bolognani wrote:
>All current IOMMU features are specific to Intel IOMMU, so
>understandably we check for the corresponding capabilities
>inside the Intel-specific switch() branch; however, we want
>to make sure SMMUv3 IOMMU users get an error if they try to
>enable any of those features in their guest, and performing
>the capability checks unconditionally is both the easiest
>way to achieve that, as well as the one least likely to
>result in us inadvertently letting users enable some new
>Intel-specific IOMMU feature for ARM guests later on.
>
>Signed-off-by: Andrea Bolognani <abologna@redhat.com>
>---
> src/qemu/qemu_domain.c | 63 +++++++++++++++++++++++-------------------
> 1 file changed, 35 insertions(+), 28 deletions(-)
>
>@@ -6158,6 +6130,41 @@ qemuDomainDeviceDefValidateIOMMU(const virDomainIOMMUDef *iommu,
>         return -1;
>     }
>
>+    /* These capability checks ensure we're not trying to use features
>+     * of Intel IOMMU that the QEMU binary does not support, but they
>+     * also make sure we report an error when trying to use features
>+     * that are not implemented by SMMUv3,

> so they must be here instead
>+     * of inside the switch() statement above */

This part of the sentence is not really necessary.

>+
>+    if (iommu->intremap != VIR_TRISTATE_SWITCH_ABSENT &&
>+        !virQEMUCapsGet(qemuCaps, QEMU_CAPS_INTEL_IOMMU_INTREMAP)) {
>+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>+                       _("iommu: interrupt remapping is not supported "
>+                         "with this QEMU binary"));
>+        return -1;
>+    }

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list