[PATCH 0/5] virtio: enable VIRTIO_F_RING_PACKED for all devices

[PATCH 0/5] virtio: enable VIRTIO_F_RING_PACKED for all devices

[Stefan Hajnoczi]

The VIRTIO 1.1 packed virtqueue layout improves performance and guest driver
support has been available since Linux v5.0. virtio-blk benchmarks show it is
beneficial for non-net devices too so I wrote patches to enable it for all
devices.

It turned out to be trickier than I expected because vhost feature negotiation
is currently not ready for new virtqueue feature bits like
VIRTIO_F_RING_PACKED. Patches 2-4 solve this.

Patch 5 then enables packed virtqueues.

Stefan Hajnoczi (5):
  tests/libqos: mask out VIRTIO_F_RING_PACKED for now
  vhost: involve device backends in feature negotiation
  vhost-user-blk: add VIRTIO_F_RING_PACKED feature bit
  vhost-scsi: add VIRTIO_F_VERSION_1 and VIRTIO_F_RING_PACKED
  virtio: enable VIRTIO_F_RING_PACKED for all devices

 include/hw/virtio/vhost.h        |  1 +
 include/hw/virtio/virtio-gpu.h   |  2 ++
 include/hw/virtio/virtio.h       |  2 +-
 include/sysemu/cryptodev-vhost.h | 11 +++++++++++
 backends/cryptodev-vhost.c       | 19 +++++++++++++++++++
 hw/block/vhost-user-blk.c        |  1 +
 hw/core/machine.c                | 18 +++++++++++++++++-
 hw/display/vhost-user-gpu.c      | 17 +++++++++++++++++
 hw/display/virtio-gpu-base.c     |  2 +-
 hw/input/vhost-user-input.c      |  9 +++++++++
 hw/scsi/vhost-scsi.c             |  2 ++
 hw/scsi/vhost-user-scsi.c        |  2 ++
 hw/virtio/vhost-user-fs.c        |  5 +++--
 hw/virtio/vhost-vsock.c          |  5 +++--
 hw/virtio/vhost.c                | 22 ++++++++++++++++++++++
 hw/virtio/virtio-crypto.c        |  3 ++-
 tests/qtest/libqos/virtio.c      |  3 ++-
 17 files changed, 115 insertions(+), 9 deletions(-)

-- 
2.25.3

[PATCH 1/5] tests/libqos: mask out VIRTIO_F_RING_PACKED for now

[Stefan Hajnoczi]

The libqos VIRTIO code does not implement the packed virtqueue layout
yet. Mask out the feature bit for now because tests have a habit of
enabling all device feature bits and we don't want packed virtqueues to
be enabled.

Later patches will enable VIRTIO_F_RING_PACKED so prepare libqos now.

Cc: Thomas Huth <thuth@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/qtest/libqos/virtio.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/libqos/virtio.c b/tests/qtest/libqos/virtio.c
index 9aa360620c..1c3f4a0c8b 100644
--- a/tests/qtest/libqos/virtio.c
+++ b/tests/qtest/libqos/virtio.c
@@ -96,7 +96,8 @@ uint64_t qvirtio_config_readq(QVirtioDevice *d, uint64_t addr)
 
 uint64_t qvirtio_get_features(QVirtioDevice *d)
 {
-    return d->bus->get_features(d);
+    /* qvirtio does not support packed virtqueues yet */
+    return d->bus->get_features(d) & ~(1ull << VIRTIO_F_RING_PACKED);
 }
 
 void qvirtio_set_features(QVirtioDevice *d, uint64_t features)
-- 
2.25.3

Re: [PATCH 1/5] tests/libqos: mask out VIRTIO_F_RING_PACKED for now

[Thomas Huth]

> From: "Stefan Hajnoczi" <stefanha@redhat.com>
> Sent: Friday, May 22, 2020 7:17:22 PM
> 
> The libqos VIRTIO code does not implement the packed virtqueue layout
> yet. Mask out the feature bit for now because tests have a habit of
> enabling all device feature

Sounds like we should rather fix these tests in the long run - they
should really only enable the bits that they support...

> bits and we don't want packed virtqueues to
> be enabled.
> 
> Later patches will enable VIRTIO_F_RING_PACKED so prepare libqos now.
> 
> Cc: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/qtest/libqos/virtio.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/libqos/virtio.c b/tests/qtest/libqos/virtio.c
> index 9aa360620c..1c3f4a0c8b 100644
> --- a/tests/qtest/libqos/virtio.c
> +++ b/tests/qtest/libqos/virtio.c
> @@ -96,7 +96,8 @@ uint64_t qvirtio_config_readq(QVirtioDevice *d, uint64_t
> addr)
>  
>  uint64_t qvirtio_get_features(QVirtioDevice *d)
>  {
> -    return d->bus->get_features(d);
> +    /* qvirtio does not support packed virtqueues yet */
> +    return d->bus->get_features(d) & ~(1ull << VIRTIO_F_RING_PACKED);
>  }

... but as a temporary work-around, that should be fine, too.
(in case you respin, maybe add a TODO comment here, too, to remind us to fix
the tests later).

Acked-by: Thomas Huth <thuth@redhat.com>

[PATCH 2/5] vhost: involve device backends in feature negotiation

[Stefan Hajnoczi]

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 <stefanha@redhat.com>
---
 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_virtqueue_mask(VirtIODevice *dev,
                                            int queue,
                                            int idx, bool mask)
@@ -333,6 +347,11 @@ void cryptodev_vhost_stop(VirtIODevice *dev, int total_queues)
 {
 }
 
+uint64_t cryptodev_vhost_get_features(VirtIODevice *dev, uint64_t features)
+{
+    return features;
+}
+
 void cryptodev_vhost_virtqueue_mask(VirtIODevice *dev,
                                     int queue,
                                     int idx, bool mask)
diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
index 4cdaee1bde..e483df2a9e 100644
--- a/hw/display/vhost-user-gpu.c
+++ b/hw/display/vhost-user-gpu.c
@@ -466,6 +466,22 @@ vhost_user_gpu_set_config(VirtIODevice *vdev,
     }
 }
 
+static uint64_t
+vhost_user_gpu_get_features(VirtIODevice *vdev, uint64_t features,
+                            Error **errp)
+{
+    VhostUserGPU *g = VHOST_USER_GPU(vdev);
+    Error *local_err = NULL;
+
+    features = virtio_gpu_base_get_features(vdev, features, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return 0;
+    }
+
+    return vhost_get_default_features(&g->vhost->dev, features);
+}
+
 static void
 vhost_user_gpu_set_status(VirtIODevice *vdev, uint8_t val)
 {
@@ -582,6 +598,7 @@ vhost_user_gpu_class_init(ObjectClass *klass, void *data)
 
     vdc->realize = vhost_user_gpu_device_realize;
     vdc->reset = vhost_user_gpu_reset;
+    vdc->get_features = vhost_user_gpu_get_features;
     vdc->set_status   = vhost_user_gpu_set_status;
     vdc->guest_notifier_mask = vhost_user_gpu_guest_notifier_mask;
     vdc->guest_notifier_pending = vhost_user_gpu_guest_notifier_pending;
diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
index c159351be3..05d1ff2db2 100644
--- a/hw/display/virtio-gpu-base.c
+++ b/hw/display/virtio-gpu-base.c
@@ -176,7 +176,7 @@ virtio_gpu_base_device_realize(DeviceState *qdev,
     return true;
 }
 
-static uint64_t
+uint64_t
 virtio_gpu_base_get_features(VirtIODevice *vdev, uint64_t features,
                              Error **errp)
 {
diff --git a/hw/input/vhost-user-input.c b/hw/input/vhost-user-input.c
index 63984a8ba7..1371fb32cc 100644
--- a/hw/input/vhost-user-input.c
+++ b/hw/input/vhost-user-input.c
@@ -45,6 +45,14 @@ static void vhost_input_change_active(VirtIOInput *vinput)
     }
 }
 
+static uint64_t vhost_input_get_features(VirtIODevice *vdev, uint64_t features,
+                                         Error **errp)
+{
+    VHostUserInput *vhi = VHOST_USER_INPUT(vdev);
+
+    return vhost_get_default_features(&vhi->vhost->dev, features);
+}
+
 static void vhost_input_get_config(VirtIODevice *vdev, uint8_t *config_data)
 {
     VirtIOInput *vinput = VIRTIO_INPUT(vdev);
@@ -89,6 +97,7 @@ static void vhost_input_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->vmsd = &vmstate_vhost_input;
+    vdc->get_features = vhost_input_get_features;
     vdc->get_config = vhost_input_get_config;
     vdc->set_config = vhost_input_set_config;
     vic->realize = vhost_input_realize;
diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
index 1bc5d03a00..56015ca3d4 100644
--- a/hw/virtio/vhost-user-fs.c
+++ b/hw/virtio/vhost-user-fs.c
@@ -130,8 +130,9 @@ static uint64_t vuf_get_features(VirtIODevice *vdev,
                                       uint64_t requested_features,
                                       Error **errp)
 {
-    /* No feature bits used yet */
-    return requested_features;
+    VHostUserFS *fs = VHOST_USER_FS(vdev);
+
+    return vhost_get_default_features(&fs->vhost_dev, requested_features);
 }
 
 static void vuf_handle_output(VirtIODevice *vdev, VirtQueue *vq)
diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
index 4a228f5168..7276587be6 100644
--- a/hw/virtio/vhost-vsock.c
+++ b/hw/virtio/vhost-vsock.c
@@ -180,8 +180,9 @@ static uint64_t vhost_vsock_get_features(VirtIODevice *vdev,
                                          uint64_t requested_features,
                                          Error **errp)
 {
-    /* No feature bits used yet */
-    return requested_features;
+    VHostVSock *vsock = VHOST_VSOCK(vdev);
+
+    return vhost_get_default_features(&vsock->vhost_dev, requested_features);
 }
 
 static void vhost_vsock_handle_output(VirtIODevice *vdev, VirtQueue *vq)
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.
+ *
+ * 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.
+ */
+static const int vhost_default_feature_bits[] = {
+    VIRTIO_F_IOMMU_PLATFORM,
+    VIRTIO_F_RING_PACKED,
+    VHOST_INVALID_FEATURE_BIT
+};
+
 bool vhost_has_free_slot(void)
 {
     unsigned int slots_limit = ~0U;
@@ -1468,6 +1485,11 @@ uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
     return features;
 }
 
+uint64_t vhost_get_default_features(struct vhost_dev *hdev, uint64_t features)
+{
+    return vhost_get_features(hdev, vhost_default_feature_bits, features);
+}
+
 void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
                         uint64_t features)
 {
diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
index bd9165c565..ef711b56f4 100644
--- a/hw/virtio/virtio-crypto.c
+++ b/hw/virtio/virtio-crypto.c
@@ -739,7 +739,8 @@ static uint64_t virtio_crypto_get_features(VirtIODevice *vdev,
                                            uint64_t features,
                                            Error **errp)
 {
-    return features;
+    /* Just returns features when vhost is disabled */
+    return cryptodev_vhost_get_features(vdev, features);
 }
 
 static void virtio_crypto_reset(VirtIODevice *vdev)
-- 
2.25.3

Re: [PATCH 2/5] vhost: involve device backends in feature negotiation

[Marc-André Lureau]

Hi Stefan

On Fri, May 22, 2020 at 7:18 PM Stefan Hajnoczi <stefanha@redhat.com> 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 <stefanha@redhat.com>
> ---
>  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_virtqueue_mask(VirtIODevice *dev,
>                                             int queue,
>                                             int idx, bool mask)
> @@ -333,6 +347,11 @@ void cryptodev_vhost_stop(VirtIODevice *dev, int total_queues)
>  {
>  }
>
> +uint64_t cryptodev_vhost_get_features(VirtIODevice *dev, uint64_t features)
> +{
> +    return features;
> +}
> +
>  void cryptodev_vhost_virtqueue_mask(VirtIODevice *dev,
>                                      int queue,
>                                      int idx, bool mask)
> diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
> index 4cdaee1bde..e483df2a9e 100644
> --- a/hw/display/vhost-user-gpu.c
> +++ b/hw/display/vhost-user-gpu.c
> @@ -466,6 +466,22 @@ vhost_user_gpu_set_config(VirtIODevice *vdev,
>      }
>  }
>
> +static uint64_t
> +vhost_user_gpu_get_features(VirtIODevice *vdev, uint64_t features,
> +                            Error **errp)
> +{
> +    VhostUserGPU *g = VHOST_USER_GPU(vdev);
> +    Error *local_err = NULL;
> +
> +    features = virtio_gpu_base_get_features(vdev, features, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return 0;
> +    }
> +
> +    return vhost_get_default_features(&g->vhost->dev, features);
> +}
> +
>  static void
>  vhost_user_gpu_set_status(VirtIODevice *vdev, uint8_t val)
>  {
> @@ -582,6 +598,7 @@ vhost_user_gpu_class_init(ObjectClass *klass, void *data)
>
>      vdc->realize = vhost_user_gpu_device_realize;
>      vdc->reset = vhost_user_gpu_reset;
> +    vdc->get_features = vhost_user_gpu_get_features;
>      vdc->set_status   = vhost_user_gpu_set_status;
>      vdc->guest_notifier_mask = vhost_user_gpu_guest_notifier_mask;
>      vdc->guest_notifier_pending = vhost_user_gpu_guest_notifier_pending;
> diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
> index c159351be3..05d1ff2db2 100644
> --- a/hw/display/virtio-gpu-base.c
> +++ b/hw/display/virtio-gpu-base.c
> @@ -176,7 +176,7 @@ virtio_gpu_base_device_realize(DeviceState *qdev,
>      return true;
>  }
>
> -static uint64_t
> +uint64_t
>  virtio_gpu_base_get_features(VirtIODevice *vdev, uint64_t features,
>                               Error **errp)
>  {
> diff --git a/hw/input/vhost-user-input.c b/hw/input/vhost-user-input.c
> index 63984a8ba7..1371fb32cc 100644
> --- a/hw/input/vhost-user-input.c
> +++ b/hw/input/vhost-user-input.c
> @@ -45,6 +45,14 @@ static void vhost_input_change_active(VirtIOInput *vinput)
>      }
>  }
>
> +static uint64_t vhost_input_get_features(VirtIODevice *vdev, uint64_t features,
> +                                         Error **errp)
> +{
> +    VHostUserInput *vhi = VHOST_USER_INPUT(vdev);
> +
> +    return vhost_get_default_features(&vhi->vhost->dev, features);
> +}
> +
>  static void vhost_input_get_config(VirtIODevice *vdev, uint8_t *config_data)
>  {
>      VirtIOInput *vinput = VIRTIO_INPUT(vdev);
> @@ -89,6 +97,7 @@ static void vhost_input_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>
>      dc->vmsd = &vmstate_vhost_input;
> +    vdc->get_features = vhost_input_get_features;
>      vdc->get_config = vhost_input_get_config;
>      vdc->set_config = vhost_input_set_config;
>      vic->realize = vhost_input_realize;
> diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> index 1bc5d03a00..56015ca3d4 100644
> --- a/hw/virtio/vhost-user-fs.c
> +++ b/hw/virtio/vhost-user-fs.c
> @@ -130,8 +130,9 @@ static uint64_t vuf_get_features(VirtIODevice *vdev,
>                                        uint64_t requested_features,
>                                        Error **errp)
>  {
> -    /* No feature bits used yet */
> -    return requested_features;
> +    VHostUserFS *fs = VHOST_USER_FS(vdev);
> +
> +    return vhost_get_default_features(&fs->vhost_dev, requested_features);
>  }
>
>  static void vuf_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
> index 4a228f5168..7276587be6 100644
> --- a/hw/virtio/vhost-vsock.c
> +++ b/hw/virtio/vhost-vsock.c
> @@ -180,8 +180,9 @@ static uint64_t vhost_vsock_get_features(VirtIODevice *vdev,
>                                           uint64_t requested_features,
>                                           Error **errp)
>  {
> -    /* No feature bits used yet */
> -    return requested_features;
> +    VHostVSock *vsock = VHOST_VSOCK(vdev);
> +
> +    return vhost_get_default_features(&vsock->vhost_dev, requested_features);
>  }
>
>  static void vhost_vsock_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> 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.
> + *
> + * 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.
> + */
> +static const int vhost_default_feature_bits[] = {
> +    VIRTIO_F_IOMMU_PLATFORM,
> +    VIRTIO_F_RING_PACKED,

So effectively, we don't care about backend features except for those
2 bits, and an extra F_LOG_ALL check in vhost.c, right?

It's not entirely clear to me how the feature flags of device and
backend should combine tbh.

> +    VHOST_INVALID_FEATURE_BIT
> +};
> +
>  bool vhost_has_free_slot(void)
>  {
>      unsigned int slots_limit = ~0U;
> @@ -1468,6 +1485,11 @@ uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
>      return features;
>  }
>
> +uint64_t vhost_get_default_features(struct vhost_dev *hdev, uint64_t features)
> +{
> +    return vhost_get_features(hdev, vhost_default_feature_bits, features);
> +}
> +
>  void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
>                          uint64_t features)
>  {
> diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
> index bd9165c565..ef711b56f4 100644
> --- a/hw/virtio/virtio-crypto.c
> +++ b/hw/virtio/virtio-crypto.c
> @@ -739,7 +739,8 @@ static uint64_t virtio_crypto_get_features(VirtIODevice *vdev,
>                                             uint64_t features,
>                                             Error **errp)
>  {
> -    return features;
> +    /* Just returns features when vhost is disabled */
> +    return cryptodev_vhost_get_features(vdev, features);
>  }
>
>  static void virtio_crypto_reset(VirtIODevice *vdev)
> --
> 2.25.3
>

Re: [PATCH 2/5] vhost: involve device backends in feature negotiation

[Stefan Hajnoczi]

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

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 <stefanha@redhat.com> 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 <stefanha@redhat.com>
> > ---
> >  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_virtqueue_mask(VirtIODevice *dev,
> >                                             int queue,
> >                                             int idx, bool mask)
> > @@ -333,6 +347,11 @@ void cryptodev_vhost_stop(VirtIODevice *dev, int total_queues)
> >  {
> >  }
> >
> > +uint64_t cryptodev_vhost_get_features(VirtIODevice *dev, uint64_t features)
> > +{
> > +    return features;
> > +}
> > +
> >  void cryptodev_vhost_virtqueue_mask(VirtIODevice *dev,
> >                                      int queue,
> >                                      int idx, bool mask)
> > diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
> > index 4cdaee1bde..e483df2a9e 100644
> > --- a/hw/display/vhost-user-gpu.c
> > +++ b/hw/display/vhost-user-gpu.c
> > @@ -466,6 +466,22 @@ vhost_user_gpu_set_config(VirtIODevice *vdev,
> >      }
> >  }
> >
> > +static uint64_t
> > +vhost_user_gpu_get_features(VirtIODevice *vdev, uint64_t features,
> > +                            Error **errp)
> > +{
> > +    VhostUserGPU *g = VHOST_USER_GPU(vdev);
> > +    Error *local_err = NULL;
> > +
> > +    features = virtio_gpu_base_get_features(vdev, features, &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        return 0;
> > +    }
> > +
> > +    return vhost_get_default_features(&g->vhost->dev, features);
> > +}
> > +
> >  static void
> >  vhost_user_gpu_set_status(VirtIODevice *vdev, uint8_t val)
> >  {
> > @@ -582,6 +598,7 @@ vhost_user_gpu_class_init(ObjectClass *klass, void *data)
> >
> >      vdc->realize = vhost_user_gpu_device_realize;
> >      vdc->reset = vhost_user_gpu_reset;
> > +    vdc->get_features = vhost_user_gpu_get_features;
> >      vdc->set_status   = vhost_user_gpu_set_status;
> >      vdc->guest_notifier_mask = vhost_user_gpu_guest_notifier_mask;
> >      vdc->guest_notifier_pending = vhost_user_gpu_guest_notifier_pending;
> > diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
> > index c159351be3..05d1ff2db2 100644
> > --- a/hw/display/virtio-gpu-base.c
> > +++ b/hw/display/virtio-gpu-base.c
> > @@ -176,7 +176,7 @@ virtio_gpu_base_device_realize(DeviceState *qdev,
> >      return true;
> >  }
> >
> > -static uint64_t
> > +uint64_t
> >  virtio_gpu_base_get_features(VirtIODevice *vdev, uint64_t features,
> >                               Error **errp)
> >  {
> > diff --git a/hw/input/vhost-user-input.c b/hw/input/vhost-user-input.c
> > index 63984a8ba7..1371fb32cc 100644
> > --- a/hw/input/vhost-user-input.c
> > +++ b/hw/input/vhost-user-input.c
> > @@ -45,6 +45,14 @@ static void vhost_input_change_active(VirtIOInput *vinput)
> >      }
> >  }
> >
> > +static uint64_t vhost_input_get_features(VirtIODevice *vdev, uint64_t features,
> > +                                         Error **errp)
> > +{
> > +    VHostUserInput *vhi = VHOST_USER_INPUT(vdev);
> > +
> > +    return vhost_get_default_features(&vhi->vhost->dev, features);
> > +}
> > +
> >  static void vhost_input_get_config(VirtIODevice *vdev, uint8_t *config_data)
> >  {
> >      VirtIOInput *vinput = VIRTIO_INPUT(vdev);
> > @@ -89,6 +97,7 @@ static void vhost_input_class_init(ObjectClass *klass, void *data)
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> >
> >      dc->vmsd = &vmstate_vhost_input;
> > +    vdc->get_features = vhost_input_get_features;
> >      vdc->get_config = vhost_input_get_config;
> >      vdc->set_config = vhost_input_set_config;
> >      vic->realize = vhost_input_realize;
> > diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> > index 1bc5d03a00..56015ca3d4 100644
> > --- a/hw/virtio/vhost-user-fs.c
> > +++ b/hw/virtio/vhost-user-fs.c
> > @@ -130,8 +130,9 @@ static uint64_t vuf_get_features(VirtIODevice *vdev,
> >                                        uint64_t requested_features,
> >                                        Error **errp)
> >  {
> > -    /* No feature bits used yet */
> > -    return requested_features;
> > +    VHostUserFS *fs = VHOST_USER_FS(vdev);
> > +
> > +    return vhost_get_default_features(&fs->vhost_dev, requested_features);
> >  }
> >
> >  static void vuf_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> > diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
> > index 4a228f5168..7276587be6 100644
> > --- a/hw/virtio/vhost-vsock.c
> > +++ b/hw/virtio/vhost-vsock.c
> > @@ -180,8 +180,9 @@ static uint64_t vhost_vsock_get_features(VirtIODevice *vdev,
> >                                           uint64_t requested_features,
> >                                           Error **errp)
> >  {
> > -    /* No feature bits used yet */
> > -    return requested_features;
> > +    VHostVSock *vsock = VHOST_VSOCK(vdev);
> > +
> > +    return vhost_get_default_features(&vsock->vhost_dev, requested_features);
> >  }
> >
> >  static void vhost_vsock_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> > 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.
> > + *
> > + * 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.
> > + */
> > +static const int vhost_default_feature_bits[] = {
> > +    VIRTIO_F_IOMMU_PLATFORM,
> > +    VIRTIO_F_RING_PACKED,
> 
> So effectively, we don't care about backend features except for those
> 2 bits, and an extra F_LOG_ALL check in vhost.c, right?
> 
> It's not entirely clear to me how the feature flags of device and
> backend should combine tbh.

I've always found this confusing too. Maybe a vhost-user spec
clarification would help improve the situation at least for future
vhost-user backend implementors.

libvhost-user only advertises VHOST_F_LOG_ALL and
VHOST_USER_F_PROTOCOL_FEATURES. Everything else is left up to the device
backend implementation. This is a problem because libvhost-user handles
virtqueues and needs to be aware of VIRTIO_RING_F_EVENT_IDX, etc. I'll
send a separate patch to advertise the necessary feature bits in
libvhost-user.

QEMU calculates the VIRTIO device's feature bit masks as follows:

1. -device vhost-foo initializes the device features with whatever QEMU
   supports. Either the QEMU vhost-foo code itself sets features or the
   user overrides them on the command-line.

2. Some vhost devices in QEMU call vhost_get_features() to mask out
   features missing from the VHOST_USER_GET_FEATURES response. This is
   how the vhost-user backend participates in feature negotiation.

This patch series changes moves us one step in the direction of always
involving the vhost-user backend in feature negotiation. It's only a
step because to do so fully would break existing vhost-user backends
that don't advertise features properly :(.

Stefan

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

Re: [PATCH 2/5] vhost: involve device backends in feature negotiation

[Jason Wang]

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 <stefanha@redhat.com>
> ---
>   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_virtqueue_mask(VirtIODevice *dev,
>                                              int queue,
>                                              int idx, bool mask)
> @@ -333,6 +347,11 @@ void cryptodev_vhost_stop(VirtIODevice *dev, int total_queues)
>   {
>   }
>   
> +uint64_t cryptodev_vhost_get_features(VirtIODevice *dev, uint64_t features)
> +{
> +    return features;
> +}
> +
>   void cryptodev_vhost_virtqueue_mask(VirtIODevice *dev,
>                                       int queue,
>                                       int idx, bool mask)
> diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
> index 4cdaee1bde..e483df2a9e 100644
> --- a/hw/display/vhost-user-gpu.c
> +++ b/hw/display/vhost-user-gpu.c
> @@ -466,6 +466,22 @@ vhost_user_gpu_set_config(VirtIODevice *vdev,
>       }
>   }
>   
> +static uint64_t
> +vhost_user_gpu_get_features(VirtIODevice *vdev, uint64_t features,
> +                            Error **errp)
> +{
> +    VhostUserGPU *g = VHOST_USER_GPU(vdev);
> +    Error *local_err = NULL;
> +
> +    features = virtio_gpu_base_get_features(vdev, features, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return 0;
> +    }
> +
> +    return vhost_get_default_features(&g->vhost->dev, features);
> +}
> +
>   static void
>   vhost_user_gpu_set_status(VirtIODevice *vdev, uint8_t val)
>   {
> @@ -582,6 +598,7 @@ vhost_user_gpu_class_init(ObjectClass *klass, void *data)
>   
>       vdc->realize = vhost_user_gpu_device_realize;
>       vdc->reset = vhost_user_gpu_reset;
> +    vdc->get_features = vhost_user_gpu_get_features;
>       vdc->set_status   = vhost_user_gpu_set_status;
>       vdc->guest_notifier_mask = vhost_user_gpu_guest_notifier_mask;
>       vdc->guest_notifier_pending = vhost_user_gpu_guest_notifier_pending;
> diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
> index c159351be3..05d1ff2db2 100644
> --- a/hw/display/virtio-gpu-base.c
> +++ b/hw/display/virtio-gpu-base.c
> @@ -176,7 +176,7 @@ virtio_gpu_base_device_realize(DeviceState *qdev,
>       return true;
>   }
>   
> -static uint64_t
> +uint64_t
>   virtio_gpu_base_get_features(VirtIODevice *vdev, uint64_t features,
>                                Error **errp)
>   {
> diff --git a/hw/input/vhost-user-input.c b/hw/input/vhost-user-input.c
> index 63984a8ba7..1371fb32cc 100644
> --- a/hw/input/vhost-user-input.c
> +++ b/hw/input/vhost-user-input.c
> @@ -45,6 +45,14 @@ static void vhost_input_change_active(VirtIOInput *vinput)
>       }
>   }
>   
> +static uint64_t vhost_input_get_features(VirtIODevice *vdev, uint64_t features,
> +                                         Error **errp)
> +{
> +    VHostUserInput *vhi = VHOST_USER_INPUT(vdev);
> +
> +    return vhost_get_default_features(&vhi->vhost->dev, features);
> +}
> +
>   static void vhost_input_get_config(VirtIODevice *vdev, uint8_t *config_data)
>   {
>       VirtIOInput *vinput = VIRTIO_INPUT(vdev);
> @@ -89,6 +97,7 @@ static void vhost_input_class_init(ObjectClass *klass, void *data)
>       DeviceClass *dc = DEVICE_CLASS(klass);
>   
>       dc->vmsd = &vmstate_vhost_input;
> +    vdc->get_features = vhost_input_get_features;
>       vdc->get_config = vhost_input_get_config;
>       vdc->set_config = vhost_input_set_config;
>       vic->realize = vhost_input_realize;
> diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> index 1bc5d03a00..56015ca3d4 100644
> --- a/hw/virtio/vhost-user-fs.c
> +++ b/hw/virtio/vhost-user-fs.c
> @@ -130,8 +130,9 @@ static uint64_t vuf_get_features(VirtIODevice *vdev,
>                                         uint64_t requested_features,
>                                         Error **errp)
>   {
> -    /* No feature bits used yet */
> -    return requested_features;
> +    VHostUserFS *fs = VHOST_USER_FS(vdev);
> +
> +    return vhost_get_default_features(&fs->vhost_dev, requested_features);
>   }
>   
>   static void vuf_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
> index 4a228f5168..7276587be6 100644
> --- a/hw/virtio/vhost-vsock.c
> +++ b/hw/virtio/vhost-vsock.c
> @@ -180,8 +180,9 @@ static uint64_t vhost_vsock_get_features(VirtIODevice *vdev,
>                                            uint64_t requested_features,
>                                            Error **errp)
>   {
> -    /* No feature bits used yet */
> -    return requested_features;
> +    VHostVSock *vsock = VHOST_VSOCK(vdev);
> +
> +    return vhost_get_default_features(&vsock->vhost_dev, requested_features);
>   }
>   
>   static void vhost_vsock_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> 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.


> + *
> + * 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.


> + */
> +static const int vhost_default_feature_bits[] = {
> +    VIRTIO_F_IOMMU_PLATFORM,
> +    VIRTIO_F_RING_PACKED,
> +    VHOST_INVALID_FEATURE_BIT
> +};
> +
>   bool vhost_has_free_slot(void)
>   {
>       unsigned int slots_limit = ~0U;
> @@ -1468,6 +1485,11 @@ uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
>       return features;
>   }
>   
> +uint64_t vhost_get_default_features(struct vhost_dev *hdev, uint64_t features)
> +{
> +    return vhost_get_features(hdev, vhost_default_feature_bits, features);
> +}


There's probably no need for a new helper, we can do all these inside 
vhost_get_features().

Thanks


> +
>   void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
>                           uint64_t features)
>   {
> diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
> index bd9165c565..ef711b56f4 100644
> --- a/hw/virtio/virtio-crypto.c
> +++ b/hw/virtio/virtio-crypto.c
> @@ -739,7 +739,8 @@ static uint64_t virtio_crypto_get_features(VirtIODevice *vdev,
>                                              uint64_t features,
>                                              Error **errp)
>   {
> -    return features;
> +    /* Just returns features when vhost is disabled */
> +    return cryptodev_vhost_get_features(vdev, features);
>   }
>   
>   static void virtio_crypto_reset(VirtIODevice *vdev)

Re: [PATCH 2/5] vhost: involve device backends in feature negotiation

[Stefan Hajnoczi]

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

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_bits[] = {
> > +    VIRTIO_F_IOMMU_PLATFORM,
> > +    VIRTIO_F_RING_PACKED,
> > +    VHOST_INVALID_FEATURE_BIT
> > +};
> > +
> >   bool vhost_has_free_slot(void)
> >   {
> >       unsigned int slots_limit = ~0U;
> > @@ -1468,6 +1485,11 @@ uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
> >       return features;
> >   }
> > +uint64_t vhost_get_default_features(struct vhost_dev *hdev, uint64_t features)
> > +{
> > +    return vhost_get_features(hdev, vhost_default_feature_bits, features);
> > +}
> 
> 
> There's probably no need for a new helper, we can do all these inside
> vhost_get_features().

Would you prefer:

  extern const int vhost_default_feature_bits[];

And then callers do:

  vhost_get_features(hdev, vhost_default_feature_bits, features);

?

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

Re: [PATCH 2/5] vhost: involve device backends in feature negotiation

[Dr. David Alan Gilbert]

* Stefan Hajnoczi (stefanha@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 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_bits[] = {
> > > +    VIRTIO_F_IOMMU_PLATFORM,
> > > +    VIRTIO_F_RING_PACKED,
> > > +    VHOST_INVALID_FEATURE_BIT
> > > +};
> > > +
> > >   bool vhost_has_free_slot(void)
> > >   {
> > >       unsigned int slots_limit = ~0U;
> > > @@ -1468,6 +1485,11 @@ uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
> > >       return features;
> > >   }
> > > +uint64_t vhost_get_default_features(struct vhost_dev *hdev, uint64_t features)
> > > +{
> > > +    return vhost_get_features(hdev, vhost_default_feature_bits, features);
> > > +}
> > 
> > 
> > There's probably no need for a new helper, we can do all these inside
> > vhost_get_features().
> 
> Would you prefer:
> 
>   extern const int vhost_default_feature_bits[];
> 
> And then callers do:
> 
>   vhost_get_features(hdev, vhost_default_feature_bits, features);
> 
> ?


--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 2/5] vhost: involve device backends in feature negotiation

[Stefan Hajnoczi]

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

On Fri, May 29, 2020 at 03:29:13PM +0100, Dr. David Alan Gilbert wrote:
> * Stefan Hajnoczi (stefanha@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

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

Re: [PATCH 2/5] vhost: involve device backends in feature negotiation

[Jason Wang]

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 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_bits[] = {
>>> +    VIRTIO_F_IOMMU_PLATFORM,
>>> +    VIRTIO_F_RING_PACKED,
>>> +    VHOST_INVALID_FEATURE_BIT
>>> +};
>>> +
>>>    bool vhost_has_free_slot(void)
>>>    {
>>>        unsigned int slots_limit = ~0U;
>>> @@ -1468,6 +1485,11 @@ uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
>>>        return features;
>>>    }
>>> +uint64_t vhost_get_default_features(struct vhost_dev *hdev, uint64_t features)
>>> +{
>>> +    return vhost_get_features(hdev, vhost_default_feature_bits, features);
>>> +}
>>
>> There's probably no need for a new helper, we can do all these inside
>> vhost_get_features().
> Would you prefer:
>
>    extern const int vhost_default_feature_bits[];
>
> And then callers do:
>
>    vhost_get_features(hdev, vhost_default_feature_bits, features);
>
> ?


That's fine or maybe just do features |= vhost_default_feature_bits 
inside vhost_get_features().

Thanks

Re: [PATCH 2/5] vhost: involve device backends in feature negotiation

[Stefan Hajnoczi]

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

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 existing backends.
> > 
> > Would you like me to rephrase this comment in some way?
> 
> 
> That will be better.

Will fix.

> 
> 
> > 
> > > > + *
> > > > + * 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_bits[] = {
> > > > +    VIRTIO_F_IOMMU_PLATFORM,
> > > > +    VIRTIO_F_RING_PACKED,
> > > > +    VHOST_INVALID_FEATURE_BIT
> > > > +};
> > > > +
> > > >    bool vhost_has_free_slot(void)
> > > >    {
> > > >        unsigned int slots_limit = ~0U;
> > > > @@ -1468,6 +1485,11 @@ uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
> > > >        return features;
> > > >    }
> > > > +uint64_t vhost_get_default_features(struct vhost_dev *hdev, uint64_t features)
> > > > +{
> > > > +    return vhost_get_features(hdev, vhost_default_feature_bits, features);
> > > > +}
> > > 
> > > There's probably no need for a new helper, we can do all these inside
> > > vhost_get_features().
> > Would you prefer:
> > 
> >    extern const int vhost_default_feature_bits[];
> > 
> > And then callers do:
> > 
> >    vhost_get_features(hdev, vhost_default_feature_bits, features);
> > 
> > ?
> 
> 
> That's fine or maybe just do features |= vhost_default_feature_bits inside
> vhost_get_features().

Will fix.

Stefan

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

[PATCH 3/5] vhost-user-blk: add VIRTIO_F_RING_PACKED feature bit

[Stefan Hajnoczi]

Vhost devices have a list of feature bits that the device backend is
allowed to control. The VIRTIO_F_RING_PACKED feature is a feature that
must be negotiated through all the way to the device backend. Add it so
the device backend can declare whether or not it supports the packed
ring layout.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/block/vhost-user-blk.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 9d8c0b3909..10e114a19a 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -44,6 +44,7 @@ static const int user_feature_bits[] = {
     VIRTIO_BLK_F_DISCARD,
     VIRTIO_BLK_F_WRITE_ZEROES,
     VIRTIO_F_VERSION_1,
+    VIRTIO_F_RING_PACKED,
     VIRTIO_RING_F_INDIRECT_DESC,
     VIRTIO_RING_F_EVENT_IDX,
     VIRTIO_F_NOTIFY_ON_EMPTY,
-- 
2.25.3

Re: [PATCH 3/5] vhost-user-blk: add VIRTIO_F_RING_PACKED feature bit

[Raphael Norwitz]

On Fri, May 22, 2020 at 1:20 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> Vhost devices have a list of feature bits that the device backend is
> allowed to control. The VIRTIO_F_RING_PACKED feature is a feature that
> must be negotiated through all the way to the device backend. Add it so
> the device backend can declare whether or not it supports the packed
> ring layout.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>

> ---
>  hw/block/vhost-user-blk.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 9d8c0b3909..10e114a19a 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -44,6 +44,7 @@ static const int user_feature_bits[] = {
>      VIRTIO_BLK_F_DISCARD,
>      VIRTIO_BLK_F_WRITE_ZEROES,
>      VIRTIO_F_VERSION_1,
> +    VIRTIO_F_RING_PACKED,
>      VIRTIO_RING_F_INDIRECT_DESC,
>      VIRTIO_RING_F_EVENT_IDX,
>      VIRTIO_F_NOTIFY_ON_EMPTY,
> --
> 2.25.3
>

[PATCH 4/5] vhost-scsi: add VIRTIO_F_VERSION_1 and VIRTIO_F_RING_PACKED

[Stefan Hajnoczi]

Let vhost-scsi and vhost-user-scsi device backends determine whether
VIRTIO 1.0 and packed virtqueues are supported. It doesn't make sense to
handle these feature bits in QEMU since the device backend needs to
support them if we want to use them.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/scsi/vhost-scsi.c      | 2 ++
 hw/scsi/vhost-user-scsi.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index c1b012aea4..a7fb788af5 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -34,6 +34,8 @@
 
 /* Features supported by host kernel. */
 static const int kernel_feature_bits[] = {
+    VIRTIO_F_VERSION_1,
+    VIRTIO_F_RING_PACKED,
     VIRTIO_F_NOTIFY_ON_EMPTY,
     VIRTIO_RING_F_INDIRECT_DESC,
     VIRTIO_RING_F_EVENT_IDX,
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index cbb5d97599..6aa0d5ded2 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -32,6 +32,8 @@
 
 /* Features supported by the host application */
 static const int user_feature_bits[] = {
+    VIRTIO_F_VERSION_1,
+    VIRTIO_F_RING_PACKED,
     VIRTIO_F_NOTIFY_ON_EMPTY,
     VIRTIO_RING_F_INDIRECT_DESC,
     VIRTIO_RING_F_EVENT_IDX,
-- 
2.25.3

Re: [PATCH 4/5] vhost-scsi: add VIRTIO_F_VERSION_1 and VIRTIO_F_RING_PACKED

[Raphael Norwitz]

On Fri, May 22, 2020 at 1:19 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> Let vhost-scsi and vhost-user-scsi device backends determine whether
> VIRTIO 1.0 and packed virtqueues are supported. It doesn't make sense to
> handle these feature bits in QEMU since the device backend needs to
> support them if we want to use them.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>

> ---
>  hw/scsi/vhost-scsi.c      | 2 ++
>  hw/scsi/vhost-user-scsi.c | 2 ++
>  2 files changed, 4 insertions(+)
>
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index c1b012aea4..a7fb788af5 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -34,6 +34,8 @@
>
>  /* Features supported by host kernel. */
>  static const int kernel_feature_bits[] = {
> +    VIRTIO_F_VERSION_1,
> +    VIRTIO_F_RING_PACKED,
>      VIRTIO_F_NOTIFY_ON_EMPTY,
>      VIRTIO_RING_F_INDIRECT_DESC,
>      VIRTIO_RING_F_EVENT_IDX,
> diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
> index cbb5d97599..6aa0d5ded2 100644
> --- a/hw/scsi/vhost-user-scsi.c
> +++ b/hw/scsi/vhost-user-scsi.c
> @@ -32,6 +32,8 @@
>
>  /* Features supported by the host application */
>  static const int user_feature_bits[] = {
> +    VIRTIO_F_VERSION_1,
> +    VIRTIO_F_RING_PACKED,
>      VIRTIO_F_NOTIFY_ON_EMPTY,
>      VIRTIO_RING_F_INDIRECT_DESC,
>      VIRTIO_RING_F_EVENT_IDX,
> --
> 2.25.3
>

[PATCH 5/5] virtio: enable VIRTIO_F_RING_PACKED for all devices

[Stefan Hajnoczi]

The packed virtqueue layout was introduced in VIRTIO 1.1. It is a single
ring instead of a split avail/used ring design. There are CPU cache
advantages to this layout and it is also suited better to hardware
implementation.

The vhost-net backend has already supported packed virtqueues for some
time. Performance benchmarks show that virtio-blk performance on NVMe
drives is also improved.

Go ahead and enable this feature for all VIRTIO devices. Keep it
disabled for QEMU 5.0 and earlier machine types.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/hw/virtio/virtio.h |  2 +-
 hw/core/machine.c          | 18 +++++++++++++++++-
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index b69d517496..fd5b4a2044 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -292,7 +292,7 @@ typedef struct VirtIORNGConf VirtIORNGConf;
     DEFINE_PROP_BIT64("iommu_platform", _state, _field, \
                       VIRTIO_F_IOMMU_PLATFORM, false), \
     DEFINE_PROP_BIT64("packed", _state, _field, \
-                      VIRTIO_F_RING_PACKED, false)
+                      VIRTIO_F_RING_PACKED, true)
 
 hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
 bool virtio_queue_enabled(VirtIODevice *vdev, int n);
diff --git a/hw/core/machine.c b/hw/core/machine.c
index bb3a7b18b1..3598c3c825 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -28,7 +28,23 @@
 #include "hw/mem/nvdimm.h"
 #include "migration/vmstate.h"
 
-GlobalProperty hw_compat_5_0[] = {};
+GlobalProperty hw_compat_5_0[] = {
+    { "vhost-user-blk", "packed", "off" },
+    { "vhost-user-fs-device", "packed", "off" },
+    { "vhost-vsock-device", "packed", "off" },
+    { "virtio-9p-device", "packed", "off" },
+    { "virtio-balloon-device", "packed", "off" },
+    { "virtio-blk-device", "packed", "off" },
+    { "virtio-crypto-device", "packed", "off" },
+    { "virtio-gpu-device", "packed", "off" },
+    { "virtio-input-device", "packed", "off" },
+    { "virtio-iommu-device", "packed", "off" },
+    { "virtio-net-device", "packed", "off" },
+    { "virtio-pmem", "packed", "off" },
+    { "virtio-rng-device", "packed", "off" },
+    { "virtio-scsi-common", "packed", "off" },
+    { "virtio-serial-device", "packed", "off" },
+};
 const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
 
 GlobalProperty hw_compat_4_2[] = {
-- 
2.25.3

Re: [PATCH 5/5] virtio: enable VIRTIO_F_RING_PACKED for all devices

[Dr. David Alan Gilbert]

* Stefan Hajnoczi (stefanha@redhat.com) wrote:
> The packed virtqueue layout was introduced in VIRTIO 1.1. It is a single
> ring instead of a split avail/used ring design. There are CPU cache
> advantages to this layout and it is also suited better to hardware
> implementation.
> 
> The vhost-net backend has already supported packed virtqueues for some
> time. Performance benchmarks show that virtio-blk performance on NVMe
> drives is also improved.
> 
> Go ahead and enable this feature for all VIRTIO devices. Keep it
> disabled for QEMU 5.0 and earlier machine types.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  include/hw/virtio/virtio.h |  2 +-
>  hw/core/machine.c          | 18 +++++++++++++++++-
>  2 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index b69d517496..fd5b4a2044 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -292,7 +292,7 @@ typedef struct VirtIORNGConf VirtIORNGConf;
>      DEFINE_PROP_BIT64("iommu_platform", _state, _field, \
>                        VIRTIO_F_IOMMU_PLATFORM, false), \
>      DEFINE_PROP_BIT64("packed", _state, _field, \
> -                      VIRTIO_F_RING_PACKED, false)
> +                      VIRTIO_F_RING_PACKED, true)
>  
>  hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
>  bool virtio_queue_enabled(VirtIODevice *vdev, int n);
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index bb3a7b18b1..3598c3c825 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -28,7 +28,23 @@
>  #include "hw/mem/nvdimm.h"
>  #include "migration/vmstate.h"
>  
> -GlobalProperty hw_compat_5_0[] = {};
> +GlobalProperty hw_compat_5_0[] = {
> +    { "vhost-user-blk", "packed", "off" },
> +    { "vhost-user-fs-device", "packed", "off" },
> +    { "vhost-vsock-device", "packed", "off" },
> +    { "virtio-9p-device", "packed", "off" },
> +    { "virtio-balloon-device", "packed", "off" },
> +    { "virtio-blk-device", "packed", "off" },
> +    { "virtio-crypto-device", "packed", "off" },
> +    { "virtio-gpu-device", "packed", "off" },
> +    { "virtio-input-device", "packed", "off" },
> +    { "virtio-iommu-device", "packed", "off" },
> +    { "virtio-net-device", "packed", "off" },
> +    { "virtio-pmem", "packed", "off" },
> +    { "virtio-rng-device", "packed", "off" },
> +    { "virtio-scsi-common", "packed", "off" },
> +    { "virtio-serial-device", "packed", "off" },
> +};
>  const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
>  
>  GlobalProperty hw_compat_4_2[] = {
> -- 
> 2.25.3
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 5/5] virtio: enable VIRTIO_F_RING_PACKED for all devices

[Jason Wang]

On 2020/5/23 上午1:17, Stefan Hajnoczi wrote:
> The packed virtqueue layout was introduced in VIRTIO 1.1. It is a single
> ring instead of a split avail/used ring design. There are CPU cache
> advantages to this layout and it is also suited better to hardware
> implementation.
>
> The vhost-net backend has already supported packed virtqueues for some
> time. Performance benchmarks show that virtio-blk performance on NVMe
> drives is also improved.
>
> Go ahead and enable this feature for all VIRTIO devices. Keep it
> disabled for QEMU 5.0 and earlier machine types.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   include/hw/virtio/virtio.h |  2 +-
>   hw/core/machine.c          | 18 +++++++++++++++++-
>   2 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index b69d517496..fd5b4a2044 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -292,7 +292,7 @@ typedef struct VirtIORNGConf VirtIORNGConf;
>       DEFINE_PROP_BIT64("iommu_platform", _state, _field, \
>                         VIRTIO_F_IOMMU_PLATFORM, false), \
>       DEFINE_PROP_BIT64("packed", _state, _field, \
> -                      VIRTIO_F_RING_PACKED, false)
> +                      VIRTIO_F_RING_PACKED, true)
>   
>   hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
>   bool virtio_queue_enabled(VirtIODevice *vdev, int n);
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index bb3a7b18b1..3598c3c825 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -28,7 +28,23 @@
>   #include "hw/mem/nvdimm.h"
>   #include "migration/vmstate.h"
>   
> -GlobalProperty hw_compat_5_0[] = {};
> +GlobalProperty hw_compat_5_0[] = {
> +    { "vhost-user-blk", "packed", "off" },
> +    { "vhost-user-fs-device", "packed", "off" },
> +    { "vhost-vsock-device", "packed", "off" },
> +    { "virtio-9p-device", "packed", "off" },
> +    { "virtio-balloon-device", "packed", "off" },
> +    { "virtio-blk-device", "packed", "off" },
> +    { "virtio-crypto-device", "packed", "off" },
> +    { "virtio-gpu-device", "packed", "off" },
> +    { "virtio-input-device", "packed", "off" },
> +    { "virtio-iommu-device", "packed", "off" },
> +    { "virtio-net-device", "packed", "off" },
> +    { "virtio-pmem", "packed", "off" },
> +    { "virtio-rng-device", "packed", "off" },
> +    { "virtio-scsi-common", "packed", "off" },
> +    { "virtio-serial-device", "packed", "off" },


Missing "vhost-user-gpu" here?

I try to do something like this in the past but give up since I end up 
with similar list.

It would be better to consider something more smart, probably need some 
refactor for a common parent class.

Thanks


> +};
>   const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
>   
>   GlobalProperty hw_compat_4_2[] = {

Re: [PATCH 5/5] virtio: enable VIRTIO_F_RING_PACKED for all devices

[Stefan Hajnoczi]

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

On Fri, May 29, 2020 at 03:15:59PM +0800, Jason Wang wrote:
> 
> On 2020/5/23 上午1:17, Stefan Hajnoczi wrote:
> > The packed virtqueue layout was introduced in VIRTIO 1.1. It is a single
> > ring instead of a split avail/used ring design. There are CPU cache
> > advantages to this layout and it is also suited better to hardware
> > implementation.
> > 
> > The vhost-net backend has already supported packed virtqueues for some
> > time. Performance benchmarks show that virtio-blk performance on NVMe
> > drives is also improved.
> > 
> > Go ahead and enable this feature for all VIRTIO devices. Keep it
> > disabled for QEMU 5.0 and earlier machine types.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >   include/hw/virtio/virtio.h |  2 +-
> >   hw/core/machine.c          | 18 +++++++++++++++++-
> >   2 files changed, 18 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > index b69d517496..fd5b4a2044 100644
> > --- a/include/hw/virtio/virtio.h
> > +++ b/include/hw/virtio/virtio.h
> > @@ -292,7 +292,7 @@ typedef struct VirtIORNGConf VirtIORNGConf;
> >       DEFINE_PROP_BIT64("iommu_platform", _state, _field, \
> >                         VIRTIO_F_IOMMU_PLATFORM, false), \
> >       DEFINE_PROP_BIT64("packed", _state, _field, \
> > -                      VIRTIO_F_RING_PACKED, false)
> > +                      VIRTIO_F_RING_PACKED, true)
> >   hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
> >   bool virtio_queue_enabled(VirtIODevice *vdev, int n);
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index bb3a7b18b1..3598c3c825 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -28,7 +28,23 @@
> >   #include "hw/mem/nvdimm.h"
> >   #include "migration/vmstate.h"
> > -GlobalProperty hw_compat_5_0[] = {};
> > +GlobalProperty hw_compat_5_0[] = {
> > +    { "vhost-user-blk", "packed", "off" },
> > +    { "vhost-user-fs-device", "packed", "off" },
> > +    { "vhost-vsock-device", "packed", "off" },
> > +    { "virtio-9p-device", "packed", "off" },
> > +    { "virtio-balloon-device", "packed", "off" },
> > +    { "virtio-blk-device", "packed", "off" },
> > +    { "virtio-crypto-device", "packed", "off" },
> > +    { "virtio-gpu-device", "packed", "off" },
> > +    { "virtio-input-device", "packed", "off" },
> > +    { "virtio-iommu-device", "packed", "off" },
> > +    { "virtio-net-device", "packed", "off" },
> > +    { "virtio-pmem", "packed", "off" },
> > +    { "virtio-rng-device", "packed", "off" },
> > +    { "virtio-scsi-common", "packed", "off" },
> > +    { "virtio-serial-device", "packed", "off" },
> 
> 
> Missing "vhost-user-gpu" here?

Thanks, you're right.

I'll see if virtio-gpu-base works. If not it will be necessary to add
all the derived classes. The same is true for virtio-scsi-common, I'd
better check it works correctly!

Stefan

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