On Mon, Mar 18, 2024 at 5:35 AM Jason Wang <jasow...@redhat.com> 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. > > (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-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]. > > 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 > > >> > > > > > >
