Re: [PATCH] vdpa: dont check vhost_vdpa->suspended when unsupported

[Zhu, Lingshan]

On 6/9/2023 11:13 AM, Jason Wang wrote:
> On Wed, Jun 7, 2023 at 11:37 PM Eugenio Perez Martin
> <eperezma@redhat.com> wrote:
> > On Wed, Jun 7, 2023 at 11:09 AM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
> > > When read the state of a virtqueue, vhost_vdpa need
> > > to check whether the device is suspended.
> > >
> > > This commit verifies whether VHOST_BACKEND_F_SUSPEND is
> > > negotiated when checking vhost_vdpa->suspended.
> > I'll add: Otherwise, qemu prints XXX error message.
> >
> > On second thought, not returning an error prevents the caller
> > (vhost.c:vhost_virtqueue_stop) from recovering used idx from the
> > guest.
> >
> > I'm not sure about the right fix for this, should we call
> > virtio_queue_restore_last_avail_idx here?
> It should be the duty of the caller:
>
> E.g in vhost_virtqueue_stop() we had:
>
> =>  r = dev->vhost_ops->vhost_get_vring_base(dev, &state);
>      if (r < 0) {
>          VHOST_OPS_DEBUG(r, "vhost VQ %u ring restore failed: %d", idx, r);
>          /* Connection to the backend is broken, so let's sync internal
>           * last avail idx to the device used idx.
>           */
> virtio_queue_restore_last_avail_idx(vdev, idx);
>      } else {
>          virtio_queue_set_last_avail_idx(vdev, idx, state.num);
I agree call virtio_queue_restore_last_avail_idx here in the caller is
better. However I found this debug message is confused:

_get_vring_base() read the queue state, it does not restore anything,
and if failed to read the queue state, it invokes 
virtio_queue_restore_last_avail_idx()
to recover, still works, handled the problem internally.

So how about we remove this debug message or change it to:
restore last_avail_idx of vhost vq N from vring used_idx.

Thanks
> Thansk
>
> > Communicate that the backend
> > cannot suspend so vhost.c can print a better error message?
> >
> > Thanks!
> >
> > > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> > > ---
> > >   hw/virtio/vhost-vdpa.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > > index b3094e8a8b..ae176c06dd 100644
> > > --- a/hw/virtio/vhost-vdpa.c
> > > +++ b/hw/virtio/vhost-vdpa.c
> > > @@ -1397,7 +1397,7 @@ static int vhost_vdpa_get_vring_base(struct vhost_dev 
> > > *dev,
> > >           return 0;
> > >       }
> > >
> > > -    if (!v->suspended) {
> > > +    if ((dev->backend_cap & BIT_ULL(VHOST_BACKEND_F_SUSPEND)) && 
> > > (!v->suspended)) {
> > >           /*
> > >            * Cannot trust in value returned by device, let vhost recover used
> > >            * idx from guest.
> > > --
> > > 2.39.1