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 <pab...@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
