Re: [PATCH v16 Kernel 5/7] vfio iommu: Update UNMAP_DMA ioctl to get dirty bitmap before unmap

[Kirti Wankhede]

On 3/27/2020 5:34 AM, Yan Zhao wrote:
> On Fri, Mar 27, 2020 at 05:39:44AM +0800, Kirti Wankhede wrote:
> > On 3/25/2020 7:48 AM, Yan Zhao wrote:
> > > On Wed, Mar 25, 2020 at 03:32:37AM +0800, Kirti Wankhede wrote:
> > > > DMA mapped pages, including those pinned by mdev vendor drivers, might
> > > > get unpinned and unmapped while migration is active and device is still
> > > > running. For example, in pre-copy phase while guest driver could access
> > > > those pages, host device or vendor driver can dirty these mapped pages.
> > > > Such pages should be marked dirty so as to maintain memory consistency
> > > > for a user making use of dirty page tracking.
> > > >
> > > > To get bitmap during unmap, user should allocate memory for bitmap, set
> > > > size of allocated memory, set page size to be considered for bitmap and
> > > > set flag VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP.
> > > >
> > > > Signed-off-by: Kirti Wankhede <address@hidden>
> > > > Reviewed-by: Neo Jia <address@hidden>
> > > > ---
> > > >    drivers/vfio/vfio_iommu_type1.c | 54 
> > > > ++++++++++++++++++++++++++++++++++++++---
> > > >    include/uapi/linux/vfio.h       | 10 ++++++++
> > > >    2 files changed, 60 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> > > > index 27ed069c5053..b98a8d79e13a 100644
> > > > --- a/drivers/vfio/vfio_iommu_type1.c
> > > > +++ b/drivers/vfio/vfio_iommu_type1.c
> > > > @@ -982,7 +982,8 @@ static int verify_bitmap_size(uint64_t npages, uint64_t 
> > > > bitmap_size)
> > > >    }
> > > >    
> > > >    static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> > > > -                            struct vfio_iommu_type1_dma_unmap *unmap)
> > > > +                            struct vfio_iommu_type1_dma_unmap *unmap,
> > > > +                            struct vfio_bitmap *bitmap)
> > > >    {
> > > >         uint64_t mask;
> > > >         struct vfio_dma *dma, *dma_last = NULL;
> > > > @@ -1033,6 +1034,10 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> > > >          * will be returned if these conditions are not met.  The v2 interface
> > > >          * will only return success and a size of zero if there were no
> > > >          * mappings within the range.
> > > > +        *
> > > > +        * When VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP flag is set, unmap request
> > > > +        * must be for single mapping. Multiple mappings with this flag set is
> > > > +        * not supported.
> > > >          */
> > > >         if (iommu->v2) {
> > > >                 dma = vfio_find_dma(iommu, unmap->iova, 1);
> > > > @@ -1040,6 +1045,13 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> > > >                         ret = -EINVAL;
> > > >                         goto unlock;
> > > >                 }
> > > > +
> > > > +               if ((unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) &&
> > > > +                   (dma->iova != unmap->iova || dma->size != unmap->size)) {
> > > potential NULL pointer!
> > >
> > > And could you address the comments in v14?
> > > How to handle DSI unmaps in vIOMMU
> > > (https://lore.kernel.org/kvm/20200323011041.GB5456@joy-OptiPlex-7040/)
> >
> > Sorry, I drafted reply to it, but I missed to send, it remained in my drafts
> >
> >   >
> >   > it happens in vIOMMU Domain level invalidation of IOTLB
> >   > (domain-selective invalidation, see vtd_iotlb_domain_invalidate() in
> > qemu).
> >   > common in VTD lazy mode, and NOT just happening once at boot time.
> >   > rather than invalidate page by page, it batches the page invalidation.
> >   > so, when this invalidation takes place, even higher level page tables
> >   > have been invalid and therefore it has to invalidate a bigger
> > combined range.
> >   > That's why we see IOVAs are mapped in 4k pages, but are unmapped in 2M
> >   > pages.
> >   >
> >   > I think those UNMAPs should also have GET_DIRTY_BIMTAP flag on, right?
> >
> >
> > vtd_iotlb_domain_invalidate()
> >     vtd_sync_shadow_page_table()
> >       vtd_sync_shadow_page_table_range(vtd_as, &ce, 0, UINT64_MAX)
> >         vtd_page_walk()
> >           vtd_page_walk_level() - walk over specific level for IOVA range
> >             vtd_page_walk_one()
> >               memory_region_notify_iommu()
> >               ...
> >                 vfio_iommu_map_notify()
> >
> > In the above trace, isn't page walk will take care of creating proper
> > IOTLB entry which should be same as created during mapping for that
> > IOTLB entry?
> No. It does walk the page table, but as it's dsi (delay & batched unmap),
> pages table entry for a whole 2M (the higher level, not last level for 4K)
> range is invalid, so the iotlb->addr_mask what vfio_iommu_map_notify()
> receives is (2M - 1), not the same as the size for map.

When do this happen? during my testing I never hit this case. How can I 
hit this case?

In this case, will adjacent whole vfio_dmas will be clubbed together or 
will there be any intersection of vfio_dmas?

Thanks,
Kirti