Re: [Qemu-devel] [PATCH] memory: do not do out of bound notification

[Auger Eric <eric.auger@redhat.com>]

Hi Yan,

[+ Peter]

On 6/19/19 10:49 AM, Yan Zhao wrote:
> even if an entry overlaps with notifier's range, should not map/unmap
> out of bound part in the entry.

I don't think the patch was based on the master as the trace at the very
end if not part of the upstream code.
> 
> This would cause problem in below case:
> 1. initially there are two notifiers with ranges
> 0-0xfedfffff, 0xfef00000-0xffffffffffffffff,
> IOVAs from 0x3c000000 - 0x3c1fffff is in shadow page table.
> 
> 2. in vfio, memory_region_register_iommu_notifier() is followed by
> memory_region_iommu_replay(), which will first call address space unmap,
> and walk and add back all entries in vtd shadow page table. e.g.
> (1) for notifier 0-0xfedfffff,
>     IOVAs from 0 - 0xffffffff get unmapped,
>     and IOVAs from 0x3c000000 - 0x3c1fffff get mapped

While the patch looks sensible, the issue is the notifier scope used in
vtd_address_space_unmap is not a valid mask (ctpop64(size) != 1). Then
the size is recomputed (either using n = 64 - clz64(size) for the 1st
notifier or n = s->aw_bits for the 2d) and also the entry (especially
for the 2d notifier where it becomes 0) to get a proper alignment.

vtd_page_walk sends notifications per block or page (with valid
addr_mask) so stays within the notifier.

Modifying the entry->iova/addr_mask again in memory_region_notify_one
leads to unaligned start address / addr_mask. I don't think we want that.

Can't we modity the vtd_address_space_unmap() implementation to split
the invalidation in smaller chunks instead?


Thanks

Eric


> (2) for notifier 0xfef00000-0xffffffffffffffff
>     IOVAs from 0 - 0x7fffffffff get unmapped,>     but IOVAs from 0x3c000000 - 0x3c1fffff cannot get mapped back.
> 
> Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> ---
>  memory.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/memory.c b/memory.c
> index 07c8315..a6b9da6 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1948,6 +1948,14 @@ void memory_region_notify_one(IOMMUNotifier *notifier,
>          return;
>      }
>  
> +    if (entry->iova < notifier->start) {
> +        entry->iova = notifier->start;
> +    }
> +
> +    if (entry->iova + entry->addr_mask > notifier->end) {
> +        entry->addr_mask = notifier->end - entry->iova;> +    }
> +
>      if (entry->perm & IOMMU_RW) {
>          printf("map %lx %lx\n", entry->iova, entry->iova + entry->addr_mask);
>          request_flags = IOMMU_NOTIFIER_MAP;

>