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
