Re: [PATCH V2] vhost_vdpa: no need to fetch vring base when poweroff

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

On 7/11/2023 10:50 AM, Jason Wang wrote:
> On Mon, Jul 10, 2023 at 4:53 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>> In the poweroff routine, no need to fetch last available index.
>>
> This is because there's no concept of shutdown in the vhost layer, it
> only knows start and stop.
>
>> This commit also provides a better debug message in the vhost
>> caller vhost_virtqueue_stop,
> A separate patch is better.
OK
>
>> because if vhost does not fetch
>> the last avail idx successfully, maybe the device does not
>> suspend, vhost will sync last avail idx to vring used idx as a
>> work around, not a failure.
> This only happens if we return a negative value?
Yes
>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>>   hw/virtio/vhost-vdpa.c | 10 ++++++++++
>>   hw/virtio/vhost.c      |  2 +-
>>   2 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
>> index 3c575a9a6e..10b445f64e 100644
>> --- a/hw/virtio/vhost-vdpa.c
>> +++ b/hw/virtio/vhost-vdpa.c
>> @@ -26,6 +26,7 @@
>>   #include "cpu.h"
>>   #include "trace.h"
>>   #include "qapi/error.h"
>> +#include "sysemu/runstate.h"
>>
>>   /*
>>    * Return one past the end of the end of section. Be careful with uint64_t
>> @@ -1391,6 +1392,15 @@ static int vhost_vdpa_get_vring_base(struct vhost_dev *dev,
>>       struct vhost_vdpa *v = dev->opaque;
>>       int ret;
>>
>> +    if (runstate_check(RUN_STATE_SHUTDOWN)) {
>> +        /*
>> +         * Some devices do not support this call properly,
>> +         * and we don't need to retrieve the indexes
>> +         * if it is shutting down
>> +         */
>> +        return 0;
> Checking runstate in the vhost code seems like a layer violation.
>
> What happens without this patch?
vhost tries to fetch vring base,
vhost_vdpa needs suspend the device before retrieving last_avail_idx.
However not all devices can support .suspend properly so this call
may fail. Then vhost will print an error shows something failed.

The error msg is confused, as stated in the commit log, restoring 
last_avail_idx with guest used idx
is a workaround rather than a failure. And no needs to fetch last_avail_idx
when power off.

Thanks
>
> Thanks
>
>> +    }
>> +
>>       if (v->shadow_vqs_enabled) {
>>           ring->num = virtio_queue_get_last_avail_idx(dev->vdev, ring->index);
>>           return 0;
>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> index 82394331bf..7dd90cff3a 100644
>> --- a/hw/virtio/vhost.c
>> +++ b/hw/virtio/vhost.c
>> @@ -1262,7 +1262,7 @@ void vhost_virtqueue_stop(struct vhost_dev *dev,
>>
>>       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);
>> +        VHOST_OPS_DEBUG(r, "sync last avail idx to the guest used idx for vhost VQ %u", idx);
>>           /* Connection to the backend is broken, so let's sync internal
>>            * last avail idx to the device used idx.
>>            */
>> --
>> 2.39.3
>>