On Thu, Nov 2, 2023 at 9:48 AM Si-Wei Liu <si-wei....@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 shadow_data member 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 <epere...@redhat.com>
> > ---
> > include/hw/virtio/vhost-vdpa.h | 5 +++--
> > hw/virtio/vhost-vdpa.c | 6 +++---
> > net/vhost-vdpa.c | 23 ++++++-----------------
> > 3 files changed, 12 insertions(+), 22 deletions(-)
> >
> > diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
> > index 8d52a7e498..01e0f25e27 100644
> > --- a/include/hw/virtio/vhost-vdpa.h
> > +++ b/include/hw/virtio/vhost-vdpa.h
> > @@ -36,6 +36,9 @@ typedef struct vhost_vdpa_shared {
> >
> > /* IOVA mapping used by the Shadow Virtqueue */
> > VhostIOVATree *iova_tree;
> > +
> > + /* Vdpa must send shadow addresses as IOTLB key for data queues, not
> > GPA */
> > + bool shadow_data;
> > } VhostVDPAShared;
> >
> > typedef struct vhost_vdpa {
> > @@ -47,8 +50,6 @@ typedef struct vhost_vdpa {
> > MemoryListener listener;
> > uint64_t acked_features;
> > bool shadow_vqs_enabled;
> > - /* Vdpa must send shadow addresses as IOTLB key for data queues, not
> > GPA */
> > - bool shadow_data;
> > /* Device suspended successfully */
> > bool suspended;
> > VhostVDPAShared *shared;
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index 2bceadd118..ec028e4c56 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -353,7 +353,7 @@ static void
> > vhost_vdpa_listener_region_add(MemoryListener *listener,
> > vaddr, section->readonly);
> >
> > llsize = int128_sub(llend, int128_make64(iova));
> > - if (v->shadow_data) {
> > + if (v->shared->shadow_data) {
> > int r;
> >
> > mem_region.translated_addr = (hwaddr)(uintptr_t)vaddr,
> > @@ -380,7 +380,7 @@ static void
> > vhost_vdpa_listener_region_add(MemoryListener *listener,
> > return;
> >
> > fail_map:
> > - if (v->shadow_data) {
> > + if (v->shared->shadow_data) {
> > vhost_iova_tree_remove(v->shared->iova_tree, mem_region);
> > }
> >
> > @@ -435,7 +435,7 @@ static void
> > vhost_vdpa_listener_region_del(MemoryListener *listener,
> >
> > llsize = int128_sub(llend, int128_make64(iova));
> >
> > - if (v->shadow_data) {
> > + if (v->shared->shadow_data) {
> > const DMAMap *result;
> > const void *vaddr = memory_region_get_ram_ptr(section->mr) +
> > section->offset_within_region +
> > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > index 9648b0ef7e..01202350ea 100644
> > --- a/net/vhost-vdpa.c
> > +++ b/net/vhost-vdpa.c
> > @@ -282,15 +282,6 @@ static ssize_t vhost_vdpa_receive(NetClientState *nc,
> > const uint8_t *buf,
> > return size;
> > }
> >
> > -/** From any vdpa net client, get the netclient of the first queue pair */
> > -static VhostVDPAState *vhost_vdpa_net_first_nc_vdpa(VhostVDPAState *s)
> > -{
> > - NICState *nic = qemu_get_nic(s->nc.peer);
> > - NetClientState *nc0 = qemu_get_peer(nic->ncs, 0);
> > -
> > - return DO_UPCAST(VhostVDPAState, nc, nc0);
> > -}
> > -
> > static void vhost_vdpa_net_log_global_enable(VhostVDPAState *s, bool
> > enable)
> > {
> > struct vhost_vdpa *v = &s->vhost_vdpa;
> > @@ -360,10 +351,10 @@ static int vhost_vdpa_net_data_start(NetClientState
> > *nc)
> > if (s->always_svq ||
> > migration_is_setup_or_active(migrate_get_current()->state)) {
> > v->shadow_vqs_enabled = true;
> > - v->shadow_data = true;
> > + v->shared->shadow_data = true;
> > } else {
> > v->shadow_vqs_enabled = false;
> > - v->shadow_data = false;
> > + v->shared->shadow_data = false;
> > }
> >
> > if (v->index == 0) {
> > @@ -513,7 +504,7 @@ dma_map_err:
> >
> > static int vhost_vdpa_net_cvq_start(NetClientState *nc)
> > {
> > - VhostVDPAState *s, *s0;
> > + VhostVDPAState *s;
> > struct vhost_vdpa *v;
> > int64_t cvq_group;
> > int r;
> > @@ -524,12 +515,10 @@ static int vhost_vdpa_net_cvq_start(NetClientState
> > *nc)
> > s = DO_UPCAST(VhostVDPAState, nc, nc);
> > v = &s->vhost_vdpa;
> >
> > - s0 = vhost_vdpa_net_first_nc_vdpa(s);
> > - v->shadow_data = s0->vhost_vdpa.shadow_vqs_enabled;
> > - v->shadow_vqs_enabled = s0->vhost_vdpa.shadow_vqs_enabled;
> > + v->shadow_vqs_enabled = s->always_svq;
> This doesn't seem equivalent to the previous code. If always_svq is not
> set and migration is active, will it cause CVQ not shadowed at all? The
> "goto out;" line below would effectively return from this function,
> resulting in cvq's shadow_vqs_enabled left behind as false.
>
You're right, this is probably a bad rebase. Thanks for the catch!
>
> > s->vhost_vdpa.address_space_id = VHOST_VDPA_GUEST_PA_ASID;
> >
> > - if (s->vhost_vdpa.shadow_data) {
> > + if (v->shared->shadow_data) {
> > /* SVQ is already configured for all virtqueues */
> > goto out;
> > }
> > @@ -1455,12 +1444,12 @@ static NetClientState
> > *net_vhost_vdpa_init(NetClientState *peer,
> > s->always_svq = svq;
> > s->migration_state.notify = vdpa_net_migration_state_notifier;
> > s->vhost_vdpa.shadow_vqs_enabled = svq;
> > - s->vhost_vdpa.shadow_data = svq;
> > if (queue_pair_index == 0) {
> > vhost_vdpa_net_valid_svq_features(features,
> >
> > &s->vhost_vdpa.migration_blocker);
> > s->vhost_vdpa.shared = g_new0(VhostVDPAShared, 1);
> > s->vhost_vdpa.shared->iova_range = iova_range;
> > + s->vhost_vdpa.shared->shadow_data = svq;
> > } else if (!is_datapath) {
> > s->cvq_cmd_out_buffer = mmap(NULL,
> > vhost_vdpa_net_cvq_cmd_page_len(),
> > PROT_READ | PROT_WRITE,
>
