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

On 4/30/25 6:44 AM, Parav Pandit wrote:
>> From: Michael S. Tsirkin <[email protected]>
>> Sent: Wednesday, April 30, 2025 2:14 AM
>>
>> On Mon, Apr 28, 2025 at 10:39:59AM +0200, Paolo Abeni wrote:
>>> On 4/23/25 8:07 PM, Michael S. Tsirkin wrote:
>>>> On Wed, Apr 23, 2025 at 09:29:11AM -0700, Daniel Verkamp wrote:
>>>>> On Tue, Apr 22, 2025 at 10:46 PM Michael S. Tsirkin <[email protected]>
>> wrote:
>>>>>> I'm afraid we'll have to bite the bullet.
>>>>>
>>>>> One other issue with bits > 63 is that the vhost-user protocol
>>>>> VHOST_USER_GET_FEATURES and VHOST_USER_SET_FEATURES
>> messages use
>>>>> u64 to represent the features, so vhost-user-net devices can't
>>>>> query or enable these features. vhost-user is outside the scope of
>>>>> the virtio spec, though, and I think it's reasonable to extend the
>>>>> protocol to enable high feature bits rather than avoiding them forever.
>>>>
>>>> Yes you would use VHOST_USER_SET_PROTOCOL_FEATURES to make
>>>> VHOST_USER_GET_FEATURES return two u64s, or even a new message
>> returning an array.
>>>
>>> I think that additionally the VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET
>>> command will need some clarification, as in the current text looks a
>>> bit
>>> inconsistent:
>>>
>>> """
>>> // in Offloads State Configuration / Setting Offloads State:
>>>
>>> #define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO  46
>>>
>>> // ...
>>>
>>> The class VIRTIO_NET_CTRL_GUEST_OFFLOADS has one command:
>>> VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET applies the new offloads
>> configuration.
>>>
>>> le64 value passed as command data is a bitmask, bits set define
>>> offloads to be enabled, bits cleared - offloads to be disabled.
>>>
>>> There is a corresponding device feature for each offload. Upon feature
>>> negotiation corresponding offload gets enabled to preserve backward
>>> compatibility """
>>>
>>> The "corresponding device feature" has the same numerical value of the
>>> selected offloads, except for UDP tunnels related one (which are
>>> mapped to bits corresponding to reserved features).
>>>
>>> It's unclear to me which should be the better way to address this
>>> inconsistency.
>>>
>>> /P
>>
>>
>> Parav, what's your take here? Given your change broke
>> VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET, do you want to revert it?
> 
> I see two options.
> Opt_1: 
> Open source Linux kernel driver and DPDK PMD has not used RSS_CONTEXT yet.
> If Heng from Alibaba acks that they do not have any internal implementation either, it may be safe to shift _all_ feature > 63 to lower position.
> We can get Yuri's feedback, if at all windows driver has used RSS context.
> 
> And once for all we mark it that feature bits are limited to 0-63.
> There is enough infrastructure in place in virtio spec to not try to squeeze things in feature bits.
> And these 4 bits are good example of it already, which could have been negotiated/communicated at later phase of driver at runtime.
> Only bit required was a bit to expand vnet header size at early stage.
> 
> Advantage: brings the good practice to adapt to the modern and efficient driver->device interface.
> Risk: May break RSS_CONTEXT (risk looks low)
> 
> Opt_2. VIRTIO_NET_CTRL_GUEST_OFFLOADS command text to be updated to indicate that,
> 
> Below defines corresponds to respective feature bits 65 to 68. There is still one to one mapping, its just position is different inside the class.
> This is clarification text to be added and sw can adjust for it.
> 
> #define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO 46
> #define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO_CSUM 47
> #define VIRTIO_NET_F_GUEST_USO4 54
> #define VIRTIO_NET_F_GUEST_USO6 55
> 
> Advantages of 2nd option are:
> a. featuring bits remain open upto 127.
> b. Does not break RSS_CONTEXT.
> 
> Both options are practical to me.
> I prefer #1, if Heng acks it, but also ok for #2.

I spent quite of bit of time trying to evaluate the scope of features
bit expansion (implied by the option 2 above).

While strictly speaking I haven't hit yet a complete blocker,
implementation-wise it's going to be huge and error prone, as great deal
of both the kernel and the user-space/qemu infrastructure hard-codes the
64 bit limit.

Even exposing the feature extensions only the the virtio-net device
(AFAICS it will "minimize" the code churn) a lot of code and devices
implementations are going to be impacted.

I expect a far away in time timeline for implementations based on option 2.

/P