Re: [PATCH RFC v2 03/13] virtio: introduce extended features type

[Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>]

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;