Re: [PATCH] vdpa: Avoid reset when stop device

[Si-Wei Liu <si-wei.liu@oracle.com>]

On 3/23/2022 2:20 AM, Jason Wang wrote:
> Adding Eugenio,  and Ling Shan.
>
> On Wed, Mar 23, 2022 at 4:58 PM <08005325@163.com> wrote:
>> From: Michael Qiu <qiudayu@archeros.com>
>>
>> Currently, when VM poweroff, it will trigger vdpa
>> device(such as mlx bluefield2 VF) reset twice, this leads
>> to below issue:
>>
>> vhost VQ 2 ring restore failed: -22: Invalid argument (22)
>>
>> This because in vhost_dev_stop(), qemu tries to stop the device,
>> then stop the queue: vhost_virtqueue_stop().
>> In vhost_dev_stop(), it resets the device, which clear some flags
>> in low level driver, and the driver finds
>> that the VQ is invalied, this is the root cause.
>>
>> Actually, device reset will be called within func release()
>>
>> To solve the issue, vdpa should set vring unready, and
>> remove reset ops in device stop: vhost_dev_start(hdev, false).
> This is an interesting issue. Do you see a real issue except for the
> above warnings.
>
> The reason we "abuse" reset is that we don't have a stop uAPI for
> vhost. We plan to add a status bit to stop the whole device in the
> virtio spec, but considering it may take a while maybe we can first
> introduce a new uAPI/ioctl for that.
Yep. What was missing here is a vdpa specific uAPI for per-virtqueue 
stop/suspend rather than spec level amendment to stop the whole device 
(including both vq and config space). For now we can have vDPA specific 
means to control the vq, something vDPA hardware vendor must support for 
live migration, e.g. datapath switching to shadow vq. I believe the spec 
amendment may follow to define a bit for virtio feature negotiation 
later on if needed (FWIW virtio-vdpa already does set_vq_ready(..., 0) 
to stop the vq).

However, there's a flaw in this patch, see below.
>
> Note that the stop doesn't just work for virtqueue but others like,
> e.g config space. But considering we don't have config interrupt
> support right now, we're probably fine.
>
> Checking the driver, it looks to me only the IFCVF's set_vq_ready() is
> problematic, Ling Shan, please have a check. And we probably need a
> workaround for vp_vdpa as well.
>
> Anyhow, this seems to be better than reset. So for 7.1:
>
> Acked-by: Jason Wang <jasowang@redhat.com>
>
>> Signed-off-by: Michael Qiu<qiudayu@archeros.com>
>> ---
>>   hw/virtio/vhost-vdpa.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
>> index c5ed7a3..d858b4f 100644
>> --- a/hw/virtio/vhost-vdpa.c
>> +++ b/hw/virtio/vhost-vdpa.c
>> @@ -719,14 +719,14 @@ static int vhost_vdpa_get_vq_index(struct vhost_dev *dev, int idx)
>>       return idx;
>>   }
>>
>> -static int vhost_vdpa_set_vring_ready(struct vhost_dev *dev)
>> +static int vhost_vdpa_set_vring_ready(struct vhost_dev *dev, unsigned int ready)
>>   {
>>       int i;
>>       trace_vhost_vdpa_set_vring_ready(dev);
>>       for (i = 0; i < dev->nvqs; ++i) {
>>           struct vhost_vring_state state = {
>>               .index = dev->vq_index + i,
>> -            .num = 1,
>> +            .num = ready,
>>           };
>>           vhost_vdpa_call(dev, VHOST_VDPA_SET_VRING_ENABLE, &state);
>>       }
>> @@ -1088,8 +1088,9 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
>>           if (unlikely(!ok)) {
>>               return -1;
>>           }
>> -        vhost_vdpa_set_vring_ready(dev);
>> +        vhost_vdpa_set_vring_ready(dev, 1);
>>       } else {
>> +        vhost_vdpa_set_vring_ready(dev, 0);
>>           ok = vhost_vdpa_svqs_stop(dev);
>>           if (unlikely(!ok)) {
>>               return -1;
>> @@ -1105,7 +1106,6 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
>>           memory_listener_register(&v->listener, &address_space_memory);
>>           return vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
>>       } else {
>> -        vhost_vdpa_reset_device(dev);
Unfortunately, the reset can't be be removed from here as this code path 
usually involves virtio reset or status change for e.g. invoked via 
virtio_net_set_status(... , 0). Ideally we should use the 
VhostOps.vhost_reset_device() to reset the vhost-vdpa device where 
status change is involved after vhost_dev_stop() is done, but this 
distinction is not there yet as of today in all of the virtio devices 
except vhost_user_scsi.

Alternatively we may be able to do something like below, stop the 
virtqueue in vhost_vdpa_get_vring_base() in the vhost_virtqueue_stop() 
context. Only until the hardware vq is stopped, svq can stop and unmap 
then vhost-vdpa would reset the device status. It kinda works, but not 
in a perfect way...

--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -564,14 +564,14 @@ static int vhost_vdpa_get_vq_index(struct 
vhost_dev *dev, int idx)
      return idx;
  }

-static int vhost_vdpa_set_vring_ready(struct vhost_dev *dev)
+static int vhost_vdpa_set_vring_ready(struct vhost_dev *dev, int enable)
  {
      int i;
      trace_vhost_vdpa_set_vring_ready(dev);
      for (i = 0; i < dev->nvqs; ++i) {
          struct vhost_vring_state state = {
              .index = dev->vq_index + i,
-            .num = 1,
+            .num = enable,
          };
          vhost_vdpa_call(dev, VHOST_VDPA_SET_VRING_ENABLE, &state);
      }
@@ -641,7 +641,7 @@ static int vhost_vdpa_dev_start(struct vhost_dev 
*dev, bool started)

      if (started) {
          vhost_vdpa_host_notifiers_init(dev);
-        vhost_vdpa_set_vring_ready(dev);
+        vhost_vdpa_set_vring_ready(dev, 1);
      } else {
          vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs);
      }
@@ -708,6 +708,9 @@ static int vhost_vdpa_get_vring_base(struct 
vhost_dev *dev,
  {
      int ret;

+    /* Deactivate the queue (best effort) */
+    vhost_vdpa_set_vring_ready(dev, 0);
+
      ret = vhost_vdpa_call(dev, VHOST_GET_VRING_BASE, ring);
      trace_vhost_vdpa_get_vring_base(dev, ring->index, ring->num);
      return ret;
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 437347a..2e917d8 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1832,15 +1832,15 @@ void vhost_dev_stop(struct vhost_dev *hdev, 
VirtIODevice *vdev)
      /* should only be called after backend is connected */
      assert(hdev->vhost_ops);

-    if (hdev->vhost_ops->vhost_dev_start) {
-        hdev->vhost_ops->vhost_dev_start(hdev, false);
-    }
      for (i = 0; i < hdev->nvqs; ++i) {
          vhost_virtqueue_stop(hdev,
                               vdev,
                               hdev->vqs + i,
                               hdev->vq_index + i);
      }
+    if (hdev->vhost_ops->vhost_dev_start) {
+        hdev->vhost_ops->vhost_dev_start(hdev, false);
+    }

      if (vhost_dev_has_iommu(hdev)) {
          if (hdev->vhost_ops->vhost_set_iotlb_callback) {

Regards,
-Siwei

>>           vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
>>                                      VIRTIO_CONFIG_S_DRIVER);
>>           memory_listener_unregister(&v->listener);
>> --
>> 1.8.3.1
>>
>