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

[Zhu, Lingshan]

On 6/8/2023 4:49 PM, Eugenio Perez Martin wrote:
> 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.
This interface is used to read the last_avail_idx, the ideal case
is to fetch it after device has been suspended.

If the device is not suspended, the value may be unreliable
because the device may keep consuming descriptors.
However at that moment, the guest may be freezed as well,
so the guest wouldn't supply more available descriptors to the device.

I think that means the last_avail_idx may not be reliable but still safe,
better than read nothing. Because the last_avail_idx always lags behind
the in-guest avail_idx.

I am not sure we can restore last_avail_idx with last_used_idx, there is 
always
a delta between them.

Thanks

> 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