Re: [PATCH v4 07/15] vdpa: add vhost_vdpa_suspend

[Eugenio Perez Martin]

On Wed, Mar 1, 2023 at 2:30 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>
>
>
> On 2/24/2023 7:54 AM, Eugenio Pérez wrote:
> > The function vhost.c:vhost_dev_stop fetches the vring base so the vq
> > state can be migrated to other devices.  However, this is unreliable in
> > vdpa, since we didn't signal the device to suspend the queues, making
> > the value fetched useless.
> >
> > Suspend the device if possible before fetching first and subsequent
> > vring bases.
> >
> > Moreover, vdpa totally reset and wipes the device at the last device
> > before fetch its vrings base, making that operation useless in the last
> > device. This will be fixed in later patches of this series.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> > v4:
> > * Look for _F_SUSPEND at vhost_dev->backend_cap, not backend_features
> > * Fall back on reset & fetch used idx from guest's memory
> > ---
> >   hw/virtio/vhost-vdpa.c | 25 +++++++++++++++++++++++++
> >   hw/virtio/trace-events |  1 +
> >   2 files changed, 26 insertions(+)
> >
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index 228677895a..f542960a64 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -712,6 +712,7 @@ static int vhost_vdpa_reset_device(struct vhost_dev 
> > *dev)
> >
> >       ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &status);
> >       trace_vhost_vdpa_reset_device(dev, status);
> > +    v->suspended = false;
> >       return ret;
> >   }
> >
> > @@ -1109,6 +1110,29 @@ static void vhost_vdpa_svqs_stop(struct vhost_dev 
> > *dev)
> >       }
> >   }
> >
> > +static void vhost_vdpa_suspend(struct vhost_dev *dev)
> > +{
> > +    struct vhost_vdpa *v = dev->opaque;
> > +    int r;
> > +
> > +    if (!vhost_vdpa_first_dev(dev)) {
> > +        return;
> > +    }
> > +
> > +    if (!(dev->backend_cap & BIT_ULL(VHOST_BACKEND_F_SUSPEND))) {
> Polarity reversed. This ends up device getting reset always even if the
> backend offers _F_SUSPEND.
>

Good catch, I'll fix it in v5.

Thanks!

> -Siwei
>
> > +        trace_vhost_vdpa_suspend(dev);
> > +        r = ioctl(v->device_fd, VHOST_VDPA_SUSPEND);
> > +        if (unlikely(r)) {
> > +            error_report("Cannot suspend: %s(%d)", g_strerror(errno), 
> > errno);
> > +        } else {
> > +            v->suspended = true;
> > +            return;
> > +        }
> > +    }
> > +
> > +    vhost_vdpa_reset_device(dev);
> > +}
> > +
> >   static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
> >   {
> >       struct vhost_vdpa *v = dev->opaque;
> > @@ -1123,6 +1147,7 @@ static int vhost_vdpa_dev_start(struct vhost_dev 
> > *dev, bool started)
> >           }
> >           vhost_vdpa_set_vring_ready(dev);
> >       } else {
> > +        vhost_vdpa_suspend(dev);
> >           vhost_vdpa_svqs_stop(dev);
> >           vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs);
> >       }
> > diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
> > index a87c5f39a2..8f8d05cf9b 100644
> > --- a/hw/virtio/trace-events
> > +++ b/hw/virtio/trace-events
> > @@ -50,6 +50,7 @@ vhost_vdpa_set_vring_ready(void *dev) "dev: %p"
> >   vhost_vdpa_dump_config(void *dev, const char *line) "dev: %p %s"
> >   vhost_vdpa_set_config(void *dev, uint32_t offset, uint32_t size, uint32_t 
> > flags) "dev: %p offset: %"PRIu32" size: %"PRIu32" flags: 0x%"PRIx32
> >   vhost_vdpa_get_config(void *dev, void *config, uint32_t config_len) "dev: 
> > %p config: %p config_len: %"PRIu32
> > +vhost_vdpa_suspend(void *dev) "dev: %p"
> >   vhost_vdpa_dev_start(void *dev, bool started) "dev: %p started: %d"
> >   vhost_vdpa_set_log_base(void *dev, uint64_t base, unsigned long long 
> > size, int refcnt, int fd, void *log) "dev: %p base: 0x%"PRIx64" size: %llu 
> > refcnt: %d fd: %d log: %p"
> >   vhost_vdpa_set_vring_addr(void *dev, unsigned int index, unsigned int 
> > flags, uint64_t desc_user_addr, uint64_t used_user_addr, uint64_t 
> > avail_user_addr, uint64_t log_guest_addr) "dev: %p index: %u flags: 0x%x 
> > desc_user_addr: 0x%"PRIx64" used_user_addr: 0x%"PRIx64" avail_user_addr: 
> > 0x%"PRIx64" log_guest_addr: 0x%"PRIx64
>