Re: [PATCH] virtio-net: clarify guest offloads <> features mapping

On Tue, May 20, 2025 at 2:39 PM Paolo Abeni <[email protected]> wrote:
>
> On 5/20/25 2:50 AM, Jason Wang wrote:
> > On Mon, May 19, 2025 at 6:12 PM Paolo Abeni <[email protected]> wrote:
> >> The guest offloads, as specified by 'Setting Offloads State,' use a
> >> 64-bit wide space, while some of the corresponding virtio net features
> >> are in the upper 64 bits of a 128-bit wide space.
> >>
> >> Clarify the relevant mapping as somewhat implied by the current text.
> >> Also rename the offload definitions to avoid referring to different
> >> numeric values with the same name in the features table and in the
> >> offload table.
> >>
> >> Signed-off-by: Paolo Abeni <[email protected]>
> >> ---
> >>  device-types/net/description.tex | 7 +++++--
> >>  1 file changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> >> index 1b6b54d..8c501bc 100644
> >> --- a/device-types/net/description.tex
> >> +++ b/device-types/net/description.tex
> >> @@ -2186,8 +2186,8 @@ \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_UDP_TUNNEL_GSO_CSUM 47
> >> +#define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO_MAPPED  46
> >> +#define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO_CSUM_MAPPED 47
> >
> > As the introduction of more offload features in the future, I wonder
> > if it's better to just avoid reusing feature names here.
>
> No strong objections, but than it would make less clear the 1to1 mapping
>  between offloads and features.

Right, so let's start from the patch.

>
> > For example, rename all here to something like
> >
> > VIRTIO_NET_CTRL_GUEST_OFFLOAD_TSO6
> > ...
> >
> > Or introduce a new command to be as wide as the feature bits.
>
> I would prefer to avoid such additional specification and code churn,
> especially given there is already a lot of stuff pending due to the
> features space extension. Note that the benefit will be limited, as
> there are a lot of bits available in the 'offloads' field and we could
> easily map more features, as needed.
>
> I think the new command should be added as a last resort after
> 'offloads' space exhaustion (hopefully far in the future).

Agree.

>
> Cheers,
>
> Paolo
>
>

Thanks