[virtio-dev] [PATCH v8] virtio-net: correct conditions for devices to validate packet checksum

* [virtio-dev] [PATCH v8] virtio-net: correct conditions for devices to validate packet checksum
@ 2024-01-03  6:35 ` Heng Qi
  0 siblings, 0 replies; 18+ messages in thread
From: Heng Qi @ 2024-01-03  6:35 UTC (permalink / raw)
  To: virtio-comment, virtio-dev
  Cc: Jason Wang, Michael S. Tsirkin, Yuri Benditovich, Xuan Zhuo,
	Cornelia Huck

There is a historical error in virtio spec:
  "If VIRTIO_NET_F_GUEST_CSUM is not negotiated, the device *MUST* set flags
   to zero and SHOULD supply a fully checksummed packet to the driver."

Currently in Linux and virtio-related implementations, the device validates
the packet checksum and sets DATA_VALID regardless of whether
VIRTIO_NET_F_GUEST_CSUM is negotiated.

Please refer to the following summary and thread[1] for details and reasons:

Summary
=============
1. GUEST_CSUM at virtio spec 0.95 is intended to be compatible with partially
checksummed packets (NEEDS_CSUM <-> CHECKSUM_PARTIAL). So GUEST_CSUM is mapped
to NETIF_F_RXCSUM.
GUEST_CSUM only indicates whether the driver handles partially checksummed packets.
When XDP is loaded, the GUEST_CSUM's offload will be disabled, which means that
packets have NEEDS_CSUM set will be dropped, and packets have DATA_VALID set will
still be received.

2. When DATA_VALID was added to Linux in 2011[2] and virtio1.0, it was actually
expected that rx checksum offload (the driver can set CHECKSUM_UNNECESSARY) had
nothing to do with whether GUEST_CSUM was negotiated. But due to an error, below
desctiption was added incorrectly:
  "If VIRTIO_NET_F_GUEST_CSUM is not negotiated, the device *MUST* set flags
   to zero and SHOULD supply a fully checksummed packet to the driver."

3. We now hope to correct this error. Let the setting of DATA_VALID not be
controlled by whether GUEST_CSUM is negotiated, but only controlled by whether
rx checksum offload is enabled on the OS side. The state of this rx checksum
offload is not aware of the device.

4. [Optional] NETIF_F_RXCSUM corresponding to rx checksum offload should be
added to dev->hw_features. When the user turns off rx checksum offload through
ethtool -K, neither NEEDS_CSUM nor DATA_VALID should be taken care of, that is,
all packets will be marked as CHECKSUM_NONE.

Related Link
=============
[1] https://lists.oasis-open.org/archives/virtio-comment/202312/msg00135.html
[2] 10a8d94a9574 ("virtio_net: introduce VIRTIO_NET_HDR_F_DATA_VALID")

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

Suggested-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
---
v7->v8:
- Drop the new FULLY_CSUM feature and optimize the description. @Jason

v6->v7:
- FULLY_CSUM no longer depends on GUEST_CSUM.
- The description related to DATA_VALID has nothing to do with
  whether GUEST_CSUM is negotiated or not. @Jason

v5->v6:
- Rewrite and clarify patch description. @Michael

v4->v5:
- Remove the modification to the GUEST_CSUM. @Jason
- The description of this feature has been reorganized for greater clarity. @Michael

v3->v4:
- Streamline some repetitive descriptions. @Jason
- Add how features should work, when to be enabled, and overhead. @Jason @Michael

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 | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index aff5e08..14978c0 100644
--- a/device-types/net/description.tex
+++ b/device-types/net/description.tex
@@ -723,8 +723,7 @@ \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
-  VIRTIO_NET_HDR_F_DATA_VALID bit in \field{flags} can be
+\item 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
   has been validated.
@@ -783,9 +782,8 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
 packet together, such that at least \field{num_buffers} are
 observed by driver as used.
 
-If VIRTIO_NET_F_GUEST_CSUM is not negotiated, the device MUST set
-\field{flags} to zero and SHOULD supply a fully checksummed
-packet to the driver.
+If VIRTIO_NET_F_GUEST_CSUM is not negotiated, the device SHOULD supply
+a fully checksummed packet to the driver.
 
 If VIRTIO_NET_F_GUEST_TSO4 is not negotiated, the device MUST NOT set
 \field{gso_type} to VIRTIO_NET_HDR_GSO_TCPV4.
@@ -842,12 +840,6 @@ \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
-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
-of checksums is validated).
-
 \drivernormative{\paragraph}{Processing of Incoming
 Packets}{Device Types / Network Device / Device Operation /
 Processing of Incoming Packets}
-- 
1.8.3.1


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply related	[flat|nested] 18+ messages in thread