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

["Zhu, Lingshan" <lingshan.zhu@intel.com>]

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