Re: [PATCH v2 03/13] vdpa: add vhost_vdpa_suspend

[Eugenio Perez Martin]

On Tue, Feb 21, 2023 at 6:33 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2023/2/21 13:27, Jason Wang 写道:
> >
> > 在 2023/2/8 17:42, Eugenio Pérez 写道:
> >> 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.
> >
> >
> > It would be better not introduce a bug first and fix it in the
> > following patch.
> >
> >
> >>
> >> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >> ---
> >>   hw/virtio/vhost-vdpa.c | 19 +++++++++++++++++++
> >>   hw/virtio/trace-events |  1 +
> >>   2 files changed, 20 insertions(+)
> >>
> >> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> >> index 2e79fbe4b2..cbbe92ffe8 100644
> >> --- a/hw/virtio/vhost-vdpa.c
> >> +++ b/hw/virtio/vhost-vdpa.c
> >> @@ -1108,6 +1108,24 @@ 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) ||
> >
> >
> > Any reason we need to use vhost_vdpa_first_dev() instead of replacing the
> >
> > if (started) {
> > } else {
> >     vhost_vdpa_reset_device(dev);
> >     ....
> > }

I can also move the check to vhost_vdpa_dev_start, for sure.

>
> Ok, I think I kind of understand, so I think we need re-order the
> patches, at least patch 4 should come before this patch?
>

I think it is doable, yes. I'll check and come back to you.

Thanks!

> Thanks
>
>
> >
> >
> > We check
> >
> > if (dev->vq_index + dev->nvqs != dev->vq_index_end) in
> > vhost_vdpa_dev_start() but vhost_vdpa_first_dev() inside
> > vhost_vdpa_suspend(). This will result code that is hard to maintain.
> >
> > Thanks
> >
> >
> >> +        !(dev->backend_cap & BIT_ULL(VHOST_BACKEND_F_SUSPEND))) {
> >> +        return;
> >> +    }
> >> +
> >> +    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);
> >> +        /* Not aborting since we're called from stop context */
> >> +    }
> >> +}
> >> +
> >>   static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
> >>   {
> >>       struct vhost_vdpa *v = dev->opaque;
> >> @@ -1122,6 +1140,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
>