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

[Paolo Abeni <pabeni@redhat.com>]

On 7/21/25 9:49 AM, Akihiko Odaki wrote:
> On 2025/07/21 16:33, Paolo Abeni wrote:
>> 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
> 
> U64 and U32 should be plural (i.e., rename them into 
> VIRTIO_U64S_PER_FEATURES and VIRTIO_U32S_PER_FEATURES)
> 
> "PER_FEATURES" also sounds a bit awkward; BITS_PER_BYTE and 
> BITS_PER_LONG had singular after "per" so the unit was clear, but it is 
> not in this case.
> 
> I could think of several options:
> - VIRTIO_U64S_PER_FEATURES (what you proposed + plural U64S)
> - VIRTIO_FEATURES_U64S (closer to the previous version)
> - VIRTIO_FEATURES_NU64S (like CPU_TEMP_BUF_NLONGS)
> - VIRTIO_U64S_PER_FEATURE_BITMASK
> 
> They have downsides and upsides, and I don't have an idea what's the best.

Indeed an optimal name here could be impossible to fit. IMHO
VIRTIO_FEATURES_NU64S is the best option among the above and I like it.

I'll use that one.

Thanks,

Paolo