On Tue, Aug 12, 2025 at 4:03 PM Akihiko Odaki
<od...@rsg.ci.i.u-tokyo.ac.jp> wrote:
>
> 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.
Things will be fine if we do the compatibility work correctly, so host
"host_features_ex_ex" won't contain anything for the legacy machine
types.
But I'm not sure it's worth worrying about it now considering we take
years to reach 64.
>
> 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
Thanks
>
