Re: [PATCH v6 1/2] virtio-net: Fix ECN feature descritpion

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Apr 15, 2025 at 01:33:45PM +0000, Chia-Yu Chang (Nokia) wrote:
> > -----Original Message-----
> > From: Michael S. Tsirkin <mst@redhat.com> 
> > Sent: Monday, April 14, 2025 8:59 PM
> > To: Chia-Yu Chang (Nokia) <chia-yu.chang@nokia-bell-labs.com>
> > Cc: virtio-comment@lists.linux.dev; cohuck@redhat.com; mvaralar@redhat.com; jasowang@redhat.com; xuanzhuo@linux.alibaba.com; eperezma@redhat.com; ij@kernel.org; ncardwell@google.com; Koen De Schepper (Nokia) <koen.de_schepper@nokia-bell-labs.com>; g.white <g.white@cablelabs.com>; vidhi_goel <vidhi_goel@apple.com>; ingemar.s.johansson@ericsson.com; mirja.kuehlewind@ericsson.com
> > Subject: Re: [PATCH v6 1/2] virtio-net: Fix ECN feature descritpion
> > 
> > 
> > CAUTION: This is an external email. Please be very careful when clicking links or opening attachments. See the URL nok.it/ext for additional information.
> > 
> > 
> > 
> > On Mon, Apr 14, 2025 at 05:34:24PM +0200, chia-yu.chang@nokia-bell-labs.com wrote:
> > > From: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
> > >
> > > Clarify that the VIRTIO_NET_HDR_GSO_ECN gso_type flag does not mean 
> > > that TCP has IP-ECN set, instead it identifies TCP CWR flag is set and 
> > > will be cleared from the second segment. This is used to offload TCP 
> > > CWR flag in a way that is compatible with RFC3168 ECN but is 
> > > problematic for non-RFC3168 use of CWR flag.
> > >
> > > Signed-off-by: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
> > > ---
> > 
> > 
> > if this is accepted, we might want to backport this to 1.3 maybe?
> 
> Thanks for feedback.
> In my opinion, this fix shall be backported to clarify its relation to RFC3168 ECN.
> Shall we make further patch, or you will do the merge once this patch is accepted?
> 
> Best regards,
> Chia-Yu

For the backport, you will need to open another github issue.


> > 
> > 
> > >  device-types/net/description.tex | 33 +++++++++++++++++---------------
> > >  introduction.tex                 |  3 +++
> > >  2 files changed, 21 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/device-types/net/description.tex 
> > > b/device-types/net/description.tex
> > > index 1b6b54d..a2c9de8 100644
> > > --- a/device-types/net/description.tex
> > > +++ b/device-types/net/description.tex
> > > @@ -54,7 +54,9 @@ \subsection{Feature bits}\label{sec:Device Types / 
> > > Network Device / Feature bits
> > >
> > >  \item[VIRTIO_NET_F_GUEST_TSO6 (8)] Driver can receive TSOv6.
> > >
> > > -\item[VIRTIO_NET_F_GUEST_ECN (9)] Driver can receive TSO with ECN.
> > > +\item[VIRTIO_NET_F_GUEST_ECN (9)] Driver can receive TSO with TCP CWR flag set
> > > +    and follow the ACE bits handling approach mentioned in
> > > +    \hyperref[intro:rfc3168]{[RFC3168]}.
> > >
> > >  \item[VIRTIO_NET_F_GUEST_UFO (10)] Driver can receive UFO.
> > >
> > > @@ -62,7 +64,9 @@ \subsection{Feature bits}\label{sec:Device Types / 
> > > Network Device / Feature bits
> > >
> > >  \item[VIRTIO_NET_F_HOST_TSO6 (12)] Device can receive TSOv6.
> > >
> > > -\item[VIRTIO_NET_F_HOST_ECN (13)] Device can receive TSO with ECN.
> > > +\item[VIRTIO_NET_F_HOST_ECN (13)] Device can receive TSO with TCP CWR flag set
> > > +    and follow the ACE bits handling approach mentioend in
> > > +    \hyperref[intro:rfc3168]{[RFC3168]}.
> > >
> > >  \item[VIRTIO_NET_F_HOST_UFO (14)] Device can receive UFO.
> > >
> > > @@ -695,8 +699,9 @@ \subsubsection{Packet 
> > > Transmission}\label{sec:Device Types / Network Device / De
> > >
> > >    \item If the driver negotiated the VIRTIO_NET_F_HOST_ECN feature,
> > >      the VIRTIO_NET_HDR_GSO_ECN bit in \field{gso_type}
> > > -    indicates that the TCP packet has the ECN bit set\footnote{This case is not handled by some older hardware, so is called out
> > > -specifically in the protocol.}.
> > > +    indicates that the TCP packet has TCP CWR flag set and the flag will be handled differently to all segements of
> > > +    an aggregated segment, as mentioned in \hyperref[intro:rfc3168]{[RFC3168]}
> > > +    \footnote{This case is not handled by some older hardware, so is called out specifically in the protocol.}.
> > >     \end{itemize}
> > >
> > >  \item If the driver negotiated the VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO 
> > > feature and the @@ -788,10 +793,9 @@ \subsubsection{Packet 
> > > Transmission}\label{sec:Device Types / Network Device / De  \field{gso_type} to VIRTIO_NET_HDR_GSO_UDP_L4.
> > >
> > >  The driver SHOULD NOT send to the device TCP packets requiring 
> > > segmentation offload -which have the Explicit Congestion Notification 
> > > bit set, unless the -VIRTIO_NET_F_HOST_ECN feature is negotiated, in 
> > > which case the -driver MUST set the VIRTIO_NET_HDR_GSO_ECN bit in 
> > > -\field{gso_type}.
> > > +which have the TCP CWR flag set and require the flag be handled as 
> > > +mentioned in \hyperref[intro:rfc3168]{[RFC3168]}, unless the 
> > > +VIRTIO_NET_F_HOST_ECN feature is negotiated, in which case the driver MUST set the VIRTIO_NET_HDR_GSO_ECN bit in \field{gso_type}.
> > >
> > >  If VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO is negotiated, the driver MAY set
> > >  VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV4 bit or the 
> > > VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV6 bit @@ -1105,9 +1109,9 @@ 
> > > \subsubsection{Processing of Incoming Packets}\label{sec:Device Types 
> > > / Network  \end{enumerate}
> > >
> > >  Additionally, VIRTIO_NET_F_GUEST_CSUM, TSO4, TSO6, UDP, UDP_TUNNEL 
> > > -and ECN features enable receive checksum, large receive offload and 
> > > ECN -support which are the input equivalents of the transmit checksum, 
> > > -transmit segmentation offloading and ECN features, as described
> > > +and ECN features enable receive checksum, large receive offload and 
> > > +RFC3168 ECN support which are the input equivalents of the transmit 
> > > +checksum, transmit segmentation offloading and RFC3168 ECN features, 
> > > +as described
> > >  in \ref{sec:Device Types / Network Device / Device Operation /  
> > > Packet Transmission}:
> > >  \begin{enumerate}
> > > @@ -1210,10 +1214,9 @@ \subsubsection{Processing of Incoming 
> > > Packets}\label{sec:Device Types / Network  the VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM bit in \field{flags}.
> > >
> > >  The device SHOULD NOT send to the driver TCP packets requiring 
> > > segmentation offload -which have the Explicit Congestion Notification 
> > > bit set, unless the -VIRTIO_NET_F_GUEST_ECN feature is negotiated, in 
> > > which case the -device MUST set the VIRTIO_NET_HDR_GSO_ECN bit in 
> > > -\field{gso_type}.
> > > +which have the TCP CWR flag set and require the flag be handled as 
> > > +mentioned in \hyperref[intro:rfc3168]{[RFC3168]}, unless the 
> > > +VIRTIO_NET_F_GUEST_ECN feature is negotiated, in which case the device MUST set the VIRTIO_NET_HDR_GSO_ECN bit in \field{gso_type}.
> > >
> > >  If the VIRTIO_NET_F_GUEST_CSUM feature has been negotiated, the  
> > > device MAY set the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in diff --git 
> > > a/introduction.tex b/introduction.tex index e60298a..d52622e 100644
> > > --- a/introduction.tex
> > > +++ b/introduction.tex
> > > @@ -168,6 +168,9 @@ \section{Normative References}\label{sec:Normative References}
> > >      Leiba, B., "Ambiguity of Uppercase vs Lowercase in RFC 2119 Key Words", BCP
> > >      14, RFC 8174, DOI 10.17487/RFC8174, May 2017
> > >          \newline\url{http://www.ietf.org/rfc/rfc8174.txt}\\
> > > +     \phantomsection\label{intro:rfc3168}\textbf{[RFC3168]} &
> > > +    S. Floyd., ``The Addition of Explicit Congestion Notification (ECN) to IP'', September 2001.
> > > +     \newline\url{http://www.ietf.org/rfc/rfc3168.txt}\\
> > >  \end{longtable}
> > >
> > >  \section{Non-Normative References}
> > > --
> > > 2.34.1