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

> From: Paolo Abeni <[email protected]>
> Sent: Tuesday, May 6, 2025 9:10 PM
> 
> On 5/6/25 5:00 PM, Parav Pandit wrote:
> >> From: Paolo Abeni <[email protected]>
> >> Sent: Tuesday, May 6, 2025 8:09 PM
> >> On 5/6/25 10:56 AM, Parav Pandit wrote:
> >>>> From: Michael S. Tsirkin <[email protected]>
> >>>> Sent: Tuesday, May 6, 2025 1:26 PM
> >>>>> 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?
> >>
> >> I'm sorry for the latency. Let me double check to avoid possible
> >> misunderstanding; #3 means:
> >>
> >> - 0 to 23, and 50 to 127 Feature bits for the specific device type
> >> + 0 to 23, and 45 to 127 Feature bits for the specific device type
> >>
> > No change in above feature bits.
> >
> >> using bits 46-39 for UDP tunnel offloads and likely bit 45 for
> >> VIRTIO_NET_F_OUT_NET_HEADER.
> >>
> > This also to use bit 69 as proposed.
> >
> >> The VIRTIO_NET_F_CTRL_GUEST_OFFLOADS mapping should be specified
> >> after eventually a new offload feature will be defined using a bit >= 64.
> >>
> > No. UDP tunnel feature bits 65 to 68 maps to command bits 46,47,48,49.
> > This is the only description change in
> VIRTIO_NET_F_CTRL_GUEST_OFFLOADS command.
> > Would it work?
> 
> AFAICT, yes, it should work.
> 
> But it will not avoid the immediate need to expand the virtio features
> negotiation above 64 bits, with the already mentioned complexity.
> 
> I would preferably avoid that, if possible: I restarted this thread with such a
> goal.
> 
In that case we should adopt #2.

> Note: the implementation complexity spark from u64 hard-coded
> "everywhere" in both the kernel, qemu and the related APIs.
> 
Hence it applies to already 2nd feature which also needs new feature bit.

> > In fact, this command extension alone would have been sufficient if the
> vnet_hdr had a fixed size where new fields could be added.
> 
> I could not parse this last statement. The virtio_net_hdr size depends on the
> negotiated features, and AFAICT  VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
> can't replace features negotiation. Could you please rephrase?
>
I meant to say, if we have fixed size vnet hdr, say 64B, than there is no need of feature negotiation per offload.
VIRTIO_NET_F_CTRL_GUEST_OFFLOADS or any other command can dynamically enable/disable a feature on invocation of ethtool/netlink/devlink or similar OS UAPIs.
It does not apply presently to this feature as the infrastructure of 64B vnet hdr is not in present and it cannot a fix anyway for this issue.
It was just a forward thinking on how to build the optimize data path.
 
> Thanks,
> 
> Paolo