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

[Eugenio Perez Martin]

On Thu, Jun 8, 2023 at 9:07 AM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>
>
>
> On 6/7/2023 9:51 PM, Eugenio Perez Martin 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? Communicate that the backend
> > cannot suspend so vhost.c can print a better error message?
> I agree it is better not to return an error.
>
> Instead if neither the device does not support _F_SUSPEND nor failed to
> suspend,
> I think vhost_vdpa should try to read the last_avail_idx from
> the device anyway. We can print an error message here,
> like: failed to suspend, the value may not reliable.
>

We need to drop that value if it is not reliable. If the device keeps
using descriptors, used_idx may increase beyond avail_idx, so the
usual is to just restore whatever used_idx value the guest had at
stop.

Thanks!

> Thanks
> >
> > 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
> >>
>