diff mbox

[RFC,v3,07/14] memory: add section range info for IOMMU notifier

Message ID 20170113092345.GX4450@pxdev.xzpeter.org
State New
Headers show

Commit Message

Peter Xu Jan. 13, 2017, 9:23 a.m. UTC 
On Fri, Jan 13, 2017 at 03:55:22PM +0800, Jason Wang wrote:
> 
> 
> On 2017年01月13日 11:06, Peter Xu wrote:
> >In this patch, IOMMUNotifier.{start|end} are introduced to store section
> >information for a specific notifier. When notification occurs, we not
> >only check the notification type (MAP|UNMAP), but also check whether the
> >notified iova is in the range of specific IOMMU notifier, and skip those
> >notifiers if not in the listened range.
> >
> >When removing an region, we need to make sure we removed the correct
> >VFIOGuestIOMMU by checking the IOMMUNotifier.start address as well.
> >
> >Suggested-by: David Gibson <david@gibson.dropbear.id.au>
> >Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> >Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> >Signed-off-by: Peter Xu <peterx@redhat.com>
> >---
> >  hw/vfio/common.c      | 7 ++++++-
> >  include/exec/memory.h | 3 +++
> >  memory.c              | 4 +++-
> >  3 files changed, 12 insertions(+), 2 deletions(-)
> >
> >diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> >index 801578b..6f648da 100644
> >--- a/hw/vfio/common.c
> >+++ b/hw/vfio/common.c
> >@@ -455,6 +455,10 @@ static void vfio_listener_region_add(MemoryListener *listener,
> >          giommu->container = container;
> >          giommu->n.notify = vfio_iommu_map_notify;
> >          giommu->n.notifier_flags = IOMMU_NOTIFIER_ALL;
> >+        giommu->n.start = section->offset_within_region;
> >+        llend = int128_add(int128_make64(giommu->n.start), section->size);
> >+        llend = int128_sub(llend, int128_one());
> >+        giommu->n.end = int128_get64(llend);
> >          QLIST_INSERT_HEAD(&container->giommu_list, giommu, giommu_next);
> >          memory_region_register_iommu_notifier(giommu->iommu, &giommu->n);
> >@@ -525,7 +529,8 @@ static void vfio_listener_region_del(MemoryListener *listener,
> >          VFIOGuestIOMMU *giommu;
> >          QLIST_FOREACH(giommu, &container->giommu_list, giommu_next) {
> >-            if (giommu->iommu == section->mr) {
> >+            if (giommu->iommu == section->mr &&
> >+                giommu->n.start == section->offset_within_region) {
> >                  memory_region_unregister_iommu_notifier(giommu->iommu,
> >                                                          &giommu->n);
> >                  QLIST_REMOVE(giommu, giommu_next);
> >diff --git a/include/exec/memory.h b/include/exec/memory.h
> >index bec9756..7649e74 100644
> >--- a/include/exec/memory.h
> >+++ b/include/exec/memory.h
> >@@ -84,6 +84,9 @@ typedef enum {
> >  struct IOMMUNotifier {
> >      void (*notify)(struct IOMMUNotifier *notifier, IOMMUTLBEntry *data);
> >      IOMMUNotifierFlag notifier_flags;
> >+    /* Notify for address space range start <= addr <= end */
> >+    hwaddr start;
> >+    hwaddr end;
> >      QLIST_ENTRY(IOMMUNotifier) node;
> >  };
> >  typedef struct IOMMUNotifier IOMMUNotifier;
> >diff --git a/memory.c b/memory.c
> >index 2bfc37f..e88bb54 100644
> >--- a/memory.c
> >+++ b/memory.c
> >@@ -1671,7 +1671,9 @@ void memory_region_notify_iommu(MemoryRegion *mr,
> >      }
> >      QLIST_FOREACH(iommu_notifier, &mr->iommu_notify, node) {
> >-        if (iommu_notifier->notifier_flags & request_flags) {
> >+        if (iommu_notifier->notifier_flags & request_flags &&
> >+            iommu_notifier->start <= entry.iova &&
> >+            iommu_notifier->end >= entry.iova) {
> >              iommu_notifier->notify(iommu_notifier, &entry);
> >          }
> >      }
> 
> This seems breaks vhost device IOTLB. How about keep the the behavior
> somehow?

Thanks to point out. How about I squash this into this patch?

--------8<--------
-------->8--------

-- peterx

Comments

Jason Wang Jan. 13, 2017, 9:37 a.m. UTC | #1 
On 2017年01月13日 17:23, Peter Xu wrote:
> On Fri, Jan 13, 2017 at 03:55:22PM +0800, Jason Wang wrote:
>>
>> On 2017年01月13日 11:06, Peter Xu wrote:
>>> In this patch, IOMMUNotifier.{start|end} are introduced to store section
>>> information for a specific notifier. When notification occurs, we not
>>> only check the notification type (MAP|UNMAP), but also check whether the
>>> notified iova is in the range of specific IOMMU notifier, and skip those
>>> notifiers if not in the listened range.
>>>
>>> When removing an region, we need to make sure we removed the correct
>>> VFIOGuestIOMMU by checking the IOMMUNotifier.start address as well.
>>>
>>> Suggested-by: David Gibson <david@gibson.dropbear.id.au>
>>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>>> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
>>> Signed-off-by: Peter Xu <peterx@redhat.com>
>>> ---
>>>   hw/vfio/common.c      | 7 ++++++-
>>>   include/exec/memory.h | 3 +++
>>>   memory.c              | 4 +++-
>>>   3 files changed, 12 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>>> index 801578b..6f648da 100644
>>> --- a/hw/vfio/common.c
>>> +++ b/hw/vfio/common.c
>>> @@ -455,6 +455,10 @@ static void vfio_listener_region_add(MemoryListener *listener,
>>>           giommu->container = container;
>>>           giommu->n.notify = vfio_iommu_map_notify;
>>>           giommu->n.notifier_flags = IOMMU_NOTIFIER_ALL;
>>> +        giommu->n.start = section->offset_within_region;
>>> +        llend = int128_add(int128_make64(giommu->n.start), section->size);
>>> +        llend = int128_sub(llend, int128_one());
>>> +        giommu->n.end = int128_get64(llend);
>>>           QLIST_INSERT_HEAD(&container->giommu_list, giommu, giommu_next);
>>>           memory_region_register_iommu_notifier(giommu->iommu, &giommu->n);
>>> @@ -525,7 +529,8 @@ static void vfio_listener_region_del(MemoryListener *listener,
>>>           VFIOGuestIOMMU *giommu;
>>>           QLIST_FOREACH(giommu, &container->giommu_list, giommu_next) {
>>> -            if (giommu->iommu == section->mr) {
>>> +            if (giommu->iommu == section->mr &&
>>> +                giommu->n.start == section->offset_within_region) {
>>>                   memory_region_unregister_iommu_notifier(giommu->iommu,
>>>                                                           &giommu->n);
>>>                   QLIST_REMOVE(giommu, giommu_next);
>>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>>> index bec9756..7649e74 100644
>>> --- a/include/exec/memory.h
>>> +++ b/include/exec/memory.h
>>> @@ -84,6 +84,9 @@ typedef enum {
>>>   struct IOMMUNotifier {
>>>       void (*notify)(struct IOMMUNotifier *notifier, IOMMUTLBEntry *data);
>>>       IOMMUNotifierFlag notifier_flags;
>>> +    /* Notify for address space range start <= addr <= end */
>>> +    hwaddr start;
>>> +    hwaddr end;
>>>       QLIST_ENTRY(IOMMUNotifier) node;
>>>   };
>>>   typedef struct IOMMUNotifier IOMMUNotifier;
>>> diff --git a/memory.c b/memory.c
>>> index 2bfc37f..e88bb54 100644
>>> --- a/memory.c
>>> +++ b/memory.c
>>> @@ -1671,7 +1671,9 @@ void memory_region_notify_iommu(MemoryRegion *mr,
>>>       }
>>>       QLIST_FOREACH(iommu_notifier, &mr->iommu_notify, node) {
>>> -        if (iommu_notifier->notifier_flags & request_flags) {
>>> +        if (iommu_notifier->notifier_flags & request_flags &&
>>> +            iommu_notifier->start <= entry.iova &&
>>> +            iommu_notifier->end >= entry.iova) {
>>>               iommu_notifier->notify(iommu_notifier, &entry);
>>>           }
>>>       }
>> This seems breaks vhost device IOTLB. How about keep the the behavior
>> somehow?
> Thanks to point out. How about I squash this into this patch?
>
> --------8<--------
> diff --git a/memory.c b/memory.c
> index e88bb54..6de02dd 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1608,8 +1608,14 @@ void memory_region_register_iommu_notifier(MemoryRegion *mr,
>           return;
>       }
>   
> +    if (n->start == 0 && n->end == 0) {
> +        /* If these are not specified, we listen to the whole range */
> +        n->end = (hwaddr)(-1);
> +    }
> +
>       /* We need to register for at least one bitfield */
>       assert(n->notifier_flags != IOMMU_NOTIFIER_NONE);
> +    assert(n->start <= n->end);
>       QLIST_INSERT_HEAD(&mr->iommu_notify, n, node);
>       memory_region_update_iommu_notify_flags(mr);
>   }
> -------->8--------
>
> -- peterx

This should work, or you can introduce a 
memory_region_iommu_notifier_init() to force user to explicitly 
initialize start and end.

Thanks
Peter Xu Jan. 13, 2017, 10:22 a.m. UTC | #2 
On Fri, Jan 13, 2017 at 05:37:43PM +0800, Jason Wang wrote:

[...]

> >>>diff --git a/memory.c b/memory.c
> >>>index 2bfc37f..e88bb54 100644
> >>>--- a/memory.c
> >>>+++ b/memory.c
> >>>@@ -1671,7 +1671,9 @@ void memory_region_notify_iommu(MemoryRegion *mr,
> >>>      }
> >>>      QLIST_FOREACH(iommu_notifier, &mr->iommu_notify, node) {
> >>>-        if (iommu_notifier->notifier_flags & request_flags) {
> >>>+        if (iommu_notifier->notifier_flags & request_flags &&
> >>>+            iommu_notifier->start <= entry.iova &&
> >>>+            iommu_notifier->end >= entry.iova) {
> >>>              iommu_notifier->notify(iommu_notifier, &entry);
> >>>          }
> >>>      }
> >>This seems breaks vhost device IOTLB. How about keep the the behavior
> >>somehow?
> >Thanks to point out. How about I squash this into this patch?
> >
> >--------8<--------
> >diff --git a/memory.c b/memory.c
> >index e88bb54..6de02dd 100644
> >--- a/memory.c
> >+++ b/memory.c
> >@@ -1608,8 +1608,14 @@ void memory_region_register_iommu_notifier(MemoryRegion *mr,
> >          return;
> >      }
> >+    if (n->start == 0 && n->end == 0) {
> >+        /* If these are not specified, we listen to the whole range */
> >+        n->end = (hwaddr)(-1);
> >+    }
> >+
> >      /* We need to register for at least one bitfield */
> >      assert(n->notifier_flags != IOMMU_NOTIFIER_NONE);
> >+    assert(n->start <= n->end);
> >      QLIST_INSERT_HEAD(&mr->iommu_notify, n, node);
> >      memory_region_update_iommu_notify_flags(mr);
> >  }
> >-------->8--------
> >
> >-- peterx
> 
> This should work, or you can introduce a memory_region_iommu_notifier_init()
> to force user to explicitly initialize start and end.

Hmm, this sounds better, considering that IOMMUNotifier is getting
more fields to be inited. Thanks,

-- peterx
diff mbox

Patch

diff --git a/memory.c b/memory.c
index e88bb54..6de02dd 100644
--- a/memory.c
+++ b/memory.c
@@ -1608,8 +1608,14 @@  void memory_region_register_iommu_notifier(MemoryRegion *mr,
         return;
     }
 
+    if (n->start == 0 && n->end == 0) {
+        /* If these are not specified, we listen to the whole range */
+        n->end = (hwaddr)(-1);
+    }
+
     /* We need to register for at least one bitfield */
     assert(n->notifier_flags != IOMMU_NOTIFIER_NONE);
+    assert(n->start <= n->end);
     QLIST_INSERT_HEAD(&mr->iommu_notify, n, node);
     memory_region_update_iommu_notify_flags(mr);
 }