Re: [RFC v3 1/1] memory: Skip bad range assertion if notifier supports arbitrary masks

[Jason Wang <jasowang@redhat.com>]

On 2020/8/12 上午1:55, Eugenio Pérez wrote:
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>   hw/virtio/vhost.c     |  2 +-
>   include/exec/memory.h |  2 ++
>   softmmu/memory.c      | 10 ++++++++--
>   3 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 1a1384e7a6..e74ad9e09b 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -729,7 +729,7 @@ static void vhost_iommu_region_add(MemoryListener *listener,
>       iommu_idx = memory_region_iommu_attrs_to_index(iommu_mr,
>                                                      MEMTXATTRS_UNSPECIFIED);
>       iommu_notifier_init(&iommu->n, vhost_iommu_unmap_notify,
> -                        IOMMU_NOTIFIER_UNMAP,
> +                        IOMMU_NOTIFIER_UNMAP | IOMMU_NOTIFIER_IOTLB,


I think we can safely drop IOMMU_NOTIFIER_UNMAP here since device IOTLB 
is sufficient.

Btw, IOMMU_NOTIFIER_IOTLB is kind of confusing, maybe something like 
IOMMU_NOTIFIER_DEVIOTLB.


>                           section->offset_within_region,
>                           int128_get64(end),
>                           iommu_idx);
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 307e527835..4d94c1e984 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -87,6 +87,8 @@ typedef enum {
>       IOMMU_NOTIFIER_UNMAP = 0x1,
>       /* Notify entry changes (newly created entries) */
>       IOMMU_NOTIFIER_MAP = 0x2,
> +    /* Notify changes on IOTLB entries */
> +    IOMMU_NOTIFIER_IOTLB = 0x04,
>   } IOMMUNotifierFlag;
>   
>   #define IOMMU_NOTIFIER_ALL (IOMMU_NOTIFIER_MAP | IOMMU_NOTIFIER_UNMAP)
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index af25987518..e2c5f6d0e7 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -1895,6 +1895,7 @@ void memory_region_notify_one(IOMMUNotifier *notifier,


(we probably need a better name of this function, at least something 
like "memory_region_iommu_notify_one").


>   {
>       IOMMUNotifierFlag request_flags;
>       hwaddr entry_end = entry->iova + entry->addr_mask;
> +    IOMMUTLBEntry tmp = *entry;
>   
>       /*
>        * Skip the notification if the notification does not overlap
> @@ -1904,7 +1905,12 @@ void memory_region_notify_one(IOMMUNotifier *notifier,
>           return;
>       }
>   
> -    assert(entry->iova >= notifier->start && entry_end <= notifier->end);
> +    if (notifier->notifier_flags & IOMMU_NOTIFIER_IOTLB) {
> +        tmp.iova = MAX(tmp.iova, notifier->start);
> +        tmp.addr_mask = MIN(entry_end, notifier->end) - tmp.iova;


Any reason for doing such re-calculation here, a comment would be helpful.


> +    } else {
> +        assert(entry->iova >= notifier->start && entry_end <= notifier->end);


I wonder if it's better to convert the assert so some kind of log or 
warn here.

Thanks


> +    }
>   
>       if (entry->perm & IOMMU_RW) {
>           request_flags = IOMMU_NOTIFIER_MAP;
> @@ -1913,7 +1919,7 @@ void memory_region_notify_one(IOMMUNotifier *notifier,
>       }
>   
>       if (notifier->notifier_flags & request_flags) {
> -        notifier->notify(notifier, entry);
> +        notifier->notify(notifier, &tmp);
>       }
>   }
>