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

[virtio-comment] [PATCH v3] 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>
---
v2->v3:
- Add a section named "Driver Handles Fully Checksummed Packets"
  and more descriptions. @Michael

v1->v2:
- Modify full checksum functionality as a configurable offload
  that is initially turned off. @Jason

 device-types/net/description.tex        | 73 +++++++++++++++++++++++--
 device-types/net/device-conformance.tex |  1 +
 device-types/net/driver-conformance.tex |  1 +
 3 files changed, 69 insertions(+), 6 deletions(-)

diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index aff5e08..6937a2f 100644
--- a/device-types/net/description.tex
+++ b/device-types/net/description.tex
@@ -122,6 +122,8 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
     device with the same MAC address.
 
 \item[VIRTIO_NET_F_SPEED_DUPLEX(63)] Device reports speed and duplex.
+
+\item[VIRTIO_NET_F_GUEST_FULL_CSUM (64)] Driver handles packets with full checksum.
 \end{description}
 
 \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device / Feature bits / Feature bit requirements}
@@ -136,6 +138,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.
@@ -398,6 +401,58 @@ \subsection{Device Initialization}\label{sec:Device Types / Network Device / Dev
 A truly minimal driver would only accept VIRTIO_NET_F_MAC and ignore
 everything else.
 
+\subsubsection{Driver Handles Fully Checksummed Packets}\label{sec:Device Types / Network Device / Device Initialization / Driver Handles Fully Checksummed Packets}
+
+The VIRTIO_NET_F_GUEST_CSUM feature indicates that the driver can handle
+partially or fully checksummed packets from the device. When the
+driver only expects fully checksummed packets, the VIRTIO_NET_F_GUEST_FULL_CSUM
+feature can be negotiated if the device offers it.
+Then the driver only handles packets with full checksum.
+
+By negotiating the VIRTIO_NET_F_GUEST_FULL_CSUM feature, the driver can
+benefit, for example, from the device's ability to calculate and validate the checksum
+in scenarios where partially checksummed packets are not compatible.
+
+Delivering fully checksummed packets rather than partially
+checksummed packets incurs additional overhead for the device.
+As a result, receive full-checksum offload (meaning the driver only handles
+packets with full checksum) is disabled by default \ref{sec:Device Types / Network Device
+/ Device Operation / Control Virtqueue / Offloads State Configuration}.
+
+Receive full-checksum offload can be enabled if the driver successfully
+sends the VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET command with the
+VIRTIO_NET_F_GUEST_FULL_CSUM bit set.
+
+\drivernormative{\subsubsection}{Driver Handles Fully Checksummed Packets}{sec:Device Types / Network Device / Device Initialization / Driver Handles Fully Checksummed Packets}
+
+The driver MUST NOT enable receive full-checksum offload for which
+VIRTIO_NET_F_GUEST_FULL_CSUM has not been negotiated.
+
+\devicenormative{\subsubsection}{Driver Handles Fully Checksummed Packets}{sec:Device Types / Network Device / Device Initialization / Driver Handles Fully Checksummed Packets}
+
+Initially (before the device successfully receives any
+VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET command with the VIRTIO_NET_F_GUEST_FULL_CSUM
+bit set) 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 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 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 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.
+
 \subsection{Device Operation}\label{sec:Device Types / Network Device / Device Operation}
 
 Packets are transmitted by placing them in the
@@ -723,7 +778,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
@@ -747,7 +803,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
@@ -805,8 +862,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
@@ -826,7 +884,8 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
 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 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
@@ -842,7 +901,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
@@ -1633,6 +1693,7 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
 #define VIRTIO_NET_F_GUEST_UFO        10
 #define VIRTIO_NET_F_GUEST_USO4       54
 #define VIRTIO_NET_F_GUEST_USO6       55
+#define VIRTIO_NET_F_GUEST_FULL_CSUM  64
 
 #define VIRTIO_NET_CTRL_GUEST_OFFLOADS       5
  #define VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET   0
diff --git a/device-types/net/device-conformance.tex b/device-types/net/device-conformance.tex
index 52526e4..e72cb5b 100644
--- a/device-types/net/device-conformance.tex
+++ b/device-types/net/device-conformance.tex
@@ -16,4 +16,5 @@
 \item \ref{devicenormative:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing}
 \item \ref{devicenormative:Device Types / Network Device / Device Operation / Control Virtqueue / Inner Header Hash}
 \item \ref{devicenormative:Device Types / Network Device / Device Operation / Control Virtqueue / Device Statistics}
+\item \ref{devicenormative:Device Types / Network Device / Device Initialization / Driver Handles Fully Checksummed Packets}
 \end{itemize}
diff --git a/device-types/net/driver-conformance.tex b/device-types/net/driver-conformance.tex
index c693c4f..6a1d7a7 100644
--- a/device-types/net/driver-conformance.tex
+++ b/device-types/net/driver-conformance.tex
@@ -16,4 +16,5 @@
 \item \ref{drivernormative:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing}
 \item \ref{drivernormative:Device Types / Network Device / Device Operation / Control Virtqueue / Inner Header Hash}
 \item \ref{drivernormative:Device Types / Network Device / Device Operation / Control Virtqueue / Device Statistics}
+\item \ref{drivernormative:Device Types / Network Device / Device Initialization / Driver Handles Fully Checksummed Packets}
 \end{itemize}
-- 
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/

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

[Jason Wang]

On Tue, Nov 14, 2023 at 4:49 PM Heng Qi <hengqi@linux.alibaba.com> 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>
> ---
> v2->v3:
> - Add a section named "Driver Handles Fully Checksummed Packets"
>   and more descriptions. @Michael
>
> v1->v2:
> - Modify full checksum functionality as a configurable offload
>   that is initially turned off. @Jason
>
>  device-types/net/description.tex        | 73 +++++++++++++++++++++++--
>  device-types/net/device-conformance.tex |  1 +
>  device-types/net/driver-conformance.tex |  1 +
>  3 files changed, 69 insertions(+), 6 deletions(-)
>
> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> index aff5e08..6937a2f 100644
> --- a/device-types/net/description.tex
> +++ b/device-types/net/description.tex
> @@ -122,6 +122,8 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
>      device with the same MAC address.
>
>  \item[VIRTIO_NET_F_SPEED_DUPLEX(63)] Device reports speed and duplex.
> +
> +\item[VIRTIO_NET_F_GUEST_FULL_CSUM (64)] Driver handles packets with full checksum.
>  \end{description}
>
>  \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device / Feature bits / Feature bit requirements}
> @@ -136,6 +138,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.
> @@ -398,6 +401,58 @@ \subsection{Device Initialization}\label{sec:Device Types / Network Device / Dev
>  A truly minimal driver would only accept VIRTIO_NET_F_MAC and ignore
>  everything else.
>
> +\subsubsection{Driver Handles Fully Checksummed Packets}\label{sec:Device Types / Network Device / Device Initialization / Driver Handles Fully Checksummed Packets}
> +
> +The VIRTIO_NET_F_GUEST_CSUM feature indicates that the driver can handle
> +partially or fully checksummed packets from the device. When the
> +driver only expects fully checksummed packets, the VIRTIO_NET_F_GUEST_FULL_CSUM
> +feature can be negotiated if the device offers it.
> +Then the driver only handles packets with full checksum.
> +
> +By negotiating the VIRTIO_NET_F_GUEST_FULL_CSUM feature, the driver can
> +benefit, for example, from the device's ability to calculate and validate the checksum
> +in scenarios where partially checksummed packets are not compatible.

Not a native speaker, but I think it's not about compatibility. Maybe
we can just drop the "in scenarios ..."?

> +
> +Delivering fully checksummed packets rather than partially
> +checksummed packets incurs additional overhead for the device.
> +As a result, receive full-checksum offload (meaning the driver only handles
> +packets with full checksum) is disabled by default \ref{sec:Device Types / Network Device
> +/ Device Operation / Control Virtqueue / Offloads State Configuration}.
> +
> +Receive full-checksum offload can be enabled if the driver successfully
> +sends the VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET command with the
> +VIRTIO_NET_F_GUEST_FULL_CSUM bit set.
> +
> +\drivernormative{\subsubsection}{Driver Handles Fully Checksummed Packets}{sec:Device Types / Network Device / Device Initialization / Driver Handles Fully Checksummed Packets}
> +
> +The driver MUST NOT enable receive full-checksum offload for which
> +VIRTIO_NET_F_GUEST_FULL_CSUM has not been negotiated.
> +
> +\devicenormative{\subsubsection}{Driver Handles Fully Checksummed Packets}{sec:Device Types / Network Device / Device Initialization / Driver Handles Fully Checksummed Packets}
> +
> +Initially (before the device successfully receives any
> +VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET command with the VIRTIO_NET_F_GUEST_FULL_CSUM
> +bit set) receive full-checksum offload MUST be disabled.

This seems duplicated with the above

"
+As a result, receive full-checksum offload (meaning the driver only handles
+packets with full checksum) is disabled by default \ref{sec:Device
Types / Network Device
+/ Device Operation / Control Virtqueue / Offloads State Configuration}.
"

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

Well the control offload semantic explains this by itself, so I don't
think we need to repeat it here.

> +
> +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}.

This seems another duplication as no matter what kind of packet is
received the full checksum should be calculated.

> +
> +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 receive full-checksum offload has been enabled and \field{gso_type}
> +differs from VIRTIO_NET_HDR_GSO_NONE,

Any reason why VIRTIO_NET_HDR_GSO_NONE differs from others?

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

I think we need to clarify:

1) when the packet has full checksum, we set DATA_VALID but not NEEDS_CSUM?
2) when the packet has partial checksum, we set NEEDS_CSUM for sure,
but a conditional DATA_VALID?

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/

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

[Michael S. Tsirkin]

On Tue, Nov 14, 2023 at 04:49:45PM +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>
> ---
> v2->v3:
> - Add a section named "Driver Handles Fully Checksummed Packets"
>   and more descriptions. @Michael
> 
> v1->v2:
> - Modify full checksum functionality as a configurable offload
>   that is initially turned off. @Jason
> 
>  device-types/net/description.tex        | 73 +++++++++++++++++++++++--
>  device-types/net/device-conformance.tex |  1 +
>  device-types/net/driver-conformance.tex |  1 +
>  3 files changed, 69 insertions(+), 6 deletions(-)
> 
> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> index aff5e08..6937a2f 100644
> --- a/device-types/net/description.tex
> +++ b/device-types/net/description.tex
> @@ -122,6 +122,8 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
>      device with the same MAC address.
>  
>  \item[VIRTIO_NET_F_SPEED_DUPLEX(63)] Device reports speed and duplex.
> +
> +\item[VIRTIO_NET_F_GUEST_FULL_CSUM (64)] Driver handles packets with full checksum.
>  \end{description}
>  
>  \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device / Feature bits / Feature bit requirements}
> @@ -136,6 +138,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.



Apparently this is just to make the patch a bit smaller so you do not
have to find all instances of VIRTIO_NET_F_GUEST_CSUM and replace them
with "VIRTIO_NET_F_GUEST_CSUM or VIRTIO_NET_F_GUEST_FULL_CSUM".

But, this is actually a problem : we have places in spec which only
say VIRTIO_NET_F_GUEST_CSUM but actually mean "VIRTIO_NET_F_GUEST_CSUM
negotiated and not disabled by VIRTIO_NET_F_CTRL_GUEST_OFFLOADS".

So this just makes no sense to me. We have:
\item[VIRTIO_NET_F_GUEST_CSUM (1)] Driver handles packets with partial checksum.


and here apparently when you have driver that handles packets with partial checksum
*and* packets with full checksum then this means that no, it does not
handle packets with partial checksum.
It might look ok when you just look at the patch but when people read
the full spec this is just confusing.



Let me try: in fact VIRTIO_NET_F_GUEST_CSUM now means "driver handles
packets with checksum (partial or full)"? Even with no features checksum can cover
all of the packet nothing prevents that.

So now we have a new flag that means
"driver can only handle fully checksummed packets".






> @@ -398,6 +401,58 @@ \subsection{Device Initialization}\label{sec:Device Types / Network Device / Dev
>  A truly minimal driver would only accept VIRTIO_NET_F_MAC and ignore
>  everything else.
>  
> +\subsubsection{Driver Handles Fully Checksummed Packets}\label{sec:Device Types / Network Device / Device Initialization / Driver Handles Fully Checksummed Packets}
> +
> +The VIRTIO_NET_F_GUEST_CSUM feature indicates that the driver can handle
> +partially or fully checksummed packets from the device. When the
> +driver only expects fully checksummed packets, the VIRTIO_NET_F_GUEST_FULL_CSUM
> +feature can be negotiated if the device offers it.
> +Then the driver only handles packets with full checksum.
> +
> +By negotiating the VIRTIO_NET_F_GUEST_FULL_CSUM feature, the driver can
> +benefit, for example, from the device's ability to calculate and validate the checksum
> +in scenarios where partially checksummed packets are not compatible.
> +
> +Delivering fully checksummed packets rather than partially
> +checksummed packets incurs additional overhead for the device.
> +As a result, receive full-checksum offload (meaning the driver only handles
> +packets with full checksum) is disabled by default \ref{sec:Device Types / Network Device
> +/ Device Operation / Control Virtqueue / Offloads State Configuration}.
> +
> +Receive full-checksum offload can be enabled if the driver successfully
> +sends the VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET command with the
> +VIRTIO_NET_F_GUEST_FULL_CSUM bit set.

But how much is this "additional overhead" and how does driver know when
this should be enabled as opposed to just disabling checksum offload
completely?


> +
> +\drivernormative{\subsubsection}{Driver Handles Fully Checksummed Packets}{sec:Device Types / Network Device / Device Initialization / Driver Handles Fully Checksummed Packets}
> +
> +The driver MUST NOT enable receive full-checksum offload for which
> +VIRTIO_NET_F_GUEST_FULL_CSUM has not been negotiated.

I don't understand what this is saying.  We need to come up with a way
to document this all without inventing terms like "full-checksum offload".

IIUC all this does is basically require that checksum covers all of the
packet.

-- 
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] Re: [PATCH v3] virtio-net: support distinguishing between partial and full checksum

[Heng Qi]

在 2023/11/16 下午1:34, Jason Wang 写道:
> On Tue, Nov 14, 2023 at 4:49 PM Heng Qi <hengqi@linux.alibaba.com> 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>
>> ---
>> v2->v3:
>> - Add a section named "Driver Handles Fully Checksummed Packets"
>>    and more descriptions. @Michael
>>
>> v1->v2:
>> - Modify full checksum functionality as a configurable offload
>>    that is initially turned off. @Jason
>>
>>   device-types/net/description.tex        | 73 +++++++++++++++++++++++--
>>   device-types/net/device-conformance.tex |  1 +
>>   device-types/net/driver-conformance.tex |  1 +
>>   3 files changed, 69 insertions(+), 6 deletions(-)
>>
>> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
>> index aff5e08..6937a2f 100644
>> --- a/device-types/net/description.tex
>> +++ b/device-types/net/description.tex
>> @@ -122,6 +122,8 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
>>       device with the same MAC address.
>>
>>   \item[VIRTIO_NET_F_SPEED_DUPLEX(63)] Device reports speed and duplex.
>> +
>> +\item[VIRTIO_NET_F_GUEST_FULL_CSUM (64)] Driver handles packets with full checksum.
>>   \end{description}
>>
>>   \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device / Feature bits / Feature bit requirements}
>> @@ -136,6 +138,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.
>> @@ -398,6 +401,58 @@ \subsection{Device Initialization}\label{sec:Device Types / Network Device / Dev
>>   A truly minimal driver would only accept VIRTIO_NET_F_MAC and ignore
>>   everything else.
>>
>> +\subsubsection{Driver Handles Fully Checksummed Packets}\label{sec:Device Types / Network Device / Device Initialization / Driver Handles Fully Checksummed Packets}
>> +
>> +The VIRTIO_NET_F_GUEST_CSUM feature indicates that the driver can handle
>> +partially or fully checksummed packets from the device. When the
>> +driver only expects fully checksummed packets, the VIRTIO_NET_F_GUEST_FULL_CSUM
>> +feature can be negotiated if the device offers it.
>> +Then the driver only handles packets with full checksum.
>> +
>> +By negotiating the VIRTIO_NET_F_GUEST_FULL_CSUM feature, the driver can
>> +benefit, for example, from the device's ability to calculate and validate the checksum
>> +in scenarios where partially checksummed packets are not compatible.
> Not a native speaker, but I think it's not about compatibility. Maybe
> we can just drop the "in scenarios ..."?

Ok! Agree.

>
>> +
>> +Delivering fully checksummed packets rather than partially
>> +checksummed packets incurs additional overhead for the device.
>> +As a result, receive full-checksum offload (meaning the driver only handles
>> +packets with full checksum) is disabled by default \ref{sec:Device Types / Network Device
>> +/ Device Operation / Control Virtqueue / Offloads State Configuration}.
>> +
>> +Receive full-checksum offload can be enabled if the driver successfully
>> +sends the VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET command with the
>> +VIRTIO_NET_F_GUEST_FULL_CSUM bit set.
>> +
>> +\drivernormative{\subsubsection}{Driver Handles Fully Checksummed Packets}{sec:Device Types / Network Device / Device Initialization / Driver Handles Fully Checksummed Packets}
>> +
>> +The driver MUST NOT enable receive full-checksum offload for which
>> +VIRTIO_NET_F_GUEST_FULL_CSUM has not been negotiated.
>> +
>> +\devicenormative{\subsubsection}{Driver Handles Fully Checksummed Packets}{sec:Device Types / Network Device / Device Initialization / Driver Handles Fully Checksummed Packets}
>> +
>> +Initially (before the device successfully receives any
>> +VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET command with the VIRTIO_NET_F_GUEST_FULL_CSUM
>> +bit set) receive full-checksum offload MUST be disabled.
> This seems duplicated with the above
>
> "
> +As a result, receive full-checksum offload (meaning the driver only handles
> +packets with full checksum) is disabled by default \ref{sec:Device
> Types / Network Device
> +/ Device Operation / Control Virtqueue / Offloads State Configuration}.
> "

Yes, you are right. But the two places are text and normativity 
respectively.

>
>> +
>> +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.
> Well the control offload semantic explains this by itself, so I don't
> think we need to repeat it here.

I will drop this.

>
>> +
>> +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}.
> This seems another duplication as no matter what kind of packet is
> received the full checksum should be calculated.

>> +
>> +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 receive full-checksum offload has been enabled and \field{gso_type}
>> +differs from VIRTIO_NET_HDR_GSO_NONE,
> Any reason why VIRTIO_NET_HDR_GSO_NONE differs from others?

It is to correspond to the following
(this patch also modifies the following paragraph accordingly):

"If gso_type differs from VIRTIO_NET_HDR_GSO_NONE, then the driver MUST
also set the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in flags and MUST set
gso_size to indicate the desired MSS."

These descriptions are there to make it clear to the reader what this 
feature does, OK,
if you think we don't need to mention this, I'll simplify the above you 
mentioned and here.

>
>> 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.
> I think we need to clarify:
>
> 1) when the packet has full checksum, we set DATA_VALID but not NEEDS_CSUM?

Yes.

> 2) when the packet has partial checksum, we set NEEDS_CSUM for sure,

"for sure" is when GUEST_CSUM is on. If GUEST_FULL_CSUM is on, 
NEEDS_CSUM will not be set.

> but a conditional DATA_VALID?

How about this:

1. When the packet has full checksum, the device sets DATA_VALID but not 
NEEDS_CSUM;
2. When the packet has partial checksum,
1) If GUEST_FULL_CSUM is on, the device calculates the full checksum and 
can set DATA_VALID.
2) Otherwise, NEEDS_CSUM will be set.

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] Re: [PATCH v3] virtio-net: support distinguishing between partial and full checksum

[Heng Qi]

在 2023/11/16 下午2:18, Michael S. Tsirkin 写道:
> On Tue, Nov 14, 2023 at 04:49:45PM +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>
>> ---
>> v2->v3:
>> - Add a section named "Driver Handles Fully Checksummed Packets"
>>    and more descriptions. @Michael
>>
>> v1->v2:
>> - Modify full checksum functionality as a configurable offload
>>    that is initially turned off. @Jason
>>
>>   device-types/net/description.tex        | 73 +++++++++++++++++++++++--
>>   device-types/net/device-conformance.tex |  1 +
>>   device-types/net/driver-conformance.tex |  1 +
>>   3 files changed, 69 insertions(+), 6 deletions(-)
>>
>> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
>> index aff5e08..6937a2f 100644
>> --- a/device-types/net/description.tex
>> +++ b/device-types/net/description.tex
>> @@ -122,6 +122,8 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
>>       device with the same MAC address.
>>   
>>   \item[VIRTIO_NET_F_SPEED_DUPLEX(63)] Device reports speed and duplex.
>> +
>> +\item[VIRTIO_NET_F_GUEST_FULL_CSUM (64)] Driver handles packets with full checksum.
>>   \end{description}
>>   
>>   \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device / Feature bits / Feature bit requirements}
>> @@ -136,6 +138,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.
>
>
> Apparently this is just to make the patch a bit smaller so you do not
> have to find all instances of VIRTIO_NET_F_GUEST_CSUM and replace them
> with "VIRTIO_NET_F_GUEST_CSUM or VIRTIO_NET_F_GUEST_FULL_CSUM".

Yes, but, additionally, describing for GUEST_FULL_CSUM what common 
things drivers and devices
should do regarding checksums would repeat many things for GUEST_CSUM.

>
> But, this is actually a problem : we have places in spec which only
> say VIRTIO_NET_F_GUEST_CSUM but actually mean "VIRTIO_NET_F_GUEST_CSUM
> negotiated and not disabled by VIRTIO_NET_F_CTRL_GUEST_OFFLOADS".

I don't think it's a problem.

When we describe features, we use "can". What it can do will not change, 
and we have made constraints for this in all places where GUEST_CSUM 
appears.

>
> So this just makes no sense to me. We have:
> \item[VIRTIO_NET_F_GUEST_CSUM (1)] Driver handles packets with partial checksum.

I always think this description is inappropriate, it should be
\item[VIRTIO_NET_F_GUEST_CSUM (1)] Driver handles packets with partial 
checksum and full checksum.

"with full checksum" should not be saved here. Otherwise it could be 
misunderstood that the driver can *only* handle packets with partial 
checksum.

The description of VIRTIO_NET_F_GUEST_CSUM in the full text of virtio 
spec is divided into two parts: NEEDS_CSUM and DATA_VALID.

Maybe we need an update for this description?

>
>
> and here apparently when you have driver that handles packets with partial checksum
> *and* packets with full checksum then this means that no, it does not
> handle packets with partial checksum.
> It might look ok when you just look at the patch but when people read
> the full spec this is just confusing.
>
>
>
> Let me try: in fact VIRTIO_NET_F_GUEST_CSUM now means "driver handles
> packets with checksum (partial or full)"?

Yes, That's what it's doing now.

> Even with no features checksum can cover
> all of the packet nothing prevents that.
>
> So now we have a new flag that means
> "driver can only handle fully checksummed packets".

Yes.

Thanks!

>
>
>
>
>
>
>> @@ -398,6 +401,58 @@ \subsection{Device Initialization}\label{sec:Device Types / Network Device / Dev
>>   A truly minimal driver would only accept VIRTIO_NET_F_MAC and ignore
>>   everything else.
>>   
>> +\subsubsection{Driver Handles Fully Checksummed Packets}\label{sec:Device Types / Network Device / Device Initialization / Driver Handles Fully Checksummed Packets}
>> +
>> +The VIRTIO_NET_F_GUEST_CSUM feature indicates that the driver can handle
>> +partially or fully checksummed packets from the device. When the
>> +driver only expects fully checksummed packets, the VIRTIO_NET_F_GUEST_FULL_CSUM
>> +feature can be negotiated if the device offers it.
>> +Then the driver only handles packets with full checksum.
>> +
>> +By negotiating the VIRTIO_NET_F_GUEST_FULL_CSUM feature, the driver can
>> +benefit, for example, from the device's ability to calculate and validate the checksum
>> +in scenarios where partially checksummed packets are not compatible.
>> +
>> +Delivering fully checksummed packets rather than partially
>> +checksummed packets incurs additional overhead for the device.
>> +As a result, receive full-checksum offload (meaning the driver only handles
>> +packets with full checksum) is disabled by default \ref{sec:Device Types / Network Device
>> +/ Device Operation / Control Virtqueue / Offloads State Configuration}.
>> +
>> +Receive full-checksum offload can be enabled if the driver successfully
>> +sends the VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET command with the
>> +VIRTIO_NET_F_GUEST_FULL_CSUM bit set.
> But how much is this "additional overhead" and how does driver know when
> this should be enabled as opposed to just disabling checksum offload
> completely?
>
>
>> +
>> +\drivernormative{\subsubsection}{Driver Handles Fully Checksummed Packets}{sec:Device Types / Network Device / Device Initialization / Driver Handles Fully Checksummed Packets}
>> +
>> +The driver MUST NOT enable receive full-checksum offload for which
>> +VIRTIO_NET_F_GUEST_FULL_CSUM has not been negotiated.
> I don't understand what this is saying.  We need to come up with a way
> to document this all without inventing terms like "full-checksum offload".
>
> IIUC all this does is basically require that checksum covers all of the
> packet.
>


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/

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

[Xuan Zhuo]

On Thu, 16 Nov 2023 01:18:07 -0500, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Tue, Nov 14, 2023 at 04:49:45PM +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>
> > ---
> > v2->v3:
> > - Add a section named "Driver Handles Fully Checksummed Packets"
> >   and more descriptions. @Michael
> >
> > v1->v2:
> > - Modify full checksum functionality as a configurable offload
> >   that is initially turned off. @Jason
> >
> >  device-types/net/description.tex        | 73 +++++++++++++++++++++++--
> >  device-types/net/device-conformance.tex |  1 +
> >  device-types/net/driver-conformance.tex |  1 +
> >  3 files changed, 69 insertions(+), 6 deletions(-)
> >
> > diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> > index aff5e08..6937a2f 100644
> > --- a/device-types/net/description.tex
> > +++ b/device-types/net/description.tex
> > @@ -122,6 +122,8 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
> >      device with the same MAC address.
> >
> >  \item[VIRTIO_NET_F_SPEED_DUPLEX(63)] Device reports speed and duplex.
> > +
> > +\item[VIRTIO_NET_F_GUEST_FULL_CSUM (64)] Driver handles packets with full checksum.
> >  \end{description}
> >
> >  \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device / Feature bits / Feature bit requirements}
> > @@ -136,6 +138,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.
>
>
>
> Apparently this is just to make the patch a bit smaller so you do not
> have to find all instances of VIRTIO_NET_F_GUEST_CSUM and replace them
> with "VIRTIO_NET_F_GUEST_CSUM or VIRTIO_NET_F_GUEST_FULL_CSUM".
>
> But, this is actually a problem : we have places in spec which only
> say VIRTIO_NET_F_GUEST_CSUM but actually mean "VIRTIO_NET_F_GUEST_CSUM
> negotiated and not disabled by VIRTIO_NET_F_CTRL_GUEST_OFFLOADS".
>
> So this just makes no sense to me. We have:
> \item[VIRTIO_NET_F_GUEST_CSUM (1)] Driver handles packets with partial checksum.
>
>
> and here apparently when you have driver that handles packets with partial checksum
> *and* packets with full checksum then this means that no, it does not
> handle packets with partial checksum.
> It might look ok when you just look at the patch but when people read
> the full spec this is just confusing.
>
>
>
> Let me try: in fact VIRTIO_NET_F_GUEST_CSUM now means "driver handles
> packets with checksum (partial or full)"? Even with no features checksum can cover
> all of the packet nothing prevents that.

YES, I agree. I think we should change the description of the
VIRTIO_NET_F_GUEST_CSUM. That confuses the reader.

>
> So now we have a new flag that means
> "driver can only handle fully checksummed packets".

If the description of the VIRTIO_NET_F_GUEST_CSUM is changed.
I think make the new flag as the sub set is a good way.
That will benefit the future modification.

Thanks.

>
>
>
>
>
>
> > @@ -398,6 +401,58 @@ \subsection{Device Initialization}\label{sec:Device Types / Network Device / Dev
> >  A truly minimal driver would only accept VIRTIO_NET_F_MAC and ignore
> >  everything else.
> >
> > +\subsubsection{Driver Handles Fully Checksummed Packets}\label{sec:Device Types / Network Device / Device Initialization / Driver Handles Fully Checksummed Packets}
> > +
> > +The VIRTIO_NET_F_GUEST_CSUM feature indicates that the driver can handle
> > +partially or fully checksummed packets from the device. When the
> > +driver only expects fully checksummed packets, the VIRTIO_NET_F_GUEST_FULL_CSUM
> > +feature can be negotiated if the device offers it.
> > +Then the driver only handles packets with full checksum.
> > +
> > +By negotiating the VIRTIO_NET_F_GUEST_FULL_CSUM feature, the driver can
> > +benefit, for example, from the device's ability to calculate and validate the checksum
> > +in scenarios where partially checksummed packets are not compatible.
> > +
> > +Delivering fully checksummed packets rather than partially
> > +checksummed packets incurs additional overhead for the device.
> > +As a result, receive full-checksum offload (meaning the driver only handles
> > +packets with full checksum) is disabled by default \ref{sec:Device Types / Network Device
> > +/ Device Operation / Control Virtqueue / Offloads State Configuration}.
> > +
> > +Receive full-checksum offload can be enabled if the driver successfully
> > +sends the VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET command with the
> > +VIRTIO_NET_F_GUEST_FULL_CSUM bit set.
>
> But how much is this "additional overhead" and how does driver know when
> this should be enabled as opposed to just disabling checksum offload
> completely?
>
>
> > +
> > +\drivernormative{\subsubsection}{Driver Handles Fully Checksummed Packets}{sec:Device Types / Network Device / Device Initialization / Driver Handles Fully Checksummed Packets}
> > +
> > +The driver MUST NOT enable receive full-checksum offload for which
> > +VIRTIO_NET_F_GUEST_FULL_CSUM has not been negotiated.
>
> I don't understand what this is saying.  We need to come up with a way
> to document this all without inventing terms like "full-checksum offload".
>
> IIUC all this does is basically require that checksum covers all of the
> packet.
>
> --
> 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/