|
| From: | Stefano Garzarella |
| Subject: | Re: [PATCH] vhost-vdpa: check vhost_vdpa_set_vring_ready() return value |
| Date: | Mon, 18 Mar 2024 09:27:11 +0100 |
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 >> >
| [Prev in Thread] | Current Thread | [Next in Thread] |