On 8/13/25 7:55 AM, Jason Wang wrote:
> 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.
FTR, I gave a shot to the last option mentioned by Akihiko - make
virtio_pci_select_max() future proof - and the implementation complexity
is almost the same of the current one, so I guess I'll opt for such a thing.
Cheers,
Paolo
