Re: [Qemu-devel] [PATCH for 2.8 10/11] Revert "intel_iommu: Throw hw_error on notify_started"

[Peter Xu <peterx@redhat.com>]

On Wed, Aug 31, 2016 at 10:45:37AM +0800, Jason Wang wrote:
> 
> 
> On 2016年08月30日 11:37, Alex Williamson wrote:
> >On Tue, 30 Aug 2016 11:06:58 +0800
> >Jason Wang <jasowang@redhat.com> wrote:
> >
> >>From: Peter Xu <peterx@redhat.com>
> >>
> >>This reverts commit 3cb3b1549f5401dc3a5e1d073e34063dc274136f. Vhost
> >>device IOTLB API will get notified and send invalidation request to
> >>vhost through this notifier.
> >AFAICT this series does not address the original problem for which
> >commit 3cb3b1549f54 was added.  We've only addressed the very narrow
> >use case of a device iotlb firing the iommu notifier therefore this
> >change is a regression versus 2.7 since it allows invalid
> >configurations with a physical iommu which will never receive the
> >necessary notifies from intel-iommu emulation to work properly.  Thanks,
> >
> >Alex
> 
> Looking at vfio, it cares about map but vhost only cares about IOTLB
> invalidation. Then I think we probably need another kind of notifier in this
> case to avoid this.

Shall we leverage IOMMUTLBEntry.perm == IOMMU_NONE as a sign for
invalidation? If so, we can use the same IOTLB interface as before.
IMHO these two interfaces are not conflicting?

Alex,

Do you mean we should still disallow user from passing through devices
while Intel IOMMU enabled? If so, not sure whether patch below can
solve the issue.

It seems that we need a "name" for either IOMMU notifier
provider/consumer, and we should not allow (provider==Intel &&
consumer==VFIO) happen. In the following case, I added a name for
provider, and VFIO checks it.

--------8<----------

diff --git a/hw/alpha/typhoon.c b/hw/alpha/typhoon.c
index 883db13..936c2e6 100644
--- a/hw/alpha/typhoon.c
+++ b/hw/alpha/typhoon.c
@@ -725,6 +725,7 @@ static IOMMUTLBEntry typhoon_translate_iommu(MemoryRegion *iommu, hwaddr addr,
 }

 static const MemoryRegionIOMMUOps typhoon_iommu_ops = {
+    .iommu_type = "typhoon",
     .translate = typhoon_translate_iommu,
 };

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 28c31a2..f5e3875 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2347,6 +2347,7 @@ static void vtd_init(IntelIOMMUState *s)
     memset(s->w1cmask, 0, DMAR_REG_SIZE);
     memset(s->womask, 0, DMAR_REG_SIZE);

+    s->iommu_ops.iommu_type = "intel";
     s->iommu_ops.translate = vtd_iommu_translate;
     s->iommu_ops.notify_started = vtd_iommu_notify_started;
     s->root = 0;
diff --git a/hw/pci-host/apb.c b/hw/pci-host/apb.c
index 653e711..9cfbb73 100644
--- a/hw/pci-host/apb.c
+++ b/hw/pci-host/apb.c
@@ -323,6 +323,7 @@ static IOMMUTLBEntry pbm_translate_iommu(MemoryRegion *iommu, hwaddr addr,
 }

 static MemoryRegionIOMMUOps pbm_iommu_ops = {
+    .iommu_type = "pbm",
     .translate = pbm_translate_iommu,
 };

diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
index 6bc4d4d..e3e8739 100644
--- a/hw/ppc/spapr_iommu.c
+++ b/hw/ppc/spapr_iommu.c
@@ -244,6 +244,7 @@ static const VMStateDescription vmstate_spapr_tce_table = {
 };

 static MemoryRegionIOMMUOps spapr_iommu_ops = {
+    .iommu_type = "spapr",
     .translate = spapr_tce_translate_iommu,
     .get_min_page_size = spapr_tce_get_min_page_size,
     .notify_started = spapr_tce_notify_started,
diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 9c1c04e..4414462 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -443,6 +443,7 @@ static IOMMUTLBEntry s390_translate_iommu(MemoryRegion *iommu, hwaddr addr,
 }

 static const MemoryRegionIOMMUOps s390_iommu_ops = {
+    .iommu_type = "s390",
     .translate = s390_translate_iommu,
 };

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index b313e7c..317e08b 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -441,6 +441,11 @@ static void vfio_listener_region_add(MemoryListener *listener,
     if (memory_region_is_iommu(section->mr)) {
         VFIOGuestIOMMU *giommu;

+        if (!strcmp(memory_region_iommu_type(section->mr), "intel")) {
+            error_report("Device passthrough cannot work with Intel IOMMU");
+            exit(1);
+        }
+
         trace_vfio_listener_region_add_iommu(iova, end);
         /*
          * FIXME: For VFIO iommu types which have KVM acceleration to
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 3e4d416..f012f77 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -149,6 +149,8 @@ struct MemoryRegionOps {
 typedef struct MemoryRegionIOMMUOps MemoryRegionIOMMUOps;

 struct MemoryRegionIOMMUOps {
+    /* Type of IOMMU */
+    const char *iommu_type;
     /* Return a TLB entry that contains a given address. */
     IOMMUTLBEntry (*translate)(MemoryRegion *iommu, hwaddr addr, bool is_write);
     /* Returns minimum supported page size */
@@ -593,6 +595,21 @@ static inline bool memory_region_is_iommu(MemoryRegion *mr)
     return mr->iommu_ops;
 }

+/**
+ * memory_region_iommu_type: return type of IOMMU
+ *
+ * Returns type of IOMMU, empty string ("") if not a IOMMU region.
+ *
+ * @mr: the memory region being queried
+ */
+static inline const char *memory_region_iommu_type(MemoryRegion *mr)
+{
+    if (mr->iommu_ops && mr->iommu_ops->iommu_type) {
+        return mr->iommu_ops->iommu_type;
+    }
+
+    return "";
+}

 /**
  * memory_region_iommu_get_min_page_size: get minimum supported page size

------>8--------

Thanks,

-- peterx