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

[Raphael Norwitz]

> On Nov 24, 2022, at 2:54 AM, Stefano Garzarella <sgarzare@redhat.com> wrote:
> 
> 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@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 :-(
> 

Sure - no need to force a more disruptive change in right before the release. 
If anything can be simplified later.

>> 
>>> >
>>> > 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://urldefense.proofpoint.com/v2/url?u=https-3A__gitlab.com_virtio-2Dfs_virtiofsd&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=In4gmR1pGzKB8G5p6LUrWqkSMec2L5EtXZow_FZNJZk&m=PcC4TEq5C80Knek-ScCNI26rQ13h0n3QEMNNhc-ENd7Txd8wHYqwC1TYfXW_hYor&s=5pdxt8m4-ks8VB2tRSXQV05kdfdP50iy-aAxuGe-Ffc&e=
>>> >  > [2] 
>>> > https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_rust-2Dvmm_vhost_blob_240fc2966_crates_vhost-2Duser-2Dbackend_src_event-5Floop.rs-23L217&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=In4gmR1pGzKB8G5p6LUrWqkSMec2L5EtXZow_FZNJZk&m=PcC4TEq5C80Knek-ScCNI26rQ13h0n3QEMNNhc-ENd7Txd8wHYqwC1TYfXW_hYor&s=-3NUG1pPKN-FwUeDkuu52roXoPoeLR1y4gjrddHUz2U&e=
>>> >  > Fixes: 02b61f38d3 ("hw/virtio: incorporate backend features in 
>>> > features")
>>> > Reported-by: German Maglione <gmaglione@redhat.com>
>>> > Tested-by: German Maglione <gmaglione@redhat.com>
>>> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>>> 
>>> Looks good for vhost-user-blk/vhost-user-scsi.
>>> 
>>> Acked-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> 
> Thanks for the review!
> Stefano