[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH 02/18] vdpa: move iova tree to the shared struct
|
From: |
Eugenio Perez Martin |
|
Subject: |
Re: [RFC PATCH 02/18] vdpa: move iova tree to the shared struct |
|
Date: |
Fri, 24 Nov 2023 18:11:44 +0100 |
On Thu, Nov 2, 2023 at 10:37 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>
>
>
> On 10/19/2023 7:34 AM, Eugenio Pérez wrote:
> > Next patches will register the vhost_vdpa memory listener while the VM
> > is migrating at the destination, so we can map the memory to the device
> > before stopping the VM at the source. The main goal is to reduce the
> > downtime.
> >
> > However, the destination QEMU is unaware of which vhost_vdpa device will
> > register its memory_listener. If the source guest has CVQ enabled, it
> > will be the CVQ device. Otherwise, it will be the first one.
> >
> > Move the iova tree to VhostVDPAShared so all vhost_vdpa can use it,
> > rather than always in the first or last vhost_vdpa.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> > include/hw/virtio/vhost-vdpa.h | 4 +--
> > hw/virtio/vhost-vdpa.c | 19 ++++++------
> > net/vhost-vdpa.c | 54 +++++++++++++++-------------------
> > 3 files changed, 35 insertions(+), 42 deletions(-)
> >
> > diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
> > index eb1a56d75a..ac036055d3 100644
> > --- a/include/hw/virtio/vhost-vdpa.h
> > +++ b/include/hw/virtio/vhost-vdpa.h
> > @@ -32,6 +32,8 @@ typedef struct VhostVDPAHostNotifier {
> >
> > /* Info shared by all vhost_vdpa device models */
> > typedef struct vhost_vdpa_shared {
> > + /* IOVA mapping used by the Shadow Virtqueue */
> > + VhostIOVATree *iova_tree;
> > } VhostVDPAShared;
> >
> > typedef struct vhost_vdpa {
> > @@ -48,8 +50,6 @@ typedef struct vhost_vdpa {
> > bool shadow_data;
> > /* Device suspended successfully */
> > bool suspended;
> > - /* IOVA mapping used by the Shadow Virtqueue */
> > - VhostIOVATree *iova_tree;
> > VhostVDPAShared *shared;
> > GPtrArray *shadow_vqs;
> > const VhostShadowVirtqueueOps *shadow_vq_ops;
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index 819b2d811a..9cee38cb6d 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -358,7 +358,7 @@ static void
> > vhost_vdpa_listener_region_add(MemoryListener *listener,
> > 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);
> > + r = vhost_iova_tree_map_alloc(v->shared->iova_tree, &mem_region);
> > if (unlikely(r != IOVA_OK)) {
> > error_report("Can't allocate a mapping (%d)", r);
> > goto fail;
> > @@ -379,7 +379,7 @@ static void
> > vhost_vdpa_listener_region_add(MemoryListener *listener,
> >
> > fail_map:
> > if (v->shadow_data) {
> > - vhost_iova_tree_remove(v->iova_tree, mem_region);
> > + vhost_iova_tree_remove(v->shared->iova_tree, mem_region);
> > }
> >
> > fail:
> > @@ -441,13 +441,13 @@ static void
> > vhost_vdpa_listener_region_del(MemoryListener *listener,
> > .size = int128_get64(llsize) - 1,
> > };
> >
> > - result = vhost_iova_tree_find_iova(v->iova_tree, &mem_region);
> > + result = vhost_iova_tree_find_iova(v->shared->iova_tree,
> > &mem_region);
> > if (!result) {
> > /* The memory listener map wasn't mapped */
> > return;
> > }
> > iova = result->iova;
> > - vhost_iova_tree_remove(v->iova_tree, *result);
> > + vhost_iova_tree_remove(v->shared->iova_tree, *result);
> > }
> > vhost_vdpa_iotlb_batch_begin_once(v);
> > /*
> > @@ -1059,7 +1059,8 @@ static void vhost_vdpa_svq_unmap_ring(struct
> > vhost_vdpa *v, hwaddr addr)
> > const DMAMap needle = {
> > .translated_addr = addr,
> > };
> > - const DMAMap *result = vhost_iova_tree_find_iova(v->iova_tree,
> > &needle);
> > + const DMAMap *result = vhost_iova_tree_find_iova(v->shared->iova_tree,
> > + &needle);
> > hwaddr size;
> > int r;
> >
> > @@ -1075,7 +1076,7 @@ static void vhost_vdpa_svq_unmap_ring(struct
> > vhost_vdpa *v, hwaddr addr)
> > return;
> > }
> >
> > - vhost_iova_tree_remove(v->iova_tree, *result);
> > + vhost_iova_tree_remove(v->shared->iova_tree, *result);
> > }
> >
> > static void vhost_vdpa_svq_unmap_rings(struct vhost_dev *dev,
> > @@ -1103,7 +1104,7 @@ static bool vhost_vdpa_svq_map_ring(struct vhost_vdpa
> > *v, DMAMap *needle,
> > {
> > int r;
> >
> > - r = vhost_iova_tree_map_alloc(v->iova_tree, needle);
> > + r = vhost_iova_tree_map_alloc(v->shared->iova_tree, needle);
> > if (unlikely(r != IOVA_OK)) {
> > error_setg(errp, "Cannot allocate iova (%d)", r);
> > return false;
> > @@ -1115,7 +1116,7 @@ static bool vhost_vdpa_svq_map_ring(struct vhost_vdpa
> > *v, DMAMap *needle,
> > needle->perm == IOMMU_RO);
> > if (unlikely(r != 0)) {
> > error_setg_errno(errp, -r, "Cannot map region to device");
> > - vhost_iova_tree_remove(v->iova_tree, *needle);
> > + vhost_iova_tree_remove(v->shared->iova_tree, *needle);
> > }
> >
> > return r == 0;
> > @@ -1216,7 +1217,7 @@ static bool vhost_vdpa_svqs_start(struct vhost_dev
> > *dev)
> > goto err;
> > }
> >
> > - vhost_svq_start(svq, dev->vdev, vq, v->iova_tree);
> > + vhost_svq_start(svq, dev->vdev, vq, v->shared->iova_tree);
> > ok = vhost_vdpa_svq_map_rings(dev, svq, &addr, &err);
> > if (unlikely(!ok)) {
> > goto err_map;
> > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > index a2f9855288..15e7579b13 100644
> > --- a/net/vhost-vdpa.c
> > +++ b/net/vhost-vdpa.c
> > @@ -345,8 +345,8 @@ static void
> > vhost_vdpa_net_data_start_first(VhostVDPAState *s)
> >
> > add_migration_state_change_notifier(&s->migration_state);
> > if (v->shadow_vqs_enabled) {
> > - v->iova_tree = vhost_iova_tree_new(v->iova_range.first,
> > - v->iova_range.last);
> > + v->shared->iova_tree = vhost_iova_tree_new(v->iova_range.first,
> > + v->iova_range.last);
> This code change is okay so far without .load_setup involved, but if
> interacting with .load_setup the iova_tree can be NULL when x-svq=on.
> Below is a stacktrace showing the problem.
>
Right, the next version of the series including .load_setup will take
this into account. Thanks!
> #0 0x00005582bf00944c in vhost_iova_tree_map_alloc (tree=0x0,
> map=map@entry=0x7fb16bfffde0) at ../hw/virtio/vhost-iova-tree.c:89
> #1 0x00005582bee8cdb3 in vhost_vdpa_listener_region_add
> (listener=0x5582c138ee58, section=0x7fb16bfffe50)
> at ../include/qemu/int128.h:33
> #2 0x00005582bf029d4b in memory_listener_register (as=0x5582bfb53d20
> <address_space_memory>, listener=0x5582c138ee58)
> at ../system/memory.c:3026
> #3 0x00005582bf029d4b in memory_listener_register
> (listener=0x5582c138ee58, as=0x5582bfb53d20 <address_space_memory>)
> at ../system/memory.c:3096
> #4 0x00005582bee8e712 in vhost_vdpa_load_setup (shared=0x5582c138ee50,
> dma_as=0x5582bfb53d20 <address_space_memory>)
> at ../hw/virtio/vhost-vdpa.c:1550
> #5 0x00005582bef0b7df in vhost_vdpa_net_load_setup (nc=0x7fb172a27010,
> nic=<optimized out>) at ../net/vhost-vdpa.c:415
> #6 0x00005582beeeb4f5 in qemu_loadvm_state (f=0x5582c1c1a800) at
> ../migration/savevm.c:2682
> #7 0x00005582beeeb4f5 in qemu_loadvm_state (f=0x5582c1c1a800) at
> ../migration/savevm.c:2866
> #8 0x00005582beed5e17 in process_incoming_migration_co
> (opaque=<optimized out>) at ../migration/migration.c:548
> #9 0x00005582bf21b29b in coroutine_trampoline (i0=<optimized out>,
> i1=<optimized out>) at ../util/coroutine-ucontext.c:177
> #10 0x00007fb16e448190 in __start_context () at /lib64/libc.so.6
>
> -Siwei
> > }
> > }
> >
> > @@ -371,11 +371,6 @@ static int vhost_vdpa_net_data_start(NetClientState
> > *nc)
> > return 0;
> > }
> >
> > - if (v->shadow_vqs_enabled) {
> > - VhostVDPAState *s0 = vhost_vdpa_net_first_nc_vdpa(s);
> > - v->iova_tree = s0->vhost_vdpa.iova_tree;
> > - }
> > -
> > return 0;
> > }
> >
> > @@ -408,9 +403,8 @@ static void vhost_vdpa_net_client_stop(NetClientState
> > *nc)
> >
> > dev = s->vhost_vdpa.dev;
> > if (dev->vq_index + dev->nvqs == dev->vq_index_end) {
> > - g_clear_pointer(&s->vhost_vdpa.iova_tree, vhost_iova_tree_delete);
> > - } else {
> > - s->vhost_vdpa.iova_tree = NULL;
> > + g_clear_pointer(&s->vhost_vdpa.shared->iova_tree,
> > + vhost_iova_tree_delete);
> > }
> > }
> >
> > @@ -464,7 +458,7 @@ static int vhost_vdpa_set_address_space_id(struct
> > vhost_vdpa *v,
> >
> > static void vhost_vdpa_cvq_unmap_buf(struct vhost_vdpa *v, void *addr)
> > {
> > - VhostIOVATree *tree = v->iova_tree;
> > + VhostIOVATree *tree = v->shared->iova_tree;
> > DMAMap needle = {
> > /*
> > * No need to specify size or to look for more translations since
> > @@ -498,7 +492,7 @@ static int vhost_vdpa_cvq_map_buf(struct vhost_vdpa *v,
> > void *buf, size_t size,
> > map.translated_addr = (hwaddr)(uintptr_t)buf;
> > map.size = size - 1;
> > map.perm = write ? IOMMU_RW : IOMMU_RO,
> > - r = vhost_iova_tree_map_alloc(v->iova_tree, &map);
> > + r = vhost_iova_tree_map_alloc(v->shared->iova_tree, &map);
> > if (unlikely(r != IOVA_OK)) {
> > error_report("Cannot map injected element");
> > return r;
> > @@ -513,7 +507,7 @@ static int vhost_vdpa_cvq_map_buf(struct vhost_vdpa *v,
> > void *buf, size_t size,
> > return 0;
> >
> > dma_map_err:
> > - vhost_iova_tree_remove(v->iova_tree, map);
> > + vhost_iova_tree_remove(v->shared->iova_tree, map);
> > return r;
> > }
> >
> > @@ -573,24 +567,22 @@ out:
> > return 0;
> > }
> >
> > - if (s0->vhost_vdpa.iova_tree) {
> > - /*
> > - * SVQ is already configured for all virtqueues. Reuse IOVA tree
> > for
> > - * simplicity, whether CVQ shares ASID with guest or not, because:
> > - * - Memory listener need access to guest's memory addresses
> > allocated
> > - * in the IOVA tree.
> > - * - There should be plenty of IOVA address space for both ASID
> > not to
> > - * worry about collisions between them. Guest's translations are
> > - * still validated with virtio virtqueue_pop so there is no risk
> > for
> > - * the guest to access memory that it shouldn't.
> > - *
> > - * To allocate a iova tree per ASID is doable but it complicates
> > the
> > - * code and it is not worth it for the moment.
> > - */
> > - v->iova_tree = s0->vhost_vdpa.iova_tree;
> > - } else {
> > - v->iova_tree = vhost_iova_tree_new(v->iova_range.first,
> > - v->iova_range.last);
> > + /*
> > + * If other vhost_vdpa already have an iova_tree, reuse it for
> > simplicity,
> > + * whether CVQ shares ASID with guest or not, because:
> > + * - Memory listener need access to guest's memory addresses allocated
> > in
> > + * the IOVA tree.
> > + * - There should be plenty of IOVA address space for both ASID not to
> > + * worry about collisions between them. Guest's translations are
> > still
> > + * validated with virtio virtqueue_pop so there is no risk for the
> > guest
> > + * to access memory that it shouldn't.
> > + *
> > + * To allocate a iova tree per ASID is doable but it complicates the
> > code
> > + * and it is not worth it for the moment.
> > + */
> > + if (!v->shared->iova_tree) {
> > + v->shared->iova_tree = vhost_iova_tree_new(v->iova_range.first,
> > + v->iova_range.last);
> > }
> >
> > r = vhost_vdpa_cvq_map_buf(&s->vhost_vdpa, s->cvq_cmd_out_buffer,
>