[Virtio-fs] [PATCH for-7.2] vhost: enable vrings in vhost_dev_start() for vhost-user devices

[Stefano Garzarella]

On Thu, Nov 24, 2022 at 01:50:19AM -0500, Michael S. Tsirkin wrote:
>On Thu, Nov 24, 2022 at 12:19:25AM +0000, Raphael Norwitz wrote:
>>
>> > On Nov 23, 2022, at 8:16 AM, Stefano Garzarella <sgarzare at redhat.com> wrote:
>> >
>> > Commit 02b61f38d3 ("hw/virtio: incorporate backend features in features")
>> > properly negotiates VHOST_USER_F_PROTOCOL_FEATURES with the vhost-user
>> > backend, but we forgot to enable vrings as specified in
>> > docs/interop/vhost-user.rst:
>> >
>> >    If ``VHOST_USER_F_PROTOCOL_FEATURES`` has not been negotiated, the
>> >    ring starts directly in the enabled state.
>> >
>> >    If ``VHOST_USER_F_PROTOCOL_FEATURES`` has been negotiated, the ring is
>> >    initialized in a disabled state and is enabled by
>> >    ``VHOST_USER_SET_VRING_ENABLE`` with parameter 1.
>> >
>> > Some vhost-user front-ends already did this by calling
>> > vhost_ops.vhost_set_vring_enable() directly:
>> > - backends/cryptodev-vhost.c
>> > - hw/net/virtio-net.c
>> > - hw/virtio/vhost-user-gpio.c
>>
>> To simplify why not rather change these devices to use the new 
>> semantics?

Maybe the only one I wouldn't be scared of is vhost-user-gpio, but for 
example vhost-net would require a lot of changes, maybe better after the 
release.

For example, we could do like vhost-vdpa and call SET_VRING_ENABLE in 
the VhostOps.vhost_dev_start callback of vhost-user.c, but I think it's 
too risky to do that now.

>
>Granted this is already scary enough for this release.

Yeah, I tried to touch as little as possible but I'm scared too, I just 
haven't found a better solution for now :-(

>
>> >
>> > But most didn't do that, so we would leave the vrings disabled and some
>> > backends would not work. We observed this issue with the rust version of
>> > virtiofsd [1], which uses the event loop [2] provided by the
>> > vhost-user-backend crate where requests are not processed if vring is
>> > not enabled.
>> >
>> > Let's fix this issue by enabling the vrings in vhost_dev_start() for
>> > vhost-user front-ends that don't already do this directly. Same thing
>> > also in vhost_dev_stop() where we disable vrings.
>> >
>> > [1] https://gitlab.com/virtio-fs/virtiofsd
>> > [2] https://github.com/rust-vmm/vhost/blob/240fc2966/crates/vhost-user-backend/src/event_loop.rs#L217
>> > Fixes: 02b61f38d3 ("hw/virtio: incorporate backend features in features")
>> > Reported-by: German Maglione <gmaglione at redhat.com>
>> > Tested-by: German Maglione <gmaglione at redhat.com>
>> > Signed-off-by: Stefano Garzarella <sgarzare at redhat.com>
>>
>> Looks good for vhost-user-blk/vhost-user-scsi.
>>
>> Acked-by: Raphael Norwitz <raphael.norwitz at nutanix.com>

Thanks for the review!
Stefano