Re: [Virtio-fs] [PATCH] vhost: mask VIRTIO_F_RING_RESET for vhost and vhost-user devices

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2408 bytes --]

On Mon, Nov 21, 2022 at 11:11:01AM +0100, Stefano Garzarella wrote:
> Commit 69e1c14aa2 ("virtio: core: vq reset feature negotation support")
> enabled VIRTIO_F_RING_RESET by default for all virtio devices.
> 
> This feature is not currently emulated by QEMU, so for vhost and
> vhost-user devices we need to make sure it is supported by the offloaded
> device emulation (in-kernel or in another process).
> To do this we need to add VIRTIO_F_RING_RESET to the features bitmap
> passed to vhost_get_features(). This way it will be masked if the device
> does not support it.
> 
> This issue was initially discovered with vhost-vsock and vhost-user-vsock,
> and then also tested with vhost-user-rng which confirmed the same issue.
> They fail when sending features through VHOST_SET_FEATURES ioctl or
> VHOST_USER_SET_FEATURES message, since VIRTIO_F_RING_RESET is negotiated
> by the guest (Linux >= v6.0), but not supported by the device.
> 
> Fixes: 69e1c14aa2 ("virtio: core: vq reset feature negotation support")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1318
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> 
> To prevent this problem in the future, perhaps we should provide a function
> (e.g. vhost_device_get_features) where we go to mask all non-device-specific
> features (e.g VIRTIO_F_*, VIRTIO_RING_F_*) that are not emulated by QEMU but
> we expect them to be emulated by the vhost or vhost-user devices.
> Then we can call it in all .get_features callbacks just before return the
> features.
> 
> What do you think?
> 
> But maybe better to do that for the next release, I will send an RFC.

This patch looks good for 7.2.

I agree that in the long run this needs to be more robust so vhost
devices don't break every time a new feature bit is added.

> 
> Thanks,
> Stefano
> ---
>  hw/block/vhost-user-blk.c      |  1 +
>  hw/net/vhost_net.c             |  1 +
>  hw/scsi/vhost-scsi.c           |  1 +
>  hw/scsi/vhost-user-scsi.c      |  1 +
>  hw/virtio/vhost-user-fs.c      |  1 +
>  hw/virtio/vhost-user-gpio.c    |  1 +
>  hw/virtio/vhost-user-i2c.c     |  1 +
>  hw/virtio/vhost-user-rng.c     | 11 +++++++++--
>  hw/virtio/vhost-vsock-common.c |  1 +
>  net/vhost-vdpa.c               |  1 +
>  10 files changed, 18 insertions(+), 2 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]