| Message ID | 20220802143908.274364-1-eperezma@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | vdpa: do not save failed dma maps in SVQ iova tree | expand |
On Tue, Aug 2, 2022 at 4:41 PM Eugenio Pérez <eperezma@redhat.com> wrote: > > If a map fails for whatever reason, it must not be saved in the tree. > Otherwise, qemu will try to unmap it in cleanup, leaving to more errors. > I forgot to add: Reported-by: Lei Yang <leiyang@redhat.com> > Fixes: 34e3c94eda ("vdpa: Add custom IOTLB translations to SVQ") > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > --- > hw/virtio/vhost-vdpa.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > index 3ff9ce3501..e44c23dce5 100644 > --- a/hw/virtio/vhost-vdpa.c > +++ b/hw/virtio/vhost-vdpa.c > @@ -176,6 +176,7 @@ static void vhost_vdpa_listener_commit(MemoryListener *listener) > static void vhost_vdpa_listener_region_add(MemoryListener *listener, > MemoryRegionSection *section) > { > + DMAMap mem_region = {}; > struct vhost_vdpa *v = container_of(listener, struct vhost_vdpa, listener); > hwaddr iova; > Int128 llend, llsize; > @@ -212,13 +213,13 @@ static void vhost_vdpa_listener_region_add(MemoryListener *listener, > > llsize = int128_sub(llend, int128_make64(iova)); > if (v->shadow_vqs_enabled) { > - DMAMap mem_region = { > - .translated_addr = (hwaddr)(uintptr_t)vaddr, > - .size = int128_get64(llsize) - 1, > - .perm = IOMMU_ACCESS_FLAG(true, section->readonly), > - }; > + int r; > > - int r = vhost_iova_tree_map_alloc(v->iova_tree, &mem_region); > + mem_region.translated_addr = (hwaddr)(uintptr_t)vaddr, > + mem_region.size = int128_get64(llsize) - 1, > + mem_region.perm = IOMMU_ACCESS_FLAG(true, section->readonly), > + > + r = vhost_iova_tree_map_alloc(v->iova_tree, &mem_region); > if (unlikely(r != IOVA_OK)) { > error_report("Can't allocate a mapping (%d)", r); > goto fail; > @@ -232,11 +233,16 @@ static void vhost_vdpa_listener_region_add(MemoryListener *listener, > vaddr, section->readonly); > if (ret) { > error_report("vhost vdpa map fail!"); > - goto fail; > + goto fail_map; > } > > return; > > +fail_map: > + if (v->shadow_vqs_enabled) { > + vhost_iova_tree_remove(v->iova_tree, &mem_region); > + } > + > fail: > /* > * On the initfn path, store the first error in the container so we > -- > 2.31.1 > >
On Tue, Aug 2, 2022 at 10:39 PM Eugenio Pérez <eperezma@redhat.com> wrote: > > If a map fails for whatever reason, it must not be saved in the tree. > Otherwise, qemu will try to unmap it in cleanup, leaving to more errors. > > Fixes: 34e3c94eda ("vdpa: Add custom IOTLB translations to SVQ") > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > --- > hw/virtio/vhost-vdpa.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > index 3ff9ce3501..e44c23dce5 100644 > --- a/hw/virtio/vhost-vdpa.c > +++ b/hw/virtio/vhost-vdpa.c > @@ -176,6 +176,7 @@ static void vhost_vdpa_listener_commit(MemoryListener *listener) > static void vhost_vdpa_listener_region_add(MemoryListener *listener, > MemoryRegionSection *section) > { > + DMAMap mem_region = {}; > struct vhost_vdpa *v = container_of(listener, struct vhost_vdpa, listener); > hwaddr iova; > Int128 llend, llsize; > @@ -212,13 +213,13 @@ static void vhost_vdpa_listener_region_add(MemoryListener *listener, > > llsize = int128_sub(llend, int128_make64(iova)); > if (v->shadow_vqs_enabled) { > - DMAMap mem_region = { > - .translated_addr = (hwaddr)(uintptr_t)vaddr, > - .size = int128_get64(llsize) - 1, > - .perm = IOMMU_ACCESS_FLAG(true, section->readonly), > - }; Nit: can we keep this part unchanged? Thanks > + int r; > > - int r = vhost_iova_tree_map_alloc(v->iova_tree, &mem_region); > + mem_region.translated_addr = (hwaddr)(uintptr_t)vaddr, > + mem_region.size = int128_get64(llsize) - 1, > + mem_region.perm = IOMMU_ACCESS_FLAG(true, section->readonly), > + > + r = vhost_iova_tree_map_alloc(v->iova_tree, &mem_region); > if (unlikely(r != IOVA_OK)) { > error_report("Can't allocate a mapping (%d)", r); > goto fail; > @@ -232,11 +233,16 @@ static void vhost_vdpa_listener_region_add(MemoryListener *listener, > vaddr, section->readonly); > if (ret) { > error_report("vhost vdpa map fail!"); > - goto fail; > + goto fail_map; > } > > return; > > +fail_map: > + if (v->shadow_vqs_enabled) { > + vhost_iova_tree_remove(v->iova_tree, &mem_region); > + } > + > fail: > /* > * On the initfn path, store the first error in the container so we > -- > 2.31.1 >
On Wed, Aug 3, 2022 at 10:09 AM Jason Wang <jasowang@redhat.com> wrote: > > On Tue, Aug 2, 2022 at 10:39 PM Eugenio Pérez <eperezma@redhat.com> wrote: > > > > If a map fails for whatever reason, it must not be saved in the tree. > > Otherwise, qemu will try to unmap it in cleanup, leaving to more errors. > > > > Fixes: 34e3c94eda ("vdpa: Add custom IOTLB translations to SVQ") > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > --- > > hw/virtio/vhost-vdpa.c | 20 +++++++++++++------- > > 1 file changed, 13 insertions(+), 7 deletions(-) > > > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > > index 3ff9ce3501..e44c23dce5 100644 > > --- a/hw/virtio/vhost-vdpa.c > > +++ b/hw/virtio/vhost-vdpa.c > > @@ -176,6 +176,7 @@ static void vhost_vdpa_listener_commit(MemoryListener *listener) > > static void vhost_vdpa_listener_region_add(MemoryListener *listener, > > MemoryRegionSection *section) > > { > > + DMAMap mem_region = {}; > > struct vhost_vdpa *v = container_of(listener, struct vhost_vdpa, listener); > > hwaddr iova; > > Int128 llend, llsize; > > @@ -212,13 +213,13 @@ static void vhost_vdpa_listener_region_add(MemoryListener *listener, > > > > llsize = int128_sub(llend, int128_make64(iova)); > > if (v->shadow_vqs_enabled) { > > - DMAMap mem_region = { > > - .translated_addr = (hwaddr)(uintptr_t)vaddr, > > - .size = int128_get64(llsize) - 1, > > - .perm = IOMMU_ACCESS_FLAG(true, section->readonly), > > - }; > > Nit: can we keep this part unchanged? > We can, but that implies we should look for iova again at fail_map tag. If you are ok with that I'm fine to perform the search again. > Thanks > > > + int r; > > > > - int r = vhost_iova_tree_map_alloc(v->iova_tree, &mem_region); > > + mem_region.translated_addr = (hwaddr)(uintptr_t)vaddr, > > + mem_region.size = int128_get64(llsize) - 1, > > + mem_region.perm = IOMMU_ACCESS_FLAG(true, section->readonly), > > + > > + r = vhost_iova_tree_map_alloc(v->iova_tree, &mem_region); > > if (unlikely(r != IOVA_OK)) { > > error_report("Can't allocate a mapping (%d)", r); > > goto fail; > > @@ -232,11 +233,16 @@ static void vhost_vdpa_listener_region_add(MemoryListener *listener, > > vaddr, section->readonly); > > if (ret) { > > error_report("vhost vdpa map fail!"); > > - goto fail; > > + goto fail_map; > > } > > > > return; > > > > +fail_map: > > + if (v->shadow_vqs_enabled) { > > + vhost_iova_tree_remove(v->iova_tree, &mem_region); > > + } > > + > > fail: > > /* > > * On the initfn path, store the first error in the container so we > > -- > > 2.31.1 > > >
在 2022/8/3 16:12, Eugenio Perez Martin 写道: > On Wed, Aug 3, 2022 at 10:09 AM Jason Wang <jasowang@redhat.com> wrote: >> On Tue, Aug 2, 2022 at 10:39 PM Eugenio Pérez <eperezma@redhat.com> wrote: >>> If a map fails for whatever reason, it must not be saved in the tree. >>> Otherwise, qemu will try to unmap it in cleanup, leaving to more errors. >>> >>> Fixes: 34e3c94eda ("vdpa: Add custom IOTLB translations to SVQ") >>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com> >>> --- >>> hw/virtio/vhost-vdpa.c | 20 +++++++++++++------- >>> 1 file changed, 13 insertions(+), 7 deletions(-) >>> >>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c >>> index 3ff9ce3501..e44c23dce5 100644 >>> --- a/hw/virtio/vhost-vdpa.c >>> +++ b/hw/virtio/vhost-vdpa.c >>> @@ -176,6 +176,7 @@ static void vhost_vdpa_listener_commit(MemoryListener *listener) >>> static void vhost_vdpa_listener_region_add(MemoryListener *listener, >>> MemoryRegionSection *section) >>> { >>> + DMAMap mem_region = {}; >>> struct vhost_vdpa *v = container_of(listener, struct vhost_vdpa, listener); >>> hwaddr iova; >>> Int128 llend, llsize; >>> @@ -212,13 +213,13 @@ static void vhost_vdpa_listener_region_add(MemoryListener *listener, >>> >>> llsize = int128_sub(llend, int128_make64(iova)); >>> if (v->shadow_vqs_enabled) { >>> - DMAMap mem_region = { >>> - .translated_addr = (hwaddr)(uintptr_t)vaddr, >>> - .size = int128_get64(llsize) - 1, >>> - .perm = IOMMU_ACCESS_FLAG(true, section->readonly), >>> - }; >> Nit: can we keep this part unchanged? >> > We can, but that implies we should look for iova again at fail_map > tag. If you are ok with that I'm fine to perform the search again. I meant something like: diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c index 9a2daef7e3..edf40868e3 100644 --- a/hw/virtio/vhost-vdpa.c +++ b/hw/virtio/vhost-vdpa.c @@ -232,11 +232,15 @@ static void vhost_vdpa_listener_region_add(MemoryListener *listener, vaddr, section->readonly); if (ret) { error_report("vhost vdpa map fail!"); - goto fail; + goto fail_unmap; } return; +fail_unmap: + if (v->shadow_vqs_enabled) { + vhost_iova_tree_remove(v->iova_tree, &mem_region); + } fail: /* * On the initfn path, store the first error in the container so we Thanks > >> Thanks >> >>> + int r; >>> >>> - int r = vhost_iova_tree_map_alloc(v->iova_tree, &mem_region); >>> + mem_region.translated_addr = (hwaddr)(uintptr_t)vaddr, >>> + mem_region.size = int128_get64(llsize) - 1, >>> + mem_region.perm = IOMMU_ACCESS_FLAG(true, section->readonly), >>> + >>> + r = vhost_iova_tree_map_alloc(v->iova_tree, &mem_region); >>> if (unlikely(r != IOVA_OK)) { >>> error_report("Can't allocate a mapping (%d)", r); >>> goto fail; >>> @@ -232,11 +233,16 @@ static void vhost_vdpa_listener_region_add(MemoryListener *listener, >>> vaddr, section->readonly); >>> if (ret) { >>> error_report("vhost vdpa map fail!"); >>> - goto fail; >>> + goto fail_map; >>> } >>> >>> return; >>> >>> +fail_map: >>> + if (v->shadow_vqs_enabled) { >>> + vhost_iova_tree_remove(v->iova_tree, &mem_region); >>> + } >>> + >>> fail: >>> /* >>> * On the initfn path, store the first error in the container so we >>> -- >>> 2.31.1 >>>
On Thu, Aug 4, 2022 at 6:51 AM Jason Wang <jasowang@redhat.com> wrote: > > > 在 2022/8/3 16:12, Eugenio Perez Martin 写道: > > On Wed, Aug 3, 2022 at 10:09 AM Jason Wang <jasowang@redhat.com> wrote: > >> On Tue, Aug 2, 2022 at 10:39 PM Eugenio Pérez <eperezma@redhat.com> wrote: > >>> If a map fails for whatever reason, it must not be saved in the tree. > >>> Otherwise, qemu will try to unmap it in cleanup, leaving to more errors. > >>> > >>> Fixes: 34e3c94eda ("vdpa: Add custom IOTLB translations to SVQ") > >>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > >>> --- > >>> hw/virtio/vhost-vdpa.c | 20 +++++++++++++------- > >>> 1 file changed, 13 insertions(+), 7 deletions(-) > >>> > >>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > >>> index 3ff9ce3501..e44c23dce5 100644 > >>> --- a/hw/virtio/vhost-vdpa.c > >>> +++ b/hw/virtio/vhost-vdpa.c > >>> @@ -176,6 +176,7 @@ static void vhost_vdpa_listener_commit(MemoryListener *listener) > >>> static void vhost_vdpa_listener_region_add(MemoryListener *listener, > >>> MemoryRegionSection *section) > >>> { > >>> + DMAMap mem_region = {}; > >>> struct vhost_vdpa *v = container_of(listener, struct vhost_vdpa, listener); > >>> hwaddr iova; > >>> Int128 llend, llsize; > >>> @@ -212,13 +213,13 @@ static void vhost_vdpa_listener_region_add(MemoryListener *listener, > >>> > >>> llsize = int128_sub(llend, int128_make64(iova)); > >>> if (v->shadow_vqs_enabled) { > >>> - DMAMap mem_region = { > >>> - .translated_addr = (hwaddr)(uintptr_t)vaddr, > >>> - .size = int128_get64(llsize) - 1, > >>> - .perm = IOMMU_ACCESS_FLAG(true, section->readonly), > >>> - }; > >> Nit: can we keep this part unchanged? > >> > > We can, but that implies we should look for iova again at fail_map > > tag. If you are ok with that I'm fine to perform the search again. > > > I meant something like: > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > index 9a2daef7e3..edf40868e3 100644 > --- a/hw/virtio/vhost-vdpa.c > +++ b/hw/virtio/vhost-vdpa.c > @@ -232,11 +232,15 @@ static void > vhost_vdpa_listener_region_add(MemoryListener *listener, > vaddr, section->readonly); > if (ret) { > error_report("vhost vdpa map fail!"); > - goto fail; > + goto fail_unmap; > } > > return; > > +fail_unmap: > + if (v->shadow_vqs_enabled) { > + vhost_iova_tree_remove(v->iova_tree, &mem_region); > + } > fail: > /* > * On the initfn path, store the first error in the container so we > Sorry, still not following. mem_region does not exist in the error path, since it's declared in the if (v->shadow_vqs_enabled){} block. We can left first part unchanged if we do a lookup for the mem region, based on the translated addr. Thanks! > Thanks > > > > > >> Thanks > >> > >>> + int r; > >>> > >>> - int r = vhost_iova_tree_map_alloc(v->iova_tree, &mem_region); > >>> + mem_region.translated_addr = (hwaddr)(uintptr_t)vaddr, > >>> + mem_region.size = int128_get64(llsize) - 1, > >>> + mem_region.perm = IOMMU_ACCESS_FLAG(true, section->readonly), > >>> + > >>> + r = vhost_iova_tree_map_alloc(v->iova_tree, &mem_region); > >>> if (unlikely(r != IOVA_OK)) { > >>> error_report("Can't allocate a mapping (%d)", r); > >>> goto fail; > >>> @@ -232,11 +233,16 @@ static void vhost_vdpa_listener_region_add(MemoryListener *listener, > >>> vaddr, section->readonly); > >>> if (ret) { > >>> error_report("vhost vdpa map fail!"); > >>> - goto fail; > >>> + goto fail_map; > >>> } > >>> > >>> return; > >>> > >>> +fail_map: > >>> + if (v->shadow_vqs_enabled) { > >>> + vhost_iova_tree_remove(v->iova_tree, &mem_region); > >>> + } > >>> + > >>> fail: > >>> /* > >>> * On the initfn path, store the first error in the container so we > >>> -- > >>> 2.31.1 > >>> >
On Thu, Aug 4, 2022 at 5:02 PM Eugenio Perez Martin <eperezma@redhat.com> wrote: > > On Thu, Aug 4, 2022 at 6:51 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > 在 2022/8/3 16:12, Eugenio Perez Martin 写道: > > > On Wed, Aug 3, 2022 at 10:09 AM Jason Wang <jasowang@redhat.com> wrote: > > >> On Tue, Aug 2, 2022 at 10:39 PM Eugenio Pérez <eperezma@redhat.com> wrote: > > >>> If a map fails for whatever reason, it must not be saved in the tree. > > >>> Otherwise, qemu will try to unmap it in cleanup, leaving to more errors. > > >>> > > >>> Fixes: 34e3c94eda ("vdpa: Add custom IOTLB translations to SVQ") > > >>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > >>> --- > > >>> hw/virtio/vhost-vdpa.c | 20 +++++++++++++------- > > >>> 1 file changed, 13 insertions(+), 7 deletions(-) > > >>> > > >>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > > >>> index 3ff9ce3501..e44c23dce5 100644 > > >>> --- a/hw/virtio/vhost-vdpa.c > > >>> +++ b/hw/virtio/vhost-vdpa.c > > >>> @@ -176,6 +176,7 @@ static void vhost_vdpa_listener_commit(MemoryListener *listener) > > >>> static void vhost_vdpa_listener_region_add(MemoryListener *listener, > > >>> MemoryRegionSection *section) > > >>> { > > >>> + DMAMap mem_region = {}; > > >>> struct vhost_vdpa *v = container_of(listener, struct vhost_vdpa, listener); > > >>> hwaddr iova; > > >>> Int128 llend, llsize; > > >>> @@ -212,13 +213,13 @@ static void vhost_vdpa_listener_region_add(MemoryListener *listener, > > >>> > > >>> llsize = int128_sub(llend, int128_make64(iova)); > > >>> if (v->shadow_vqs_enabled) { > > >>> - DMAMap mem_region = { > > >>> - .translated_addr = (hwaddr)(uintptr_t)vaddr, > > >>> - .size = int128_get64(llsize) - 1, > > >>> - .perm = IOMMU_ACCESS_FLAG(true, section->readonly), > > >>> - }; > > >> Nit: can we keep this part unchanged? > > >> > > > We can, but that implies we should look for iova again at fail_map > > > tag. If you are ok with that I'm fine to perform the search again. > > > > > > I meant something like: > > > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > > index 9a2daef7e3..edf40868e3 100644 > > --- a/hw/virtio/vhost-vdpa.c > > +++ b/hw/virtio/vhost-vdpa.c > > @@ -232,11 +232,15 @@ static void > > vhost_vdpa_listener_region_add(MemoryListener *listener, > > vaddr, section->readonly); > > if (ret) { > > error_report("vhost vdpa map fail!"); > > - goto fail; > > + goto fail_unmap; > > } > > > > return; > > > > +fail_unmap: > > + if (v->shadow_vqs_enabled) { > > + vhost_iova_tree_remove(v->iova_tree, &mem_region); > > + } > > fail: > > /* > > * On the initfn path, store the first error in the container so we > > > > Sorry, still not following. > > mem_region does not exist in the error path, since it's declared in > the if (v->shadow_vqs_enabled){} block. We can left first part > unchanged if we do a lookup for the mem region, based on the > translated addr. > Sending v2 expanding the fix without this comment, please let me know ifI misunderstood something. Thanks!
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c index 3ff9ce3501..e44c23dce5 100644 --- a/hw/virtio/vhost-vdpa.c +++ b/hw/virtio/vhost-vdpa.c @@ -176,6 +176,7 @@ static void vhost_vdpa_listener_commit(MemoryListener *listener) static void vhost_vdpa_listener_region_add(MemoryListener *listener, MemoryRegionSection *section) { + DMAMap mem_region = {}; struct vhost_vdpa *v = container_of(listener, struct vhost_vdpa, listener); hwaddr iova; Int128 llend, llsize; @@ -212,13 +213,13 @@ static void vhost_vdpa_listener_region_add(MemoryListener *listener, llsize = int128_sub(llend, int128_make64(iova)); if (v->shadow_vqs_enabled) { - DMAMap mem_region = { - .translated_addr = (hwaddr)(uintptr_t)vaddr, - .size = int128_get64(llsize) - 1, - .perm = IOMMU_ACCESS_FLAG(true, section->readonly), - }; + int r; - int r = vhost_iova_tree_map_alloc(v->iova_tree, &mem_region); + mem_region.translated_addr = (hwaddr)(uintptr_t)vaddr, + mem_region.size = int128_get64(llsize) - 1, + mem_region.perm = IOMMU_ACCESS_FLAG(true, section->readonly), + + r = vhost_iova_tree_map_alloc(v->iova_tree, &mem_region); if (unlikely(r != IOVA_OK)) { error_report("Can't allocate a mapping (%d)", r); goto fail; @@ -232,11 +233,16 @@ static void vhost_vdpa_listener_region_add(MemoryListener *listener, vaddr, section->readonly); if (ret) { error_report("vhost vdpa map fail!"); - goto fail; + goto fail_map; } return; +fail_map: + if (v->shadow_vqs_enabled) { + vhost_iova_tree_remove(v->iova_tree, &mem_region); + } + fail: /* * On the initfn path, store the first error in the container so we
If a map fails for whatever reason, it must not be saved in the tree. Otherwise, qemu will try to unmap it in cleanup, leaving to more errors. Fixes: 34e3c94eda ("vdpa: Add custom IOTLB translations to SVQ") Signed-off-by: Eugenio Pérez <eperezma@redhat.com> --- hw/virtio/vhost-vdpa.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)