Re: [PATCH v1] virtio-net: Fix to avoid using reserved feature bits

On 5/6/25 5:00 PM, Parav Pandit wrote:
>> From: Paolo Abeni <[email protected]>
>> Sent: Tuesday, May 6, 2025 8:09 PM
>> On 5/6/25 10:56 AM, Parav Pandit wrote:
>>>> From: Michael S. Tsirkin <[email protected]>
>>>> Sent: Tuesday, May 6, 2025 1:26 PM
>>>>> There are few proposals on table.
>>>>>
>>>>> 1. From Paolo,
>>>>> - 0 to 23, and 50 to 127 Feature bits for the specific device type
>>>>> + 0 to 23, and 46 to 127 Feature bits for the specific device type
>>>>>
>>>>> This does not have good reason of why it should still be 127.
>>>>>
>>>>> 2. From me:
>>>>> - 0 to 23, and 50 to 127 Feature bits for the specific device type
>>>>> + 0 to 23, and 45 to 64 Feature bits for the specific device type
>>>>>
>>>>> This is an extension of Paolo, to justify that implementing feature
>>>>> bits is
>>>> extremely hard even for experts as pointed by Paolo.
>>>>> It is worth to not extend it further.
>>>>> RSS can be negotiated via new bit 44 in future bit as OR of 44 and
>>>>> 64 so that
>>>> more wider users (Linux, freebsd, qnx, Windows, dpdk pmd) can pick 44.
>>>>>
>>>>> 3. From me:
>>>>> Keep the feature bits encoding as is up to 127 bits, because may be
>>>>> there is
>>>> (unknown and weird) value in having 127 feature bits.
>>>>> (unknown because the reasoning of #1 and #3 mismatch).
>>>>> In that case,
>>>>> VIRTIO_NET_CTRL_GUEST_OFFLOADS command text to be updated to
>>>> indicate
>>>>> feature fits A to D map to
>> VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.offloads
>>>> bits A' to D'.
>>>>>
>>>>> I am fine with option #2 and #3.
>>>>> Doing #1 for sure is wrong.
>>>>> Wrong because it delays the problem of #1 from this to another
>>>>> feature [A]
>>>> who's voting already completed.
>>>>>
>>>>> [A]
>>>>> https://lore.kernel.org/virtio-
>>>> comment/DM4PR18MB4269F73B786E83EF68A70F
>>>>> [email protected]/T/#t
>>>>
>>>>
>>>> #3 seems more conservate.
>>>
>>> Are you good with #3?
>>
>> I'm sorry for the latency. Let me double check to avoid possible
>> misunderstanding; #3 means:
>>
>> - 0 to 23, and 50 to 127 Feature bits for the specific device type
>> + 0 to 23, and 45 to 127 Feature bits for the specific device type
>>
> No change in above feature bits.
> 
>> using bits 46-39 for UDP tunnel offloads and likely bit 45 for
>> VIRTIO_NET_F_OUT_NET_HEADER.
>>
> This also to use bit 69 as proposed.
>  
>> The VIRTIO_NET_F_CTRL_GUEST_OFFLOADS mapping should be specified
>> after eventually a new offload feature will be defined using a bit >= 64.
>>
> No. UDP tunnel feature bits 65 to 68 maps to command bits 46,47,48,49.
> This is the only description change in VIRTIO_NET_F_CTRL_GUEST_OFFLOADS command.
> Would it work?

AFAICT, yes, it should work.

But it will not avoid the immediate need to expand the virtio features
negotiation above 64 bits, with the already mentioned complexity.

I would preferably avoid that, if possible: I restarted this thread with
such a goal.

Note: the implementation complexity spark from u64 hard-coded
"everywhere" in both the kernel, qemu and the related APIs.

> In fact, this command extension alone would have been sufficient if the vnet_hdr had a fixed size where new fields could be added.

I could not parse this last statement. The virtio_net_hdr size depends
on the negotiated features, and AFAICT  VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
can't replace features negotiation. Could you please rephrase?

Thanks,

Paolo