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

On 5/6/25 6:20 PM, Parav Pandit wrote:
> From: Paolo Abeni <[email protected] Sent: Tuesday, May 6, 2025 9:10 PM
>> 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:
>>>>> 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.
>>
> In that case we should adopt #2.

Do we have quorum? Should I send a patch?

>>> 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?
>>
> I meant to say, if we have fixed size vnet hdr, say 64B, than there is no need of feature negotiation per offload.
> VIRTIO_NET_F_CTRL_GUEST_OFFLOADS or any other command can dynamically enable/disable a feature on invocation of ethtool/netlink/devlink or similar OS UAPIs.
> It does not apply presently to this feature as the infrastructure of 64B vnet hdr is not in present and it cannot a fix anyway for this issue.
> It was just a forward thinking on how to build the optimize data path.

Understood. I'm unsure if that schema could have issues with migration
vs VMs with different offloads, but let's put that aside from the main
topic here ;)

Thanks,

Paolo