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

Hi Paolo,

> From: Michael S. Tsirkin <[email protected]>
> Sent: Tuesday, May 6, 2025 1:26 PM
> 
> On Tue, May 06, 2025 at 06:15:41AM +0000, Parav Pandit wrote:
> > > From: Michael S. Tsirkin <[email protected]>
> > > Sent: Thursday, May 1, 2025 7:13 PM
> > > To: Parav Pandit <[email protected]>
> > > Cc: Paolo Abeni <[email protected]>; [email protected];
> > > virtio- [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
> > >
> > > On Wed, Apr 30, 2025 at 10:54:12AM +0000, Parav Pandit wrote:
> > > >
> > > >
> > > > > From: Paolo Abeni <[email protected]>
> > > > > Sent: Wednesday, April 30, 2025 3:42 PM
> > > > >
> > > > > 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
> > > > 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.
> >
> > There are few proposals on table.
> >
> > 1. From Paolo,
> > - 0 to 23, and 50 to 127 Feature bits for the specific device type
> > + 0 to 23, and 46 to 127 Feature bits for the specific device type
> >
> > This does not have good reason of why it should still be 127.
> >
> > 2. From me:
> > - 0 to 23, and 50 to 127 Feature bits for the specific device type
> > + 0 to 23, and 45 to 64 Feature bits for the specific device type
> >
> > This is an extension of Paolo, to justify that implementing feature bits is
> extremely hard even for experts as pointed by Paolo.
> > It is worth to not extend it further.
> > RSS can be negotiated via new bit 44 in future bit as OR of 44 and 64 so that
> more wider users (Linux, freebsd, qnx, Windows, dpdk pmd) can pick 44.
> >
> > 3. From me:
> > Keep the feature bits encoding as is up to 127 bits, because may be there is
> (unknown and weird) value in having 127 feature bits.
> > (unknown because the reasoning of #1 and #3 mismatch).
> > In that case,
> > VIRTIO_NET_CTRL_GUEST_OFFLOADS command text to be updated to
> indicate
> > feature fits A to D map to VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.offloads
> bits A' to D'.
> >
> > I am fine with option #2 and #3.
> > Doing #1 for sure is wrong.
> > Wrong because it delays the problem of #1 from this to another feature [A]
> who's voting already completed.
> >
> > [A]
> > https://lore.kernel.org/virtio-
> comment/DM4PR18MB4269F73B786E83EF68A70F
> > [email protected]/T/#t
> 
> 
> #3 seems more conservate.

Are you good with #3?