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

[Heng Qi <hengqi@linux.alibaba.com>]

å 2024/1/3 äå2:35, Heng Qi åé:
> 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

Hi Michael, Cornelia.

Could you please open a voting for this fix?

Thanks a lot!

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