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

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

[Paolo Abeni]

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 <pabeni@redhat.com>
---
 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
 #define VIRTIO_NET_F_GUEST_USO4       54
 #define VIRTIO_NET_F_GUEST_USO6       55
 
@@ -2205,6 +2205,9 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
 negotiation corresponding offload gets enabled to preserve backward
 compatibility.
 
+Note that device features bits in the [65 to 68] range correspond to
+offload bits in the [46 to 49] range.
+
 \drivernormative{\subparagraph}{Setting Offloads State}{Device Types / Network Device / Device Operation / Control Virtqueue / Offloads State Configuration / Setting Offloads State}
 
 A driver MUST NOT enable an offload for which the appropriate feature
-- 
2.49.0

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

[Paolo Abeni]

On 5/19/25 12:12 PM, Paolo Abeni 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 <pabeni@redhat.com>

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/225

I would like to ask a vote on this issues, thanks!

Paolo

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

[Michael S. Tsirkin]

On Mon, May 19, 2025 at 12:21:57PM +0200, Paolo Abeni wrote:
> On 5/19/25 12:12 PM, Paolo Abeni 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 <pabeni@redhat.com>
> 
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/225
> 
> I would like to ask a vote on this issues, thanks!
> 
> Paolo


Acked-by: Michael S. Tsirkin <mst@redhat.com>

Let's give people a couple of days to review pls.

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

[Parav Pandit]

Hi Paolo,

> From: Paolo Abeni <pabeni@redhat.com>
> Sent: Monday, May 19, 2025 3:42 PM
> 
> 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 <pabeni@redhat.com>
> ---
>  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
Since the offload command is defined only for the rx side, in the below 'Note' should it be only 65-66 map to 46-47?

>  #define VIRTIO_NET_F_GUEST_USO4       54
>  #define VIRTIO_NET_F_GUEST_USO6       55
> 
> @@ -2205,6 +2205,9 @@ \subsubsection{Control
> Virtqueue}\label{sec:Device Types / Network Device / Devi  negotiation
> corresponding offload gets enabled to preserve backward  compatibility.
> 
> +Note that device features bits in the [65 to 68] range correspond to
> +offload bits in the [46 to 49] range.
> +
>  \drivernormative{\subparagraph}{Setting Offloads State}{Device Types /
> Network Device / Device Operation / Control Virtqueue / Offloads State
> Configuration / Setting Offloads State}
> 
>  A driver MUST NOT enable an offload for which the appropriate feature
> --
> 2.49.0

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

[Jason Wang]

On Mon, May 19, 2025 at 6:12 PM Paolo Abeni <pabeni@redhat.com> 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 <pabeni@redhat.com>
> ---
>  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.

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.

>  #define VIRTIO_NET_F_GUEST_USO4       54
>  #define VIRTIO_NET_F_GUEST_USO6       55
>
> @@ -2205,6 +2205,9 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>  negotiation corresponding offload gets enabled to preserve backward
>  compatibility.
>
> +Note that device features bits in the [65 to 68] range correspond to
> +offload bits in the [46 to 49] range.
> +
>  \drivernormative{\subparagraph}{Setting Offloads State}{Device Types / Network Device / Device Operation / Control Virtqueue / Offloads State Configuration / Setting Offloads State}
>
>  A driver MUST NOT enable an offload for which the appropriate feature
> --
> 2.49.0
>

Thanks

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

[Paolo Abeni]

On 5/20/25 2:50 AM, Jason Wang wrote:
> On Mon, May 19, 2025 at 6:12 PM Paolo Abeni <pabeni@redhat.com> 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 <pabeni@redhat.com>
>> ---
>>  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.

> 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).

Cheers,

Paolo

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

[Paolo Abeni]

On 5/19/25 5:12 PM, Parav Pandit wrote:
>> From: Paolo Abeni <pabeni@redhat.com>
>> Sent: Monday, May 19, 2025 3:42 PM
>>
>> 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 <pabeni@redhat.com>
>> ---
>>  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
> Since the offload command is defined only for the rx side, in the below 'Note' should it be only 65-66 map to 46-47?

Fine by me. I'll wait a bit before sending a v2 to conclude the ongoing
discussion with Jason.

Thanks,

Paolo

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

[Parav Pandit]

> From: Paolo Abeni <pabeni@redhat.com>
> Sent: Tuesday, May 20, 2025 12:14 PM
> 
> On 5/19/25 5:12 PM, Parav Pandit wrote:
> >> From: Paolo Abeni <pabeni@redhat.com>
> >> Sent: Monday, May 19, 2025 3:42 PM
> >>
> >> 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 <pabeni@redhat.com>
> >> ---
> >>  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
> > Since the offload command is defined only for the rx side, in the below 'Note'
> should it be only 65-66 map to 46-47?
> 
> Fine by me. I'll wait a bit before sending a v2 to conclude the ongoing
> discussion with Jason.
> 
Ok. thanks.

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

[Jason Wang]

On Tue, May 20, 2025 at 2:39 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On 5/20/25 2:50 AM, Jason Wang wrote:
> > On Mon, May 19, 2025 at 6:12 PM Paolo Abeni <pabeni@redhat.com> 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 <pabeni@redhat.com>
> >> ---
> >>  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