On 2025/08/12 13:01, Jason Wang wrote:
On Fri, Aug 8, 2025 at 12:55 PM Akihiko Odaki
<od...@rsg.ci.i.u-tokyo.ac.jp> wrote:
On 2025/08/08 5:18, Paolo Abeni wrote:
On 7/26/25 1:52 PM, Akihiko Odaki wrote:
On 2025/07/24 4:31, Paolo Abeni wrote:
@@ -1477,6 +1509,13 @@ int virtio_pci_add_shm_cap(VirtIOPCIProxy *proxy,
return virtio_pci_add_mem_cap(proxy, &cap.cap);
}
+static int virtio_pci_select_max(const VirtIODevice *vdev)
+{
+ return virtio_features_use_ex(vdev->host_features_ex) ?
+ VIRTIO_FEATURES_NU32S :
+ 2;
This function could be simplified by replacing VIRTIO_FEATURES_NU32S
without any functional difference:
Did you mean using VIRTIO_FEATURES_NU32S instead?
1. For writes: virtio_set_features_ex() already ignores extended
features when !virtio_features_use_ex(vdev->host_features_ex)
2. For reads: When !virtio_features_use_ex(vdev->host_features_ex), the
upper bits of host_features_ex are zero, and guest_features upper bits
remain zero (since they can't be set per point 1)
I think it depends on the compatibility work which hasn't been done in
this series.
So the conditional logic is redundant here.
See below
This is to satisfy a request from Jason:
https://lists.gnu.org/archive/html/qemu-devel/2025-07/msg05291.html
https://lists.gnu.org/archive/html/qemu-devel/2025-07/msg05423.html
I agree there will not be functional differences always accessing the
full space, but the guest could still be able to notice, i.e. the
extended space will be zeroed on read with that patched qemu and
untouched by the current code and this patch. To be on the safe side I
think it would be better to avoid such difference, as suggested by Jason.
Does the above make sense to you?
By functional, I meant the functionality of QEMU, visible to the guest,
rather than the whole system including the guest, visible to the end
user. The guest cannot notice the difference because the extended space
is zero on read even without the conditional, which is described as
point 2 in the previous email.
I'm not sure I understand this correctly. But it doesn't harm here consider:
1) it's the entry point from the guest, checking and failing early is
better than depending on the low layer functions
2) we have checks in several layers (both virtio-pci and virtio core).
And it looks like a must for at least GF:
case VIRTIO_PCI_COMMON_GF:
if (proxy->gfselect < virtio_pci_select_max(vdev)) {
uint64_t features[VIRTIO_FEATURES_NU64S];
int i;
proxy->guest_features[proxy->gfselect] = val;
I missed proxy->guest_features. Indeed it makes a difference.
Now I have another concern with virtio_pci_select_max(). If the feature
set grows again, virtio_pci_select_max() will return the grown size for
the devices that have [127:64] bits, which will be a breaking change. In
this sense, VIRTIO_FEATURES_NU32S, which will grow along with the
feature set, is not appropriate here.
Hardcoding 4 ensures such a breaking change will not happen at least.
Perhaps QEMU_BUILD_BUG_ON() may be placed here to tell it needs to be
updated when the feature set grows, but I don't require that since there
is QEMU_BUILD_BUG_ON() before vmstate_virtio_pci_modern_state_sub. It
may be a good idea to move this function somewhere close to
QEMU_BUILD_BUG_ON().
Alternatively the function may compute the value to return by finding
the last non-zero element in vdev->host_features_ex and multiplying by
2. It will probably work even if the feature set grows again in the future.
Regards,
Akihiko Odaki
