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

This Is not last, but most offload features including this one does not need to tell the device in driver boot flow.
User must be able to enable/disable them dynamically as/when needed.
So we will not face the same problem when they are done dynamically without feature bit.

From: Yuri Benditovich <[email protected]>
Sent: Wednesday, April 30, 2025 10:55 AM
To: Parav Pandit <[email protected]>
Cc: Michael S. Tsirkin <[email protected]>; Paolo Abeni <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; Jason Wang <[email protected]>; Shahaf Shuler <[email protected]>; Willem de Bruijn <[email protected]>; Daniel Verkamp <[email protected]>
Subject: Re: [PATCH v1] virtio-net: Fix to avoid using reserved feature bits

Windows driver does not use RSS_CONTEXT yet. But this is not the last addition of an offload feature, so we'll face the same problem later.
I also do not see any risks in option 2.


On Wed, Apr 30, 2025 at 7:44 AM Parav Pandit <[email protected]<mailto:[email protected]>> wrote:


> From: Michael S. Tsirkin <[email protected]<mailto:[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]<mailto:[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.