On Wed, Aug 19, 2020 at 9:15 AM Jason Wang <jasow...@redhat.com> wrote:
>
>
> On 2020/8/18 下午10:24, Eugenio Perez Martin wrote:
> > On Wed, Aug 12, 2020 at 10:49 AM Eugenio Perez Martin
> > <epere...@redhat.com> wrote:
> >> On Wed, Aug 12, 2020 at 4:24 AM Jason Wang<jasow...@redhat.com> wrote:
> >>> On 2020/8/12 上午1:55, Eugenio Pérez wrote:
> >>>> Signed-off-by: Eugenio Pérez<epere...@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.
> >>>
> >> Got it, will change.
> >>
> >>>> 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").
> >>>
> >> Ok will change.
> >>
> >>>> {
> >>>> 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.
> >>>
> >> It was proposed by Peter, but I understand as limiting the
> >> address+range we pass to the notifier. Although vhost seems to support
> >> it as long as it contains (notifier->start, notifier->end) in range, a
> >> future notifier might not.
>
>
> Yes, actually, I feel confused after reading the codes. Is
> notifier->start IOVA or GPA?
>
> In vfio.c, we did:
>
> iommu_notifier_init(&giommu->n, vfio_iommu_map_notify,
> IOMMU_NOTIFIER_ALL,
> section->offset_within_region,
> int128_get64(llend),
> iommu_idx);
>
> So it looks to me the start and end are GPA, but the assertion above
> check it against IOVA which seems to be wrong ....
>
> Thanks
>
I see.
I didn't go so deep, I just assumed that:
* all the addresses were GPA in the vhost-net+virtio-net case,
although the name iova in IOMMUTLBEntry.
* memory region was initialized with IOVA addresses in case of VFIO.
Maybe the comment should warn about the bad "iova" name, if I'm right?
I assumed that nothing changed in the VFIO case since its notifier has
no IOMMU_NOTIFIER_DEVIOTLB flag and the new conditional in
memory_region_notify_one_iommu, but I will test with a device
passthrough and DPDK again. Do you think another test would be needed?
Maybe Peter can go deeper on this.
Thanks!
>
> >>
> >> It could be done as iommu_entry_crop(IOMMUTLBEntry *entry, const
> >> IOMMUNotifier *notifier) though.
>
