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

[Paolo Abeni <pabeni@redhat.com>]

On 7/20/25 12:41 PM, Akihiko Odaki wrote:
> On 2025/07/18 17:52, Paolo Abeni wrote:
>> diff --git a/include/hw/virtio/virtio-features.h b/include/hw/virtio/virtio-features.h
>> new file mode 100644
>> index 0000000000..68e326e3e8
>> --- /dev/null
>> +++ b/include/hw/virtio/virtio-features.h
>> @@ -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 "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/aab8c434-364e-4305-9d8b-943eb0c98406@rsg.ci.i.u-tokyo.ac.jp/
> 
> 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
	VIRTIO_DWORD ->  VIRTIO_FEATURES_U64
(imho VIRTIO_U64 would sound ambiguous)
        VIRTIO_BIT -> VIRTIO_FEATURES_BIT
(for consistency with the previous one)

>> +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.

Ok.

Thanks,

Paolo