[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v8 3/9] memory: provide iommu_replay_all()
From: |
Peter Xu |
Subject: |
Re: [Qemu-devel] [PATCH v8 3/9] memory: provide iommu_replay_all() |
Date: |
Fri, 7 Apr 2017 12:17:20 +0800 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
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 <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------
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
[Qemu-devel] [PATCH v8 4/9] memory: introduce memory_region_notify_one(), Peter Xu, 2017/04/06
[Qemu-devel] [PATCH v8 5/9] memory: add MemoryRegionIOMMUOps.replay() callback, Peter Xu, 2017/04/06
[Qemu-devel] [PATCH v8 6/9] intel_iommu: use the correct memory region for device IOTLB notification, Peter Xu, 2017/04/06
[Qemu-devel] [PATCH v8 7/9] intel_iommu: provide its own replay() callback, Peter Xu, 2017/04/06