Re: [Qemu-devel] [PATCH 2/3] memory: add iommu_notify_flag

[Paolo Bonzini <pbonzini@redhat.com>]

On 05/09/2016 10:38, Peter Xu wrote:
> However in this patch I was not meant to do that. I made it an
> exclusive flag to identify two different use cases. I don't know
> whether this is good, but at least for Intel IOMMU's current use case,
> these two types should be totally isolated from each other:
> 
> - IOMMU_NONE notification is used by future DMAR-enabled vhost, it
>   should only be notified with device-IOTLB invalidations, this will
>   only require "Device IOTLB" capability for Intel IOMMUs, and be
>   notified in Device IOTLB invalidation handlers.
> 
> - IOMMU_RW notifications should only be used for vfio-pci, notified
>   with IOTLB invalidations. This will only require Cache Mode (CM=1)
>   capability, and will be notified in common IOTLB invalidations (no
>   matter whether it's an cache invalidation or not, we will all use
>   IOMMU_RW flag for this kind of notifies).
> 
> Maybe here naming the flags as IOMMU_{RW_NONE} is a little bit
> confusing (just to leverage existing access flags),

Yeah, if you really want to have these semantics, you need to define an 
enum like this:

	IOMMU_NOTIFIER_NONE = -1,
	IOMMU_NOTIFIER_FLUSH = 0,
	IOMMU_NOTIFIER_CHANGED_ENTRY = 1,

But I'm still not convinced of the exclusivity between "flush" and 
"entry changed" notifiers.  If I saw the above, my first reaction would 
be that you need a bit mask:

	IOMMU_NOTIFIER_NONE = -1,
	IOMMU_NOTIFIER_FLUSH = 1,
	IOMMU_NOTIFIER_CHANGED_ENTRY = 2,

But perhaps what you're looking for is to change the "notifier" to a 
"listener" like

	struct IOMMUListener {
	    void (*flush)(IOMMUListener *);
	    void (*entry_changed)(IOMMUListener *, IOMMUTLBEntry *);
	    QLIST_ENTRY(IOMMUListener) node;
	};

The patches can start with an IOMMUListener that only has the 
entry_changed callback and that replaces the current use of Notifier.  
Then notify_started and notify_stopped can be called on every notifier 
that is added/removed (see attached prototype), and the Intel IOMMU can 
simply reject registration of a listener that has a non-NULL 
iotlb_changed member.

Thanks,

Paolo

diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
index 6bc4d4d..d8cbd90 100644
--- a/hw/ppc/spapr_iommu.c
+++ b/hw/ppc/spapr_iommu.c
@@ -156,14 +156,20 @@ static uint64_t spapr_tce_get_min_page_size(MemoryRegion *iommu)
     return 1ULL << tcet->page_shift;
 }
 
-static void spapr_tce_notify_started(MemoryRegion *iommu)
+static void spapr_tce_listener_add(MemoryRegion *iommu, IOMMUListener *l)
 {
-    spapr_tce_set_need_vfio(container_of(iommu, sPAPRTCETable, iommu), true);
+    sPAPRTCETable *tcet = container_of(iommu, sPAPRTCETable, iommu);
+    if (tcet->users++ == 0) {
+        spapr_tce_set_need_vfio(tcet, true);
+    }
 }
 
-static void spapr_tce_notify_stopped(MemoryRegion *iommu)
+static void spapr_tce_listener_del(MemoryRegion *iommu, IOMMUListener *l)
 {
-    spapr_tce_set_need_vfio(container_of(iommu, sPAPRTCETable, iommu), false);
+    sPAPRTCETable *tcet = container_of(iommu, sPAPRTCETable, iommu);
+    if (--tcet->users == 0) {
+        spapr_tce_set_need_vfio(tcet, false);
+    }
 }
 
 static int spapr_tce_table_post_load(void *opaque, int version_id)
@@ -246,8 +252,8 @@ static const VMStateDescription vmstate_spapr_tce_table = {
 static MemoryRegionIOMMUOps spapr_iommu_ops = {
     .translate = spapr_tce_translate_iommu,
     .get_min_page_size = spapr_tce_get_min_page_size,
-    .notify_started = spapr_tce_notify_started,
-    .notify_stopped = spapr_tce_notify_stopped,
+    .listener_add = spapr_tce_listener_add,
+    .listener_del = spapr_tce_listener_del,
 };
 
 static int spapr_tce_table_realize(DeviceState *dev)
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index caf7be9..4761afd 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -558,6 +558,7 @@ struct sPAPRTCETable {
     uint64_t *table;
     uint32_t mig_nb_table;
     uint64_t *mig_table;
+    int users;
     bool bypass;
     bool need_vfio;
     int fd;
diff --git a/memory.c b/memory.c
index 0eb6895..e9f50af 100644
--- a/memory.c
+++ b/memory.c
@@ -1515,9 +1515,8 @@ bool memory_region_is_logging(MemoryRegion *mr, uint8_t client)
 
 void memory_region_register_iommu_notifier(MemoryRegion *mr, Notifier *n)
 {
-    if (mr->iommu_ops->notify_started &&
-        QLIST_EMPTY(&mr->iommu_notify.notifiers)) {
-        mr->iommu_ops->notify_started(mr);
+    if (mr->iommu_ops->listener_add) {
+        mr->iommu_ops->listener_add(mr, ...);
     }
     notifier_list_add(&mr->iommu_notify, n);
 }
@@ -1555,9 +1554,8 @@ void memory_region_iommu_replay(MemoryRegion *mr, Notifier *n, bool is_write)
 void memory_region_unregister_iommu_notifier(MemoryRegion *mr, Notifier *n)
 {
     notifier_remove(n);
-    if (mr->iommu_ops->notify_stopped &&
-        QLIST_EMPTY(&mr->iommu_notify.notifiers)) {
-        mr->iommu_ops->notify_stopped(mr);
+    if (mr->iommu_ops->listener_del) {
+        mr->iommu_ops->listener_del(mr, ...);
     }
 }