Re: [Qemu-devel] [PATCH v8 3/9] memory: provide iommu_replay_all()

[Peter Xu]

On Thu, Apr 06, 2017 at 12:52:19PM +0200, Auger Eric wrote:
> Hi Peter,
> 
> On 06/04/2017 09:08, Peter Xu wrote:
> > This is an "global" version of exising memory_region_iommu_replay() - we
> s/exising/existing
> > announce the translations to all the registered notifiers, instead of a
> > specific one.
> > 
> > Reviewed-by: David Gibson <address@hidden>
> > Signed-off-by: Peter Xu <address@hidden>
> > ---
> >  include/exec/memory.h | 8 ++++++++
> >  memory.c              | 9 +++++++++
> >  2 files changed, 17 insertions(+)
> > 
> > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > index 07e43da..fb7dff3 100644
> > --- a/include/exec/memory.h
> > +++ b/include/exec/memory.h
> > @@ -713,6 +713,14 @@ void memory_region_iommu_replay(MemoryRegion *mr, 
> > IOMMUNotifier *n,
> >                                  bool is_write);
> >  
> >  /**
> > + * memory_region_iommu_replay_all: replay existing IOMMU translations
> > + * to all the notifiers registered.
> > + *
> > + * @mr: the memory region to observe
> > + */
> > +void memory_region_iommu_replay_all(MemoryRegion *mr);
> > +
> > +/**
> >   * memory_region_unregister_iommu_notifier: unregister a notifier for
> >   * changes to IOMMU translation entries.
> >   *
> > diff --git a/memory.c b/memory.c
> > index 7496b3d..b4ed67b 100644
> > --- a/memory.c
> > +++ b/memory.c
> > @@ -1642,6 +1642,15 @@ void memory_region_iommu_replay(MemoryRegion *mr, 
> > IOMMUNotifier *n,
> >      }
> >  }
> >  
> > +void memory_region_iommu_replay_all(MemoryRegion *mr)
> > +{
> > +    IOMMUNotifier *notifier;
> > +
> > +    IOMMU_NOTIFIER_FOREACH(notifier, mr) {
> > +        memory_region_iommu_replay(mr, notifier, false);
> It is not fully clear to me what is the consequence of setting
> is_write=false always?

I am not quite sure about it either, on why we are having the is_write
parameter on memory_region_iommu_replay(). It's there since the first
commit that introduced the interface:

    commit a788f227ef7bd2912fcaacdfe13d13ece2998149
    Author: David Gibson <address@hidden>
    Date:   Wed Sep 30 12:13:55 2015 +1000

    memory: Allow replay of IOMMU mapping notifications

However imho that should be a check against page RW permissions, which
seems unecessary during replay. Or say, not sure whether below patch
would be good (as a standalone one besides current patch/series):

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

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 6b33b9f..d008a4b 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -488,7 +488,7 @@ static void vfio_listener_region_add(MemoryListener 
*listener,
         QLIST_INSERT_HEAD(&container->giommu_list, giommu, giommu_next);
 
         memory_region_register_iommu_notifier(giommu->iommu, &giommu->n);
-        memory_region_iommu_replay(giommu->iommu, &giommu->n, false);
+        memory_region_iommu_replay(giommu->iommu, &giommu->n);
 
         return;
     }
diff --git a/include/exec/memory.h b/include/exec/memory.h
index c4fc94d..5ab0151 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -725,11 +725,8 @@ void memory_region_register_iommu_notifier(MemoryRegion 
*mr,
  *
  * @mr: the memory region to observe
  * @n: the notifier to which to replay iommu mappings
- * @is_write: Whether to treat the replay as a translate "write"
- *     through the iommu
  */
-void memory_region_iommu_replay(MemoryRegion *mr, IOMMUNotifier *n,
-                                bool is_write);
+void memory_region_iommu_replay(MemoryRegion *mr, IOMMUNotifier *n);
 
 /**
  * memory_region_iommu_replay_all: replay existing IOMMU translations
diff --git a/memory.c b/memory.c
index b782d5b..155ca1c 100644
--- a/memory.c
+++ b/memory.c
@@ -1620,8 +1620,7 @@ uint64_t 
memory_region_iommu_get_min_page_size(MemoryRegion *mr)
     return TARGET_PAGE_SIZE;
 }
 
-void memory_region_iommu_replay(MemoryRegion *mr, IOMMUNotifier *n,
-                                bool is_write)
+void memory_region_iommu_replay(MemoryRegion *mr, IOMMUNotifier *n)
 {
     hwaddr addr, granularity;
     IOMMUTLBEntry iotlb;
@@ -1635,7 +1634,13 @@ void memory_region_iommu_replay(MemoryRegion *mr, 
IOMMUNotifier *n,
     granularity = memory_region_iommu_get_min_page_size(mr);
 
     for (addr = 0; addr < memory_region_size(mr); addr += granularity) {
-        iotlb = mr->iommu_ops->translate(mr, addr, is_write);
+        /*
+         * For a replay, we don't need to do permission check.
+         * Assuming a "read" operation here to make sure we will pass
+         * the check (in case we don't have write permission on the
+         * page).
+         */
+        iotlb = mr->iommu_ops->translate(mr, addr, false);
         if (iotlb.perm != IOMMU_NONE) {
             n->notify(n, &iotlb);
         }
@@ -1653,7 +1658,7 @@ void memory_region_iommu_replay_all(MemoryRegion *mr)
     IOMMUNotifier *notifier;
 
     IOMMU_NOTIFIER_FOREACH(notifier, mr) {
-        memory_region_iommu_replay(mr, notifier, false);
+        memory_region_iommu_replay(mr, notifier);
     }
 }

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

Thanks,

-- peterx