[virtio-comment] [PATCH v2] virtio-net: support distinguishing between partial and full checksum

[virtio-comment] [PATCH v2] virtio-net: support distinguishing between partial and full checksum

[Heng Qi]

virtio-net works in a virtualized system and is somewhat different from
physical nics. One of the differences is that to save virtio device
resources, rx may receive packets with partial checksum. However, XDP may
cause partially checksummed packets to be dropped. So XDP loading conflicts
with the feature VIRTIO_NET_F_GUEST_CSUM.

This patch lets the device to supply fully checksummed packets to the driver.
Then XDP can coexist with VIRTIO_NET_F_GUEST_CSUM to enjoy the benefits of
device verification checksum.

In addition, implementation of some performant devices do not generate
partially checksummed packets, but the standard driver still need to clear
VIRTIO_NET_F_GUEST_CSUM when loading XDP. If these devices enable the
full checksum offloading, then the driver can load XDP without clearing
VIRTIO_NET_F_GUEST_CSUM.

A new feature bit VIRTIO_NET_F_GUEST_FULL_CSUM is added to solve the above
situation, which provides the driver with configurable receive full checksum
offload. If the offload is enabled, then the device must supply fully
checksummed packets to the driver.

Use case example:
If VIRTIO_NET_F_GUEST_FULL_CSUM is negotiated and receive full checksum
offload is enabled, after XDP processes a packet with full checksum, the
VIRTIO_NET_HDR_F_DATA_VALID bit is still retained, resulting in the stack
not needing to validate the checksum again. This is useful for guests:
  1. Bring the driver advantages such as cpu savings.
  2. For devices that do not generate partially checksummed packets themselves,
     XDP can be loaded in the driver without modifying the hardware behavior.

Several solutions have been discussed in the previous proposal[1].
After historical discussion, we have tried the method proposed by Jason[2],
but some complex scenarios and challenges are difficult to deal with.
We now return to the method suggested in [1].

[1] https://lists.oasis-open.org/archives/virtio-dev/202305/msg00291.html
[2] https://lore.kernel.org/all/20230628030506.2213-1-hengqi@linux.alibaba.com/

Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
v1->v2:
    1. Modify full checksum functionality as a configurable offload
       that is initially turned off. @Jason

 device-types/net/description.tex | 54 ++++++++++++++++++++++++++++----
 1 file changed, 48 insertions(+), 6 deletions(-)

diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index 76585b0..3c34f27 100644
--- a/device-types/net/description.tex
+++ b/device-types/net/description.tex
@@ -88,6 +88,8 @@ \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_FULL_CSUM (50)] Driver handles packets with full checksum.
+
 \item[VIRTIO_NET_F_HASH_TUNNEL(51)] Device supports inner header hash for encapsulated packets.
 
 \item[VIRTIO_NET_F_VQ_NOTF_COAL(52)] Device supports virtqueue notification coalescing.
@@ -133,6 +135,7 @@ \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device
 \item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM.
 \item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM.
 \item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM.
+\item[VIRTIO_NET_F_GUEST_FULL_CSUM] Requires VIRTIO_NET_F_GUEST_CSUM and VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
 
 \item[VIRTIO_NET_F_HOST_TSO4] Requires VIRTIO_NET_F_CSUM.
 \item[VIRTIO_NET_F_HOST_TSO6] Requires VIRTIO_NET_F_CSUM.
@@ -390,6 +393,13 @@ \subsection{Device Initialization}\label{sec:Device Types / Network Device / Dev
 \ref{sec:Device Types / Network Device / Device Operation /
 Processing of Incoming Packets}~\nameref{sec:Device Types /
 Network Device / Device Operation / Processing of Incoming Packets} below.
+
+\item The VIRTIO_NET_F_GUEST_FULL_CSUM feature indicates that the driver handles
+  packets with full checksum and does not handle packets with partial checksum,
+  which means that partially checksummed packets can not be received by the driver
+  and the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in \field{flags} can not be set by the device.
+  The driver can send the VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET command to control
+  the receive full checksum offload state.
 \end{enumerate}
 
 A truly minimal driver would only accept VIRTIO_NET_F_MAC and ignore
@@ -720,7 +730,8 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
   \field{num_buffers} is one, then the entire packet will be
   contained within this buffer, immediately following the struct
   virtio_net_hdr.
-\item If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the
+\item If the VIRTIO_NET_F_GUEST_CSUM feature (regardless of whether
+  VIRTIO_NET_F_GUEST_FULL_CSUM was negotiated) was negotiated, the
   VIRTIO_NET_HDR_F_DATA_VALID bit in \field{flags} can be
   set: if so, device has validated the packet checksum.
   In case of multiple encapsulated protocols, one level of checksums
@@ -744,7 +755,8 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
   number of coalesced TCP segments in \field{csum_start} field and
   number of duplicated ACK segments in \field{csum_offset} field
   and sets bit VIRTIO_NET_HDR_F_RSC_INFO in \field{flags}.
-\item If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the
+\item If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated but the
+  VIRTIO_NET_F_GUEST_FULL_CSUM feature was not negotiated, the
   VIRTIO_NET_HDR_F_NEEDS_CSUM bit in \field{flags} can be
   set: if so, the packet checksum at offset \field{csum_offset}
   from \field{csum_start} and any preceding checksums
@@ -802,8 +814,9 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
 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
+If the VIRTIO_NET_F_GUEST_CSUM feature has been negotiated but
+the VIRTIO_NET_F_GUEST_FULL_CSUM feature has not been negotiated,
+the device MAY set the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in
 \field{flags}, if so:
 \begin{enumerate}
 \item the device MUST validate the packet checksum at
@@ -819,11 +832,38 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
 	fully checksummed packet;
 \end{enumerate}
 
+The driver MUST NOT enable receive full checksum offload for which
+VIRTIO_NET_F_GUEST_FULL_CSUM has not been negotiated.
+
+Initially (before the device receives any VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET
+command carrying the VIRTIO_NET_F_GUEST_FULL_CSUM feature parameter) receive
+full checksum offload MUST be disabled.
+
+Upon the device reset, the device MUST disable receive full checksum offload.
+
+If VIRTIO_NET_F_GUEST_FULL_CSUM has been negotiated and receive full checksum
+offload has not been enabled, the device MUST NOT perform any of the
+functionality provided by VIRTIO_NET_F_GUEST_FULL_CSUM.
+
+If receive full checksum offload has been enabled, the device MUST NOT set
+the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in \field{flags} and MUST supply a
+fully checksummed packet to the driver.
+
+If a partially checksummed packet is received by the device, the device MUST
+calculate full checksum for the packet and then supply it to the driver
+\ref{sec:Device Types / Network Device / Device Operation / Packet Transmission}.
+
 If none of the VIRTIO_NET_F_GUEST_TSO4, TSO6, UFO, USO4 or USO6 options have
 been negotiated, the device MUST set \field{gso_type} to
 VIRTIO_NET_HDR_GSO_NONE.
 
-If \field{gso_type} differs from VIRTIO_NET_HDR_GSO_NONE, then
+If receive full checksum offload has been enabled and \field{gso_type}
+differs from VIRTIO_NET_HDR_GSO_NONE, then the device MUST NOT set
+the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in \field{flags} and MUST calculate
+full checksum for the packet and then supply it to the driver.
+
+If the VIRTIO_NET_F_GUEST_FULL_CSUM feature has not been negotiated and
+\field{gso_type} differs from VIRTIO_NET_HDR_GSO_NONE, then
 the device MUST also set the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in
 \field{flags} MUST set \field{gso_size} to indicate the desired MSS.
 If VIRTIO_NET_F_RSC_EXT was negotiated, the device MUST also
@@ -839,7 +879,8 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
 not less than the length of the headers, including the transport
 header.
 
-If the VIRTIO_NET_F_GUEST_CSUM feature has been negotiated, the
+If the VIRTIO_NET_F_GUEST_CSUM feature (regardless of whether
+VIRTIO_NET_F_GUEST_FULL_CSUM has been negotiated) has been negotiated, the
 device MAY set the VIRTIO_NET_HDR_F_DATA_VALID bit in
 \field{flags}, if so, the device MUST validate the packet
 checksum (in case of multiple encapsulated protocols, one level
@@ -1624,6 +1665,7 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
 #define VIRTIO_NET_F_GUEST_TSO6       8
 #define VIRTIO_NET_F_GUEST_ECN        9
 #define VIRTIO_NET_F_GUEST_UFO        10
+#define VIRTIO_NET_F_GUEST_FULL_CSUM  50
 #define VIRTIO_NET_F_GUEST_USO4       54
 #define VIRTIO_NET_F_GUEST_USO6       55
 
-- 
2.19.1.6.gb485710b


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v2] virtio-net: support distinguishing between partial and full checksum

[Heng Qi]

Hi Jason and all, do you have any more comments on this version?^^

This feature is important for users using XDP on Alibaba Cloud.

Thanks!

在 2023/10/19 下午2:17, Heng Qi 写道:
> virtio-net works in a virtualized system and is somewhat different from
> physical nics. One of the differences is that to save virtio device
> resources, rx may receive packets with partial checksum. However, XDP may
> cause partially checksummed packets to be dropped. So XDP loading conflicts
> with the feature VIRTIO_NET_F_GUEST_CSUM.
>
> This patch lets the device to supply fully checksummed packets to the driver.
> Then XDP can coexist with VIRTIO_NET_F_GUEST_CSUM to enjoy the benefits of
> device verification checksum.
>
> In addition, implementation of some performant devices do not generate
> partially checksummed packets, but the standard driver still need to clear
> VIRTIO_NET_F_GUEST_CSUM when loading XDP. If these devices enable the
> full checksum offloading, then the driver can load XDP without clearing
> VIRTIO_NET_F_GUEST_CSUM.
>
> A new feature bit VIRTIO_NET_F_GUEST_FULL_CSUM is added to solve the above
> situation, which provides the driver with configurable receive full checksum
> offload. If the offload is enabled, then the device must supply fully
> checksummed packets to the driver.
>
> Use case example:
> If VIRTIO_NET_F_GUEST_FULL_CSUM is negotiated and receive full checksum
> offload is enabled, after XDP processes a packet with full checksum, the
> VIRTIO_NET_HDR_F_DATA_VALID bit is still retained, resulting in the stack
> not needing to validate the checksum again. This is useful for guests:
>    1. Bring the driver advantages such as cpu savings.
>    2. For devices that do not generate partially checksummed packets themselves,
>       XDP can be loaded in the driver without modifying the hardware behavior.
>
> Several solutions have been discussed in the previous proposal[1].
> After historical discussion, we have tried the method proposed by Jason[2],
> but some complex scenarios and challenges are difficult to deal with.
> We now return to the method suggested in [1].
>
> [1] https://lists.oasis-open.org/archives/virtio-dev/202305/msg00291.html
> [2] https://lore.kernel.org/all/20230628030506.2213-1-hengqi@linux.alibaba.com/
>
> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
> v1->v2:
>      1. Modify full checksum functionality as a configurable offload
>         that is initially turned off. @Jason
>
>   device-types/net/description.tex | 54 ++++++++++++++++++++++++++++----
>   1 file changed, 48 insertions(+), 6 deletions(-)
>
> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> index 76585b0..3c34f27 100644
> --- a/device-types/net/description.tex
> +++ b/device-types/net/description.tex
> @@ -88,6 +88,8 @@ \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_FULL_CSUM (50)] Driver handles packets with full checksum.
> +
>   \item[VIRTIO_NET_F_HASH_TUNNEL(51)] Device supports inner header hash for encapsulated packets.
>   
>   \item[VIRTIO_NET_F_VQ_NOTF_COAL(52)] Device supports virtqueue notification coalescing.
> @@ -133,6 +135,7 @@ \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device
>   \item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM.
>   \item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM.
>   \item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM.
> +\item[VIRTIO_NET_F_GUEST_FULL_CSUM] Requires VIRTIO_NET_F_GUEST_CSUM and VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
>   
>   \item[VIRTIO_NET_F_HOST_TSO4] Requires VIRTIO_NET_F_CSUM.
>   \item[VIRTIO_NET_F_HOST_TSO6] Requires VIRTIO_NET_F_CSUM.
> @@ -390,6 +393,13 @@ \subsection{Device Initialization}\label{sec:Device Types / Network Device / Dev
>   \ref{sec:Device Types / Network Device / Device Operation /
>   Processing of Incoming Packets}~\nameref{sec:Device Types /
>   Network Device / Device Operation / Processing of Incoming Packets} below.
> +
> +\item The VIRTIO_NET_F_GUEST_FULL_CSUM feature indicates that the driver handles
> +  packets with full checksum and does not handle packets with partial checksum,
> +  which means that partially checksummed packets can not be received by the driver
> +  and the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in \field{flags} can not be set by the device.
> +  The driver can send the VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET command to control
> +  the receive full checksum offload state.
>   \end{enumerate}
>   
>   A truly minimal driver would only accept VIRTIO_NET_F_MAC and ignore
> @@ -720,7 +730,8 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>     \field{num_buffers} is one, then the entire packet will be
>     contained within this buffer, immediately following the struct
>     virtio_net_hdr.
> -\item If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the
> +\item If the VIRTIO_NET_F_GUEST_CSUM feature (regardless of whether
> +  VIRTIO_NET_F_GUEST_FULL_CSUM was negotiated) was negotiated, the
>     VIRTIO_NET_HDR_F_DATA_VALID bit in \field{flags} can be
>     set: if so, device has validated the packet checksum.
>     In case of multiple encapsulated protocols, one level of checksums
> @@ -744,7 +755,8 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>     number of coalesced TCP segments in \field{csum_start} field and
>     number of duplicated ACK segments in \field{csum_offset} field
>     and sets bit VIRTIO_NET_HDR_F_RSC_INFO in \field{flags}.
> -\item If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the
> +\item If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated but the
> +  VIRTIO_NET_F_GUEST_FULL_CSUM feature was not negotiated, the
>     VIRTIO_NET_HDR_F_NEEDS_CSUM bit in \field{flags} can be
>     set: if so, the packet checksum at offset \field{csum_offset}
>     from \field{csum_start} and any preceding checksums
> @@ -802,8 +814,9 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>   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
> +If the VIRTIO_NET_F_GUEST_CSUM feature has been negotiated but
> +the VIRTIO_NET_F_GUEST_FULL_CSUM feature has not been negotiated,
> +the device MAY set the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in
>   \field{flags}, if so:
>   \begin{enumerate}
>   \item the device MUST validate the packet checksum at
> @@ -819,11 +832,38 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>   	fully checksummed packet;
>   \end{enumerate}
>   
> +The driver MUST NOT enable receive full checksum offload for which
> +VIRTIO_NET_F_GUEST_FULL_CSUM has not been negotiated.
> +
> +Initially (before the device receives any VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET
> +command carrying the VIRTIO_NET_F_GUEST_FULL_CSUM feature parameter) receive
> +full checksum offload MUST be disabled.
> +
> +Upon the device reset, the device MUST disable receive full checksum offload.
> +
> +If VIRTIO_NET_F_GUEST_FULL_CSUM has been negotiated and receive full checksum
> +offload has not been enabled, the device MUST NOT perform any of the
> +functionality provided by VIRTIO_NET_F_GUEST_FULL_CSUM.
> +
> +If receive full checksum offload has been enabled, the device MUST NOT set
> +the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in \field{flags} and MUST supply a
> +fully checksummed packet to the driver.
> +
> +If a partially checksummed packet is received by the device, the device MUST
> +calculate full checksum for the packet and then supply it to the driver
> +\ref{sec:Device Types / Network Device / Device Operation / Packet Transmission}.
> +
>   If none of the VIRTIO_NET_F_GUEST_TSO4, TSO6, UFO, USO4 or USO6 options have
>   been negotiated, the device MUST set \field{gso_type} to
>   VIRTIO_NET_HDR_GSO_NONE.
>   
> -If \field{gso_type} differs from VIRTIO_NET_HDR_GSO_NONE, then
> +If receive full checksum offload has been enabled and \field{gso_type}
> +differs from VIRTIO_NET_HDR_GSO_NONE, then the device MUST NOT set
> +the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in \field{flags} and MUST calculate
> +full checksum for the packet and then supply it to the driver.
> +
> +If the VIRTIO_NET_F_GUEST_FULL_CSUM feature has not been negotiated and
> +\field{gso_type} differs from VIRTIO_NET_HDR_GSO_NONE, then
>   the device MUST also set the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in
>   \field{flags} MUST set \field{gso_size} to indicate the desired MSS.
>   If VIRTIO_NET_F_RSC_EXT was negotiated, the device MUST also
> @@ -839,7 +879,8 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>   not less than the length of the headers, including the transport
>   header.
>   
> -If the VIRTIO_NET_F_GUEST_CSUM feature has been negotiated, the
> +If the VIRTIO_NET_F_GUEST_CSUM feature (regardless of whether
> +VIRTIO_NET_F_GUEST_FULL_CSUM has been negotiated) has been negotiated, the
>   device MAY set the VIRTIO_NET_HDR_F_DATA_VALID bit in
>   \field{flags}, if so, the device MUST validate the packet
>   checksum (in case of multiple encapsulated protocols, one level
> @@ -1624,6 +1665,7 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>   #define VIRTIO_NET_F_GUEST_TSO6       8
>   #define VIRTIO_NET_F_GUEST_ECN        9
>   #define VIRTIO_NET_F_GUEST_UFO        10
> +#define VIRTIO_NET_F_GUEST_FULL_CSUM  50
>   #define VIRTIO_NET_F_GUEST_USO4       54
>   #define VIRTIO_NET_F_GUEST_USO6       55
>   


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v2] virtio-net: support distinguishing between partial and full checksum

[Michael S. Tsirkin]

On Thu, Oct 19, 2023 at 02:17:20PM +0800, Heng Qi wrote:
> virtio-net works in a virtualized system and is somewhat different from
> physical nics. One of the differences is that to save virtio device
> resources, rx may receive packets with partial checksum. However, XDP may
> cause partially checksummed packets to be dropped. So XDP loading conflicts
> with the feature VIRTIO_NET_F_GUEST_CSUM.
> 
> This patch lets the device to supply fully checksummed packets to the driver.
> Then XDP can coexist with VIRTIO_NET_F_GUEST_CSUM to enjoy the benefits of
> device verification checksum.
> 
> In addition, implementation of some performant devices do not generate
> partially checksummed packets, but the standard driver still need to clear
> VIRTIO_NET_F_GUEST_CSUM when loading XDP. If these devices enable the
> full checksum offloading, then the driver can load XDP without clearing
> VIRTIO_NET_F_GUEST_CSUM.
> 
> A new feature bit VIRTIO_NET_F_GUEST_FULL_CSUM is added to solve the above
> situation, which provides the driver with configurable receive full checksum
> offload. If the offload is enabled, then the device must supply fully
> checksummed packets to the driver.
> 
> Use case example:
> If VIRTIO_NET_F_GUEST_FULL_CSUM is negotiated and receive full checksum
> offload is enabled, after XDP processes a packet with full checksum, the
> VIRTIO_NET_HDR_F_DATA_VALID bit is still retained, resulting in the stack
> not needing to validate the checksum again. This is useful for guests:
>   1. Bring the driver advantages such as cpu savings.
>   2. For devices that do not generate partially checksummed packets themselves,
>      XDP can be loaded in the driver without modifying the hardware behavior.
> 
> Several solutions have been discussed in the previous proposal[1].
> After historical discussion, we have tried the method proposed by Jason[2],
> but some complex scenarios and challenges are difficult to deal with.
> We now return to the method suggested in [1].
> 
> [1] https://lists.oasis-open.org/archives/virtio-dev/202305/msg00291.html
> [2] https://lore.kernel.org/all/20230628030506.2213-1-hengqi@linux.alibaba.com/
> 
> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
> v1->v2:
>     1. Modify full checksum functionality as a configurable offload
>        that is initially turned off. @Jason
> 
>  device-types/net/description.tex | 54 ++++++++++++++++++++++++++++----
>  1 file changed, 48 insertions(+), 6 deletions(-)
> 
> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> index 76585b0..3c34f27 100644
> --- a/device-types/net/description.tex
> +++ b/device-types/net/description.tex
> @@ -88,6 +88,8 @@ \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_FULL_CSUM (50)] Driver handles packets with full checksum.
> +
>  \item[VIRTIO_NET_F_HASH_TUNNEL(51)] Device supports inner header hash for encapsulated packets.
>  
>  \item[VIRTIO_NET_F_VQ_NOTF_COAL(52)] Device supports virtqueue notification coalescing.
> @@ -133,6 +135,7 @@ \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device
>  \item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM.
>  \item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM.
>  \item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM.
> +\item[VIRTIO_NET_F_GUEST_FULL_CSUM] Requires VIRTIO_NET_F_GUEST_CSUM and VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
>  
>  \item[VIRTIO_NET_F_HOST_TSO4] Requires VIRTIO_NET_F_CSUM.
>  \item[VIRTIO_NET_F_HOST_TSO6] Requires VIRTIO_NET_F_CSUM.

What about all of these:

device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_TSO4] Requires VIRTIO_NET_F_GUEST_CSUM.
device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_TSO6] Requires VIRTIO_NET_F_GUEST_CSUM.
device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM.
device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM.
device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM.



can TSO/UFO/USO work with VIRTIO_NET_F_GUEST_FULL_CSUM as opposed to VIRTIO_NET_F_GUEST_CSUM?


> @@ -390,6 +393,13 @@ \subsection{Device Initialization}\label{sec:Device Types / Network Device / Dev
>  \ref{sec:Device Types / Network Device / Device Operation /
>  Processing of Incoming Packets}~\nameref{sec:Device Types /
>  Network Device / Device Operation / Processing of Incoming Packets} below.
> +
> +\item The VIRTIO_NET_F_GUEST_FULL_CSUM feature indicates that the driver handles
> +  packets with full checksum and does not handle packets with partial checksum,


So we need to change definition of VIRTIO_NET_F_GUEST_CSUM then.


Also this is not exactly right. As defined driver must be able to handle
partial checksum too.


How about this:

- change definition above to just "Driver handles packets with full checksum."

- if VIRTIO_NET_F_GUEST_FULL_CSUM is set but VIRTIO_NET_F_GUEST_CSUM is
  clear driver requires full checksum

- if VIRTIO_NET_F_GUEST_FULL_CSUM is clear but VIRTIO_NET_F_GUEST_CSUM is
  set driver supports partial checksum

- if VIRTIO_NET_F_GUEST_FULL_CSUM and VIRTIO_NET_F_GUEST_CSUM are
  set then the behavior is as you describe: VIRTIO_NET_F_GUEST_CSUM
  takes preference, but you can disable it with VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
  if that is supported.


> +  which means that partially checksummed packets can not be received by the driver
> +  and the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in \field{flags} can not be set by the device.
> +  The driver can send the VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET command to control
> +  the receive full checksum offload state.


I don't like it that we are coming with "offload state" here that isn't
explained anywhere. Let's try to make this offload more like
other existing offloads.

I find it is also confusing to say "receive full checksum offload has
been enabled". It is more that driver handles packets with full checksum.

>  \end{enumerate}
>  
>  A truly minimal driver would only accept VIRTIO_NET_F_MAC and ignore
> @@ -720,7 +730,8 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>    \field{num_buffers} is one, then the entire packet will be
>    contained within this buffer, immediately following the struct
>    virtio_net_hdr.
> -\item If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the
> +\item If the VIRTIO_NET_F_GUEST_CSUM feature (regardless of whether
> +  VIRTIO_NET_F_GUEST_FULL_CSUM was negotiated) was negotiated, the
>    VIRTIO_NET_HDR_F_DATA_VALID bit in \field{flags} can be
>    set: if so, device has validated the packet checksum.
>    In case of multiple encapsulated protocols, one level of checksums
> @@ -744,7 +755,8 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>    number of coalesced TCP segments in \field{csum_start} field and
>    number of duplicated ACK segments in \field{csum_offset} field
>    and sets bit VIRTIO_NET_HDR_F_RSC_INFO in \field{flags}.
> -\item If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the
> +\item If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated but the
> +  VIRTIO_NET_F_GUEST_FULL_CSUM feature was not negotiated, the
>    VIRTIO_NET_HDR_F_NEEDS_CSUM bit in \field{flags} can be
>    set: if so, the packet checksum at offset \field{csum_offset}
>    from \field{csum_start} and any preceding checksums
> @@ -802,8 +814,9 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>  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
> +If the VIRTIO_NET_F_GUEST_CSUM feature has been negotiated but
> +the VIRTIO_NET_F_GUEST_FULL_CSUM feature has not been negotiated,
> +the device MAY set the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in
>  \field{flags}, if so:
>  \begin{enumerate}
>  \item the device MUST validate the packet checksum at
> @@ -819,11 +832,38 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>  	fully checksummed packet;
>  \end{enumerate}
>  
> +The driver MUST NOT enable receive full checksum offload for which
> +VIRTIO_NET_F_GUEST_FULL_CSUM has not been negotiated.
> +
> +Initially (before the device receives any VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET
> +command carrying the VIRTIO_NET_F_GUEST_FULL_CSUM feature parameter) receive
> +full checksum offload MUST be disabled.
> +
> +Upon the device reset, the device MUST disable receive full checksum offload.
> +
> +If VIRTIO_NET_F_GUEST_FULL_CSUM has been negotiated and receive full checksum
> +offload has not been enabled, the device MUST NOT perform any of the
> +functionality provided by VIRTIO_NET_F_GUEST_FULL_CSUM.
> +
> +If receive full checksum offload has been enabled, the device MUST NOT set
> +the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in \field{flags} and MUST supply a
> +fully checksummed packet to the driver.
> +
> +If a partially checksummed packet is received by the device, the device MUST
> +calculate full checksum for the packet and then supply it to the driver
> +\ref{sec:Device Types / Network Device / Device Operation / Packet Transmission}.
> +
>  If none of the VIRTIO_NET_F_GUEST_TSO4, TSO6, UFO, USO4 or USO6 options have
>  been negotiated, the device MUST set \field{gso_type} to
>  VIRTIO_NET_HDR_GSO_NONE.
>  
> -If \field{gso_type} differs from VIRTIO_NET_HDR_GSO_NONE, then
> +If receive full checksum offload has been enabled and \field{gso_type}
> +differs from VIRTIO_NET_HDR_GSO_NONE, then the device MUST NOT set
> +the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in \field{flags} and MUST calculate
> +full checksum for the packet and then supply it to the driver.
> +
> +If the VIRTIO_NET_F_GUEST_FULL_CSUM feature has not been negotiated and
> +\field{gso_type} differs from VIRTIO_NET_HDR_GSO_NONE, then
>  the device MUST also set the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in
>  \field{flags} MUST set \field{gso_size} to indicate the desired MSS.
>  If VIRTIO_NET_F_RSC_EXT was negotiated, the device MUST also
> @@ -839,7 +879,8 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>  not less than the length of the headers, including the transport
>  header.
>  
> -If the VIRTIO_NET_F_GUEST_CSUM feature has been negotiated, the
> +If the VIRTIO_NET_F_GUEST_CSUM feature (regardless of whether
> +VIRTIO_NET_F_GUEST_FULL_CSUM has been negotiated) has been negotiated, the
>  device MAY set the VIRTIO_NET_HDR_F_DATA_VALID bit in
>  \field{flags}, if so, the device MUST validate the packet
>  checksum (in case of multiple encapsulated protocols, one level
> @@ -1624,6 +1665,7 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>  #define VIRTIO_NET_F_GUEST_TSO6       8
>  #define VIRTIO_NET_F_GUEST_ECN        9
>  #define VIRTIO_NET_F_GUEST_UFO        10
> +#define VIRTIO_NET_F_GUEST_FULL_CSUM  50
>  #define VIRTIO_NET_F_GUEST_USO4       54
>  #define VIRTIO_NET_F_GUEST_USO6       55
>  
> -- 
> 2.19.1.6.gb485710b
> 
> 
> This publicly archived list offers a means to provide input to the
> OASIS Virtual I/O Device (VIRTIO) TC.
> 
> In order to verify user consent to the Feedback License terms and
> to minimize spam in the list archive, subscription is required
> before posting.
> 
> Subscribe: virtio-comment-subscribe@lists.oasis-open.org
> Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
> List help: virtio-comment-help@lists.oasis-open.org
> List archive: https://lists.oasis-open.org/archives/virtio-comment/
> Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
> List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
> Committee: https://www.oasis-open.org/committees/virtio/
> Join OASIS: https://www.oasis-open.org/join/
> 


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v2] virtio-net: support distinguishing between partial and full checksum

[Xuan Zhuo]

On Fri, 27 Oct 2023 03:39:48 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Oct 19, 2023 at 02:17:20PM +0800, Heng Qi wrote:
> > virtio-net works in a virtualized system and is somewhat different from
> > physical nics. One of the differences is that to save virtio device
> > resources, rx may receive packets with partial checksum. However, XDP may
> > cause partially checksummed packets to be dropped. So XDP loading conflicts
> > with the feature VIRTIO_NET_F_GUEST_CSUM.
> >
> > This patch lets the device to supply fully checksummed packets to the driver.
> > Then XDP can coexist with VIRTIO_NET_F_GUEST_CSUM to enjoy the benefits of
> > device verification checksum.
> >
> > In addition, implementation of some performant devices do not generate
> > partially checksummed packets, but the standard driver still need to clear
> > VIRTIO_NET_F_GUEST_CSUM when loading XDP. If these devices enable the
> > full checksum offloading, then the driver can load XDP without clearing
> > VIRTIO_NET_F_GUEST_CSUM.
> >
> > A new feature bit VIRTIO_NET_F_GUEST_FULL_CSUM is added to solve the above
> > situation, which provides the driver with configurable receive full checksum
> > offload. If the offload is enabled, then the device must supply fully
> > checksummed packets to the driver.
> >
> > Use case example:
> > If VIRTIO_NET_F_GUEST_FULL_CSUM is negotiated and receive full checksum
> > offload is enabled, after XDP processes a packet with full checksum, the
> > VIRTIO_NET_HDR_F_DATA_VALID bit is still retained, resulting in the stack
> > not needing to validate the checksum again. This is useful for guests:
> >   1. Bring the driver advantages such as cpu savings.
> >   2. For devices that do not generate partially checksummed packets themselves,
> >      XDP can be loaded in the driver without modifying the hardware behavior.
> >
> > Several solutions have been discussed in the previous proposal[1].
> > After historical discussion, we have tried the method proposed by Jason[2],
> > but some complex scenarios and challenges are difficult to deal with.
> > We now return to the method suggested in [1].
> >
> > [1] https://lists.oasis-open.org/archives/virtio-dev/202305/msg00291.html
> > [2] https://lore.kernel.org/all/20230628030506.2213-1-hengqi@linux.alibaba.com/
> >
> > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > ---
> > v1->v2:
> >     1. Modify full checksum functionality as a configurable offload
> >        that is initially turned off. @Jason
> >
> >  device-types/net/description.tex | 54 ++++++++++++++++++++++++++++----
> >  1 file changed, 48 insertions(+), 6 deletions(-)
> >
> > diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> > index 76585b0..3c34f27 100644
> > --- a/device-types/net/description.tex
> > +++ b/device-types/net/description.tex
> > @@ -88,6 +88,8 @@ \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_FULL_CSUM (50)] Driver handles packets with full checksum.
> > +
> >  \item[VIRTIO_NET_F_HASH_TUNNEL(51)] Device supports inner header hash for encapsulated packets.
> >
> >  \item[VIRTIO_NET_F_VQ_NOTF_COAL(52)] Device supports virtqueue notification coalescing.
> > @@ -133,6 +135,7 @@ \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device
> >  \item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM.
> >  \item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM.
> >  \item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM.
> > +\item[VIRTIO_NET_F_GUEST_FULL_CSUM] Requires VIRTIO_NET_F_GUEST_CSUM and VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
> >
> >  \item[VIRTIO_NET_F_HOST_TSO4] Requires VIRTIO_NET_F_CSUM.
> >  \item[VIRTIO_NET_F_HOST_TSO6] Requires VIRTIO_NET_F_CSUM.
>
> What about all of these:
>
> device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_TSO4] Requires VIRTIO_NET_F_GUEST_CSUM.
> device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_TSO6] Requires VIRTIO_NET_F_GUEST_CSUM.
> device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM.
> device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM.
> device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM.
>
>
>
> can TSO/UFO/USO work with VIRTIO_NET_F_GUEST_FULL_CSUM as opposed to VIRTIO_NET_F_GUEST_CSUM?

Why is it opposite, I understand that full csum is a subset of it.

>
>
> > @@ -390,6 +393,13 @@ \subsection{Device Initialization}\label{sec:Device Types / Network Device / Dev
> >  \ref{sec:Device Types / Network Device / Device Operation /
> >  Processing of Incoming Packets}~\nameref{sec:Device Types /
> >  Network Device / Device Operation / Processing of Incoming Packets} below.
> > +
> > +\item The VIRTIO_NET_F_GUEST_FULL_CSUM feature indicates that the driver handles
> > +  packets with full checksum and does not handle packets with partial checksum,
>
>
> So we need to change definition of VIRTIO_NET_F_GUEST_CSUM then.
>
>
> Also this is not exactly right. As defined driver must be able to handle
> partial checksum too.
>
>
> How about this:
>
> - change definition above to just "Driver handles packets with full checksum."
>
> - if VIRTIO_NET_F_GUEST_FULL_CSUM is set but VIRTIO_NET_F_GUEST_CSUM is
>   clear driver requires full checksum

It is ok. But as you said, we must change many places that depend on
VIRTIO_NET_F_GUEST_CSUM.

So I more like think VIRTIO_NET_F_GUEST_FULL_CSUM as the subset of VIRTIO_NET_F_GUEST_CSUM.
Then all the cases depended on the VIRTIO_NET_F_GUEST_CSUM can
still work with VIRTIO_NET_F_GUEST_FULL_CSUM & VIRTIO_NET_F_GUEST_CSUM.

So:
 - if VIRTIO_NET_F_GUEST_CSUM is not set,  VIRTIO_NET_F_GUEST_FULL_CSUM MUST be clear.


>
> - if VIRTIO_NET_F_GUEST_FULL_CSUM is clear but VIRTIO_NET_F_GUEST_CSUM is
>   set driver supports partial checksum
>
> - if VIRTIO_NET_F_GUEST_FULL_CSUM and VIRTIO_NET_F_GUEST_CSUM are
>   set then the behavior is as you describe: VIRTIO_NET_F_GUEST_CSUM
>   takes preference, but you can disable it with VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
>   if that is supported.
>
>
> > +  which means that partially checksummed packets can not be received by the driver
> > +  and the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in \field{flags} can not be set by the device.
> > +  The driver can send the VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET command to control
> > +  the receive full checksum offload state.
>
>
> I don't like it that we are coming with "offload state" here that isn't
> explained anywhere. Let's try to make this offload more like
> other existing offloads.
>
> I find it is also confusing to say "receive full checksum offload has
> been enabled". It is more that driver handles packets with full checksum.
>
> >  \end{enumerate}
> >
> >  A truly minimal driver would only accept VIRTIO_NET_F_MAC and ignore
> > @@ -720,7 +730,8 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
> >    \field{num_buffers} is one, then the entire packet will be
> >    contained within this buffer, immediately following the struct
> >    virtio_net_hdr.
> > -\item If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the
> > +\item If the VIRTIO_NET_F_GUEST_CSUM feature (regardless of whether
> > +  VIRTIO_NET_F_GUEST_FULL_CSUM was negotiated) was negotiated, the
> >    VIRTIO_NET_HDR_F_DATA_VALID bit in \field{flags} can be
> >    set: if so, device has validated the packet checksum.
> >    In case of multiple encapsulated protocols, one level of checksums
> > @@ -744,7 +755,8 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
> >    number of coalesced TCP segments in \field{csum_start} field and
> >    number of duplicated ACK segments in \field{csum_offset} field
> >    and sets bit VIRTIO_NET_HDR_F_RSC_INFO in \field{flags}.
> > -\item If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the
> > +\item If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated but the
> > +  VIRTIO_NET_F_GUEST_FULL_CSUM feature was not negotiated, the
> >    VIRTIO_NET_HDR_F_NEEDS_CSUM bit in \field{flags} can be
> >    set: if so, the packet checksum at offset \field{csum_offset}
> >    from \field{csum_start} and any preceding checksums
> > @@ -802,8 +814,9 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
> >  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
> > +If the VIRTIO_NET_F_GUEST_CSUM feature has been negotiated but
> > +the VIRTIO_NET_F_GUEST_FULL_CSUM feature has not been negotiated,
> > +the device MAY set the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in
> >  \field{flags}, if so:
> >  \begin{enumerate}
> >  \item the device MUST validate the packet checksum at
> > @@ -819,11 +832,38 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
> >  	fully checksummed packet;
> >  \end{enumerate}
> >
> > +The driver MUST NOT enable receive full checksum offload for which
> > +VIRTIO_NET_F_GUEST_FULL_CSUM has not been negotiated.
> > +
> > +Initially (before the device receives any VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET
> > +command carrying the VIRTIO_NET_F_GUEST_FULL_CSUM feature parameter) receive
> > +full checksum offload MUST be disabled.
> > +
> > +Upon the device reset, the device MUST disable receive full checksum offload.
> > +
> > +If VIRTIO_NET_F_GUEST_FULL_CSUM has been negotiated and receive full checksum
> > +offload has not been enabled, the device MUST NOT perform any of the
> > +functionality provided by VIRTIO_NET_F_GUEST_FULL_CSUM.
> > +
> > +If receive full checksum offload has been enabled, the device MUST NOT set
> > +the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in \field{flags} and MUST supply a
> > +fully checksummed packet to the driver.
> > +
> > +If a partially checksummed packet is received by the device, the device MUST
> > +calculate full checksum for the packet and then supply it to the driver
> > +\ref{sec:Device Types / Network Device / Device Operation / Packet Transmission}.
> > +
> >  If none of the VIRTIO_NET_F_GUEST_TSO4, TSO6, UFO, USO4 or USO6 options have
> >  been negotiated, the device MUST set \field{gso_type} to
> >  VIRTIO_NET_HDR_GSO_NONE.
> >
> > -If \field{gso_type} differs from VIRTIO_NET_HDR_GSO_NONE, then
> > +If receive full checksum offload has been enabled and \field{gso_type}
> > +differs from VIRTIO_NET_HDR_GSO_NONE, then the device MUST NOT set
> > +the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in \field{flags} and MUST calculate
> > +full checksum for the packet and then supply it to the driver.
> > +
> > +If the VIRTIO_NET_F_GUEST_FULL_CSUM feature has not been negotiated and
> > +\field{gso_type} differs from VIRTIO_NET_HDR_GSO_NONE, then
> >  the device MUST also set the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in
> >  \field{flags} MUST set \field{gso_size} to indicate the desired MSS.
> >  If VIRTIO_NET_F_RSC_EXT was negotiated, the device MUST also
> > @@ -839,7 +879,8 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
> >  not less than the length of the headers, including the transport
> >  header.
> >
> > -If the VIRTIO_NET_F_GUEST_CSUM feature has been negotiated, the
> > +If the VIRTIO_NET_F_GUEST_CSUM feature (regardless of whether
> > +VIRTIO_NET_F_GUEST_FULL_CSUM has been negotiated) has been negotiated, the
> >  device MAY set the VIRTIO_NET_HDR_F_DATA_VALID bit in
> >  \field{flags}, if so, the device MUST validate the packet
> >  checksum (in case of multiple encapsulated protocols, one level
> > @@ -1624,6 +1665,7 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> >  #define VIRTIO_NET_F_GUEST_TSO6       8
> >  #define VIRTIO_NET_F_GUEST_ECN        9
> >  #define VIRTIO_NET_F_GUEST_UFO        10
> > +#define VIRTIO_NET_F_GUEST_FULL_CSUM  50
> >  #define VIRTIO_NET_F_GUEST_USO4       54
> >  #define VIRTIO_NET_F_GUEST_USO6       55
> >
> > --
> > 2.19.1.6.gb485710b
> >
> >
> > This publicly archived list offers a means to provide input to the
> > OASIS Virtual I/O Device (VIRTIO) TC.
> >
> > In order to verify user consent to the Feedback License terms and
> > to minimize spam in the list archive, subscription is required
> > before posting.
> >
> > Subscribe: virtio-comment-subscribe@lists.oasis-open.org
> > Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
> > List help: virtio-comment-help@lists.oasis-open.org
> > List archive: https://lists.oasis-open.org/archives/virtio-comment/
> > Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
> > List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
> > Committee: https://www.oasis-open.org/committees/virtio/
> > Join OASIS: https://www.oasis-open.org/join/
> >
>

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v2] virtio-net: support distinguishing between partial and full checksum

[Heng Qi]

在 2023/10/27 下午3:39, Michael S. Tsirkin 写道:
> On Thu, Oct 19, 2023 at 02:17:20PM +0800, Heng Qi wrote:
>> virtio-net works in a virtualized system and is somewhat different from
>> physical nics. One of the differences is that to save virtio device
>> resources, rx may receive packets with partial checksum. However, XDP may
>> cause partially checksummed packets to be dropped. So XDP loading conflicts
>> with the feature VIRTIO_NET_F_GUEST_CSUM.
>>
>> This patch lets the device to supply fully checksummed packets to the driver.
>> Then XDP can coexist with VIRTIO_NET_F_GUEST_CSUM to enjoy the benefits of
>> device verification checksum.
>>
>> In addition, implementation of some performant devices do not generate
>> partially checksummed packets, but the standard driver still need to clear
>> VIRTIO_NET_F_GUEST_CSUM when loading XDP. If these devices enable the
>> full checksum offloading, then the driver can load XDP without clearing
>> VIRTIO_NET_F_GUEST_CSUM.
>>
>> A new feature bit VIRTIO_NET_F_GUEST_FULL_CSUM is added to solve the above
>> situation, which provides the driver with configurable receive full checksum
>> offload. If the offload is enabled, then the device must supply fully
>> checksummed packets to the driver.
>>
>> Use case example:
>> If VIRTIO_NET_F_GUEST_FULL_CSUM is negotiated and receive full checksum
>> offload is enabled, after XDP processes a packet with full checksum, the
>> VIRTIO_NET_HDR_F_DATA_VALID bit is still retained, resulting in the stack
>> not needing to validate the checksum again. This is useful for guests:
>>    1. Bring the driver advantages such as cpu savings.
>>    2. For devices that do not generate partially checksummed packets themselves,
>>       XDP can be loaded in the driver without modifying the hardware behavior.
>>
>> Several solutions have been discussed in the previous proposal[1].
>> After historical discussion, we have tried the method proposed by Jason[2],
>> but some complex scenarios and challenges are difficult to deal with.
>> We now return to the method suggested in [1].
>>
>> [1] https://lists.oasis-open.org/archives/virtio-dev/202305/msg00291.html
>> [2] https://lore.kernel.org/all/20230628030506.2213-1-hengqi@linux.alibaba.com/
>>
>> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
>> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>> ---
>> v1->v2:
>>      1. Modify full checksum functionality as a configurable offload
>>         that is initially turned off. @Jason
>>
>>   device-types/net/description.tex | 54 ++++++++++++++++++++++++++++----
>>   1 file changed, 48 insertions(+), 6 deletions(-)
>>
>> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
>> index 76585b0..3c34f27 100644
>> --- a/device-types/net/description.tex
>> +++ b/device-types/net/description.tex
>> @@ -88,6 +88,8 @@ \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_FULL_CSUM (50)] Driver handles packets with full checksum.
>> +
>>   \item[VIRTIO_NET_F_HASH_TUNNEL(51)] Device supports inner header hash for encapsulated packets.
>>   
>>   \item[VIRTIO_NET_F_VQ_NOTF_COAL(52)] Device supports virtqueue notification coalescing.
>> @@ -133,6 +135,7 @@ \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device
>>   \item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM.
>>   \item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM.
>>   \item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM.
>> +\item[VIRTIO_NET_F_GUEST_FULL_CSUM] Requires VIRTIO_NET_F_GUEST_CSUM and VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
>>   
>>   \item[VIRTIO_NET_F_HOST_TSO4] Requires VIRTIO_NET_F_CSUM.
>>   \item[VIRTIO_NET_F_HOST_TSO6] Requires VIRTIO_NET_F_CSUM.
> What about all of these:
>
> device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_TSO4] Requires VIRTIO_NET_F_GUEST_CSUM.
> device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_TSO6] Requires VIRTIO_NET_F_GUEST_CSUM.
> device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM.
> device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM.
> device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM.
>
>
>
> can TSO/UFO/USO work with VIRTIO_NET_F_GUEST_FULL_CSUM as opposed to VIRTIO_NET_F_GUEST_CSUM?

Both GUEST_FULL_CSUM and GUEST_CSUM can work with GUEST_TSO/USO/UFO.
Their important difference is that if GUEST_CSUM is negotiated, the 
driver can handle partial checksum.

>
>
>> @@ -390,6 +393,13 @@ \subsection{Device Initialization}\label{sec:Device Types / Network Device / Dev
>>   \ref{sec:Device Types / Network Device / Device Operation /
>>   Processing of Incoming Packets}~\nameref{sec:Device Types /
>>   Network Device / Device Operation / Processing of Incoming Packets} below.
>> +
>> +\item The VIRTIO_NET_F_GUEST_FULL_CSUM feature indicates that the driver handles
>> +  packets with full checksum and does not handle packets with partial checksum,
>
> So we need to change definition of VIRTIO_NET_F_GUEST_CSUM then.
>
>
> Also this is not exactly right. As defined driver must be able to handle
> partial checksum too.
>
>
> How about this:
>
> - change definition above to just "Driver handles packets with full checksum."
>
> - if VIRTIO_NET_F_GUEST_FULL_CSUM is set but VIRTIO_NET_F_GUEST_CSUM is
>    clear driver requires full checksum
>
> - if VIRTIO_NET_F_GUEST_FULL_CSUM is clear but VIRTIO_NET_F_GUEST_CSUM is
>    set driver supports partial checksum
>
> - if VIRTIO_NET_F_GUEST_FULL_CSUM and VIRTIO_NET_F_GUEST_CSUM are
>    set then the behavior is as you describe: VIRTIO_NET_F_GUEST_CSUM
>    takes preference, but you can disable it with VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
>    if that is supported.

Jason wanted this feature to be enabled only when XDP is loading,
and this is the context in which this patch was proposed.

How do you pay attention to this?

>
>
>> +  which means that partially checksummed packets can not be received by the driver
>> +  and the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in \field{flags} can not be set by the device.
>> +  The driver can send the VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET command to control
>> +  the receive full checksum offload state.
>
> I don't like it that we are coming with "offload state" here that isn't
> explained anywhere. Let's try to make this offload more like

In "Offloads State Configuration" paragraph, "offload state" has already 
been mentioned and used.

I think the discussion of this patch needs to focus on:
If this feature is negotiated, the full csum offload is enabled when 
driver probing phase?  Or:
If this feature is negotiated, the full csum offload is disabled when 
driver probing phase, waiting to be dynamically turned on?

> other existing offloads.
>
> I find it is also confusing to say "receive full checksum offload has
> been enabled". It is more that driver handles packets with full checksum.

OK, I will describe it this way in the next version.

Thanks!

>
>>   \end{enumerate}
>>   
>>   A truly minimal driver would only accept VIRTIO_NET_F_MAC and ignore
>> @@ -720,7 +730,8 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>>     \field{num_buffers} is one, then the entire packet will be
>>     contained within this buffer, immediately following the struct
>>     virtio_net_hdr.
>> -\item If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the
>> +\item If the VIRTIO_NET_F_GUEST_CSUM feature (regardless of whether
>> +  VIRTIO_NET_F_GUEST_FULL_CSUM was negotiated) was negotiated, the
>>     VIRTIO_NET_HDR_F_DATA_VALID bit in \field{flags} can be
>>     set: if so, device has validated the packet checksum.
>>     In case of multiple encapsulated protocols, one level of checksums
>> @@ -744,7 +755,8 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>>     number of coalesced TCP segments in \field{csum_start} field and
>>     number of duplicated ACK segments in \field{csum_offset} field
>>     and sets bit VIRTIO_NET_HDR_F_RSC_INFO in \field{flags}.
>> -\item If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the
>> +\item If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated but the
>> +  VIRTIO_NET_F_GUEST_FULL_CSUM feature was not negotiated, the
>>     VIRTIO_NET_HDR_F_NEEDS_CSUM bit in \field{flags} can be
>>     set: if so, the packet checksum at offset \field{csum_offset}
>>     from \field{csum_start} and any preceding checksums
>> @@ -802,8 +814,9 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>>   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
>> +If the VIRTIO_NET_F_GUEST_CSUM feature has been negotiated but
>> +the VIRTIO_NET_F_GUEST_FULL_CSUM feature has not been negotiated,
>> +the device MAY set the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in
>>   \field{flags}, if so:
>>   \begin{enumerate}
>>   \item the device MUST validate the packet checksum at
>> @@ -819,11 +832,38 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>>   	fully checksummed packet;
>>   \end{enumerate}
>>   
>> +The driver MUST NOT enable receive full checksum offload for which
>> +VIRTIO_NET_F_GUEST_FULL_CSUM has not been negotiated.
>> +
>> +Initially (before the device receives any VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET
>> +command carrying the VIRTIO_NET_F_GUEST_FULL_CSUM feature parameter) receive
>> +full checksum offload MUST be disabled.
>> +
>> +Upon the device reset, the device MUST disable receive full checksum offload.
>> +
>> +If VIRTIO_NET_F_GUEST_FULL_CSUM has been negotiated and receive full checksum
>> +offload has not been enabled, the device MUST NOT perform any of the
>> +functionality provided by VIRTIO_NET_F_GUEST_FULL_CSUM.
>> +
>> +If receive full checksum offload has been enabled, the device MUST NOT set
>> +the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in \field{flags} and MUST supply a
>> +fully checksummed packet to the driver.
>> +
>> +If a partially checksummed packet is received by the device, the device MUST
>> +calculate full checksum for the packet and then supply it to the driver
>> +\ref{sec:Device Types / Network Device / Device Operation / Packet Transmission}.
>> +
>>   If none of the VIRTIO_NET_F_GUEST_TSO4, TSO6, UFO, USO4 or USO6 options have
>>   been negotiated, the device MUST set \field{gso_type} to
>>   VIRTIO_NET_HDR_GSO_NONE.
>>   
>> -If \field{gso_type} differs from VIRTIO_NET_HDR_GSO_NONE, then
>> +If receive full checksum offload has been enabled and \field{gso_type}
>> +differs from VIRTIO_NET_HDR_GSO_NONE, then the device MUST NOT set
>> +the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in \field{flags} and MUST calculate
>> +full checksum for the packet and then supply it to the driver.
>> +
>> +If the VIRTIO_NET_F_GUEST_FULL_CSUM feature has not been negotiated and
>> +\field{gso_type} differs from VIRTIO_NET_HDR_GSO_NONE, then
>>   the device MUST also set the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in
>>   \field{flags} MUST set \field{gso_size} to indicate the desired MSS.
>>   If VIRTIO_NET_F_RSC_EXT was negotiated, the device MUST also
>> @@ -839,7 +879,8 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>>   not less than the length of the headers, including the transport
>>   header.
>>   
>> -If the VIRTIO_NET_F_GUEST_CSUM feature has been negotiated, the
>> +If the VIRTIO_NET_F_GUEST_CSUM feature (regardless of whether
>> +VIRTIO_NET_F_GUEST_FULL_CSUM has been negotiated) has been negotiated, the
>>   device MAY set the VIRTIO_NET_HDR_F_DATA_VALID bit in
>>   \field{flags}, if so, the device MUST validate the packet
>>   checksum (in case of multiple encapsulated protocols, one level
>> @@ -1624,6 +1665,7 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>>   #define VIRTIO_NET_F_GUEST_TSO6       8
>>   #define VIRTIO_NET_F_GUEST_ECN        9
>>   #define VIRTIO_NET_F_GUEST_UFO        10
>> +#define VIRTIO_NET_F_GUEST_FULL_CSUM  50
>>   #define VIRTIO_NET_F_GUEST_USO4       54
>>   #define VIRTIO_NET_F_GUEST_USO6       55
>>   
>> -- 
>> 2.19.1.6.gb485710b
>>
>>
>> This publicly archived list offers a means to provide input to the
>> OASIS Virtual I/O Device (VIRTIO) TC.
>>
>> In order to verify user consent to the Feedback License terms and
>> to minimize spam in the list archive, subscription is required
>> before posting.
>>
>> Subscribe: virtio-comment-subscribe@lists.oasis-open.org
>> Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
>> List help: virtio-comment-help@lists.oasis-open.org
>> List archive: https://lists.oasis-open.org/archives/virtio-comment/
>> Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
>> List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
>> Committee: https://www.oasis-open.org/committees/virtio/
>> Join OASIS: https://www.oasis-open.org/join/
>>


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v2] virtio-net: support distinguishing between partial and full checksum

[Jason Wang]

On Sat, Oct 28, 2023 at 10:36 AM Heng Qi <hengqi@linux.alibaba.com> wrote:
>
>
>
> 在 2023/10/27 下午3:39, Michael S. Tsirkin 写道:
> > On Thu, Oct 19, 2023 at 02:17:20PM +0800, Heng Qi wrote:
> >> virtio-net works in a virtualized system and is somewhat different from
> >> physical nics. One of the differences is that to save virtio device
> >> resources, rx may receive packets with partial checksum. However, XDP may
> >> cause partially checksummed packets to be dropped. So XDP loading conflicts
> >> with the feature VIRTIO_NET_F_GUEST_CSUM.
> >>
> >> This patch lets the device to supply fully checksummed packets to the driver.
> >> Then XDP can coexist with VIRTIO_NET_F_GUEST_CSUM to enjoy the benefits of
> >> device verification checksum.
> >>
> >> In addition, implementation of some performant devices do not generate
> >> partially checksummed packets, but the standard driver still need to clear
> >> VIRTIO_NET_F_GUEST_CSUM when loading XDP. If these devices enable the
> >> full checksum offloading, then the driver can load XDP without clearing
> >> VIRTIO_NET_F_GUEST_CSUM.
> >>
> >> A new feature bit VIRTIO_NET_F_GUEST_FULL_CSUM is added to solve the above
> >> situation, which provides the driver with configurable receive full checksum
> >> offload. If the offload is enabled, then the device must supply fully
> >> checksummed packets to the driver.
> >>
> >> Use case example:
> >> If VIRTIO_NET_F_GUEST_FULL_CSUM is negotiated and receive full checksum
> >> offload is enabled, after XDP processes a packet with full checksum, the
> >> VIRTIO_NET_HDR_F_DATA_VALID bit is still retained, resulting in the stack
> >> not needing to validate the checksum again. This is useful for guests:
> >>    1. Bring the driver advantages such as cpu savings.
> >>    2. For devices that do not generate partially checksummed packets themselves,
> >>       XDP can be loaded in the driver without modifying the hardware behavior.
> >>
> >> Several solutions have been discussed in the previous proposal[1].
> >> After historical discussion, we have tried the method proposed by Jason[2],
> >> but some complex scenarios and challenges are difficult to deal with.
> >> We now return to the method suggested in [1].
> >>
> >> [1] https://lists.oasis-open.org/archives/virtio-dev/202305/msg00291.html
> >> [2] https://lore.kernel.org/all/20230628030506.2213-1-hengqi@linux.alibaba.com/
> >>
> >> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> >> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> >> ---
> >> v1->v2:
> >>      1. Modify full checksum functionality as a configurable offload
> >>         that is initially turned off. @Jason
> >>
> >>   device-types/net/description.tex | 54 ++++++++++++++++++++++++++++----
> >>   1 file changed, 48 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> >> index 76585b0..3c34f27 100644
> >> --- a/device-types/net/description.tex
> >> +++ b/device-types/net/description.tex
> >> @@ -88,6 +88,8 @@ \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_FULL_CSUM (50)] Driver handles packets with full checksum.
> >> +
> >>   \item[VIRTIO_NET_F_HASH_TUNNEL(51)] Device supports inner header hash for encapsulated packets.
> >>
> >>   \item[VIRTIO_NET_F_VQ_NOTF_COAL(52)] Device supports virtqueue notification coalescing.
> >> @@ -133,6 +135,7 @@ \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device
> >>   \item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM.
> >>   \item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM.
> >>   \item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM.
> >> +\item[VIRTIO_NET_F_GUEST_FULL_CSUM] Requires VIRTIO_NET_F_GUEST_CSUM and VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
> >>
> >>   \item[VIRTIO_NET_F_HOST_TSO4] Requires VIRTIO_NET_F_CSUM.
> >>   \item[VIRTIO_NET_F_HOST_TSO6] Requires VIRTIO_NET_F_CSUM.
> > What about all of these:
> >
> > device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_TSO4] Requires VIRTIO_NET_F_GUEST_CSUM.
> > device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_TSO6] Requires VIRTIO_NET_F_GUEST_CSUM.
> > device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM.
> > device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM.
> > device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM.
> >
> >
> >
> > can TSO/UFO/USO work with VIRTIO_NET_F_GUEST_FULL_CSUM as opposed to VIRTIO_NET_F_GUEST_CSUM?
>
> Both GUEST_FULL_CSUM and GUEST_CSUM can work with GUEST_TSO/USO/UFO.

Yes. For software devices I guess it will have a lot of performance
penalty. So it should be disabled by default anyhow. The idea is to
delay the csum as late as possible.

> Their important difference is that if GUEST_CSUM is negotiated, the
> driver can handle partial checksum.
>
> >
> >
> >> @@ -390,6 +393,13 @@ \subsection{Device Initialization}\label{sec:Device Types / Network Device / Dev
> >>   \ref{sec:Device Types / Network Device / Device Operation /
> >>   Processing of Incoming Packets}~\nameref{sec:Device Types /
> >>   Network Device / Device Operation / Processing of Incoming Packets} below.
> >> +
> >> +\item The VIRTIO_NET_F_GUEST_FULL_CSUM feature indicates that the driver handles
> >> +  packets with full checksum and does not handle packets with partial checksum,
> >
> > So we need to change definition of VIRTIO_NET_F_GUEST_CSUM then.
> >
> >
> > Also this is not exactly right. As defined driver must be able to handle
> > partial checksum too.
> >
> >
> > How about this:
> >
> > - change definition above to just "Driver handles packets with full checksum."
> >
> > - if VIRTIO_NET_F_GUEST_FULL_CSUM is set but VIRTIO_NET_F_GUEST_CSUM is
> >    clear driver requires full checksum
> >
> > - if VIRTIO_NET_F_GUEST_FULL_CSUM is clear but VIRTIO_NET_F_GUEST_CSUM is
> >    set driver supports partial checksum
> >
> > - if VIRTIO_NET_F_GUEST_FULL_CSUM and VIRTIO_NET_F_GUEST_CSUM are
> >    set then the behavior is as you describe: VIRTIO_NET_F_GUEST_CSUM
> >    takes preference, but you can disable it with VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
> >    if that is supported.
>
> Jason wanted this feature to be enabled only when XDP is loading,
> and this is the context in which this patch was proposed.
>
> How do you pay attention to this?

I don't see any conflict, or anything I miss?

Thanks


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v2] virtio-net: support distinguishing between partial and full checksum

[Heng Qi]

在 2023/11/1 下午12:16, Jason Wang 写道:
> On Sat, Oct 28, 2023 at 10:36 AM Heng Qi <hengqi@linux.alibaba.com> wrote:
>>
>>
>> 在 2023/10/27 下午3:39, Michael S. Tsirkin 写道:
>>> On Thu, Oct 19, 2023 at 02:17:20PM +0800, Heng Qi wrote:
>>>> virtio-net works in a virtualized system and is somewhat different from
>>>> physical nics. One of the differences is that to save virtio device
>>>> resources, rx may receive packets with partial checksum. However, XDP may
>>>> cause partially checksummed packets to be dropped. So XDP loading conflicts
>>>> with the feature VIRTIO_NET_F_GUEST_CSUM.
>>>>
>>>> This patch lets the device to supply fully checksummed packets to the driver.
>>>> Then XDP can coexist with VIRTIO_NET_F_GUEST_CSUM to enjoy the benefits of
>>>> device verification checksum.
>>>>
>>>> In addition, implementation of some performant devices do not generate
>>>> partially checksummed packets, but the standard driver still need to clear
>>>> VIRTIO_NET_F_GUEST_CSUM when loading XDP. If these devices enable the
>>>> full checksum offloading, then the driver can load XDP without clearing
>>>> VIRTIO_NET_F_GUEST_CSUM.
>>>>
>>>> A new feature bit VIRTIO_NET_F_GUEST_FULL_CSUM is added to solve the above
>>>> situation, which provides the driver with configurable receive full checksum
>>>> offload. If the offload is enabled, then the device must supply fully
>>>> checksummed packets to the driver.
>>>>
>>>> Use case example:
>>>> If VIRTIO_NET_F_GUEST_FULL_CSUM is negotiated and receive full checksum
>>>> offload is enabled, after XDP processes a packet with full checksum, the
>>>> VIRTIO_NET_HDR_F_DATA_VALID bit is still retained, resulting in the stack
>>>> not needing to validate the checksum again. This is useful for guests:
>>>>     1. Bring the driver advantages such as cpu savings.
>>>>     2. For devices that do not generate partially checksummed packets themselves,
>>>>        XDP can be loaded in the driver without modifying the hardware behavior.
>>>>
>>>> Several solutions have been discussed in the previous proposal[1].
>>>> After historical discussion, we have tried the method proposed by Jason[2],
>>>> but some complex scenarios and challenges are difficult to deal with.
>>>> We now return to the method suggested in [1].
>>>>
>>>> [1] https://lists.oasis-open.org/archives/virtio-dev/202305/msg00291.html
>>>> [2] https://lore.kernel.org/all/20230628030506.2213-1-hengqi@linux.alibaba.com/
>>>>
>>>> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
>>>> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>>> ---
>>>> v1->v2:
>>>>       1. Modify full checksum functionality as a configurable offload
>>>>          that is initially turned off. @Jason
>>>>
>>>>    device-types/net/description.tex | 54 ++++++++++++++++++++++++++++----
>>>>    1 file changed, 48 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
>>>> index 76585b0..3c34f27 100644
>>>> --- a/device-types/net/description.tex
>>>> +++ b/device-types/net/description.tex
>>>> @@ -88,6 +88,8 @@ \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_FULL_CSUM (50)] Driver handles packets with full checksum.
>>>> +
>>>>    \item[VIRTIO_NET_F_HASH_TUNNEL(51)] Device supports inner header hash for encapsulated packets.
>>>>
>>>>    \item[VIRTIO_NET_F_VQ_NOTF_COAL(52)] Device supports virtqueue notification coalescing.
>>>> @@ -133,6 +135,7 @@ \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device
>>>>    \item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM.
>>>>    \item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM.
>>>>    \item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM.
>>>> +\item[VIRTIO_NET_F_GUEST_FULL_CSUM] Requires VIRTIO_NET_F_GUEST_CSUM and VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
>>>>
>>>>    \item[VIRTIO_NET_F_HOST_TSO4] Requires VIRTIO_NET_F_CSUM.
>>>>    \item[VIRTIO_NET_F_HOST_TSO6] Requires VIRTIO_NET_F_CSUM.
>>> What about all of these:
>>>
>>> device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_TSO4] Requires VIRTIO_NET_F_GUEST_CSUM.
>>> device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_TSO6] Requires VIRTIO_NET_F_GUEST_CSUM.
>>> device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM.
>>> device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM.
>>> device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM.
>>>
>>>
>>>
>>> can TSO/UFO/USO work with VIRTIO_NET_F_GUEST_FULL_CSUM as opposed to VIRTIO_NET_F_GUEST_CSUM?
>> Both GUEST_FULL_CSUM and GUEST_CSUM can work with GUEST_TSO/USO/UFO.
> Yes. For software devices I guess it will have a lot of performance
> penalty. So it should be disabled by default anyhow. The idea is to
> delay the csum as late as possible.

Yes. I totally agree.

>
>> Their important difference is that if GUEST_CSUM is negotiated, the
>> driver can handle partial checksum.
>>
>>>
>>>> @@ -390,6 +393,13 @@ \subsection{Device Initialization}\label{sec:Device Types / Network Device / Dev
>>>>    \ref{sec:Device Types / Network Device / Device Operation /
>>>>    Processing of Incoming Packets}~\nameref{sec:Device Types /
>>>>    Network Device / Device Operation / Processing of Incoming Packets} below.
>>>> +
>>>> +\item The VIRTIO_NET_F_GUEST_FULL_CSUM feature indicates that the driver handles
>>>> +  packets with full checksum and does not handle packets with partial checksum,
>>> So we need to change definition of VIRTIO_NET_F_GUEST_CSUM then.
>>>
>>>
>>> Also this is not exactly right. As defined driver must be able to handle
>>> partial checksum too.
>>>
>>>
>>> How about this:
>>>
>>> - change definition above to just "Driver handles packets with full checksum."
>>>
>>> - if VIRTIO_NET_F_GUEST_FULL_CSUM is set but VIRTIO_NET_F_GUEST_CSUM is
>>>     clear driver requires full checksum
>>>
>>> - if VIRTIO_NET_F_GUEST_FULL_CSUM is clear but VIRTIO_NET_F_GUEST_CSUM is
>>>     set driver supports partial checksum
>>>
>>> - if VIRTIO_NET_F_GUEST_FULL_CSUM and VIRTIO_NET_F_GUEST_CSUM are
>>>     set then the behavior is as you describe: VIRTIO_NET_F_GUEST_CSUM
>>>     takes preference, but you can disable it with VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
>>>     if that is supported.
>> Jason wanted this feature to be enabled only when XDP is loading,
>> and this is the context in which this patch was proposed.
>>
>> How do you pay attention to this?
> I don't see any conflict, or anything I miss?

Yes, our request was met.

If GUEST_FULL_CSUM and GUEST_CSUM are independent,
that is, GUEST_FULL_CSUM can be successfully validated without GUEST_CSUM.
Then we need to re-describe most of the existing behavior of GUEST_CSUM 
for FULL_CSUM
in the spec, this part is overlapping. Moreover, the relationship 
between FULL_CSUM
and GUEST_CSUM also needs to be processed in the full text.

So I think it seems clearer to constrain the behavior of GUEST_CSUM by
treating FULL_CSUM as a subset of GUEST_CSUM.

For example we don't need to make the following changes:
\item[VIRTIO_NET_F_GUEST_TSO4] Requires VIRTIO_NET_F_GUEST_CSUM or 
VIRTIO_NET_F_GUEST_FULL_CSUM.
\item[VIRTIO_NET_F_GUEST_TSO6] Requires VIRTIO_NET_F_GUEST_CSUM or 
VIRTIO_NET_F_GUEST_FULL_CSUM.
\item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM or 
VIRTIO_NET_F_GUEST_FULL_CSUM.
\item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM or 
VIRTIO_NET_F_GUEST_FULL_CSUM.
\item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM or 
VIRTIO_NET_F_GUEST_FULL_CSUM.

Thanks!

>
> Thanks


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v2] virtio-net: support distinguishing between partial and full checksum

[Michael S. Tsirkin]

On Wed, Nov 01, 2023 at 12:16:23PM +0800, Jason Wang wrote:
> On Sat, Oct 28, 2023 at 10:36 AM Heng Qi <hengqi@linux.alibaba.com> wrote:
> >
> >
> >
> > 在 2023/10/27 下午3:39, Michael S. Tsirkin 写道:
> > > On Thu, Oct 19, 2023 at 02:17:20PM +0800, Heng Qi wrote:
> > >> virtio-net works in a virtualized system and is somewhat different from
> > >> physical nics. One of the differences is that to save virtio device
> > >> resources, rx may receive packets with partial checksum. However, XDP may
> > >> cause partially checksummed packets to be dropped. So XDP loading conflicts
> > >> with the feature VIRTIO_NET_F_GUEST_CSUM.
> > >>
> > >> This patch lets the device to supply fully checksummed packets to the driver.
> > >> Then XDP can coexist with VIRTIO_NET_F_GUEST_CSUM to enjoy the benefits of
> > >> device verification checksum.
> > >>
> > >> In addition, implementation of some performant devices do not generate
> > >> partially checksummed packets, but the standard driver still need to clear
> > >> VIRTIO_NET_F_GUEST_CSUM when loading XDP. If these devices enable the
> > >> full checksum offloading, then the driver can load XDP without clearing
> > >> VIRTIO_NET_F_GUEST_CSUM.
> > >>
> > >> A new feature bit VIRTIO_NET_F_GUEST_FULL_CSUM is added to solve the above
> > >> situation, which provides the driver with configurable receive full checksum
> > >> offload. If the offload is enabled, then the device must supply fully
> > >> checksummed packets to the driver.
> > >>
> > >> Use case example:
> > >> If VIRTIO_NET_F_GUEST_FULL_CSUM is negotiated and receive full checksum
> > >> offload is enabled, after XDP processes a packet with full checksum, the
> > >> VIRTIO_NET_HDR_F_DATA_VALID bit is still retained, resulting in the stack
> > >> not needing to validate the checksum again. This is useful for guests:
> > >>    1. Bring the driver advantages such as cpu savings.
> > >>    2. For devices that do not generate partially checksummed packets themselves,
> > >>       XDP can be loaded in the driver without modifying the hardware behavior.
> > >>
> > >> Several solutions have been discussed in the previous proposal[1].
> > >> After historical discussion, we have tried the method proposed by Jason[2],
> > >> but some complex scenarios and challenges are difficult to deal with.
> > >> We now return to the method suggested in [1].
> > >>
> > >> [1] https://lists.oasis-open.org/archives/virtio-dev/202305/msg00291.html
> > >> [2] https://lore.kernel.org/all/20230628030506.2213-1-hengqi@linux.alibaba.com/
> > >>
> > >> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > >> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > >> ---
> > >> v1->v2:
> > >>      1. Modify full checksum functionality as a configurable offload
> > >>         that is initially turned off. @Jason
> > >>
> > >>   device-types/net/description.tex | 54 ++++++++++++++++++++++++++++----
> > >>   1 file changed, 48 insertions(+), 6 deletions(-)
> > >>
> > >> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> > >> index 76585b0..3c34f27 100644
> > >> --- a/device-types/net/description.tex
> > >> +++ b/device-types/net/description.tex
> > >> @@ -88,6 +88,8 @@ \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_FULL_CSUM (50)] Driver handles packets with full checksum.
> > >> +
> > >>   \item[VIRTIO_NET_F_HASH_TUNNEL(51)] Device supports inner header hash for encapsulated packets.
> > >>
> > >>   \item[VIRTIO_NET_F_VQ_NOTF_COAL(52)] Device supports virtqueue notification coalescing.
> > >> @@ -133,6 +135,7 @@ \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device
> > >>   \item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM.
> > >>   \item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM.
> > >>   \item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM.
> > >> +\item[VIRTIO_NET_F_GUEST_FULL_CSUM] Requires VIRTIO_NET_F_GUEST_CSUM and VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
> > >>
> > >>   \item[VIRTIO_NET_F_HOST_TSO4] Requires VIRTIO_NET_F_CSUM.
> > >>   \item[VIRTIO_NET_F_HOST_TSO6] Requires VIRTIO_NET_F_CSUM.
> > > What about all of these:
> > >
> > > device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_TSO4] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_TSO6] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM.
> > >
> > >
> > >
> > > can TSO/UFO/USO work with VIRTIO_NET_F_GUEST_FULL_CSUM as opposed to VIRTIO_NET_F_GUEST_CSUM?
> >
> > Both GUEST_FULL_CSUM and GUEST_CSUM can work with GUEST_TSO/USO/UFO.
> 
> Yes. For software devices I guess it will have a lot of performance
> penalty. So it should be disabled by default anyhow. The idea is to
> delay the csum as late as possible.

But for hardware it's actually better. Maybe we need a flag
to say which offloads are expensive?


> > Their important difference is that if GUEST_CSUM is negotiated, the
> > driver can handle partial checksum.
> >
> > >
> > >
> > >> @@ -390,6 +393,13 @@ \subsection{Device Initialization}\label{sec:Device Types / Network Device / Dev
> > >>   \ref{sec:Device Types / Network Device / Device Operation /
> > >>   Processing of Incoming Packets}~\nameref{sec:Device Types /
> > >>   Network Device / Device Operation / Processing of Incoming Packets} below.
> > >> +
> > >> +\item The VIRTIO_NET_F_GUEST_FULL_CSUM feature indicates that the driver handles
> > >> +  packets with full checksum and does not handle packets with partial checksum,
> > >
> > > So we need to change definition of VIRTIO_NET_F_GUEST_CSUM then.
> > >
> > >
> > > Also this is not exactly right. As defined driver must be able to handle
> > > partial checksum too.
> > >
> > >
> > > How about this:
> > >
> > > - change definition above to just "Driver handles packets with full checksum."
> > >
> > > - if VIRTIO_NET_F_GUEST_FULL_CSUM is set but VIRTIO_NET_F_GUEST_CSUM is
> > >    clear driver requires full checksum
> > >
> > > - if VIRTIO_NET_F_GUEST_FULL_CSUM is clear but VIRTIO_NET_F_GUEST_CSUM is
> > >    set driver supports partial checksum
> > >
> > > - if VIRTIO_NET_F_GUEST_FULL_CSUM and VIRTIO_NET_F_GUEST_CSUM are
> > >    set then the behavior is as you describe: VIRTIO_NET_F_GUEST_CSUM
> > >    takes preference, but you can disable it with VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
> > >    if that is supported.
> >
> > Jason wanted this feature to be enabled only when XDP is loading,
> > and this is the context in which this patch was proposed.
> >
> > How do you pay attention to this?
> 
> I don't see any conflict, or anything I miss?
> 
> Thanks


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v2] virtio-net: support distinguishing between partial and full checksum

[Heng Qi]

在 2023/11/1 下午1:37, Michael S. Tsirkin 写道:
> On Wed, Nov 01, 2023 at 12:16:23PM +0800, Jason Wang wrote:
>> On Sat, Oct 28, 2023 at 10:36 AM Heng Qi <hengqi@linux.alibaba.com> wrote:
>>>
>>>
>>> 在 2023/10/27 下午3:39, Michael S. Tsirkin 写道:
>>>> On Thu, Oct 19, 2023 at 02:17:20PM +0800, Heng Qi wrote:
>>>>> virtio-net works in a virtualized system and is somewhat different from
>>>>> physical nics. One of the differences is that to save virtio device
>>>>> resources, rx may receive packets with partial checksum. However, XDP may
>>>>> cause partially checksummed packets to be dropped. So XDP loading conflicts
>>>>> with the feature VIRTIO_NET_F_GUEST_CSUM.
>>>>>
>>>>> This patch lets the device to supply fully checksummed packets to the driver.
>>>>> Then XDP can coexist with VIRTIO_NET_F_GUEST_CSUM to enjoy the benefits of
>>>>> device verification checksum.
>>>>>
>>>>> In addition, implementation of some performant devices do not generate
>>>>> partially checksummed packets, but the standard driver still need to clear
>>>>> VIRTIO_NET_F_GUEST_CSUM when loading XDP. If these devices enable the
>>>>> full checksum offloading, then the driver can load XDP without clearing
>>>>> VIRTIO_NET_F_GUEST_CSUM.
>>>>>
>>>>> A new feature bit VIRTIO_NET_F_GUEST_FULL_CSUM is added to solve the above
>>>>> situation, which provides the driver with configurable receive full checksum
>>>>> offload. If the offload is enabled, then the device must supply fully
>>>>> checksummed packets to the driver.
>>>>>
>>>>> Use case example:
>>>>> If VIRTIO_NET_F_GUEST_FULL_CSUM is negotiated and receive full checksum
>>>>> offload is enabled, after XDP processes a packet with full checksum, the
>>>>> VIRTIO_NET_HDR_F_DATA_VALID bit is still retained, resulting in the stack
>>>>> not needing to validate the checksum again. This is useful for guests:
>>>>>     1. Bring the driver advantages such as cpu savings.
>>>>>     2. For devices that do not generate partially checksummed packets themselves,
>>>>>        XDP can be loaded in the driver without modifying the hardware behavior.
>>>>>
>>>>> Several solutions have been discussed in the previous proposal[1].
>>>>> After historical discussion, we have tried the method proposed by Jason[2],
>>>>> but some complex scenarios and challenges are difficult to deal with.
>>>>> We now return to the method suggested in [1].
>>>>>
>>>>> [1] https://lists.oasis-open.org/archives/virtio-dev/202305/msg00291.html
>>>>> [2] https://lore.kernel.org/all/20230628030506.2213-1-hengqi@linux.alibaba.com/
>>>>>
>>>>> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
>>>>> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>>>> ---
>>>>> v1->v2:
>>>>>       1. Modify full checksum functionality as a configurable offload
>>>>>          that is initially turned off. @Jason
>>>>>
>>>>>    device-types/net/description.tex | 54 ++++++++++++++++++++++++++++----
>>>>>    1 file changed, 48 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
>>>>> index 76585b0..3c34f27 100644
>>>>> --- a/device-types/net/description.tex
>>>>> +++ b/device-types/net/description.tex
>>>>> @@ -88,6 +88,8 @@ \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_FULL_CSUM (50)] Driver handles packets with full checksum.
>>>>> +
>>>>>    \item[VIRTIO_NET_F_HASH_TUNNEL(51)] Device supports inner header hash for encapsulated packets.
>>>>>
>>>>>    \item[VIRTIO_NET_F_VQ_NOTF_COAL(52)] Device supports virtqueue notification coalescing.
>>>>> @@ -133,6 +135,7 @@ \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device
>>>>>    \item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM.
>>>>>    \item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM.
>>>>>    \item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM.
>>>>> +\item[VIRTIO_NET_F_GUEST_FULL_CSUM] Requires VIRTIO_NET_F_GUEST_CSUM and VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
>>>>>
>>>>>    \item[VIRTIO_NET_F_HOST_TSO4] Requires VIRTIO_NET_F_CSUM.
>>>>>    \item[VIRTIO_NET_F_HOST_TSO6] Requires VIRTIO_NET_F_CSUM.
>>>> What about all of these:
>>>>
>>>> device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_TSO4] Requires VIRTIO_NET_F_GUEST_CSUM.
>>>> device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_TSO6] Requires VIRTIO_NET_F_GUEST_CSUM.
>>>> device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM.
>>>> device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM.
>>>> device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM.
>>>>
>>>>
>>>>
>>>> can TSO/UFO/USO work with VIRTIO_NET_F_GUEST_FULL_CSUM as opposed to VIRTIO_NET_F_GUEST_CSUM?
>>> Both GUEST_FULL_CSUM and GUEST_CSUM can work with GUEST_TSO/USO/UFO.
>> Yes. For software devices I guess it will have a lot of performance
>> penalty. So it should be disabled by default anyhow. The idea is to
>> delay the csum as late as possible.
> But for hardware it's actually better. Maybe we need a flag
> to say which offloads are expensive?

FULL_CSUM requires more work on both sw and hw devices. I'm guessing you 
mean that
the full csum overhead is acceptable for a performant device.

Yes, then having the driver load xdp is a good deal.

As Jason said, now GUEST_CSUM works well and we only need FULL_CSUM when 
necessary (xdp loading).

Thanks!

>
>
>>> Their important difference is that if GUEST_CSUM is negotiated, the
>>> driver can handle partial checksum.
>>>
>>>>
>>>>> @@ -390,6 +393,13 @@ \subsection{Device Initialization}\label{sec:Device Types / Network Device / Dev
>>>>>    \ref{sec:Device Types / Network Device / Device Operation /
>>>>>    Processing of Incoming Packets}~\nameref{sec:Device Types /
>>>>>    Network Device / Device Operation / Processing of Incoming Packets} below.
>>>>> +
>>>>> +\item The VIRTIO_NET_F_GUEST_FULL_CSUM feature indicates that the driver handles
>>>>> +  packets with full checksum and does not handle packets with partial checksum,
>>>> So we need to change definition of VIRTIO_NET_F_GUEST_CSUM then.
>>>>
>>>>
>>>> Also this is not exactly right. As defined driver must be able to handle
>>>> partial checksum too.
>>>>
>>>>
>>>> How about this:
>>>>
>>>> - change definition above to just "Driver handles packets with full checksum."
>>>>
>>>> - if VIRTIO_NET_F_GUEST_FULL_CSUM is set but VIRTIO_NET_F_GUEST_CSUM is
>>>>     clear driver requires full checksum
>>>>
>>>> - if VIRTIO_NET_F_GUEST_FULL_CSUM is clear but VIRTIO_NET_F_GUEST_CSUM is
>>>>     set driver supports partial checksum
>>>>
>>>> - if VIRTIO_NET_F_GUEST_FULL_CSUM and VIRTIO_NET_F_GUEST_CSUM are
>>>>     set then the behavior is as you describe: VIRTIO_NET_F_GUEST_CSUM
>>>>     takes preference, but you can disable it with VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
>>>>     if that is supported.
>>> Jason wanted this feature to be enabled only when XDP is loading,
>>> and this is the context in which this patch was proposed.
>>>
>>> How do you pay attention to this?
>> I don't see any conflict, or anything I miss?
>>
>> Thanks


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v2] virtio-net: support distinguishing between partial and full checksum

[Jason Wang]

On Wed, Nov 1, 2023 at 1:37 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Nov 01, 2023 at 12:16:23PM +0800, Jason Wang wrote:
> > On Sat, Oct 28, 2023 at 10:36 AM Heng Qi <hengqi@linux.alibaba.com> wrote:
> > >
> > >
> > >
> > > 在 2023/10/27 下午3:39, Michael S. Tsirkin 写道:
> > > > On Thu, Oct 19, 2023 at 02:17:20PM +0800, Heng Qi wrote:
> > > >> virtio-net works in a virtualized system and is somewhat different from
> > > >> physical nics. One of the differences is that to save virtio device
> > > >> resources, rx may receive packets with partial checksum. However, XDP may
> > > >> cause partially checksummed packets to be dropped. So XDP loading conflicts
> > > >> with the feature VIRTIO_NET_F_GUEST_CSUM.
> > > >>
> > > >> This patch lets the device to supply fully checksummed packets to the driver.
> > > >> Then XDP can coexist with VIRTIO_NET_F_GUEST_CSUM to enjoy the benefits of
> > > >> device verification checksum.
> > > >>
> > > >> In addition, implementation of some performant devices do not generate
> > > >> partially checksummed packets, but the standard driver still need to clear
> > > >> VIRTIO_NET_F_GUEST_CSUM when loading XDP. If these devices enable the
> > > >> full checksum offloading, then the driver can load XDP without clearing
> > > >> VIRTIO_NET_F_GUEST_CSUM.
> > > >>
> > > >> A new feature bit VIRTIO_NET_F_GUEST_FULL_CSUM is added to solve the above
> > > >> situation, which provides the driver with configurable receive full checksum
> > > >> offload. If the offload is enabled, then the device must supply fully
> > > >> checksummed packets to the driver.
> > > >>
> > > >> Use case example:
> > > >> If VIRTIO_NET_F_GUEST_FULL_CSUM is negotiated and receive full checksum
> > > >> offload is enabled, after XDP processes a packet with full checksum, the
> > > >> VIRTIO_NET_HDR_F_DATA_VALID bit is still retained, resulting in the stack
> > > >> not needing to validate the checksum again. This is useful for guests:
> > > >>    1. Bring the driver advantages such as cpu savings.
> > > >>    2. For devices that do not generate partially checksummed packets themselves,
> > > >>       XDP can be loaded in the driver without modifying the hardware behavior.
> > > >>
> > > >> Several solutions have been discussed in the previous proposal[1].
> > > >> After historical discussion, we have tried the method proposed by Jason[2],
> > > >> but some complex scenarios and challenges are difficult to deal with.
> > > >> We now return to the method suggested in [1].
> > > >>
> > > >> [1] https://lists.oasis-open.org/archives/virtio-dev/202305/msg00291.html
> > > >> [2] https://lore.kernel.org/all/20230628030506.2213-1-hengqi@linux.alibaba.com/
> > > >>
> > > >> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > >> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > >> ---
> > > >> v1->v2:
> > > >>      1. Modify full checksum functionality as a configurable offload
> > > >>         that is initially turned off. @Jason
> > > >>
> > > >>   device-types/net/description.tex | 54 ++++++++++++++++++++++++++++----
> > > >>   1 file changed, 48 insertions(+), 6 deletions(-)
> > > >>
> > > >> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> > > >> index 76585b0..3c34f27 100644
> > > >> --- a/device-types/net/description.tex
> > > >> +++ b/device-types/net/description.tex
> > > >> @@ -88,6 +88,8 @@ \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_FULL_CSUM (50)] Driver handles packets with full checksum.
> > > >> +
> > > >>   \item[VIRTIO_NET_F_HASH_TUNNEL(51)] Device supports inner header hash for encapsulated packets.
> > > >>
> > > >>   \item[VIRTIO_NET_F_VQ_NOTF_COAL(52)] Device supports virtqueue notification coalescing.
> > > >> @@ -133,6 +135,7 @@ \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device
> > > >>   \item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > >>   \item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > >>   \item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > >> +\item[VIRTIO_NET_F_GUEST_FULL_CSUM] Requires VIRTIO_NET_F_GUEST_CSUM and VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
> > > >>
> > > >>   \item[VIRTIO_NET_F_HOST_TSO4] Requires VIRTIO_NET_F_CSUM.
> > > >>   \item[VIRTIO_NET_F_HOST_TSO6] Requires VIRTIO_NET_F_CSUM.
> > > > What about all of these:
> > > >
> > > > device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_TSO4] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > > device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_TSO6] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > > device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > > device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > > device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > >
> > > >
> > > >
> > > > can TSO/UFO/USO work with VIRTIO_NET_F_GUEST_FULL_CSUM as opposed to VIRTIO_NET_F_GUEST_CSUM?
> > >
> > > Both GUEST_FULL_CSUM and GUEST_CSUM can work with GUEST_TSO/USO/UFO.
> >
> > Yes. For software devices I guess it will have a lot of performance
> > penalty. So it should be disabled by default anyhow. The idea is to
> > delay the csum as late as possible.
>
> But for hardware it's actually better.

I can't think of a case where it might be better than XDP.

Most userspace doesn't care about the checksum though.

> Maybe we need a flag
> to say which offloads are expensive?
>

That exposes some device details which seem not good (e.g we may want
to do migration among hardware and software).

Thanks


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v2] virtio-net: support distinguishing between partial and full checksum

[Jason Wang]

On Wed, Nov 1, 2023 at 12:59 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
>
>
>
> 在 2023/11/1 下午12:16, Jason Wang 写道:
> > On Sat, Oct 28, 2023 at 10:36 AM Heng Qi <hengqi@linux.alibaba.com> wrote:
> >>
> >>
> >> 在 2023/10/27 下午3:39, Michael S. Tsirkin 写道:
> >>> On Thu, Oct 19, 2023 at 02:17:20PM +0800, Heng Qi wrote:
> >>>> virtio-net works in a virtualized system and is somewhat different from
> >>>> physical nics. One of the differences is that to save virtio device
> >>>> resources, rx may receive packets with partial checksum. However, XDP may
> >>>> cause partially checksummed packets to be dropped. So XDP loading conflicts
> >>>> with the feature VIRTIO_NET_F_GUEST_CSUM.
> >>>>
> >>>> This patch lets the device to supply fully checksummed packets to the driver.
> >>>> Then XDP can coexist with VIRTIO_NET_F_GUEST_CSUM to enjoy the benefits of
> >>>> device verification checksum.
> >>>>
> >>>> In addition, implementation of some performant devices do not generate
> >>>> partially checksummed packets, but the standard driver still need to clear
> >>>> VIRTIO_NET_F_GUEST_CSUM when loading XDP. If these devices enable the
> >>>> full checksum offloading, then the driver can load XDP without clearing
> >>>> VIRTIO_NET_F_GUEST_CSUM.
> >>>>
> >>>> A new feature bit VIRTIO_NET_F_GUEST_FULL_CSUM is added to solve the above
> >>>> situation, which provides the driver with configurable receive full checksum
> >>>> offload. If the offload is enabled, then the device must supply fully
> >>>> checksummed packets to the driver.
> >>>>
> >>>> Use case example:
> >>>> If VIRTIO_NET_F_GUEST_FULL_CSUM is negotiated and receive full checksum
> >>>> offload is enabled, after XDP processes a packet with full checksum, the
> >>>> VIRTIO_NET_HDR_F_DATA_VALID bit is still retained, resulting in the stack
> >>>> not needing to validate the checksum again. This is useful for guests:
> >>>>     1. Bring the driver advantages such as cpu savings.
> >>>>     2. For devices that do not generate partially checksummed packets themselves,
> >>>>        XDP can be loaded in the driver without modifying the hardware behavior.
> >>>>
> >>>> Several solutions have been discussed in the previous proposal[1].
> >>>> After historical discussion, we have tried the method proposed by Jason[2],
> >>>> but some complex scenarios and challenges are difficult to deal with.
> >>>> We now return to the method suggested in [1].
> >>>>
> >>>> [1] https://lists.oasis-open.org/archives/virtio-dev/202305/msg00291.html
> >>>> [2] https://lore.kernel.org/all/20230628030506.2213-1-hengqi@linux.alibaba.com/
> >>>>
> >>>> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> >>>> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> >>>> ---
> >>>> v1->v2:
> >>>>       1. Modify full checksum functionality as a configurable offload
> >>>>          that is initially turned off. @Jason
> >>>>
> >>>>    device-types/net/description.tex | 54 ++++++++++++++++++++++++++++----
> >>>>    1 file changed, 48 insertions(+), 6 deletions(-)
> >>>>
> >>>> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> >>>> index 76585b0..3c34f27 100644
> >>>> --- a/device-types/net/description.tex
> >>>> +++ b/device-types/net/description.tex
> >>>> @@ -88,6 +88,8 @@ \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_FULL_CSUM (50)] Driver handles packets with full checksum.
> >>>> +
> >>>>    \item[VIRTIO_NET_F_HASH_TUNNEL(51)] Device supports inner header hash for encapsulated packets.
> >>>>
> >>>>    \item[VIRTIO_NET_F_VQ_NOTF_COAL(52)] Device supports virtqueue notification coalescing.
> >>>> @@ -133,6 +135,7 @@ \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device
> >>>>    \item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM.
> >>>>    \item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM.
> >>>>    \item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM.
> >>>> +\item[VIRTIO_NET_F_GUEST_FULL_CSUM] Requires VIRTIO_NET_F_GUEST_CSUM and VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
> >>>>
> >>>>    \item[VIRTIO_NET_F_HOST_TSO4] Requires VIRTIO_NET_F_CSUM.
> >>>>    \item[VIRTIO_NET_F_HOST_TSO6] Requires VIRTIO_NET_F_CSUM.
> >>> What about all of these:
> >>>
> >>> device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_TSO4] Requires VIRTIO_NET_F_GUEST_CSUM.
> >>> device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_TSO6] Requires VIRTIO_NET_F_GUEST_CSUM.
> >>> device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM.
> >>> device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM.
> >>> device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM.
> >>>
> >>>
> >>>
> >>> can TSO/UFO/USO work with VIRTIO_NET_F_GUEST_FULL_CSUM as opposed to VIRTIO_NET_F_GUEST_CSUM?
> >> Both GUEST_FULL_CSUM and GUEST_CSUM can work with GUEST_TSO/USO/UFO.
> > Yes. For software devices I guess it will have a lot of performance
> > penalty. So it should be disabled by default anyhow. The idea is to
> > delay the csum as late as possible.
>
> Yes. I totally agree.
>
> >
> >> Their important difference is that if GUEST_CSUM is negotiated, the
> >> driver can handle partial checksum.
> >>
> >>>
> >>>> @@ -390,6 +393,13 @@ \subsection{Device Initialization}\label{sec:Device Types / Network Device / Dev
> >>>>    \ref{sec:Device Types / Network Device / Device Operation /
> >>>>    Processing of Incoming Packets}~\nameref{sec:Device Types /
> >>>>    Network Device / Device Operation / Processing of Incoming Packets} below.
> >>>> +
> >>>> +\item The VIRTIO_NET_F_GUEST_FULL_CSUM feature indicates that the driver handles
> >>>> +  packets with full checksum and does not handle packets with partial checksum,
> >>> So we need to change definition of VIRTIO_NET_F_GUEST_CSUM then.
> >>>
> >>>
> >>> Also this is not exactly right. As defined driver must be able to handle
> >>> partial checksum too.
> >>>
> >>>
> >>> How about this:
> >>>
> >>> - change definition above to just "Driver handles packets with full checksum."
> >>>
> >>> - if VIRTIO_NET_F_GUEST_FULL_CSUM is set but VIRTIO_NET_F_GUEST_CSUM is
> >>>     clear driver requires full checksum
> >>>
> >>> - if VIRTIO_NET_F_GUEST_FULL_CSUM is clear but VIRTIO_NET_F_GUEST_CSUM is
> >>>     set driver supports partial checksum
> >>>
> >>> - if VIRTIO_NET_F_GUEST_FULL_CSUM and VIRTIO_NET_F_GUEST_CSUM are
> >>>     set then the behavior is as you describe: VIRTIO_NET_F_GUEST_CSUM
> >>>     takes preference, but you can disable it with VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
> >>>     if that is supported.
> >> Jason wanted this feature to be enabled only when XDP is loading,
> >> and this is the context in which this patch was proposed.
> >>
> >> How do you pay attention to this?
> > I don't see any conflict, or anything I miss?
>
> Yes, our request was met.
>
> If GUEST_FULL_CSUM and GUEST_CSUM are independent,
> that is, GUEST_FULL_CSUM can be successfully validated without GUEST_CSUM.
> Then we need to re-describe most of the existing behavior of GUEST_CSUM
> for FULL_CSUM
> in the spec, this part is overlapping. Moreover, the relationship
> between FULL_CSUM
> and GUEST_CSUM also needs to be processed in the full text.
>
> So I think it seems clearer to constrain the behavior of GUEST_CSUM by
> treating FULL_CSUM as a subset of GUEST_CSUM.

This seems to work.

Thanks

>
> For example we don't need to make the following changes:
> \item[VIRTIO_NET_F_GUEST_TSO4] Requires VIRTIO_NET_F_GUEST_CSUM or
> VIRTIO_NET_F_GUEST_FULL_CSUM.
> \item[VIRTIO_NET_F_GUEST_TSO6] Requires VIRTIO_NET_F_GUEST_CSUM or
> VIRTIO_NET_F_GUEST_FULL_CSUM.
> \item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM or
> VIRTIO_NET_F_GUEST_FULL_CSUM.
> \item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM or
> VIRTIO_NET_F_GUEST_FULL_CSUM.
> \item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM or
> VIRTIO_NET_F_GUEST_FULL_CSUM.
>
> Thanks!
>
> >
> > Thanks
>
>
> This publicly archived list offers a means to provide input to the
> OASIS Virtual I/O Device (VIRTIO) TC.
>
> In order to verify user consent to the Feedback License terms and
> to minimize spam in the list archive, subscription is required
> before posting.
>
> Subscribe: virtio-comment-subscribe@lists.oasis-open.org
> Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
> List help: virtio-comment-help@lists.oasis-open.org
> List archive: https://lists.oasis-open.org/archives/virtio-comment/
> Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
> List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
> Committee: https://www.oasis-open.org/committees/virtio/
> Join OASIS: https://www.oasis-open.org/join/
>


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v2] virtio-net: support distinguishing between partial and full checksum

[Michael S. Tsirkin]

On Thu, Nov 02, 2023 at 12:40:03PM +0800, Jason Wang wrote:
> On Wed, Nov 1, 2023 at 1:37 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Wed, Nov 01, 2023 at 12:16:23PM +0800, Jason Wang wrote:
> > > On Sat, Oct 28, 2023 at 10:36 AM Heng Qi <hengqi@linux.alibaba.com> wrote:
> > > >
> > > >
> > > >
> > > > 在 2023/10/27 下午3:39, Michael S. Tsirkin 写道:
> > > > > On Thu, Oct 19, 2023 at 02:17:20PM +0800, Heng Qi wrote:
> > > > >> virtio-net works in a virtualized system and is somewhat different from
> > > > >> physical nics. One of the differences is that to save virtio device
> > > > >> resources, rx may receive packets with partial checksum. However, XDP may
> > > > >> cause partially checksummed packets to be dropped. So XDP loading conflicts
> > > > >> with the feature VIRTIO_NET_F_GUEST_CSUM.
> > > > >>
> > > > >> This patch lets the device to supply fully checksummed packets to the driver.
> > > > >> Then XDP can coexist with VIRTIO_NET_F_GUEST_CSUM to enjoy the benefits of
> > > > >> device verification checksum.
> > > > >>
> > > > >> In addition, implementation of some performant devices do not generate
> > > > >> partially checksummed packets, but the standard driver still need to clear
> > > > >> VIRTIO_NET_F_GUEST_CSUM when loading XDP. If these devices enable the
> > > > >> full checksum offloading, then the driver can load XDP without clearing
> > > > >> VIRTIO_NET_F_GUEST_CSUM.
> > > > >>
> > > > >> A new feature bit VIRTIO_NET_F_GUEST_FULL_CSUM is added to solve the above
> > > > >> situation, which provides the driver with configurable receive full checksum
> > > > >> offload. If the offload is enabled, then the device must supply fully
> > > > >> checksummed packets to the driver.
> > > > >>
> > > > >> Use case example:
> > > > >> If VIRTIO_NET_F_GUEST_FULL_CSUM is negotiated and receive full checksum
> > > > >> offload is enabled, after XDP processes a packet with full checksum, the
> > > > >> VIRTIO_NET_HDR_F_DATA_VALID bit is still retained, resulting in the stack
> > > > >> not needing to validate the checksum again. This is useful for guests:
> > > > >>    1. Bring the driver advantages such as cpu savings.
> > > > >>    2. For devices that do not generate partially checksummed packets themselves,
> > > > >>       XDP can be loaded in the driver without modifying the hardware behavior.
> > > > >>
> > > > >> Several solutions have been discussed in the previous proposal[1].
> > > > >> After historical discussion, we have tried the method proposed by Jason[2],
> > > > >> but some complex scenarios and challenges are difficult to deal with.
> > > > >> We now return to the method suggested in [1].
> > > > >>
> > > > >> [1] https://lists.oasis-open.org/archives/virtio-dev/202305/msg00291.html
> > > > >> [2] https://lore.kernel.org/all/20230628030506.2213-1-hengqi@linux.alibaba.com/
> > > > >>
> > > > >> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > > >> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > >> ---
> > > > >> v1->v2:
> > > > >>      1. Modify full checksum functionality as a configurable offload
> > > > >>         that is initially turned off. @Jason
> > > > >>
> > > > >>   device-types/net/description.tex | 54 ++++++++++++++++++++++++++++----
> > > > >>   1 file changed, 48 insertions(+), 6 deletions(-)
> > > > >>
> > > > >> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> > > > >> index 76585b0..3c34f27 100644
> > > > >> --- a/device-types/net/description.tex
> > > > >> +++ b/device-types/net/description.tex
> > > > >> @@ -88,6 +88,8 @@ \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_FULL_CSUM (50)] Driver handles packets with full checksum.
> > > > >> +
> > > > >>   \item[VIRTIO_NET_F_HASH_TUNNEL(51)] Device supports inner header hash for encapsulated packets.
> > > > >>
> > > > >>   \item[VIRTIO_NET_F_VQ_NOTF_COAL(52)] Device supports virtqueue notification coalescing.
> > > > >> @@ -133,6 +135,7 @@ \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device
> > > > >>   \item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > > >>   \item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > > >>   \item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > > >> +\item[VIRTIO_NET_F_GUEST_FULL_CSUM] Requires VIRTIO_NET_F_GUEST_CSUM and VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
> > > > >>
> > > > >>   \item[VIRTIO_NET_F_HOST_TSO4] Requires VIRTIO_NET_F_CSUM.
> > > > >>   \item[VIRTIO_NET_F_HOST_TSO6] Requires VIRTIO_NET_F_CSUM.
> > > > > What about all of these:
> > > > >
> > > > > device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_TSO4] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > > > device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_TSO6] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > > > device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > > > device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > > > device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > > >
> > > > >
> > > > >
> > > > > can TSO/UFO/USO work with VIRTIO_NET_F_GUEST_FULL_CSUM as opposed to VIRTIO_NET_F_GUEST_CSUM?
> > > >
> > > > Both GUEST_FULL_CSUM and GUEST_CSUM can work with GUEST_TSO/USO/UFO.
> > >
> > > Yes. For software devices I guess it will have a lot of performance
> > > penalty. So it should be disabled by default anyhow. The idea is to
> > > delay the csum as late as possible.
> >
> > But for hardware it's actually better.
> 
> I can't think of a case where it might be better than XDP.

Of course CHECKSUM_COMPLETE is better than CHECKSUM_PARTIAL if you can
support it: you don't even need to look at csum_start + csum_offset.
And the hardware doesn't need to parse L3/L4 headers to implement
CHECKSUM_COMPLETE.


> Most userspace doesn't care about the checksum though.
> 
> > Maybe we need a flag
> > to say which offloads are expensive?
> >
> 
> That exposes some device details which seem not good (e.g we may want
> to do migration among hardware and software).
> 
> Thanks

If you do then things will be less well tuned on one of the migration
ends but then that is by design, isn't it?

-- 
MST


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v2] virtio-net: support distinguishing between partial and full checksum

[Michael S. Tsirkin]

On Thu, Nov 02, 2023 at 01:30:22PM +0800, Jason Wang wrote:
> On Wed, Nov 1, 2023 at 12:59 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
> >
> >
> >
> > 在 2023/11/1 下午12:16, Jason Wang 写道:
> > > On Sat, Oct 28, 2023 at 10:36 AM Heng Qi <hengqi@linux.alibaba.com> wrote:
> > >>
> > >>
> > >> 在 2023/10/27 下午3:39, Michael S. Tsirkin 写道:
> > >>> On Thu, Oct 19, 2023 at 02:17:20PM +0800, Heng Qi wrote:
> > >>>> virtio-net works in a virtualized system and is somewhat different from
> > >>>> physical nics. One of the differences is that to save virtio device
> > >>>> resources, rx may receive packets with partial checksum. However, XDP may
> > >>>> cause partially checksummed packets to be dropped. So XDP loading conflicts
> > >>>> with the feature VIRTIO_NET_F_GUEST_CSUM.
> > >>>>
> > >>>> This patch lets the device to supply fully checksummed packets to the driver.
> > >>>> Then XDP can coexist with VIRTIO_NET_F_GUEST_CSUM to enjoy the benefits of
> > >>>> device verification checksum.
> > >>>>
> > >>>> In addition, implementation of some performant devices do not generate
> > >>>> partially checksummed packets, but the standard driver still need to clear
> > >>>> VIRTIO_NET_F_GUEST_CSUM when loading XDP. If these devices enable the
> > >>>> full checksum offloading, then the driver can load XDP without clearing
> > >>>> VIRTIO_NET_F_GUEST_CSUM.
> > >>>>
> > >>>> A new feature bit VIRTIO_NET_F_GUEST_FULL_CSUM is added to solve the above
> > >>>> situation, which provides the driver with configurable receive full checksum
> > >>>> offload. If the offload is enabled, then the device must supply fully
> > >>>> checksummed packets to the driver.
> > >>>>
> > >>>> Use case example:
> > >>>> If VIRTIO_NET_F_GUEST_FULL_CSUM is negotiated and receive full checksum
> > >>>> offload is enabled, after XDP processes a packet with full checksum, the
> > >>>> VIRTIO_NET_HDR_F_DATA_VALID bit is still retained, resulting in the stack
> > >>>> not needing to validate the checksum again. This is useful for guests:
> > >>>>     1. Bring the driver advantages such as cpu savings.
> > >>>>     2. For devices that do not generate partially checksummed packets themselves,
> > >>>>        XDP can be loaded in the driver without modifying the hardware behavior.
> > >>>>
> > >>>> Several solutions have been discussed in the previous proposal[1].
> > >>>> After historical discussion, we have tried the method proposed by Jason[2],
> > >>>> but some complex scenarios and challenges are difficult to deal with.
> > >>>> We now return to the method suggested in [1].
> > >>>>
> > >>>> [1] https://lists.oasis-open.org/archives/virtio-dev/202305/msg00291.html
> > >>>> [2] https://lore.kernel.org/all/20230628030506.2213-1-hengqi@linux.alibaba.com/
> > >>>>
> > >>>> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > >>>> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > >>>> ---
> > >>>> v1->v2:
> > >>>>       1. Modify full checksum functionality as a configurable offload
> > >>>>          that is initially turned off. @Jason
> > >>>>
> > >>>>    device-types/net/description.tex | 54 ++++++++++++++++++++++++++++----
> > >>>>    1 file changed, 48 insertions(+), 6 deletions(-)
> > >>>>
> > >>>> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> > >>>> index 76585b0..3c34f27 100644
> > >>>> --- a/device-types/net/description.tex
> > >>>> +++ b/device-types/net/description.tex
> > >>>> @@ -88,6 +88,8 @@ \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_FULL_CSUM (50)] Driver handles packets with full checksum.
> > >>>> +
> > >>>>    \item[VIRTIO_NET_F_HASH_TUNNEL(51)] Device supports inner header hash for encapsulated packets.
> > >>>>
> > >>>>    \item[VIRTIO_NET_F_VQ_NOTF_COAL(52)] Device supports virtqueue notification coalescing.
> > >>>> @@ -133,6 +135,7 @@ \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device
> > >>>>    \item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM.
> > >>>>    \item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM.
> > >>>>    \item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM.
> > >>>> +\item[VIRTIO_NET_F_GUEST_FULL_CSUM] Requires VIRTIO_NET_F_GUEST_CSUM and VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
> > >>>>
> > >>>>    \item[VIRTIO_NET_F_HOST_TSO4] Requires VIRTIO_NET_F_CSUM.
> > >>>>    \item[VIRTIO_NET_F_HOST_TSO6] Requires VIRTIO_NET_F_CSUM.
> > >>> What about all of these:
> > >>>
> > >>> device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_TSO4] Requires VIRTIO_NET_F_GUEST_CSUM.
> > >>> device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_TSO6] Requires VIRTIO_NET_F_GUEST_CSUM.
> > >>> device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM.
> > >>> device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM.
> > >>> device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM.
> > >>>
> > >>>
> > >>>
> > >>> can TSO/UFO/USO work with VIRTIO_NET_F_GUEST_FULL_CSUM as opposed to VIRTIO_NET_F_GUEST_CSUM?
> > >> Both GUEST_FULL_CSUM and GUEST_CSUM can work with GUEST_TSO/USO/UFO.
> > > Yes. For software devices I guess it will have a lot of performance
> > > penalty. So it should be disabled by default anyhow. The idea is to
> > > delay the csum as late as possible.
> >
> > Yes. I totally agree.
> >
> > >
> > >> Their important difference is that if GUEST_CSUM is negotiated, the
> > >> driver can handle partial checksum.
> > >>
> > >>>
> > >>>> @@ -390,6 +393,13 @@ \subsection{Device Initialization}\label{sec:Device Types / Network Device / Dev
> > >>>>    \ref{sec:Device Types / Network Device / Device Operation /
> > >>>>    Processing of Incoming Packets}~\nameref{sec:Device Types /
> > >>>>    Network Device / Device Operation / Processing of Incoming Packets} below.
> > >>>> +
> > >>>> +\item The VIRTIO_NET_F_GUEST_FULL_CSUM feature indicates that the driver handles
> > >>>> +  packets with full checksum and does not handle packets with partial checksum,
> > >>> So we need to change definition of VIRTIO_NET_F_GUEST_CSUM then.
> > >>>
> > >>>
> > >>> Also this is not exactly right. As defined driver must be able to handle
> > >>> partial checksum too.
> > >>>
> > >>>
> > >>> How about this:
> > >>>
> > >>> - change definition above to just "Driver handles packets with full checksum."
> > >>>
> > >>> - if VIRTIO_NET_F_GUEST_FULL_CSUM is set but VIRTIO_NET_F_GUEST_CSUM is
> > >>>     clear driver requires full checksum
> > >>>
> > >>> - if VIRTIO_NET_F_GUEST_FULL_CSUM is clear but VIRTIO_NET_F_GUEST_CSUM is
> > >>>     set driver supports partial checksum
> > >>>
> > >>> - if VIRTIO_NET_F_GUEST_FULL_CSUM and VIRTIO_NET_F_GUEST_CSUM are
> > >>>     set then the behavior is as you describe: VIRTIO_NET_F_GUEST_CSUM
> > >>>     takes preference, but you can disable it with VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
> > >>>     if that is supported.
> > >> Jason wanted this feature to be enabled only when XDP is loading,
> > >> and this is the context in which this patch was proposed.
> > >>
> > >> How do you pay attention to this?
> > > I don't see any conflict, or anything I miss?
> >
> > Yes, our request was met.
> >
> > If GUEST_FULL_CSUM and GUEST_CSUM are independent,
> > that is, GUEST_FULL_CSUM can be successfully validated without GUEST_CSUM.
> > Then we need to re-describe most of the existing behavior of GUEST_CSUM
> > for FULL_CSUM
> > in the spec, this part is overlapping. Moreover, the relationship
> > between FULL_CSUM
> > and GUEST_CSUM also needs to be processed in the full text.
> >
> > So I think it seems clearer to constrain the behavior of GUEST_CSUM by
> > treating FULL_CSUM as a subset of GUEST_CSUM.
> 
> This seems to work.
> 
> Thanks


I feel the changes here are minor and worth making for purposes
of making the features orthogonal. 
Further, I feel it's ok to default to offloads off for some features.
However, it is not at all clear from this patch when should driver
enable the offload and when it should not.
The fact that it needs a command to be enabled does not mean much:
drivers are  quite capable to invoke VIRTIO_NET_CTRL_GUEST_OFFLOADS
at startup and in fact some do.
So when should driver enable this as opposed to calculating the checksum
in software?



> >
> > For example we don't need to make the following changes:
> > \item[VIRTIO_NET_F_GUEST_TSO4] Requires VIRTIO_NET_F_GUEST_CSUM or
> > VIRTIO_NET_F_GUEST_FULL_CSUM.
> > \item[VIRTIO_NET_F_GUEST_TSO6] Requires VIRTIO_NET_F_GUEST_CSUM or
> > VIRTIO_NET_F_GUEST_FULL_CSUM.
> > \item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM or
> > VIRTIO_NET_F_GUEST_FULL_CSUM.
> > \item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM or
> > VIRTIO_NET_F_GUEST_FULL_CSUM.
> > \item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM or
> > VIRTIO_NET_F_GUEST_FULL_CSUM.
> >
> > Thanks!
> >
> > >
> > > Thanks
> >
> >
> > This publicly archived list offers a means to provide input to the
> > OASIS Virtual I/O Device (VIRTIO) TC.
> >
> > In order to verify user consent to the Feedback License terms and
> > to minimize spam in the list archive, subscription is required
> > before posting.
> >
> > Subscribe: virtio-comment-subscribe@lists.oasis-open.org
> > Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
> > List help: virtio-comment-help@lists.oasis-open.org
> > List archive: https://lists.oasis-open.org/archives/virtio-comment/
> > Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
> > List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
> > Committee: https://www.oasis-open.org/committees/virtio/
> > Join OASIS: https://www.oasis-open.org/join/
> >


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v2] virtio-net: support distinguishing between partial and full checksum

[Heng Qi]

在 2023/11/2 下午2:59, Michael S. Tsirkin 写道:
> On Thu, Nov 02, 2023 at 01:30:22PM +0800, Jason Wang wrote:
>> On Wed, Nov 1, 2023 at 12:59 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
>>>
>>>
>>> 在 2023/11/1 下午12:16, Jason Wang 写道:
>>>> On Sat, Oct 28, 2023 at 10:36 AM Heng Qi <hengqi@linux.alibaba.com> wrote:
>>>>>
>>>>> 在 2023/10/27 下午3:39, Michael S. Tsirkin 写道:
>>>>>> On Thu, Oct 19, 2023 at 02:17:20PM +0800, Heng Qi wrote:
>>>>>>> virtio-net works in a virtualized system and is somewhat different from
>>>>>>> physical nics. One of the differences is that to save virtio device
>>>>>>> resources, rx may receive packets with partial checksum. However, XDP may
>>>>>>> cause partially checksummed packets to be dropped. So XDP loading conflicts
>>>>>>> with the feature VIRTIO_NET_F_GUEST_CSUM.
>>>>>>>
>>>>>>> This patch lets the device to supply fully checksummed packets to the driver.
>>>>>>> Then XDP can coexist with VIRTIO_NET_F_GUEST_CSUM to enjoy the benefits of
>>>>>>> device verification checksum.
>>>>>>>
>>>>>>> In addition, implementation of some performant devices do not generate
>>>>>>> partially checksummed packets, but the standard driver still need to clear
>>>>>>> VIRTIO_NET_F_GUEST_CSUM when loading XDP. If these devices enable the
>>>>>>> full checksum offloading, then the driver can load XDP without clearing
>>>>>>> VIRTIO_NET_F_GUEST_CSUM.
>>>>>>>
>>>>>>> A new feature bit VIRTIO_NET_F_GUEST_FULL_CSUM is added to solve the above
>>>>>>> situation, which provides the driver with configurable receive full checksum
>>>>>>> offload. If the offload is enabled, then the device must supply fully
>>>>>>> checksummed packets to the driver.
>>>>>>>
>>>>>>> Use case example:
>>>>>>> If VIRTIO_NET_F_GUEST_FULL_CSUM is negotiated and receive full checksum
>>>>>>> offload is enabled, after XDP processes a packet with full checksum, the
>>>>>>> VIRTIO_NET_HDR_F_DATA_VALID bit is still retained, resulting in the stack
>>>>>>> not needing to validate the checksum again. This is useful for guests:
>>>>>>>      1. Bring the driver advantages such as cpu savings.
>>>>>>>      2. For devices that do not generate partially checksummed packets themselves,
>>>>>>>         XDP can be loaded in the driver without modifying the hardware behavior.
>>>>>>>
>>>>>>> Several solutions have been discussed in the previous proposal[1].
>>>>>>> After historical discussion, we have tried the method proposed by Jason[2],
>>>>>>> but some complex scenarios and challenges are difficult to deal with.
>>>>>>> We now return to the method suggested in [1].
>>>>>>>
>>>>>>> [1] https://lists.oasis-open.org/archives/virtio-dev/202305/msg00291.html
>>>>>>> [2] https://lore.kernel.org/all/20230628030506.2213-1-hengqi@linux.alibaba.com/
>>>>>>>
>>>>>>> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
>>>>>>> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>>>>>> ---
>>>>>>> v1->v2:
>>>>>>>        1. Modify full checksum functionality as a configurable offload
>>>>>>>           that is initially turned off. @Jason
>>>>>>>
>>>>>>>     device-types/net/description.tex | 54 ++++++++++++++++++++++++++++----
>>>>>>>     1 file changed, 48 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
>>>>>>> index 76585b0..3c34f27 100644
>>>>>>> --- a/device-types/net/description.tex
>>>>>>> +++ b/device-types/net/description.tex
>>>>>>> @@ -88,6 +88,8 @@ \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_FULL_CSUM (50)] Driver handles packets with full checksum.
>>>>>>> +
>>>>>>>     \item[VIRTIO_NET_F_HASH_TUNNEL(51)] Device supports inner header hash for encapsulated packets.
>>>>>>>
>>>>>>>     \item[VIRTIO_NET_F_VQ_NOTF_COAL(52)] Device supports virtqueue notification coalescing.
>>>>>>> @@ -133,6 +135,7 @@ \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device
>>>>>>>     \item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM.
>>>>>>>     \item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM.
>>>>>>>     \item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM.
>>>>>>> +\item[VIRTIO_NET_F_GUEST_FULL_CSUM] Requires VIRTIO_NET_F_GUEST_CSUM and VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
>>>>>>>
>>>>>>>     \item[VIRTIO_NET_F_HOST_TSO4] Requires VIRTIO_NET_F_CSUM.
>>>>>>>     \item[VIRTIO_NET_F_HOST_TSO6] Requires VIRTIO_NET_F_CSUM.
>>>>>> What about all of these:
>>>>>>
>>>>>> device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_TSO4] Requires VIRTIO_NET_F_GUEST_CSUM.
>>>>>> device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_TSO6] Requires VIRTIO_NET_F_GUEST_CSUM.
>>>>>> device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM.
>>>>>> device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM.
>>>>>> device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM.
>>>>>>
>>>>>>
>>>>>>
>>>>>> can TSO/UFO/USO work with VIRTIO_NET_F_GUEST_FULL_CSUM as opposed to VIRTIO_NET_F_GUEST_CSUM?
>>>>> Both GUEST_FULL_CSUM and GUEST_CSUM can work with GUEST_TSO/USO/UFO.
>>>> Yes. For software devices I guess it will have a lot of performance
>>>> penalty. So it should be disabled by default anyhow. The idea is to
>>>> delay the csum as late as possible.
>>> Yes. I totally agree.
>>>
>>>>> Their important difference is that if GUEST_CSUM is negotiated, the
>>>>> driver can handle partial checksum.
>>>>>
>>>>>>> @@ -390,6 +393,13 @@ \subsection{Device Initialization}\label{sec:Device Types / Network Device / Dev
>>>>>>>     \ref{sec:Device Types / Network Device / Device Operation /
>>>>>>>     Processing of Incoming Packets}~\nameref{sec:Device Types /
>>>>>>>     Network Device / Device Operation / Processing of Incoming Packets} below.
>>>>>>> +
>>>>>>> +\item The VIRTIO_NET_F_GUEST_FULL_CSUM feature indicates that the driver handles
>>>>>>> +  packets with full checksum and does not handle packets with partial checksum,
>>>>>> So we need to change definition of VIRTIO_NET_F_GUEST_CSUM then.
>>>>>>
>>>>>>
>>>>>> Also this is not exactly right. As defined driver must be able to handle
>>>>>> partial checksum too.
>>>>>>
>>>>>>
>>>>>> How about this:
>>>>>>
>>>>>> - change definition above to just "Driver handles packets with full checksum."
>>>>>>
>>>>>> - if VIRTIO_NET_F_GUEST_FULL_CSUM is set but VIRTIO_NET_F_GUEST_CSUM is
>>>>>>      clear driver requires full checksum
>>>>>>
>>>>>> - if VIRTIO_NET_F_GUEST_FULL_CSUM is clear but VIRTIO_NET_F_GUEST_CSUM is
>>>>>>      set driver supports partial checksum
>>>>>>
>>>>>> - if VIRTIO_NET_F_GUEST_FULL_CSUM and VIRTIO_NET_F_GUEST_CSUM are
>>>>>>      set then the behavior is as you describe: VIRTIO_NET_F_GUEST_CSUM
>>>>>>      takes preference, but you can disable it with VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
>>>>>>      if that is supported.
>>>>> Jason wanted this feature to be enabled only when XDP is loading,
>>>>> and this is the context in which this patch was proposed.
>>>>>
>>>>> How do you pay attention to this?
>>>> I don't see any conflict, or anything I miss?
>>> Yes, our request was met.
>>>
>>> If GUEST_FULL_CSUM and GUEST_CSUM are independent,
>>> that is, GUEST_FULL_CSUM can be successfully validated without GUEST_CSUM.
>>> Then we need to re-describe most of the existing behavior of GUEST_CSUM
>>> for FULL_CSUM
>>> in the spec, this part is overlapping. Moreover, the relationship
>>> between FULL_CSUM
>>> and GUEST_CSUM also needs to be processed in the full text.
>>>
>>> So I think it seems clearer to constrain the behavior of GUEST_CSUM by
>>> treating FULL_CSUM as a subset of GUEST_CSUM.
>> This seems to work.
>>
>> Thanks
>
> I feel the changes here are minor and worth making for purposes
> of making the features orthogonal.

I think it is also possible to make FULL_CSUM and GUEST_CSUM orthogonal,
but could you explain the benefits of this approach in comparison to the 
current version?

> Further, I feel it's ok to default to offloads off for some features.
> However, it is not at all clear from this patch when should driver
> enable the offload and when it should not.
> The fact that it needs a command to be enabled does not mean much:
> drivers are  quite capable to invoke VIRTIO_NET_CTRL_GUEST_OFFLOADS
> at startup and in fact some do.
> So when should driver enable this as opposed to calculating the checksum
> in software?

I think the timing is when something happens to the driver that 
conflicts with
partial checksum, at which point the driver want to enable this full csum.

Other than that, guest csum works.

Thanks!

>
>
>
>>> For example we don't need to make the following changes:
>>> \item[VIRTIO_NET_F_GUEST_TSO4] Requires VIRTIO_NET_F_GUEST_CSUM or
>>> VIRTIO_NET_F_GUEST_FULL_CSUM.
>>> \item[VIRTIO_NET_F_GUEST_TSO6] Requires VIRTIO_NET_F_GUEST_CSUM or
>>> VIRTIO_NET_F_GUEST_FULL_CSUM.
>>> \item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM or
>>> VIRTIO_NET_F_GUEST_FULL_CSUM.
>>> \item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM or
>>> VIRTIO_NET_F_GUEST_FULL_CSUM.
>>> \item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM or
>>> VIRTIO_NET_F_GUEST_FULL_CSUM.
>>>
>>> Thanks!
>>>
>>>> Thanks
>>>
>>> This publicly archived list offers a means to provide input to the
>>> OASIS Virtual I/O Device (VIRTIO) TC.
>>>
>>> In order to verify user consent to the Feedback License terms and
>>> to minimize spam in the list archive, subscription is required
>>> before posting.
>>>
>>> Subscribe: virtio-comment-subscribe@lists.oasis-open.org
>>> Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
>>> List help: virtio-comment-help@lists.oasis-open.org
>>> List archive: https://lists.oasis-open.org/archives/virtio-comment/
>>> Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
>>> List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
>>> Committee: https://www.oasis-open.org/committees/virtio/
>>> Join OASIS: https://www.oasis-open.org/join/
>>>
>
> This publicly archived list offers a means to provide input to the
> OASIS Virtual I/O Device (VIRTIO) TC.
>
> In order to verify user consent to the Feedback License terms and
> to minimize spam in the list archive, subscription is required
> before posting.
>
> Subscribe: virtio-comment-subscribe@lists.oasis-open.org
> Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
> List help: virtio-comment-help@lists.oasis-open.org
> List archive: https://lists.oasis-open.org/archives/virtio-comment/
> Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
> List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
> Committee: https://www.oasis-open.org/committees/virtio/
> Join OASIS: https://www.oasis-open.org/join/


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v2] virtio-net: support distinguishing between partial and full checksum

[Jason Wang]

On Thu, Nov 2, 2023 at 2:50 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Nov 02, 2023 at 12:40:03PM +0800, Jason Wang wrote:
> > On Wed, Nov 1, 2023 at 1:37 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Wed, Nov 01, 2023 at 12:16:23PM +0800, Jason Wang wrote:
> > > > On Sat, Oct 28, 2023 at 10:36 AM Heng Qi <hengqi@linux.alibaba.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > 在 2023/10/27 下午3:39, Michael S. Tsirkin 写道:
> > > > > > On Thu, Oct 19, 2023 at 02:17:20PM +0800, Heng Qi wrote:
> > > > > >> virtio-net works in a virtualized system and is somewhat different from
> > > > > >> physical nics. One of the differences is that to save virtio device
> > > > > >> resources, rx may receive packets with partial checksum. However, XDP may
> > > > > >> cause partially checksummed packets to be dropped. So XDP loading conflicts
> > > > > >> with the feature VIRTIO_NET_F_GUEST_CSUM.
> > > > > >>
> > > > > >> This patch lets the device to supply fully checksummed packets to the driver.
> > > > > >> Then XDP can coexist with VIRTIO_NET_F_GUEST_CSUM to enjoy the benefits of
> > > > > >> device verification checksum.
> > > > > >>
> > > > > >> In addition, implementation of some performant devices do not generate
> > > > > >> partially checksummed packets, but the standard driver still need to clear
> > > > > >> VIRTIO_NET_F_GUEST_CSUM when loading XDP. If these devices enable the
> > > > > >> full checksum offloading, then the driver can load XDP without clearing
> > > > > >> VIRTIO_NET_F_GUEST_CSUM.
> > > > > >>
> > > > > >> A new feature bit VIRTIO_NET_F_GUEST_FULL_CSUM is added to solve the above
> > > > > >> situation, which provides the driver with configurable receive full checksum
> > > > > >> offload. If the offload is enabled, then the device must supply fully
> > > > > >> checksummed packets to the driver.
> > > > > >>
> > > > > >> Use case example:
> > > > > >> If VIRTIO_NET_F_GUEST_FULL_CSUM is negotiated and receive full checksum
> > > > > >> offload is enabled, after XDP processes a packet with full checksum, the
> > > > > >> VIRTIO_NET_HDR_F_DATA_VALID bit is still retained, resulting in the stack
> > > > > >> not needing to validate the checksum again. This is useful for guests:
> > > > > >>    1. Bring the driver advantages such as cpu savings.
> > > > > >>    2. For devices that do not generate partially checksummed packets themselves,
> > > > > >>       XDP can be loaded in the driver without modifying the hardware behavior.
> > > > > >>
> > > > > >> Several solutions have been discussed in the previous proposal[1].
> > > > > >> After historical discussion, we have tried the method proposed by Jason[2],
> > > > > >> but some complex scenarios and challenges are difficult to deal with.
> > > > > >> We now return to the method suggested in [1].
> > > > > >>
> > > > > >> [1] https://lists.oasis-open.org/archives/virtio-dev/202305/msg00291.html
> > > > > >> [2] https://lore.kernel.org/all/20230628030506.2213-1-hengqi@linux.alibaba.com/
> > > > > >>
> > > > > >> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > > > >> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > > >> ---
> > > > > >> v1->v2:
> > > > > >>      1. Modify full checksum functionality as a configurable offload
> > > > > >>         that is initially turned off. @Jason
> > > > > >>
> > > > > >>   device-types/net/description.tex | 54 ++++++++++++++++++++++++++++----
> > > > > >>   1 file changed, 48 insertions(+), 6 deletions(-)
> > > > > >>
> > > > > >> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> > > > > >> index 76585b0..3c34f27 100644
> > > > > >> --- a/device-types/net/description.tex
> > > > > >> +++ b/device-types/net/description.tex
> > > > > >> @@ -88,6 +88,8 @@ \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_FULL_CSUM (50)] Driver handles packets with full checksum.
> > > > > >> +
> > > > > >>   \item[VIRTIO_NET_F_HASH_TUNNEL(51)] Device supports inner header hash for encapsulated packets.
> > > > > >>
> > > > > >>   \item[VIRTIO_NET_F_VQ_NOTF_COAL(52)] Device supports virtqueue notification coalescing.
> > > > > >> @@ -133,6 +135,7 @@ \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device
> > > > > >>   \item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > > > >>   \item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > > > >>   \item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > > > >> +\item[VIRTIO_NET_F_GUEST_FULL_CSUM] Requires VIRTIO_NET_F_GUEST_CSUM and VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
> > > > > >>
> > > > > >>   \item[VIRTIO_NET_F_HOST_TSO4] Requires VIRTIO_NET_F_CSUM.
> > > > > >>   \item[VIRTIO_NET_F_HOST_TSO6] Requires VIRTIO_NET_F_CSUM.
> > > > > > What about all of these:
> > > > > >
> > > > > > device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_TSO4] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > > > > device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_TSO6] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > > > > device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > > > > device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > > > > device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > > > >
> > > > > >
> > > > > >
> > > > > > can TSO/UFO/USO work with VIRTIO_NET_F_GUEST_FULL_CSUM as opposed to VIRTIO_NET_F_GUEST_CSUM?
> > > > >
> > > > > Both GUEST_FULL_CSUM and GUEST_CSUM can work with GUEST_TSO/USO/UFO.
> > > >
> > > > Yes. For software devices I guess it will have a lot of performance
> > > > penalty. So it should be disabled by default anyhow. The idea is to
> > > > delay the csum as late as possible.
> > >
> > > But for hardware it's actually better.
> >
> > I can't think of a case where it might be better than XDP.
>
> Of course CHECKSUM_COMPLETE is better than CHECKSUM_PARTIAL if you can
> support it: you don't even need to look at csum_start + csum_offset.

I may miss something here, if the packet was delivered to the
userspace then they don't care.

> And the hardware doesn't need to parse L3/L4 headers to implement
> CHECKSUM_COMPLETE.

Hardware may choose to coalesce packets.

>
>
> > Most userspace doesn't care about the checksum though.
> >
> > > Maybe we need a flag
> > > to say which offloads are expensive?
> > >
> >
> > That exposes some device details which seem not good (e.g we may want
> > to do migration among hardware and software).
> >
> > Thanks
>
> If you do then things will be less well tuned on one of the migration
> ends but then that is by design, isn't it?

Ok, so I'm fine to enable it by default.

Thanks

>
> --
> MST
>


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v2] virtio-net: support distinguishing between partial and full checksum

[Michael S. Tsirkin]

On Thu, Nov 09, 2023 at 11:55:01AM +0800, Jason Wang wrote:
> > > Most userspace doesn't care about the checksum though.
> > >
> > > > Maybe we need a flag
> > > > to say which offloads are expensive?
> > > >
> > >
> > > That exposes some device details which seem not good (e.g we may want
> > > to do migration among hardware and software).
> > >
> > > Thanks
> >
> > If you do then things will be less well tuned on one of the migration
> > ends but then that is by design, isn't it?
> 
> Ok, so I'm fine to enable it by default.
> 
> Thanks

I was really asking a question. Is it true that full is more expensive
than partial for the device? So it should only be enabled if
strictly required?

> >
> > --
> > MST
> >


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v2] virtio-net: support distinguishing between partial and full checksum

[Heng Qi]

在 2023/11/9 下午4:01, Michael S. Tsirkin 写道:
> On Thu, Nov 09, 2023 at 11:55:01AM +0800, Jason Wang wrote:
>>>> Most userspace doesn't care about the checksum though.
>>>>
>>>>> Maybe we need a flag
>>>>> to say which offloads are expensive?
>>>>>
>>>> That exposes some device details which seem not good (e.g we may want
>>>> to do migration among hardware and software).
>>>>
>>>> Thanks
>>> If you do then things will be less well tuned on one of the migration
>>> ends but then that is by design, isn't it?
>> Ok, so I'm fine to enable it by default.
>>
>> Thanks
> I was really asking a question. Is it true that full is more expensive
> than partial for the device?

It depends on the device. For performant (e.g. IO queues are hardware 
accelerated)
or well-resourced devices, the overhead of full csum compared to partial 
csum is almost negligible.

> So it should only be enabled if
> strictly required?

I think for software devices, yes.

Thanks.

>
>>> --
>>> MST
>>>
>
> This publicly archived list offers a means to provide input to the
> OASIS Virtual I/O Device (VIRTIO) TC.
>
> In order to verify user consent to the Feedback License terms and
> to minimize spam in the list archive, subscription is required
> before posting.
>
> Subscribe: virtio-comment-subscribe@lists.oasis-open.org
> Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
> List help: virtio-comment-help@lists.oasis-open.org
> List archive: https://lists.oasis-open.org/archives/virtio-comment/
> Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
> List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
> Committee: https://www.oasis-open.org/committees/virtio/
> Join OASIS: https://www.oasis-open.org/join/


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/