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

[Jason Wang]

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.

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
>