Use the extended types and helpers to manipulate the virtio_net
features.
Note that offloads are still 64bits wide, as per specification,
and extended offloads will be mapped into such range.
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
hw/net/virtio-net.c | 87 +++++++++++++++++++++-------------
include/hw/virtio/virtio-net.h | 2 +-
2 files changed, 55 insertions(+), 34 deletions(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 9f500c64e7..193469fc27 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -90,6 +90,17 @@
VIRTIO_NET_RSS_HASH_TYPE_TCP_EX | \
VIRTIO_NET_RSS_HASH_TYPE_UDP_EX)
+#define VIRTIO_OFFLOAD_MAP_MIN 46
+#define VIRTIO_OFFLOAD_MAP_LENGTH 4
+#define VIRTIO_OFFLOAD_MAP MAKE_64BIT_MASK(VIRTIO_OFFLOAD_MAP_MIN, \
+ VIRTIO_OFFLOAD_MAP_LENGTH)
+#define VIRTIO_FEATURES_MAP_MIN 65
+#define VIRTIO_O2F_DELTA (VIRTIO_FEATURES_MAP_MIN - \
+ VIRTIO_OFFLOAD_MAP_MIN)
+
+#define VIRTIO_FEATURE_TO_OFFLOAD(fbit) (fbit >= 64 ? \
+ fbit - VIRTIO_O2F_DELTA : fbit)
+
static const VirtIOFeature feature_sizes[] = {
{.flags = 1ULL << VIRTIO_NET_F_MAC,
.end = endof(struct virtio_net_config, mac)},
@@ -751,44 +762,45 @@ static void virtio_net_set_queue_pairs(VirtIONet *n)
static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue);
-static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
- Error **errp)
+static virtio_features_t virtio_net_get_features(VirtIODevice *vdev,
+ virtio_features_t features,
+ Error **errp)
{
VirtIONet *n = VIRTIO_NET(vdev);
NetClientState *nc = qemu_get_queue(n->nic);
/* Firstly sync all virtio-net possible supported features */
- features |= n->host_features;
+ features |= n->host_features_ex;
- virtio_add_feature(&features, VIRTIO_NET_F_MAC);
+ virtio_add_feature_ex(&features, VIRTIO_NET_F_MAC);
if (!peer_has_vnet_hdr(n)) {
- virtio_clear_feature(&features, VIRTIO_NET_F_CSUM);
- virtio_clear_feature(&features, VIRTIO_NET_F_HOST_TSO4);
- virtio_clear_feature(&features, VIRTIO_NET_F_HOST_TSO6);
- virtio_clear_feature(&features, VIRTIO_NET_F_HOST_ECN);
+ virtio_clear_feature_ex(&features, VIRTIO_NET_F_CSUM);
+ virtio_clear_feature_ex(&features, VIRTIO_NET_F_HOST_TSO4);
+ virtio_clear_feature_ex(&features, VIRTIO_NET_F_HOST_TSO6);
+ virtio_clear_feature_ex(&features, VIRTIO_NET_F_HOST_ECN);
- virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_CSUM);
- virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_TSO4);
- virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_TSO6);
- virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_ECN);
+ virtio_clear_feature_ex(&features, VIRTIO_NET_F_GUEST_CSUM);
+ virtio_clear_feature_ex(&features, VIRTIO_NET_F_GUEST_TSO4);
+ virtio_clear_feature_ex(&features, VIRTIO_NET_F_GUEST_TSO6);
+ virtio_clear_feature_ex(&features, VIRTIO_NET_F_GUEST_ECN);
- virtio_clear_feature(&features, VIRTIO_NET_F_HOST_USO);
- virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_USO4);
- virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_USO6);
+ virtio_clear_feature_ex(&features, VIRTIO_NET_F_HOST_USO);
+ virtio_clear_feature_ex(&features, VIRTIO_NET_F_GUEST_USO4);
+ virtio_clear_feature_ex(&features, VIRTIO_NET_F_GUEST_USO6);
- virtio_clear_feature(&features, VIRTIO_NET_F_HASH_REPORT);
+ virtio_clear_feature_ex(&features, VIRTIO_NET_F_HASH_REPORT);
}
if (!peer_has_vnet_hdr(n) || !peer_has_ufo(n)) {
- virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_UFO);
- virtio_clear_feature(&features, VIRTIO_NET_F_HOST_UFO);
+ virtio_clear_feature_ex(&features, VIRTIO_NET_F_GUEST_UFO);
+ virtio_clear_feature_ex(&features, VIRTIO_NET_F_HOST_UFO);
}
if (!peer_has_uso(n)) {
- virtio_clear_feature(&features, VIRTIO_NET_F_HOST_USO);
- virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_USO4);
- virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_USO6);
+ virtio_clear_feature_ex(&features, VIRTIO_NET_F_HOST_USO);
+ virtio_clear_feature_ex(&features, VIRTIO_NET_F_GUEST_USO4);
+ virtio_clear_feature_ex(&features, VIRTIO_NET_F_GUEST_USO6);
}
if (!get_vhost_net(nc->peer)) {
@@ -796,7 +808,7 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
}
if (!ebpf_rss_is_loaded(&n->ebpf_rss)) {
- virtio_clear_feature(&features, VIRTIO_NET_F_RSS);
+ virtio_clear_feature_ex(&features, VIRTIO_NET_F_RSS);
}
features = vhost_net_get_features(get_vhost_net(nc->peer), features);
vdev->backend_features_ex = features;
@@ -818,7 +830,7 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
* support it.
*/
if (!virtio_has_feature(vdev->backend_features, VIRTIO_NET_F_CTRL_VQ)) {
- virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_ANNOUNCE);
+ virtio_clear_feature_ex(&features, VIRTIO_NET_F_GUEST_ANNOUNCE);
}
return features;
@@ -851,9 +863,16 @@ static void virtio_net_apply_guest_offloads(VirtIONet *n)
!!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_USO6)));
}
-static uint64_t virtio_net_guest_offloads_by_features(uint64_t features)
+static uint64_t virtio_net_features_to_offload(virtio_features_t features)
+{
+ return (features & ~VIRTIO_OFFLOAD_MAP) |
+ ((features >> VIRTIO_O2F_DELTA) & VIRTIO_OFFLOAD_MAP);
+}
+
+static uint64_t
+virtio_net_guest_offloads_by_features(virtio_features_t features)
{
- static const uint64_t guest_offloads_mask =
+ static const virtio_features_t guest_offloads_mask =
(1ULL << VIRTIO_NET_F_GUEST_CSUM) |
(1ULL << VIRTIO_NET_F_GUEST_TSO4) |
(1ULL << VIRTIO_NET_F_GUEST_TSO6) |
@@ -862,13 +881,13 @@ static uint64_t virtio_net_guest_offloads_by_features(uint64_t features)
(1ULL << VIRTIO_NET_F_GUEST_USO4) |
(1ULL << VIRTIO_NET_F_GUEST_USO6);
- return guest_offloads_mask & features;
+ return guest_offloads_mask & virtio_net_features_to_offload(features);
}
uint64_t virtio_net_supported_guest_offloads(const VirtIONet *n)
{
VirtIODevice *vdev = VIRTIO_DEVICE(n);
- return virtio_net_guest_offloads_by_features(vdev->guest_features);
+ return virtio_net_guest_offloads_by_features(vdev->guest_features_ex);
}
typedef struct {
@@ -947,7 +966,8 @@ static void failover_add_primary(VirtIONet *n, Error **errp)
error_propagate(errp, err);
}
-static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
+static void virtio_net_set_features(VirtIODevice *vdev,
+ virtio_features_t features)
{
VirtIONet *n = VIRTIO_NET(vdev);
Error *err = NULL;
@@ -955,7 +975,7 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
if (n->mtu_bypass_backend &&
!virtio_has_feature(vdev->backend_features, VIRTIO_NET_F_MTU)) {
- features &= ~(1ULL << VIRTIO_NET_F_MTU);
+ features &= ~VIRTIO_BIT(VIRTIO_NET_F_MTU);
}
virtio_net_set_multiqueue(n,
@@ -1962,10 +1982,11 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
virtio_error(vdev, "virtio-net unexpected empty queue: "
"i %zd mergeable %d offset %zd, size %zd, "
"guest hdr len %zd, host hdr len %zd "
- "guest features 0x%" PRIx64,
+ "guest features 0x" VIRTIO_FEATURES_FMT,
i, n->mergeable_rx_bufs, offset, size,
n->guest_hdr_len, n->host_hdr_len,
- vdev->guest_features);
+ VIRTIO_FEATURES_HI(vdev->guest_features_ex),
+ VIRTIO_FEATURES_LOW(vdev->guest_features_ex));
}
err = -1;
goto err;
@@ -4146,8 +4167,8 @@ static void virtio_net_class_init(ObjectClass *klass, const void *data)
vdc->unrealize = virtio_net_device_unrealize;
vdc->get_config = virtio_net_get_config;
vdc->set_config = virtio_net_set_config;
- vdc->get_features = virtio_net_get_features;
- vdc->set_features = virtio_net_set_features;
+ vdc->get_features_ex = virtio_net_get_features;
+ vdc->set_features_ex = virtio_net_set_features;
vdc->bad_features = virtio_net_bad_features;
vdc->reset = virtio_net_reset;
vdc->queue_reset = virtio_net_queue_reset;
diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
index b9ea9e824e..5ccdbeb253 100644
--- a/include/hw/virtio/virtio-net.h
+++ b/include/hw/virtio/virtio-net.h
@@ -178,7 +178,7 @@ struct VirtIONet {
uint32_t has_vnet_hdr;
size_t host_hdr_len;
size_t guest_hdr_len;
- uint64_t host_features;
+ DECLARE_FEATURES(host_features);
uint32_t rsc_timeout;
uint8_t rsc4_enabled;
uint8_t rsc6_enabled;
--
2.49.0On 2025/05/21 20:34, Paolo Abeni wrote:
> Use the extended types and helpers to manipulate the virtio_net
> features.
>
> Note that offloads are still 64bits wide, as per specification,
> and extended offloads will be mapped into such range.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> hw/net/virtio-net.c | 87 +++++++++++++++++++++-------------
> include/hw/virtio/virtio-net.h | 2 +-
> 2 files changed, 55 insertions(+), 34 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 9f500c64e7..193469fc27 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -90,6 +90,17 @@
> VIRTIO_NET_RSS_HASH_TYPE_TCP_EX | \
> VIRTIO_NET_RSS_HASH_TYPE_UDP_EX)
>
> +#define VIRTIO_OFFLOAD_MAP_MIN 46
> +#define VIRTIO_OFFLOAD_MAP_LENGTH 4
> +#define VIRTIO_OFFLOAD_MAP MAKE_64BIT_MASK(VIRTIO_OFFLOAD_MAP_MIN, \
> + VIRTIO_OFFLOAD_MAP_LENGTH)
> +#define VIRTIO_FEATURES_MAP_MIN 65
> +#define VIRTIO_O2F_DELTA (VIRTIO_FEATURES_MAP_MIN - \
> + VIRTIO_OFFLOAD_MAP_MIN)
> +
> +#define VIRTIO_FEATURE_TO_OFFLOAD(fbit) (fbit >= 64 ? \
> + fbit - VIRTIO_O2F_DELTA : fbit)
> +
These are specific to virtio-net but look like they are common for
virtio as the names don't contain "NET".
VIRTIO_FEATURES_MAP_MIN is also a bit confusing. It points to the least
significant bit that refers to an offloading feature in the upper-half
of the feature bits, but the name lacks the context.
> static const VirtIOFeature feature_sizes[] = {
> {.flags = 1ULL << VIRTIO_NET_F_MAC,
> .end = endof(struct virtio_net_config, mac)},
> @@ -751,44 +762,45 @@ static void virtio_net_set_queue_pairs(VirtIONet *n)
>
> static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue);
>
> -static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
> - Error **errp)
> +static virtio_features_t virtio_net_get_features(VirtIODevice *vdev,
> + virtio_features_t features,
> + Error **errp)
> {
> VirtIONet *n = VIRTIO_NET(vdev);
> NetClientState *nc = qemu_get_queue(n->nic);
>
> /* Firstly sync all virtio-net possible supported features */
> - features |= n->host_features;
> + features |= n->host_features_ex;
>
> - virtio_add_feature(&features, VIRTIO_NET_F_MAC);
> + virtio_add_feature_ex(&features, VIRTIO_NET_F_MAC);
>
> if (!peer_has_vnet_hdr(n)) {
> - virtio_clear_feature(&features, VIRTIO_NET_F_CSUM);
> - virtio_clear_feature(&features, VIRTIO_NET_F_HOST_TSO4);
> - virtio_clear_feature(&features, VIRTIO_NET_F_HOST_TSO6);
> - virtio_clear_feature(&features, VIRTIO_NET_F_HOST_ECN);
> + virtio_clear_feature_ex(&features, VIRTIO_NET_F_CSUM);
> + virtio_clear_feature_ex(&features, VIRTIO_NET_F_HOST_TSO4);
> + virtio_clear_feature_ex(&features, VIRTIO_NET_F_HOST_TSO6);
> + virtio_clear_feature_ex(&features, VIRTIO_NET_F_HOST_ECN);
>
> - virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_CSUM);
> - virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_TSO4);
> - virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_TSO6);
> - virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_ECN);
> + virtio_clear_feature_ex(&features, VIRTIO_NET_F_GUEST_CSUM);
> + virtio_clear_feature_ex(&features, VIRTIO_NET_F_GUEST_TSO4);
> + virtio_clear_feature_ex(&features, VIRTIO_NET_F_GUEST_TSO6);
> + virtio_clear_feature_ex(&features, VIRTIO_NET_F_GUEST_ECN);
>
> - virtio_clear_feature(&features, VIRTIO_NET_F_HOST_USO);
> - virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_USO4);
> - virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_USO6);
> + virtio_clear_feature_ex(&features, VIRTIO_NET_F_HOST_USO);
> + virtio_clear_feature_ex(&features, VIRTIO_NET_F_GUEST_USO4);
> + virtio_clear_feature_ex(&features, VIRTIO_NET_F_GUEST_USO6);
>
> - virtio_clear_feature(&features, VIRTIO_NET_F_HASH_REPORT);
> + virtio_clear_feature_ex(&features, VIRTIO_NET_F_HASH_REPORT);
> }
>
> if (!peer_has_vnet_hdr(n) || !peer_has_ufo(n)) {
> - virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_UFO);
> - virtio_clear_feature(&features, VIRTIO_NET_F_HOST_UFO);
> + virtio_clear_feature_ex(&features, VIRTIO_NET_F_GUEST_UFO);
> + virtio_clear_feature_ex(&features, VIRTIO_NET_F_HOST_UFO);
> }
>
> if (!peer_has_uso(n)) {
> - virtio_clear_feature(&features, VIRTIO_NET_F_HOST_USO);
> - virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_USO4);
> - virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_USO6);
> + virtio_clear_feature_ex(&features, VIRTIO_NET_F_HOST_USO);
> + virtio_clear_feature_ex(&features, VIRTIO_NET_F_GUEST_USO4);
> + virtio_clear_feature_ex(&features, VIRTIO_NET_F_GUEST_USO6);
> }
>
> if (!get_vhost_net(nc->peer)) {
> @@ -796,7 +808,7 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
> }
>
> if (!ebpf_rss_is_loaded(&n->ebpf_rss)) {
> - virtio_clear_feature(&features, VIRTIO_NET_F_RSS);
> + virtio_clear_feature_ex(&features, VIRTIO_NET_F_RSS);
> }
> features = vhost_net_get_features(get_vhost_net(nc->peer), features);
> vdev->backend_features_ex = features;
> @@ -818,7 +830,7 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
> * support it.
> */
> if (!virtio_has_feature(vdev->backend_features, VIRTIO_NET_F_CTRL_VQ)) {
> - virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_ANNOUNCE);
> + virtio_clear_feature_ex(&features, VIRTIO_NET_F_GUEST_ANNOUNCE);
> }
>
> return features;
> @@ -851,9 +863,16 @@ static void virtio_net_apply_guest_offloads(VirtIONet *n)
> !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_USO6)));
> }
>
> -static uint64_t virtio_net_guest_offloads_by_features(uint64_t features)
> +static uint64_t virtio_net_features_to_offload(virtio_features_t features)
> +{> + return (features & ~VIRTIO_OFFLOAD_MAP) |
> + ((features >> VIRTIO_O2F_DELTA) & VIRTIO_OFFLOAD_MAP);
> +}
> +
> +static uint64_t
> +virtio_net_guest_offloads_by_features(virtio_features_t features)
> {
> - static const uint64_t guest_offloads_mask =
> + static const virtio_features_t guest_offloads_mask =
> (1ULL << VIRTIO_NET_F_GUEST_CSUM) |
> (1ULL << VIRTIO_NET_F_GUEST_TSO4) |
> (1ULL << VIRTIO_NET_F_GUEST_TSO6) |
> @@ -862,13 +881,13 @@ static uint64_t virtio_net_guest_offloads_by_features(uint64_t features)
> (1ULL << VIRTIO_NET_F_GUEST_USO4) |
> (1ULL << VIRTIO_NET_F_GUEST_USO6);
>
> - return guest_offloads_mask & features;
> + return guest_offloads_mask & virtio_net_features_to_offload(features);
How about:
static const virtio_features_t guest_offload_features_mask = ...
virtio_features_t masked_features = guest_offload_features_mask & features;
return masked_features | ((masked_features >> VIRTIO_FEATURES_MAP_MIN)
<< VIRTIO_OFFLOAD_MAP_MIN);
This makes virtio_net_features_to_offload() unnecessary.
> }
>
> uint64_t virtio_net_supported_guest_offloads(const VirtIONet *n)
> {
> VirtIODevice *vdev = VIRTIO_DEVICE(n);
> - return virtio_net_guest_offloads_by_features(vdev->guest_features);
> + return virtio_net_guest_offloads_by_features(vdev->guest_features_ex);
> }
>
> typedef struct {
> @@ -947,7 +966,8 @@ static void failover_add_primary(VirtIONet *n, Error **errp)
> error_propagate(errp, err);
> }
>
> -static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
> +static void virtio_net_set_features(VirtIODevice *vdev,
> + virtio_features_t features)
> {
> VirtIONet *n = VIRTIO_NET(vdev);
> Error *err = NULL;
> @@ -955,7 +975,7 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
>
> if (n->mtu_bypass_backend &&
> !virtio_has_feature(vdev->backend_features, VIRTIO_NET_F_MTU)) {
> - features &= ~(1ULL << VIRTIO_NET_F_MTU);
> + features &= ~VIRTIO_BIT(VIRTIO_NET_F_MTU);
> }
>
> virtio_net_set_multiqueue(n,
> @@ -1962,10 +1982,11 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
> virtio_error(vdev, "virtio-net unexpected empty queue: "
> "i %zd mergeable %d offset %zd, size %zd, "
> "guest hdr len %zd, host hdr len %zd "
> - "guest features 0x%" PRIx64,
> + "guest features 0x" VIRTIO_FEATURES_FMT,
> i, n->mergeable_rx_bufs, offset, size,
> n->guest_hdr_len, n->host_hdr_len,
> - vdev->guest_features);
> + VIRTIO_FEATURES_HI(vdev->guest_features_ex),
> + VIRTIO_FEATURES_LOW(vdev->guest_features_ex));
> }
> err = -1;
> goto err;
> @@ -4146,8 +4167,8 @@ static void virtio_net_class_init(ObjectClass *klass, const void *data)
> vdc->unrealize = virtio_net_device_unrealize;
> vdc->get_config = virtio_net_get_config;
> vdc->set_config = virtio_net_set_config;
> - vdc->get_features = virtio_net_get_features;
> - vdc->set_features = virtio_net_set_features;
> + vdc->get_features_ex = virtio_net_get_features;
> + vdc->set_features_ex = virtio_net_set_features;
> vdc->bad_features = virtio_net_bad_features;
> vdc->reset = virtio_net_reset;
> vdc->queue_reset = virtio_net_queue_reset;
> diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> index b9ea9e824e..5ccdbeb253 100644
> --- a/include/hw/virtio/virtio-net.h
> +++ b/include/hw/virtio/virtio-net.h
> @@ -178,7 +178,7 @@ struct VirtIONet {
> uint32_t has_vnet_hdr;
> size_t host_hdr_len;
> size_t guest_hdr_len;
> - uint64_t host_features;
> + DECLARE_FEATURES(host_features);
> uint32_t rsc_timeout;
> uint8_t rsc4_enabled;
> uint8_t rsc6_enabled;
On 5/23/25 10:09 AM, Akihiko Odaki wrote: > On 2025/05/21 20:34, Paolo Abeni wrote: >> Use the extended types and helpers to manipulate the virtio_net >> features. >> >> Note that offloads are still 64bits wide, as per specification, >> and extended offloads will be mapped into such range. >> >> Signed-off-by: Paolo Abeni <pabeni@redhat.com> >> --- >> hw/net/virtio-net.c | 87 +++++++++++++++++++++------------- >> include/hw/virtio/virtio-net.h | 2 +- >> 2 files changed, 55 insertions(+), 34 deletions(-) >> >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c >> index 9f500c64e7..193469fc27 100644 >> --- a/hw/net/virtio-net.c >> +++ b/hw/net/virtio-net.c >> @@ -90,6 +90,17 @@ >> VIRTIO_NET_RSS_HASH_TYPE_TCP_EX | \ >> VIRTIO_NET_RSS_HASH_TYPE_UDP_EX) >> >> +#define VIRTIO_OFFLOAD_MAP_MIN 46 >> +#define VIRTIO_OFFLOAD_MAP_LENGTH 4 >> +#define VIRTIO_OFFLOAD_MAP MAKE_64BIT_MASK(VIRTIO_OFFLOAD_MAP_MIN, \ >> + VIRTIO_OFFLOAD_MAP_LENGTH) >> +#define VIRTIO_FEATURES_MAP_MIN 65 >> +#define VIRTIO_O2F_DELTA (VIRTIO_FEATURES_MAP_MIN - \ >> + VIRTIO_OFFLOAD_MAP_MIN) >> + >> +#define VIRTIO_FEATURE_TO_OFFLOAD(fbit) (fbit >= 64 ? \ >> + fbit - VIRTIO_O2F_DELTA : fbit) >> + > > These are specific to virtio-net but look like they are common for > virtio as the names don't contain "NET". > > VIRTIO_FEATURES_MAP_MIN is also a bit confusing. It points to the least > significant bit that refers to an offloading feature in the upper-half > of the feature bits, but the name lacks the context. Uhmmm... putting the whole context in the macro name sounds very verbose and/or hard, what about: How about VIRTIO_NET_OFFLOAD_MAPPED_MIN ? > @@ -862,13 +881,13 @@ static uint64_t virtio_net_guest_offloads_by_features(uint64_t features) >> (1ULL << VIRTIO_NET_F_GUEST_USO4) | >> (1ULL << VIRTIO_NET_F_GUEST_USO6); >> >> - return guest_offloads_mask & features; >> + return guest_offloads_mask & virtio_net_features_to_offload(features); > > > How about: > > static const virtio_features_t guest_offload_features_mask = ... > virtio_features_t masked_features = guest_offload_features_mask & features; > > return masked_features | ((masked_features >> VIRTIO_FEATURES_MAP_MIN) > << VIRTIO_OFFLOAD_MAP_MIN); > > This makes virtio_net_features_to_offload() unnecessary. The above looks a little fragile, as (in future) 'features' could have some bit in the mapped range set (and not representing a guest offload): we need to explicitly mask such bits out before the first '&' operator. If dropping the helper is preferred, it can still be dropped. /P
On 2025/05/23 19:01, Paolo Abeni wrote: > On 5/23/25 10:09 AM, Akihiko Odaki wrote: >> On 2025/05/21 20:34, Paolo Abeni wrote: >>> Use the extended types and helpers to manipulate the virtio_net >>> features. >>> >>> Note that offloads are still 64bits wide, as per specification, >>> and extended offloads will be mapped into such range. >>> >>> Signed-off-by: Paolo Abeni <pabeni@redhat.com> >>> --- >>> hw/net/virtio-net.c | 87 +++++++++++++++++++++------------- >>> include/hw/virtio/virtio-net.h | 2 +- >>> 2 files changed, 55 insertions(+), 34 deletions(-) >>> >>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c >>> index 9f500c64e7..193469fc27 100644 >>> --- a/hw/net/virtio-net.c >>> +++ b/hw/net/virtio-net.c >>> @@ -90,6 +90,17 @@ >>> VIRTIO_NET_RSS_HASH_TYPE_TCP_EX | \ >>> VIRTIO_NET_RSS_HASH_TYPE_UDP_EX) >>> >>> +#define VIRTIO_OFFLOAD_MAP_MIN 46 >>> +#define VIRTIO_OFFLOAD_MAP_LENGTH 4 >>> +#define VIRTIO_OFFLOAD_MAP MAKE_64BIT_MASK(VIRTIO_OFFLOAD_MAP_MIN, \ >>> + VIRTIO_OFFLOAD_MAP_LENGTH) >>> +#define VIRTIO_FEATURES_MAP_MIN 65 >>> +#define VIRTIO_O2F_DELTA (VIRTIO_FEATURES_MAP_MIN - \ >>> + VIRTIO_OFFLOAD_MAP_MIN) >>> + >>> +#define VIRTIO_FEATURE_TO_OFFLOAD(fbit) (fbit >= 64 ? \ >>> + fbit - VIRTIO_O2F_DELTA : fbit) >>> + >> >> These are specific to virtio-net but look like they are common for >> virtio as the names don't contain "NET". >> >> VIRTIO_FEATURES_MAP_MIN is also a bit confusing. It points to the least >> significant bit that refers to an offloading feature in the upper-half >> of the feature bits, but the name lacks the context. > > Uhmmm... putting the whole context in the macro name sounds very verbose > and/or hard, what about: > > How about VIRTIO_NET_OFFLOAD_MAPPED_MIN > > ? It looks like it represents a bit in the 64-bit mapping (VIRTIO_OFFLOAD_MAP) instead of a feature bit as it contains "MAPPED" while it doesn't contain "FEATURE". Perhaps VIRTIO_OFFLOAD_MAP is the one that is confusing. As it is intended to be a compact 64-bit representation, how about: VIRTIO_OFFLOAD_MAP -> VIRTIO_NET_OFFLOAD64 VIRTIO_FEATURE_TO_OFFLOAD -> VIRTIO_NET_FEATURE_TO_OFFLOAD64 VIRTIO_FEATURES_MAP_MIN -> VIRTIO_NET_OFFLOAD_FEATURE_MIN > >> @@ -862,13 +881,13 @@ static uint64_t virtio_net_guest_offloads_by_features(uint64_t features) >>> (1ULL << VIRTIO_NET_F_GUEST_USO4) | >>> (1ULL << VIRTIO_NET_F_GUEST_USO6); >>> >>> - return guest_offloads_mask & features; >>> + return guest_offloads_mask & virtio_net_features_to_offload(features); >> >> >> How about: >> >> static const virtio_features_t guest_offload_features_mask = ... >> virtio_features_t masked_features = guest_offload_features_mask & features; >> >> return masked_features | ((masked_features >> VIRTIO_FEATURES_MAP_MIN) >> << VIRTIO_OFFLOAD_MAP_MIN); >> >> This makes virtio_net_features_to_offload() unnecessary. > > The above looks a little fragile, as (in future) 'features' could have > some bit in the mapped range set (and not representing a guest offload): > we need to explicitly mask such bits out before the first '&' operator. My suggestion is to mask all feature bits that don't represent guest offloading first. masked_features should only contain guest offload features. Regards, Akihiko Odaki
© 2016 - 2025 Red Hat, Inc.