[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v9 1/9] memory: add section range info for IOMMU
From: |
David Gibson |
Subject: |
Re: [Qemu-devel] [PATCH v9 1/9] memory: add section range info for IOMMU notifier |
Date: |
Tue, 18 Apr 2017 21:55:11 +1000 |
User-agent: |
Mutt/1.8.0 (2017-02-23) |
On Tue, Apr 18, 2017 at 05:56:37PM +0800, Peter Xu wrote:
> On Tue, Apr 11, 2017 at 11:56:54AM +1000, David Gibson wrote:
> > On Mon, Apr 10, 2017 at 03:09:50PM +0800, Peter Xu wrote:
> > > On Mon, Apr 10, 2017 at 02:39:22PM +1000, David Gibson wrote:
> > > > On Fri, Apr 07, 2017 at 06:59:07PM +0800, 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 range overlaps with 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.
> > > > >
> > > > > This patch is solving the problem that vfio-pci devices receive
> > > > > duplicated UNMAP notification on x86 platform when vIOMMU is there.
> > > > > The
> > > > > issue is that x86 IOMMU has a (0, 2^64-1) IOMMU region, which is
> > > > > splitted by the (0xfee00000, 0xfeefffff) IRQ region. AFAIK
> > > > > this (splitted IOMMU region) is only happening on x86.
> > > > >
> > > > > This patch also helps vhost to leverage the new interface as well, so
> > > > > that vhost won't get duplicated cache flushes. In that sense, it's an
> > > > > slight performance improvement.
> > > > >
> > > > > Suggested-by: David Gibson <address@hidden>
> > > > > Reviewed-by: Eric Auger <address@hidden>
> > > > > Reviewed-by: Michael S. Tsirkin <address@hidden>
> > > > > Acked-by: Alex Williamson <address@hidden>
> > > > > Signed-off-by: Peter Xu <address@hidden>
> > > > > ---
> > > > > hw/vfio/common.c | 12 +++++++++---
> > > > > hw/virtio/vhost.c | 10 ++++++++--
> > > > > include/exec/memory.h | 19 ++++++++++++++++++-
> > > > > memory.c | 9 +++++++++
> > > > > 4 files changed, 44 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > > > > index f3ba9b9..6b33b9f 100644
> > > > > --- a/hw/vfio/common.c
> > > > > +++ b/hw/vfio/common.c
> > > > > @@ -478,8 +478,13 @@ static void
> > > > > vfio_listener_region_add(MemoryListener *listener,
> > > > > giommu->iommu_offset = section->offset_within_address_space -
> > > > > section->offset_within_region;
> > > > > giommu->container = container;
> > > > > - giommu->n.notify = vfio_iommu_map_notify;
> > > > > - giommu->n.notifier_flags = IOMMU_NOTIFIER_ALL;
> > > > > + llend =
> > > > > int128_add(int128_make64(section->offset_within_region),
> > > > > + section->size);
> > > > > + llend = int128_sub(llend, int128_one());
> > > > > + iommu_notifier_init(&giommu->n, vfio_iommu_map_notify,
> > > > > + IOMMU_NOTIFIER_ALL,
> > > > > + section->offset_within_region,
> > > > > + int128_get64(llend));
> > > >
> > > > Seems to me it would make sense to put the fiddling around to convert
> > > > the MemoryRegionSection into the necessary values would make sense to
> > > > go inside iommu_notifier_init().
> > >
> > > But will we always get one MemoryRegionSection if we are not in any of
> > > the region_{add|del} callback? E.g., what if we want to init an IOMMU
> > > notifier that covers just the whole IOMMU region range?
> >
> > I suppose so. It's just the only likely users of the interface I can
> > see will be always doing this from region_{add,del}.
> >
> > > Considering above, I would still slightly prefer current interface.
> >
> > Ok, well my opinion on the matter isn't terribly strong.
>
> Hi, David,
>
> Sorry to respond late (so that context might be lost). Just want to
> make sure that you are okay with current patch and interface, right?
>
> I think Marcel is going to merge it if np, and I would like to have
> your confirmation on this before the merge. Thanks!
Yes, that's fine.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
[Qemu-devel] [PATCH v9 2/9] memory: provide IOMMU_NOTIFIER_FOREACH macro, Peter Xu, 2017/04/07
[Qemu-devel] [PATCH v9 3/9] memory: provide iommu_replay_all(), Peter Xu, 2017/04/07
[Qemu-devel] [PATCH v9 4/9] memory: introduce memory_region_notify_one(), Peter Xu, 2017/04/07
[Qemu-devel] [PATCH v9 5/9] memory: add MemoryRegionIOMMUOps.replay() callback, Peter Xu, 2017/04/07
[Qemu-devel] [PATCH v9 6/9] intel_iommu: use the correct memory region for device IOTLB notification, Peter Xu, 2017/04/07
[Qemu-devel] [PATCH v9 7/9] intel_iommu: provide its own replay() callback, Peter Xu, 2017/04/07