virtio: introduce support for GSO over UDP tunnel

The set_offload() argument list is already pretty long and we are going to introduce soon a bunch of additional offloads.

Replace the offload arguments with a single struct and update all the relevant call-sites.

No functional changes intended.

Signed-off-by Paolo Abeni <[email protected]>

---
Note: I maintained the struct usage as opposed to uint64_t bitmask usage as suggested by Akihiko, because the latter feel a bit more invasive.

v1 -> v2:
- drop unneeded 'struct' keywords
- moved to series start

---
  hw/net/e1000e_core.c |  5 +++--
  hw/net/igb_core.c    |  5 +++--
  hw/net/virtio-net.c  | 19 +++++++++++--------
  hw/net/vmxnet3.c     | 13 +++++--------
  include/net/net.h    | 15 ++++++++++++---
  net/net.c            |  5 ++---
  net/netmap.c         |  3 +--
  net/tap-bsd.c        |  3 +--
  net/tap-linux.c      | 21 ++++++++++++---------
  net/tap-solaris.c    |  4 ++--
  net/tap-stub.c       |  3 +--
  net/tap.c            |  8 ++++----
  net/tap_int.h        |  4 ++--
  13 files changed, 59 insertions(+), 49 deletions(-)

hw/net/e1000e_core.c +0 -0

index 2413858790..27599a0dc2 100644

hw/net/e1000e_core.c +2 -0

+
+        qemu_set_offload(qemu_get_queue(core->owner_nic)->peer, &ol);
      }
  }
 

hw/net/igb_core.c +3 -2

index 39e3ce1c8f..45d8fd795b 100644
@@ -3058,8 +3058,9 @@ igb_update_rx_offloads(IGBCore *core)
      trace_e1000e_rx_set_cso(cso_state);
 
      if (core->has_vnet) {
-        qemu_set_offload(qemu_get_queue(core->owner_nic)->peer,
-                         cso_state, 0, 0, 0, 0, 0, 0);
+        NetOffloads ol = {.csum = cso_state };
+
+        qemu_set_offload(qemu_get_queue(core->owner_nic)->peer, &ol);
      }
  }
 

hw/net/virtio-net.c +11 -8

index c4c49b0f9c..8953c329e7 100644
@@ -773,14 +773,17 @@ static uint64_t virtio_net_bad_features(VirtIODevice *vdev)
 
  static void virtio_net_apply_guest_offloads(VirtIONet *n)
  {
-    qemu_set_offload(qemu_get_queue(n->nic)->peer,
-            !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_CSUM)),
-            !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_TSO4)),
-            !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_TSO6)),
-            !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_ECN)),
-            !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_UFO)),
-            !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_USO4)),
-            !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_USO6)));
+    NetOffloads ol = {
+       .csum = !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_CSUM)),
+       .tso4 = !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_TSO4)),
+       .tso6 = !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_TSO6)),
+       .ecn  = !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_ECN)),
+       .ufo  = !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_UFO)),
+       .uso4 = !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_USO4)),
+       .uso6 = !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_USO6)),
+    };
+
+    qemu_set_offload(qemu_get_queue(n->nic)->peer, &ol);
  }
 
  static uint64_t virtio_net_guest_offloads_by_features(uint64_t features)

hw/net/vmxnet3.c +5 -8

index af73aa8ef2..03732375a7 100644
@@ -1322,14 +1322,11 @@ static void vmxnet3_update_features(VMXNET3State *s)
                s->lro_supported, rxcso_supported,
                s->rx_vlan_stripping);
      if (s->peer_has_vhdr) {
-        qemu_set_offload(qemu_get_queue(s->nic)->peer,
-                         rxcso_supported,
-                         s->lro_supported,
-                         s->lro_supported,
-                         0,
-                         0,
-                         0,
-                         0);
+        NetOffloads ol = { .csum = rxcso_supported,
+                           .tso4 = s->lro_supported,
+                           .tso6 = s->lro_supported };
+
+        qemu_set_offload(qemu_get_queue(s->nic)->peer, &ol);
      }
  }
 

include/net/net.h +12 -3

index 84ee18e0f9..48ba333d02 100644
@@ -35,6 +35,16 @@ typedef struct NICConf {
      int32_t bootindex;
  } NICConf;
 
+typedef struct NetOffloads {
+    bool csum;
+    bool tso4;
+    bool tso6;
+    bool ecn;
+    bool ufo;
+    bool uso4;
+    bool uso6;
+} NetOffloads;
+
  #define DEFINE_NIC_PROPERTIES(_state, _conf)                            \
      DEFINE_PROP_MACADDR("mac",   _state, _conf.macaddr),                \
      DEFINE_PROP_NETDEV("netdev", _state, _conf.peers)

@@ -57,7 +67,7 @@ typedef bool (HasUfo)(NetClientState *);
  typedef bool (HasUso)(NetClientState *);
  typedef bool (HasVnetHdr)(NetClientState *);
  typedef bool (HasVnetHdrLen)(NetClientState *, int);
-typedef void (SetOffload)(NetClientState *, int, int, int, int, int, int, int);
+typedef void (SetOffload)(NetClientState *, const NetOffloads *);
  typedef int (GetVnetHdrLen)(NetClientState *);
  typedef void (SetVnetHdrLen)(NetClientState *, int);
  typedef bool (GetVnetHashSupportedTypes)(NetClientState *, uint32_t *);

@@ -189,8 +199,7 @@ bool qemu_has_ufo(NetClientState *nc);
  bool qemu_has_uso(NetClientState *nc);
  bool qemu_has_vnet_hdr(NetClientState *nc);
  bool qemu_has_vnet_hdr_len(NetClientState *nc, int len);
-void qemu_set_offload(NetClientState *nc, int csum, int tso4, int tso6,
-                      int ecn, int ufo, int uso4, int uso6);
+void qemu_set_offload(NetClientState *nc, const NetOffloads *ol);
  int qemu_get_vnet_hdr_len(NetClientState *nc);
  void qemu_set_vnet_hdr_len(NetClientState *nc, int len);
  bool qemu_get_vnet_hash_supported_types(NetClientState *nc, uint32_t *types);

net/net.c +2 -3

index da275db86e..63872b6855 100644
@@ -540,14 +540,13 @@ bool qemu_has_vnet_hdr_len(NetClientState *nc, int len)
      return nc->info->has_vnet_hdr_len(nc, len);
  }
 
-void qemu_set_offload(NetClientState *nc, int csum, int tso4, int tso6,
-                          int ecn, int ufo, int uso4, int uso6)
+void qemu_set_offload(NetClientState *nc, const NetOffloads *ol)
  {
      if (!nc || !nc->info->set_offload) {
          return;
      }
 
-    nc->info->set_offload(nc, csum, tso4, tso6, ecn, ufo, uso4, uso6);
+    nc->info->set_offload(nc, ol);
  }
 
  int qemu_get_vnet_hdr_len(NetClientState *nc)

net/netmap.c +1 -2

index 297510e190..6cd8f2bdc5 100644
@@ -366,8 +366,7 @@ static void netmap_set_vnet_hdr_len(NetClientState *nc, int len)
      }
  }
 
-static void netmap_set_offload(NetClientState *nc, int csum, int tso4, int tso6,
-                               int ecn, int ufo, int uso4, int uso6)
+static void netmap_set_offload(NetClientState *nc, const NetOffloads *ol)
  {
      NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
 

net/tap-bsd.c +1 -2

index b4c84441ba..86b6edee94 100644
@@ -231,8 +231,7 @@ int tap_fd_set_vnet_be(int fd, int is_be)
      return -EINVAL;
  }
 
-void tap_fd_set_offload(int fd, int csum, int tso4,
-                        int tso6, int ecn, int ufo, int uso4, int uso6)
+void tap_fd_set_offload(int fd, const NetOffloads *ol)
  {
  }
 

net/tap-linux.c +12 -9

index 22ec2f45d2..a1c58f74f5 100644
@@ -239,8 +239,7 @@ int tap_fd_set_vnet_be(int fd, int is_be)
      abort();
  }
 
-void tap_fd_set_offload(int fd, int csum, int tso4,
-                        int tso6, int ecn, int ufo, int uso4, int uso6)
+void tap_fd_set_offload(int fd, const NetOffloads *ol)
  {
      unsigned int offload = 0;
 

@@ -249,20 +248,24 @@ void tap_fd_set_offload(int fd, int csum, int tso4,
          return;
      }
 
-    if (csum) {
+    if (ol->csum) {
          offload |= TUN_F_CSUM;
-        if (tso4)
+        if (ol->tso4) {
              offload |= TUN_F_TSO4;
-        if (tso6)
+        }
+        if (ol->tso6) {
              offload |= TUN_F_TSO6;
-        if ((tso4 || tso6) && ecn)
+        }
+        if ((ol->tso4 || ol->tso6) && ol->ecn) {
              offload |= TUN_F_TSO_ECN;
-        if (ufo)
+        }
+        if (ol->ufo) {
              offload |= TUN_F_UFO;
-        if (uso4) {
+        }
+        if (ol->uso4) {
              offload |= TUN_F_USO4;
          }
-        if (uso6) {
+        if (ol->uso6) {
              offload |= TUN_F_USO6;
          }
      }

net/tap-solaris.c +2 -2

index 51b7830bef..833c066bee 100644
@@ -27,6 +27,7 @@
  #include "tap_int.h"
  #include "qemu/ctype.h"
  #include "qemu/cutils.h"
+#include "net/net.h"
 
  #include <sys/ethernet.h>
  #include <sys/sockio.h>

@@ -235,8 +236,7 @@ int tap_fd_set_vnet_be(int fd, int is_be)
      return -EINVAL;
  }
 
-void tap_fd_set_offload(int fd, int csum, int tso4,
-                        int tso6, int ecn, int ufo, int uso4, int uso6)
+void tap_fd_set_offload(int fd, const NetOffloads *ol)
  {
  }
 

net/tap-stub.c +1 -2

index 38673434cb..67d14ad4d5 100644
@@ -66,8 +66,7 @@ int tap_fd_set_vnet_be(int fd, int is_be)
      return -EINVAL;
  }
 
-void tap_fd_set_offload(int fd, int csum, int tso4,
-                        int tso6, int ecn, int ufo, int uso4, int uso6)
+void tap_fd_set_offload(int fd, const NetOffloads *ol)
  {
  }
 

net/tap.c +4 -4

index 23536c09b4..b49db19f83 100644
@@ -280,15 +280,14 @@ static int tap_set_vnet_be(NetClientState *nc, bool is_be)
      return tap_fd_set_vnet_be(s->fd, is_be);
  }
 
-static void tap_set_offload(NetClientState *nc, int csum, int tso4,
-                     int tso6, int ecn, int ufo, int uso4, int uso6)
+static void tap_set_offload(NetClientState *nc, const NetOffloads *ol)
  {
      TAPState *s = DO_UPCAST(TAPState, nc, nc);
      if (s->fd < 0) {
          return;
      }
 
-    tap_fd_set_offload(s->fd, csum, tso4, tso6, ecn, ufo, uso4, uso6);
+    tap_fd_set_offload(s->fd, ol);
  }
 
  static void tap_exit_notify(Notifier *notifier, void *data)

@@ -386,6 +385,7 @@ static TAPState *net_tap_fd_init(NetClientState *peer,
                                   int fd,
                                   int vnet_hdr)
  {
+    NetOffloads ol = {};
      NetClientState *nc;
      TAPState *s;
 

@@ -399,7 +399,7 @@ static TAPState *net_tap_fd_init(NetClientState *peer,
      s->has_ufo = tap_probe_has_ufo(s->fd);
      s->has_uso = tap_probe_has_uso(s->fd);
      s->enabled = true;
-    tap_set_offload(&s->nc, 0, 0, 0, 0, 0, 0, 0);
+    tap_set_offload(&s->nc, &ol);
      /*
       * Make sure host header length is set correctly in tap:
       * it might have been modified by another instance of qemu.

net/tap_int.h +2 -2

index 8857ff299d..f8bbe1cb0c 100644
@@ -27,6 +27,7 @@
  #define NET_TAP_INT_H
 
  #include "qapi/qapi-types-net.h"
+#include "net/net.h"
 
  int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
               int vnet_hdr_required, int mq_required, Error **errp);

@@ -37,8 +38,7 @@ void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp);
  int tap_probe_vnet_hdr(int fd, Error **errp);
  int tap_probe_has_ufo(int fd);
  int tap_probe_has_uso(int fd);
-void tap_fd_set_offload(int fd, int csum, int tso4, int tso6, int ecn, int ufo,
-                        int uso4, int uso6);
+void tap_fd_set_offload(int fd, const NetOffloads *ol);
  void tap_fd_set_vnet_hdr_len(int fd, int len);
  int tap_fd_set_vnet_le(int fd, int vnet_is_le);
  int tap_fd_set_vnet_be(int fd, int vnet_is_be);

The virtio specifications allows for up to 128 bits for the device features. Soon we are going to use some of the 'extended' bits features (bit 64 and above) for the virtio net driver.

Represent the virtio features bitmask with a fixes size array, and introduce a few helpers to help manipulate them.

Most drivers will keep using only 64 bits features space: use union to allow them access the lower part of the extended space without any per driver change.

Signed-off-by Paolo Abeni <[email protected]>

---
v2 -> v3:
- fix preprocessor guard name
- use BIT_ULL
- add missing parentheses
- use memcmp()
- _is_empty() -> _empty()
- _andnot() returns a bool, true if dst has any non zero bits
- _array -> _ex

v1 -> v2:
- use a fixed size array for features instead of uint128
- use union with u64 to reduce the needed code churn

---
  include/hw/virtio/virtio-features.h | 123 ++++++++++++++++++++++++++++
  include/hw/virtio/virtio.h          |   7 +-
  2 files changed, 127 insertions(+), 3 deletions(-)

create mode 100644 include/hw/virtio/virtio-features.h

include/hw/virtio/virtio-features.h new file +11 -0

new file mode 100644
index 0000000000..68e326e3e8
@@ -0,0 +1,123 @@
+/*
+ * Virtio features helpers
+ *
+ * Copyright 2025 Red Hat, Inc.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef QEMU_VIRTIO_FEATURES_H
+#define QEMU_VIRTIO_FEATURES_H
+

include/hw/virtio/virtio-features.h new file +36 -0

+
+#define VIRTIO_DECLARE_FEATURES(name)                        \
+    union {                                                  \
+        uint64_t name;                                       \
+        uint64_t name##_ex[VIRTIO_FEATURES_DWORDS];          \
+    }
+
+static inline void virtio_features_clear(uint64_t *features)
+{
+    memset(features, 0, sizeof(features[0]) * VIRTIO_FEATURES_DWORDS);
+}
+
+static inline void virtio_features_from_u64(uint64_t *features, uint64_t from)
+{
+    virtio_features_clear(features);
+    features[0] = from;
+}
+
+static inline bool virtio_has_feature_ex(const uint64_t *features,
+                                         unsigned int fbit)
+{
+    assert(fbit < VIRTIO_FEATURES_MAX);
+    return features[VIRTIO_DWORD(fbit)] & VIRTIO_BIT(fbit);
+}
+
+static inline void virtio_add_feature_ex(uint64_t *features,
+                                         unsigned int fbit)
+{
+    assert(fbit < VIRTIO_FEATURES_MAX);
+    features[VIRTIO_DWORD(fbit)] |= VIRTIO_BIT(fbit);
+}
+
+static inline void virtio_clear_feature_ex(uint64_t *features,
+                                           unsigned int fbit)
+{
+    assert(fbit < VIRTIO_FEATURES_MAX);

include/hw/virtio/virtio-features.h new file +56 -0

+{
+    int i;
+
+    for (i = 1; i < VIRTIO_FEATURES_DWORDS; ++i) {
+        if (features[i]) {
+            return true;
+        }
+    }
+    return false;
+}
+
+static inline bool virtio_features_empty(const uint64_t *features)
+{
+    return !virtio_features_use_extended(features) && !features[0];
+}
+
+static inline void virtio_features_copy(uint64_t *to, const uint64_t *from)
+{
+    memcpy(to, from, sizeof(to[0]) * VIRTIO_FEATURES_DWORDS);
+}
+
+static inline bool virtio_features_andnot(uint64_t *to, const uint64_t *f1,
+                                           const uint64_t *f2)
+{
+    uint64_t diff = 0;
+    int i;
+
+    for (i = 0; i < VIRTIO_FEATURES_DWORDS; i++) {
+        to[i] = f1[i] & ~f2[i];
+        diff |= to[i];
+    }
+    return diff;
+}
+
+static inline void virtio_features_and(uint64_t *to, const uint64_t *f1,
+                                       const uint64_t *f2)
+{
+    int i;
+
+    for (i = 0; i < VIRTIO_FEATURES_DWORDS; i++) {
+        to[i] = f1[i] & f2[i];
+    }
+}
+
+static inline void virtio_features_or(uint64_t *to, const uint64_t *f1,
+                                       const uint64_t *f2)
+{
+    int i;
+
+    for (i = 0; i < VIRTIO_FEATURES_DWORDS; i++) {
+        to[i] = f1[i] | f2[i];
+    }
+}
+
+#endif
+

include/hw/virtio/virtio.h +4 -3

index c594764f23..39e4059a66 100644
@@ -16,6 +16,7 @@
 
  #include "system/memory.h"
  #include "hw/qdev-core.h"
+#include "hw/virtio/virtio-features.h"
  #include "net/net.h"
  #include "migration/vmstate.h"
  #include "qemu/event_notifier.h"

@@ -121,9 +122,9 @@ struct VirtIODevice
       * backend (e.g. vhost) and could potentially be a subset of the
       * total feature set offered by QEMU.
       */
-    uint64_t host_features;
-    uint64_t guest_features;
-    uint64_t backend_features;
+    VIRTIO_DECLARE_FEATURES(host_features);
+    VIRTIO_DECLARE_FEATURES(guest_features);
+    VIRTIO_DECLARE_FEATURES(backend_features);
 
      size_t config_len;
      void *config;

Quoted 17 lines · Paolo Abeni

include/hw/virtio/virtio-features.h new file +11 -0

new file mode 100644
index 0000000000..68e326e3e8
@@ -0,0 +1,123 @@
+/*
+ * Virtio features helpers
+ *
+ * Copyright 2025 Red Hat, Inc.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef QEMU_VIRTIO_FEATURES_H
+#define QEMU_VIRTIO_FEATURES_H
+

include/hw/virtio/virtio-features.h line 21

Quoted 10 lines

include/hw/virtio/virtio-features.h new file +10 -0

+#include "qemu/bitops.h"
+
+#define VIRTIO_FEATURES_FMT        "%016"PRIx64"%016"PRIx64
+#define VIRTIO_FEATURES_PR(f)      (f)[1], (f)[0]
+
+#define VIRTIO_FEATURES_MAX        128
+#define VIRTIO_BIT(b)              BIT_ULL((b) % 64)
+#define VIRTIO_DWORD(b)            ((b) >> 6)
+#define VIRTIO_FEATURES_WORDS      (VIRTIO_FEATURES_MAX >> 5)
+#define VIRTIO_FEATURES_DWORDS     (VIRTIO_FEATURES_WORDS >> 1)

These shifts are better to be replaced with division for clarity; BIT_WORD() is a good example.

"WORD" and "DWORD" should be avoided due to contradicting definitions common in QEMU as described at:
https://lore.kernel.org/qemu-devel/[email protected]/

BITS_TO_U32S() is a good example this regard.

include/hw/virtio/virtio-features.h line 67

Quoted 7 lines

include/hw/virtio/virtio-features.h new file +7 -0

+static inline bool virtio_features_equal(const uint64_t *f1,
+                                         const uint64_t *f2)
+{
+    return !memcmp(f1, f2, sizeof(uint64_t) * VIRTIO_FEATURES_DWORDS);
+}
+
+static inline bool virtio_features_use_extended(const uint64_t *features)

"extended" should be abbreviated as "ex" for consistency.

Quoted 36 lines · Akihiko Odaki

Quoted 17 lines · Paolo Abeni

include/hw/virtio/virtio-features.h new file +11 -0

new file mode 100644
index 0000000000..68e326e3e8
@@ -0,0 +1,123 @@
+/*
+ * Virtio features helpers
+ *
+ * Copyright 2025 Red Hat, Inc.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef QEMU_VIRTIO_FEATURES_H
+#define QEMU_VIRTIO_FEATURES_H
+

include/hw/virtio/virtio-features.h line 21

Quoted 10 lines

include/hw/virtio/virtio-features.h new file +10 -0

+#include "qemu/bitops.h"
+
+#define VIRTIO_FEATURES_FMT        "%016"PRIx64"%016"PRIx64
+#define VIRTIO_FEATURES_PR(f)      (f)[1], (f)[0]
+
+#define VIRTIO_FEATURES_MAX        128
+#define VIRTIO_BIT(b)              BIT_ULL((b) % 64)
+#define VIRTIO_DWORD(b)            ((b) >> 6)
+#define VIRTIO_FEATURES_WORDS      (VIRTIO_FEATURES_MAX >> 5)
+#define VIRTIO_FEATURES_DWORDS     (VIRTIO_FEATURES_WORDS >> 1)

These shifts are better to be replaced with division for clarity; BIT_WORD() is a good example.

"WORD" and "DWORD" should be avoided due to contradicting definitions common in QEMU as described at:
https://lore.kernel.org/qemu-devel/[email protected]/

Quoted 1 line

BITS_TO_U32S() is a good example this regard.

Ok, I'll rename:

	VIRTIO_FEATURES_DWORDS -> VIRTIO_U64_PER_FEATURES
	VIRTIO_FEATURES_WORDS -> VIRTIO_U32_PER_FEATURES

Quoted 37 lines · Paolo Abeni

Quoted 36 lines · Akihiko Odaki

Quoted 17 lines · Paolo Abeni

include/hw/virtio/virtio-features.h new file +11 -0

new file mode 100644
index 0000000000..68e326e3e8
@@ -0,0 +1,123 @@
+/*
+ * Virtio features helpers
+ *
+ * Copyright 2025 Red Hat, Inc.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef QEMU_VIRTIO_FEATURES_H
+#define QEMU_VIRTIO_FEATURES_H
+

include/hw/virtio/virtio-features.h line 21

Quoted 10 lines

include/hw/virtio/virtio-features.h new file +10 -0

+#include "qemu/bitops.h"
+
+#define VIRTIO_FEATURES_FMT        "%016"PRIx64"%016"PRIx64
+#define VIRTIO_FEATURES_PR(f)      (f)[1], (f)[0]
+
+#define VIRTIO_FEATURES_MAX        128
+#define VIRTIO_BIT(b)              BIT_ULL((b) % 64)
+#define VIRTIO_DWORD(b)            ((b) >> 6)
+#define VIRTIO_FEATURES_WORDS      (VIRTIO_FEATURES_MAX >> 5)
+#define VIRTIO_FEATURES_DWORDS     (VIRTIO_FEATURES_WORDS >> 1)

These shifts are better to be replaced with division for clarity; BIT_WORD() is a good example.

"WORD" and "DWORD" should be avoided due to contradicting definitions common in QEMU as described at:
https://lore.kernel.org/qemu-devel/[email protected]/

Quoted 5 lines

Quoted 1 line

BITS_TO_U32S() is a good example this regard.

Ok, I'll rename:

	VIRTIO_FEATURES_DWORDS -> VIRTIO_U64_PER_FEATURES
	VIRTIO_FEATURES_WORDS -> VIRTIO_U32_PER_FEATURES

U64 and U32 should be plural (i.e., rename them into VIRTIO_U64S_PER_FEATURES and VIRTIO_U32S_PER_FEATURES)

"PER_FEATURES" also sounds a bit awkward; BITS_PER_BYTE and BITS_PER_LONG had singular after "per" so the unit was clear, but it is not in this case.

I could think of several options:
- VIRTIO_U64S_PER_FEATURES (what you proposed + plural U64S)
- VIRTIO_FEATURES_U64S (closer to the previous version)
- VIRTIO_FEATURES_NU64S (like CPU_TEMP_BUF_NLONGS)
- VIRTIO_U64S_PER_FEATURE_BITMASK

They have downsides and upsides, and I don't have an idea what's the best.

If the driver uses any of the extended features (i.e. 64 or above), store the extended features range (64-127 bits).

At load time, let legacy features initialize the full features range and pass it to the set helper; sub-states loading will have filled-up the extended part as needed.

This is one of the few spots that need explicitly to know and set in stone the extended features array size; add a build bug to prevent breaking the migration should such size change again in the future:
more serialization plumbing will be needed.

Signed-off-by Paolo Abeni <[email protected]>

---
v2 -> v3:
- 128bit_features state load/stores only the high bits
- consolidate the load implementation to use a single set helper for 128/64/32 bits features
- _array -> _ex

v1 -> v2:
- uint128_t -> u64[2]
---
hw/virtio/virtio.c | 88 ++++++++++++++++++++++++++++++----------------

1 file changed, 57 insertions(+), 31 deletions(-)

hw/virtio/virtio.c +0 -0

index 2ab1d20769..2817d3a893 100644

hw/virtio/virtio.c +25 -6

+}
+
+static const VMStateDescription vmstate_virtio_128bit_features = {
+    .name = "virtio/128bit_features",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = &virtio_128bit_features_needed,
+    .fields = (const VMStateField[]) {
+        VMSTATE_UINT64(guest_features_ex[1], VirtIODevice),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_virtio = {
    .name = "virtio",
    .version_id = 1,

@@ -2963,6 +2981,7 @@ static const VMStateDescription vmstate_virtio = {
    },
    .subsections = (const VMStateDescription * const []) {
        &vmstate_virtio_device_endian,
+        &vmstate_virtio_128bit_features,
        &vmstate_virtio_64bit_features,
        &vmstate_virtio_virtqueues,
        &vmstate_virtio_ringsize,

@@ -3059,23 +3078,28 @@ const VMStateInfo  virtio_vmstate_info = {
    .put = virtio_device_put,
 };
 
-static int virtio_set_features_nocheck(VirtIODevice *vdev, uint64_t val)
+static int virtio_set_features_nocheck(VirtIODevice *vdev, const uint64_t *val)
 {
    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
-    bool bad = (val & ~(vdev->host_features)) != 0;
+    uint64_t tmp[VIRTIO_FEATURES_DWORDS];
+    bool bad;
+
+    bad = virtio_features_andnot(tmp, val, vdev->host_features_ex);
+    virtio_features_and(tmp, val, vdev->host_features_ex);
 
-    val &= vdev->host_features;
    if (k->set_features) {
-        k->set_features(vdev, val);
+        bad = bad || virtio_features_use_extended(tmp);
+        k->set_features(vdev, tmp[0]);
    }
-    vdev->guest_features = val;
+
+    virtio_features_copy(vdev->guest_features_ex, tmp);
    return bad ? -1 : 0;
 }
 
 typedef struct VirtioSetFeaturesNocheckData {
    Coroutine *co;
    VirtIODevice *vdev;
-    uint64_t val;
+    uint64_t val[VIRTIO_FEATURES_DWORDS];
    int ret;
 } VirtioSetFeaturesNocheckData;
 

@@ -3088,14 +3112,15 @@ static void virtio_set_features_nocheck_bh(void *opaque)
 }
 

static int coroutine_mixed_fn

hw/virtio/virtio.c +2 -1

-virtio_set_features_nocheck_maybe_co(VirtIODevice *vdev, uint64_t val)
+virtio_set_features_nocheck_maybe_co(VirtIODevice *vdev,
+                                     const uint64_t *val)

{
if (qemu_in_coroutine()) {
VirtioSetFeaturesNocheckData data = {
.co = qemu_coroutine_self(),
.vdev = vdev,

hw/virtio/virtio.c +25 -24

-            .val = val,
        };
+        virtio_features_copy(data.val, val);
        aio_bh_schedule_oneshot(qemu_get_current_aio_context(),
                                virtio_set_features_nocheck_bh, &data);
        qemu_coroutine_yield();

@@ -3107,6 +3132,7 @@ virtio_set_features_nocheck_maybe_co(VirtIODevice *vdev, uint64_t val)
 
 int virtio_set_features(VirtIODevice *vdev, uint64_t val)
 {
+    uint64_t features[VIRTIO_FEATURES_DWORDS];
    int ret;
    /*
     * The driver must not attempt to set features after feature negotiation

@@ -3122,7 +3148,8 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t val)
                      __func__, vdev->name);
    }
 
-    ret = virtio_set_features_nocheck(vdev, val);
+    virtio_features_from_u64(features, val);
+    ret = virtio_set_features_nocheck(vdev, features);
    if (virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
        /* VIRTIO_RING_F_EVENT_IDX changes the size of the caches.  */
        int i;

@@ -3145,6 +3172,7 @@ void virtio_reset(void *opaque)
 {
    VirtIODevice *vdev = opaque;
    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
+    uint64_t features[VIRTIO_FEATURES_DWORDS];
    int i;
 
    virtio_set_status(vdev, 0);

@@ -3171,7 +3199,8 @@ void virtio_reset(void *opaque)
    vdev->start_on_kick = false;
    vdev->started = false;
    vdev->broken = false;
-    virtio_set_features_nocheck(vdev, 0);
+    virtio_features_clear(features);
+    virtio_set_features_nocheck(vdev, features);
    vdev->queue_sel = 0;
    vdev->status = 0;
    vdev->disabled = false;

@@ -3254,7 +3283,7 @@ virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
     * Note: devices should always test host features in future - don't create
     * new dependencies like this.
     */
-    vdev->guest_features = features;
+    virtio_features_from_u64(vdev->guest_features_ex, features);
 
    config_len = qemu_get_be32(f);
 

@@ -3333,26 +3362,23 @@ virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
        vdev->device_endian = virtio_default_endian();
    }
 
-    if (virtio_64bit_features_needed(vdev)) {
-        /*
-         * Subsection load filled vdev->guest_features.  Run them
-         * through virtio_set_features to sanity-check them against
-         * host_features.
-         */
-        uint64_t features64 = vdev->guest_features;
-        if (virtio_set_features_nocheck_maybe_co(vdev, features64) < 0) {
-            error_report("Features 0x%" PRIx64 " unsupported. "
-                         "Allowed features: 0x%" PRIx64,
-                         features64, vdev->host_features);
-            return -1;
-        }
-    } else {
-        if (virtio_set_features_nocheck_maybe_co(vdev, features) < 0) {
-            error_report("Features 0x%x unsupported. "
-                         "Allowed features: 0x%" PRIx64,
-                         features, vdev->host_features);
-            return -1;
-        }
+    /*
+     * Avoid silently breaking migration should the feature space increase
+     * even more in the (far away) future
+     */
+    QEMU_BUILD_BUG_ON(VIRTIO_FEATURES_DWORDS != 2);
+
+    /*
+     * guest_features_ex is fully initialized with u32 features and upper
+     * bits have been filled as needed by the later load.
+     */
+    if (virtio_set_features_nocheck_maybe_co(vdev,
+                                             vdev->guest_features_ex) < 0) {
+        error_report("Features 0x" VIRTIO_FEATURES_FMT " unsupported. "
+                     "Allowed features: 0x" VIRTIO_FEATURES_FMT,
+                     VIRTIO_FEATURES_PR(vdev->guest_features_ex),
+                     VIRTIO_FEATURES_PR(vdev->host_features_ex));
+        return -1;
    }
 
    if (!virtio_device_started(vdev, vdev->status) &&

Signature · 1 line

2.50.0

If the driver uses any of the extended features (i.e. 64 or above), store the extended features range (64-127 bits).

At load time, let legacy features initialize the full features range and pass it to the set helper; sub-states loading will have filled-up the extended part as needed.

This is one of the few spots that need explicitly to know and set in stone the extended features array size; add a build bug to prevent breaking the migration should such size change again in the future:
more serialization plumbing will be needed.

Signed-off-by Paolo Abeni <[email protected]>

---
v2 -> v3:
- 128bit_features state load/stores only the high bits
- consolidate the load implementation to use a single set

    helper for 128/64/32 bits features

- _array -> _ex

v1 -> v2:
- uint128_t -> u64[2]

---
  hw/virtio/virtio.c | 88 ++++++++++++++++++++++++++++++----------------
  1 file changed, 57 insertions(+), 31 deletions(-)

hw/virtio/virtio.c +40 -26

index 2ab1d20769..2817d3a893 100644
@@ -2954,6 +2954,24 @@ static const VMStateDescription vmstate_virtio_disabled = {
      }
  };
 
+static bool virtio_128bit_features_needed(void *opaque)
+{
+    VirtIODevice *vdev = opaque;
+
+    return virtio_features_use_extended(vdev->host_features_ex);
+}
+
+static const VMStateDescription vmstate_virtio_128bit_features = {
+    .name = "virtio/128bit_features",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = &virtio_128bit_features_needed,
+    .fields = (const VMStateField[]) {
+        VMSTATE_UINT64(guest_features_ex[1], VirtIODevice),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
  static const VMStateDescription vmstate_virtio = {
      .name = "virtio",
      .version_id = 1,

@@ -2963,6 +2981,7 @@ static const VMStateDescription vmstate_virtio = {
      },
      .subsections = (const VMStateDescription * const []) {
          &vmstate_virtio_device_endian,
+        &vmstate_virtio_128bit_features,
          &vmstate_virtio_64bit_features,
          &vmstate_virtio_virtqueues,
          &vmstate_virtio_ringsize,

@@ -3059,23 +3078,28 @@ const VMStateInfo  virtio_vmstate_info = {
      .put = virtio_device_put,
  };
 
-static int virtio_set_features_nocheck(VirtIODevice *vdev, uint64_t val)
+static int virtio_set_features_nocheck(VirtIODevice *vdev, const uint64_t *val)
  {
      VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
-    bool bad = (val & ~(vdev->host_features)) != 0;
+    uint64_t tmp[VIRTIO_FEATURES_DWORDS];
+    bool bad;
+
+    bad = virtio_features_andnot(tmp, val, vdev->host_features_ex);
+    virtio_features_and(tmp, val, vdev->host_features_ex);
 
-    val &= vdev->host_features;
      if (k->set_features) {
-        k->set_features(vdev, val);
+        bad = bad || virtio_features_use_extended(tmp);
+        k->set_features(vdev, tmp[0]);
      }
-    vdev->guest_features = val;
+
+    virtio_features_copy(vdev->guest_features_ex, tmp);
      return bad ? -1 : 0;
  }
 
  typedef struct VirtioSetFeaturesNocheckData {
      Coroutine *co;
      VirtIODevice *vdev;
-    uint64_t val;
+    uint64_t val[VIRTIO_FEATURES_DWORDS];
      int ret;
  } VirtioSetFeaturesNocheckData;
 

@@ -3088,14 +3112,15 @@ static void virtio_set_features_nocheck_bh(void *opaque)
  }
 
  static int coroutine_mixed_fn
-virtio_set_features_nocheck_maybe_co(VirtIODevice *vdev, uint64_t val)
+virtio_set_features_nocheck_maybe_co(VirtIODevice *vdev,
+                                     const uint64_t *val)
  {
      if (qemu_in_coroutine()) {
          VirtioSetFeaturesNocheckData data = {
              .co = qemu_coroutine_self(),
              .vdev = vdev,
-            .val = val,
          };
+        virtio_features_copy(data.val, val);
          aio_bh_schedule_oneshot(qemu_get_current_aio_context(),
                                  virtio_set_features_nocheck_bh, &data);
          qemu_coroutine_yield();

@@ -3107,6 +3132,7 @@ virtio_set_features_nocheck_maybe_co(VirtIODevice *vdev, uint64_t val)
 
  int virtio_set_features(VirtIODevice *vdev, uint64_t val)
  {
+    uint64_t features[VIRTIO_FEATURES_DWORDS];
      int ret;
      /*
       * The driver must not attempt to set features after feature negotiation

@@ -3122,7 +3148,8 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t val)
                        __func__, vdev->name);
      }
 
-    ret = virtio_set_features_nocheck(vdev, val);
+    virtio_features_from_u64(features, val);
+    ret = virtio_set_features_nocheck(vdev, features);
      if (virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
          /* VIRTIO_RING_F_EVENT_IDX changes the size of the caches.  */
          int i;

@@ -3145,6 +3172,7 @@ void virtio_reset(void *opaque)
  {
      VirtIODevice *vdev = opaque;
      VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
+    uint64_t features[VIRTIO_FEATURES_DWORDS];
      int i;
 
      virtio_set_status(vdev, 0);

@@ -3171,7 +3199,8 @@ void virtio_reset(void *opaque)
      vdev->start_on_kick = false;
      vdev->started = false;
      vdev->broken = false;
-    virtio_set_features_nocheck(vdev, 0);
+    virtio_features_clear(features);
+    virtio_set_features_nocheck(vdev, features);
      vdev->queue_sel = 0;
      vdev->status = 0;
      vdev->disabled = false;

@@ -3254,7 +3283,7 @@ virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
       * Note: devices should always test host features in future - don't create
       * new dependencies like this.
       */
-    vdev->guest_features = features;
+    virtio_features_from_u64(vdev->guest_features_ex, features);
 
      config_len = qemu_get_be32(f);
 

@@ -3333,26 +3362,23 @@ virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
          vdev->device_endian = virtio_default_endian();
      }
 
-    if (virtio_64bit_features_needed(vdev)) {
-        /*
-         * Subsection load filled vdev->guest_features.  Run them
-         * through virtio_set_features to sanity-check them against
-         * host_features.
-         */
-        uint64_t features64 = vdev->guest_features;
-        if (virtio_set_features_nocheck_maybe_co(vdev, features64) < 0) {
-            error_report("Features 0x%" PRIx64 " unsupported. "
-                         "Allowed features: 0x%" PRIx64,
-                         features64, vdev->host_features);
-            return -1;
-        }
-    } else {
-        if (virtio_set_features_nocheck_maybe_co(vdev, features) < 0) {

hw/virtio/virtio.c +12 -0

+
+    /*
+     * guest_features_ex is fully initialized with u32 features and upper
+     * bits have been filled as needed by the later load.
+     */
+    if (virtio_set_features_nocheck_maybe_co(vdev,
+                                             vdev->guest_features_ex) < 0) {
+        error_report("Features 0x" VIRTIO_FEATURES_FMT " unsupported. "
+                     "Allowed features: 0x" VIRTIO_FEATURES_FMT,
+                     VIRTIO_FEATURES_PR(vdev->guest_features_ex),
+                     VIRTIO_FEATURES_PR(vdev->host_features_ex));
+        return -1;
      }
 
      if (!virtio_device_started(vdev, vdev->status) &&

Quoted 25 lines · Paolo Abeni

hw/virtio/virtio.c +1 -20

@@ -3333,26 +3362,23 @@ virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
          vdev->device_endian = virtio_default_endian();
      }
 
-    if (virtio_64bit_features_needed(vdev)) {
-        /*
-         * Subsection load filled vdev->guest_features.  Run them
-         * through virtio_set_features to sanity-check them against
-         * host_features.
-         */
-        uint64_t features64 = vdev->guest_features;
-        if (virtio_set_features_nocheck_maybe_co(vdev, features64) < 0) {
-            error_report("Features 0x%" PRIx64 " unsupported. "
-                         "Allowed features: 0x%" PRIx64,
-                         features64, vdev->host_features);
-            return -1;
-        }
-    } else {
-        if (virtio_set_features_nocheck_maybe_co(vdev, features) < 0) {
-            error_report("Features 0x%x unsupported. "
-                         "Allowed features: 0x%" PRIx64,
-                         features, vdev->host_features);
-            return -1;
-        }
+    /*

hw/virtio/virtio.c virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id) · line 3369

Quoted 4 lines

hw/virtio/virtio.c +4 -0

+     * Avoid silently breaking migration should the feature space increase
+     * even more in the (far away) future
+     */
+    QEMU_BUILD_BUG_ON(VIRTIO_FEATURES_DWORDS != 2);

This should be moved to around virtio_128bit_features_needed() and vmstate_virtio_128bit_features as they make assumptions on the feature space while virtio_load() doesn't.

Quoted 26 lines · Akihiko Odaki

Quoted 25 lines · Paolo Abeni

hw/virtio/virtio.c +1 -20

@@ -3333,26 +3362,23 @@ virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
           vdev->device_endian = virtio_default_endian();
       }
 
-    if (virtio_64bit_features_needed(vdev)) {
-        /*
-         * Subsection load filled vdev->guest_features.  Run them
-         * through virtio_set_features to sanity-check them against
-         * host_features.
-         */
-        uint64_t features64 = vdev->guest_features;
-        if (virtio_set_features_nocheck_maybe_co(vdev, features64) < 0) {
-            error_report("Features 0x%" PRIx64 " unsupported. "
-                         "Allowed features: 0x%" PRIx64,
-                         features64, vdev->host_features);
-            return -1;
-        }
-    } else {
-        if (virtio_set_features_nocheck_maybe_co(vdev, features) < 0) {
-            error_report("Features 0x%x unsupported. "
-                         "Allowed features: 0x%" PRIx64,
-                         features, vdev->host_features);
-            return -1;
-        }
+    /*

Quoted 8 lines

hw/virtio/virtio.c virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id) · line 3369

Quoted 4 lines

hw/virtio/virtio.c +4 -0

+     * Avoid silently breaking migration should the feature space increase
+     * even more in the (far away) future
+     */
+    QEMU_BUILD_BUG_ON(VIRTIO_FEATURES_DWORDS != 2);

This should be moved to around virtio_128bit_features_needed() and vmstate_virtio_128bit_features as they make assumptions on the feature space while virtio_load() doesn't.

Please allow me the following dumb question to try to avoid more iterations for small changes. I guess moving the above inside virtio_128bit_features_needed() would suffice?

Extend the features configuration space to 128 bits, and allow the common read/write operation to access all of it.

On migration, save the 128 bit version of the features only if the upper bits are non zero. Relay reset to clear all the feature space before load.

Signed-off-by Paolo Abeni <[email protected]>

Quoted 8 lines · Paolo Abeni

Extend the features configuration space to 128 bits, and allow the common read/write operation to access all of it.

On migration, save the 128 bit version of the features only if the upper bits are non zero. Relay reset to clear all the feature space before load.

Quoted 1 line

Signed-off-by Paolo Abeni <[email protected]>

This is a guest noticeable behaviour change. I wonder if we need a command line option to enable and disable this feature for migration compatibility.

Quoted 11 lines · Jason Wang

Quoted 9 lines · Paolo Abeni

Extend the features configuration space to 128 bits, and allow the common read/write operation to access all of it.

On migration, save the 128 bit version of the features only if the upper bits are non zero. Relay reset to clear all the feature space before load.

Signed-off-by Paolo Abeni <[email protected]>

Quoted 3 lines

This is a guest noticeable behaviour change. I wonder if we need a command line option to enable and disable this feature for migration compatibility.

This point is not clear to me, could you please elaborate a bit more? do you mean we need i.e. a DEFINE_PROP_BOOL() or the like to enable the 128 bit space usage?

Thanks,

Paolo

Quoted 17 lines · Paolo Abeni

Quoted 14 lines · Jason Wang

Quoted 9 lines · Paolo Abeni

Extend the features configuration space to 128 bits, and allow the common read/write operation to access all of it.

On migration, save the 128 bit version of the features only if the upper bits are non zero. Relay reset to clear all the feature space before load.

Signed-off-by Paolo Abeni <[email protected]>

This is a guest noticeable behaviour change. I wonder if we need a command line option to enable and disable this feature for migration compatibility.

Quoted 3 lines

This point is not clear to me, could you please elaborate a bit more? do you mean we need i.e. a DEFINE_PROP_BOOL() or the like to enable the 128 bit space usage?

Yes, or maybe have a way to let the device to enable it automatically E.g when UDP GSO is enabled for virtio-net.

Similar to virtio infra, vhost core maintains the features status in the full extended format and allows the devices to implement extended version of the getter/setter.

Note that 'protocol_features' are not extended: they are only used by vhost-user, and the latter device is not going to implement extended features soon.

Signed-off-by Paolo Abeni <[email protected]>

---
v2 -> v3:
- fix compile warning
- _array -> _ex

v1 -> v2:
- uint128_t -> uint64_t[]
- add _ex() variant of features manipulation helpers
---

hw/virtio/vhost.c                 | 73 +++++++++++++++++++++++++++----
include/hw/virtio/vhost-backend.h |  6 +++
include/hw/virtio/vhost.h         | 33 ++++++++++++--
3 files changed, 100 insertions(+), 12 deletions(-)

hw/virtio/vhost.c +38 -9

index c30ea1156e..85ae1e4d4c 100644
@@ -972,20 +972,34 @@ static int vhost_virtqueue_set_addr(struct vhost_dev *dev,
 static int vhost_dev_set_features(struct vhost_dev *dev,
                                  bool enable_log)
 {
-    uint64_t features = dev->acked_features;
+    uint64_t features[VIRTIO_FEATURES_DWORDS];
    int r;
+
+    virtio_features_copy(features, dev->acked_features_ex);
    if (enable_log) {
-        features |= 0x1ULL << VHOST_F_LOG_ALL;
+        virtio_add_feature_ex(features, VHOST_F_LOG_ALL);
    }
    if (!vhost_dev_has_iommu(dev)) {
-        features &= ~(0x1ULL << VIRTIO_F_IOMMU_PLATFORM);
+        virtio_clear_feature_ex(features, VIRTIO_F_IOMMU_PLATFORM);
    }
    if (dev->vhost_ops->vhost_force_iommu) {
        if (dev->vhost_ops->vhost_force_iommu(dev) == true) {
-            features |= 0x1ULL << VIRTIO_F_IOMMU_PLATFORM;
+            virtio_add_feature_ex(features, VIRTIO_F_IOMMU_PLATFORM);
       }
    }
-    r = dev->vhost_ops->vhost_set_features(dev, features);
+
+    if (virtio_features_use_extended(features) &&
+        !dev->vhost_ops->vhost_set_features_ex) {
+        VHOST_OPS_DEBUG(r, "extended features without device support");
+        r = -EINVAL;
+        goto out;
+    }
+
+    if (dev->vhost_ops->vhost_set_features_ex) {
+        r = dev->vhost_ops->vhost_set_features_ex(dev, features);
+    } else {
+        r = dev->vhost_ops->vhost_set_features(dev, features[0]);
+    }
    if (r < 0) {
        VHOST_OPS_DEBUG(r, "vhost_set_features failed");
        goto out;

@@ -1506,12 +1520,27 @@ static void vhost_virtqueue_cleanup(struct vhost_virtqueue *vq)
    }
 }
 
+static int vhost_dev_get_features(struct vhost_dev *hdev,
+                                  uint64_t *features)
+{
+    uint64_t features64;
+    int r;
+
+    if (hdev->vhost_ops->vhost_get_features_ex) {
+        return hdev->vhost_ops->vhost_get_features_ex(hdev, features);
+    }
+
+    r = hdev->vhost_ops->vhost_get_features(hdev, &features64);
+    virtio_features_from_u64(features, features64);
+    return r;
+}
+
 int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
                   VhostBackendType backend_type, uint32_t busyloop_timeout,
                   Error **errp)
 {
+    uint64_t features[VIRTIO_FEATURES_DWORDS];
    unsigned int used, reserved, limit;
-    uint64_t features;
    int i, r, n_initialized_vqs = 0;
 
    hdev->vdev = NULL;

@@ -1531,7 +1560,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
        goto fail;
    }
 
-    r = hdev->vhost_ops->vhost_get_features(hdev, &features);
+    r = vhost_dev_get_features(hdev, features);
    if (r < 0) {
        error_setg_errno(errp, -r, "vhost_get_features failed");
        goto fail;

@@ -1569,7 +1598,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
        }
    }
 
-    hdev->features = features;
+    virtio_features_copy(hdev->features_ex, features);
 
    hdev->memory_listener = (MemoryListener) {
        .name = "vhost",

@@ -1592,7 +1621,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void 
 *opaque,
    };
 
    if (hdev->migration_blocker == NULL) {
-        if (!(hdev->features & (0x1ULL << VHOST_F_LOG_ALL))) {
+        if (!virtio_has_feature_ex(hdev->features_ex, VHOST_F_LOG_ALL)) {
            error_setg(&hdev->migration_blocker,
                       "Migration disabled: vhost lacks VHOST_F_LOG_ALL feature.");

        } else if (vhost_dev_log_is_shared(hdev) && !qemu_memfd_alloc_check()) {

hw/virtio/vhost.c +4 -0

@@ -1871,6 +1900,20 @@ uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
    return features;
 }
 
+void vhost_get_features_ex(struct vhost_dev *hdev,
+                           const int *feature_bits,
+                           uint64_t *features)
+{

hw/virtio/vhost.c +2 -0

 void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
                        uint64_t features)
 {

@@ -1884,6 +1927,18 @@ void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
    }
 }
 
+void vhost_ack_features_ex(struct vhost_dev *hdev, const int *feature_bits,
+                           const uint64_t *features)

hw/virtio/vhost.c +0 -0

 int vhost_dev_get_config(struct vhost_dev *hdev, uint8_t *config,
                         uint32_t config_len, Error **errp)
 {

include/hw/virtio/vhost-backend.h +6 -0

index d6df209a2f..ff94fa1734 100644
@@ -95,6 +95,10 @@ typedef int (*vhost_new_worker_op)(struct vhost_dev *dev,
                                   struct vhost_worker_state *worker);
 typedef int (*vhost_free_worker_op)(struct vhost_dev *dev,
                                    struct vhost_worker_state *worker);
+typedef int (*vhost_set_features_ex_op)(struct vhost_dev *dev,
+                                        const uint64_t *features);
+typedef int (*vhost_get_features_ex_op)(struct vhost_dev *dev,
+                                        uint64_t *features);
 typedef int (*vhost_set_features_op)(struct vhost_dev *dev,
                                     uint64_t features);
 typedef int (*vhost_get_features_op)(struct vhost_dev *dev,

@@ -186,6 +190,8 @@ typedef struct VhostOps {
    vhost_free_worker_op vhost_free_worker;
    vhost_get_vring_worker_op vhost_get_vring_worker;
    vhost_attach_vring_worker_op vhost_attach_vring_worker;
+    vhost_set_features_ex_op vhost_set_features_ex;
+    vhost_get_features_ex_op vhost_get_features_ex;
    vhost_set_features_op vhost_set_features;
    vhost_get_features_op vhost_get_features;
    vhost_set_backend_cap_op vhost_set_backend_cap;

include/hw/virtio/vhost.h +30 -3

index 66be6afc88..39fbffc6bc 100644
@@ -107,9 +107,9 @@ struct vhost_dev {
     * future use should be discouraged and the variable retired as
     * its easy to confuse with the VirtIO backend_features.
     */
-    uint64_t features;
-    uint64_t acked_features;
-    uint64_t backend_features;
+    VIRTIO_DECLARE_FEATURES(features);
+    VIRTIO_DECLARE_FEATURES(acked_features);
+    VIRTIO_DECLARE_FEATURES(backend_features);
 
    /**
     * @protocol_features: is the vhost-user only feature set by

@@ -333,6 +333,21 @@ void vhost_virtqueue_mask(struct vhost_dev *hdev, VirtIODevice *vdev, int n,
 uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
                            uint64_t features);
 
+/**
+ * vhost_get_features_ex() - sanitize the extended features set
+ * @hdev: common vhost_dev structure
+ * @feature_bits: pointer to terminated table of feature bits
+ * @features: original features set, filtered out on return
+ *
+ * This is the extended variant of vhost_get_features(), supporting the
+ * the extended features set. Filter it with the intersection of what is
+ * supported by the vhost backend (hdev->features) and the supported
+ * feature_bits.
+ */
+void vhost_get_features_ex(struct vhost_dev *hdev,
+                           const int *feature_bits,
+                           uint64_t *features);
+
 /**
 * vhost_ack_features() - set vhost acked_features
 * @hdev: common vhost_dev structure

@@ -344,6 +359,18 @@ uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
 */
 void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
                        uint64_t features);
+
+/**
+ * vhost_ack_features_ex() - set vhost full set of acked_features
+ * @hdev: common vhost_dev structure
+ * @feature_bits: pointer to terminated table of feature bits
+ * @features: requested feature set
+ *
+ * This sets the internal hdev->acked_features to the intersection of
+ * the backends advertised features and the supported feature_bits.
+ */
+void vhost_ack_features_ex(struct vhost_dev *hdev, const int *feature_bits,
+                           const uint64_t *features);
 unsigned int vhost_get_max_memslots(void);
 unsigned int vhost_get_free_memslots(void);
 

Signature · 1 line

2.50.0

Quoted 44 lines · Paolo Abeni

Similar to virtio infra, vhost core maintains the features status in the full extended format and allows the devices to implement extended version of the getter/setter.

Note that 'protocol_features' are not extended: they are only used by vhost-user, and the latter device is not going to implement extended features soon.

Signed-off-by Paolo Abeni <[email protected]>

---
v2 -> v3:
- fix compile warning
- _array -> _ex

v1 -> v2:
- uint128_t -> uint64_t[]
- add _ex() variant of features manipulation helpers
---

hw/virtio/vhost.c                 | 73 +++++++++++++++++++++++++++----
include/hw/virtio/vhost-backend.h |  6 +++
include/hw/virtio/vhost.h         | 33 ++++++++++++--
3 files changed, 100 insertions(+), 12 deletions(-)

hw/virtio/vhost.c +5 -3

index c30ea1156e..85ae1e4d4c 100644
@@ -972,20 +972,34 @@ static int vhost_virtqueue_set_addr(struct vhost_dev *dev,
 static int vhost_dev_set_features(struct vhost_dev *dev,
                                  bool enable_log)
 {
-    uint64_t features = dev->acked_features;
+    uint64_t features[VIRTIO_FEATURES_DWORDS];
    int r;
+
+    virtio_features_copy(features, dev->acked_features_ex);
    if (enable_log) {
-        features |= 0x1ULL << VHOST_F_LOG_ALL;
+        virtio_add_feature_ex(features, VHOST_F_LOG_ALL);
    }
    if (!vhost_dev_has_iommu(dev)) {
-        features &= ~(0x1ULL << VIRTIO_F_IOMMU_PLATFORM);
+        virtio_clear_feature_ex(features, VIRTIO_F_IOMMU_PLATFORM);
    }

hw/virtio/vhost.c static int vhost_virtqueue_set_addr(struct vhost_dev *dev, · line 990

Quoted 8 lines

hw/virtio/vhost.c +2 -2

    if (dev->vhost_ops->vhost_force_iommu) {
        if (dev->vhost_ops->vhost_force_iommu(dev) == true) {
-            features |= 0x1ULL << VIRTIO_F_IOMMU_PLATFORM;
+            virtio_add_feature_ex(features, VIRTIO_F_IOMMU_PLATFORM);
       }
    }
-    r = dev->vhost_ops->vhost_set_features(dev, features);
+

hw/virtio/vhost.c static int vhost_virtqueue_set_addr(struct vhost_dev *dev, · line 996

Quoted 6 lines

hw/virtio/vhost.c +6 -0

+    if (virtio_features_use_extended(features) &&
+        !dev->vhost_ops->vhost_set_features_ex) {
+        VHOST_OPS_DEBUG(r, "extended features without device support");
+        r = -EINVAL;
+        goto out;
+    }

Quoted 74 lines

hw/virtio/vhost.c +25 -4

+
+    if (dev->vhost_ops->vhost_set_features_ex) {
+        r = dev->vhost_ops->vhost_set_features_ex(dev, features);
+    } else {
+        r = dev->vhost_ops->vhost_set_features(dev, features[0]);
+    }
    if (r < 0) {
        VHOST_OPS_DEBUG(r, "vhost_set_features failed");
        goto out;

@@ -1506,12 +1520,27 @@ static void vhost_virtqueue_cleanup(struct vhost_virtqueue *vq)
    }
 }
 
+static int vhost_dev_get_features(struct vhost_dev *hdev,
+                                  uint64_t *features)
+{
+    uint64_t features64;
+    int r;
+
+    if (hdev->vhost_ops->vhost_get_features_ex) {
+        return hdev->vhost_ops->vhost_get_features_ex(hdev, features);
+    }
+
+    r = hdev->vhost_ops->vhost_get_features(hdev, &features64);
+    virtio_features_from_u64(features, features64);
+    return r;
+}
+
 int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
                   VhostBackendType backend_type, uint32_t busyloop_timeout,
                   Error **errp)
 {
+    uint64_t features[VIRTIO_FEATURES_DWORDS];
    unsigned int used, reserved, limit;
-    uint64_t features;
    int i, r, n_initialized_vqs = 0;
 
    hdev->vdev = NULL;

@@ -1531,7 +1560,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
        goto fail;
    }
 
-    r = hdev->vhost_ops->vhost_get_features(hdev, &features);
+    r = vhost_dev_get_features(hdev, features);
    if (r < 0) {
        error_setg_errno(errp, -r, "vhost_get_features failed");
        goto fail;

@@ -1569,7 +1598,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
        }
    }
 
-    hdev->features = features;
+    virtio_features_copy(hdev->features_ex, features);
 
    hdev->memory_listener = (MemoryListener) {
        .name = "vhost",

@@ -1592,7 +1621,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void
 *opaque,
    };
 
    if (hdev->migration_blocker == NULL) {
-        if (!(hdev->features & (0x1ULL << VHOST_F_LOG_ALL))) {
+        if (!virtio_has_feature_ex(hdev->features_ex, VHOST_F_LOG_ALL)) {
            error_setg(&hdev->migration_blocker,
                       "Migration disabled: vhost lacks VHOST_F_LOG_ALL feature.");

        } else if (vhost_dev_log_is_shared(hdev) && !qemu_memfd_alloc_check()) {

hw/virtio/vhost.c +4 -0

@@ -1871,6 +1900,20 @@ uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
    return features;
 }
 
+void vhost_get_features_ex(struct vhost_dev *hdev,
+                           const int *feature_bits,
+                           uint64_t *features)
+{

hw/virtio/vhost.c uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits, · line 1916

Quoted 10 lines

hw/virtio/vhost.c +10 -0

+    const int *bit = feature_bits;
+
+    while (*bit != VHOST_INVALID_FEATURE_BIT) {
+        if (!virtio_has_feature_ex(hdev->features_ex, *bit)) {
+            virtio_clear_feature_ex(features, *bit);
+        }
+        bit++;
+    }
+}
+

Can we do something similar of what we do in hw/virtio/virtio.c where the old virtio_set_features() use the new virtio_set_features_ex()?

Quoted 9 lines

hw/virtio/vhost.c +2 -0

 void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
                        uint64_t features)
 {

@@ -1884,6 +1927,18 @@ void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
    }
 }
 
+void vhost_ack_features_ex(struct vhost_dev *hdev, const int *feature_bits,
+                           const uint64_t *features)

hw/virtio/vhost.c void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits, · line 1941

Quoted 10 lines

hw/virtio/vhost.c +10 -0

+{
+    const int *bit = feature_bits;
+    while (*bit != VHOST_INVALID_FEATURE_BIT) {
+        if (virtio_has_feature_ex(features, *bit)) {
+            virtio_add_feature_ex(hdev->acked_features_ex, *bit);
+        }
+        bit++;
+    }
+}
+

Ditto.

Not a strong opinion, but just to reduce code duplication.

Thanks,
Stefano

Quoted 88 lines

hw/virtio/vhost.c +0 -0

 int vhost_dev_get_config(struct vhost_dev *hdev, uint8_t *config,
                         uint32_t config_len, Error **errp)
 {

include/hw/virtio/vhost-backend.h +6 -0

index d6df209a2f..ff94fa1734 100644
@@ -95,6 +95,10 @@ typedef int (*vhost_new_worker_op)(struct vhost_dev *dev,
                                   struct vhost_worker_state *worker);
 typedef int (*vhost_free_worker_op)(struct vhost_dev *dev,
                                    struct vhost_worker_state *worker);
+typedef int (*vhost_set_features_ex_op)(struct vhost_dev *dev,
+                                        const uint64_t *features);
+typedef int (*vhost_get_features_ex_op)(struct vhost_dev *dev,
+                                        uint64_t *features);
 typedef int (*vhost_set_features_op)(struct vhost_dev *dev,
                                     uint64_t features);
 typedef int (*vhost_get_features_op)(struct vhost_dev *dev,

@@ -186,6 +190,8 @@ typedef struct VhostOps {
    vhost_free_worker_op vhost_free_worker;
    vhost_get_vring_worker_op vhost_get_vring_worker;
    vhost_attach_vring_worker_op vhost_attach_vring_worker;
+    vhost_set_features_ex_op vhost_set_features_ex;
+    vhost_get_features_ex_op vhost_get_features_ex;
    vhost_set_features_op vhost_set_features;
    vhost_get_features_op vhost_get_features;
    vhost_set_backend_cap_op vhost_set_backend_cap;

include/hw/virtio/vhost.h +30 -4

index 66be6afc88..39fbffc6bc 100644
@@ -107,9 +107,9 @@ struct vhost_dev {
     * future use should be discouraged and the variable retired as
     * its easy to confuse with the VirtIO backend_features.
     */
-    uint64_t features;
-    uint64_t acked_features;
-    uint64_t backend_features;
+    VIRTIO_DECLARE_FEATURES(features);
+    VIRTIO_DECLARE_FEATURES(acked_features);
+    VIRTIO_DECLARE_FEATURES(backend_features);
 
    /**
     * @protocol_features: is the vhost-user only feature set by

@@ -333,6 +333,21 @@ void vhost_virtqueue_mask(struct vhost_dev *hdev, VirtIODevice *vdev, int n,
 uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
                            uint64_t features);
 
+/**
+ * vhost_get_features_ex() - sanitize the extended features set
+ * @hdev: common vhost_dev structure
+ * @feature_bits: pointer to terminated table of feature bits
+ * @features: original features set, filtered out on return
+ *
+ * This is the extended variant of vhost_get_features(), supporting the
+ * the extended features set. Filter it with the intersection of what is
+ * supported by the vhost backend (hdev->features) and the supported
+ * feature_bits.
+ */
+void vhost_get_features_ex(struct vhost_dev *hdev,
+                           const int *feature_bits,
+                           uint64_t *features);
+
 /**
 * vhost_ack_features() - set vhost acked_features
 * @hdev: common vhost_dev structure

@@ -344,6 +359,18 @@ uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
 */
 void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
                        uint64_t features);
+
+/**
+ * vhost_ack_features_ex() - set vhost full set of acked_features
+ * @hdev: common vhost_dev structure
+ * @feature_bits: pointer to terminated table of feature bits
+ * @features: requested feature set
+ *
+ * This sets the internal hdev->acked_features to the intersection of
+ * the backends advertised features and the supported feature_bits.
+ */
+void vhost_ack_features_ex(struct vhost_dev *hdev, const int *feature_bits,
+                           const uint64_t *features);
 unsigned int vhost_get_max_memslots(void);
 unsigned int vhost_get_free_memslots(void);
 
--

2.50.0

Quoted 45 lines · Stefano Garzarella

Quoted 44 lines · Paolo Abeni

Similar to virtio infra, vhost core maintains the features status in the full extended format and allows the devices to implement extended version of the getter/setter.

Note that 'protocol_features' are not extended: they are only used by vhost-user, and the latter device is not going to implement extended features soon.

Signed-off-by Paolo Abeni <[email protected]>

---
v2 -> v3:
- fix compile warning
- _array -> _ex

v1 -> v2:
- uint128_t -> uint64_t[]
- add _ex() variant of features manipulation helpers
---

hw/virtio/vhost.c                 | 73 +++++++++++++++++++++++++++----
include/hw/virtio/vhost-backend.h |  6 +++
include/hw/virtio/vhost.h         | 33 ++++++++++++--
3 files changed, 100 insertions(+), 12 deletions(-)

hw/virtio/vhost.c +5 -3

index c30ea1156e..85ae1e4d4c 100644
@@ -972,20 +972,34 @@ static int vhost_virtqueue_set_addr(struct vhost_dev *dev,
 static int vhost_dev_set_features(struct vhost_dev *dev,
                                  bool enable_log)
 {
-    uint64_t features = dev->acked_features;
+    uint64_t features[VIRTIO_FEATURES_DWORDS];
    int r;
+
+    virtio_features_copy(features, dev->acked_features_ex);
    if (enable_log) {
-        features |= 0x1ULL << VHOST_F_LOG_ALL;
+        virtio_add_feature_ex(features, VHOST_F_LOG_ALL);
    }
    if (!vhost_dev_has_iommu(dev)) {
-        features &= ~(0x1ULL << VIRTIO_F_IOMMU_PLATFORM);
+        virtio_clear_feature_ex(features, VIRTIO_F_IOMMU_PLATFORM);
    }

Quoted 8 lines

hw/virtio/vhost.c static int vhost_virtqueue_set_addr(struct vhost_dev *dev, · line 990

Quoted 8 lines

hw/virtio/vhost.c +2 -2

    if (dev->vhost_ops->vhost_force_iommu) {
        if (dev->vhost_ops->vhost_force_iommu(dev) == true) {
-            features |= 0x1ULL << VIRTIO_F_IOMMU_PLATFORM;
+            virtio_add_feature_ex(features, VIRTIO_F_IOMMU_PLATFORM);
       }
    }
-    r = dev->vhost_ops->vhost_set_features(dev, features);
+

Hi Paolo

Quoted 6 lines

hw/virtio/vhost.c static int vhost_virtqueue_set_addr(struct vhost_dev *dev, · line 996

Quoted 6 lines

hw/virtio/vhost.c +6 -0

+    if (virtio_features_use_extended(features) &&
+        !dev->vhost_ops->vhost_set_features_ex) {
+        VHOST_OPS_DEBUG(r, "extended features without device support");
+        r = -EINVAL;
+        goto out;
+    }

As we discussed in version 2, this code should be changed to: [1], otherwise the problem mentioned last time will occur when compiling.

[1] if (virtio_features_use_extended(features) &&

         !dev->vhost_ops->vhost_set_features_ex) {

Quoted 8 lines · Paolo Abeni

hw/virtio/vhost.c +4 -0

@@ -1871,6 +1900,20 @@ uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
    return features;
 }
 
+void vhost_get_features_ex(struct vhost_dev *hdev,
+                           const int *feature_bits,
+                           uint64_t *features)
+{

hw/virtio/vhost.c uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits, · line 1916

Quoted 10 lines

hw/virtio/vhost.c +10 -0

+    const int *bit = feature_bits;
+
+    while (*bit != VHOST_INVALID_FEATURE_BIT) {
+        if (!virtio_has_feature_ex(hdev->features_ex, *bit)) {
+            virtio_clear_feature_ex(features, *bit);
+        }
+        bit++;
+    }
+}
+

Can we do something similar of what we do in hw/virtio/virtio.c where the old virtio_set_features() use the new virtio_set_features_ex()?

Quoted 9 lines

hw/virtio/vhost.c +2 -0

 void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
                        uint64_t features)
 {

@@ -1884,6 +1927,18 @@ void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
    }
 }
 
+void vhost_ack_features_ex(struct vhost_dev *hdev, const int *feature_bits,
+                           const uint64_t *features)

hw/virtio/vhost.c void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits, · line 1941

Quoted 10 lines

hw/virtio/vhost.c +10 -0

+{
+    const int *bit = feature_bits;
+    while (*bit != VHOST_INVALID_FEATURE_BIT) {
+        if (virtio_has_feature_ex(features, *bit)) {
+            virtio_add_feature_ex(hdev->acked_features_ex, *bit);
+        }
+        bit++;
+    }
+}
+

Ditto.

Not a strong opinion, but just to reduce code duplication.

Similar to virtio infra, vhost core maintains the features status in the full extended format and allows the devices to implement extended version of the getter/setter.

Note that 'protocol_features' are not extended: they are only used by vhost-user, and the latter device is not going to implement extended features soon.

Signed-off-by Paolo Abeni <[email protected]>

---
v2 -> v3:
- fix compile warning
- _array -> _ex

v1 -> v2:
- uint128_t -> uint64_t[]
- add _ex() variant of features manipulation helpers

---
 hw/virtio/vhost.c                 | 73 +++++++++++++++++++++++++++----
 include/hw/virtio/vhost-backend.h |  6 +++
 include/hw/virtio/vhost.h         | 33 ++++++++++++--
 3 files changed, 100 insertions(+), 12 deletions(-)

hw/virtio/vhost.c +23 -5

index c30ea1156e..85ae1e4d4c 100644
@@ -972,20 +972,34 @@ static int vhost_virtqueue_set_addr(struct vhost_dev *dev,
 static int vhost_dev_set_features(struct vhost_dev *dev,
                                   bool enable_log)
 {
-    uint64_t features = dev->acked_features;
+    uint64_t features[VIRTIO_FEATURES_DWORDS];
     int r;
+
+    virtio_features_copy(features, dev->acked_features_ex);
     if (enable_log) {
-        features |= 0x1ULL << VHOST_F_LOG_ALL;
+        virtio_add_feature_ex(features, VHOST_F_LOG_ALL);
     }
     if (!vhost_dev_has_iommu(dev)) {
-        features &= ~(0x1ULL << VIRTIO_F_IOMMU_PLATFORM);
+        virtio_clear_feature_ex(features, VIRTIO_F_IOMMU_PLATFORM);
     }
     if (dev->vhost_ops->vhost_force_iommu) {
         if (dev->vhost_ops->vhost_force_iommu(dev) == true) {
-            features |= 0x1ULL << VIRTIO_F_IOMMU_PLATFORM;
+            virtio_add_feature_ex(features, VIRTIO_F_IOMMU_PLATFORM);
        }
     }
-    r = dev->vhost_ops->vhost_set_features(dev, features);
+
+    if (virtio_features_use_extended(features) &&
+        !dev->vhost_ops->vhost_set_features_ex) {
+        VHOST_OPS_DEBUG(r, "extended features without device support");
+        r = -EINVAL;
+        goto out;
+    }
+
+    if (dev->vhost_ops->vhost_set_features_ex) {
+        r = dev->vhost_ops->vhost_set_features_ex(dev, features);
+    } else {
+        r = dev->vhost_ops->vhost_set_features(dev, features[0]);
+    }
     if (r < 0) {
         VHOST_OPS_DEBUG(r, "vhost_set_features failed");
         goto out;

@@ -1506,12 +1520,27 @@ static void vhost_virtqueue_cleanup(struct vhost_virtqueue *vq)
     }
 }
 
+static int vhost_dev_get_features(struct vhost_dev *hdev,
+                                  uint64_t *features)
+{
+    uint64_t features64;

Quoted 72 lines · Paolo Abeni

Similar to virtio infra, vhost core maintains the features status in the full extended format and allows the devices to implement extended version of the getter/setter.

Note that 'protocol_features' are not extended: they are only used by vhost-user, and the latter device is not going to implement extended features soon.

Signed-off-by Paolo Abeni <[email protected]>

---
v2 -> v3:
- fix compile warning
- _array -> _ex

v1 -> v2:
- uint128_t -> uint64_t[]
- add _ex() variant of features manipulation helpers

---
 hw/virtio/vhost.c                 | 73 +++++++++++++++++++++++++++----
 include/hw/virtio/vhost-backend.h |  6 +++
 include/hw/virtio/vhost.h         | 33 ++++++++++++--
 3 files changed, 100 insertions(+), 12 deletions(-)

hw/virtio/vhost.c +19 -5

index c30ea1156e..85ae1e4d4c 100644
@@ -972,20 +972,34 @@ static int vhost_virtqueue_set_addr(struct vhost_dev *dev,
 static int vhost_dev_set_features(struct vhost_dev *dev,
                                   bool enable_log)
 {
-    uint64_t features = dev->acked_features;
+    uint64_t features[VIRTIO_FEATURES_DWORDS];
     int r;
+
+    virtio_features_copy(features, dev->acked_features_ex);
     if (enable_log) {
-        features |= 0x1ULL << VHOST_F_LOG_ALL;
+        virtio_add_feature_ex(features, VHOST_F_LOG_ALL);
     }
     if (!vhost_dev_has_iommu(dev)) {
-        features &= ~(0x1ULL << VIRTIO_F_IOMMU_PLATFORM);
+        virtio_clear_feature_ex(features, VIRTIO_F_IOMMU_PLATFORM);
     }
     if (dev->vhost_ops->vhost_force_iommu) {
         if (dev->vhost_ops->vhost_force_iommu(dev) == true) {
-            features |= 0x1ULL << VIRTIO_F_IOMMU_PLATFORM;
+            virtio_add_feature_ex(features, VIRTIO_F_IOMMU_PLATFORM);
        }
     }
-    r = dev->vhost_ops->vhost_set_features(dev, features);
+
+    if (virtio_features_use_extended(features) &&
+        !dev->vhost_ops->vhost_set_features_ex) {
+        VHOST_OPS_DEBUG(r, "extended features without device support");
+        r = -EINVAL;
+        goto out;
+    }
+
+    if (dev->vhost_ops->vhost_set_features_ex) {
+        r = dev->vhost_ops->vhost_set_features_ex(dev, features);
+    } else {
+        r = dev->vhost_ops->vhost_set_features(dev, features[0]);
+    }
     if (r < 0) {
         VHOST_OPS_DEBUG(r, "vhost_set_features failed");
         goto out;

@@ -1506,12 +1520,27 @@ static void vhost_virtqueue_cleanup(struct vhost_virtqueue *vq)
     }
 }