On Wed, Jan 4, 2023 at 1:30 AM Peter Xu <pet...@redhat.com> wrote:
>
> 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?
I meant it's more safe to have a vtd version:
> > static void vtd_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);
Or move it to the memory.c.
>
> Meanwhile, I don't immediately see what's the side effect you mentioned for
> vfio map events.
I don't see but it looks more safe. Do you know the reason why SMMU
doesn't simply do a [0, ULONG_MAX] unmap notify? (Maybe Eric know)
> 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.
If we crop the ranges, the above description:
"""
and the UNMAP messages can be actually larger than the real invalidations.
"""
doesn't apply anymore.
Thanks
> 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
>
