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.au...@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 <epere...@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
>>
>
>
