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

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

[Zhu Lingshan]

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.

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

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

[Eugenio Perez Martin]

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?

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
>

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

[Zhu, Lingshan]

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.

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

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

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

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

[Eugenio Perez Martin]

On Thu, Jun 8, 2023 at 11:34 AM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>
>
>
> 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.
>

Actually, if we cannot suspend the device we reset it, as we cannot
afford to modify used_idx.

I'm thinking now that your original idea was way better to be honest.
To not call vhost_get_vring_base in case the VM is shutting down and
we're not migrating seems to fit the situation way better. We save an
ioctl / potential device call for nothing.

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

Assuming we're migrating and we don't either reset or suspend the device
* guest set avail_idx=3
* device fetch last_avail_idx=3
* guest set avail_idx=6
* VM_SUSPEND
* we call unreliable get_vring_base, and obtain device state 3.
* device is not reset, so it reads guest's avail_idx = 6. It can
process the descriptors in avail_idx position 3, 4 and 5, and write
that used_idx.
* virtio_load detects that inconsistency, as (uint)last_avail_idx -
(uint)used_idx > vring size.

So I think we need to keep printing an error and recovery from the
guest as a fallback.

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

Yes, we assume it is safe to process these descriptors again or that
they'll be migrated when that is supported.

In any case, if a device does not work with this, we should be more
restrictive, not less.

Does it make sense to you?

Thanks!

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

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

[Zhu, Lingshan]

On 6/8/2023 6:59 PM, Eugenio Perez Martin wrote:
> On Thu, Jun 8, 2023 at 11:34 AM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>>
>>
>> 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.
>>
> Actually, if we cannot suspend the device we reset it, as we cannot
> afford to modify used_idx.
>
> I'm thinking now that your original idea was way better to be honest.
> To not call vhost_get_vring_base in case the VM is shutting down and
> we're not migrating seems to fit the situation way better. We save an
> ioctl / potential device call for nothing.
agree, but I failed to find a code patch indicting the VM is in
"shutting down" status, and it may be overkill to add
a new status field which only used here.

So maybe a fix like this:
1) if the device does not support _F_SUSPEND:
call virtio_queue_restore_last_avail_idx(dev, state->index)
2) if the device support _F_SUSPEND but failed to suspend:
return -1 and vhost_virtqueue_stop() will call 
virtio_queue_restore_last_avail_idx()
in the original code path.

Does this look good to you?
>
>> 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.
>>
> Assuming we're migrating and we don't either reset or suspend the device
> * guest set avail_idx=3
> * device fetch last_avail_idx=3
> * guest set avail_idx=6
> * VM_SUSPEND
> * we call unreliable get_vring_base, and obtain device state 3.
> * device is not reset, so it reads guest's avail_idx = 6. It can
> process the descriptors in avail_idx position 3, 4 and 5, and write
> that used_idx.
> * virtio_load detects that inconsistency, as (uint)last_avail_idx -
> (uint)used_idx > vring size.
>
> So I think we need to keep printing an error and recovery from the
> guest as a fallback.
so true, I think that is virtio_queue_restore_last_avail_idx()
>
>> I am not sure we can restore last_avail_idx with last_used_idx, there is
>> always
>> a delta between them.
>>
> Yes, we assume it is safe to process these descriptors again or that
> they'll be migrated when that is supported.
>
> In any case, if a device does not work with this, we should be more
> restrictive, not less.
>
> Does it make sense to you?
Yes, so if the device doesn't support _F_SUSPEND, we call 
virtio_queue_restore_last_avail_idx()
in vhost_vdpa_get_vring_base(), look good?

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

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

[Eugenio Perez Martin]

On Thu, Jun 8, 2023 at 3:38 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>
>
>
> On 6/8/2023 6:59 PM, Eugenio Perez Martin wrote:
> > On Thu, Jun 8, 2023 at 11:34 AM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
> >>
> >>
> >> 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.
> >>
> > Actually, if we cannot suspend the device we reset it, as we cannot
> > afford to modify used_idx.
> >
> > I'm thinking now that your original idea was way better to be honest.
> > To not call vhost_get_vring_base in case the VM is shutting down and
> > we're not migrating seems to fit the situation way better. We save an
> > ioctl / potential device call for nothing.
> agree, but I failed to find a code patch indicting the VM is in
> "shutting down" status, and it may be overkill to add
> a new status field which only used here.
>

Since vhost is stopped before migration of the net device state, I
think runstate_check returns differently depending on the case. If
migrating, runstate_check(RUN_STATE_SHUTDOWN) would return false. If
shutdown without migration, it would return true.

I didn't test it though.

Thanks!

> So maybe a fix like this:
> 1) if the device does not support _F_SUSPEND:
> call virtio_queue_restore_last_avail_idx(dev, state->index)
> 2) if the device support _F_SUSPEND but failed to suspend:
> return -1 and vhost_virtqueue_stop() will call
> virtio_queue_restore_last_avail_idx()
> in the original code path.
>
> Does this look good to you?
> >
> >> 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.
> >>
> > Assuming we're migrating and we don't either reset or suspend the device
> > * guest set avail_idx=3
> > * device fetch last_avail_idx=3
> > * guest set avail_idx=6
> > * VM_SUSPEND
> > * we call unreliable get_vring_base, and obtain device state 3.
> > * device is not reset, so it reads guest's avail_idx = 6. It can
> > process the descriptors in avail_idx position 3, 4 and 5, and write
> > that used_idx.
> > * virtio_load detects that inconsistency, as (uint)last_avail_idx -
> > (uint)used_idx > vring size.
> >
> > So I think we need to keep printing an error and recovery from the
> > guest as a fallback.
> so true, I think that is virtio_queue_restore_last_avail_idx()
> >
> >> I am not sure we can restore last_avail_idx with last_used_idx, there is
> >> always
> >> a delta between them.
> >>
> > Yes, we assume it is safe to process these descriptors again or that
> > they'll be migrated when that is supported.
> >
> > In any case, if a device does not work with this, we should be more
> > restrictive, not less.
> >
> > Does it make sense to you?
> Yes, so if the device doesn't support _F_SUSPEND, we call
> virtio_queue_restore_last_avail_idx()
> in vhost_vdpa_get_vring_base(), look good?
>
> Thanks
> >
> > Thanks!
> >
> >> 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
> >>>>>>
>

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

[Jason Wang]

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

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

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