Re: [RFC v8 3/5] memory: Add IOMMU_DEVIOTLB_UNMAP IOMMUTLBNotificationType

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

Hi Eugenio,

On 9/1/20 4:26 PM, Eugenio Pérez wrote:
> Adapt intel and vhost to use this new notification type
I think you should explain in the commit message what is the benefice to
introduce this new event type.
> 
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  hw/i386/intel_iommu.c | 2 +-
>  hw/virtio/vhost.c     | 2 +-
>  include/exec/memory.h | 2 ++
>  3 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 0c4aef5cb5..cdddb089e7 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -2468,7 +2468,7 @@ static bool vtd_process_device_iotlb_desc(IntelIOMMUState *s,
>          sz = VTD_PAGE_SIZE;
>      }
>  
> -    event.type = IOMMU_NOTIFIER_UNMAP;
> +    event.type = IOMMU_NOTIFIER_DEVIOTLB;
If this is used only for device IOTLB cache invalidation, shouldn't this
be named IOMMU_NOTIFIER_DEVIOTLB_UNMAP to be consistent with the rest?
>      event.entry.target_as = &vtd_dev_as->as;
>      event.entry.addr_mask = sz - 1;
>      event.entry.iova = addr;
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 1a1384e7a6..6ca168b47e 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_DEVIOTLB,
>                          section->offset_within_region,
>                          int128_get64(end),
>                          iommu_idx);
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 8a56707169..215e23973d 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 device IOTLB entries */
> +    IOMMU_NOTIFIER_DEVIOTLB = 0x04,
>  } IOMMUNotifierFlag;
>  
>  #define IOMMU_NOTIFIER_ALL (IOMMU_NOTIFIER_MAP | IOMMU_NOTIFIER_UNMAP)
shouldn't we rename this one??
> 

Thanks

Eric