| Message ID | 20200811175533.7359-2-eperezma@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | memory: Delete assertion in memory_region_unregister_iommu_notifier | expand |
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); > } > } >
On Wed, Aug 12, 2020 at 4:24 AM Jason Wang <jasowang@redhat.com> wrote: > > > 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. > 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. It could be done as iommu_entry_crop(IOMMUTLBEntry *entry, const IOMMUNotifier *notifier) though. > > > + } 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. > I think that if we transform that assert to a log, we should also tell the guest that something went wrong. Or would it be enough notifying the bad range? If a malicious guest cannot reach that point, I think that leaving it as an assertion allows us to detect earlier the fail in my opinion (Assert early and assert often). Thanks! > 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); > > } > > } > > >
On Wed, Aug 12, 2020 at 10:49 AM Eugenio Perez Martin <eperezma@redhat.com> wrote: > > On Wed, Aug 12, 2020 at 4:24 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > 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. > > > > 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. > > It could be done as iommu_entry_crop(IOMMUTLBEntry *entry, const > IOMMUNotifier *notifier) though. > > > > > > + } 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. > > > > I think that if we transform that assert to a log, we should also tell > the guest that something went wrong. Or would it be enough notifying > the bad range? > > If a malicious guest cannot reach that point, I think that leaving it > as an assertion allows us to detect earlier the fail in my opinion > (Assert early and assert often). > > Thanks! > Hi Jason. I just sent v4, please let me know if the changes are ok for you or you think it should be done otherwise. Is it ok for you to just let the assert, or you think we should still change to a conditional? Thanks! > > 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); > > > } > > > } > > > > >
On 2020/8/18 下午10:24, Eugenio Perez Martin wrote: > On Wed, Aug 12, 2020 at 10:49 AM Eugenio Perez Martin > <eperezma@redhat.com> wrote: >> On Wed, Aug 12, 2020 at 4:24 AM Jason Wang<jasowang@redhat.com> wrote: >>> 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. >>> >> 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 >> >> It could be done as iommu_entry_crop(IOMMUTLBEntry *entry, const >> IOMMUNotifier *notifier) though.
On Wed, Aug 19, 2020 at 9:15 AM Jason Wang <jasowang@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 > > <eperezma@redhat.com> wrote: > >> On Wed, Aug 12, 2020 at 4:24 AM Jason Wang<jasowang@redhat.com> wrote: > >>> 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. > >>> > >> 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. >
On 2020/8/19 下午4:22, Eugenio Perez Martin wrote: > On Wed, Aug 19, 2020 at 9:15 AM Jason Wang <jasowang@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 >>> <eperezma@redhat.com> wrote: >>>> On Wed, Aug 12, 2020 at 4:24 AM Jason Wang<jasowang@redhat.com> wrote: >>>>> 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. >>>>> >>>> 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. If there's no vIOMMU, IOVA = GPA, so we're fine. But if vIOMMU is enabled, IOVA allocation is done inside guest so the start/end here not IOVA anymore. > > 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? I'm not sure if it's easy, but it might be interesting to teach DPDK to use IOVA which is outside the range of [start, end] here. > > Maybe Peter can go deeper on this. Yes, or wait for Peter's comment. Thanks > > Thanks! > >>>> It could be done as iommu_entry_crop(IOMMUTLBEntry *entry, const >>>> IOMMUNotifier *notifier) though.
On Wed, Aug 19, 2020 at 03:15:26PM +0800, Jason Wang wrote: > 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 .... It should be iova; both section->offset_within_region and llend are for the device's iova address space. Thanks,
On 2020/8/19 下午11:50, Peter Xu wrote: > On Wed, Aug 19, 2020 at 03:15:26PM +0800, Jason Wang wrote: >> 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 .... > It should be iova; both section->offset_within_region and llend are for the > device's iova address space. Thanks, > Interesting, how can memory region know which IOVA is used by guest? Thanks
On Thu, Aug 20, 2020 at 10:28:00AM +0800, Jason Wang wrote: > > On 2020/8/19 下午11:50, Peter Xu wrote: > > On Wed, Aug 19, 2020 at 03:15:26PM +0800, Jason Wang wrote: > > > 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 .... > > It should be iova; both section->offset_within_region and llend are for the > > device's iova address space. Thanks, > > > > Interesting, how can memory region know which IOVA is used by guest? Does it need to know? :) AFAICT what we do here is only register with the whole possible IOVA address space (e.g., across the whole 64bit address space). Then vfio will get notifications when there're new iova ranges mapped into it.
On 2020/8/21 下午10:12, Peter Xu wrote: > On Thu, Aug 20, 2020 at 10:28:00AM +0800, Jason Wang wrote: >> On 2020/8/19 下午11:50, Peter Xu wrote: >>> On Wed, Aug 19, 2020 at 03:15:26PM +0800, Jason Wang wrote: >>>> 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 .... >>> It should be iova; both section->offset_within_region and llend are for the >>> device's iova address space. Thanks, >>> >> Interesting, how can memory region know which IOVA is used by guest? > Does it need to know? :) > > AFAICT what we do here is only register with the whole possible IOVA address > space (e.g., across the whole 64bit address space). Then vfio will get > notifications when there're new iova ranges mapped into it. Right, but the whole IOVA address space should be something vIOMMU specific, e.g for Intel it should be calculated by GAW, but I found: memory_region_init_iommu(&vtd_dev_as->iommu, sizeof(vtd_dev_as->iommu), TYPE_INTEL_IOMMU_MEMORY_REGION, OBJECT(s), name, UINT64_MAX); which assumes UINT64_MAX. Thanks >
On Tue, Sep 01, 2020 at 11:05:18AM +0800, Jason Wang wrote: > > On 2020/8/21 下午10:12, Peter Xu wrote: > > On Thu, Aug 20, 2020 at 10:28:00AM +0800, Jason Wang wrote: > > > On 2020/8/19 下午11:50, Peter Xu wrote: > > > > On Wed, Aug 19, 2020 at 03:15:26PM +0800, Jason Wang wrote: > > > > > 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 .... > > > > It should be iova; both section->offset_within_region and llend are for the > > > > device's iova address space. Thanks, > > > > > > > Interesting, how can memory region know which IOVA is used by guest? > > Does it need to know? :) > > > > AFAICT what we do here is only register with the whole possible IOVA address > > space (e.g., across the whole 64bit address space). Then vfio will get > > notifications when there're new iova ranges mapped into it. > > > Right, but the whole IOVA address space should be something vIOMMU specific, > e.g for Intel it should be calculated by GAW, but I found: > > memory_region_init_iommu(&vtd_dev_as->iommu, > sizeof(vtd_dev_as->iommu), > TYPE_INTEL_IOMMU_MEMORY_REGION, OBJECT(s), > name, UINT64_MAX); > > which assumes UINT64_MAX. Right. AFAICT it can be reduced to gaw width, but I don't see a problem either even with UINT64_MAX (as long as it covers the range specified by gaw). Or did I miss something? Thanks,
On 2020/9/2 上午3:35, Peter Xu wrote: > On Tue, Sep 01, 2020 at 11:05:18AM +0800, Jason Wang wrote: >> On 2020/8/21 下午10:12, Peter Xu wrote: >>> On Thu, Aug 20, 2020 at 10:28:00AM +0800, Jason Wang wrote: >>>> On 2020/8/19 下午11:50, Peter Xu wrote: >>>>> On Wed, Aug 19, 2020 at 03:15:26PM +0800, Jason Wang wrote: >>>>>> 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 .... >>>>> It should be iova; both section->offset_within_region and llend are for the >>>>> device's iova address space. Thanks, >>>>> >>>> Interesting, how can memory region know which IOVA is used by guest? >>> Does it need to know? :) >>> >>> AFAICT what we do here is only register with the whole possible IOVA address >>> space (e.g., across the whole 64bit address space). Then vfio will get >>> notifications when there're new iova ranges mapped into it. >> >> Right, but the whole IOVA address space should be something vIOMMU specific, >> e.g for Intel it should be calculated by GAW, but I found: >> >> memory_region_init_iommu(&vtd_dev_as->iommu, >> sizeof(vtd_dev_as->iommu), >> TYPE_INTEL_IOMMU_MEMORY_REGION, OBJECT(s), >> name, UINT64_MAX); >> >> which assumes UINT64_MAX. > Right. AFAICT it can be reduced to gaw width, but I don't see a problem either > even with UINT64_MAX (as long as it covers the range specified by gaw). Or did > I miss something? Dunno :) Just notice this difference, for safety, maybe its better to cap it with GAW. Btw, the naming of "vtd-ir" is kind of confusing, it should work without ir. Thanks > Thanks, >
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, 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, { 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; + } else { + assert(entry->iova >= notifier->start && entry_end <= notifier->end); + } 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); } }
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(-)