[PATCH 2/5] vhost: involve device backends in feature negotiation
Many vhost devices in QEMU currently do not involve the device backend
in feature negotiation. This seems fine at first glance for device types
without their own feature bits (virtio-net has many but other device
types have none).
This overlooks the fact that QEMU's virtqueue implementation and the
device backend's implementation may support different features. QEMU
must not report features to the guest that the the device backend
doesn't support.
For example, QEMU supports VIRTIO 1.1 packed virtqueues while many
existing vhost device backends do not. When the user sets packed=on the
device backend breaks. This should have been handled gracefully by
feature negotiation instead.
Introduce vhost_get_default_features() and update all vhost devices in
QEMU to involve the device backend in feature negotiation.
This patch fixes the following error:
$ x86_64-softmmu/qemu-system-x86_64 \
-drive if=virtio,file=test.img,format=raw \
-chardev socket,path=/tmp/vhost-user-blk.sock,id=char0 \
-device vhost-user-blk-pci,chardev=char0,packed=on \
-object memory-backend-memfd,size=1G,share=on,id=ram0 \
-M accel=kvm,memory-backend=ram0
qemu-system-x86_64: Failed to set msg fds.
qemu-system-x86_64: vhost VQ 0 ring restore failed: -1: Success (0)
The vhost-user-blk backend failed as follows:
$ ./vhost-user-blk --socket-path=/tmp/vhost-user-blk.sock -b test2.img
vu_panic: virtio: zero sized buffers are not allowed
virtio-blk request missing headers
Signed-off-by: Stefan Hajnoczi
---
include/hw/virtio/vhost.h| 1 +
include/hw/virtio/virtio-gpu.h | 2 ++
include/sysemu/cryptodev-vhost.h | 11 +++
backends/cryptodev-vhost.c | 19 +++
hw/display/vhost-user-gpu.c | 17 +
hw/display/virtio-gpu-base.c | 2 +-
hw/input/vhost-user-input.c | 9 +
hw/virtio/vhost-user-fs.c| 5 +++--
hw/virtio/vhost-vsock.c | 5 +++--
hw/virtio/vhost.c| 22 ++
hw/virtio/virtio-crypto.c| 3 ++-
11 files changed, 90 insertions(+), 6 deletions(-)
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 085450c6f8..d2e54dd4a8 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -112,6 +112,7 @@ void vhost_virtqueue_mask(struct vhost_dev *hdev,
VirtIODevice *vdev, int n,
bool mask);
uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
uint64_t features);
+uint64_t vhost_get_default_features(struct vhost_dev *hdev, uint64_t features);
void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
uint64_t features);
bool vhost_has_free_slot(void);
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 6dd57f2025..41d270d80e 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -192,6 +192,8 @@ bool virtio_gpu_base_device_realize(DeviceState *qdev,
void virtio_gpu_base_reset(VirtIOGPUBase *g);
void virtio_gpu_base_fill_display_info(VirtIOGPUBase *g,
struct virtio_gpu_resp_display_info *dpy_info);
+uint64_t virtio_gpu_base_get_features(VirtIODevice *vdev, uint64_t features,
+ Error **errp);
/* virtio-gpu.c */
void virtio_gpu_ctrl_response(VirtIOGPU *g,
diff --git a/include/sysemu/cryptodev-vhost.h b/include/sysemu/cryptodev-vhost.h
index f42824fbde..e629446bfb 100644
--- a/include/sysemu/cryptodev-vhost.h
+++ b/include/sysemu/cryptodev-vhost.h
@@ -122,6 +122,17 @@ int cryptodev_vhost_start(VirtIODevice *dev, int
total_queues);
*/
void cryptodev_vhost_stop(VirtIODevice *dev, int total_queues);
+/**
+ * cryptodev_vhost_get_features:
+ * @dev: the virtio crypto object
+ * @requested_features: the features being offered
+ *
+ * Returns: the requested features bits that are supported by the vhost device,
+ * or the original request feature bits if vhost is disabled
+ *
+ */
+uint64_t cryptodev_vhost_get_features(VirtIODevice *dev, uint64_t features);
+
/**
* cryptodev_vhost_virtqueue_mask:
* @dev: the virtio crypto object
diff --git a/backends/cryptodev-vhost.c b/backends/cryptodev-vhost.c
index 8337c9a495..5f5a4fda7b 100644
--- a/backends/cryptodev-vhost.c
+++ b/backends/cryptodev-vhost.c
@@ -266,6 +266,20 @@ void cryptodev_vhost_stop(VirtIODevice *dev, int
total_queues)
assert(r >= 0);
}
+uint64_t cryptodev_vhost_get_features(VirtIODevice *dev, uint64_t features)
+{
+VirtIOCrypto *vcrypto = VIRTIO_CRYPTO(dev);
+CryptoDevBackend *b = vcrypto->cryptodev;
+CryptoDevBackendClient *cc = b->conf.peers.ccs[0];
+CryptoDevBackendVhost *vhost_crypto = cryptodev_get_vhost(cc, b, 0);
+
+if (!vhost_crypto) {
+return features; /* vhost disabled */
+}
+
+return vhost_get_default_features(&vhost_crypto->dev, features);
+}
+
void cryptodev_vhost_virtqueu
Re: [PATCH 2/5] vhost: involve device backends in feature negotiation
Hi Stefan
On Fri, May 22, 2020 at 7:18 PM Stefan Hajnoczi wrote:
>
> Many vhost devices in QEMU currently do not involve the device backend
> in feature negotiation. This seems fine at first glance for device types
> without their own feature bits (virtio-net has many but other device
> types have none).
>
> This overlooks the fact that QEMU's virtqueue implementation and the
> device backend's implementation may support different features. QEMU
> must not report features to the guest that the the device backend
> doesn't support.
>
> For example, QEMU supports VIRTIO 1.1 packed virtqueues while many
> existing vhost device backends do not. When the user sets packed=on the
> device backend breaks. This should have been handled gracefully by
> feature negotiation instead.
>
> Introduce vhost_get_default_features() and update all vhost devices in
> QEMU to involve the device backend in feature negotiation.
>
> This patch fixes the following error:
>
> $ x86_64-softmmu/qemu-system-x86_64 \
> -drive if=virtio,file=test.img,format=raw \
> -chardev socket,path=/tmp/vhost-user-blk.sock,id=char0 \
> -device vhost-user-blk-pci,chardev=char0,packed=on \
> -object memory-backend-memfd,size=1G,share=on,id=ram0 \
> -M accel=kvm,memory-backend=ram0
> qemu-system-x86_64: Failed to set msg fds.
> qemu-system-x86_64: vhost VQ 0 ring restore failed: -1: Success (0)
>
> The vhost-user-blk backend failed as follows:
>
> $ ./vhost-user-blk --socket-path=/tmp/vhost-user-blk.sock -b test2.img
> vu_panic: virtio: zero sized buffers are not allowed
> virtio-blk request missing headers
>
> Signed-off-by: Stefan Hajnoczi
> ---
> include/hw/virtio/vhost.h| 1 +
> include/hw/virtio/virtio-gpu.h | 2 ++
> include/sysemu/cryptodev-vhost.h | 11 +++
> backends/cryptodev-vhost.c | 19 +++
> hw/display/vhost-user-gpu.c | 17 +
> hw/display/virtio-gpu-base.c | 2 +-
> hw/input/vhost-user-input.c | 9 +
> hw/virtio/vhost-user-fs.c| 5 +++--
> hw/virtio/vhost-vsock.c | 5 +++--
> hw/virtio/vhost.c| 22 ++
> hw/virtio/virtio-crypto.c| 3 ++-
> 11 files changed, 90 insertions(+), 6 deletions(-)
>
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index 085450c6f8..d2e54dd4a8 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -112,6 +112,7 @@ void vhost_virtqueue_mask(struct vhost_dev *hdev,
> VirtIODevice *vdev, int n,
>bool mask);
> uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
> uint64_t features);
> +uint64_t vhost_get_default_features(struct vhost_dev *hdev, uint64_t
> features);
> void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
> uint64_t features);
> bool vhost_has_free_slot(void);
> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> index 6dd57f2025..41d270d80e 100644
> --- a/include/hw/virtio/virtio-gpu.h
> +++ b/include/hw/virtio/virtio-gpu.h
> @@ -192,6 +192,8 @@ bool virtio_gpu_base_device_realize(DeviceState *qdev,
> void virtio_gpu_base_reset(VirtIOGPUBase *g);
> void virtio_gpu_base_fill_display_info(VirtIOGPUBase *g,
> struct virtio_gpu_resp_display_info *dpy_info);
> +uint64_t virtio_gpu_base_get_features(VirtIODevice *vdev, uint64_t features,
> + Error **errp);
>
> /* virtio-gpu.c */
> void virtio_gpu_ctrl_response(VirtIOGPU *g,
> diff --git a/include/sysemu/cryptodev-vhost.h
> b/include/sysemu/cryptodev-vhost.h
> index f42824fbde..e629446bfb 100644
> --- a/include/sysemu/cryptodev-vhost.h
> +++ b/include/sysemu/cryptodev-vhost.h
> @@ -122,6 +122,17 @@ int cryptodev_vhost_start(VirtIODevice *dev, int
> total_queues);
> */
> void cryptodev_vhost_stop(VirtIODevice *dev, int total_queues);
>
> +/**
> + * cryptodev_vhost_get_features:
> + * @dev: the virtio crypto object
> + * @requested_features: the features being offered
> + *
> + * Returns: the requested features bits that are supported by the vhost
> device,
> + * or the original request feature bits if vhost is disabled
> + *
> + */
> +uint64_t cryptodev_vhost_get_features(VirtIODevice *dev, uint64_t features);
> +
> /**
> * cryptodev_vhost_virtqueue_mask:
> * @dev: the virtio crypto object
> diff --git a/backends/cryptodev-vhost.c b/backends/cryptodev-vhost.c
> index 8337c9a495..5f5a4fda7b 100644
> --- a/backends/cryptodev-vhost.c
> +++ b/backends/cryptodev-vhost.c
> @@ -266,6 +266,20 @@ void cryptodev_vhost_stop(VirtIODevice *dev, int
> total_queues)
> assert(r >= 0);
> }
>
> +uint64_t cryptodev_vhost_get_features(VirtIODevice *dev, uint64_t features)
> +{
> +VirtIOCrypto *vcrypto = VIRTIO_CRYPTO(dev);
> +CryptoDevBackend *b = vcrypto->cryptodev;
> +CryptoDevBackendClient *c
Re: [PATCH 2/5] vhost: involve device backends in feature negotiation
On 2020/5/23 上午1:17, Stefan Hajnoczi wrote:
Many vhost devices in QEMU currently do not involve the device backend
in feature negotiation. This seems fine at first glance for device types
without their own feature bits (virtio-net has many but other device
types have none).
This overlooks the fact that QEMU's virtqueue implementation and the
device backend's implementation may support different features. QEMU
must not report features to the guest that the the device backend
doesn't support.
For example, QEMU supports VIRTIO 1.1 packed virtqueues while many
existing vhost device backends do not. When the user sets packed=on the
device backend breaks. This should have been handled gracefully by
feature negotiation instead.
Introduce vhost_get_default_features() and update all vhost devices in
QEMU to involve the device backend in feature negotiation.
This patch fixes the following error:
$ x86_64-softmmu/qemu-system-x86_64 \
-drive if=virtio,file=test.img,format=raw \
-chardev socket,path=/tmp/vhost-user-blk.sock,id=char0 \
-device vhost-user-blk-pci,chardev=char0,packed=on \
-object memory-backend-memfd,size=1G,share=on,id=ram0 \
-M accel=kvm,memory-backend=ram0
qemu-system-x86_64: Failed to set msg fds.
qemu-system-x86_64: vhost VQ 0 ring restore failed: -1: Success (0)
It looks to me this could be fixed simply by adding VIRTIO_F_RING_PACKED
into user_feature_bits in vhost-user-blk.c.
And for the rest, we need require them to call vhost_get_features() with
the proper feature bits that needs to be acked by the backend.
The vhost-user-blk backend failed as follows:
$ ./vhost-user-blk --socket-path=/tmp/vhost-user-blk.sock -b test2.img
vu_panic: virtio: zero sized buffers are not allowed
virtio-blk request missing headers
Signed-off-by: Stefan Hajnoczi
---
include/hw/virtio/vhost.h| 1 +
include/hw/virtio/virtio-gpu.h | 2 ++
include/sysemu/cryptodev-vhost.h | 11 +++
backends/cryptodev-vhost.c | 19 +++
hw/display/vhost-user-gpu.c | 17 +
hw/display/virtio-gpu-base.c | 2 +-
hw/input/vhost-user-input.c | 9 +
hw/virtio/vhost-user-fs.c| 5 +++--
hw/virtio/vhost-vsock.c | 5 +++--
hw/virtio/vhost.c| 22 ++
hw/virtio/virtio-crypto.c| 3 ++-
11 files changed, 90 insertions(+), 6 deletions(-)
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 085450c6f8..d2e54dd4a8 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -112,6 +112,7 @@ void vhost_virtqueue_mask(struct vhost_dev *hdev,
VirtIODevice *vdev, int n,
bool mask);
uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
uint64_t features);
+uint64_t vhost_get_default_features(struct vhost_dev *hdev, uint64_t features);
void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
uint64_t features);
bool vhost_has_free_slot(void);
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 6dd57f2025..41d270d80e 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -192,6 +192,8 @@ bool virtio_gpu_base_device_realize(DeviceState *qdev,
void virtio_gpu_base_reset(VirtIOGPUBase *g);
void virtio_gpu_base_fill_display_info(VirtIOGPUBase *g,
struct virtio_gpu_resp_display_info *dpy_info);
+uint64_t virtio_gpu_base_get_features(VirtIODevice *vdev, uint64_t features,
+ Error **errp);
/* virtio-gpu.c */
void virtio_gpu_ctrl_response(VirtIOGPU *g,
diff --git a/include/sysemu/cryptodev-vhost.h b/include/sysemu/cryptodev-vhost.h
index f42824fbde..e629446bfb 100644
--- a/include/sysemu/cryptodev-vhost.h
+++ b/include/sysemu/cryptodev-vhost.h
@@ -122,6 +122,17 @@ int cryptodev_vhost_start(VirtIODevice *dev, int
total_queues);
*/
void cryptodev_vhost_stop(VirtIODevice *dev, int total_queues);
+/**
+ * cryptodev_vhost_get_features:
+ * @dev: the virtio crypto object
+ * @requested_features: the features being offered
+ *
+ * Returns: the requested features bits that are supported by the vhost device,
+ * or the original request feature bits if vhost is disabled
+ *
+ */
+uint64_t cryptodev_vhost_get_features(VirtIODevice *dev, uint64_t features);
+
/**
* cryptodev_vhost_virtqueue_mask:
* @dev: the virtio crypto object
diff --git a/backends/cryptodev-vhost.c b/backends/cryptodev-vhost.c
index 8337c9a495..5f5a4fda7b 100644
--- a/backends/cryptodev-vhost.c
+++ b/backends/cryptodev-vhost.c
@@ -266,6 +266,20 @@ void cryptodev_vhost_stop(VirtIODevice *dev, int
total_queues)
assert(r >= 0);
}
+uint64_t cryptodev_vhost_get_features(VirtIODevice *dev, uint64_t features)
+{
+VirtIOCrypto *vcrypto = VIRTIO_CRYPTO(dev);
+Cr
Re: [PATCH 2/5] vhost: involve device backends in feature negotiation
On Fri, May 29, 2020 at 03:04:54PM +0800, Jason Wang wrote: > > On 2020/5/23 上午1:17, Stefan Hajnoczi wrote: > > Many vhost devices in QEMU currently do not involve the device backend > > in feature negotiation. This seems fine at first glance for device types > > without their own feature bits (virtio-net has many but other device > > types have none). > > > > This overlooks the fact that QEMU's virtqueue implementation and the > > device backend's implementation may support different features. QEMU > > must not report features to the guest that the the device backend > > doesn't support. > > > > For example, QEMU supports VIRTIO 1.1 packed virtqueues while many > > existing vhost device backends do not. When the user sets packed=on the > > device backend breaks. This should have been handled gracefully by > > feature negotiation instead. > > > > Introduce vhost_get_default_features() and update all vhost devices in > > QEMU to involve the device backend in feature negotiation. > > > > This patch fixes the following error: > > > >$ x86_64-softmmu/qemu-system-x86_64 \ > >-drive if=virtio,file=test.img,format=raw \ > >-chardev socket,path=/tmp/vhost-user-blk.sock,id=char0 \ > >-device vhost-user-blk-pci,chardev=char0,packed=on \ > >-object memory-backend-memfd,size=1G,share=on,id=ram0 \ > >-M accel=kvm,memory-backend=ram0 > >qemu-system-x86_64: Failed to set msg fds. > >qemu-system-x86_64: vhost VQ 0 ring restore failed: -1: Success (0) > > > It looks to me this could be fixed simply by adding VIRTIO_F_RING_PACKED > into user_feature_bits in vhost-user-blk.c. > > And for the rest, we need require them to call vhost_get_features() with the > proper feature bits that needs to be acked by the backend. There is a backwards-compatibility problem: we cannot call vhost_get_features() with the full set of feature bits that each device type supports because existing vhost-user backends don't advertise features properly. QEMU disables features not advertised by the vhost-user backend. For example, if we add VIRTIO_RING_F_EVENT_IDX then it will always be disabled for existing libvhost-user backends because they don't advertise this bit :(. The reason I introduced vhost_get_default_features() is to at least salvage VIRTIO_F_IOMMU_PLATFORM and VIRTIO_F_RING_PACKED. These bits can be safely negotiated for all devices. Any new transport/vring VIRTIO feature bits can also be added to vhost_get_default_features() safely. If a vhost-user device wants to use device type-specific feature bits then it really needs to call vhost_get_features() directly as you suggest. But it's important to check whether existing vhost-user backends actually advertise them - because if they don't advertise them but rely on them then we'll break existing backends. Would you prefer a different approach? > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > > index aff98a0ede..f8a144dcd0 100644 > > --- a/hw/virtio/vhost.c > > +++ b/hw/virtio/vhost.c > > @@ -48,6 +48,23 @@ static unsigned int used_memslots; > > static QLIST_HEAD(, vhost_dev) vhost_devices = > > QLIST_HEAD_INITIALIZER(vhost_devices); > > +/* > > + * Feature bits that device backends must explicitly report. Feature bits > > not > > + * listed here maybe set by QEMU without checking with the device backend. > > + * Ideally all feature bits would be listed here but existing vhost device > > + * implementations do not explicitly report bits like VIRTIO_F_VERSION_1, > > so we > > + * can only assume they are supported. > > > For most backends, we care about the features for datapath. So this is not > true at least for networking device, since VIRTIO_F_VERSION_1 have impact on > the length of vnet header length. The net device is in good shape and doesn't use vhost_default_feature_bits[]. vhost_default_feature_bits[] is for devices that haven't been negotiating feature bits properly. The goal is start involving them in feature negotiation without breaking existing backends. Would you like me to rephrase this comment in some way? > > + * > > + * New feature bits added to the VIRTIO spec should usually be included > > here > > + * so that existing vhost device backends that do not support them yet > > continue > > + * to work. > > > It actually depends on the type of backend. > > Kernel vhost-net does not validate GSO features since qemu can talk directly > to TAP and vhost doesn't report those features. But for vhost-user GSO > features must be validated by qemu since qemu can't see what is behind > vhost-user. Maybe the comment should say "New transport/vring feature bits". I'm thinking about things like VIRTIO_F_RING_PACKED that are not device-specific but require backend support. The GSO features you mentioned are device-specific. Devices that want to let the backend advertise device-specific features cannot use vhost_default_feature_bits[]. > > + */ > > +static const int vhost_default_feature_bi
Re: [PATCH 2/5] vhost: involve device backends in feature negotiation
* Stefan Hajnoczi (stefa...@redhat.com) wrote: > On Fri, May 29, 2020 at 03:04:54PM +0800, Jason Wang wrote: > > > > On 2020/5/23 上午1:17, Stefan Hajnoczi wrote: > > > Many vhost devices in QEMU currently do not involve the device backend > > > in feature negotiation. This seems fine at first glance for device types > > > without their own feature bits (virtio-net has many but other device > > > types have none). > > > > > > This overlooks the fact that QEMU's virtqueue implementation and the > > > device backend's implementation may support different features. QEMU > > > must not report features to the guest that the the device backend > > > doesn't support. > > > > > > For example, QEMU supports VIRTIO 1.1 packed virtqueues while many > > > existing vhost device backends do not. When the user sets packed=on the > > > device backend breaks. This should have been handled gracefully by > > > feature negotiation instead. > > > > > > Introduce vhost_get_default_features() and update all vhost devices in > > > QEMU to involve the device backend in feature negotiation. > > > > > > This patch fixes the following error: > > > > > >$ x86_64-softmmu/qemu-system-x86_64 \ > > >-drive if=virtio,file=test.img,format=raw \ > > >-chardev socket,path=/tmp/vhost-user-blk.sock,id=char0 \ > > >-device vhost-user-blk-pci,chardev=char0,packed=on \ > > >-object memory-backend-memfd,size=1G,share=on,id=ram0 \ > > >-M accel=kvm,memory-backend=ram0 > > >qemu-system-x86_64: Failed to set msg fds. > > >qemu-system-x86_64: vhost VQ 0 ring restore failed: -1: Success (0) > > > > > > It looks to me this could be fixed simply by adding VIRTIO_F_RING_PACKED > > into user_feature_bits in vhost-user-blk.c. > > > > And for the rest, we need require them to call vhost_get_features() with the > > proper feature bits that needs to be acked by the backend. > > There is a backwards-compatibility problem: we cannot call > vhost_get_features() with the full set of feature bits that each device > type supports because existing vhost-user backends don't advertise > features properly. QEMU disables features not advertised by the > vhost-user backend. > > For example, if we add VIRTIO_RING_F_EVENT_IDX then it will always be > disabled for existing libvhost-user backends because they don't > advertise this bit :(. > > The reason I introduced vhost_get_default_features() is to at least > salvage VIRTIO_F_IOMMU_PLATFORM and VIRTIO_F_RING_PACKED. These bits can > be safely negotiated for all devices. > > Any new transport/vring VIRTIO feature bits can also be added to > vhost_get_default_features() safely. > > If a vhost-user device wants to use device type-specific feature bits > then it really needs to call vhost_get_features() directly as you > suggest. But it's important to check whether existing vhost-user > backends actually advertise them - because if they don't advertise them > but rely on them then we'll break existing backends. What about adding a VHOST_USER_PROTOCOL_F_BACKEND_F to indicate the backend knows how to advertise the backend features. (Although my understanding of virtio feature flags is thin) Dave > Would you prefer a different approach? > > > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > > > index aff98a0ede..f8a144dcd0 100644 > > > --- a/hw/virtio/vhost.c > > > +++ b/hw/virtio/vhost.c > > > @@ -48,6 +48,23 @@ static unsigned int used_memslots; > > > static QLIST_HEAD(, vhost_dev) vhost_devices = > > > QLIST_HEAD_INITIALIZER(vhost_devices); > > > +/* > > > + * Feature bits that device backends must explicitly report. Feature > > > bits not > > > + * listed here maybe set by QEMU without checking with the device > > > backend. > > > + * Ideally all feature bits would be listed here but existing vhost > > > device > > > + * implementations do not explicitly report bits like > > > VIRTIO_F_VERSION_1, so we > > > + * can only assume they are supported. > > > > > > For most backends, we care about the features for datapath. So this is not > > true at least for networking device, since VIRTIO_F_VERSION_1 have impact on > > the length of vnet header length. > > The net device is in good shape and doesn't use vhost_default_feature_bits[]. > > vhost_default_feature_bits[] is for devices that haven't been > negotiating feature bits properly. The goal is start involving them in > feature negotiation without breaking existing backends. > > Would you like me to rephrase this comment in some way? > > > > + * > > > + * New feature bits added to the VIRTIO spec should usually be included > > > here > > > + * so that existing vhost device backends that do not support them yet > > > continue > > > + * to work. > > > > > > It actually depends on the type of backend. > > > > Kernel vhost-net does not validate GSO features since qemu can talk directly > > to TAP and vhost doesn't report those features. But for vhost-user GSO > > features must be valida
Re: [PATCH 2/5] vhost: involve device backends in feature negotiation
On Wed, May 27, 2020 at 04:28:41PM +0200, Marc-André Lureau wrote: > Hi Stefan > > On Fri, May 22, 2020 at 7:18 PM Stefan Hajnoczi wrote: > > > > Many vhost devices in QEMU currently do not involve the device backend > > in feature negotiation. This seems fine at first glance for device types > > without their own feature bits (virtio-net has many but other device > > types have none). > > > > This overlooks the fact that QEMU's virtqueue implementation and the > > device backend's implementation may support different features. QEMU > > must not report features to the guest that the the device backend > > doesn't support. > > > > For example, QEMU supports VIRTIO 1.1 packed virtqueues while many > > existing vhost device backends do not. When the user sets packed=on the > > device backend breaks. This should have been handled gracefully by > > feature negotiation instead. > > > > Introduce vhost_get_default_features() and update all vhost devices in > > QEMU to involve the device backend in feature negotiation. > > > > This patch fixes the following error: > > > > $ x86_64-softmmu/qemu-system-x86_64 \ > > -drive if=virtio,file=test.img,format=raw \ > > -chardev socket,path=/tmp/vhost-user-blk.sock,id=char0 \ > > -device vhost-user-blk-pci,chardev=char0,packed=on \ > > -object memory-backend-memfd,size=1G,share=on,id=ram0 \ > > -M accel=kvm,memory-backend=ram0 > > qemu-system-x86_64: Failed to set msg fds. > > qemu-system-x86_64: vhost VQ 0 ring restore failed: -1: Success (0) > > > > The vhost-user-blk backend failed as follows: > > > > $ ./vhost-user-blk --socket-path=/tmp/vhost-user-blk.sock -b test2.img > > vu_panic: virtio: zero sized buffers are not allowed > > virtio-blk request missing headers > > > > Signed-off-by: Stefan Hajnoczi > > --- > > include/hw/virtio/vhost.h| 1 + > > include/hw/virtio/virtio-gpu.h | 2 ++ > > include/sysemu/cryptodev-vhost.h | 11 +++ > > backends/cryptodev-vhost.c | 19 +++ > > hw/display/vhost-user-gpu.c | 17 + > > hw/display/virtio-gpu-base.c | 2 +- > > hw/input/vhost-user-input.c | 9 + > > hw/virtio/vhost-user-fs.c| 5 +++-- > > hw/virtio/vhost-vsock.c | 5 +++-- > > hw/virtio/vhost.c| 22 ++ > > hw/virtio/virtio-crypto.c| 3 ++- > > 11 files changed, 90 insertions(+), 6 deletions(-) > > > > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h > > index 085450c6f8..d2e54dd4a8 100644 > > --- a/include/hw/virtio/vhost.h > > +++ b/include/hw/virtio/vhost.h > > @@ -112,6 +112,7 @@ void vhost_virtqueue_mask(struct vhost_dev *hdev, > > VirtIODevice *vdev, int n, > >bool mask); > > uint64_t vhost_get_features(struct vhost_dev *hdev, const int > > *feature_bits, > > uint64_t features); > > +uint64_t vhost_get_default_features(struct vhost_dev *hdev, uint64_t > > features); > > void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits, > > uint64_t features); > > bool vhost_has_free_slot(void); > > diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h > > index 6dd57f2025..41d270d80e 100644 > > --- a/include/hw/virtio/virtio-gpu.h > > +++ b/include/hw/virtio/virtio-gpu.h > > @@ -192,6 +192,8 @@ bool virtio_gpu_base_device_realize(DeviceState *qdev, > > void virtio_gpu_base_reset(VirtIOGPUBase *g); > > void virtio_gpu_base_fill_display_info(VirtIOGPUBase *g, > > struct virtio_gpu_resp_display_info *dpy_info); > > +uint64_t virtio_gpu_base_get_features(VirtIODevice *vdev, uint64_t > > features, > > + Error **errp); > > > > /* virtio-gpu.c */ > > void virtio_gpu_ctrl_response(VirtIOGPU *g, > > diff --git a/include/sysemu/cryptodev-vhost.h > > b/include/sysemu/cryptodev-vhost.h > > index f42824fbde..e629446bfb 100644 > > --- a/include/sysemu/cryptodev-vhost.h > > +++ b/include/sysemu/cryptodev-vhost.h > > @@ -122,6 +122,17 @@ int cryptodev_vhost_start(VirtIODevice *dev, int > > total_queues); > > */ > > void cryptodev_vhost_stop(VirtIODevice *dev, int total_queues); > > > > +/** > > + * cryptodev_vhost_get_features: > > + * @dev: the virtio crypto object > > + * @requested_features: the features being offered > > + * > > + * Returns: the requested features bits that are supported by the vhost > > device, > > + * or the original request feature bits if vhost is disabled > > + * > > + */ > > +uint64_t cryptodev_vhost_get_features(VirtIODevice *dev, uint64_t > > features); > > + > > /** > > * cryptodev_vhost_virtqueue_mask: > > * @dev: the virtio crypto object > > diff --git a/backends/cryptodev-vhost.c b/backends/cryptodev-vhost.c > > index 8337c9a495..5f5a4fda7b 100644 > > --- a/backends/cryptodev-vhost.c > > +++ b/backends/cryptodev-vhost.c > > @@ -266,6 +266,20 @@ void cry
Re: [PATCH 2/5] vhost: involve device backends in feature negotiation
On 2020/5/29 下午9:56, Stefan Hajnoczi wrote: On Fri, May 29, 2020 at 03:04:54PM +0800, Jason Wang wrote: On 2020/5/23 上午1:17, Stefan Hajnoczi wrote: Many vhost devices in QEMU currently do not involve the device backend in feature negotiation. This seems fine at first glance for device types without their own feature bits (virtio-net has many but other device types have none). This overlooks the fact that QEMU's virtqueue implementation and the device backend's implementation may support different features. QEMU must not report features to the guest that the the device backend doesn't support. For example, QEMU supports VIRTIO 1.1 packed virtqueues while many existing vhost device backends do not. When the user sets packed=on the device backend breaks. This should have been handled gracefully by feature negotiation instead. Introduce vhost_get_default_features() and update all vhost devices in QEMU to involve the device backend in feature negotiation. This patch fixes the following error: $ x86_64-softmmu/qemu-system-x86_64 \ -drive if=virtio,file=test.img,format=raw \ -chardev socket,path=/tmp/vhost-user-blk.sock,id=char0 \ -device vhost-user-blk-pci,chardev=char0,packed=on \ -object memory-backend-memfd,size=1G,share=on,id=ram0 \ -M accel=kvm,memory-backend=ram0 qemu-system-x86_64: Failed to set msg fds. qemu-system-x86_64: vhost VQ 0 ring restore failed: -1: Success (0) It looks to me this could be fixed simply by adding VIRTIO_F_RING_PACKED into user_feature_bits in vhost-user-blk.c. And for the rest, we need require them to call vhost_get_features() with the proper feature bits that needs to be acked by the backend. There is a backwards-compatibility problem: we cannot call vhost_get_features() with the full set of feature bits that each device type supports because existing vhost-user backends don't advertise features properly. QEMU disables features not advertised by the vhost-user backend. For example, if we add VIRTIO_RING_F_EVENT_IDX then it will always be disabled for existing libvhost-user backends because they don't advertise this bit :(. Agree. The reason I introduced vhost_get_default_features() is to at least salvage VIRTIO_F_IOMMU_PLATFORM and VIRTIO_F_RING_PACKED. These bits can be safely negotiated for all devices. Any new transport/vring VIRTIO feature bits can also be added to vhost_get_default_features() safely. If a vhost-user device wants to use device type-specific feature bits then it really needs to call vhost_get_features() directly as you suggest. But it's important to check whether existing vhost-user backends actually advertise them - because if they don't advertise them but rely on them then we'll break existing backends. Would you prefer a different approach? I get you now so I think not. Maybe we need document the expected behavior of VHOST_USER_GET_FEATURES e.g which set of features that must be advertised explicitly. And for the set you mention here, we probably need to add: VIRTIO_F_ORDER_PLATFORM, VIRTIO_F_ANY_LAYOUT (not sure if it was too late). And VIRTIO_F_IN_ORDER diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index aff98a0ede..f8a144dcd0 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -48,6 +48,23 @@ static unsigned int used_memslots; static QLIST_HEAD(, vhost_dev) vhost_devices = QLIST_HEAD_INITIALIZER(vhost_devices); +/* + * Feature bits that device backends must explicitly report. Feature bits not + * listed here maybe set by QEMU without checking with the device backend. + * Ideally all feature bits would be listed here but existing vhost device + * implementations do not explicitly report bits like VIRTIO_F_VERSION_1, so we + * can only assume they are supported. For most backends, we care about the features for datapath. So this is not true at least for networking device, since VIRTIO_F_VERSION_1 have impact on the length of vnet header length. The net device is in good shape and doesn't use vhost_default_feature_bits[]. vhost_default_feature_bits[] is for devices that haven't been negotiating feature bits properly. The goal is start involving them in feature negotiation without breaking existing backends. Would you like me to rephrase this comment in some way? That will be better. + * + * New feature bits added to the VIRTIO spec should usually be included here + * so that existing vhost device backends that do not support them yet continue + * to work. It actually depends on the type of backend. Kernel vhost-net does not validate GSO features since qemu can talk directly to TAP and vhost doesn't report those features. But for vhost-user GSO features must be validated by qemu since qemu can't see what is behind vhost-user. Maybe the comment should say "New transport/vring feature bits". I'm thinking about things like VIRTIO_F_RING_PACKED that are not device-specific but require backend support. The GSO features you mentione
Re: [PATCH 2/5] vhost: involve device backends in feature negotiation
On Mon, Jun 01, 2020 at 08:51:43PM +0800, Jason Wang wrote: > > On 2020/5/29 下午9:56, Stefan Hajnoczi wrote: > > On Fri, May 29, 2020 at 03:04:54PM +0800, Jason Wang wrote: > > > On 2020/5/23 上午1:17, Stefan Hajnoczi wrote: > > > > Many vhost devices in QEMU currently do not involve the device backend > > > > in feature negotiation. This seems fine at first glance for device types > > > > without their own feature bits (virtio-net has many but other device > > > > types have none). > > > > > > > > This overlooks the fact that QEMU's virtqueue implementation and the > > > > device backend's implementation may support different features. QEMU > > > > must not report features to the guest that the the device backend > > > > doesn't support. > > > > > > > > For example, QEMU supports VIRTIO 1.1 packed virtqueues while many > > > > existing vhost device backends do not. When the user sets packed=on the > > > > device backend breaks. This should have been handled gracefully by > > > > feature negotiation instead. > > > > > > > > Introduce vhost_get_default_features() and update all vhost devices in > > > > QEMU to involve the device backend in feature negotiation. > > > > > > > > This patch fixes the following error: > > > > > > > > $ x86_64-softmmu/qemu-system-x86_64 \ > > > > -drive if=virtio,file=test.img,format=raw \ > > > > -chardev socket,path=/tmp/vhost-user-blk.sock,id=char0 \ > > > > -device vhost-user-blk-pci,chardev=char0,packed=on \ > > > > -object memory-backend-memfd,size=1G,share=on,id=ram0 \ > > > > -M accel=kvm,memory-backend=ram0 > > > > qemu-system-x86_64: Failed to set msg fds. > > > > qemu-system-x86_64: vhost VQ 0 ring restore failed: -1: Success (0) > > > > > > It looks to me this could be fixed simply by adding VIRTIO_F_RING_PACKED > > > into user_feature_bits in vhost-user-blk.c. > > > > > > And for the rest, we need require them to call vhost_get_features() with > > > the > > > proper feature bits that needs to be acked by the backend. > > There is a backwards-compatibility problem: we cannot call > > vhost_get_features() with the full set of feature bits that each device > > type supports because existing vhost-user backends don't advertise > > features properly. QEMU disables features not advertised by the > > vhost-user backend. > > > > For example, if we add VIRTIO_RING_F_EVENT_IDX then it will always be > > disabled for existing libvhost-user backends because they don't > > advertise this bit :(. > > > Agree. > > > > > > The reason I introduced vhost_get_default_features() is to at least > > salvage VIRTIO_F_IOMMU_PLATFORM and VIRTIO_F_RING_PACKED. These bits can > > be safely negotiated for all devices. > > > > Any new transport/vring VIRTIO feature bits can also be added to > > vhost_get_default_features() safely. > > > > If a vhost-user device wants to use device type-specific feature bits > > then it really needs to call vhost_get_features() directly as you > > suggest. But it's important to check whether existing vhost-user > > backends actually advertise them - because if they don't advertise them > > but rely on them then we'll break existing backends. > > > > Would you prefer a different approach? > > > I get you now so I think not. > > Maybe we need document the expected behavior of VHOST_USER_GET_FEATURES e.g > which set of features that must be advertised explicitly. Good idea. I'll update the vhost-user spec. > And for the set you mention here, we probably need to add: > > VIRTIO_F_ORDER_PLATFORM, > VIRTIO_F_ANY_LAYOUT (not sure if it was too late). > > And > > VIRTIO_F_IN_ORDER Thanks, will check them and add them if possible. > > > > > > > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > > > > index aff98a0ede..f8a144dcd0 100644 > > > > --- a/hw/virtio/vhost.c > > > > +++ b/hw/virtio/vhost.c > > > > @@ -48,6 +48,23 @@ static unsigned int used_memslots; > > > >static QLIST_HEAD(, vhost_dev) vhost_devices = > > > >QLIST_HEAD_INITIALIZER(vhost_devices); > > > > +/* > > > > + * Feature bits that device backends must explicitly report. Feature > > > > bits not > > > > + * listed here maybe set by QEMU without checking with the device > > > > backend. > > > > + * Ideally all feature bits would be listed here but existing vhost > > > > device > > > > + * implementations do not explicitly report bits like > > > > VIRTIO_F_VERSION_1, so we > > > > + * can only assume they are supported. > > > > > > For most backends, we care about the features for datapath. So this is not > > > true at least for networking device, since VIRTIO_F_VERSION_1 have impact > > > on > > > the length of vnet header length. > > The net device is in good shape and doesn't use > > vhost_default_feature_bits[]. > > > > vhost_default_feature_bits[] is for devices that haven't been > > negotiating feature bits properly. The goal is start involving them in > > feature negotiation without breaking exist
Re: [PATCH 2/5] vhost: involve device backends in feature negotiation
On Fri, May 29, 2020 at 03:29:13PM +0100, Dr. David Alan Gilbert wrote: > * Stefan Hajnoczi (stefa...@redhat.com) wrote: > > On Fri, May 29, 2020 at 03:04:54PM +0800, Jason Wang wrote: > > > > > > On 2020/5/23 上午1:17, Stefan Hajnoczi wrote: > > > > Many vhost devices in QEMU currently do not involve the device backend > > > > in feature negotiation. This seems fine at first glance for device types > > > > without their own feature bits (virtio-net has many but other device > > > > types have none). > > > > > > > > This overlooks the fact that QEMU's virtqueue implementation and the > > > > device backend's implementation may support different features. QEMU > > > > must not report features to the guest that the the device backend > > > > doesn't support. > > > > > > > > For example, QEMU supports VIRTIO 1.1 packed virtqueues while many > > > > existing vhost device backends do not. When the user sets packed=on the > > > > device backend breaks. This should have been handled gracefully by > > > > feature negotiation instead. > > > > > > > > Introduce vhost_get_default_features() and update all vhost devices in > > > > QEMU to involve the device backend in feature negotiation. > > > > > > > > This patch fixes the following error: > > > > > > > >$ x86_64-softmmu/qemu-system-x86_64 \ > > > >-drive if=virtio,file=test.img,format=raw \ > > > >-chardev socket,path=/tmp/vhost-user-blk.sock,id=char0 \ > > > >-device vhost-user-blk-pci,chardev=char0,packed=on \ > > > >-object memory-backend-memfd,size=1G,share=on,id=ram0 \ > > > >-M accel=kvm,memory-backend=ram0 > > > >qemu-system-x86_64: Failed to set msg fds. > > > >qemu-system-x86_64: vhost VQ 0 ring restore failed: -1: Success (0) > > > > > > > > > It looks to me this could be fixed simply by adding VIRTIO_F_RING_PACKED > > > into user_feature_bits in vhost-user-blk.c. > > > > > > And for the rest, we need require them to call vhost_get_features() with > > > the > > > proper feature bits that needs to be acked by the backend. > > > > There is a backwards-compatibility problem: we cannot call > > vhost_get_features() with the full set of feature bits that each device > > type supports because existing vhost-user backends don't advertise > > features properly. QEMU disables features not advertised by the > > vhost-user backend. > > > > For example, if we add VIRTIO_RING_F_EVENT_IDX then it will always be > > disabled for existing libvhost-user backends because they don't > > advertise this bit :(. > > > > The reason I introduced vhost_get_default_features() is to at least > > salvage VIRTIO_F_IOMMU_PLATFORM and VIRTIO_F_RING_PACKED. These bits can > > be safely negotiated for all devices. > > > > Any new transport/vring VIRTIO feature bits can also be added to > > vhost_get_default_features() safely. > > > > If a vhost-user device wants to use device type-specific feature bits > > then it really needs to call vhost_get_features() directly as you > > suggest. But it's important to check whether existing vhost-user > > backends actually advertise them - because if they don't advertise them > > but rely on them then we'll break existing backends. > > What about adding a VHOST_USER_PROTOCOL_F_BACKEND_F to indicate the > backend knows how to advertise the backend features. > > (Although my understanding of virtio feature flags is thin) Luckily the feature bits we've missed out on are mostly legacy features or features we always want to enable, so I'm not sure a solution for negotiating all feature bits is needed. Not all features should be controlled by the backend since vhost devices often are a strict subset of a VIRTIO device. Documenting which feature bits are controlled by the backend seems like a reasonable way of clarifying the current state and preventing similar issues in the future. Stefan signature.asc Description: PGP signature
