[v1] vhost-vdpa: check vhost_vdpa_set_vring_ready() return value

[PATCH] vhost-vdpa: check vhost_vdpa_set_vring_ready() return value

Posted by Stefano Garzarella 9 months, 3 weeks ago

vhost_vdpa_set_vring_ready() could already fail, but if Linux's
patch [1] will be merged, it may fail with more chance if
userspace does not activate virtqueues before DRIVER_OK when
VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is not negotiated.

So better check its return value anyway.

[1] https://lore.kernel.org/virtualization/20240206145154.118044-1-sgarzare@redhat.com/T/#u

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
Note: This patch conflicts with [2], but the resolution is simple,
so for now I sent a patch for the current master, but I'll rebase
this patch if we merge the other one first.

[2] https://lore.kernel.org/qemu-devel/20240202132521.32714-1-kwolf@redhat.com/
---
 hw/virtio/vdpa-dev.c |  8 +++++++-
 net/vhost-vdpa.c     | 15 ++++++++++++---
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
index eb9ecea83b..d57cd76c18 100644
--- a/hw/virtio/vdpa-dev.c
+++ b/hw/virtio/vdpa-dev.c
@@ -259,7 +259,11 @@ static int vhost_vdpa_device_start(VirtIODevice *vdev, Error **errp)
         goto err_guest_notifiers;
     }
     for (i = 0; i < s->dev.nvqs; ++i) {
-        vhost_vdpa_set_vring_ready(&s->vdpa, i);
+        ret = vhost_vdpa_set_vring_ready(&s->vdpa, i);
+        if (ret < 0) {
+            error_setg_errno(errp, -ret, "Error starting vring %d", i);
+            goto err_dev_stop;
+        }
     }
     s->started = true;
 
@@ -274,6 +278,8 @@ static int vhost_vdpa_device_start(VirtIODevice *vdev, Error **errp)
 
     return ret;
 
+err_dev_stop:
+    vhost_dev_stop(&s->dev, vdev, false);
 err_guest_notifiers:
     k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false);
 err_host_notifiers:
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 3726ee5d67..e3d8036479 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -381,7 +381,10 @@ static int vhost_vdpa_net_data_load(NetClientState *nc)
     }
 
     for (int i = 0; i < v->dev->nvqs; ++i) {
-        vhost_vdpa_set_vring_ready(v, i + v->dev->vq_index);
+        int ret = vhost_vdpa_set_vring_ready(v, i + v->dev->vq_index);
+        if (ret < 0) {
+            return ret;
+        }
     }
     return 0;
 }
@@ -1213,7 +1216,10 @@ static int vhost_vdpa_net_cvq_load(NetClientState *nc)
 
     assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
 
-    vhost_vdpa_set_vring_ready(v, v->dev->vq_index);
+    r = vhost_vdpa_set_vring_ready(v, v->dev->vq_index);
+    if (unlikely(r < 0)) {
+        return r;
+    }
 
     if (v->shadow_vqs_enabled) {
         n = VIRTIO_NET(v->dev->vdev);
@@ -1252,7 +1258,10 @@ static int vhost_vdpa_net_cvq_load(NetClientState *nc)
     }
 
     for (int i = 0; i < v->dev->vq_index; ++i) {
-        vhost_vdpa_set_vring_ready(v, i);
+        r = vhost_vdpa_set_vring_ready(v, i);
+        if (unlikely(r < 0)) {
+            return r;
+        }
     }
 
     return 0;
-- 
2.43.0

Re: [PATCH] vhost-vdpa: check vhost_vdpa_set_vring_ready() return value

Posted by Eugenio Perez Martin 8 months, 1 week ago

On Wed, Feb 7, 2024 at 10:27 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> vhost_vdpa_set_vring_ready() could already fail, but if Linux's
> patch [1] will be merged, it may fail with more chance if
> userspace does not activate virtqueues before DRIVER_OK when
> VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is not negotiated.
>
> So better check its return value anyway.
>
> [1] https://lore.kernel.org/virtualization/20240206145154.118044-1-sgarzare@redhat.com/T/#u
>

If any, I'd split the patches in CVQ and generic vdpa device. But I
don't think it is a big deal so

Acked-by: Eugenio Pérez <eperezma@redhat.com>

Sorry for the long delay!

> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> Note: This patch conflicts with [2], but the resolution is simple,
> so for now I sent a patch for the current master, but I'll rebase
> this patch if we merge the other one first.
>
> [2] https://lore.kernel.org/qemu-devel/20240202132521.32714-1-kwolf@redhat.com/
> ---
>  hw/virtio/vdpa-dev.c |  8 +++++++-
>  net/vhost-vdpa.c     | 15 ++++++++++++---
>  2 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
> index eb9ecea83b..d57cd76c18 100644
> --- a/hw/virtio/vdpa-dev.c
> +++ b/hw/virtio/vdpa-dev.c
> @@ -259,7 +259,11 @@ static int vhost_vdpa_device_start(VirtIODevice *vdev, Error **errp)
>          goto err_guest_notifiers;
>      }
>      for (i = 0; i < s->dev.nvqs; ++i) {
> -        vhost_vdpa_set_vring_ready(&s->vdpa, i);
> +        ret = vhost_vdpa_set_vring_ready(&s->vdpa, i);
> +        if (ret < 0) {
> +            error_setg_errno(errp, -ret, "Error starting vring %d", i);
> +            goto err_dev_stop;
> +        }
>      }
>      s->started = true;
>
> @@ -274,6 +278,8 @@ static int vhost_vdpa_device_start(VirtIODevice *vdev, Error **errp)
>
>      return ret;
>
> +err_dev_stop:
> +    vhost_dev_stop(&s->dev, vdev, false);
>  err_guest_notifiers:
>      k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false);
>  err_host_notifiers:
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 3726ee5d67..e3d8036479 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -381,7 +381,10 @@ static int vhost_vdpa_net_data_load(NetClientState *nc)
>      }
>
>      for (int i = 0; i < v->dev->nvqs; ++i) {
> -        vhost_vdpa_set_vring_ready(v, i + v->dev->vq_index);
> +        int ret = vhost_vdpa_set_vring_ready(v, i + v->dev->vq_index);
> +        if (ret < 0) {
> +            return ret;
> +        }
>      }
>      return 0;
>  }
> @@ -1213,7 +1216,10 @@ static int vhost_vdpa_net_cvq_load(NetClientState *nc)
>
>      assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
>
> -    vhost_vdpa_set_vring_ready(v, v->dev->vq_index);
> +    r = vhost_vdpa_set_vring_ready(v, v->dev->vq_index);
> +    if (unlikely(r < 0)) {
> +        return r;
> +    }
>
>      if (v->shadow_vqs_enabled) {
>          n = VIRTIO_NET(v->dev->vdev);
> @@ -1252,7 +1258,10 @@ static int vhost_vdpa_net_cvq_load(NetClientState *nc)
>      }
>
>      for (int i = 0; i < v->dev->vq_index; ++i) {
> -        vhost_vdpa_set_vring_ready(v, i);
> +        r = vhost_vdpa_set_vring_ready(v, i);
> +        if (unlikely(r < 0)) {
> +            return r;
> +        }
>      }
>
>      return 0;
> --
> 2.43.0
>

Re: [PATCH] vhost-vdpa: check vhost_vdpa_set_vring_ready() return value

Posted by Jason Wang 8 months, 2 weeks ago

On Wed, Feb 7, 2024 at 5:27 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> vhost_vdpa_set_vring_ready() could already fail, but if Linux's
> patch [1] will be merged, it may fail with more chance if
> userspace does not activate virtqueues before DRIVER_OK when
> VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is not negotiated.

I wonder what happens if we just leave it as is.

VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK: We do know enabling could be
done after driver_ok.
Without VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK: We don't know whether
enabling could be done after driver_ok or not.

Thanks

>
> So better check its return value anyway.
>
> [1] https://lore.kernel.org/virtualization/20240206145154.118044-1-sgarzare@redhat.com/T/#u
>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> Note: This patch conflicts with [2], but the resolution is simple,
> so for now I sent a patch for the current master, but I'll rebase
> this patch if we merge the other one first.
>
> [2] https://lore.kernel.org/qemu-devel/20240202132521.32714-1-kwolf@redhat.com/
> ---
>  hw/virtio/vdpa-dev.c |  8 +++++++-
>  net/vhost-vdpa.c     | 15 ++++++++++++---
>  2 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
> index eb9ecea83b..d57cd76c18 100644
> --- a/hw/virtio/vdpa-dev.c
> +++ b/hw/virtio/vdpa-dev.c
> @@ -259,7 +259,11 @@ static int vhost_vdpa_device_start(VirtIODevice *vdev, Error **errp)
>          goto err_guest_notifiers;
>      }
>      for (i = 0; i < s->dev.nvqs; ++i) {
> -        vhost_vdpa_set_vring_ready(&s->vdpa, i);
> +        ret = vhost_vdpa_set_vring_ready(&s->vdpa, i);
> +        if (ret < 0) {
> +            error_setg_errno(errp, -ret, "Error starting vring %d", i);
> +            goto err_dev_stop;
> +        }
>      }
>      s->started = true;
>
> @@ -274,6 +278,8 @@ static int vhost_vdpa_device_start(VirtIODevice *vdev, Error **errp)
>
>      return ret;
>
> +err_dev_stop:
> +    vhost_dev_stop(&s->dev, vdev, false);
>  err_guest_notifiers:
>      k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false);
>  err_host_notifiers:
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 3726ee5d67..e3d8036479 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -381,7 +381,10 @@ static int vhost_vdpa_net_data_load(NetClientState *nc)
>      }
>
>      for (int i = 0; i < v->dev->nvqs; ++i) {
> -        vhost_vdpa_set_vring_ready(v, i + v->dev->vq_index);
> +        int ret = vhost_vdpa_set_vring_ready(v, i + v->dev->vq_index);
> +        if (ret < 0) {
> +            return ret;
> +        }
>      }
>      return 0;
>  }
> @@ -1213,7 +1216,10 @@ static int vhost_vdpa_net_cvq_load(NetClientState *nc)
>
>      assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
>
> -    vhost_vdpa_set_vring_ready(v, v->dev->vq_index);
> +    r = vhost_vdpa_set_vring_ready(v, v->dev->vq_index);
> +    if (unlikely(r < 0)) {
> +        return r;
> +    }
>
>      if (v->shadow_vqs_enabled) {
>          n = VIRTIO_NET(v->dev->vdev);
> @@ -1252,7 +1258,10 @@ static int vhost_vdpa_net_cvq_load(NetClientState *nc)
>      }
>
>      for (int i = 0; i < v->dev->vq_index; ++i) {
> -        vhost_vdpa_set_vring_ready(v, i);
> +        r = vhost_vdpa_set_vring_ready(v, i);
> +        if (unlikely(r < 0)) {
> +            return r;
> +        }
>      }
>
>      return 0;
> --
> 2.43.0
>

Re: [PATCH] vhost-vdpa: check vhost_vdpa_set_vring_ready() return value

Posted by Stefano Garzarella 8 months, 2 weeks ago

On Thu, Mar 14, 2024 at 11:17:01AM +0800, Jason Wang wrote:
>On Wed, Feb 7, 2024 at 5:27 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> vhost_vdpa_set_vring_ready() could already fail, but if Linux's
>> patch [1] will be merged, it may fail with more chance if
>> userspace does not activate virtqueues before DRIVER_OK when
>> VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is not negotiated.
>
>I wonder what happens if we just leave it as is.

Are you referring to this patch or the kernel patch?

Here I'm just checking the return value of vhost_vdpa_set_vring_ready().
It can return an error also without that kernel patch, so IMHO is
better to check the return value here in QEMU.

What issue do you see with this patch applied?

>
>VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK: We do know enabling could be
>done after driver_ok.
>Without VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK: We don't know whether
>enabling could be done after driver_ok or not.

I see your point, indeed I didn't send a v2 of that patch.
Maybe we should document that, because it could be interpreted that if
VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is not negotiated the enabling
should always be done before driver_ok (which is true for example in
VDUSE).

BTW I think we should discuss it in the kernel patch.

Thanks,
Stefano

>
>Thanks
>
>>
>> So better check its return value anyway.
>>
>> [1] https://lore.kernel.org/virtualization/20240206145154.118044-1-sgarzare@redhat.com/T/#u
>>
>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> ---
>> Note: This patch conflicts with [2], but the resolution is simple,
>> so for now I sent a patch for the current master, but I'll rebase
>> this patch if we merge the other one first.
>>
>> [2] 
>> https://lore.kernel.org/qemu-devel/20240202132521.32714-1-kwolf@redhat.com/
>> ---
>>  hw/virtio/vdpa-dev.c |  8 +++++++-
>>  net/vhost-vdpa.c     | 15 ++++++++++++---
>>  2 files changed, 19 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
>> index eb9ecea83b..d57cd76c18 100644
>> --- a/hw/virtio/vdpa-dev.c
>> +++ b/hw/virtio/vdpa-dev.c
>> @@ -259,7 +259,11 @@ static int vhost_vdpa_device_start(VirtIODevice *vdev, Error **errp)
>>          goto err_guest_notifiers;
>>      }
>>      for (i = 0; i < s->dev.nvqs; ++i) {
>> -        vhost_vdpa_set_vring_ready(&s->vdpa, i);
>> +        ret = vhost_vdpa_set_vring_ready(&s->vdpa, i);
>> +        if (ret < 0) {
>> +            error_setg_errno(errp, -ret, "Error starting vring %d", i);
>> +            goto err_dev_stop;
>> +        }
>>      }
>>      s->started = true;
>>
>> @@ -274,6 +278,8 @@ static int vhost_vdpa_device_start(VirtIODevice *vdev, Error **errp)
>>
>>      return ret;
>>
>> +err_dev_stop:
>> +    vhost_dev_stop(&s->dev, vdev, false);
>>  err_guest_notifiers:
>>      k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false);
>>  err_host_notifiers:
>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>> index 3726ee5d67..e3d8036479 100644
>> --- a/net/vhost-vdpa.c
>> +++ b/net/vhost-vdpa.c
>> @@ -381,7 +381,10 @@ static int vhost_vdpa_net_data_load(NetClientState *nc)
>>      }
>>
>>      for (int i = 0; i < v->dev->nvqs; ++i) {
>> -        vhost_vdpa_set_vring_ready(v, i + v->dev->vq_index);
>> +        int ret = vhost_vdpa_set_vring_ready(v, i + v->dev->vq_index);
>> +        if (ret < 0) {
>> +            return ret;
>> +        }
>>      }
>>      return 0;
>>  }
>> @@ -1213,7 +1216,10 @@ static int vhost_vdpa_net_cvq_load(NetClientState *nc)
>>
>>      assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
>>
>> -    vhost_vdpa_set_vring_ready(v, v->dev->vq_index);
>> +    r = vhost_vdpa_set_vring_ready(v, v->dev->vq_index);
>> +    if (unlikely(r < 0)) {
>> +        return r;
>> +    }
>>
>>      if (v->shadow_vqs_enabled) {
>>          n = VIRTIO_NET(v->dev->vdev);
>> @@ -1252,7 +1258,10 @@ static int vhost_vdpa_net_cvq_load(NetClientState *nc)
>>      }
>>
>>      for (int i = 0; i < v->dev->vq_index; ++i) {
>> -        vhost_vdpa_set_vring_ready(v, i);
>> +        r = vhost_vdpa_set_vring_ready(v, i);
>> +        if (unlikely(r < 0)) {
>> +            return r;
>> +        }
>>      }
>>
>>      return 0;
>> --
>> 2.43.0
>>
>

Re: [PATCH] vhost-vdpa: check vhost_vdpa_set_vring_ready() return value

Posted by Jason Wang 8 months, 1 week ago

On Fri, Mar 15, 2024 at 4:23 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Thu, Mar 14, 2024 at 11:17:01AM +0800, Jason Wang wrote:
> >On Wed, Feb 7, 2024 at 5:27 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> >>
> >> vhost_vdpa_set_vring_ready() could already fail, but if Linux's
> >> patch [1] will be merged, it may fail with more chance if
> >> userspace does not activate virtqueues before DRIVER_OK when
> >> VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is not negotiated.
> >
> >I wonder what happens if we just leave it as is.
>
> Are you referring to this patch or the kernel patch?

This patch.

>
> Here I'm just checking the return value of vhost_vdpa_set_vring_ready().
> It can return an error also without that kernel patch, so IMHO is
> better to check the return value here in QEMU.
>
> What issue do you see with this patch applied?

For the parent which can enable after driver_ok but not advertise it.

(To say the truth, I'm not sure if we need to care about this)

>
> >
> >VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK: We do know enabling could be
> >done after driver_ok.
> >Without VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK: We don't know whether
> >enabling could be done after driver_ok or not.
>
> I see your point, indeed I didn't send a v2 of that patch.
> Maybe we should document that, because it could be interpreted that if
> VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is not negotiated the enabling
> should always be done before driver_ok (which is true for example in
> VDUSE).

I see, so I think we probably need the fix.

>
> BTW I think we should discuss it in the kernel patch.
>
> Thanks,
> Stefano
>
> >
> >Thanks
> >
> >>
> >> So better check its return value anyway.
> >>
> >> [1] https://lore.kernel.org/virtualization/20240206145154.118044-1-sgarzare@redhat.com/T/#u
> >>
> >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> >> ---
> >> Note: This patch conflicts with [2], but the resolution is simple,
> >> so for now I sent a patch for the current master, but I'll rebase
> >> this patch if we merge the other one first.

Will go through [2].

Thanks


> >>
> >> [2]
> >> https://lore.kernel.org/qemu-devel/20240202132521.32714-1-kwolf@redhat.com/
> >> ---
> >>  hw/virtio/vdpa-dev.c |  8 +++++++-
> >>  net/vhost-vdpa.c     | 15 ++++++++++++---
> >>  2 files changed, 19 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
> >> index eb9ecea83b..d57cd76c18 100644
> >> --- a/hw/virtio/vdpa-dev.c
> >> +++ b/hw/virtio/vdpa-dev.c
> >> @@ -259,7 +259,11 @@ static int vhost_vdpa_device_start(VirtIODevice *vdev, Error **errp)
> >>          goto err_guest_notifiers;
> >>      }
> >>      for (i = 0; i < s->dev.nvqs; ++i) {
> >> -        vhost_vdpa_set_vring_ready(&s->vdpa, i);
> >> +        ret = vhost_vdpa_set_vring_ready(&s->vdpa, i);
> >> +        if (ret < 0) {
> >> +            error_setg_errno(errp, -ret, "Error starting vring %d", i);
> >> +            goto err_dev_stop;
> >> +        }
> >>      }
> >>      s->started = true;
> >>
> >> @@ -274,6 +278,8 @@ static int vhost_vdpa_device_start(VirtIODevice *vdev, Error **errp)
> >>
> >>      return ret;
> >>
> >> +err_dev_stop:
> >> +    vhost_dev_stop(&s->dev, vdev, false);
> >>  err_guest_notifiers:
> >>      k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false);
> >>  err_host_notifiers:
> >> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> >> index 3726ee5d67..e3d8036479 100644
> >> --- a/net/vhost-vdpa.c
> >> +++ b/net/vhost-vdpa.c
> >> @@ -381,7 +381,10 @@ static int vhost_vdpa_net_data_load(NetClientState *nc)
> >>      }
> >>
> >>      for (int i = 0; i < v->dev->nvqs; ++i) {
> >> -        vhost_vdpa_set_vring_ready(v, i + v->dev->vq_index);
> >> +        int ret = vhost_vdpa_set_vring_ready(v, i + v->dev->vq_index);
> >> +        if (ret < 0) {
> >> +            return ret;
> >> +        }
> >>      }
> >>      return 0;
> >>  }
> >> @@ -1213,7 +1216,10 @@ static int vhost_vdpa_net_cvq_load(NetClientState *nc)
> >>
> >>      assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
> >>
> >> -    vhost_vdpa_set_vring_ready(v, v->dev->vq_index);
> >> +    r = vhost_vdpa_set_vring_ready(v, v->dev->vq_index);
> >> +    if (unlikely(r < 0)) {
> >> +        return r;
> >> +    }
> >>
> >>      if (v->shadow_vqs_enabled) {
> >>          n = VIRTIO_NET(v->dev->vdev);
> >> @@ -1252,7 +1258,10 @@ static int vhost_vdpa_net_cvq_load(NetClientState *nc)
> >>      }
> >>
> >>      for (int i = 0; i < v->dev->vq_index; ++i) {
> >> -        vhost_vdpa_set_vring_ready(v, i);
> >> +        r = vhost_vdpa_set_vring_ready(v, i);
> >> +        if (unlikely(r < 0)) {
> >> +            return r;
> >> +        }
> >>      }
> >>
> >>      return 0;
> >> --
> >> 2.43.0
> >>
> >
>

Re: [PATCH] vhost-vdpa: check vhost_vdpa_set_vring_ready() return value

Posted by Eugenio Perez Martin 8 months, 1 week ago

On Mon, Mar 18, 2024 at 5:35 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Fri, Mar 15, 2024 at 4:23 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> >
> > On Thu, Mar 14, 2024 at 11:17:01AM +0800, Jason Wang wrote:
> > >On Wed, Feb 7, 2024 at 5:27 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> > >>
> > >> vhost_vdpa_set_vring_ready() could already fail, but if Linux's
> > >> patch [1] will be merged, it may fail with more chance if
> > >> userspace does not activate virtqueues before DRIVER_OK when
> > >> VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is not negotiated.
> > >
> > >I wonder what happens if we just leave it as is.
> >
> > Are you referring to this patch or the kernel patch?
>
> This patch.
>
> >
> > Here I'm just checking the return value of vhost_vdpa_set_vring_ready().
> > It can return an error also without that kernel patch, so IMHO is
> > better to check the return value here in QEMU.
> >
> > What issue do you see with this patch applied?
>
> For the parent which can enable after driver_ok but not advertise it.
>
> (To say the truth, I'm not sure if we need to care about this)
>
> >
> > >
> > >VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK: We do know enabling could be
> > >done after driver_ok.
> > >Without VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK: We don't know whether
> > >enabling could be done after driver_ok or not.
> >
> > I see your point, indeed I didn't send a v2 of that patch.
> > Maybe we should document that, because it could be interpreted that if
> > VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is not negotiated the enabling
> > should always be done before driver_ok (which is true for example in
> > VDUSE).
>
> I see, so I think we probably need the fix.
>

A more complete fix is proper checking for
VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK. But even with that, checking
for failure in the ioctl is also worth it, isn't it?

The only point I see to not checking is to closely mimic virtio
behavior, but vDPA can already fail here.

> >
> > BTW I think we should discuss it in the kernel patch.
> >
> > Thanks,
> > Stefano
> >
> > >
> > >Thanks
> > >
> > >>
> > >> So better check its return value anyway.
> > >>
> > >> [1] https://lore.kernel.org/virtualization/20240206145154.118044-1-sgarzare@redhat.com/T/#u
> > >>
> > >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > >> ---
> > >> Note: This patch conflicts with [2], but the resolution is simple,
> > >> so for now I sent a patch for the current master, but I'll rebase
> > >> this patch if we merge the other one first.
>
> Will go through [2].
>
> Thanks
>
>
> > >>
> > >> [2]
> > >> https://lore.kernel.org/qemu-devel/20240202132521.32714-1-kwolf@redhat.com/
> > >> ---
> > >>  hw/virtio/vdpa-dev.c |  8 +++++++-
> > >>  net/vhost-vdpa.c     | 15 ++++++++++++---
> > >>  2 files changed, 19 insertions(+), 4 deletions(-)
> > >>
> > >> diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
> > >> index eb9ecea83b..d57cd76c18 100644
> > >> --- a/hw/virtio/vdpa-dev.c
> > >> +++ b/hw/virtio/vdpa-dev.c
> > >> @@ -259,7 +259,11 @@ static int vhost_vdpa_device_start(VirtIODevice *vdev, Error **errp)
> > >>          goto err_guest_notifiers;
> > >>      }
> > >>      for (i = 0; i < s->dev.nvqs; ++i) {
> > >> -        vhost_vdpa_set_vring_ready(&s->vdpa, i);
> > >> +        ret = vhost_vdpa_set_vring_ready(&s->vdpa, i);
> > >> +        if (ret < 0) {
> > >> +            error_setg_errno(errp, -ret, "Error starting vring %d", i);
> > >> +            goto err_dev_stop;
> > >> +        }
> > >>      }
> > >>      s->started = true;
> > >>
> > >> @@ -274,6 +278,8 @@ static int vhost_vdpa_device_start(VirtIODevice *vdev, Error **errp)
> > >>
> > >>      return ret;
> > >>
> > >> +err_dev_stop:
> > >> +    vhost_dev_stop(&s->dev, vdev, false);
> > >>  err_guest_notifiers:
> > >>      k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false);
> > >>  err_host_notifiers:
> > >> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > >> index 3726ee5d67..e3d8036479 100644
> > >> --- a/net/vhost-vdpa.c
> > >> +++ b/net/vhost-vdpa.c
> > >> @@ -381,7 +381,10 @@ static int vhost_vdpa_net_data_load(NetClientState *nc)
> > >>      }
> > >>
> > >>      for (int i = 0; i < v->dev->nvqs; ++i) {
> > >> -        vhost_vdpa_set_vring_ready(v, i + v->dev->vq_index);
> > >> +        int ret = vhost_vdpa_set_vring_ready(v, i + v->dev->vq_index);
> > >> +        if (ret < 0) {
> > >> +            return ret;
> > >> +        }
> > >>      }
> > >>      return 0;
> > >>  }
> > >> @@ -1213,7 +1216,10 @@ static int vhost_vdpa_net_cvq_load(NetClientState *nc)
> > >>
> > >>      assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
> > >>
> > >> -    vhost_vdpa_set_vring_ready(v, v->dev->vq_index);
> > >> +    r = vhost_vdpa_set_vring_ready(v, v->dev->vq_index);
> > >> +    if (unlikely(r < 0)) {
> > >> +        return r;
> > >> +    }
> > >>
> > >>      if (v->shadow_vqs_enabled) {
> > >>          n = VIRTIO_NET(v->dev->vdev);
> > >> @@ -1252,7 +1258,10 @@ static int vhost_vdpa_net_cvq_load(NetClientState *nc)
> > >>      }
> > >>
> > >>      for (int i = 0; i < v->dev->vq_index; ++i) {
> > >> -        vhost_vdpa_set_vring_ready(v, i);
> > >> +        r = vhost_vdpa_set_vring_ready(v, i);
> > >> +        if (unlikely(r < 0)) {
> > >> +            return r;
> > >> +        }
> > >>      }
> > >>
> > >>      return 0;
> > >> --
> > >> 2.43.0
> > >>
> > >
> >
>

Re: [PATCH] vhost-vdpa: check vhost_vdpa_set_vring_ready() return value

Posted by Stefano Garzarella 8 months, 1 week ago

On Mon, Mar 18, 2024 at 12:31:59PM +0800, Jason Wang wrote:
>On Fri, Mar 15, 2024 at 4:23 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> On Thu, Mar 14, 2024 at 11:17:01AM +0800, Jason Wang wrote:
>> >On Wed, Feb 7, 2024 at 5:27 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>> >>
>> >> vhost_vdpa_set_vring_ready() could already fail, but if Linux's
>> >> patch [1] will be merged, it may fail with more chance if
>> >> userspace does not activate virtqueues before DRIVER_OK when
>> >> VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is not negotiated.
>> >
>> >I wonder what happens if we just leave it as is.
>>
>> Are you referring to this patch or the kernel patch?
>
>This patch.
>
>>
>> Here I'm just checking the return value of vhost_vdpa_set_vring_ready().
>> It can return an error also without that kernel patch, so IMHO is
>> better to check the return value here in QEMU.
>>
>> What issue do you see with this patch applied?
>
>For the parent which can enable after driver_ok but not advertise it.

But this patch is not changing anything in that sense, it just controls
the return value of the VHOST_VDPA_SET_VRING_ENABLE ioctl.

Why would QEMU ignore an error if it can't activate vrings?
If we really want to ignore it we should document it both in QEMU, but
also in the kernel, because honestly the way the code is now it
shouldn't fail from what I understand.

That said, even if we ignore it, IMHO we should at least print a warning
in QEMU.

>
>(To say the truth, I'm not sure if we need to care about this)

I agree on that, but this is related to the patch in the kernel, not
this simple patch to fix QEMU error path, right?

>
>>
>> >
>> >VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK: We do know enabling could be
>> >done after driver_ok.
>> >Without VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK: We don't know whether
>> >enabling could be done after driver_ok or not.
>>
>> I see your point, indeed I didn't send a v2 of that patch.
>> Maybe we should document that, because it could be interpreted that if
>> VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is not negotiated the enabling
>> should always be done before driver_ok (which is true for example in
>> VDUSE).
>
>I see, so I think we probably need the fix.
>
>>
>> BTW I think we should discuss it in the kernel patch.
>>
>> Thanks,
>> Stefano
>>
>> >
>> >Thanks
>> >
>> >>
>> >> So better check its return value anyway.
>> >>
>> >> [1] https://lore.kernel.org/virtualization/20240206145154.118044-1-sgarzare@redhat.com/T/#u
>> >>
>> >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> >> ---
>> >> Note: This patch conflicts with [2], but the resolution is simple,
>> >> so for now I sent a patch for the current master, but I'll rebase
>> >> this patch if we merge the other one first.
>
>Will go through [2].

Here I meant that the conflict is only in the code touched, because
Kevin's patch remove/move some of the code touched by this patch.
And rightly he checked the return value of the ioctl as I would like to
do in the other places where we call the same ioctl.

So honestly I still don't understand what's wrong with this patch...

Thanks,
Stefano

>
>Thanks
>
>
>> >>
>> >> [2]
>> >> https://lore.kernel.org/qemu-devel/20240202132521.32714-1-kwolf@redhat.com/
>> >> ---
>> >>  hw/virtio/vdpa-dev.c |  8 +++++++-
>> >>  net/vhost-vdpa.c     | 15 ++++++++++++---
>> >>  2 files changed, 19 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
>> >> index eb9ecea83b..d57cd76c18 100644
>> >> --- a/hw/virtio/vdpa-dev.c
>> >> +++ b/hw/virtio/vdpa-dev.c
>> >> @@ -259,7 +259,11 @@ static int vhost_vdpa_device_start(VirtIODevice *vdev, Error **errp)
>> >>          goto err_guest_notifiers;
>> >>      }
>> >>      for (i = 0; i < s->dev.nvqs; ++i) {
>> >> -        vhost_vdpa_set_vring_ready(&s->vdpa, i);
>> >> +        ret = vhost_vdpa_set_vring_ready(&s->vdpa, i);
>> >> +        if (ret < 0) {
>> >> +            error_setg_errno(errp, -ret, "Error starting vring %d", i);
>> >> +            goto err_dev_stop;
>> >> +        }
>> >>      }
>> >>      s->started = true;
>> >>
>> >> @@ -274,6 +278,8 @@ static int vhost_vdpa_device_start(VirtIODevice *vdev, Error **errp)
>> >>
>> >>      return ret;
>> >>
>> >> +err_dev_stop:
>> >> +    vhost_dev_stop(&s->dev, vdev, false);
>> >>  err_guest_notifiers:
>> >>      k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false);
>> >>  err_host_notifiers:
>> >> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>> >> index 3726ee5d67..e3d8036479 100644
>> >> --- a/net/vhost-vdpa.c
>> >> +++ b/net/vhost-vdpa.c
>> >> @@ -381,7 +381,10 @@ static int vhost_vdpa_net_data_load(NetClientState *nc)
>> >>      }
>> >>
>> >>      for (int i = 0; i < v->dev->nvqs; ++i) {
>> >> -        vhost_vdpa_set_vring_ready(v, i + v->dev->vq_index);
>> >> +        int ret = vhost_vdpa_set_vring_ready(v, i + v->dev->vq_index);
>> >> +        if (ret < 0) {
>> >> +            return ret;
>> >> +        }
>> >>      }
>> >>      return 0;
>> >>  }
>> >> @@ -1213,7 +1216,10 @@ static int vhost_vdpa_net_cvq_load(NetClientState *nc)
>> >>
>> >>      assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
>> >>
>> >> -    vhost_vdpa_set_vring_ready(v, v->dev->vq_index);
>> >> +    r = vhost_vdpa_set_vring_ready(v, v->dev->vq_index);
>> >> +    if (unlikely(r < 0)) {
>> >> +        return r;
>> >> +    }
>> >>
>> >>      if (v->shadow_vqs_enabled) {
>> >>          n = VIRTIO_NET(v->dev->vdev);
>> >> @@ -1252,7 +1258,10 @@ static int vhost_vdpa_net_cvq_load(NetClientState *nc)
>> >>      }
>> >>
>> >>      for (int i = 0; i < v->dev->vq_index; ++i) {
>> >> -        vhost_vdpa_set_vring_ready(v, i);
>> >> +        r = vhost_vdpa_set_vring_ready(v, i);
>> >> +        if (unlikely(r < 0)) {
>> >> +            return r;
>> >> +        }
>> >>      }
>> >>
>> >>      return 0;
>> >> --
>> >> 2.43.0
>> >>
>> >
>>
>

Re: [PATCH] vhost-vdpa: check vhost_vdpa_set_vring_ready() return value

Posted by Jason Wang 8 months, 1 week ago

On Mon, Mar 18, 2024 at 4:27 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Mon, Mar 18, 2024 at 12:31:59PM +0800, Jason Wang wrote:
> >On Fri, Mar 15, 2024 at 4:23 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> >>
> >> On Thu, Mar 14, 2024 at 11:17:01AM +0800, Jason Wang wrote:
> >> >On Wed, Feb 7, 2024 at 5:27 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> >> >>
> >> >> vhost_vdpa_set_vring_ready() could already fail, but if Linux's
> >> >> patch [1] will be merged, it may fail with more chance if
> >> >> userspace does not activate virtqueues before DRIVER_OK when
> >> >> VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is not negotiated.
> >> >
> >> >I wonder what happens if we just leave it as is.
> >>
> >> Are you referring to this patch or the kernel patch?
> >
> >This patch.
> >
> >>
> >> Here I'm just checking the return value of vhost_vdpa_set_vring_ready().
> >> It can return an error also without that kernel patch, so IMHO is
> >> better to check the return value here in QEMU.
> >>
> >> What issue do you see with this patch applied?
> >
> >For the parent which can enable after driver_ok but not advertise it.
>
> But this patch is not changing anything in that sense, it just controls
> the return value of the VHOST_VDPA_SET_VRING_ENABLE ioctl.
>
> Why would QEMU ignore an error if it can't activate vrings?
> If we really want to ignore it we should document it both in QEMU, but
> also in the kernel, because honestly the way the code is now it
> shouldn't fail from what I understand.
>
> That said, even if we ignore it, IMHO we should at least print a warning
> in QEMU.

Right.

>
> >
> >(To say the truth, I'm not sure if we need to care about this)
>
> I agree on that, but this is related to the patch in the kernel, not
.> this simple patch to fix QEMU error path, right?

Or it's the charge of the Qemu vDPA layer to avoid calling
set_vq_ready() after driver_ok if no
VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK. Or it might be too late.

>
> >
> >>
> >> >
> >> >VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK: We do know enabling could be
> >> >done after driver_ok.
> >> >Without VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK: We don't know whether
> >> >enabling could be done after driver_ok or not.
> >>
> >> I see your point, indeed I didn't send a v2 of that patch.
> >> Maybe we should document that, because it could be interpreted that if
> >> VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is not negotiated the enabling
> >> should always be done before driver_ok (which is true for example in
> >> VDUSE).
> >
> >I see, so I think we probably need the fix.
> >
> >>
> >> BTW I think we should discuss it in the kernel patch.
> >>
> >> Thanks,
> >> Stefano
> >>
> >> >
> >> >Thanks
> >> >
> >> >>
> >> >> So better check its return value anyway.
> >> >>
> >> >> [1] https://lore.kernel.org/virtualization/20240206145154.118044-1-sgarzare@redhat.com/T/#u
> >> >>
> >> >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> >> >> ---
> >> >> Note: This patch conflicts with [2], but the resolution is simple,
> >> >> so for now I sent a patch for the current master, but I'll rebase
> >> >> this patch if we merge the other one first.
> >
> >Will go through [2].
>
> Here I meant that the conflict is only in the code touched, because
> Kevin's patch remove/move some of the code touched by this patch.
> And rightly he checked the return value of the ioctl as I would like to
> do in the other places where we call the same ioctl.
>
> So honestly I still don't understand what's wrong with this patch...

Nothing wrong now.

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks

>
> Thanks,
> Stefano
>
> >
> >Thanks
> >
> >
> >> >>
> >> >> [2]
> >> >> https://lore.kernel.org/qemu-devel/20240202132521.32714-1-kwolf@redhat.com/
> >> >> ---
> >> >>  hw/virtio/vdpa-dev.c |  8 +++++++-
> >> >>  net/vhost-vdpa.c     | 15 ++++++++++++---
> >> >>  2 files changed, 19 insertions(+), 4 deletions(-)
> >> >>
> >> >> diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
> >> >> index eb9ecea83b..d57cd76c18 100644
> >> >> --- a/hw/virtio/vdpa-dev.c
> >> >> +++ b/hw/virtio/vdpa-dev.c
> >> >> @@ -259,7 +259,11 @@ static int vhost_vdpa_device_start(VirtIODevice *vdev, Error **errp)
> >> >>          goto err_guest_notifiers;
> >> >>      }
> >> >>      for (i = 0; i < s->dev.nvqs; ++i) {
> >> >> -        vhost_vdpa_set_vring_ready(&s->vdpa, i);
> >> >> +        ret = vhost_vdpa_set_vring_ready(&s->vdpa, i);
> >> >> +        if (ret < 0) {
> >> >> +            error_setg_errno(errp, -ret, "Error starting vring %d", i);
> >> >> +            goto err_dev_stop;
> >> >> +        }
> >> >>      }
> >> >>      s->started = true;
> >> >>
> >> >> @@ -274,6 +278,8 @@ static int vhost_vdpa_device_start(VirtIODevice *vdev, Error **errp)
> >> >>
> >> >>      return ret;
> >> >>
> >> >> +err_dev_stop:
> >> >> +    vhost_dev_stop(&s->dev, vdev, false);
> >> >>  err_guest_notifiers:
> >> >>      k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false);
> >> >>  err_host_notifiers:
> >> >> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> >> >> index 3726ee5d67..e3d8036479 100644
> >> >> --- a/net/vhost-vdpa.c
> >> >> +++ b/net/vhost-vdpa.c
> >> >> @@ -381,7 +381,10 @@ static int vhost_vdpa_net_data_load(NetClientState *nc)
> >> >>      }
> >> >>
> >> >>      for (int i = 0; i < v->dev->nvqs; ++i) {
> >> >> -        vhost_vdpa_set_vring_ready(v, i + v->dev->vq_index);
> >> >> +        int ret = vhost_vdpa_set_vring_ready(v, i + v->dev->vq_index);
> >> >> +        if (ret < 0) {
> >> >> +            return ret;
> >> >> +        }
> >> >>      }
> >> >>      return 0;
> >> >>  }
> >> >> @@ -1213,7 +1216,10 @@ static int vhost_vdpa_net_cvq_load(NetClientState *nc)
> >> >>
> >> >>      assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
> >> >>
> >> >> -    vhost_vdpa_set_vring_ready(v, v->dev->vq_index);
> >> >> +    r = vhost_vdpa_set_vring_ready(v, v->dev->vq_index);
> >> >> +    if (unlikely(r < 0)) {
> >> >> +        return r;
> >> >> +    }
> >> >>
> >> >>      if (v->shadow_vqs_enabled) {
> >> >>          n = VIRTIO_NET(v->dev->vdev);
> >> >> @@ -1252,7 +1258,10 @@ static int vhost_vdpa_net_cvq_load(NetClientState *nc)
> >> >>      }
> >> >>
> >> >>      for (int i = 0; i < v->dev->vq_index; ++i) {
> >> >> -        vhost_vdpa_set_vring_ready(v, i);
> >> >> +        r = vhost_vdpa_set_vring_ready(v, i);
> >> >> +        if (unlikely(r < 0)) {
> >> >> +            return r;
> >> >> +        }
> >> >>      }
> >> >>
> >> >>      return 0;
> >> >> --
> >> >> 2.43.0
> >> >>
> >> >
> >>
> >
>

Re: [PATCH] vhost-vdpa: check vhost_vdpa_set_vring_ready() return value

Posted by Stefano Garzarella 8 months, 1 week ago

On Wed, Mar 20, 2024 at 12:18:14PM +0800, Jason Wang wrote:
>On Mon, Mar 18, 2024 at 4:27 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> On Mon, Mar 18, 2024 at 12:31:59PM +0800, Jason Wang wrote:
>> >On Fri, Mar 15, 2024 at 4:23 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>> >>
>> >> On Thu, Mar 14, 2024 at 11:17:01AM +0800, Jason Wang wrote:
>> >> >On Wed, Feb 7, 2024 at 5:27 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>> >> >>
>> >> >> vhost_vdpa_set_vring_ready() could already fail, but if Linux's
>> >> >> patch [1] will be merged, it may fail with more chance if
>> >> >> userspace does not activate virtqueues before DRIVER_OK when
>> >> >> VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is not negotiated.
>> >> >
>> >> >I wonder what happens if we just leave it as is.
>> >>
>> >> Are you referring to this patch or the kernel patch?
>> >
>> >This patch.
>> >
>> >>
>> >> Here I'm just checking the return value of vhost_vdpa_set_vring_ready().
>> >> It can return an error also without that kernel patch, so IMHO is
>> >> better to check the return value here in QEMU.
>> >>
>> >> What issue do you see with this patch applied?
>> >
>> >For the parent which can enable after driver_ok but not advertise it.
>>
>> But this patch is not changing anything in that sense, it just controls
>> the return value of the VHOST_VDPA_SET_VRING_ENABLE ioctl.
>>
>> Why would QEMU ignore an error if it can't activate vrings?
>> If we really want to ignore it we should document it both in QEMU, but
>> also in the kernel, because honestly the way the code is now it
>> shouldn't fail from what I understand.
>>
>> That said, even if we ignore it, IMHO we should at least print a warning
>> in QEMU.
>
>Right.
>
>>
>> >
>> >(To say the truth, I'm not sure if we need to care about this)
>>
>> I agree on that, but this is related to the patch in the kernel, not
>.> this simple patch to fix QEMU error path, right?
>
>Or it's the charge of the Qemu vDPA layer to avoid calling
>set_vq_ready() after driver_ok if no
>VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK. Or it might be too late.

Yeah, maybe is too late. We already released several versions without
that.

>
>>
>> >
>> >>
>> >> >
>> >> >VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK: We do know enabling could 
>> >> >be
>> >> >done after driver_ok.
>> >> >Without VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK: We don't know whether
>> >> >enabling could be done after driver_ok or not.
>> >>
>> >> I see your point, indeed I didn't send a v2 of that patch.
>> >> Maybe we should document that, because it could be interpreted that if
>> >> VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is not negotiated the enabling
>> >> should always be done before driver_ok (which is true for example 
>> >> in
>> >> VDUSE).
>> >
>> >I see, so I think we probably need the fix.
>> >
>> >>
>> >> BTW I think we should discuss it in the kernel patch.
>> >>
>> >> Thanks,
>> >> Stefano
>> >>
>> >> >
>> >> >Thanks
>> >> >
>> >> >>
>> >> >> So better check its return value anyway.
>> >> >>
>> >> >> [1] https://lore.kernel.org/virtualization/20240206145154.118044-1-sgarzare@redhat.com/T/#u
>> >> >>
>> >> >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> >> >> ---
>> >> >> Note: This patch conflicts with [2], but the resolution is simple,
>> >> >> so for now I sent a patch for the current master, but I'll rebase
>> >> >> this patch if we merge the other one first.
>> >
>> >Will go through [2].
>>
>> Here I meant that the conflict is only in the code touched, because
>> Kevin's patch remove/move some of the code touched by this patch.
>> And rightly he checked the return value of the ioctl as I would like to
>> do in the other places where we call the same ioctl.
>>
>> So honestly I still don't understand what's wrong with this patch...
>
>Nothing wrong now.
>
>Acked-by: Jason Wang <jasowang@redhat.com>

Thanks for the review,
I'll send a v2 carrying your and Eugenio acks, rebasing on top of 
Kevin's patch, so it should be easy to merge.

Stefano

Re: [PATCH] vhost-vdpa: check vhost_vdpa_set_vring_ready() return value

Posted by Stefano Garzarella 8 months, 2 weeks ago

Gentle ping :-)

On Wed, Feb 7, 2024 at 10:27 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> vhost_vdpa_set_vring_ready() could already fail, but if Linux's
> patch [1] will be merged, it may fail with more chance if
> userspace does not activate virtqueues before DRIVER_OK when
> VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is not negotiated.
>
> So better check its return value anyway.
>
> [1] https://lore.kernel.org/virtualization/20240206145154.118044-1-sgarzare@redhat.com/T/#u
>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> Note: This patch conflicts with [2], but the resolution is simple,
> so for now I sent a patch for the current master, but I'll rebase
> this patch if we merge the other one first.
>
> [2] https://lore.kernel.org/qemu-devel/20240202132521.32714-1-kwolf@redhat.com/
> ---
>  hw/virtio/vdpa-dev.c |  8 +++++++-
>  net/vhost-vdpa.c     | 15 ++++++++++++---
>  2 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
> index eb9ecea83b..d57cd76c18 100644
> --- a/hw/virtio/vdpa-dev.c
> +++ b/hw/virtio/vdpa-dev.c
> @@ -259,7 +259,11 @@ static int vhost_vdpa_device_start(VirtIODevice *vdev, Error **errp)
>          goto err_guest_notifiers;
>      }
>      for (i = 0; i < s->dev.nvqs; ++i) {
> -        vhost_vdpa_set_vring_ready(&s->vdpa, i);
> +        ret = vhost_vdpa_set_vring_ready(&s->vdpa, i);
> +        if (ret < 0) {
> +            error_setg_errno(errp, -ret, "Error starting vring %d", i);
> +            goto err_dev_stop;
> +        }
>      }
>      s->started = true;
>
> @@ -274,6 +278,8 @@ static int vhost_vdpa_device_start(VirtIODevice *vdev, Error **errp)
>
>      return ret;
>
> +err_dev_stop:
> +    vhost_dev_stop(&s->dev, vdev, false);
>  err_guest_notifiers:
>      k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false);
>  err_host_notifiers:
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 3726ee5d67..e3d8036479 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -381,7 +381,10 @@ static int vhost_vdpa_net_data_load(NetClientState *nc)
>      }
>
>      for (int i = 0; i < v->dev->nvqs; ++i) {
> -        vhost_vdpa_set_vring_ready(v, i + v->dev->vq_index);
> +        int ret = vhost_vdpa_set_vring_ready(v, i + v->dev->vq_index);
> +        if (ret < 0) {
> +            return ret;
> +        }
>      }
>      return 0;
>  }
> @@ -1213,7 +1216,10 @@ static int vhost_vdpa_net_cvq_load(NetClientState *nc)
>
>      assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
>
> -    vhost_vdpa_set_vring_ready(v, v->dev->vq_index);
> +    r = vhost_vdpa_set_vring_ready(v, v->dev->vq_index);
> +    if (unlikely(r < 0)) {
> +        return r;
> +    }
>
>      if (v->shadow_vqs_enabled) {
>          n = VIRTIO_NET(v->dev->vdev);
> @@ -1252,7 +1258,10 @@ static int vhost_vdpa_net_cvq_load(NetClientState *nc)
>      }
>
>      for (int i = 0; i < v->dev->vq_index; ++i) {
> -        vhost_vdpa_set_vring_ready(v, i);
> +        r = vhost_vdpa_set_vring_ready(v, i);
> +        if (unlikely(r < 0)) {
> +            return r;
> +        }
>      }
>
>      return 0;
> --
> 2.43.0
>