On 2025/07/11 22:02, Paolo Abeni wrote:
> 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 (above 64) 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 <pabeni@redhat.com>
> ---
> 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 | 124 ++++++++++++++++++++++++++++
> include/hw/virtio/virtio.h | 7 +-
> 2 files changed, 128 insertions(+), 3 deletions(-)
> create mode 100644 include/hw/virtio/virtio-features.h
>
> diff --git a/include/hw/virtio/virtio-features.h b/include/hw/virtio/virtio-features.h
> new file mode 100644
> index 0000000000..cc735f7f81
> --- /dev/null
> +++ b/include/hw/virtio/virtio-features.h
> @@ -0,0 +1,124 @@
> +/*
> + * 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
An identifier prefixed with an underscore and a capital letter is
reserved and better to avoid. Let's also keep consistent with QEMU_VIRTIO_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) (1ULL << (b & 0x3f))
Let's use BIT_ULL().
I also think (b % 64) is easier to understand than (b & 0x3f); you may
refer to BIT_MASK() and BIT_WORD() as prior examples.
> +#define VIRTIO_DWORD(b) ((b) >> 6)
VIRTIO_FEATURES_PR() and VIRTIO_BIT() should have parentheses around
their parameters as VIRTIO_DWORD() does.
> +#define VIRTIO_FEATURES_WORDS (VIRTIO_FEATURES_MAX >> 5)
This macro is misaligned; personally I prefer just use one whitespace to
delimit the macro name and value to avoid the trouble of aligning them
and polluting "git blame".
> +#define VIRTIO_FEATURES_DWORDS (VIRTIO_FEATURES_WORDS >> 1)
> +
> +#define VIRTIO_DECLARE_FEATURES(name) \
> + union { \
> + uint64_t name; \
> + uint64_t name##_array[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);
> + features[VIRTIO_DWORD(fbit)] &= ~VIRTIO_BIT(fbit);
> +}
> +
> +static inline bool virtio_features_equal(const uint64_t *f1,
> + const uint64_t *f2)
> +{
> + uint64_t diff = 0;
> + int i;
> +
> + for (i = 0; i < VIRTIO_FEATURES_DWORDS; ++i) {
> + diff |= f1[i] ^ f2[i];
> + }
> + return !!diff;
Let's use memcmp().
> +}
> +
> +static inline bool virtio_features_use_extended(const uint64_t *features)
> +{
> + int i;
> +
> + for (i = 1; i < VIRTIO_FEATURES_DWORDS; ++i) {
> + if (features[i]) {
> + return true;
> + }
> + }
> + return false;
> +}
> +
> +static inline bool virtio_features_is_empty(const uint64_t *features)
Let's follow bitmap_empty() and omit "is".
> +{
> + 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 void virtio_features_andnot(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_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
> +
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 214d4a77e9..0d1eb20489 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -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;