Re: [PATCH v2 2/4] intel_iommu: Fix a potential issue in VFIO dirty page sync

[Peter Xu]

On Tue, Jun 06, 2023 at 02:35:41AM +0000, Duan, Zhenzhong wrote:
> >-----Original Message-----
> >From: Peter Xu <peterx@redhat.com>
> >Sent: Tuesday, June 6, 2023 2:39 AM
> >To: Duan, Zhenzhong <zhenzhong.duan@intel.com>
> >Cc: qemu-devel@nongnu.org; mst@redhat.com; jasowang@redhat.com;
> >pbonzini@redhat.com; richard.henderson@linaro.org; eduardo@habkost.net;
> >marcel.apfelbaum@gmail.com; alex.williamson@redhat.com;
> >clg@redhat.com; david@redhat.com; philmd@linaro.org;
> >kwankhede@nvidia.com; cjia@nvidia.com; Liu, Yi L <yi.l.liu@intel.com>; Peng,
> >Chao P <chao.p.peng@intel.com>
> >Subject: Re: [PATCH v2 2/4] intel_iommu: Fix a potential issue in VFIO dirty
> >page sync
> >
> >On Thu, Jun 01, 2023 at 02:33:18PM +0800, Zhenzhong Duan wrote:
> >> Peter Xu found a potential issue:
> >>
> >> "The other thing is when I am looking at the new code I found that we
> >> actually extended the replay() to be used also in dirty tracking of vfio,
> >> in vfio_sync_dirty_bitmap().  For that maybe it's already broken if
> >> unmap_all() because afaiu log_sync() can be called in migration thread
> >> anytime during DMA so I think it means the device is prone to DMA with the
> >> IOMMU pgtable quickly erased and rebuilt here, which means the DMA
> >could
> >> fail unexpectedly.  Copy Alex, Kirti and Neo."
> >>
> >> To eliminate this small window with empty mapping, we should remove the
> >> call to unmap_all(). Besides that, introduce a new notifier type called
> >> IOMMU_NOTIFIER_FULL_MAP to get full mappings as intel_iommu only
> >notifies
> >> changed mappings while VFIO dirty page sync needs full mappings. Thanks
> >> to current implementation of iova tree, we could pick mappings from iova
> >> trees directly instead of walking through guest IOMMU page table.
> >>
> >> IOMMU_NOTIFIER_MAP is still used to get changed mappings for
> >optimization
> >> purpose. As long as notification for IOMMU_NOTIFIER_MAP could ensure
> >shadow
> >> page table in sync, then it's OK.
> >>
> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> >> ---
> >>  hw/i386/intel_iommu.c | 49 +++++++++++++++++++++++++++++++++++------
> >--
> >>  hw/vfio/common.c      |  2 +-
> >>  include/exec/memory.h | 13 ++++++++++++
> >>  softmmu/memory.c      |  4 ++++
> >>  4 files changed, 58 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> >> index 94d52f4205d2..061fcded0dfb 100644
> >> --- a/hw/i386/intel_iommu.c
> >> +++ b/hw/i386/intel_iommu.c
> >> @@ -3819,6 +3819,41 @@ static int vtd_replay_hook(IOMMUTLBEvent
> >*event, void *private)
> >>      return 0;
> >>  }
> >>
> >> +static gboolean vtd_replay_full_map(DMAMap *map, gpointer *private)
> >> +{
> >> +    IOMMUTLBEvent event;
> >> +
> >> +    event.type = IOMMU_NOTIFIER_MAP;
> >> +    event.entry.iova = map->iova;
> >> +    event.entry.addr_mask = map->size;
> >> +    event.entry.target_as = &address_space_memory;
> >> +    event.entry.perm = map->perm;
> >> +    event.entry.translated_addr = map->translated_addr;
> >> +
> >> +    return vtd_replay_hook(&event, private);
> >> +}
> >> +
> >> +/*
> >> + * This is a fast path to notify the full mappings falling in the scope
> >> + * of IOMMU notifier. The call site should ensure no iova tree update by
> >> + * taking necessary locks(e.x. BQL).
> >
> >We should be accurate on the locking - I think it's the BQL so far.
> 
> Will update comments.
> 
> >
> >> + */
> >> +static int vtd_page_walk_full_map_fast_path(IOVATree *iova_tree,
> >> +                                            IOMMUNotifier *n)
> >> +{
> >> +    DMAMap map;
> >> +
> >> +    map.iova = n->start;
> >> +    map.size = n->end - n->start;
> >> +    if (!iova_tree_find(iova_tree, &map)) {
> >> +        return 0;
> >> +    }
> >> +
> >> +    iova_tree_foreach_range_data(iova_tree, &map, vtd_replay_full_map,
> >> +                                 (gpointer *)n);
> >> +    return 0;
> >> +}
> >> +
> >>  static void vtd_iommu_replay(IOMMUMemoryRegion *iommu_mr,
> >IOMMUNotifier *n)
> >>  {
> >>      VTDAddressSpace *vtd_as = container_of(iommu_mr, VTDAddressSpace,
> >iommu);
> >> @@ -3826,13 +3861,6 @@ static void
> >vtd_iommu_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n)
> >>      uint8_t bus_n = pci_bus_num(vtd_as->bus);
> >>      VTDContextEntry ce;
> >>
> >> -    /*
> >> -     * The replay can be triggered by either a invalidation or a newly
> >> -     * created entry. No matter what, we release existing mappings
> >> -     * (it means flushing caches for UNMAP-only registers).
> >> -     */
> >> -    vtd_address_space_unmap(vtd_as, n);
> >> -
> >>      if (vtd_dev_to_context_entry(s, bus_n, vtd_as->devfn, &ce) == 0) {
> >>          trace_vtd_replay_ce_valid(s->root_scalable ? "scalable mode" :
> >>                                    "legacy mode",
> >> @@ -3850,8 +3878,11 @@ static void
> >vtd_iommu_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n)
> >>                  .as = vtd_as,
> >>                  .domain_id = vtd_get_domain_id(s, &ce, vtd_as->pasid),
> >>              };
> >> -
> >> -            vtd_page_walk(s, &ce, 0, ~0ULL, &info, vtd_as->pasid);
> >> +            if (n->notifier_flags & IOMMU_NOTIFIER_FULL_MAP) {
> >> +                vtd_page_walk_full_map_fast_path(vtd_as->iova_tree, n);
> >> +            } else {
> >> +                vtd_page_walk(s, &ce, 0, ~0ULL, &info, vtd_as->pasid);
> >> +            }
> >>          }
> >>      } else {
> >>          trace_vtd_replay_ce_invalid(bus_n, PCI_SLOT(vtd_as->devfn),
> >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> >> index 78358ede2764..5dae4502b908 100644
> >> --- a/hw/vfio/common.c
> >> +++ b/hw/vfio/common.c
> >> @@ -1890,7 +1890,7 @@ static int vfio_sync_dirty_bitmap(VFIOContainer
> >*container,
> >>
> >>                  iommu_notifier_init(&gdn.n,
> >>                                      vfio_iommu_map_dirty_notify,
> >> -                                    IOMMU_NOTIFIER_MAP,
> >> +                                    IOMMU_NOTIFIER_FULL_MAP,
> >>                                      section->offset_within_region,
> >>                                      int128_get64(llend),
> >>                                      idx);
> >> diff --git a/include/exec/memory.h b/include/exec/memory.h
> >> index c3661b2276c7..eecc3eec6702 100644
> >> --- a/include/exec/memory.h
> >> +++ b/include/exec/memory.h
> >> @@ -142,6 +142,10 @@ struct IOMMUTLBEntry {
> >>   *       events (e.g. VFIO). Both notifications must be accurate so that
> >>   *       the shadow page table is fully in sync with the guest view.
> >>   *
> >> + *       Besides MAP, there is a special use case called FULL_MAP which
> >> + *       requests notification for all the existent mappings (e.g. VFIO
> >> + *       dirty page sync).
> >
> >Why do we need FULL_MAP?  Can we simply reimpl MAP?
> 
> Sorry, I just realized IOMMU_NOTIFIER_FULL_MAP is confusing.
> Maybe IOMMU_NOTIFIER_MAP_FAST_PATH could be a bit more accurate.
> 
> IIUC, currently replay() is called from two paths, one is VFIO device address
> space switch which walks over the IOMMU page table to setup initial
> mapping and cache it in IOVA tree. The other is VFIO dirty sync which
> walks over the IOMMU page table to notify the mapping, because we
> already cache the mapping in IOVA tree and VFIO dirty sync is protected
> by BQL, so I think it's fine to pick mapping from IOVA tree directly instead
> of walking over IOMMU page table. That's the reason of FULL_MAP
> (IOMMU_NOTIFIER_MAP_FAST_PATH better).
> 
> About "reimpl MAP", do you mean to walk over IOMMU page table to
> notify all existing MAP events without checking with the IOVA tree for
> difference? If you prefer, I'll rewrite an implementation this way.

We still need to maintain iova tree. IIUC that's the major complexity of
vt-d emulation, because we have that extra cache layer to sync with the
real guest iommu pgtables.

But I think we were just wrong to also notify in the unmap_all() procedure.

IIUC the right thing to do (keeping replay() the interface as-is, per it
used to be defined) is we should replace the unmap_all() to only evacuate
the iova tree (keeping all host mappings untouched, IOW, don't notify
UNMAP), and do a full resync there, which will notify all existing mappings
as MAP.  Then we don't interrupt with any existing mapping if there is
(e.g. for the dirty sync case), meanwhile we keep sync too to latest (for
moving a vfio device into an existing iommu group).

Do you think that'll work for us?

-- 
Peter Xu