RE: [PATCH v11 3/4] virtio-net: define UDP tunnel segmentation offload feature

[Cornelia Huck <cohuck@redhat.com>]

On Mon, Jan 20 2025, Parav Pandit <parav@nvidia.com> wrote:

>> From: Cornelia Huck <cohuck@redhat.com>
>> Sent: Monday, January 20, 2025 2:34 PM
>> 
>> On Sun, Jan 19 2025, Parav Pandit <parav@nvidia.com> wrote:
>> 
>> > Hi Paolo, Michael,
>> >
>> >> From: Paolo Abeni <pabeni@redhat.com>
>> >> Sent: Wednesday, November 27, 2024 3:07 PM
>> >>
>> >>
>> >> +\item[VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO (46)] Driver can receive
>> >> GSO
>> >> +packets
>> >> +  carried by a UDP tunnel.
>> >> +
>> >> +\item[VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO (48)] Device can receive
>> >> GSO
>> >> +packets
>> >> +  carried by a UDP tunnel.
>> >
>> > Feature bits 42 to 49 are not applicable to device specific type. They are
>> reserved.
>> > Please see the note.
>> >
>> > Current spec description is:
>> >
>> > 0 to 23, and 50 to 127 Feature bits for the specific device type
>> > 24 to 41 Feature bits reserved for extensions to the queue and feature
>> > negotiation mechanisms, see 6
>> > 42 to 49, and 128 and above Feature bits reserved for future extensions.
>> >
>> > So 2 bits of these patch and of patch 4, 46 to 49 to be shifted to bits 65 to
>> 68.
>> >
>> > Or an alternative would be to amend,
>> >
>> > Bits 46 to 127 for specific device type, And 42 to 45 and 128 reserved
>> > for future extensions.
>> >
>> > Michael,
>> > Please let me know so that I can have the follow up patch to this one as
>> voting is started.
>> > And the fix is required for it.
>> >
>> > Is there any historic reason for 42-49 as reserved, or it can be used as
>> proposed in this patch?
>> > If it was only kept as elastic buffer for device generic, and device type to
>> grow it is probably ok.
>> > Using 46 to 49 has small advantage to reuse in the offloads bit as below.
>> > Please confirm.
>> 
>> See the reasoning given in 88895f56e642 ("Reserve more feature bits for
>> device type usage") -- the idea was to push out the point in time when
>> generic feature bits will have to be in the bit area beyond 63. Given that we
>> only used one of those generic bits in the meantime, I assume it will still be
>> some time before we run out of bits, even if we let the net driver grab some
>> bits there. Still, I'd probably prefer that the net device used different bits.
>> [And issues like this are easy to miss during review unfortunately.]
>> 
> I made assumption that one would have picked the device specific rage.
> I missed to detect this early enough, my bad. :(
>
> Given that we are only left with 4 bits in range 42 to 45, at generic level, it seems wise for device-type to consume feature bits in its defined range.
> For virtio-net, bits 65 and higher are preferred.
>
>> However, if the device was to use different bits, the clean solution would be
>> to withdraw, respin, and revote. If we only changed the reserved ranges,
>> we'd be fine with a change on top, so that would be less hassle in the short
>> team. If we decide that we can live with the smaller feature bit space below
>> 63, I'd be fine with that solution.
>>
>
> I am fine either way to merge this and fix immediately for using bits 65 and higher.
> My preference is to use bits 65 and higher.
>
> Or as you suggested to respin and revote right away to forward with bit 65.
>
> Please let me know.

Personally, I'd prefer to handle this in an "atomic" manner, i.e. making
sure that no version ever has the wrong bit numbers... maybe if you did a
fixup patch that changes the numbers, and commit these patches + the
fixup all at once? Less churn than doing the respin and revote dance.

>  
>> >
>> >
>> >> @@ -1862,6 +2068,7 @@ \subsubsection{Control
>> >> Virtqueue}\label{sec:Device Types / Network Device / Devi
>> >>  #define VIRTIO_NET_F_GUEST_TSO6       8
>> >>  #define VIRTIO_NET_F_GUEST_ECN        9
>> >>  #define VIRTIO_NET_F_GUEST_UFO        10
>> >> +#define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO  46
>> >>  #define VIRTIO_NET_F_GUEST_USO4       54
>> >>  #define VIRTIO_NET_F_GUEST_USO6       55