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

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

[Parav Pandit]

Listed patches in the fixes tag, incorrectly used the reserved feature bits.
Fix them to use the well defined device specific range.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/212
Fixes: https://github.com/oasis-tcs/virtio-spec/issues/213
Fixes: 8cd457d8aa82 ("virtio-net: define UDP tunnel segmentation offload feature")
Fixes: 3fea589bd7c6 ("virtio-net: define UDP tunnel checksum offload feature")
Signed-off-by: Parav Pandit <parav@nvidia.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>

---
changelog:
v0->v1:
- added Cornelia's reviewed-by
- added github issue number
- skipped Paolo's suggestion to modify 'le64 offloads' as its only 64-bits
  and new feature bits are in > 64 bits range
---
 device-types/net/description.tex | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index efb543f..b546cd7 100644
--- a/device-types/net/description.tex
+++ b/device-types/net/description.tex
@@ -88,18 +88,6 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
 \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
     channel.
 
-\item[VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO (46)] Driver can receive GSO packets
-  carried by a UDP tunnel.
-
-\item[VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO_CSUM (47)] Driver handles packets
-  carried by a UDP tunnel with partial csum for the outer header.
-
-\item[VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO (48)] Device can receive GSO packets
-  carried by a UDP tunnel.
-
-\item[VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO_CSUM (49)] Device handles packets
-  carried by a UDP tunnel with partial csum for the outer header.
-
 \item[VIRTIO_NET_F_DEVICE_STATS(50)] Device can provide device-level statistics
     to the driver through the control virtqueue.
 
@@ -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}
 
 \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device / Feature bits / Feature bit requirements}
-- 
2.34.1

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

[Michael S. Tsirkin]

On Sun, Jan 26, 2025 at 08:20:58AM +0200, Parav Pandit wrote:
> Listed patches in the fixes tag, incorrectly used the reserved feature bits.
> Fix them to use the well defined device specific range.
> 
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/212
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/213
> Fixes: 8cd457d8aa82 ("virtio-net: define UDP tunnel segmentation offload feature")
> Fixes: 3fea589bd7c6 ("virtio-net: define UDP tunnel checksum offload feature")


Well the hashes will be wrong I think.
So, we can do the ballot for this fixup, and wait with
applying the patches until that new ballot completes.
Or we can do a completely new ballot.

> Signed-off-by: Parav Pandit <parav@nvidia.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 
> ---
> changelog:
> v0->v1:
> - added Cornelia's reviewed-by
> - added github issue number
> - skipped Paolo's suggestion to modify 'le64 offloads' as its only 64-bits
>   and new feature bits are in > 64 bits range
> ---
>  device-types/net/description.tex | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> index efb543f..b546cd7 100644
> --- a/device-types/net/description.tex
> +++ b/device-types/net/description.tex
> @@ -88,18 +88,6 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
>  \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
>      channel.
>  
> -\item[VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO (46)] Driver can receive GSO packets
> -  carried by a UDP tunnel.
> -
> -\item[VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO_CSUM (47)] Driver handles packets
> -  carried by a UDP tunnel with partial csum for the outer header.
> -
> -\item[VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO (48)] Device can receive GSO packets
> -  carried by a UDP tunnel.
> -
> -\item[VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO_CSUM (49)] Device handles packets
> -  carried by a UDP tunnel with partial csum for the outer header.
> -
>  \item[VIRTIO_NET_F_DEVICE_STATS(50)] Device can provide device-level statistics
>      to the driver through the control virtqueue.
>  
> @@ -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}
>  
>  \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device / Feature bits / Feature bit requirements}
> -- 
> 2.34.1

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

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Sunday, January 26, 2025 2:50 PM
> 
> On Sun, Jan 26, 2025 at 08:20:58AM +0200, Parav Pandit wrote:
> > Listed patches in the fixes tag, incorrectly used the reserved feature bits.
> > Fix them to use the well defined device specific range.
> >
> > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/212
> > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/213
> > Fixes: 8cd457d8aa82 ("virtio-net: define UDP tunnel segmentation
> > offload feature")
> > Fixes: 3fea589bd7c6 ("virtio-net: define UDP tunnel checksum offload
> > feature")
> 
> 
> Well the hashes will be wrong I think.
I pulled the patches from the mailing list to local repo using b4.
Once the new fix is voted, I will merge both the series together so that hashes stays the same.

> So, we can do the ballot for this fixup, and wait with applying the patches
> until that new ballot completes.
Yes. The small fix is reviewed by Paolo and Cornelia, so if it looks ok, we can wait for this week for any additional review comments.
And merge by Feb-10 or so after new ballot completes.

> Or we can do a completely new ballot.
> 
> > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> >
> > ---
> > changelog:
> > v0->v1:
> > - added Cornelia's reviewed-by
> > - added github issue number
> > - skipped Paolo's suggestion to modify 'le64 offloads' as its only 64-bits
> >   and new feature bits are in > 64 bits range
> > ---
> >  device-types/net/description.tex | 24 ++++++++++++------------
> >  1 file changed, 12 insertions(+), 12 deletions(-)
> >
> > diff --git a/device-types/net/description.tex
> > b/device-types/net/description.tex
> > index efb543f..b546cd7 100644
> > --- a/device-types/net/description.tex
> > +++ b/device-types/net/description.tex
> > @@ -88,18 +88,6 @@ \subsection{Feature bits}\label{sec:Device Types /
> > Network Device / Feature bits  \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)]
> Set MAC address through control
> >      channel.
> >
> > -\item[VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO (46)] Driver can receive
> GSO
> > packets
> > -  carried by a UDP tunnel.
> > -
> > -\item[VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO_CSUM (47)] Driver
> handles
> > packets
> > -  carried by a UDP tunnel with partial csum for the outer header.
> > -
> > -\item[VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO (48)] Device can receive
> GSO
> > packets
> > -  carried by a UDP tunnel.
> > -
> > -\item[VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO_CSUM (49)] Device
> handles
> > packets
> > -  carried by a UDP tunnel with partial csum for the outer header.
> > -
> >  \item[VIRTIO_NET_F_DEVICE_STATS(50)] Device can provide device-level
> statistics
> >      to the driver through the control virtqueue.
> >
> > @@ -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}
> >
> >  \subsubsection{Feature bit requirements}\label{sec:Device Types /
> > Network Device / Feature bits / Feature bit requirements}
> > --
> > 2.34.1

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

[Michael S. Tsirkin]

On Sun, Jan 26, 2025 at 04:44:51PM +0000, Parav Pandit wrote:
> 
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Sunday, January 26, 2025 2:50 PM
> > 
> > On Sun, Jan 26, 2025 at 08:20:58AM +0200, Parav Pandit wrote:
> > > Listed patches in the fixes tag, incorrectly used the reserved feature bits.
> > > Fix them to use the well defined device specific range.
> > >
> > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/212
> > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/213
> > > Fixes: 8cd457d8aa82 ("virtio-net: define UDP tunnel segmentation
> > > offload feature")
> > > Fixes: 3fea589bd7c6 ("virtio-net: define UDP tunnel checksum offload
> > > feature")
> > 
> > 
> > Well the hashes will be wrong I think.
> I pulled the patches from the mailing list to local repo using b4.
> Once the new fix is voted, I will merge both the series together so that hashes stays the same.

ok. just make sure to verify them before you push.

> > So, we can do the ballot for this fixup, and wait with applying the patches
> > until that new ballot completes.
> Yes. The small fix is reviewed by Paolo and Cornelia, so if it looks ok, we can wait for this week for any additional review comments.
> And merge by Feb-10 or so after new ballot completes.


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

> > Or we can do a completely new ballot.
> > 
> > > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > > Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> > >
> > > ---
> > > changelog:
> > > v0->v1:
> > > - added Cornelia's reviewed-by
> > > - added github issue number
> > > - skipped Paolo's suggestion to modify 'le64 offloads' as its only 64-bits
> > >   and new feature bits are in > 64 bits range
> > > ---
> > >  device-types/net/description.tex | 24 ++++++++++++------------
> > >  1 file changed, 12 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/device-types/net/description.tex
> > > b/device-types/net/description.tex
> > > index efb543f..b546cd7 100644
> > > --- a/device-types/net/description.tex
> > > +++ b/device-types/net/description.tex
> > > @@ -88,18 +88,6 @@ \subsection{Feature bits}\label{sec:Device Types /
> > > Network Device / Feature bits  \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)]
> > Set MAC address through control
> > >      channel.
> > >
> > > -\item[VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO (46)] Driver can receive
> > GSO
> > > packets
> > > -  carried by a UDP tunnel.
> > > -
> > > -\item[VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO_CSUM (47)] Driver
> > handles
> > > packets
> > > -  carried by a UDP tunnel with partial csum for the outer header.
> > > -
> > > -\item[VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO (48)] Device can receive
> > GSO
> > > packets
> > > -  carried by a UDP tunnel.
> > > -
> > > -\item[VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO_CSUM (49)] Device
> > handles
> > > packets
> > > -  carried by a UDP tunnel with partial csum for the outer header.
> > > -
> > >  \item[VIRTIO_NET_F_DEVICE_STATS(50)] Device can provide device-level
> > statistics
> > >      to the driver through the control virtqueue.
> > >
> > > @@ -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}
> > >
> > >  \subsubsection{Feature bit requirements}\label{sec:Device Types /
> > > Network Device / Feature bits / Feature bit requirements}
> > > --
> > > 2.34.1

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

[Cornelia Huck]

On Sun, Jan 26 2025, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Sun, Jan 26, 2025 at 04:44:51PM +0000, Parav Pandit wrote:
>> 
>> 
>> > From: Michael S. Tsirkin <mst@redhat.com>
>> > Sent: Sunday, January 26, 2025 2:50 PM
>> > 
>> > On Sun, Jan 26, 2025 at 08:20:58AM +0200, Parav Pandit wrote:
>> > > Listed patches in the fixes tag, incorrectly used the reserved feature bits.
>> > > Fix them to use the well defined device specific range.
>> > >
>> > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/212
>> > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/213
>> > > Fixes: 8cd457d8aa82 ("virtio-net: define UDP tunnel segmentation
>> > > offload feature")
>> > > Fixes: 3fea589bd7c6 ("virtio-net: define UDP tunnel checksum offload
>> > > feature")
>> > 
>> > 
>> > Well the hashes will be wrong I think.
>> I pulled the patches from the mailing list to local repo using b4.
>> Once the new fix is voted, I will merge both the series together so that hashes stays the same.
>
> ok. just make sure to verify them before you push.
>
>> > So, we can do the ballot for this fixup, and wait with applying the patches
>> > until that new ballot completes.
>> Yes. The small fix is reviewed by Paolo and Cornelia, so if it looks ok, we can wait for this week for any additional review comments.
>> And merge by Feb-10 or so after new ballot completes.
>
>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>

As I don't see this change as particularly contentious, I'd suggest to
just go ahead and start the voting process -- there's plenty of time to
voice concerns while the ballot is open.

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

[Parav Pandit]

Hi Matias, Michael, Cornelia,

> From: Cornelia Huck <cohuck@redhat.com>
> Sent: Monday, January 27, 2025 2:52 PM
> To: Michael S. Tsirkin <mst@redhat.com>; Parav Pandit <parav@nvidia.com>
> Cc: virtio-comment@lists.linux.dev; mvaralar@redhat.com; Shahaf Shuler
> <shahafs@nvidia.com>
> Subject: Re: [PATCH v1] virtio-net: Fix to avoid using reserved feature bits
> 
> On Sun, Jan 26 2025, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Sun, Jan 26, 2025 at 04:44:51PM +0000, Parav Pandit wrote:
> >>
> >>
> >> > From: Michael S. Tsirkin <mst@redhat.com>
> >> > Sent: Sunday, January 26, 2025 2:50 PM
> >> >
> >> > On Sun, Jan 26, 2025 at 08:20:58AM +0200, Parav Pandit wrote:
> >> > > Listed patches in the fixes tag, incorrectly used the reserved feature bits.
> >> > > Fix them to use the well defined device specific range.
> >> > >
> >> > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/212
> >> > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/213
> >> > > Fixes: 8cd457d8aa82 ("virtio-net: define UDP tunnel segmentation
> >> > > offload feature")
> >> > > Fixes: 3fea589bd7c6 ("virtio-net: define UDP tunnel checksum
> >> > > offload
> >> > > feature")
> >> >
> >> >
> >> > Well the hashes will be wrong I think.
> >> I pulled the patches from the mailing list to local repo using b4.
> >> Once the new fix is voted, I will merge both the series together so that
> hashes stays the same.
> >
> > ok. just make sure to verify them before you push.
> >
> >> > So, we can do the ballot for this fixup, and wait with applying the
> >> > patches until that new ballot completes.
> >> Yes. The small fix is reviewed by Paolo and Cornelia, so if it looks ok, we can
> wait for this week for any additional review comments.
> >> And merge by Feb-10 or so after new ballot completes.
> >
> >
> > Acked-by: Michael S. Tsirkin <mst@redhat.com>
> 
> As I don't see this change as particularly contentious, I'd suggest to just go
> ahead and start the voting process -- there's plenty of time to voice concerns
> while the ballot is open.

Can you please open the voting ballot for https://github.com/oasis-tcs/virtio-spec/issues/213 ?
The proposed patch is: https://lore.kernel.org/virtio-comment/20250126062058.13695-1-parav@nvidia.com/T/#u

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

[Paolo Abeni]

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

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

[Michael S. Tsirkin]

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.

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'm afraid we'll have to bite the bullet.

-- 
MST

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

[Paolo Abeni]

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

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

[Daniel Verkamp]

On Tue, Apr 22, 2025 at 10:46 PM Michael S. Tsirkin <mst@redhat.com> 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.
>
> 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'm afraid we'll have to bite the bullet.

One other issue with bits > 63 is that the vhost-user protocol
VHOST_USER_GET_FEATURES and VHOST_USER_SET_FEATURES messages use u64
to represent the features, so vhost-user-net devices can't query or
enable these features. vhost-user is outside the scope of the virtio
spec, though, and I think it's reasonable to extend the protocol to
enable high feature bits rather than avoiding them forever.

Thanks,
-- Daniel

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

[Michael S. Tsirkin]

On Wed, Apr 23, 2025 at 09:29:11AM -0700, Daniel Verkamp wrote:
> On Tue, Apr 22, 2025 at 10:46 PM Michael S. Tsirkin <mst@redhat.com> 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.
> >
> > 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'm afraid we'll have to bite the bullet.
> 
> One other issue with bits > 63 is that the vhost-user protocol
> VHOST_USER_GET_FEATURES and VHOST_USER_SET_FEATURES messages use u64
> to represent the features, so vhost-user-net devices can't query or
> enable these features. vhost-user is outside the scope of the virtio
> spec, though, and I think it's reasonable to extend the protocol to
> enable high feature bits rather than avoiding them forever.
> 
> Thanks,
> -- Daniel


Yes you would use VHOST_USER_SET_PROTOCOL_FEATURES to make VHOST_USER_GET_FEATURES
return two u64s, or even a new message returning an array.

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

[Paolo Abeni]

On 4/23/25 8:07 PM, Michael S. Tsirkin wrote:
> On Wed, Apr 23, 2025 at 09:29:11AM -0700, Daniel Verkamp wrote:
>> On Tue, Apr 22, 2025 at 10:46 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>> I'm afraid we'll have to bite the bullet.
>>
>> One other issue with bits > 63 is that the vhost-user protocol
>> VHOST_USER_GET_FEATURES and VHOST_USER_SET_FEATURES messages use u64
>> to represent the features, so vhost-user-net devices can't query or
>> enable these features. vhost-user is outside the scope of the virtio
>> spec, though, and I think it's reasonable to extend the protocol to
>> enable high feature bits rather than avoiding them forever.
> 
> Yes you would use VHOST_USER_SET_PROTOCOL_FEATURES to make VHOST_USER_GET_FEATURES
> return two u64s, or even a new message returning an array.

I think that additionally the VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET command
will need some clarification, as in the current text looks a bit
inconsistent:

"""
// in Offloads State Configuration / Setting Offloads State:

#define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO  46

// ...

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.

There is a corresponding device feature for each offload. Upon feature
negotiation corresponding offload gets enabled to preserve backward
compatibility
"""

The "corresponding device feature" has the same numerical value of the
selected offloads, except for UDP tunnels related one (which are mapped
to bits corresponding to reserved features).

It's unclear to me which should be the better way to address this
inconsistency.

/P

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

[Michael S. Tsirkin]

On Mon, Apr 28, 2025 at 10:39:59AM +0200, Paolo Abeni wrote:
> On 4/23/25 8:07 PM, Michael S. Tsirkin wrote:
> > On Wed, Apr 23, 2025 at 09:29:11AM -0700, Daniel Verkamp wrote:
> >> On Tue, Apr 22, 2025 at 10:46 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >>> I'm afraid we'll have to bite the bullet.
> >>
> >> One other issue with bits > 63 is that the vhost-user protocol
> >> VHOST_USER_GET_FEATURES and VHOST_USER_SET_FEATURES messages use u64
> >> to represent the features, so vhost-user-net devices can't query or
> >> enable these features. vhost-user is outside the scope of the virtio
> >> spec, though, and I think it's reasonable to extend the protocol to
> >> enable high feature bits rather than avoiding them forever.
> > 
> > Yes you would use VHOST_USER_SET_PROTOCOL_FEATURES to make VHOST_USER_GET_FEATURES
> > return two u64s, or even a new message returning an array.
> 
> I think that additionally the VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET command
> will need some clarification, as in the current text looks a bit
> inconsistent:
> 
> """
> // in Offloads State Configuration / Setting Offloads State:
> 
> #define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO  46
> 
> // ...
> 
> 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.
> 
> There is a corresponding device feature for each offload. Upon feature
> negotiation corresponding offload gets enabled to preserve backward
> compatibility
> """
> 
> The "corresponding device feature" has the same numerical value of the
> selected offloads, except for UDP tunnels related one (which are mapped
> to bits corresponding to reserved features).
> 
> It's unclear to me which should be the better way to address this
> inconsistency.
> 
> /P

Could you pls explain what is inconsistent?

-- 
MST

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

[Michael S. Tsirkin]

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

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

[Paolo Abeni]

Hi,

I'll try to consolidate here the current discussion.

My understanding is that the we are on same WRT that there is a problem
with the current spec.

On 4/28/25 11:13 AM, Michael S. Tsirkin wrote:
> 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.

Do you mean that the 'offload' field should be 64 or 128 bits long
depending on the negotiated features?

Could that be problematic with VM migrations and/or backward
compatibility? I can't see a specific faulty scenario, but the 'implied'
field size sounds fragile to me.

/P

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

[Michael S. Tsirkin]

On Mon, Apr 28, 2025 at 07:07:12PM +0200, Paolo Abeni wrote:
> Hi,
> 
> I'll try to consolidate here the current discussion.
> 
> My understanding is that the we are on same WRT that there is a problem
> with the current spec.
> 
> On 4/28/25 11:13 AM, Michael S. Tsirkin wrote:
> > 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.
> 
> Do you mean that the 'offload' field should be 64 or 128 bits long
> depending on the negotiated features?
> 
> Could that be problematic with VM migrations and/or backward
> compatibility? I can't see a specific faulty scenario, but the 'implied'
> field size sounds fragile to me.
> 
> /P

My idea is to say any number of le64 values, and any bits excluded
must be 0.

So looking at qemu for example, if you send it more than 64 bit
it will just ignore the top 64 bit. But it does not currently
support any low bits, so it's fine.

Again any change will break compat technicall but then
any change to reserved bits does, too.


-- 
MST

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

[Michael S. Tsirkin]

On Mon, Apr 28, 2025 at 10:39:59AM +0200, Paolo Abeni wrote:
> On 4/23/25 8:07 PM, Michael S. Tsirkin wrote:
> > On Wed, Apr 23, 2025 at 09:29:11AM -0700, Daniel Verkamp wrote:
> >> On Tue, Apr 22, 2025 at 10:46 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >>> I'm afraid we'll have to bite the bullet.
> >>
> >> One other issue with bits > 63 is that the vhost-user protocol
> >> VHOST_USER_GET_FEATURES and VHOST_USER_SET_FEATURES messages use u64
> >> to represent the features, so vhost-user-net devices can't query or
> >> enable these features. vhost-user is outside the scope of the virtio
> >> spec, though, and I think it's reasonable to extend the protocol to
> >> enable high feature bits rather than avoiding them forever.
> > 
> > Yes you would use VHOST_USER_SET_PROTOCOL_FEATURES to make VHOST_USER_GET_FEATURES
> > return two u64s, or even a new message returning an array.
> 
> I think that additionally the VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET command
> will need some clarification, as in the current text looks a bit
> inconsistent:
> 
> """
> // in Offloads State Configuration / Setting Offloads State:
> 
> #define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO  46
> 
> // ...
> 
> 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.
> 
> There is a corresponding device feature for each offload. Upon feature
> negotiation corresponding offload gets enabled to preserve backward
> compatibility
> """
> 
> The "corresponding device feature" has the same numerical value of the
> selected offloads, except for UDP tunnels related one (which are mapped
> to bits corresponding to reserved features).
> 
> It's unclear to me which should be the better way to address this
> inconsistency.
> 
> /P


Parav, what's your take here? Given your change broke
VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET, do you want to revert it?

-- 
MST

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

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, April 30, 2025 2:14 AM
> 
> On Mon, Apr 28, 2025 at 10:39:59AM +0200, Paolo Abeni wrote:
> > On 4/23/25 8:07 PM, Michael S. Tsirkin wrote:
> > > On Wed, Apr 23, 2025 at 09:29:11AM -0700, Daniel Verkamp wrote:
> > >> On Tue, Apr 22, 2025 at 10:46 PM Michael S. Tsirkin <mst@redhat.com>
> wrote:
> > >>> I'm afraid we'll have to bite the bullet.
> > >>
> > >> One other issue with bits > 63 is that the vhost-user protocol
> > >> VHOST_USER_GET_FEATURES and VHOST_USER_SET_FEATURES
> messages use
> > >> u64 to represent the features, so vhost-user-net devices can't
> > >> query or enable these features. vhost-user is outside the scope of
> > >> the virtio spec, though, and I think it's reasonable to extend the
> > >> protocol to enable high feature bits rather than avoiding them forever.
> > >
> > > Yes you would use VHOST_USER_SET_PROTOCOL_FEATURES to make
> > > VHOST_USER_GET_FEATURES return two u64s, or even a new message
> returning an array.
> >
> > I think that additionally the VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET
> > command will need some clarification, as in the current text looks a
> > bit
> > inconsistent:
> >
> > """
> > // in Offloads State Configuration / Setting Offloads State:
> >
> > #define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO  46
> >
> > // ...
> >
> > 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.
> >
> > There is a corresponding device feature for each offload. Upon feature
> > negotiation corresponding offload gets enabled to preserve backward
> > compatibility """
> >
> > The "corresponding device feature" has the same numerical value of the
> > selected offloads, except for UDP tunnels related one (which are
> > mapped to bits corresponding to reserved features).
> >
> > It's unclear to me which should be the better way to address this
> > inconsistency.
> >
> > /P
> 
> 
> Parav, what's your take here? Given your change broke
> VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET, do you want to revert it?

I see two options.
Opt_1: 
Open source Linux kernel driver and DPDK PMD has not used RSS_CONTEXT yet.
If Heng from Alibaba acks that they do not have any internal implementation either, it may be safe to shift _all_ feature > 63 to lower position.
We can get Yuri's feedback, if at all windows driver has used RSS context.

And once for all we mark it that feature bits are limited to 0-63.
There is enough infrastructure in place in virtio spec to not try to squeeze things in feature bits.
And these 4 bits are good example of it already, which could have been negotiated/communicated at later phase of driver at runtime.
Only bit required was a bit to expand vnet header size at early stage.

Advantage: brings the good practice to adapt to the modern and efficient driver->device interface.
Risk: May break RSS_CONTEXT (risk looks low)

Opt_2. VIRTIO_NET_CTRL_GUEST_OFFLOADS command text to be updated to indicate that,

Below defines corresponds to respective feature bits 65 to 68. There is still one to one mapping, its just position is different inside the class.
This is clarification text to be added and sw can adjust for it.

#define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO 46
#define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO_CSUM 47
#define VIRTIO_NET_F_GUEST_USO4 54
#define VIRTIO_NET_F_GUEST_USO6 55

Advantages of 2nd option are:
a. featuring bits remain open upto 127.
b. Does not break RSS_CONTEXT.

Both options are practical to me.
I prefer #1, if Heng acks it, but also ok for #2.

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

[Yuri Benditovich]

[-- Attachment #1: Type: text/plain, Size: 4275 bytes --]

Windows driver does not use RSS_CONTEXT yet. But this is not the
last addition of an offload feature, so we'll face the same problem later.
I also do not see any risks in option 2.


On Wed, Apr 30, 2025 at 7:44 AM Parav Pandit <parav@nvidia.com> wrote:

>
>
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Wednesday, April 30, 2025 2:14 AM
> >
> > On Mon, Apr 28, 2025 at 10:39:59AM +0200, Paolo Abeni wrote:
> > > On 4/23/25 8:07 PM, Michael S. Tsirkin wrote:
> > > > On Wed, Apr 23, 2025 at 09:29:11AM -0700, Daniel Verkamp wrote:
> > > >> On Tue, Apr 22, 2025 at 10:46 PM Michael S. Tsirkin <mst@redhat.com
> >
> > wrote:
> > > >>> I'm afraid we'll have to bite the bullet.
> > > >>
> > > >> One other issue with bits > 63 is that the vhost-user protocol
> > > >> VHOST_USER_GET_FEATURES and VHOST_USER_SET_FEATURES
> > messages use
> > > >> u64 to represent the features, so vhost-user-net devices can't
> > > >> query or enable these features. vhost-user is outside the scope of
> > > >> the virtio spec, though, and I think it's reasonable to extend the
> > > >> protocol to enable high feature bits rather than avoiding them
> forever.
> > > >
> > > > Yes you would use VHOST_USER_SET_PROTOCOL_FEATURES to make
> > > > VHOST_USER_GET_FEATURES return two u64s, or even a new message
> > returning an array.
> > >
> > > I think that additionally the VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET
> > > command will need some clarification, as in the current text looks a
> > > bit
> > > inconsistent:
> > >
> > > """
> > > // in Offloads State Configuration / Setting Offloads State:
> > >
> > > #define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO  46
> > >
> > > // ...
> > >
> > > 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.
> > >
> > > There is a corresponding device feature for each offload. Upon feature
> > > negotiation corresponding offload gets enabled to preserve backward
> > > compatibility """
> > >
> > > The "corresponding device feature" has the same numerical value of the
> > > selected offloads, except for UDP tunnels related one (which are
> > > mapped to bits corresponding to reserved features).
> > >
> > > It's unclear to me which should be the better way to address this
> > > inconsistency.
> > >
> > > /P
> >
> >
> > Parav, what's your take here? Given your change broke
> > VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET, do you want to revert it?
>
> I see two options.
> Opt_1:
> Open source Linux kernel driver and DPDK PMD has not used RSS_CONTEXT yet.
> If Heng from Alibaba acks that they do not have any internal
> implementation either, it may be safe to shift _all_ feature > 63 to lower
> position.
> We can get Yuri's feedback, if at all windows driver has used RSS context.
>
> And once for all we mark it that feature bits are limited to 0-63.
> There is enough infrastructure in place in virtio spec to not try to
> squeeze things in feature bits.
> And these 4 bits are good example of it already, which could have been
> negotiated/communicated at later phase of driver at runtime.
> Only bit required was a bit to expand vnet header size at early stage.
>
> Advantage: brings the good practice to adapt to the modern and efficient
> driver->device interface.
> Risk: May break RSS_CONTEXT (risk looks low)
>
> Opt_2. VIRTIO_NET_CTRL_GUEST_OFFLOADS command text to be updated to
> indicate that,
>
> Below defines corresponds to respective feature bits 65 to 68. There is
> still one to one mapping, its just position is different inside the class.
> This is clarification text to be added and sw can adjust for it.
>
> #define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO 46
> #define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO_CSUM 47
> #define VIRTIO_NET_F_GUEST_USO4 54
> #define VIRTIO_NET_F_GUEST_USO6 55
>
> Advantages of 2nd option are:
> a. featuring bits remain open upto 127.
> b. Does not break RSS_CONTEXT.
>
> Both options are practical to me.
> I prefer #1, if Heng acks it, but also ok for #2.
>

[-- Attachment #2: Type: text/html, Size: 5330 bytes --]

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

[Parav Pandit]

[-- Attachment #1: Type: text/plain, Size: 4956 bytes --]

This Is not last, but most offload features including this one does not need to tell the device in driver boot flow.
User must be able to enable/disable them dynamically as/when needed.
So we will not face the same problem when they are done dynamically without feature bit.

From: Yuri Benditovich <yuri.benditovich@daynix.com>
Sent: Wednesday, April 30, 2025 10:55 AM
To: Parav Pandit <parav@nvidia.com>
Cc: Michael S. Tsirkin <mst@redhat.com>; Paolo Abeni <pabeni@redhat.com>; hengqi@linux.alibaba.com; virtio-comment@lists.linux.dev; cohuck@redhat.com; mvaralar@redhat.com; Jason Wang <jasowang@redhat.com>; Shahaf Shuler <shahafs@nvidia.com>; Willem de Bruijn <willemb@google.com>; Daniel Verkamp <dverkamp@chromium.org>
Subject: Re: [PATCH v1] virtio-net: Fix to avoid using reserved feature bits

Windows driver does not use RSS_CONTEXT yet. But this is not the last addition of an offload feature, so we'll face the same problem later.
I also do not see any risks in option 2.


On Wed, Apr 30, 2025 at 7:44 AM Parav Pandit <parav@nvidia.com<mailto:parav@nvidia.com>> wrote:


> From: Michael S. Tsirkin <mst@redhat.com<mailto:mst@redhat.com>>
> Sent: Wednesday, April 30, 2025 2:14 AM
>
> On Mon, Apr 28, 2025 at 10:39:59AM +0200, Paolo Abeni wrote:
> > On 4/23/25 8:07 PM, Michael S. Tsirkin wrote:
> > > On Wed, Apr 23, 2025 at 09:29:11AM -0700, Daniel Verkamp wrote:
> > >> On Tue, Apr 22, 2025 at 10:46 PM Michael S. Tsirkin <mst@redhat.com<mailto:mst@redhat.com>>
> wrote:
> > >>> I'm afraid we'll have to bite the bullet.
> > >>
> > >> One other issue with bits > 63 is that the vhost-user protocol
> > >> VHOST_USER_GET_FEATURES and VHOST_USER_SET_FEATURES
> messages use
> > >> u64 to represent the features, so vhost-user-net devices can't
> > >> query or enable these features. vhost-user is outside the scope of
> > >> the virtio spec, though, and I think it's reasonable to extend the
> > >> protocol to enable high feature bits rather than avoiding them forever.
> > >
> > > Yes you would use VHOST_USER_SET_PROTOCOL_FEATURES to make
> > > VHOST_USER_GET_FEATURES return two u64s, or even a new message
> returning an array.
> >
> > I think that additionally the VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET
> > command will need some clarification, as in the current text looks a
> > bit
> > inconsistent:
> >
> > """
> > // in Offloads State Configuration / Setting Offloads State:
> >
> > #define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO  46
> >
> > // ...
> >
> > 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.
> >
> > There is a corresponding device feature for each offload. Upon feature
> > negotiation corresponding offload gets enabled to preserve backward
> > compatibility """
> >
> > The "corresponding device feature" has the same numerical value of the
> > selected offloads, except for UDP tunnels related one (which are
> > mapped to bits corresponding to reserved features).
> >
> > It's unclear to me which should be the better way to address this
> > inconsistency.
> >
> > /P
>
>
> Parav, what's your take here? Given your change broke
> VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET, do you want to revert it?

I see two options.
Opt_1:
Open source Linux kernel driver and DPDK PMD has not used RSS_CONTEXT yet.
If Heng from Alibaba acks that they do not have any internal implementation either, it may be safe to shift _all_ feature > 63 to lower position.
We can get Yuri's feedback, if at all windows driver has used RSS context.

And once for all we mark it that feature bits are limited to 0-63.
There is enough infrastructure in place in virtio spec to not try to squeeze things in feature bits.
And these 4 bits are good example of it already, which could have been negotiated/communicated at later phase of driver at runtime.
Only bit required was a bit to expand vnet header size at early stage.

Advantage: brings the good practice to adapt to the modern and efficient driver->device interface.
Risk: May break RSS_CONTEXT (risk looks low)

Opt_2. VIRTIO_NET_CTRL_GUEST_OFFLOADS command text to be updated to indicate that,

Below defines corresponds to respective feature bits 65 to 68. There is still one to one mapping, its just position is different inside the class.
This is clarification text to be added and sw can adjust for it.

#define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO 46
#define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO_CSUM 47
#define VIRTIO_NET_F_GUEST_USO4 54
#define VIRTIO_NET_F_GUEST_USO6 55

Advantages of 2nd option are:
a. featuring bits remain open upto 127.
b. Does not break RSS_CONTEXT.

Both options are practical to me.
I prefer #1, if Heng acks it, but also ok for #2.

[-- Attachment #2: Type: text/html, Size: 8755 bytes --]

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

[Paolo Abeni]

On 4/30/25 6:44 AM, Parav Pandit wrote:
>> From: Michael S. Tsirkin <mst@redhat.com>
>> Sent: Wednesday, April 30, 2025 2:14 AM
>>
>> On Mon, Apr 28, 2025 at 10:39:59AM +0200, Paolo Abeni wrote:
>>> On 4/23/25 8:07 PM, Michael S. Tsirkin wrote:
>>>> On Wed, Apr 23, 2025 at 09:29:11AM -0700, Daniel Verkamp wrote:
>>>>> On Tue, Apr 22, 2025 at 10:46 PM Michael S. Tsirkin <mst@redhat.com>
>> wrote:
>>>>>> I'm afraid we'll have to bite the bullet.
>>>>>
>>>>> One other issue with bits > 63 is that the vhost-user protocol
>>>>> VHOST_USER_GET_FEATURES and VHOST_USER_SET_FEATURES
>> messages use
>>>>> u64 to represent the features, so vhost-user-net devices can't
>>>>> query or enable these features. vhost-user is outside the scope of
>>>>> the virtio spec, though, and I think it's reasonable to extend the
>>>>> protocol to enable high feature bits rather than avoiding them forever.
>>>>
>>>> Yes you would use VHOST_USER_SET_PROTOCOL_FEATURES to make
>>>> VHOST_USER_GET_FEATURES return two u64s, or even a new message
>> returning an array.
>>>
>>> I think that additionally the VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET
>>> command will need some clarification, as in the current text looks a
>>> bit
>>> inconsistent:
>>>
>>> """
>>> // in Offloads State Configuration / Setting Offloads State:
>>>
>>> #define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO  46
>>>
>>> // ...
>>>
>>> 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.
>>>
>>> There is a corresponding device feature for each offload. Upon feature
>>> negotiation corresponding offload gets enabled to preserve backward
>>> compatibility """
>>>
>>> The "corresponding device feature" has the same numerical value of the
>>> selected offloads, except for UDP tunnels related one (which are
>>> mapped to bits corresponding to reserved features).
>>>
>>> It's unclear to me which should be the better way to address this
>>> inconsistency.
>>>
>>> /P
>>
>>
>> Parav, what's your take here? Given your change broke
>> VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET, do you want to revert it?
> 
> I see two options.
> Opt_1: 
> Open source Linux kernel driver and DPDK PMD has not used RSS_CONTEXT yet.
> If Heng from Alibaba acks that they do not have any internal implementation either, it may be safe to shift _all_ feature > 63 to lower position.
> We can get Yuri's feedback, if at all windows driver has used RSS context.
> 
> And once for all we mark it that feature bits are limited to 0-63.
> There is enough infrastructure in place in virtio spec to not try to squeeze things in feature bits.
> And these 4 bits are good example of it already, which could have been negotiated/communicated at later phase of driver at runtime.
> Only bit required was a bit to expand vnet header size at early stage.
> 
> Advantage: brings the good practice to adapt to the modern and efficient driver->device interface.
> Risk: May break RSS_CONTEXT (risk looks low)
> 
> Opt_2. VIRTIO_NET_CTRL_GUEST_OFFLOADS command text to be updated to indicate that,
> 
> Below defines corresponds to respective feature bits 65 to 68. There is still one to one mapping, its just position is different inside the class.
> This is clarification text to be added and sw can adjust for it.
> 
> #define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO 46
> #define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO_CSUM 47
> #define VIRTIO_NET_F_GUEST_USO4 54
> #define VIRTIO_NET_F_GUEST_USO6 55
> 
> Advantages of 2nd option are:
> a. featuring bits remain open upto 127.
> b. Does not break RSS_CONTEXT.
> 
> Both options are practical to me.
> I prefer #1, if Heng acks it, but also ok for #2.

I spent quite of bit of time trying to evaluate the scope of features
bit expansion (implied by the option 2 above).

While strictly speaking I haven't hit yet a complete blocker,
implementation-wise it's going to be huge and error prone, as great deal
of both the kernel and the user-space/qemu infrastructure hard-codes the
64 bit limit.

Even exposing the feature extensions only the the virtio-net device
(AFAICS it will "minimize" the code churn) a lot of code and devices
implementations are going to be impacted.

I expect a far away in time timeline for implementations based on option 2.

/P

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

[Parav Pandit]

> From: Paolo Abeni <pabeni@redhat.com>
> Sent: Wednesday, April 30, 2025 3:42 PM
> 
> On 4/30/25 6:44 AM, Parav Pandit wrote:
> >> From: Michael S. Tsirkin <mst@redhat.com>
> >> Sent: Wednesday, April 30, 2025 2:14 AM
> >>
> >> On Mon, Apr 28, 2025 at 10:39:59AM +0200, Paolo Abeni wrote:
> >>> On 4/23/25 8:07 PM, Michael S. Tsirkin wrote:
> >>>> On Wed, Apr 23, 2025 at 09:29:11AM -0700, Daniel Verkamp wrote:
> >>>>> On Tue, Apr 22, 2025 at 10:46 PM Michael S. Tsirkin
> >>>>> <mst@redhat.com>
> >> wrote:
> >>>>>> I'm afraid we'll have to bite the bullet.
> >>>>>
> >>>>> One other issue with bits > 63 is that the vhost-user protocol
> >>>>> VHOST_USER_GET_FEATURES and VHOST_USER_SET_FEATURES
> >> messages use
> >>>>> u64 to represent the features, so vhost-user-net devices can't
> >>>>> query or enable these features. vhost-user is outside the scope of
> >>>>> the virtio spec, though, and I think it's reasonable to extend the
> >>>>> protocol to enable high feature bits rather than avoiding them forever.
> >>>>
> >>>> Yes you would use VHOST_USER_SET_PROTOCOL_FEATURES to make
> >>>> VHOST_USER_GET_FEATURES return two u64s, or even a new message
> >> returning an array.
> >>>
> >>> I think that additionally the VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET
> >>> command will need some clarification, as in the current text looks a
> >>> bit
> >>> inconsistent:
> >>>
> >>> """
> >>> // in Offloads State Configuration / Setting Offloads State:
> >>>
> >>> #define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO  46
> >>>
> >>> // ...
> >>>
> >>> 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.
> >>>
> >>> There is a corresponding device feature for each offload. Upon
> >>> feature negotiation corresponding offload gets enabled to preserve
> >>> backward compatibility """
> >>>
> >>> The "corresponding device feature" has the same numerical value of
> >>> the selected offloads, except for UDP tunnels related one (which are
> >>> mapped to bits corresponding to reserved features).
> >>>
> >>> It's unclear to me which should be the better way to address this
> >>> inconsistency.
> >>>
> >>> /P
> >>
> >>
> >> Parav, what's your take here? Given your change broke
> >> VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET, do you want to revert it?
> >
> > I see two options.
> > Opt_1:
> > Open source Linux kernel driver and DPDK PMD has not used RSS_CONTEXT
> yet.
> > If Heng from Alibaba acks that they do not have any internal
> implementation either, it may be safe to shift _all_ feature > 63 to lower
> position.
> > We can get Yuri's feedback, if at all windows driver has used RSS context.
> >
> > And once for all we mark it that feature bits are limited to 0-63.
> > There is enough infrastructure in place in virtio spec to not try to squeeze
> things in feature bits.
> > And these 4 bits are good example of it already, which could have been
> negotiated/communicated at later phase of driver at runtime.
> > Only bit required was a bit to expand vnet header size at early stage.
> >
> > Advantage: brings the good practice to adapt to the modern and efficient
> driver->device interface.
> > Risk: May break RSS_CONTEXT (risk looks low)
> >
> > Opt_2. VIRTIO_NET_CTRL_GUEST_OFFLOADS command text to be updated
> to
> > indicate that,
> >
> > Below defines corresponds to respective feature bits 65 to 68. There is still
> one to one mapping, its just position is different inside the class.
> > This is clarification text to be added and sw can adjust for it.
> >
> > #define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO 46 #define
> > VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO_CSUM 47 #define
> > VIRTIO_NET_F_GUEST_USO4 54 #define VIRTIO_NET_F_GUEST_USO6 55
> >
> > Advantages of 2nd option are:
> > a. featuring bits remain open upto 127.
> > b. Does not break RSS_CONTEXT.
> >
> > Both options are practical to me.
> > I prefer #1, if Heng acks it, but also ok for #2.
> 
> I spent quite of bit of time trying to evaluate the scope of features bit
> expansion (implied by the option 2 above).
> 
> While strictly speaking I haven't hit yet a complete blocker, implementation-
> wise it's going to be huge and error prone, as great deal of both the kernel
> and the user-space/qemu infrastructure hard-codes the
> 64 bit limit.
> 
> Even exposing the feature extensions only the the virtio-net device (AFAICS it
> will "minimize" the code churn) a lot of code and devices implementations are
> going to be impacted.
> 
> I expect a far away in time timeline for implementations based on option 2.
> 
> /P
Given that its painful enough, we can update the virtio spec to limit to 64-bit features as option_1.

A virtio spec diff like below,
- 0 to 23, and 50 to 127 Feature bits for the specific device type
+ 0 to 23, and 50 to 63 Feature bits for the specific device type

Otherwise, we are delaying the problem to next feature, which is not good.

Heng,
Yuri already acked, so we are good from Windows side.
With that Windows, Linux kernel, DPDK PMD are safe.

Can you also please confirm that there are no users within Alibaba relying on RSS_CONTEXT, so that shifting the feature bit will not break any existing functionality?
I think it will not break, because we can update the non-open-source software to detect RSS_CONTEXT on a new bit.

For example, driver can do,
Enable_rss_context if bit X OR Bit 64 is set.

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

[Michael S. Tsirkin]

On Wed, Apr 30, 2025 at 10:54:12AM +0000, Parav Pandit wrote:
> 
> 
> > From: Paolo Abeni <pabeni@redhat.com>
> > Sent: Wednesday, April 30, 2025 3:42 PM
> > 
> > On 4/30/25 6:44 AM, Parav Pandit wrote:
> > >> From: Michael S. Tsirkin <mst@redhat.com>
> > >> Sent: Wednesday, April 30, 2025 2:14 AM
> > >>
> > >> On Mon, Apr 28, 2025 at 10:39:59AM +0200, Paolo Abeni wrote:
> > >>> On 4/23/25 8:07 PM, Michael S. Tsirkin wrote:
> > >>>> On Wed, Apr 23, 2025 at 09:29:11AM -0700, Daniel Verkamp wrote:
> > >>>>> On Tue, Apr 22, 2025 at 10:46 PM Michael S. Tsirkin
> > >>>>> <mst@redhat.com>
> > >> wrote:
> > >>>>>> I'm afraid we'll have to bite the bullet.
> > >>>>>
> > >>>>> One other issue with bits > 63 is that the vhost-user protocol
> > >>>>> VHOST_USER_GET_FEATURES and VHOST_USER_SET_FEATURES
> > >> messages use
> > >>>>> u64 to represent the features, so vhost-user-net devices can't
> > >>>>> query or enable these features. vhost-user is outside the scope of
> > >>>>> the virtio spec, though, and I think it's reasonable to extend the
> > >>>>> protocol to enable high feature bits rather than avoiding them forever.
> > >>>>
> > >>>> Yes you would use VHOST_USER_SET_PROTOCOL_FEATURES to make
> > >>>> VHOST_USER_GET_FEATURES return two u64s, or even a new message
> > >> returning an array.
> > >>>
> > >>> I think that additionally the VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET
> > >>> command will need some clarification, as in the current text looks a
> > >>> bit
> > >>> inconsistent:
> > >>>
> > >>> """
> > >>> // in Offloads State Configuration / Setting Offloads State:
> > >>>
> > >>> #define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO  46
> > >>>
> > >>> // ...
> > >>>
> > >>> 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.
> > >>>
> > >>> There is a corresponding device feature for each offload. Upon
> > >>> feature negotiation corresponding offload gets enabled to preserve
> > >>> backward compatibility """
> > >>>
> > >>> The "corresponding device feature" has the same numerical value of
> > >>> the selected offloads, except for UDP tunnels related one (which are
> > >>> mapped to bits corresponding to reserved features).
> > >>>
> > >>> It's unclear to me which should be the better way to address this
> > >>> inconsistency.
> > >>>
> > >>> /P
> > >>
> > >>
> > >> Parav, what's your take here? Given your change broke
> > >> VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET, do you want to revert it?
> > >
> > > I see two options.
> > > Opt_1:
> > > Open source Linux kernel driver and DPDK PMD has not used RSS_CONTEXT
> > yet.
> > > If Heng from Alibaba acks that they do not have any internal
> > implementation either, it may be safe to shift _all_ feature > 63 to lower
> > position.
> > > We can get Yuri's feedback, if at all windows driver has used RSS context.
> > >
> > > And once for all we mark it that feature bits are limited to 0-63.
> > > There is enough infrastructure in place in virtio spec to not try to squeeze
> > things in feature bits.
> > > And these 4 bits are good example of it already, which could have been
> > negotiated/communicated at later phase of driver at runtime.
> > > Only bit required was a bit to expand vnet header size at early stage.
> > >
> > > Advantage: brings the good practice to adapt to the modern and efficient
> > driver->device interface.
> > > Risk: May break RSS_CONTEXT (risk looks low)
> > >
> > > Opt_2. VIRTIO_NET_CTRL_GUEST_OFFLOADS command text to be updated
> > to
> > > indicate that,
> > >
> > > Below defines corresponds to respective feature bits 65 to 68. There is still
> > one to one mapping, its just position is different inside the class.
> > > This is clarification text to be added and sw can adjust for it.
> > >
> > > #define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO 46 #define
> > > VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO_CSUM 47 #define
> > > VIRTIO_NET_F_GUEST_USO4 54 #define VIRTIO_NET_F_GUEST_USO6 55
> > >
> > > Advantages of 2nd option are:
> > > a. featuring bits remain open upto 127.
> > > b. Does not break RSS_CONTEXT.
> > >
> > > Both options are practical to me.
> > > I prefer #1, if Heng acks it, but also ok for #2.
> > 
> > I spent quite of bit of time trying to evaluate the scope of features bit
> > expansion (implied by the option 2 above).
> > 
> > While strictly speaking I haven't hit yet a complete blocker, implementation-
> > wise it's going to be huge and error prone, as great deal of both the kernel
> > and the user-space/qemu infrastructure hard-codes the
> > 64 bit limit.
> > 
> > Even exposing the feature extensions only the the virtio-net device (AFAICS it
> > will "minimize" the code churn) a lot of code and devices implementations are
> > going to be impacted.
> > 
> > I expect a far away in time timeline for implementations based on option 2.
> > 
> > /P
> Given that its painful enough, we can update the virtio spec to limit to 64-bit features as option_1.
> 
> A virtio spec diff like below,
> - 0 to 23, and 50 to 127 Feature bits for the specific device type
> + 0 to 23, and 50 to 63 Feature bits for the specific device type
> 
> Otherwise, we are delaying the problem to next feature, which is not good.
> 
> Heng,
> Yuri already acked, so we are good from Windows side.
> With that Windows, Linux kernel, DPDK PMD are safe.
> 
> Can you also please confirm that there are no users within Alibaba relying on RSS_CONTEXT, so that shifting the feature bit will not break any existing functionality?
> I think it will not break, because we can update the non-open-source software to detect RSS_CONTEXT on a new bit.
> 
> For example, driver can do,
> Enable_rss_context if bit X OR Bit 64 is set.


sorry if I am unclear, I think it's too late to move RSS really. It has
been out there for two years. The reason we can be reasonably sure no
one implemented the offloads so far is because the set offloads command
is broken and no one complained.  We need to fix them in the spec,
current one is broken. So the tunnel offloads can move if
it's convenient.

-- 
MST

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

[Paolo Abeni]

On Thu, May 1, 2025 at 3:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> sorry if I am unclear, I think it's too late to move RSS really. It has
> been out there for two years. The reason we can be reasonably sure no
> one implemented the offloads so far is because the set offloads command
> is broken and no one complained.  We need to fix them in the spec,
> current one is broken. So the tunnel offloads can move if
> it's convenient.

I understand the argument WRT RSS. I think and hope RSS and GSO over
tunnel offload can follow different paths: I would opt to move around
the tunnel offload bit in the reserved range (and possibly make a
little more room there for other future features).

Cheers,

Paolo

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

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Thursday, May 1, 2025 7:13 PM
> To: Parav Pandit <parav@nvidia.com>
> Cc: Paolo Abeni <pabeni@redhat.com>; hengqi@linux.alibaba.com; virtio-
> comment@lists.linux.dev; cohuck@redhat.com; mvaralar@redhat.com; Jason
> Wang <jasowang@redhat.com>; Shahaf Shuler <shahafs@nvidia.com>;
> Willem de Bruijn <willemb@google.com>; Daniel Verkamp
> <dverkamp@chromium.org>
> Subject: Re: [PATCH v1] virtio-net: Fix to avoid using reserved feature bits
> 
> On Wed, Apr 30, 2025 at 10:54:12AM +0000, Parav Pandit wrote:
> >
> >
> > > From: Paolo Abeni <pabeni@redhat.com>
> > > Sent: Wednesday, April 30, 2025 3:42 PM
> > >
> > > On 4/30/25 6:44 AM, Parav Pandit wrote:
> > > >> From: Michael S. Tsirkin <mst@redhat.com>
> > > >> Sent: Wednesday, April 30, 2025 2:14 AM
> > > >>
> > > >> On Mon, Apr 28, 2025 at 10:39:59AM +0200, Paolo Abeni wrote:
> > > >>> On 4/23/25 8:07 PM, Michael S. Tsirkin wrote:
> > > >>>> On Wed, Apr 23, 2025 at 09:29:11AM -0700, Daniel Verkamp wrote:
> > > >>>>> On Tue, Apr 22, 2025 at 10:46 PM Michael S. Tsirkin
> > > >>>>> <mst@redhat.com>
> > > >> wrote:
> > > >>>>>> I'm afraid we'll have to bite the bullet.
> > > >>>>>
> > > >>>>> One other issue with bits > 63 is that the vhost-user protocol
> > > >>>>> VHOST_USER_GET_FEATURES and VHOST_USER_SET_FEATURES
> > > >> messages use
> > > >>>>> u64 to represent the features, so vhost-user-net devices can't
> > > >>>>> query or enable these features. vhost-user is outside the
> > > >>>>> scope of the virtio spec, though, and I think it's reasonable
> > > >>>>> to extend the protocol to enable high feature bits rather than
> avoiding them forever.
> > > >>>>
> > > >>>> Yes you would use VHOST_USER_SET_PROTOCOL_FEATURES to make
> > > >>>> VHOST_USER_GET_FEATURES return two u64s, or even a new
> message
> > > >> returning an array.
> > > >>>
> > > >>> I think that additionally the VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET
> > > >>> command will need some clarification, as in the current text
> > > >>> looks a bit
> > > >>> inconsistent:
> > > >>>
> > > >>> """
> > > >>> // in Offloads State Configuration / Setting Offloads State:
> > > >>>
> > > >>> #define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO  46
> > > >>>
> > > >>> // ...
> > > >>>
> > > >>> 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.
> > > >>>
> > > >>> There is a corresponding device feature for each offload. Upon
> > > >>> feature negotiation corresponding offload gets enabled to
> > > >>> preserve backward compatibility """
> > > >>>
> > > >>> The "corresponding device feature" has the same numerical value
> > > >>> of the selected offloads, except for UDP tunnels related one
> > > >>> (which are mapped to bits corresponding to reserved features).
> > > >>>
> > > >>> It's unclear to me which should be the better way to address
> > > >>> this inconsistency.
> > > >>>
> > > >>> /P
> > > >>
> > > >>
> > > >> Parav, what's your take here? Given your change broke
> > > >> VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET, do you want to revert it?
> > > >
> > > > I see two options.
> > > > Opt_1:
> > > > Open source Linux kernel driver and DPDK PMD has not used
> > > > RSS_CONTEXT
> > > yet.
> > > > If Heng from Alibaba acks that they do not have any internal
> > > implementation either, it may be safe to shift _all_ feature > 63 to
> > > lower position.
> > > > We can get Yuri's feedback, if at all windows driver has used RSS
> context.
> > > >
> > > > And once for all we mark it that feature bits are limited to 0-63.
> > > > There is enough infrastructure in place in virtio spec to not try
> > > > to squeeze
> > > things in feature bits.
> > > > And these 4 bits are good example of it already, which could have
> > > > been
> > > negotiated/communicated at later phase of driver at runtime.
> > > > Only bit required was a bit to expand vnet header size at early stage.
> > > >
> > > > Advantage: brings the good practice to adapt to the modern and
> > > > efficient
> > > driver->device interface.
> > > > Risk: May break RSS_CONTEXT (risk looks low)
> > > >
> > > > Opt_2. VIRTIO_NET_CTRL_GUEST_OFFLOADS command text to be
> updated
> > > to
> > > > indicate that,
> > > >
> > > > Below defines corresponds to respective feature bits 65 to 68.
> > > > There is still
> > > one to one mapping, its just position is different inside the class.
> > > > This is clarification text to be added and sw can adjust for it.
> > > >
> > > > #define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO 46 #define
> > > > VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO_CSUM 47 #define
> > > > VIRTIO_NET_F_GUEST_USO4 54 #define VIRTIO_NET_F_GUEST_USO6 55
> > > >
> > > > Advantages of 2nd option are:
> > > > a. featuring bits remain open upto 127.
> > > > b. Does not break RSS_CONTEXT.
> > > >
> > > > Both options are practical to me.
> > > > I prefer #1, if Heng acks it, but also ok for #2.
> > >
> > > I spent quite of bit of time trying to evaluate the scope of
> > > features bit expansion (implied by the option 2 above).
> > >
> > > While strictly speaking I haven't hit yet a complete blocker,
> > > implementation- wise it's going to be huge and error prone, as great
> > > deal of both the kernel and the user-space/qemu infrastructure
> > > hard-codes the
> > > 64 bit limit.
> > >
> > > Even exposing the feature extensions only the the virtio-net device
> > > (AFAICS it will "minimize" the code churn) a lot of code and devices
> > > implementations are going to be impacted.
> > >
> > > I expect a far away in time timeline for implementations based on option
> 2.
> > >
> > > /P
> > Given that its painful enough, we can update the virtio spec to limit to 64-bit
> features as option_1.
> >
> > A virtio spec diff like below,
> > - 0 to 23, and 50 to 127 Feature bits for the specific device type
> > + 0 to 23, and 50 to 63 Feature bits for the specific device type
> >
> > Otherwise, we are delaying the problem to next feature, which is not good.
> >
> > Heng,
> > Yuri already acked, so we are good from Windows side.
> > With that Windows, Linux kernel, DPDK PMD are safe.
> >
> > Can you also please confirm that there are no users within Alibaba relying
> on RSS_CONTEXT, so that shifting the feature bit will not break any existing
> functionality?
> > I think it will not break, because we can update the non-open-source
> software to detect RSS_CONTEXT on a new bit.
> >
> > For example, driver can do,
> > Enable_rss_context if bit X OR Bit 64 is set.
> 
> 
> sorry if I am unclear, I think it's too late to move RSS really. It has been out
> there for two years. The reason we can be reasonably sure no one
> implemented the offloads so far is because the set offloads command is
> broken and no one complained.  We need to fix them in the spec, current one
> is broken. So the tunnel offloads can move if it's convenient.

There are few proposals on table.

1. From Paolo,
- 0 to 23, and 50 to 127 Feature bits for the specific device type
+ 0 to 23, and 46 to 127 Feature bits for the specific device type

This does not have good reason of why it should still be 127.

2. From me:
- 0 to 23, and 50 to 127 Feature bits for the specific device type
+ 0 to 23, and 45 to 64 Feature bits for the specific device type

This is an extension of Paolo, to justify that implementing feature bits is extremely hard even for experts as pointed by Paolo.
It is worth to not extend it further.
RSS can be negotiated via new bit 44 in future bit as OR of 44 and 64 so that more wider users (Linux, freebsd, qnx, Windows, dpdk pmd) can pick 44.

3. From me:
Keep the feature bits encoding as is up to 127 bits, because may be there is (unknown and weird) value in having 127 feature bits.
(unknown because the reasoning of #1 and #3 mismatch).

In that case,
VIRTIO_NET_CTRL_GUEST_OFFLOADS command text to be updated to indicate feature fits A to D map to 
VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.offloads bits A' to D'.

I am fine with option #2 and #3.
Doing #1 for sure is wrong.
Wrong because it delays the problem of #1 from this to another feature [A] who's voting already completed.

[A] https://lore.kernel.org/virtio-comment/DM4PR18MB4269F73B786E83EF68A70F37DFB82@DM4PR18MB4269.namprd18.prod.outlook.com/T/#t

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

[Michael S. Tsirkin]

On Tue, May 06, 2025 at 06:15:41AM +0000, Parav Pandit wrote:
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Thursday, May 1, 2025 7:13 PM
> > To: Parav Pandit <parav@nvidia.com>
> > Cc: Paolo Abeni <pabeni@redhat.com>; hengqi@linux.alibaba.com; virtio-
> > comment@lists.linux.dev; cohuck@redhat.com; mvaralar@redhat.com; Jason
> > Wang <jasowang@redhat.com>; Shahaf Shuler <shahafs@nvidia.com>;
> > Willem de Bruijn <willemb@google.com>; Daniel Verkamp
> > <dverkamp@chromium.org>
> > Subject: Re: [PATCH v1] virtio-net: Fix to avoid using reserved feature bits
> > 
> > On Wed, Apr 30, 2025 at 10:54:12AM +0000, Parav Pandit wrote:
> > >
> > >
> > > > From: Paolo Abeni <pabeni@redhat.com>
> > > > Sent: Wednesday, April 30, 2025 3:42 PM
> > > >
> > > > On 4/30/25 6:44 AM, Parav Pandit wrote:
> > > > >> From: Michael S. Tsirkin <mst@redhat.com>
> > > > >> Sent: Wednesday, April 30, 2025 2:14 AM
> > > > >>
> > > > >> On Mon, Apr 28, 2025 at 10:39:59AM +0200, Paolo Abeni wrote:
> > > > >>> On 4/23/25 8:07 PM, Michael S. Tsirkin wrote:
> > > > >>>> On Wed, Apr 23, 2025 at 09:29:11AM -0700, Daniel Verkamp wrote:
> > > > >>>>> On Tue, Apr 22, 2025 at 10:46 PM Michael S. Tsirkin
> > > > >>>>> <mst@redhat.com>
> > > > >> wrote:
> > > > >>>>>> I'm afraid we'll have to bite the bullet.
> > > > >>>>>
> > > > >>>>> One other issue with bits > 63 is that the vhost-user protocol
> > > > >>>>> VHOST_USER_GET_FEATURES and VHOST_USER_SET_FEATURES
> > > > >> messages use
> > > > >>>>> u64 to represent the features, so vhost-user-net devices can't
> > > > >>>>> query or enable these features. vhost-user is outside the
> > > > >>>>> scope of the virtio spec, though, and I think it's reasonable
> > > > >>>>> to extend the protocol to enable high feature bits rather than
> > avoiding them forever.
> > > > >>>>
> > > > >>>> Yes you would use VHOST_USER_SET_PROTOCOL_FEATURES to make
> > > > >>>> VHOST_USER_GET_FEATURES return two u64s, or even a new
> > message
> > > > >> returning an array.
> > > > >>>
> > > > >>> I think that additionally the VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET
> > > > >>> command will need some clarification, as in the current text
> > > > >>> looks a bit
> > > > >>> inconsistent:
> > > > >>>
> > > > >>> """
> > > > >>> // in Offloads State Configuration / Setting Offloads State:
> > > > >>>
> > > > >>> #define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO  46
> > > > >>>
> > > > >>> // ...
> > > > >>>
> > > > >>> 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.
> > > > >>>
> > > > >>> There is a corresponding device feature for each offload. Upon
> > > > >>> feature negotiation corresponding offload gets enabled to
> > > > >>> preserve backward compatibility """
> > > > >>>
> > > > >>> The "corresponding device feature" has the same numerical value
> > > > >>> of the selected offloads, except for UDP tunnels related one
> > > > >>> (which are mapped to bits corresponding to reserved features).
> > > > >>>
> > > > >>> It's unclear to me which should be the better way to address
> > > > >>> this inconsistency.
> > > > >>>
> > > > >>> /P
> > > > >>
> > > > >>
> > > > >> Parav, what's your take here? Given your change broke
> > > > >> VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET, do you want to revert it?
> > > > >
> > > > > I see two options.
> > > > > Opt_1:
> > > > > Open source Linux kernel driver and DPDK PMD has not used
> > > > > RSS_CONTEXT
> > > > yet.
> > > > > If Heng from Alibaba acks that they do not have any internal
> > > > implementation either, it may be safe to shift _all_ feature > 63 to
> > > > lower position.
> > > > > We can get Yuri's feedback, if at all windows driver has used RSS
> > context.
> > > > >
> > > > > And once for all we mark it that feature bits are limited to 0-63.
> > > > > There is enough infrastructure in place in virtio spec to not try
> > > > > to squeeze
> > > > things in feature bits.
> > > > > And these 4 bits are good example of it already, which could have
> > > > > been
> > > > negotiated/communicated at later phase of driver at runtime.
> > > > > Only bit required was a bit to expand vnet header size at early stage.
> > > > >
> > > > > Advantage: brings the good practice to adapt to the modern and
> > > > > efficient
> > > > driver->device interface.
> > > > > Risk: May break RSS_CONTEXT (risk looks low)
> > > > >
> > > > > Opt_2. VIRTIO_NET_CTRL_GUEST_OFFLOADS command text to be
> > updated
> > > > to
> > > > > indicate that,
> > > > >
> > > > > Below defines corresponds to respective feature bits 65 to 68.
> > > > > There is still
> > > > one to one mapping, its just position is different inside the class.
> > > > > This is clarification text to be added and sw can adjust for it.
> > > > >
> > > > > #define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO 46 #define
> > > > > VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO_CSUM 47 #define
> > > > > VIRTIO_NET_F_GUEST_USO4 54 #define VIRTIO_NET_F_GUEST_USO6 55
> > > > >
> > > > > Advantages of 2nd option are:
> > > > > a. featuring bits remain open upto 127.
> > > > > b. Does not break RSS_CONTEXT.
> > > > >
> > > > > Both options are practical to me.
> > > > > I prefer #1, if Heng acks it, but also ok for #2.
> > > >
> > > > I spent quite of bit of time trying to evaluate the scope of
> > > > features bit expansion (implied by the option 2 above).
> > > >
> > > > While strictly speaking I haven't hit yet a complete blocker,
> > > > implementation- wise it's going to be huge and error prone, as great
> > > > deal of both the kernel and the user-space/qemu infrastructure
> > > > hard-codes the
> > > > 64 bit limit.
> > > >
> > > > Even exposing the feature extensions only the the virtio-net device
> > > > (AFAICS it will "minimize" the code churn) a lot of code and devices
> > > > implementations are going to be impacted.
> > > >
> > > > I expect a far away in time timeline for implementations based on option
> > 2.
> > > >
> > > > /P
> > > Given that its painful enough, we can update the virtio spec to limit to 64-bit
> > features as option_1.
> > >
> > > A virtio spec diff like below,
> > > - 0 to 23, and 50 to 127 Feature bits for the specific device type
> > > + 0 to 23, and 50 to 63 Feature bits for the specific device type
> > >
> > > Otherwise, we are delaying the problem to next feature, which is not good.
> > >
> > > Heng,
> > > Yuri already acked, so we are good from Windows side.
> > > With that Windows, Linux kernel, DPDK PMD are safe.
> > >
> > > Can you also please confirm that there are no users within Alibaba relying
> > on RSS_CONTEXT, so that shifting the feature bit will not break any existing
> > functionality?
> > > I think it will not break, because we can update the non-open-source
> > software to detect RSS_CONTEXT on a new bit.
> > >
> > > For example, driver can do,
> > > Enable_rss_context if bit X OR Bit 64 is set.
> > 
> > 
> > sorry if I am unclear, I think it's too late to move RSS really. It has been out
> > there for two years. The reason we can be reasonably sure no one
> > implemented the offloads so far is because the set offloads command is
> > broken and no one complained.  We need to fix them in the spec, current one
> > is broken. So the tunnel offloads can move if it's convenient.
> 
> There are few proposals on table.
> 
> 1. From Paolo,
> - 0 to 23, and 50 to 127 Feature bits for the specific device type
> + 0 to 23, and 46 to 127 Feature bits for the specific device type
> 
> This does not have good reason of why it should still be 127.
> 
> 2. From me:
> - 0 to 23, and 50 to 127 Feature bits for the specific device type
> + 0 to 23, and 45 to 64 Feature bits for the specific device type
> 
> This is an extension of Paolo, to justify that implementing feature bits is extremely hard even for experts as pointed by Paolo.
> It is worth to not extend it further.
> RSS can be negotiated via new bit 44 in future bit as OR of 44 and 64 so that more wider users (Linux, freebsd, qnx, Windows, dpdk pmd) can pick 44.
> 
> 3. From me:
> Keep the feature bits encoding as is up to 127 bits, because may be there is (unknown and weird) value in having 127 feature bits.
> (unknown because the reasoning of #1 and #3 mismatch).
> In that case,
> VIRTIO_NET_CTRL_GUEST_OFFLOADS command text to be updated to indicate feature fits A to D map to 
> VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.offloads bits A' to D'.
> 
> I am fine with option #2 and #3.
> Doing #1 for sure is wrong.
> Wrong because it delays the problem of #1 from this to another feature [A] who's voting already completed.
> 
> [A] https://lore.kernel.org/virtio-comment/DM4PR18MB4269F73B786E83EF68A70F37DFB82@DM4PR18MB4269.namprd18.prod.outlook.com/T/#t


#3 seems more conservate.

-- 
MST

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

[Parav Pandit]

Hi Paolo,

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Tuesday, May 6, 2025 1:26 PM
> 
> On Tue, May 06, 2025 at 06:15:41AM +0000, Parav Pandit wrote:
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Thursday, May 1, 2025 7:13 PM
> > > To: Parav Pandit <parav@nvidia.com>
> > > Cc: Paolo Abeni <pabeni@redhat.com>; hengqi@linux.alibaba.com;
> > > virtio- comment@lists.linux.dev; cohuck@redhat.com;
> > > mvaralar@redhat.com; Jason Wang <jasowang@redhat.com>; Shahaf
> Shuler
> > > <shahafs@nvidia.com>; Willem de Bruijn <willemb@google.com>; Daniel
> > > Verkamp <dverkamp@chromium.org>
> > > Subject: Re: [PATCH v1] virtio-net: Fix to avoid using reserved
> > > feature bits
> > >
> > > On Wed, Apr 30, 2025 at 10:54:12AM +0000, Parav Pandit wrote:
> > > >
> > > >
> > > > > From: Paolo Abeni <pabeni@redhat.com>
> > > > > Sent: Wednesday, April 30, 2025 3:42 PM
> > > > >
> > > > > On 4/30/25 6:44 AM, Parav Pandit wrote:
> > > > > >> From: Michael S. Tsirkin <mst@redhat.com>
> > > > > >> Sent: Wednesday, April 30, 2025 2:14 AM
> > > > > >>
> > > > > >> On Mon, Apr 28, 2025 at 10:39:59AM +0200, Paolo Abeni wrote:
> > > > > >>> On 4/23/25 8:07 PM, Michael S. Tsirkin wrote:
> > > > > >>>> On Wed, Apr 23, 2025 at 09:29:11AM -0700, Daniel Verkamp
> wrote:
> > > > > >>>>> On Tue, Apr 22, 2025 at 10:46 PM Michael S. Tsirkin
> > > > > >>>>> <mst@redhat.com>
> > > > > >> wrote:
> > > > > >>>>>> I'm afraid we'll have to bite the bullet.
> > > > > >>>>>
> > > > > >>>>> One other issue with bits > 63 is that the vhost-user
> > > > > >>>>> protocol VHOST_USER_GET_FEATURES and
> > > > > >>>>> VHOST_USER_SET_FEATURES
> > > > > >> messages use
> > > > > >>>>> u64 to represent the features, so vhost-user-net devices
> > > > > >>>>> can't query or enable these features. vhost-user is
> > > > > >>>>> outside the scope of the virtio spec, though, and I think
> > > > > >>>>> it's reasonable to extend the protocol to enable high
> > > > > >>>>> feature bits rather than
> > > avoiding them forever.
> > > > > >>>>
> > > > > >>>> Yes you would use VHOST_USER_SET_PROTOCOL_FEATURES to
> make
> > > > > >>>> VHOST_USER_GET_FEATURES return two u64s, or even a new
> > > message
> > > > > >> returning an array.
> > > > > >>>
> > > > > >>> I think that additionally the
> > > > > >>> VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET
> > > > > >>> command will need some clarification, as in the current text
> > > > > >>> looks a bit
> > > > > >>> inconsistent:
> > > > > >>>
> > > > > >>> """
> > > > > >>> // in Offloads State Configuration / Setting Offloads State:
> > > > > >>>
> > > > > >>> #define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO  46
> > > > > >>>
> > > > > >>> // ...
> > > > > >>>
> > > > > >>> 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.
> > > > > >>>
> > > > > >>> There is a corresponding device feature for each offload.
> > > > > >>> Upon feature negotiation corresponding offload gets enabled
> > > > > >>> to preserve backward compatibility """
> > > > > >>>
> > > > > >>> The "corresponding device feature" has the same numerical
> > > > > >>> value of the selected offloads, except for UDP tunnels
> > > > > >>> related one (which are mapped to bits corresponding to reserved
> features).
> > > > > >>>
> > > > > >>> It's unclear to me which should be the better way to address
> > > > > >>> this inconsistency.
> > > > > >>>
> > > > > >>> /P
> > > > > >>
> > > > > >>
> > > > > >> Parav, what's your take here? Given your change broke
> > > > > >> VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET, do you want to revert it?
> > > > > >
> > > > > > I see two options.
> > > > > > Opt_1:
> > > > > > Open source Linux kernel driver and DPDK PMD has not used
> > > > > > RSS_CONTEXT
> > > > > yet.
> > > > > > If Heng from Alibaba acks that they do not have any internal
> > > > > implementation either, it may be safe to shift _all_ feature >
> > > > > 63 to lower position.
> > > > > > We can get Yuri's feedback, if at all windows driver has used
> > > > > > RSS
> > > context.
> > > > > >
> > > > > > And once for all we mark it that feature bits are limited to 0-63.
> > > > > > There is enough infrastructure in place in virtio spec to not
> > > > > > try to squeeze
> > > > > things in feature bits.
> > > > > > And these 4 bits are good example of it already, which could
> > > > > > have been
> > > > > negotiated/communicated at later phase of driver at runtime.
> > > > > > Only bit required was a bit to expand vnet header size at early stage.
> > > > > >
> > > > > > Advantage: brings the good practice to adapt to the modern and
> > > > > > efficient
> > > > > driver->device interface.
> > > > > > Risk: May break RSS_CONTEXT (risk looks low)
> > > > > >
> > > > > > Opt_2. VIRTIO_NET_CTRL_GUEST_OFFLOADS command text to be
> > > updated
> > > > > to
> > > > > > indicate that,
> > > > > >
> > > > > > Below defines corresponds to respective feature bits 65 to 68.
> > > > > > There is still
> > > > > one to one mapping, its just position is different inside the class.
> > > > > > This is clarification text to be added and sw can adjust for it.
> > > > > >
> > > > > > #define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO 46 #define
> > > > > > VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO_CSUM 47 #define
> > > > > > VIRTIO_NET_F_GUEST_USO4 54 #define
> VIRTIO_NET_F_GUEST_USO6 55
> > > > > >
> > > > > > Advantages of 2nd option are:
> > > > > > a. featuring bits remain open upto 127.
> > > > > > b. Does not break RSS_CONTEXT.
> > > > > >
> > > > > > Both options are practical to me.
> > > > > > I prefer #1, if Heng acks it, but also ok for #2.
> > > > >
> > > > > I spent quite of bit of time trying to evaluate the scope of
> > > > > features bit expansion (implied by the option 2 above).
> > > > >
> > > > > While strictly speaking I haven't hit yet a complete blocker,
> > > > > implementation- wise it's going to be huge and error prone, as
> > > > > great deal of both the kernel and the user-space/qemu
> > > > > infrastructure hard-codes the
> > > > > 64 bit limit.
> > > > >
> > > > > Even exposing the feature extensions only the the virtio-net
> > > > > device (AFAICS it will "minimize" the code churn) a lot of code
> > > > > and devices implementations are going to be impacted.
> > > > >
> > > > > I expect a far away in time timeline for implementations based
> > > > > on option
> > > 2.
> > > > >
> > > > > /P
> > > > Given that its painful enough, we can update the virtio spec to
> > > > limit to 64-bit
> > > features as option_1.
> > > >
> > > > A virtio spec diff like below,
> > > > - 0 to 23, and 50 to 127 Feature bits for the specific device type
> > > > + 0 to 23, and 50 to 63 Feature bits for the specific device type
> > > >
> > > > Otherwise, we are delaying the problem to next feature, which is not
> good.
> > > >
> > > > Heng,
> > > > Yuri already acked, so we are good from Windows side.
> > > > With that Windows, Linux kernel, DPDK PMD are safe.
> > > >
> > > > Can you also please confirm that there are no users within Alibaba
> > > > relying
> > > on RSS_CONTEXT, so that shifting the feature bit will not break any
> > > existing functionality?
> > > > I think it will not break, because we can update the
> > > > non-open-source
> > > software to detect RSS_CONTEXT on a new bit.
> > > >
> > > > For example, driver can do,
> > > > Enable_rss_context if bit X OR Bit 64 is set.
> > >
> > >
> > > sorry if I am unclear, I think it's too late to move RSS really. It
> > > has been out there for two years. The reason we can be reasonably
> > > sure no one implemented the offloads so far is because the set
> > > offloads command is broken and no one complained.  We need to fix
> > > them in the spec, current one is broken. So the tunnel offloads can move
> if it's convenient.
> >
> > There are few proposals on table.
> >
> > 1. From Paolo,
> > - 0 to 23, and 50 to 127 Feature bits for the specific device type
> > + 0 to 23, and 46 to 127 Feature bits for the specific device type
> >
> > This does not have good reason of why it should still be 127.
> >
> > 2. From me:
> > - 0 to 23, and 50 to 127 Feature bits for the specific device type
> > + 0 to 23, and 45 to 64 Feature bits for the specific device type
> >
> > This is an extension of Paolo, to justify that implementing feature bits is
> extremely hard even for experts as pointed by Paolo.
> > It is worth to not extend it further.
> > RSS can be negotiated via new bit 44 in future bit as OR of 44 and 64 so that
> more wider users (Linux, freebsd, qnx, Windows, dpdk pmd) can pick 44.
> >
> > 3. From me:
> > Keep the feature bits encoding as is up to 127 bits, because may be there is
> (unknown and weird) value in having 127 feature bits.
> > (unknown because the reasoning of #1 and #3 mismatch).
> > In that case,
> > VIRTIO_NET_CTRL_GUEST_OFFLOADS command text to be updated to
> indicate
> > feature fits A to D map to VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.offloads
> bits A' to D'.
> >
> > I am fine with option #2 and #3.
> > Doing #1 for sure is wrong.
> > Wrong because it delays the problem of #1 from this to another feature [A]
> who's voting already completed.
> >
> > [A]
> > https://lore.kernel.org/virtio-
> comment/DM4PR18MB4269F73B786E83EF68A70F
> > 37DFB82@DM4PR18MB4269.namprd18.prod.outlook.com/T/#t
> 
> 
> #3 seems more conservate.

Are you good with #3?

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

[Paolo Abeni]

Hi all,

On 5/6/25 10:56 AM, Parav Pandit wrote:
>> From: Michael S. Tsirkin <mst@redhat.com>
>> Sent: Tuesday, May 6, 2025 1:26 PM
>>> There are few proposals on table.
>>>
>>> 1. From Paolo,
>>> - 0 to 23, and 50 to 127 Feature bits for the specific device type
>>> + 0 to 23, and 46 to 127 Feature bits for the specific device type
>>>
>>> This does not have good reason of why it should still be 127.
>>>
>>> 2. From me:
>>> - 0 to 23, and 50 to 127 Feature bits for the specific device type
>>> + 0 to 23, and 45 to 64 Feature bits for the specific device type
>>>
>>> This is an extension of Paolo, to justify that implementing feature bits is
>> extremely hard even for experts as pointed by Paolo.
>>> It is worth to not extend it further.
>>> RSS can be negotiated via new bit 44 in future bit as OR of 44 and 64 so that
>> more wider users (Linux, freebsd, qnx, Windows, dpdk pmd) can pick 44.
>>>
>>> 3. From me:
>>> Keep the feature bits encoding as is up to 127 bits, because may be there is
>> (unknown and weird) value in having 127 feature bits.
>>> (unknown because the reasoning of #1 and #3 mismatch).
>>> In that case,
>>> VIRTIO_NET_CTRL_GUEST_OFFLOADS command text to be updated to
>> indicate
>>> feature fits A to D map to VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.offloads
>> bits A' to D'.
>>>
>>> I am fine with option #2 and #3.
>>> Doing #1 for sure is wrong.
>>> Wrong because it delays the problem of #1 from this to another feature [A]
>> who's voting already completed.
>>>
>>> [A]
>>> https://lore.kernel.org/virtio-
>> comment/DM4PR18MB4269F73B786E83EF68A70F
>>> 37DFB82@DM4PR18MB4269.namprd18.prod.outlook.com/T/#t
>>
>>
>> #3 seems more conservate.
> 
> Are you good with #3?

I'm sorry for the latency. Let me double check to avoid possible
misunderstanding; #3 means:

- 0 to 23, and 50 to 127 Feature bits for the specific device type
+ 0 to 23, and 45 to 127 Feature bits for the specific device type

using bits 46-39 for UDP tunnel offloads and likely bit 45 for
VIRTIO_NET_F_OUT_NET_HEADER.

The VIRTIO_NET_F_CTRL_GUEST_OFFLOADS mapping should be specified after
eventually a new offload feature will be defined using a bit >= 64.

Am I correct?

Thanks,

Paolo

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

[Parav Pandit]

> From: Paolo Abeni <pabeni@redhat.com>
> Sent: Tuesday, May 6, 2025 8:09 PM
> 
> Hi all,
> 
> On 5/6/25 10:56 AM, Parav Pandit wrote:
> >> From: Michael S. Tsirkin <mst@redhat.com>
> >> Sent: Tuesday, May 6, 2025 1:26 PM
> >>> There are few proposals on table.
> >>>
> >>> 1. From Paolo,
> >>> - 0 to 23, and 50 to 127 Feature bits for the specific device type
> >>> + 0 to 23, and 46 to 127 Feature bits for the specific device type
> >>>
> >>> This does not have good reason of why it should still be 127.
> >>>
> >>> 2. From me:
> >>> - 0 to 23, and 50 to 127 Feature bits for the specific device type
> >>> + 0 to 23, and 45 to 64 Feature bits for the specific device type
> >>>
> >>> This is an extension of Paolo, to justify that implementing feature
> >>> bits is
> >> extremely hard even for experts as pointed by Paolo.
> >>> It is worth to not extend it further.
> >>> RSS can be negotiated via new bit 44 in future bit as OR of 44 and
> >>> 64 so that
> >> more wider users (Linux, freebsd, qnx, Windows, dpdk pmd) can pick 44.
> >>>
> >>> 3. From me:
> >>> Keep the feature bits encoding as is up to 127 bits, because may be
> >>> there is
> >> (unknown and weird) value in having 127 feature bits.
> >>> (unknown because the reasoning of #1 and #3 mismatch).
> >>> In that case,
> >>> VIRTIO_NET_CTRL_GUEST_OFFLOADS command text to be updated to
> >> indicate
> >>> feature fits A to D map to
> VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.offloads
> >> bits A' to D'.
> >>>
> >>> I am fine with option #2 and #3.
> >>> Doing #1 for sure is wrong.
> >>> Wrong because it delays the problem of #1 from this to another
> >>> feature [A]
> >> who's voting already completed.
> >>>
> >>> [A]
> >>> https://lore.kernel.org/virtio-
> >> comment/DM4PR18MB4269F73B786E83EF68A70F
> >>> 37DFB82@DM4PR18MB4269.namprd18.prod.outlook.com/T/#t
> >>
> >>
> >> #3 seems more conservate.
> >
> > Are you good with #3?
> 
> I'm sorry for the latency. Let me double check to avoid possible
> misunderstanding; #3 means:
> 
> - 0 to 23, and 50 to 127 Feature bits for the specific device type
> + 0 to 23, and 45 to 127 Feature bits for the specific device type
> 
No change in above feature bits.

> using bits 46-39 for UDP tunnel offloads and likely bit 45 for
> VIRTIO_NET_F_OUT_NET_HEADER.
>
This also to use bit 69 as proposed.
 
> The VIRTIO_NET_F_CTRL_GUEST_OFFLOADS mapping should be specified
> after eventually a new offload feature will be defined using a bit >= 64.
>
No. UDP tunnel feature bits 65 to 68 maps to command bits 46,47,48,49.
This is the only description change in VIRTIO_NET_F_CTRL_GUEST_OFFLOADS command.
Would it work?

In fact, this command extension alone would have been sufficient if the vnet_hdr had a fixed size where new fields could be added.

> Am I correct?
> 
> Thanks,
> 
> Paolo

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

[Paolo Abeni]

On 5/6/25 5:00 PM, Parav Pandit wrote:
>> From: Paolo Abeni <pabeni@redhat.com>
>> Sent: Tuesday, May 6, 2025 8:09 PM
>> On 5/6/25 10:56 AM, Parav Pandit wrote:
>>>> From: Michael S. Tsirkin <mst@redhat.com>
>>>> Sent: Tuesday, May 6, 2025 1:26 PM
>>>>> There are few proposals on table.
>>>>>
>>>>> 1. From Paolo,
>>>>> - 0 to 23, and 50 to 127 Feature bits for the specific device type
>>>>> + 0 to 23, and 46 to 127 Feature bits for the specific device type
>>>>>
>>>>> This does not have good reason of why it should still be 127.
>>>>>
>>>>> 2. From me:
>>>>> - 0 to 23, and 50 to 127 Feature bits for the specific device type
>>>>> + 0 to 23, and 45 to 64 Feature bits for the specific device type
>>>>>
>>>>> This is an extension of Paolo, to justify that implementing feature
>>>>> bits is
>>>> extremely hard even for experts as pointed by Paolo.
>>>>> It is worth to not extend it further.
>>>>> RSS can be negotiated via new bit 44 in future bit as OR of 44 and
>>>>> 64 so that
>>>> more wider users (Linux, freebsd, qnx, Windows, dpdk pmd) can pick 44.
>>>>>
>>>>> 3. From me:
>>>>> Keep the feature bits encoding as is up to 127 bits, because may be
>>>>> there is
>>>> (unknown and weird) value in having 127 feature bits.
>>>>> (unknown because the reasoning of #1 and #3 mismatch).
>>>>> In that case,
>>>>> VIRTIO_NET_CTRL_GUEST_OFFLOADS command text to be updated to
>>>> indicate
>>>>> feature fits A to D map to
>> VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.offloads
>>>> bits A' to D'.
>>>>>
>>>>> I am fine with option #2 and #3.
>>>>> Doing #1 for sure is wrong.
>>>>> Wrong because it delays the problem of #1 from this to another
>>>>> feature [A]
>>>> who's voting already completed.
>>>>>
>>>>> [A]
>>>>> https://lore.kernel.org/virtio-
>>>> comment/DM4PR18MB4269F73B786E83EF68A70F
>>>>> 37DFB82@DM4PR18MB4269.namprd18.prod.outlook.com/T/#t
>>>>
>>>>
>>>> #3 seems more conservate.
>>>
>>> Are you good with #3?
>>
>> I'm sorry for the latency. Let me double check to avoid possible
>> misunderstanding; #3 means:
>>
>> - 0 to 23, and 50 to 127 Feature bits for the specific device type
>> + 0 to 23, and 45 to 127 Feature bits for the specific device type
>>
> No change in above feature bits.
> 
>> using bits 46-39 for UDP tunnel offloads and likely bit 45 for
>> VIRTIO_NET_F_OUT_NET_HEADER.
>>
> This also to use bit 69 as proposed.
>  
>> The VIRTIO_NET_F_CTRL_GUEST_OFFLOADS mapping should be specified
>> after eventually a new offload feature will be defined using a bit >= 64.
>>
> No. UDP tunnel feature bits 65 to 68 maps to command bits 46,47,48,49.
> This is the only description change in VIRTIO_NET_F_CTRL_GUEST_OFFLOADS command.
> Would it work?

AFAICT, yes, it should work.

But it will not avoid the immediate need to expand the virtio features
negotiation above 64 bits, with the already mentioned complexity.

I would preferably avoid that, if possible: I restarted this thread with
such a goal.

Note: the implementation complexity spark from u64 hard-coded
"everywhere" in both the kernel, qemu and the related APIs.

> In fact, this command extension alone would have been sufficient if the vnet_hdr had a fixed size where new fields could be added.

I could not parse this last statement. The virtio_net_hdr size depends
on the negotiated features, and AFAICT  VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
can't replace features negotiation. Could you please rephrase?

Thanks,

Paolo

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

[Parav Pandit]

> From: Paolo Abeni <pabeni@redhat.com>
> Sent: Tuesday, May 6, 2025 9:10 PM
> 
> On 5/6/25 5:00 PM, Parav Pandit wrote:
> >> From: Paolo Abeni <pabeni@redhat.com>
> >> Sent: Tuesday, May 6, 2025 8:09 PM
> >> On 5/6/25 10:56 AM, Parav Pandit wrote:
> >>>> From: Michael S. Tsirkin <mst@redhat.com>
> >>>> Sent: Tuesday, May 6, 2025 1:26 PM
> >>>>> There are few proposals on table.
> >>>>>
> >>>>> 1. From Paolo,
> >>>>> - 0 to 23, and 50 to 127 Feature bits for the specific device type
> >>>>> + 0 to 23, and 46 to 127 Feature bits for the specific device type
> >>>>>
> >>>>> This does not have good reason of why it should still be 127.
> >>>>>
> >>>>> 2. From me:
> >>>>> - 0 to 23, and 50 to 127 Feature bits for the specific device type
> >>>>> + 0 to 23, and 45 to 64 Feature bits for the specific device type
> >>>>>
> >>>>> This is an extension of Paolo, to justify that implementing
> >>>>> feature bits is
> >>>> extremely hard even for experts as pointed by Paolo.
> >>>>> It is worth to not extend it further.
> >>>>> RSS can be negotiated via new bit 44 in future bit as OR of 44 and
> >>>>> 64 so that
> >>>> more wider users (Linux, freebsd, qnx, Windows, dpdk pmd) can pick 44.
> >>>>>
> >>>>> 3. From me:
> >>>>> Keep the feature bits encoding as is up to 127 bits, because may
> >>>>> be there is
> >>>> (unknown and weird) value in having 127 feature bits.
> >>>>> (unknown because the reasoning of #1 and #3 mismatch).
> >>>>> In that case,
> >>>>> VIRTIO_NET_CTRL_GUEST_OFFLOADS command text to be updated to
> >>>> indicate
> >>>>> feature fits A to D map to
> >> VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.offloads
> >>>> bits A' to D'.
> >>>>>
> >>>>> I am fine with option #2 and #3.
> >>>>> Doing #1 for sure is wrong.
> >>>>> Wrong because it delays the problem of #1 from this to another
> >>>>> feature [A]
> >>>> who's voting already completed.
> >>>>>
> >>>>> [A]
> >>>>> https://lore.kernel.org/virtio-
> >>>> comment/DM4PR18MB4269F73B786E83EF68A70F
> >>>>> 37DFB82@DM4PR18MB4269.namprd18.prod.outlook.com/T/#t
> >>>>
> >>>>
> >>>> #3 seems more conservate.
> >>>
> >>> Are you good with #3?
> >>
> >> I'm sorry for the latency. Let me double check to avoid possible
> >> misunderstanding; #3 means:
> >>
> >> - 0 to 23, and 50 to 127 Feature bits for the specific device type
> >> + 0 to 23, and 45 to 127 Feature bits for the specific device type
> >>
> > No change in above feature bits.
> >
> >> using bits 46-39 for UDP tunnel offloads and likely bit 45 for
> >> VIRTIO_NET_F_OUT_NET_HEADER.
> >>
> > This also to use bit 69 as proposed.
> >
> >> The VIRTIO_NET_F_CTRL_GUEST_OFFLOADS mapping should be specified
> >> after eventually a new offload feature will be defined using a bit >= 64.
> >>
> > No. UDP tunnel feature bits 65 to 68 maps to command bits 46,47,48,49.
> > This is the only description change in
> VIRTIO_NET_F_CTRL_GUEST_OFFLOADS command.
> > Would it work?
> 
> AFAICT, yes, it should work.
> 
> But it will not avoid the immediate need to expand the virtio features
> negotiation above 64 bits, with the already mentioned complexity.
> 
> I would preferably avoid that, if possible: I restarted this thread with such a
> goal.
> 
In that case we should adopt #2.

> Note: the implementation complexity spark from u64 hard-coded
> "everywhere" in both the kernel, qemu and the related APIs.
> 
Hence it applies to already 2nd feature which also needs new feature bit.

> > In fact, this command extension alone would have been sufficient if the
> vnet_hdr had a fixed size where new fields could be added.
> 
> I could not parse this last statement. The virtio_net_hdr size depends on the
> negotiated features, and AFAICT  VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
> can't replace features negotiation. Could you please rephrase?
>
I meant to say, if we have fixed size vnet hdr, say 64B, than there is no need of feature negotiation per offload.
VIRTIO_NET_F_CTRL_GUEST_OFFLOADS or any other command can dynamically enable/disable a feature on invocation of ethtool/netlink/devlink or similar OS UAPIs.
It does not apply presently to this feature as the infrastructure of 64B vnet hdr is not in present and it cannot a fix anyway for this issue.
It was just a forward thinking on how to build the optimize data path.
 
> Thanks,
> 
> Paolo

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

[Paolo Abeni]

On 5/6/25 6:20 PM, Parav Pandit wrote:
> From: Paolo Abeni <pabeni@redhat.com Sent: Tuesday, May 6, 2025 9:10 PM
>> On 5/6/25 5:00 PM, Parav Pandit wrote:
>>> From: Paolo Abeni <pabeni@redhat.com Sent: Tuesday, May 6, 2025 8:09 PM
>>>> On 5/6/25 10:56 AM, Parav Pandit wrote:
>>>>> Are you good with #3?
>>>>
>>>> I'm sorry for the latency. Let me double check to avoid possible
>>>> misunderstanding; #3 means:
>>>>
>>>> - 0 to 23, and 50 to 127 Feature bits for the specific device type
>>>> + 0 to 23, and 45 to 127 Feature bits for the specific device type
>>>>
>>> No change in above feature bits.
>>>
>>>> using bits 46-39 for UDP tunnel offloads and likely bit 45 for
>>>> VIRTIO_NET_F_OUT_NET_HEADER.
>>>>
>>> This also to use bit 69 as proposed.
>>>
>>>> The VIRTIO_NET_F_CTRL_GUEST_OFFLOADS mapping should be specified
>>>> after eventually a new offload feature will be defined using a bit >= 64.
>>>>
>>> No. UDP tunnel feature bits 65 to 68 maps to command bits 46,47,48,49.
>>> This is the only description change in
>> VIRTIO_NET_F_CTRL_GUEST_OFFLOADS command.
>>> Would it work?
>>
>> AFAICT, yes, it should work.
>>
>> But it will not avoid the immediate need to expand the virtio features
>> negotiation above 64 bits, with the already mentioned complexity.
>>
>> I would preferably avoid that, if possible: I restarted this thread with such a
>> goal.
>>
> In that case we should adopt #2.

Do we have quorum? Should I send a patch?

>>> In fact, this command extension alone would have been sufficient if the
>> vnet_hdr had a fixed size where new fields could be added.
>>
>> I could not parse this last statement. The virtio_net_hdr size depends on the
>> negotiated features, and AFAICT  VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
>> can't replace features negotiation. Could you please rephrase?
>>
> I meant to say, if we have fixed size vnet hdr, say 64B, than there is no need of feature negotiation per offload.
> VIRTIO_NET_F_CTRL_GUEST_OFFLOADS or any other command can dynamically enable/disable a feature on invocation of ethtool/netlink/devlink or similar OS UAPIs.
> It does not apply presently to this feature as the infrastructure of 64B vnet hdr is not in present and it cannot a fix anyway for this issue.
> It was just a forward thinking on how to build the optimize data path.

Understood. I'm unsure if that schema could have issues with migration
vs VMs with different offloads, but let's put that aside from the main
topic here ;)

Thanks,

Paolo

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

[Michael S. Tsirkin]

On Wed, May 07, 2025 at 11:57:26AM +0200, Paolo Abeni wrote:
> On 5/6/25 6:20 PM, Parav Pandit wrote:
> > From: Paolo Abeni <pabeni@redhat.com Sent: Tuesday, May 6, 2025 9:10 PM
> >> On 5/6/25 5:00 PM, Parav Pandit wrote:
> >>> From: Paolo Abeni <pabeni@redhat.com Sent: Tuesday, May 6, 2025 8:09 PM
> >>>> On 5/6/25 10:56 AM, Parav Pandit wrote:
> >>>>> Are you good with #3?
> >>>>
> >>>> I'm sorry for the latency. Let me double check to avoid possible
> >>>> misunderstanding; #3 means:
> >>>>
> >>>> - 0 to 23, and 50 to 127 Feature bits for the specific device type
> >>>> + 0 to 23, and 45 to 127 Feature bits for the specific device type
> >>>>
> >>> No change in above feature bits.
> >>>
> >>>> using bits 46-39 for UDP tunnel offloads and likely bit 45 for
> >>>> VIRTIO_NET_F_OUT_NET_HEADER.
> >>>>
> >>> This also to use bit 69 as proposed.
> >>>
> >>>> The VIRTIO_NET_F_CTRL_GUEST_OFFLOADS mapping should be specified
> >>>> after eventually a new offload feature will be defined using a bit >= 64.
> >>>>
> >>> No. UDP tunnel feature bits 65 to 68 maps to command bits 46,47,48,49.
> >>> This is the only description change in
> >> VIRTIO_NET_F_CTRL_GUEST_OFFLOADS command.
> >>> Would it work?
> >>
> >> AFAICT, yes, it should work.
> >>
> >> But it will not avoid the immediate need to expand the virtio features
> >> negotiation above 64 bits, with the already mentioned complexity.
> >>
> >> I would preferably avoid that, if possible: I restarted this thread with such a
> >> goal.
> >>
> > In that case we should adopt #2.
> 
> Do we have quorum? Should I send a patch?

I'm curious how hard is 128 bit support, gimme a couple
of days to try and maybe post a patch, just for comparison.

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

[Paolo Abeni]

On 5/7/25 11:57 AM, Paolo Abeni wrote:
> On 5/6/25 6:20 PM, Parav Pandit wrote:
>> From: Paolo Abeni <pabeni@redhat.com Sent: Tuesday, May 6, 2025 9:10 PM
>>> On 5/6/25 5:00 PM, Parav Pandit wrote:
>>>> From: Paolo Abeni <pabeni@redhat.com Sent: Tuesday, May 6, 2025 8:09 PM
>>>>> On 5/6/25 10:56 AM, Parav Pandit wrote:
>>>>>> Are you good with #3?
>>>>>
>>>>> I'm sorry for the latency. Let me double check to avoid possible
>>>>> misunderstanding; #3 means:
>>>>>
>>>>> - 0 to 23, and 50 to 127 Feature bits for the specific device type
>>>>> + 0 to 23, and 45 to 127 Feature bits for the specific device type
>>>>>
>>>> No change in above feature bits.
>>>>
>>>>> using bits 46-39 for UDP tunnel offloads and likely bit 45 for
>>>>> VIRTIO_NET_F_OUT_NET_HEADER.
>>>>>
>>>> This also to use bit 69 as proposed.
>>>>
>>>>> The VIRTIO_NET_F_CTRL_GUEST_OFFLOADS mapping should be specified
>>>>> after eventually a new offload feature will be defined using a bit >= 64.
>>>>>
>>>> No. UDP tunnel feature bits 65 to 68 maps to command bits 46,47,48,49.
>>>> This is the only description change in
>>> VIRTIO_NET_F_CTRL_GUEST_OFFLOADS command.
>>>> Would it work?
>>>
>>> AFAICT, yes, it should work.
>>>
>>> But it will not avoid the immediate need to expand the virtio features
>>> negotiation above 64 bits, with the already mentioned complexity.
>>>
>>> I would preferably avoid that, if possible: I restarted this thread with such a
>>> goal.
>>>
>> In that case we should adopt #2.
> 
> Do we have quorum? Should I send a patch?

As per off-list discussion with Michel, there is no agreement on
reserved bits re-use.

That means the only available option is #3 above.

@Parav, would you send a patch to fix the offloads <> features mapping,
or do you prefer I'll do it?

Thanks,

Paolo

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

[Parav Pandit]

> From: Paolo Abeni <pabeni@redhat.com>
> Sent: Monday, May 19, 2025 2:28 PM
> 
> On 5/7/25 11:57 AM, Paolo Abeni wrote:
> > On 5/6/25 6:20 PM, Parav Pandit wrote:
> >> From: Paolo Abeni <pabeni@redhat.com Sent: Tuesday, May 6, 2025 9:10
> >> PM
> >>> On 5/6/25 5:00 PM, Parav Pandit wrote:
> >>>> From: Paolo Abeni <pabeni@redhat.com Sent: Tuesday, May 6, 2025
> >>>> 8:09 PM
> >>>>> On 5/6/25 10:56 AM, Parav Pandit wrote:
> >>>>>> Are you good with #3?
> >>>>>
> >>>>> I'm sorry for the latency. Let me double check to avoid possible
> >>>>> misunderstanding; #3 means:
> >>>>>
> >>>>> - 0 to 23, and 50 to 127 Feature bits for the specific device type
> >>>>> + 0 to 23, and 45 to 127 Feature bits for the specific device type
> >>>>>
> >>>> No change in above feature bits.
> >>>>
> >>>>> using bits 46-39 for UDP tunnel offloads and likely bit 45 for
> >>>>> VIRTIO_NET_F_OUT_NET_HEADER.
> >>>>>
> >>>> This also to use bit 69 as proposed.
> >>>>
> >>>>> The VIRTIO_NET_F_CTRL_GUEST_OFFLOADS mapping should be
> specified
> >>>>> after eventually a new offload feature will be defined using a bit >= 64.
> >>>>>
> >>>> No. UDP tunnel feature bits 65 to 68 maps to command bits
> 46,47,48,49.
> >>>> This is the only description change in
> >>> VIRTIO_NET_F_CTRL_GUEST_OFFLOADS command.
> >>>> Would it work?
> >>>
> >>> AFAICT, yes, it should work.
> >>>
> >>> But it will not avoid the immediate need to expand the virtio
> >>> features negotiation above 64 bits, with the already mentioned
> complexity.
> >>>
> >>> I would preferably avoid that, if possible: I restarted this thread
> >>> with such a goal.
> >>>
> >> In that case we should adopt #2.
> >
> > Do we have quorum? Should I send a patch?
> 
> As per off-list discussion with Michel, there is no agreement on reserved bits
> re-use.
> 
> That means the only available option is #3 above.
> 
> @Parav, would you send a patch to fix the offloads <> features mapping, or do
> you prefer I'll do it?
> 
If its urgent, please do it.
If it can wait till 22 May this week, I will prepare one.

> Thanks,
> 
> Paolo

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

[Paolo Abeni]

On 5/19/25 11:04 AM, Parav Pandit wrote:
>> From: Paolo Abeni <pabeni@redhat.com>
>> Sent: Monday, May 19, 2025 2:28 PM
>> On 5/7/25 11:57 AM, Paolo Abeni wrote:
>>> On 5/6/25 6:20 PM, Parav Pandit wrote:
>>>> From: Paolo Abeni <pabeni@redhat.com Sent: Tuesday, May 6, 2025 9:10
>>>> PM
>>>>> On 5/6/25 5:00 PM, Parav Pandit wrote:
>>>>>> From: Paolo Abeni <pabeni@redhat.com Sent: Tuesday, May 6, 2025
>>>>>> 8:09 PM
>>>>>>> On 5/6/25 10:56 AM, Parav Pandit wrote:
>>>>>>>> Are you good with #3?
>>>>>>>
>>>>>>> I'm sorry for the latency. Let me double check to avoid possible
>>>>>>> misunderstanding; #3 means:
>>>>>>>
>>>>>>> - 0 to 23, and 50 to 127 Feature bits for the specific device type
>>>>>>> + 0 to 23, and 45 to 127 Feature bits for the specific device type
>>>>>>>
>>>>>> No change in above feature bits.
>>>>>>
>>>>>>> using bits 46-39 for UDP tunnel offloads and likely bit 45 for
>>>>>>> VIRTIO_NET_F_OUT_NET_HEADER.
>>>>>>>
>>>>>> This also to use bit 69 as proposed.
>>>>>>
>>>>>>> The VIRTIO_NET_F_CTRL_GUEST_OFFLOADS mapping should be
>> specified
>>>>>>> after eventually a new offload feature will be defined using a bit >= 64.
>>>>>>>
>>>>>> No. UDP tunnel feature bits 65 to 68 maps to command bits
>> 46,47,48,49.
>>>>>> This is the only description change in
>>>>> VIRTIO_NET_F_CTRL_GUEST_OFFLOADS command.
>>>>>> Would it work?
>>>>>
>>>>> AFAICT, yes, it should work.
>>>>>
>>>>> But it will not avoid the immediate need to expand the virtio
>>>>> features negotiation above 64 bits, with the already mentioned
>> complexity.
>>>>>
>>>>> I would preferably avoid that, if possible: I restarted this thread
>>>>> with such a goal.
>>>>>
>>>> In that case we should adopt #2.
>>>
>>> Do we have quorum? Should I send a patch?
>>
>> As per off-list discussion with Michel, there is no agreement on reserved bits
>> re-use.
>>
>> That means the only available option is #3 above.
>>
>> @Parav, would you send a patch to fix the offloads <> features mapping, or do
>> you prefer I'll do it?
>>
> If its urgent, please do it.
> If it can wait till 22 May this week, I will prepare one.

I'll do, because I want to move this thing forward. Note that I think
it's better to change the offload bit name definition for UDP related
offload, to avoid multiple definitions using specifying the string and
different value. i.e.:

---
diff --git a/device-types/net/description.tex
b/device-types/net/description.tex
index 1b6b54d..5d869bc 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 bit in the [65 to 68] range correspond to
+offloads 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