On Mon, Dec 26, 2022 at 12:09:52PM +0800, Jason Wang wrote:
> On Sat, Dec 24, 2022 at 12:26 AM Peter Xu <pet...@redhat.com> wrote:
> >
> > On Fri, Dec 23, 2022 at 03:48:01PM +0800, Jason Wang wrote:
> > > On Wed, Dec 7, 2022 at 6:13 AM Peter Xu <pet...@redhat.com> wrote:
> > > >
> > > > It seems not super clear on when iova_tree is used, and why. Add a rich
> > > > comment above iova_tree to track why we needed the iova_tree, and when
> > > > we
> > > > need it.
> > > >
> > > > Also comment for the map/unmap messages, on how they're used and
> > > > implications (e.g. unmap can be larger than the mapped ranges).
> > > >
> > > > Suggested-by: Jason Wang <jasow...@redhat.com>
> > > > Signed-off-by: Peter Xu <pet...@redhat.com>
> > > > ---
> > > > v3:
> > > > - Adjust according to Eric's comment
> > > > ---
> > > > include/exec/memory.h | 28 ++++++++++++++++++++++++++
> > > > include/hw/i386/intel_iommu.h | 38 ++++++++++++++++++++++++++++++++++-
> > > > 2 files changed, 65 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > > > index 91f8a2395a..269ecb873b 100644
> > > > --- a/include/exec/memory.h
> > > > +++ b/include/exec/memory.h
> > > > @@ -129,6 +129,34 @@ struct IOMMUTLBEntry {
> > > > /*
> > > > * Bitmap for different IOMMUNotifier capabilities. Each notifier can
> > > > * register with one or multiple IOMMU Notifier capability bit(s).
> > > > + *
> > > > + * Normally there're two use cases for the notifiers:
> > > > + *
> > > > + * (1) When the device needs accurate synchronizations of the vIOMMU
> > > > page
> > > > + * tables, it needs to register with both MAP|UNMAP notifies
> > > > (which
> > > > + * is defined as IOMMU_NOTIFIER_IOTLB_EVENTS below).
> > > > + *
> > > > + * Regarding to accurate synchronization, it's when the notified
> > > > + * device maintains a shadow page table and must be notified on
> > > > each
> > > > + * guest MAP (page table entry creation) and UNMAP (invalidation)
> > > > + * events (e.g. VFIO). Both notifications must be accurate so
> > > > that
> > > > + * the shadow page table is fully in sync with the guest view.
> > > > + *
> > > > + * (2) When the device doesn't need accurate synchronizations of the
> > > > + * vIOMMU page tables, it needs to register only with UNMAP or
> > > > + * DEVIOTLB_UNMAP notifies.
> > > > + *
> > > > + * It's when the device maintains a cache of IOMMU translations
> > > > + * (IOTLB) and is able to fill that cache by requesting
> > > > translations
> > > > + * from the vIOMMU through a protocol similar to ATS (Address
> > > > + * Translation Service).
> > > > + *
> > > > + * Note that in this mode the vIOMMU will not maintain a shadowed
> > > > + * page table for the address space, and the UNMAP messages can
> > > > be
> > > > + * actually larger than the real invalidations (just like how the
> > > > + * Linux IOMMU driver normally works, where an invalidation can
> > > > be
> > > > + * enlarged as long as it still covers the target range). The
> > > > IOMMU
> > >
> > > Just spot this when testing your fix for DSI:
> > >
> > > assert(entry->iova >= notifier->start && entry_end <=
> > > notifier->end);
> > >
> > > Do we need to remove this (but it seems a partial revert of
> > > 03c7140c1a0336af3d4fca768de791b9c0e2b128)?
> >
> > Replied in the othe thread.
> >
> > I assume this documentation patch is still correct, am I right? It's
> > talking about the possibility of enlarged invalidation range sent from the
> > guest and vIOMMU. That should still not be bigger than the registered
> > range in iommu notifiers (even if bigger than the actual unmapped range).
>
> Adding Eugenio.
>
> So I think we need to evaluate the possible side effects to all the
> current nmap notifiers. For example the vfio_iommu_map_notify().
>
> And in another thread, if we crop the size, it basically means the
> notifier itself will still assume the range is valid, which is not
> what is documented in this patch.
>
> What's more interesting I see smmu had:
>
> /* Unmap the whole notifier's range */
> static void smmu_unmap_notifier_range(IOMMUNotifier *n)
> {
> IOMMUTLBEvent event;
>
> event.type = IOMMU_NOTIFIER_UNMAP;
> event.entry.target_as = &address_space_memory;
> event.entry.iova = n->start;
> event.entry.perm = IOMMU_NONE;
> event.entry.addr_mask = n->end - n->start;
>
> memory_region_notify_iommu_one(n, &event);
> }
>
> So it looks to me it's more safe to do something similar for vtd first.
Jason, could you elaborate more on this one?
Meanwhile, I don't immediately see what's the side effect you mentioned for
vfio map events. I thought any map event should always be in the notifier
range anyway because map event only comes in page sizes and generated by
vt-d page walkers (not guest driver, which is IIUC the only place where the
range of invalidation can be enlarged). So I don't expect any functional
change to map events if we decide to crop the ranges unconditionally. Did
I miss anything?
Thanks,
>
> Btw, I forgot the reason why we need to crop the size in the case of
> device IOTLB, Eguenio do you know that?
>
> Thanks
>
> >
> > Thanks,
> >
> > --
> > Peter Xu
> >
>
--
Peter Xu
