Re: [PATCH V2 2/3] amd-iommu: Sync IOVA-to-GPA translation during page invalidation

[Peter Xu <peterx@redhat.com>]

On Fri, Oct 02, 2020 at 09:59:06AM -0500, Wei Huang wrote:
> +static void amdvi_sync_domain(AMDVIState *s, uint32_t domid,
> +                              uint64_t addr, uint16_t flags)
> +{

[...]

> +        /*
> +         * In case of syncing more than a page, we invalidate the entire
> +         * address range and re-walk the whole page table.
> +         */
> +        if (size == AMDVI_PGSZ_ENTIRE) {
> +            IOMMU_NOTIFIER_FOREACH(n, &as->iommu) {
> +                amdvi_address_space_unmap(as, n);
> +            }
> +        } else if (size > 0x1000) {
> +            IOMMU_NOTIFIER_FOREACH(n, &as->iommu) {
> +                if (n->start <= addr && addr + size < n->end) {
> +                    amdvi_address_space_unmap(as, n);
> +                }
> +            }
> +        }

So this may not work... Because DMA could happen concurrently with the page
sync, so the DMA could fail if the mapping is suddenly gone (even if it's a
very short period).

We encountered this problem on x86 and those will be reported as host DMAR
errors (since those pages were missing), please feel free to have a look at:

  63b88968f1 ("intel-iommu: rework the page walk logic", 2018-05-23)

Majorly this paragraph:

  For each VTDAddressSpace, now we maintain what IOVA ranges we have
  mapped and what we have not.  With that information, now we only send
  MAP or UNMAP when necessary.  Say, we don't send MAP notifies if we
  know we have already mapped the range, meanwhile we don't send UNMAP
  notifies if we know we never mapped the range at all.

So the simple idea is, as long as one page was mapped and it's not unmapped, we
can _never_ unmap it.. because the device might be using it simultaneously.

What VT-d does right now is to maintain a per-device iova tree, so that we know
what exactly have been mapped.  That's kind of an overkill for sure (especially
because vfio kernel module will maintain a similar iova tree, so it's at least
kind of duplicated), however that should fix this problem.

-- 
Peter Xu