On Mon, Mar 18, 2024 at 4:27 PM Stefano Garzarella <sgarz...@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 <sgarz...@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 <sgarz...@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-sgarz...@redhat.com/T/#u > >> >> > >> >> Signed-off-by: Stefano Garzarella <sgarz...@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 <jasow...@redhat.com> Thanks > > Thanks, > Stefano > > > > >Thanks > > > > > >> >> > >> >> [2] > >> >> https://lore.kernel.org/qemu-devel/20240202132521.32714-1-kw...@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 > >> >> > >> > > >> > > >
