Re: [PATCH] intel-iommu: Document iova_tree

[Peter Xu]

On Thu, Dec 01, 2022 at 07:17:41PM +0100, Eric Auger wrote:
> Hi Peter

Hi, Eric,

> 
> On 12/1/22 17:25, Peter Xu 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.
> >
> > Suggested-by: Jason Wang <jasowang@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  include/hw/i386/intel_iommu.h | 30 +++++++++++++++++++++++++++++-
> >  1 file changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> > index 46d973e629..8d130ab2e3 100644
> > --- a/include/hw/i386/intel_iommu.h
> > +++ b/include/hw/i386/intel_iommu.h
> > @@ -109,7 +109,35 @@ struct VTDAddressSpace {
> >      QLIST_ENTRY(VTDAddressSpace) next;
> >      /* Superset of notifier flags that this address space has */
> >      IOMMUNotifierFlag notifier_flags;
> > -    IOVATree *iova_tree;          /* Traces mapped IOVA ranges */
> > +    /*
> > +     * @iova_tree traces mapped IOVA ranges.
> > +     *
> > +     * The tree is not needed if no MAP notifiers is registered with
> 
> s/no MAP notifiers/no MAP notifier

Will fix.

> > +     * current VTD address space, because all UNMAP (including iotlb or
> > +     * dev-iotlb) events can be transparently delivered to !MAP iommu
> > +     * notifiers.
> because all UNMAP notifications (iotlb or dev-iotlb) can be triggered
> directly, as opposed to MAP notifications. (?)

What I wanted to say is any PSI or DSI messages we got from the guest can
be transparently delivered to QEMU's iommu notifiers.  I'm not sure
"triggered directly" best describe the case here.

PSI: Page Selective Invalidations
DSI: Domain Selective Invalidations

Sorry to mention these terms again, but that's really what the "transparent
delivery" means here - we get the PSI/DSI messages, then we notify with the
same ranges in IOMMU notifiers.  They're not the same concept but we do
that transparently without changing the core of the messages.

Maybe I should spell out "!MAP" as "UNMAP-only"?  Would that help?

> > +     *
> > +     * The tree OTOH is required for MAP typed iommu notifiers for a few
> > +     * reasons.
> > +     *
> > +     * Firstly, there's no way to identify whether an PSI event is MAP or
> maybe give the decryption of the 'PSI' and 'DSI" acronyms once ;-)

Please see above. :)

These are VT-d terms used in multiple places in the .[ch] files, I assume
I'll just keep using them because otherwise I'll need to comment them
everytime we use any PSI/DSI terms.  It might become an overkill I'm afraid.

> > +     * UNMAP within the PSI message itself.  Without having prior knowledge
> > +     * of existing state vIOMMU doesn't know whether it should notify MAP
> > +     * or UNMAP for a PSI message it received.
> > +     *
> > +     * Secondly, PSI received from guest driver (or even a large PSI can
> > +     * grow into a DSI at least with Linux intel-iommu driver) can be
> > +     * larger in range than the newly mapped ranges for either MAP or UNMAP
> > +     * events. If it directly pass-throughs any such event it may confuse
> 
> If it directly notifies the registered device with the unmodified range, it 
> may confuse the drivers ../..

Will fix.

> 
> So the range of the MAP notification can be adapted based on the existing 
> IOVA mappings.  

Yes, e.g. the iova tree makes sure we don't map something again if it's mapped.

Thanks,

> 
> > +     * the registered drivers (e.g. vfio-pci) on either: (1) trying to map
> > +     * the same region more than once (for VFIO_IOMMU_MAP_DMA, -EEXIST will
> > +     * trigger), or (2) trying to UNMAP a range that is still partially
> > +     * mapped.  That accuracy is not required for UNMAP-only notifiers, but
> > +     * it is a must-to-have for MAP-inclusive notifiers, because the vIOMMU
> > +     * needs to make sure the shadow page table is always in sync with the
> > +     * guest IOMMU pgtables for a device.
> > +     */
> > +    IOVATree *iova_tree;
> >  };
> >  
> >  struct VTDIOTLBEntry {
> Thanks
> 
> Eric
> 

-- 
Peter Xu