This is an "global" version of exising memory_region_iommu_replay() - we
announce the translations to all the registered notifiers, instead of a
specific one.
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
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);
+ }
+}
+
void memory_region_unregister_iommu_notifier(MemoryRegion *mr,
IOMMUNotifier *n)
{
--
2.7.4
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 <david@gibson.dropbear.id.au>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> 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?
Thanks
Eric
> + }
> +}
> +
> void memory_region_unregister_iommu_notifier(MemoryRegion *mr,
> IOMMUNotifier *n)
> {
>
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 <david@gibson.dropbear.id.au>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> > 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 <david@gibson.dropbear.id.au>
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
On Thu, Apr 06, 2017 at 07:46:47PM +0800, Peter Xu wrote:
> On Thu, Apr 06, 2017 at 12:52:19PM +0200, Auger Eric wrote:
> > Hi Peter,
> >
> > On 06/04/2017 09:08, Peter Xu wrote:
[...]
> > > +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 <david@gibson.dropbear.id.au>
> 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------
This follow-up patch has an assumption that all the pages would have
read permission. That may not be true, e.g., for write-only pages. A
better way (after a short discussion with David Gibson) would be
converting is_write in iommu_ops.translate() to IOMMUAccessFlags, so
that we can pass in IOMMU_NONE here (it means, we don't check
permission for this page translation). Further, we can remove the
is_write parameter in memory_region_iommu_replay().
In all cases, I'll move this change out of current series and send a
fresh-new post after 2.10. Thanks,
-- peterx
On Thu, Apr 06, 2017 at 03:08:38PM +0800, Peter Xu wrote:
> This is an "global" version of exising memory_region_iommu_replay() - we
> announce the translations to all the registered notifiers, instead of a
> specific one.
>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> 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);
> + }
> +}
> +
> void memory_region_unregister_iommu_notifier(MemoryRegion *mr,
> IOMMUNotifier *n)
> {
> --
> 2.7.4
© 2016 - 2026 Red Hat, Inc.