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

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Apr 30, 2025 at 10:54:12AM +0000, Parav Pandit wrote:
> 
> 
> > From: Paolo Abeni <pabeni@redhat.com>
> > Sent: Wednesday, April 30, 2025 3:42 PM
> > 
> > On 4/30/25 6:44 AM, Parav Pandit wrote:
> > >> From: Michael S. Tsirkin <mst@redhat.com>
> > >> 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
> > >>>>> <mst@redhat.com>
> > >> 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
> Given that its painful enough, we can update the virtio spec to limit to 64-bit features as option_1.
> 
> A virtio spec diff like below,
> - 0 to 23, and 50 to 127 Feature bits for the specific device type
> + 0 to 23, and 50 to 63 Feature bits for the specific device type
> 
> Otherwise, we are delaying the problem to next feature, which is not good.
> 
> Heng,
> Yuri already acked, so we are good from Windows side.
> With that Windows, Linux kernel, DPDK PMD are safe.
> 
> Can you also please confirm that there are no users within Alibaba relying on RSS_CONTEXT, so that shifting the feature bit will not break any existing functionality?
> I think it will not break, because we can update the non-open-source software to detect RSS_CONTEXT on a new bit.
> 
> For example, driver can do,
> Enable_rss_context if bit X OR Bit 64 is set.


sorry if I am unclear, I think it's too late to move RSS really. It has
been out there for two years. The reason we can be reasonably sure no
one implemented the offloads so far is because the set offloads command
is broken and no one complained.  We need to fix them in the spec,
current one is broken. So the tunnel offloads can move if
it's convenient.

-- 
MST