> From: Jason Wang [mailto:jasow...@redhat.com]
> Sent: Friday, April 27, 2018 2:08 PM
>
> On 2018年04月25日 12:51, Peter Xu wrote:
> > For each VTDAddressSpace, now we maintain what IOVA ranges we have
> > mapped and what we have not. With that information, now we only
> send
> > MAP or UNMAP when necessary. Say, we don't send MAP notifies if we
> know
> > we have already mapped the range, meanwhile we don't send UNMAP
> notifies
> > if we know we never mapped the range at all.
> >
> > Signed-off-by: Peter Xu <pet...@redhat.com>
> > ---
> > include/hw/i386/intel_iommu.h | 2 ++
> > hw/i386/intel_iommu.c | 28 ++++++++++++++++++++++++++++
> > hw/i386/trace-events | 2 ++
> > 3 files changed, 32 insertions(+)
> >
> > diff --git a/include/hw/i386/intel_iommu.h
> b/include/hw/i386/intel_iommu.h
> > index 486e205e79..09a2e94404 100644
> > --- a/include/hw/i386/intel_iommu.h
> > +++ b/include/hw/i386/intel_iommu.h
> > @@ -27,6 +27,7 @@
> > #include "hw/i386/ioapic.h"
> > #include "hw/pci/msi.h"
> > #include "hw/sysbus.h"
> > +#include "qemu/interval-tree.h"
> >
> > #define TYPE_INTEL_IOMMU_DEVICE "intel-iommu"
> > #define INTEL_IOMMU_DEVICE(obj) \
> > @@ -95,6 +96,7 @@ struct VTDAddressSpace {
> > QLIST_ENTRY(VTDAddressSpace) next;
> > /* Superset of notifier flags that this address space has */
> > IOMMUNotifierFlag notifier_flags;
> > + ITTree *iova_tree; /* Traces mapped IOVA ranges */
> > };
> >
> > struct VTDBus {
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index a19c18b8d4..8f396a5d13 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -768,12 +768,37 @@ typedef struct {
> > static int vtd_page_walk_one(IOMMUTLBEntry *entry, int level,
> > vtd_page_walk_info *info)
> > {
> > + VTDAddressSpace *as = info->as;
> > vtd_page_walk_hook hook_fn = info->hook_fn;
> > void *private = info->private;
> > + ITRange *mapped = it_tree_find(as->iova_tree, entry->iova,
> > + entry->iova + entry->addr_mask);
> >
> > assert(hook_fn);
> > +
> > + /* Update local IOVA mapped ranges */
> > + if (entry->perm) {
> > + if (mapped) {
> > + /* Skip since we have already mapped this range */
> > + trace_vtd_page_walk_one_skip_map(entry->iova, entry-
> >addr_mask,
> > + mapped->start, mapped->end);
> > + return 0;
> > + }
> > + it_tree_insert(as->iova_tree, entry->iova,
> > + entry->iova + entry->addr_mask);
>
> I was consider a case e.g:
>
> 1) map A (iova) to B (pa)
> 2) invalidate A
> 3) map A (iova) to C (pa)
> 4) invalidate A
>
> In this case, we will probably miss a walk here. But I'm not sure it was
> allowed by the spec (though I think so).
>
I thought it was wrong in a glimpse, but then changed my mind after
another thinking. As long as device driver can quiescent the device
to not access A (iova) within above window, then above sequence
has no problem since any stale mappings (A->B) added before step 4)
won't be used and then will get flushed after step 4). Regarding to
that actually the 1st invalidation can be skipped:
1) map A (iova) to B (pa)
2) driver programs device to use A
3) driver programs device to not use A
4) map A (iova) to C (pa)
A->B may be still valid in IOTLB
5) invalidate A
6) driver programs device to use A
Of course above doesn't generate a sane IOMMU API framework,
just as Peter pointed out. But from hardware p.o.v it looks no
problem.
Thanks
Kevin
