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

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

On Wed, Apr 23, 2025 at 06:05:43PM +0200, Paolo Abeni wrote:
> On 4/23/25 7:46 AM, Michael S. Tsirkin wrote:
> > On Tue, Apr 22, 2025 at 07:49:41PM +0200, Paolo Abeni wrote:
> >> On 1/26/25 7:20 AM, Parav Pandit wrote:
> >>> @@ -136,6 +124,18 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
> >>>  \item[VIRTIO_NET_F_SPEED_DUPLEX(63)] Device reports speed and duplex.
> >>>  
> >>>  \item[VIRTIO_NET_F_RSS_CONTEXT(64)] Device supports multiple RSS contexts.
> >>> +
> >>> +\item[VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO (65)] Driver can receive GSO packets
> >>> +  carried by a UDP tunnel.
> >>> +
> >>> +\item[VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO_CSUM (66)] Driver handles packets
> >>> +  carried by a UDP tunnel with partial csum for the outer header.
> >>> +
> >>> +\item[VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO (67)] Device can receive GSO packets
> >>> +  carried by a UDP tunnel.
> >>> +
> >>> +\item[VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO_CSUM (68)] Device handles packets
> >>> +  carried by a UDP tunnel with partial csum for the outer header.
> >>>  \end{description}
> >>
> >> I'm finally trying to finalize the implementation and I must admit I
> >> underlooked the implication of using features bit >= 64.
> >>
> >> AFAICS all the virtio drivers in the Linux kernel and qemu assume that
> >> the features can be represented with a single u64.
> >>
> >> Supporting the above requires major reworks in both user-space and
> >> kernel space. At least on the kernel side a similar rework (extending
> >> the _netdev_ features field) has proven to be complex enough to defeat
> >> any implementation attempts for years and in the end it was dismissed in
> >> favor of lower bits reuse.
> >>
> >> I'm wondering if we could reconsider using reserved bits here?
> >>
> >> Thanks!
> >>
> >> Paolo
> > 
> > :(
> > 
> > It's been there for 2 months now.
> 
> Unfortunately I was not able to get back earlier.
> 
> > And if you want to avoid using all bits > 63 then it's even more of
> > a mess, you need to move VIRTIO_NET_F_RSS_CONTEXT which has been
> > there for a year and a half.
> 
> I started the 'support [arbitrary] larger features bitmap' exercise and
> I think there is a difference between the VIRTIO_NET_F_RSS_CONTEXT bit
> and UDP_TUNNEL_GSO related ones: AFAICS the latter go through the
> VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET command, which in turn limit the
> offload to 64 bit:
> 
> """
> 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.
> """
> 
> while I don't see similar constraints for the  VIRTIO_NET_F_RSS_CONTEXT
> feature.
> 
> The current specification is inconsistent, as it does not allow enabling
> the UDP_TUNNEL guest offloads.

Through the command, yes.


> AFAICS we have to either add the specification for another
> VIRTIO_NET_CTRL_GUEST_OFFLOADS command to allow passing the additional
> data

Given features are negotiated first, we can just say the value
can be 64 or 128 bits? In any case, I agree it's buggy as defined.

> or reconsider using reserved bits just for the UDP_TUNNEL offload
> (I'm sorry for reiterating on this, it still looks like a very tempting
> path).
> 
> Thanks,
> 
> Paolo

Parav what's your take on this one?

-- 
MST