Re: [PATCH v3] intel-iommu: Document iova_tree

[Peter Xu]

On Mon, Dec 26, 2022 at 12:09:52PM +0800, Jason Wang wrote:
> On Sat, Dec 24, 2022 at 12:26 AM Peter Xu <peterx@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 <peterx@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 <jasowang@redhat.com>
> > > > Signed-off-by: Peter Xu <peterx@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