Re: [PATCH QEMU v25 13/17] vfio: create mapped iova list when vIOMMU is enabled

[Peter Xu <peterx@redhat.com>]

On Thu, Jun 25, 2020 at 11:40:39AM -0600, Alex Williamson wrote:
> On Thu, 25 Jun 2020 20:04:08 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> 
> > On 6/25/2020 12:25 AM, Alex Williamson wrote:
> > > On Sun, 21 Jun 2020 01:51:22 +0530
> > > Kirti Wankhede <kwankhede@nvidia.com> wrote:
> > >   
> > >> Create mapped iova list when vIOMMU is enabled. For each mapped iova
> > >> save translated address. Add node to list on MAP and remove node from
> > >> list on UNMAP.
> > >> This list is used to track dirty pages during migration.  
> > > 
> > > This seems like a lot of overhead to support that the VM might migrate.
> > > Is there no way we can build this when we start migration, for example
> > > replaying the mappings at that time?  Thanks,
> > >   
> > 
> > In my previous version I tried to go through whole range and find valid 
> > iotlb, as below:
> > 
> > +        if (memory_region_is_iommu(section->mr)) {
> > +            iotlb = address_space_get_iotlb_entry(container->space->as, 
> > iova,
> > +                                                 true, 
> > MEMTXATTRS_UNSPECIFIED);
> > 
> > When mapping doesn't exist, qemu throws error as below:
> > 
> > qemu-system-x86_64: vtd_iova_to_slpte: detected slpte permission error 
> > (iova=0x0, level=0x3, slpte=0x0, write=1)
> > qemu-system-x86_64: vtd_iommu_translate: detected translation failure 
> > (dev=00:03:00, iova=0x0)
> > qemu-system-x86_64: New fault is not recorded due to compression of faults
> 
> My assumption would have been that we use the replay mechanism, which
> is known to work because we need to use it when we hot-add a device.
> We'd make use of iommu_notifier_init() to create a new handler for this
> purpose, then we'd walk our container->giommu_list and call
> memory_region_iommu_replay() for each.
> 
> Peter, does this sound like the right approach to you?

(Sorry I may not have the complete picture of this series, please bear with
 me...)

This seems to be a workable approach to me.  However then we might have a
similar mapping entry cached the 3rd time...  VFIO kernel has a copy initially,
then QEMU vIOMMU has another one (please grep iova_tree in intel_iommu.c).

My wild guess is that the mapping should still be in control in most cases, so
even if we cache it multiple times (for better layering) it would still be
fine.  However since we're in QEMU right now, I'm also thinking whether we can
share the information with the vIOMMU somehow, because even if the page table
entry is wiped off at that time we may still have a chance to use the DMAMap
object that cached in vIOMMU when iommu notify() happens.  Though that may
require some vIOMMU change too (e.g., vtd_page_walk_one may need to postpone
the iova_tree_remove to be after the hook_fn is called, also we may need to
pass the DMAMap object or at least the previous translated addr to the hook
somehow before removal), so maybe that can also be done on top.

> 
> > Secondly, it iterates through whole range with IOMMU page size 
> > granularity which is 4K, so it takes long time resulting in large 
> > downtime. With this optimization, downtime with vIOMMU reduced 
> > significantly.
> 
> Right, but we amortize that overhead and the resulting bloat across the
> 99.9999% of the time that we're not migrating.  I wonder if we could
> startup another thread to handle this when we enable dirty logging.  We
> don't really need the result until we start processing the dirty
> bitmap, right?  Also, if we're dealing with this many separate pages,
> shouldn't we be using a tree rather than a list to give us O(logN)
> rather than O(N)?

Yep I agree.  At least the vIOMMU cache is using gtree.

Btw, IIUC we won't walk the whole range using 4K granularity always, not for
VT-d emulation.  Because vtd_page_walk_level() is smart enough to skip higher
levels of invalid entries so it can jump with 2M/1G/... chunks if the whole
chunk is invalid.

Thanks,

-- 
Peter Xu