[v4] Dynamically switch to vhost shadow virtqueues at vdpa net migration

[PATCH v4 06/15] vdpa: add vhost_vdpa->suspended parameter

Posted by Eugenio Pérez 2 years, 2 months ago

This allows vhost_vdpa to track if it is safe to get vring base from the
device or not.  If it is not, vhost can fall back to fetch idx from the
guest buffer again.

No functional change intended in this patch, later patches will use this
field.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 include/hw/virtio/vhost-vdpa.h | 2 ++
 hw/virtio/vhost-vdpa.c         | 8 ++++++++
 2 files changed, 10 insertions(+)

diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
index 7997f09a8d..4a7d396674 100644
--- a/include/hw/virtio/vhost-vdpa.h
+++ b/include/hw/virtio/vhost-vdpa.h
@@ -42,6 +42,8 @@ typedef struct vhost_vdpa {
     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;
     /* IOVA mapping used by the Shadow Virtqueue */
     VhostIOVATree *iova_tree;
     GPtrArray *shadow_vqs;
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 8cc9c98db9..228677895a 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -1227,6 +1227,14 @@ static int vhost_vdpa_get_vring_base(struct vhost_dev *dev,
         return 0;
     }
 
+    if (!v->suspended) {
+        /*
+         * Cannot trust in value returned by device, let vhost recover used
+         * idx from guest.
+         */
+        return -1;
+    }
+
     ret = vhost_vdpa_call(dev, VHOST_GET_VRING_BASE, ring);
     trace_vhost_vdpa_get_vring_base(dev, ring->index, ring->num);
     return ret;
-- 
2.31.1

Re: [PATCH v4 06/15] vdpa: add vhost_vdpa->suspended parameter

Posted by Jason Wang 2 years, 2 months ago

在 2023/2/24 23:54, Eugenio Pérez 写道:
> This allows vhost_vdpa to track if it is safe to get vring base from the
> device or not.  If it is not, vhost can fall back to fetch idx from the
> guest buffer again.
>
> No functional change intended in this patch, later patches will use this
> field.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>


I think we probably need to re-order the patch, e.g to let this come 
before at least patch 5.


> ---
>   include/hw/virtio/vhost-vdpa.h | 2 ++
>   hw/virtio/vhost-vdpa.c         | 8 ++++++++
>   2 files changed, 10 insertions(+)
>
> diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
> index 7997f09a8d..4a7d396674 100644
> --- a/include/hw/virtio/vhost-vdpa.h
> +++ b/include/hw/virtio/vhost-vdpa.h
> @@ -42,6 +42,8 @@ typedef struct vhost_vdpa {
>       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;


Should we implement the set/clear in this patch as well?

Thanks


>       /* IOVA mapping used by the Shadow Virtqueue */
>       VhostIOVATree *iova_tree;
>       GPtrArray *shadow_vqs;
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 8cc9c98db9..228677895a 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -1227,6 +1227,14 @@ static int vhost_vdpa_get_vring_base(struct vhost_dev *dev,
>           return 0;
>       }
>   
> +    if (!v->suspended) {
> +        /*
> +         * Cannot trust in value returned by device, let vhost recover used
> +         * idx from guest.
> +         */
> +        return -1;
> +    }
> +
>       ret = vhost_vdpa_call(dev, VHOST_GET_VRING_BASE, ring);
>       trace_vhost_vdpa_get_vring_base(dev, ring->index, ring->num);
>       return ret;

Re: [PATCH v4 06/15] vdpa: add vhost_vdpa->suspended parameter

Posted by Eugenio Perez Martin 2 years, 2 months ago

On Mon, Feb 27, 2023 at 8:24 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2023/2/24 23:54, Eugenio Pérez 写道:
> > This allows vhost_vdpa to track if it is safe to get vring base from the
> > device or not.  If it is not, vhost can fall back to fetch idx from the
> > guest buffer again.
> >
> > No functional change intended in this patch, later patches will use this
> > field.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>
>
> I think we probably need to re-order the patch, e.g to let this come
> before at least patch 5.
>

Right, that was a miss. I'll reorder them.

>
> > ---
> >   include/hw/virtio/vhost-vdpa.h | 2 ++
> >   hw/virtio/vhost-vdpa.c         | 8 ++++++++
> >   2 files changed, 10 insertions(+)
> >
> > diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
> > index 7997f09a8d..4a7d396674 100644
> > --- a/include/hw/virtio/vhost-vdpa.h
> > +++ b/include/hw/virtio/vhost-vdpa.h
> > @@ -42,6 +42,8 @@ typedef struct vhost_vdpa {
> >       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;
>
>
> Should we implement the set/clear in this patch as well?
>

I'd prefer to keep each patch separated in declaration / usage but
they can be squashed for sure.

Thanks!

> Thanks
>
>
> >       /* IOVA mapping used by the Shadow Virtqueue */
> >       VhostIOVATree *iova_tree;
> >       GPtrArray *shadow_vqs;
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index 8cc9c98db9..228677895a 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -1227,6 +1227,14 @@ static int vhost_vdpa_get_vring_base(struct vhost_dev *dev,
> >           return 0;
> >       }
> >
> > +    if (!v->suspended) {
> > +        /*
> > +         * Cannot trust in value returned by device, let vhost recover used
> > +         * idx from guest.
> > +         */
> > +        return -1;
> > +    }
> > +
> >       ret = vhost_vdpa_call(dev, VHOST_GET_VRING_BASE, ring);
> >       trace_vhost_vdpa_get_vring_base(dev, ring->index, ring->num);
> >       return ret;
>