diff mbox series

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

Message ID 20200901142608.24481-4-eperezma@redhat.com (mailing list archive)
State New, archived
Headers show
Series memory: Delete assertion in memory_region_unregister_iommu_notifier | expand

Commit Message

Eugenio Perez Martin Sept. 1, 2020, 2:26 p.m. UTC 
Adapt intel and vhost to use this new notification 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(-)

Comments

Peter Xu Sept. 1, 2020, 8:56 p.m. UTC | #1 
On Tue, Sep 01, 2020 at 04:26:06PM +0200, Eugenio Pérez wrote:
> Adapt intel and vhost to use this new notification type
> 
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>
Juan Quintela Sept. 2, 2020, 7:55 a.m. UTC | #2 
Eugenio Pérez <eperezma@redhat.com> wrote:
> Adapt intel and vhost to use this new notification type
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>
Eric Auger Sept. 2, 2020, 10:31 a.m. UTC | #3 
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
Eugenio Perez Martin Sept. 3, 2020, 10:13 a.m. UTC | #4 
Hi Eric,

On Wed, Sep 2, 2020 at 12:32 PM Auger Eric <eric.auger@redhat.com> wrote:
>
> 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.

Will do, thanks!

> >
> > 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??
> >
>

Agree, but I'm not sure about the right name. IOMMU_NOTIFIER_ALL_ROOT?
IOMMU_NOTIFIER_ALL_REGULAR?

Thanks!

> Thanks
>
> Eric
>
Eric Auger Sept. 3, 2020, 11:06 a.m. UTC | #5 
Hi Eugenio,
On 9/3/20 12:13 PM, Eugenio Perez Martin wrote:
> Hi Eric,
> 
> On Wed, Sep 2, 2020 at 12:32 PM Auger Eric <eric.auger@redhat.com> wrote:
>>
>> 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.
> 
> Will do, thanks!
> 
>>>
>>> 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??
>>>
>>
> 
> Agree, but I'm not sure about the right name. IOMMU_NOTIFIER_ALL_ROOT?
> IOMMU_NOTIFIER_ALL_REGULAR?
I would rather name it IOMMU_NOTIFIER_IOTLB_EVENTS versus
IOMMU_NOTIFIER_DEVIOTLB_EVENTS? This is the cache type that differs,
isn't it?

Thanks

Eric
> 
> Thanks!
> 
>> Thanks
>>
>> Eric
>>
> 
>
Eugenio Perez Martin Sept. 3, 2020, 12:21 p.m. UTC | #6 
On Thu, Sep 3, 2020 at 1:06 PM Auger Eric <eric.auger@redhat.com> wrote:
>
> Hi Eugenio,
> On 9/3/20 12:13 PM, Eugenio Perez Martin wrote:
> > Hi Eric,
> >
> > On Wed, Sep 2, 2020 at 12:32 PM Auger Eric <eric.auger@redhat.com> wrote:
> >>
> >> 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.
> >
> > Will do, thanks!
> >
> >>>
> >>> 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??
> >>>
> >>
> >
> > Agree, but I'm not sure about the right name. IOMMU_NOTIFIER_ALL_ROOT?
> > IOMMU_NOTIFIER_ALL_REGULAR?
> I would rather name it IOMMU_NOTIFIER_IOTLB_EVENTS versus
> IOMMU_NOTIFIER_DEVIOTLB_EVENTS? This is the cache type that differs,
> isn't it?
>

Ok will propose it.

Thanks!

> Thanks
>
> Eric
> >
> > Thanks!
> >
> >> Thanks
> >>
> >> Eric
> >>
> >
> >
>
diff mbox series

Patch

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;
     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)