Re: [PATCH RFC v2 04/13] virtio: serialize extended features state

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

On 2025/07/16 0:40, Paolo Abeni wrote:
> On 7/15/25 9:24 AM, Akihiko Odaki wrote:
>> On 2025/07/11 22:02, Paolo Abeni wrote:
>>> +     */
>>> +    QEMU_BUILD_BUG_ON(VIRTIO_FEATURES_DWORDS != 2);
>>> +    if (virtio_128bit_features_needed(vdev)) {
>>
>> There is no need to distinguish virtio_128bit_features_needed() and
>> virtio_64bit_features_needed() here.
> 
> Double checking I'm reading the above correctly. Are you suggesting to
> replace this chunk with something alike:
> 
>      if (virtio_64bit_features_needed(vdev)) {

This condition is not right as virtio_64bit_features_needed() doesn't 
return true when the some of bits [64, 128) is set while bits [32, 64) 
are cleared. I see two options to fix:

- Check: virtio_64bit_features_needed(vdev) ||
          virtio_128bit_features_needed(vdev)

- Ensure that virtio_64bit_features_needed(vdev) returns true when a bit 
more significant than bit 31 is set.

>          /* The 64 highest bit has been cleared by the previous
>           *  virtio_features_from_u64() and ev.
>           * initialized as needed when loading
>           * "virtio/128bit_features"*/
>          uint64_t *val = vdev->guest_features_array;
> 
>          if (virtio_set_128bit_features_nocheck_maybe_co(vdev, val) < 0)
> // ...> >> For the 32-bit case, it will be simpler to have an array here and use
>> virtio_set_128bit_features_nocheck_maybe_co() instead of having
>> virtio_set_features_nocheck_maybe_co().
> 
> Again double checking I'm parsing the above correctly. You are
> suggesting to dismiss the  virtio_set_features_nocheck_maybe_co() helper
> entirely and use virtio_set_128bit_features_nocheck_maybe_co() even when
> only 32bit features are loaded. Am I correct?

Yes, but now I found it is unnecessary to special-case even the 32-bit case.

Commit 019a3edbb25f ("virtio: make features 64bit wide") had to add a 
conditional to distinguish the 64-bit and 32-bit cases because 
vdev->guest_features was not set before executing this part of code.

However, commit 62cee1a28aad ("virtio: set low features early on load") 
later added preceding code to set vdev->guest_features. In summary, this 
part of code can be simply replaced with:

     if (virtio_set_128bit_features_nocheck_maybe_co(vdev, 
vdev->guest_features_array) < 0) {
         error_report("Features 0x" VIRTIO_FEATURES_FMT " unsupported. "
                         "Allowed features: 0x" VIRTIO_FEATURES_FMT,
                         VIRTIO_FEATURES_PR(val),
                         VIRTIO_FEATURES_PR(vdev->host_features_array));
         return -1;
     }

There is no need of virtio_64bit_features_needed(vdev) or 
virtio_128bit_features_needed(vdev) at all.

I have another finding by the way; there are three phrases that refers 
to the new extension: array (e.g., guest_features_array), _ex (e.g., 
virtio_add_feature_ex), 128bit (e.g., virtio_128bit_features_needed).

It makes sense to make "128bit" an exception in the migration code 
because the migration format is fixed and will require e.g., "192bit" 
for a future extension. But two suffixes, _ex and _array, can be unified.

Regards,
Akihiko Odaki