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

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.

AFAICS we have to either add the specification for another
VIRTIO_NET_CTRL_GUEST_OFFLOADS command to allow passing the additional
data 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