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

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

[Stefan Hajnoczi]

v2:
 * Add libvhost-user VHOST_USER_GET_FEATURES patch to start reporting feature
   bits correctly (already reviewed by Marc-André Lureau)
 * Rephrase doc comments and drop vhost_get_default_features() [Jason]
 * Simplify hw/core/machine.c compat props by setting packed=off on the
   virtio-dev base class [Jason]

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.

Patch 1 fixes libqos. Patch 2 fixes libvhost-user. Patch 3 adds clarifications
to the vhost-user specification. Patches 4-6 solve the vhost feature issues in
QEMU. Finally, Patch 6 enables packed virtqueues.

Stefan Hajnoczi (7):
  tests/libqos: mask out VIRTIO_F_RING_PACKED for now
  libvhost-user: advertise vring features
  docs: document non-net VHOST_USER_GET_FEATURES behavior
  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              | 21 +++++++++++++++++++++
 contrib/libvhost-user/libvhost-user.c   | 10 ++++++++++
 contrib/vhost-user-blk/vhost-user-blk.c |  4 +---
 hw/block/vhost-user-blk.c               |  1 +
 hw/core/machine.c                       |  4 +++-
 hw/display/vhost-user-gpu.c             | 18 ++++++++++++++++++
 hw/display/virtio-gpu-base.c            |  2 +-
 hw/input/vhost-user-input.c             | 11 +++++++++++
 hw/scsi/vhost-scsi.c                    |  2 ++
 hw/scsi/vhost-user-scsi.c               |  2 ++
 hw/virtio/vhost-user-fs.c               |  6 ++++--
 hw/virtio/vhost-vsock.c                 |  7 +++++--
 hw/virtio/vhost.c                       | 24 ++++++++++++++++++++++++
 hw/virtio/virtio-crypto.c               |  3 ++-
 tests/qtest/libqos/virtio.c             |  3 ++-
 docs/interop/vhost-user.rst             | 21 +++++++++++++++++++++
 20 files changed, 143 insertions(+), 12 deletions(-)

-- 
2.26.2

[PATCH v2 1/7] 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.

Acked-by: 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.26.2

[PATCH v2 2/7] libvhost-user: advertise vring features

[Stefan Hajnoczi]

libvhost-user implements several vring features without advertising
them. There is no way for the vhost-user master to detect support for
these features.

Things more or less work today because QEMU assumes the vhost-user
backend always implements certain feature bits like
VIRTIO_RING_F_EVENT_IDX. This is not documented anywhere.

This patch explicitly advertises features implemented in libvhost-user
so that the vhost-user master does not need to make undocumented
assumptions.

Feature bits that libvhost-user now advertises can be removed from
vhost-user-blk.c. Devices should not be responsible for advertising
vring feature bits, that is libvhost-user's job.

Cc: Jason Wang <jasowang@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
I have tested make check and virtiofsd.
---
 contrib/libvhost-user/libvhost-user.c   | 10 ++++++++++
 contrib/vhost-user-blk/vhost-user-blk.c |  4 +---
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c
index 3bca996c62..b43874ba12 100644
--- a/contrib/libvhost-user/libvhost-user.c
+++ b/contrib/libvhost-user/libvhost-user.c
@@ -495,6 +495,16 @@ static bool
 vu_get_features_exec(VuDev *dev, VhostUserMsg *vmsg)
 {
     vmsg->payload.u64 =
+        /*
+         * The following VIRTIO feature bits are supported by our virtqueue
+         * implementation:
+         */
+        1ULL << VIRTIO_F_NOTIFY_ON_EMPTY |
+        1ULL << VIRTIO_RING_F_INDIRECT_DESC |
+        1ULL << VIRTIO_RING_F_EVENT_IDX |
+        1ULL << VIRTIO_F_VERSION_1 |
+
+        /* vhost-user feature bits */
         1ULL << VHOST_F_LOG_ALL |
         1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
 
diff --git a/contrib/vhost-user-blk/vhost-user-blk.c b/contrib/vhost-user-blk/vhost-user-blk.c
index 6fd91c7e99..25eccd02b5 100644
--- a/contrib/vhost-user-blk/vhost-user-blk.c
+++ b/contrib/vhost-user-blk/vhost-user-blk.c
@@ -382,9 +382,7 @@ vub_get_features(VuDev *dev)
                1ull << VIRTIO_BLK_F_DISCARD |
                1ull << VIRTIO_BLK_F_WRITE_ZEROES |
                #endif
-               1ull << VIRTIO_BLK_F_CONFIG_WCE |
-               1ull << VIRTIO_F_VERSION_1 |
-               1ull << VHOST_USER_F_PROTOCOL_FEATURES;
+               1ull << VIRTIO_BLK_F_CONFIG_WCE;
 
     if (vdev_blk->enable_ro) {
         features |= 1ull << VIRTIO_BLK_F_RO;
-- 
2.26.2

[PATCH v2 3/7] docs: document non-net VHOST_USER_GET_FEATURES behavior

[Stefan Hajnoczi]

QEMU enabled several feature bits for non-net devices without allowing
the device backend to control them. This only works when the device
backend implements support for those features. It won't work for new
features like the packed virtqueue layout, where proper feature
negotiation will be needed.

Document the legacy behavior and specify that device backends must
report features so that we can avoid problems in the future.

Cc: Ben Walker <benjamin.walker@intel.com>
Cc: Sebastien Boeuf <sebastien.boeuf@intel.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
CCing SPDK and cloud-hypervisor folks in case they are affected. DPDK
isn't affected since vhost-user-net performs full feature negotiation.
---
 docs/interop/vhost-user.rst | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
index 3b1b6602c7..dfadee411d 100644
--- a/docs/interop/vhost-user.rst
+++ b/docs/interop/vhost-user.rst
@@ -290,6 +290,27 @@ bit was dedicated for this purpose::
 
   #define VHOST_USER_F_PROTOCOL_FEATURES 30
 
+Feature negotiation
+-------------------
+The master fetches features from the backend using the
+``VHOST_USER_GET_FEATURES`` message. The feature bits correspond to those from
+the virtio specification, VHOST_F_LOG_ALL (26), and
+``VHOST_USER_F_PROTOCOL_FEATURES`` (30).
+
+Backends must report all supported feature bits. If a feature bit is set then
+the master may set it in the ``VHOST_USER_SET_FEATURES`` message. If a feature
+bit is cleared then the master must not set it in the
+``VHOST_USER_SET_FEATURES`` message.
+
+For devices other than the networking device, masters may assume the following
+feature bits are always set in ``VHOST_USER_GET_FEATURES`` for compatibility
+with legacy backend implementations that do not report them correctly:
+* ``VIRTIO_F_RING_INDIRECT_DESC``
+* ``VIRTIO_F_RING_EVENT_IDX``
+* ``VIRTIO_F_VERSION_1``
+* ``VIRTIO_F_NOTIFY_ON_EMPTY``
+* ``VIRTIO_F_ANY_LAYOUT``
+
 Starting and stopping rings
 ---------------------------
 
-- 
2.26.2

[PATCH v2 4/7] 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 when no
device-specific feature bits are defined (virtio-net has many but some
devices have none).

Unfortunately this causes problems when QEMU's virtqueue implementation
and the device backend's implementation support different features.
QEMU must not report features to the guest that aren't supported by the
device backend.

For example, QEMU supports VIRTIO 1.1 packed virtqueues while many
existing vhost device backends do not. The device backend breaks when
the user sets packed=on. This should have been handled gracefully by
feature negotiation instead of resulting in a cryptic failure when the
device backend cannot parse the vring.

Introduce the vhost_old_default_feature_bits[] array so existing
devices can involve the device backend in feature negotiation.
libvhost-user does not report VIRTIO_RING_F_INDIRECT_DESC and other core
feature bits even though it implements them. Therefore
vhost_old_default_feature_bits[] only includes feature bits that can be
explicitly negotiated without breaking existing libvhost-user device
backends.

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       | 21 +++++++++++++++++++++
 hw/display/vhost-user-gpu.c      | 18 ++++++++++++++++++
 hw/display/virtio-gpu-base.c     |  2 +-
 hw/input/vhost-user-input.c      | 11 +++++++++++
 hw/virtio/vhost-user-fs.c        |  6 ++++--
 hw/virtio/vhost-vsock.c          |  7 +++++--
 hw/virtio/vhost.c                | 24 ++++++++++++++++++++++++
 hw/virtio/virtio-crypto.c        |  3 ++-
 11 files changed, 100 insertions(+), 6 deletions(-)

diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 085450c6f8..4cd278a395 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -110,6 +110,7 @@ bool vhost_virtqueue_pending(struct vhost_dev *hdev, int n);
  */
 void vhost_virtqueue_mask(struct vhost_dev *hdev, VirtIODevice *vdev, int n,
                           bool mask);
+extern const int vhost_old_default_feature_bits[];
 uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
                             uint64_t features);
 void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
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..945004c536 100644
--- a/backends/cryptodev-vhost.c
+++ b/backends/cryptodev-vhost.c
@@ -266,6 +266,22 @@ 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_features(&vhost_crypto->dev,
+                              vhost_old_default_feature_bits,
+                              features);
+}
+
 void cryptodev_vhost_virtqueue_mask(VirtIODevice *dev,
                                            int queue,
                                            int idx, bool mask)
@@ -333,6 +349,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..eeb000bcbe 100644
--- a/hw/display/vhost-user-gpu.c
+++ b/hw/display/vhost-user-gpu.c
@@ -466,6 +466,23 @@ 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_features(&g->vhost->dev, vhost_old_default_feature_bits,
+                              features);
+}
+
 static void
 vhost_user_gpu_set_status(VirtIODevice *vdev, uint8_t val)
 {
@@ -582,6 +599,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..c9ec675c49 100644
--- a/hw/input/vhost-user-input.c
+++ b/hw/input/vhost-user-input.c
@@ -45,6 +45,16 @@ 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_features(&vhi->vhost->dev,
+                              vhost_old_default_feature_bits,
+                              features);
+}
+
 static void vhost_input_get_config(VirtIODevice *vdev, uint8_t *config_data)
 {
     VirtIOInput *vinput = VIRTIO_INPUT(vdev);
@@ -89,6 +99,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..9ca9f9d3d8 100644
--- a/hw/virtio/vhost-user-fs.c
+++ b/hw/virtio/vhost-user-fs.c
@@ -130,8 +130,10 @@ 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_features(&fs->vhost_dev, vhost_old_default_feature_bits,
+                              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..b96b03cb06 100644
--- a/hw/virtio/vhost-vsock.c
+++ b/hw/virtio/vhost-vsock.c
@@ -180,8 +180,11 @@ 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_features(&vsock->vhost_dev,
+                              vhost_old_default_feature_bits,
+                              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..56d671852b 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1454,6 +1454,30 @@ void vhost_virtqueue_mask(struct vhost_dev *hdev, VirtIODevice *vdev, int n,
     }
 }
 
+/*
+ * Default vhost_get_features() feature bits for existing device types that do
+ * not define their own.
+ *
+ * This is a workaround for existing device types, do not use this in new vhost
+ * device types. Explicitly define a list of feature bits instead.
+ *
+ * The following feature bits are excluded because libvhost-user device
+ * backends did not advertise them for a long time. Therefore we cannot detect
+ * their presence. Instead we assume they are always supported by the device
+ * backend:
+ * VIRTIO_F_NOTIFY_ON_EMPTY
+ * VIRTIO_F_ANY_LAYOUT
+ * VIRTIO_F_VERSION_1
+ * VIRTIO_RING_F_INDIRECT_DESC
+ * VIRTIO_RING_F_EVENT_IDX
+ */
+const int vhost_old_default_feature_bits[] = {
+    VIRTIO_F_IOMMU_PLATFORM,
+    VIRTIO_F_RING_PACKED,
+    VIRTIO_F_ORDER_PLATFORM,
+    VHOST_INVALID_FEATURE_BIT
+};
+
 uint64_t vhost_get_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.26.2

[PATCH v2 5/7] 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>
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.26.2

[PATCH v2 6/7] 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>
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.26.2

[PATCH v2 7/7] 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          | 4 +++-
 2 files changed, 4 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..a9bf76f318 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -28,7 +28,9 @@
 #include "hw/mem/nvdimm.h"
 #include "migration/vmstate.h"
 
-GlobalProperty hw_compat_5_0[] = {};
+GlobalProperty hw_compat_5_0[] = {
+    { "virtio-device", "packed", "off" },
+};
 const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
 
 GlobalProperty hw_compat_4_2[] = {
-- 
2.26.2

Re: [PATCH v2 4/7] vhost: involve device backends in feature negotiation

[Michael S. Tsirkin]

On Tue, Jun 09, 2020 at 06:02:15PM +0100, Stefan Hajnoczi wrote:
> Many vhost devices in QEMU currently do not involve the device backend
> in feature negotiation. This seems fine at first glance when no
> device-specific feature bits are defined (virtio-net has many but some
> devices have none).
> 
> Unfortunately this causes problems when QEMU's virtqueue implementation
> and the device backend's implementation support different features.
> QEMU must not report features to the guest that aren't supported by the
> device backend.
> 
> For example, QEMU supports VIRTIO 1.1 packed virtqueues while many
> existing vhost device backends do not. The device backend breaks when
> the user sets packed=on. This should have been handled gracefully by
> feature negotiation instead of resulting in a cryptic failure when the
> device backend cannot parse the vring.
> 
> Introduce the vhost_old_default_feature_bits[] array so existing
> devices can involve the device backend in feature negotiation.
> libvhost-user does not report VIRTIO_RING_F_INDIRECT_DESC and other core
> feature bits even though it implements them. Therefore
> vhost_old_default_feature_bits[] only includes feature bits that can be
> explicitly negotiated without breaking existing libvhost-user device
> backends.

libvhost-user is in contrib. Can't we just fix it as appropriate?
Work arounds have their cost ..


> 
> 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       | 21 +++++++++++++++++++++
>  hw/display/vhost-user-gpu.c      | 18 ++++++++++++++++++
>  hw/display/virtio-gpu-base.c     |  2 +-
>  hw/input/vhost-user-input.c      | 11 +++++++++++
>  hw/virtio/vhost-user-fs.c        |  6 ++++--
>  hw/virtio/vhost-vsock.c          |  7 +++++--
>  hw/virtio/vhost.c                | 24 ++++++++++++++++++++++++
>  hw/virtio/virtio-crypto.c        |  3 ++-
>  11 files changed, 100 insertions(+), 6 deletions(-)

Hmm what about network devices?

I also wonder whether there's a way to just make everyone use
the features that we already request at startup
instead of slowing startup down by re-reading features.


> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index 085450c6f8..4cd278a395 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -110,6 +110,7 @@ bool vhost_virtqueue_pending(struct vhost_dev *hdev, int n);
>   */
>  void vhost_virtqueue_mask(struct vhost_dev *hdev, VirtIODevice *vdev, int n,
>                            bool mask);
> +extern const int vhost_old_default_feature_bits[];
>  uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
>                              uint64_t features);
>  void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
> 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..945004c536 100644
> --- a/backends/cryptodev-vhost.c
> +++ b/backends/cryptodev-vhost.c
> @@ -266,6 +266,22 @@ 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_features(&vhost_crypto->dev,
> +                              vhost_old_default_feature_bits,
> +                              features);
> +}
> +
>  void cryptodev_vhost_virtqueue_mask(VirtIODevice *dev,
>                                             int queue,
>                                             int idx, bool mask)
> @@ -333,6 +349,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..eeb000bcbe 100644
> --- a/hw/display/vhost-user-gpu.c
> +++ b/hw/display/vhost-user-gpu.c
> @@ -466,6 +466,23 @@ 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_features(&g->vhost->dev, vhost_old_default_feature_bits,
> +                              features);
> +}
> +
>  static void
>  vhost_user_gpu_set_status(VirtIODevice *vdev, uint8_t val)
>  {
> @@ -582,6 +599,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..c9ec675c49 100644
> --- a/hw/input/vhost-user-input.c
> +++ b/hw/input/vhost-user-input.c
> @@ -45,6 +45,16 @@ 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_features(&vhi->vhost->dev,
> +                              vhost_old_default_feature_bits,
> +                              features);
> +}
> +
>  static void vhost_input_get_config(VirtIODevice *vdev, uint8_t *config_data)
>  {
>      VirtIOInput *vinput = VIRTIO_INPUT(vdev);
> @@ -89,6 +99,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..9ca9f9d3d8 100644
> --- a/hw/virtio/vhost-user-fs.c
> +++ b/hw/virtio/vhost-user-fs.c
> @@ -130,8 +130,10 @@ 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_features(&fs->vhost_dev, vhost_old_default_feature_bits,
> +                              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..b96b03cb06 100644
> --- a/hw/virtio/vhost-vsock.c
> +++ b/hw/virtio/vhost-vsock.c
> @@ -180,8 +180,11 @@ 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_features(&vsock->vhost_dev,
> +                              vhost_old_default_feature_bits,
> +                              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..56d671852b 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1454,6 +1454,30 @@ void vhost_virtqueue_mask(struct vhost_dev *hdev, VirtIODevice *vdev, int n,
>      }
>  }
>  
> +/*
> + * Default vhost_get_features() feature bits for existing device types that do
> + * not define their own.
> + *
> + * This is a workaround for existing device types, do not use this in new vhost
> + * device types. Explicitly define a list of feature bits instead.
> + *
> + * The following feature bits are excluded because libvhost-user device
> + * backends did not advertise them for a long time. Therefore we cannot detect
> + * their presence. Instead we assume they are always supported by the device
> + * backend:
> + * VIRTIO_F_NOTIFY_ON_EMPTY
> + * VIRTIO_F_ANY_LAYOUT
> + * VIRTIO_F_VERSION_1
> + * VIRTIO_RING_F_INDIRECT_DESC
> + * VIRTIO_RING_F_EVENT_IDX

Weird. I remember that it's common for vhost-user not to set
VIRTIO_RING_F_INDIRECT_DESC - they have huge queues so
don't need it and inline descriptors give them better
performance.

So what's going on here?

> + */
> +const int vhost_old_default_feature_bits[] = {

Hmm. Not a great name here - going forward it will be pretty hard to
know which versions are old and which are new.  And assuming new clients
are fixed, don't we want to detect that and skip work-arounds?


> +    VIRTIO_F_IOMMU_PLATFORM,
> +    VIRTIO_F_RING_PACKED,
> +    VIRTIO_F_ORDER_PLATFORM,
> +    VHOST_INVALID_FEATURE_BIT
> +};
> +
>  uint64_t vhost_get_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.26.2
> 

Re: [PATCH v2 4/7] vhost: involve device backends in feature negotiation

[Jason Wang]

On 2020/6/10 上午2:07, Michael S. Tsirkin wrote:
>> +/*
>> + * Default vhost_get_features() feature bits for existing device types that do
>> + * not define their own.
>> + *
>> + * This is a workaround for existing device types, do not use this in new vhost
>> + * device types. Explicitly define a list of feature bits instead.
>> + *
>> + * The following feature bits are excluded because libvhost-user device
>> + * backends did not advertise them for a long time. Therefore we cannot detect
>> + * their presence. Instead we assume they are always supported by the device
>> + * backend:
>> + * VIRTIO_F_NOTIFY_ON_EMPTY
>> + * VIRTIO_F_ANY_LAYOUT
>> + * VIRTIO_F_VERSION_1
>> + * VIRTIO_RING_F_INDIRECT_DESC
>> + * VIRTIO_RING_F_EVENT_IDX
> Weird. I remember that it's common for vhost-user not to set
> VIRTIO_RING_F_INDIRECT_DESC - they have huge queues so
> don't need it and inline descriptors give them better
> performance.
>
> So what's going on here?


I guess one reason is to support live migration between vhost-user and 
vhost-net.

Thanks

Re: [PATCH v2 4/7] vhost: involve device backends in feature negotiation

[Michael S. Tsirkin]

On Wed, Jun 10, 2020 at 11:21:50AM +0800, Jason Wang wrote:
> 
> On 2020/6/10 上午2:07, Michael S. Tsirkin wrote:
> > > +/*
> > > + * Default vhost_get_features() feature bits for existing device types that do
> > > + * not define their own.
> > > + *
> > > + * This is a workaround for existing device types, do not use this in new vhost
> > > + * device types. Explicitly define a list of feature bits instead.
> > > + *
> > > + * The following feature bits are excluded because libvhost-user device
> > > + * backends did not advertise them for a long time. Therefore we cannot detect
> > > + * their presence. Instead we assume they are always supported by the device
> > > + * backend:
> > > + * VIRTIO_F_NOTIFY_ON_EMPTY
> > > + * VIRTIO_F_ANY_LAYOUT
> > > + * VIRTIO_F_VERSION_1
> > > + * VIRTIO_RING_F_INDIRECT_DESC
> > > + * VIRTIO_RING_F_EVENT_IDX
> > Weird. I remember that it's common for vhost-user not to set
> > VIRTIO_RING_F_INDIRECT_DESC - they have huge queues so
> > don't need it and inline descriptors give them better
> > performance.
> > 
> > So what's going on here?
> 
> 
> I guess one reason is to support live migration between vhost-user and
> vhost-net.
> 
> Thanks
> 

But how can we force-enable features backend doesn't want to enable?
This may or may not break backends ...
I would rather just be strict and ask backends to fix their feature
bits. See user_feature_bits in hw/net/vhost-net.c which supports
all these features.

-- 
MST

Re: [PATCH v2 3/7] docs: document non-net VHOST_USER_GET_FEATURES behavior

[Michael S. Tsirkin]

On Tue, Jun 09, 2020 at 06:02:14PM +0100, Stefan Hajnoczi wrote:
> QEMU enabled several feature bits for non-net devices without allowing
> the device backend to control them. This only works when the device
> backend implements support for those features. It won't work for new
> features like the packed virtqueue layout, where proper feature
> negotiation will be needed.
> 
> Document the legacy behavior and specify that device backends must
> report features so that we can avoid problems in the future.
> 
> Cc: Ben Walker <benjamin.walker@intel.com>
> Cc: Sebastien Boeuf <sebastien.boeuf@intel.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> CCing SPDK and cloud-hypervisor folks in case they are affected. DPDK
> isn't affected since vhost-user-net performs full feature negotiation.
> ---
>  docs/interop/vhost-user.rst | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
> index 3b1b6602c7..dfadee411d 100644
> --- a/docs/interop/vhost-user.rst
> +++ b/docs/interop/vhost-user.rst
> @@ -290,6 +290,27 @@ bit was dedicated for this purpose::
>  
>    #define VHOST_USER_F_PROTOCOL_FEATURES 30
>  
> +Feature negotiation
> +-------------------
> +The master fetches features from the backend using the
> +``VHOST_USER_GET_FEATURES`` message. The feature bits correspond to those from
> +the virtio specification, VHOST_F_LOG_ALL (26), and
> +``VHOST_USER_F_PROTOCOL_FEATURES`` (30).

Seems to partially duplicate the description of the message
VHOST_USER_GET_FEATURES, except that is missing
description of VHOST_F_LOG_ALL. How about tweaking that
instead of adding more text? BTW description of VHOST_USER_GET_FEATURES
has some typos worth fixing ...




> +Backends must report all supported feature bits. If a feature bit is set then
> +the master may set it in the ``VHOST_USER_SET_FEATURES`` message. If a feature
> +bit is cleared then the master must not set it in the
> +``VHOST_USER_SET_FEATURES`` message.

Again let's extend description of VHOST_USER_SET_FEATURES if that's
unclear. BTW description of VHOST_USER_SET_FEATURES has
some typos worth fixing ...

> +
> +For devices other than the networking device, masters may assume the following
> +feature bits are always set in ``VHOST_USER_GET_FEATURES`` for compatibility
> +with legacy backend implementations that do not report them correctly:
> +* ``VIRTIO_F_RING_INDIRECT_DESC``
> +* ``VIRTIO_F_RING_EVENT_IDX``
> +* ``VIRTIO_F_VERSION_1``
> +* ``VIRTIO_F_NOTIFY_ON_EMPTY``
> +* ``VIRTIO_F_ANY_LAYOUT``
> +
>  Starting and stopping rings
>  ---------------------------

How common are these backends? Anything shipped for a while?  IIUC we
are not talking about years of history here, so I really think we should
just enforce what spec always said, rather than work around some broken
clients.

> -- 
> 2.26.2
> 

Re: [PATCH v2 0/7] virtio: enable VIRTIO_F_RING_PACKED for all devices

[Michael S. Tsirkin]

On Tue, Jun 09, 2020 at 06:02:11PM +0100, Stefan Hajnoczi wrote:
> v2:
>  * Add libvhost-user VHOST_USER_GET_FEATURES patch to start reporting feature
>    bits correctly (already reviewed by Marc-André Lureau)
>  * Rephrase doc comments and drop vhost_get_default_features() [Jason]
>  * Simplify hw/core/machine.c compat props by setting packed=off on the
>    virtio-dev base class [Jason]
> 
> 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's exciting that it's widely useful!
Could you include some numbers please though?
No need to be exhaustive ...

> 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.
> 
> Patch 1 fixes libqos. Patch 2 fixes libvhost-user. Patch 3 adds clarifications
> to the vhost-user specification. Patches 4-6 solve the vhost feature issues in
> QEMU. Finally, Patch 6 enables packed virtqueues.
> 
> Stefan Hajnoczi (7):
>   tests/libqos: mask out VIRTIO_F_RING_PACKED for now
>   libvhost-user: advertise vring features
>   docs: document non-net VHOST_USER_GET_FEATURES behavior
>   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              | 21 +++++++++++++++++++++
>  contrib/libvhost-user/libvhost-user.c   | 10 ++++++++++
>  contrib/vhost-user-blk/vhost-user-blk.c |  4 +---
>  hw/block/vhost-user-blk.c               |  1 +
>  hw/core/machine.c                       |  4 +++-
>  hw/display/vhost-user-gpu.c             | 18 ++++++++++++++++++
>  hw/display/virtio-gpu-base.c            |  2 +-
>  hw/input/vhost-user-input.c             | 11 +++++++++++
>  hw/scsi/vhost-scsi.c                    |  2 ++
>  hw/scsi/vhost-user-scsi.c               |  2 ++
>  hw/virtio/vhost-user-fs.c               |  6 ++++--
>  hw/virtio/vhost-vsock.c                 |  7 +++++--
>  hw/virtio/vhost.c                       | 24 ++++++++++++++++++++++++
>  hw/virtio/virtio-crypto.c               |  3 ++-
>  tests/qtest/libqos/virtio.c             |  3 ++-
>  docs/interop/vhost-user.rst             | 21 +++++++++++++++++++++
>  20 files changed, 143 insertions(+), 12 deletions(-)
> 
> -- 
> 2.26.2
> 

Re: [PATCH v2 4/7] vhost: involve device backends in feature negotiation

[Jason Wang]

On 2020/6/10 下午12:15, Michael S. Tsirkin wrote:
> On Wed, Jun 10, 2020 at 11:21:50AM +0800, Jason Wang wrote:
>> On 2020/6/10 上午2:07, Michael S. Tsirkin wrote:
>>>> +/*
>>>> + * Default vhost_get_features() feature bits for existing device types that do
>>>> + * not define their own.
>>>> + *
>>>> + * This is a workaround for existing device types, do not use this in new vhost
>>>> + * device types. Explicitly define a list of feature bits instead.
>>>> + *
>>>> + * The following feature bits are excluded because libvhost-user device
>>>> + * backends did not advertise them for a long time. Therefore we cannot detect
>>>> + * their presence. Instead we assume they are always supported by the device
>>>> + * backend:
>>>> + * VIRTIO_F_NOTIFY_ON_EMPTY
>>>> + * VIRTIO_F_ANY_LAYOUT
>>>> + * VIRTIO_F_VERSION_1
>>>> + * VIRTIO_RING_F_INDIRECT_DESC
>>>> + * VIRTIO_RING_F_EVENT_IDX
>>> Weird. I remember that it's common for vhost-user not to set
>>> VIRTIO_RING_F_INDIRECT_DESC - they have huge queues so
>>> don't need it and inline descriptors give them better
>>> performance.
>>>
>>> So what's going on here?
>>
>> I guess one reason is to support live migration between vhost-user and
>> vhost-net.
>>
>> Thanks
>>
> But how can we force-enable features backend doesn't want to enable?


We can't and the code just forces qemu to validate 
VIRTIO_RING_F_INDIRECT_DESC for each vhost backends instead of assuming 
the support silently.

Thanks


> This may or may not break backends ...
> I would rather just be strict and ask backends to fix their feature
> bits. See user_feature_bits in hw/net/vhost-net.c which supports
> all these features.
>

Re: [PATCH v2 4/7] vhost: involve device backends in feature negotiation

[Michael S. Tsirkin]

On Wed, Jun 10, 2020 at 01:53:57PM +0800, Jason Wang wrote:
> 
> On 2020/6/10 下午12:15, Michael S. Tsirkin wrote:
> > On Wed, Jun 10, 2020 at 11:21:50AM +0800, Jason Wang wrote:
> > > On 2020/6/10 上午2:07, Michael S. Tsirkin wrote:
> > > > > +/*
> > > > > + * Default vhost_get_features() feature bits for existing device types that do
> > > > > + * not define their own.
> > > > > + *
> > > > > + * This is a workaround for existing device types, do not use this in new vhost
> > > > > + * device types. Explicitly define a list of feature bits instead.
> > > > > + *
> > > > > + * The following feature bits are excluded because libvhost-user device
> > > > > + * backends did not advertise them for a long time. Therefore we cannot detect
> > > > > + * their presence. Instead we assume they are always supported by the device
> > > > > + * backend:
> > > > > + * VIRTIO_F_NOTIFY_ON_EMPTY
> > > > > + * VIRTIO_F_ANY_LAYOUT
> > > > > + * VIRTIO_F_VERSION_1
> > > > > + * VIRTIO_RING_F_INDIRECT_DESC
> > > > > + * VIRTIO_RING_F_EVENT_IDX
> > > > Weird. I remember that it's common for vhost-user not to set
> > > > VIRTIO_RING_F_INDIRECT_DESC - they have huge queues so
> > > > don't need it and inline descriptors give them better
> > > > performance.
> > > > 
> > > > So what's going on here?
> > > 
> > > I guess one reason is to support live migration between vhost-user and
> > > vhost-net.
> > > 
> > > Thanks
> > > 
> > But how can we force-enable features backend doesn't want to enable?
> 
> 
> We can't and the code just forces qemu to validate
> VIRTIO_RING_F_INDIRECT_DESC for each vhost backends instead of assuming the
> support silently.
> 
> Thanks

So why does the comment above say:

     Instead we assume they are always supported by the device backend




> 
> > This may or may not break backends ...
> > I would rather just be strict and ask backends to fix their feature
> > bits. See user_feature_bits in hw/net/vhost-net.c which supports
> > all these features.
> > 

Re: [PATCH v2 4/7] vhost: involve device backends in feature negotiation

[Michael S. Tsirkin]

On Tue, Jun 09, 2020 at 06:02:15PM +0100, Stefan Hajnoczi wrote:
> Many vhost devices in QEMU currently do not involve the device backend
> in feature negotiation. This seems fine at first glance when no
> device-specific feature bits are defined (virtio-net has many but some
> devices have none).
> 
> Unfortunately this causes problems when QEMU's virtqueue implementation
> and the device backend's implementation support different features.
> QEMU must not report features to the guest that aren't supported by the
> device backend.
> 
> For example, QEMU supports VIRTIO 1.1 packed virtqueues while many
> existing vhost device backends do not. The device backend breaks when
> the user sets packed=on. This should have been handled gracefully by
> feature negotiation instead of resulting in a cryptic failure when the
> device backend cannot parse the vring.
> 
> Introduce the vhost_old_default_feature_bits[] array so existing
> devices can involve the device backend in feature negotiation.
> libvhost-user does not report VIRTIO_RING_F_INDIRECT_DESC and other core
> feature bits even though it implements them. Therefore
> vhost_old_default_feature_bits[] only includes feature bits that can be
> explicitly negotiated without breaking existing libvhost-user device
> backends.
> 
> 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       | 21 +++++++++++++++++++++
>  hw/display/vhost-user-gpu.c      | 18 ++++++++++++++++++
>  hw/display/virtio-gpu-base.c     |  2 +-
>  hw/input/vhost-user-input.c      | 11 +++++++++++
>  hw/virtio/vhost-user-fs.c        |  6 ++++--
>  hw/virtio/vhost-vsock.c          |  7 +++++--
>  hw/virtio/vhost.c                | 24 ++++++++++++++++++++++++
>  hw/virtio/virtio-crypto.c        |  3 ++-
>  11 files changed, 100 insertions(+), 6 deletions(-)
> 
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index 085450c6f8..4cd278a395 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -110,6 +110,7 @@ bool vhost_virtqueue_pending(struct vhost_dev *hdev, int n);
>   */
>  void vhost_virtqueue_mask(struct vhost_dev *hdev, VirtIODevice *vdev, int n,
>                            bool mask);
> +extern const int vhost_old_default_feature_bits[];
>  uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
>                              uint64_t features);
>  void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
> 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..945004c536 100644
> --- a/backends/cryptodev-vhost.c
> +++ b/backends/cryptodev-vhost.c
> @@ -266,6 +266,22 @@ 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_features(&vhost_crypto->dev,
> +                              vhost_old_default_feature_bits,
> +                              features);
> +}
> +
>  void cryptodev_vhost_virtqueue_mask(VirtIODevice *dev,
>                                             int queue,
>                                             int idx, bool mask)
> @@ -333,6 +349,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..eeb000bcbe 100644
> --- a/hw/display/vhost-user-gpu.c
> +++ b/hw/display/vhost-user-gpu.c
> @@ -466,6 +466,23 @@ 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_features(&g->vhost->dev, vhost_old_default_feature_bits,
> +                              features);
> +}
> +
>  static void
>  vhost_user_gpu_set_status(VirtIODevice *vdev, uint8_t val)
>  {
> @@ -582,6 +599,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..c9ec675c49 100644
> --- a/hw/input/vhost-user-input.c
> +++ b/hw/input/vhost-user-input.c
> @@ -45,6 +45,16 @@ 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_features(&vhi->vhost->dev,
> +                              vhost_old_default_feature_bits,
> +                              features);
> +}
> +
>  static void vhost_input_get_config(VirtIODevice *vdev, uint8_t *config_data)
>  {
>      VirtIOInput *vinput = VIRTIO_INPUT(vdev);
> @@ -89,6 +99,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..9ca9f9d3d8 100644
> --- a/hw/virtio/vhost-user-fs.c
> +++ b/hw/virtio/vhost-user-fs.c
> @@ -130,8 +130,10 @@ 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_features(&fs->vhost_dev, vhost_old_default_feature_bits,
> +                              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..b96b03cb06 100644
> --- a/hw/virtio/vhost-vsock.c
> +++ b/hw/virtio/vhost-vsock.c
> @@ -180,8 +180,11 @@ 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_features(&vsock->vhost_dev,
> +                              vhost_old_default_feature_bits,
> +                              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..56d671852b 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1454,6 +1454,30 @@ void vhost_virtqueue_mask(struct vhost_dev *hdev, VirtIODevice *vdev, int n,
>      }
>  }
>  
> +/*
> + * Default vhost_get_features() feature bits for existing device types that do
> + * not define their own.
> + *
> + * This is a workaround for existing device types, do not use this in new vhost
> + * device types. Explicitly define a list of feature bits instead.
> + *
> + * The following feature bits are excluded because libvhost-user device
> + * backends did not advertise them for a long time. Therefore we cannot detect
> + * their presence. Instead we assume they are always supported by the device
> + * backend:
> + * VIRTIO_F_NOTIFY_ON_EMPTY
> + * VIRTIO_F_ANY_LAYOUT
> + * VIRTIO_F_VERSION_1
> + * VIRTIO_RING_F_INDIRECT_DESC
> + * VIRTIO_RING_F_EVENT_IDX


So the affected devices are all exclusively modern.

Some points:
- VIRTIO_F_NOTIFY_ON_EMPTY and VIRTIO_F_ANY_LAYOUT are not supposed to
  be exposed by modern devices at all. Do we even set them? If we do
  we might need to be careful with migration compat as features
  are guest visible.
- VIRTIO_F_VERSION_1 must be set by modern devices. Probably OK to just
  force it on.

This leaves just two:
 VIRTIO_RING_F_INDIRECT_DESC
 VIRTIO_RING_F_EVENT_IDX

I guess we could (ab)use VIRTIO_F_VERSION_1 to detect a broken client
and ignore these two.

-- 
MST

Re: [PATCH v2 4/7] vhost: involve device backends in feature negotiation

[Jason Wang]

On 2020/6/10 下午2:11, Michael S. Tsirkin wrote:
> On Wed, Jun 10, 2020 at 01:53:57PM +0800, Jason Wang wrote:
>> On 2020/6/10 下午12:15, Michael S. Tsirkin wrote:
>>> On Wed, Jun 10, 2020 at 11:21:50AM +0800, Jason Wang wrote:
>>>> On 2020/6/10 上午2:07, Michael S. Tsirkin wrote:
>>>>>> +/*
>>>>>> + * Default vhost_get_features() feature bits for existing device types that do
>>>>>> + * not define their own.
>>>>>> + *
>>>>>> + * This is a workaround for existing device types, do not use this in new vhost
>>>>>> + * device types. Explicitly define a list of feature bits instead.
>>>>>> + *
>>>>>> + * The following feature bits are excluded because libvhost-user device
>>>>>> + * backends did not advertise them for a long time. Therefore we cannot detect
>>>>>> + * their presence. Instead we assume they are always supported by the device
>>>>>> + * backend:
>>>>>> + * VIRTIO_F_NOTIFY_ON_EMPTY
>>>>>> + * VIRTIO_F_ANY_LAYOUT
>>>>>> + * VIRTIO_F_VERSION_1
>>>>>> + * VIRTIO_RING_F_INDIRECT_DESC
>>>>>> + * VIRTIO_RING_F_EVENT_IDX
>>>>> Weird. I remember that it's common for vhost-user not to set
>>>>> VIRTIO_RING_F_INDIRECT_DESC - they have huge queues so
>>>>> don't need it and inline descriptors give them better
>>>>> performance.
>>>>>
>>>>> So what's going on here?
>>>> I guess one reason is to support live migration between vhost-user and
>>>> vhost-net.
>>>>
>>>> Thanks
>>>>
>>> But how can we force-enable features backend doesn't want to enable?
>>
>> We can't and the code just forces qemu to validate
>> VIRTIO_RING_F_INDIRECT_DESC for each vhost backends instead of assuming the
>> support silently.
>>
>> Thanks
> So why does the comment above say:
>
>       Instead we assume they are always supported by the device backend


Sorry for being unclear. I meant, for any new type of vhost devices, 
they should advertise features as what spec said and qemu must validate 
VIRTIO_RING_F_INDIRECT_DESC.

Thanks


>
>
>
>
>>> This may or may not break backends ...
>>> I would rather just be strict and ask backends to fix their feature
>>> bits. See user_feature_bits in hw/net/vhost-net.c which supports
>>> all these features.
>>>

Re: [PATCH v2 3/7] docs: document non-net VHOST_USER_GET_FEATURES behavior

[Stefan Hajnoczi]

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

On Wed, Jun 10, 2020 at 01:01:26AM -0400, Michael S. Tsirkin wrote:
> On Tue, Jun 09, 2020 at 06:02:14PM +0100, Stefan Hajnoczi wrote:
> > +For devices other than the networking device, masters may assume the following
> > +feature bits are always set in ``VHOST_USER_GET_FEATURES`` for compatibility
> > +with legacy backend implementations that do not report them correctly:
> > +* ``VIRTIO_F_RING_INDIRECT_DESC``
> > +* ``VIRTIO_F_RING_EVENT_IDX``
> > +* ``VIRTIO_F_VERSION_1``
> > +* ``VIRTIO_F_NOTIFY_ON_EMPTY``
> > +* ``VIRTIO_F_ANY_LAYOUT``
> > +
> >  Starting and stopping rings
> >  ---------------------------
> 
> How common are these backends? Anything shipped for a while?  IIUC we
> are not talking about years of history here, so I really think we should
> just enforce what spec always said, rather than work around some broken
> clients.

QEMU didn't include some device backends in feature negotiation. This is
why we didn't notice that libvhost-user does not advertise some feature
bits. Therefore the chance of third-party device backends missing
feature negotiation seems high too.

I will move this section into the VHOST_USER_GET_FEATURES description.

Stefan

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

Re: [PATCH v2 4/7] vhost: involve device backends in feature negotiation

[Stefan Hajnoczi]

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

On Tue, Jun 09, 2020 at 02:07:44PM -0400, Michael S. Tsirkin wrote:
> On Tue, Jun 09, 2020 at 06:02:15PM +0100, Stefan Hajnoczi wrote:
> > Many vhost devices in QEMU currently do not involve the device backend
> > in feature negotiation. This seems fine at first glance when no
> > device-specific feature bits are defined (virtio-net has many but some
> > devices have none).
> > 
> > Unfortunately this causes problems when QEMU's virtqueue implementation
> > and the device backend's implementation support different features.
> > QEMU must not report features to the guest that aren't supported by the
> > device backend.
> > 
> > For example, QEMU supports VIRTIO 1.1 packed virtqueues while many
> > existing vhost device backends do not. The device backend breaks when
> > the user sets packed=on. This should have been handled gracefully by
> > feature negotiation instead of resulting in a cryptic failure when the
> > device backend cannot parse the vring.
> > 
> > Introduce the vhost_old_default_feature_bits[] array so existing
> > devices can involve the device backend in feature negotiation.
> > libvhost-user does not report VIRTIO_RING_F_INDIRECT_DESC and other core
> > feature bits even though it implements them. Therefore
> > vhost_old_default_feature_bits[] only includes feature bits that can be
> > explicitly negotiated without breaking existing libvhost-user device
> > backends.
> 
> libvhost-user is in contrib. Can't we just fix it as appropriate?
> Work arounds have their cost ..

libvhost-user's behavior is unfortunate but the bigger problem is that
QEMU does not include backends in feature negotiation. Existing backends
for devices touched in this patch may have a broken
VHOST_USER_GET_FEATURES implementations and changing QEMU's behavior
will break them.

If you feel confident that third-party vhost-user backends will work,
then we can simplify this patch series. I think we should be very
careful but it's up to you.

Stefan

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