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]> 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.
>