Re: [PATCH v7 1/2] virtio-net: correct conditions for devices to validate packet checksum

[Jason Wang <jasowang@redhat.com>]

On Thu, Dec 28, 2023 at 3:46âPM Heng Qi <hengqi@linux.alibaba.com> wrote:
>
> 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 offload of GUEST_CSUM will be disabled, which means that
> packets have NEEDS_CSUM set will be dropped, but packets have DATA_VALID set will
> still be received because NETIF_F_RXCSUM still exists.
>
> 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 also not aware of the device.
>
> 4 NETIF_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 CHECKSUM_NONE.
>
> 5. GUEST_FULLY_CSUM is added to disable the offload of GUEST_CSUM:
> When a packet have NEEDS_CSUM set is received, it is either dropped or a fully checksummed
> packet is calculated.
> When the corresponding offload of the GUEST_FULLY_CSUM is disabled, it is as if only
> GUEST_CSUM was negotiated.
>
> [1] https://lists.oasis-open.org/archives/virtio-comment/202312/msg00135.html
> [2] 10a8d94a9574 ("virtio_net: introduce VIRTIO_NET_HDR_F_DATA_VALID")
>
> Suggested-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> ---
>  device-types/net/description.tex | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> index aff5e08..a5210f2 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,7 +782,7 @@ \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
> +If VIRTIO_NET_F_GUEST_CSUM is not negotiated, the device MAY set
>  \field{flags} to zero and SHOULD supply a fully checksummed
>  packet to the driver.

I think it would more clearer if we just drop the flags part:

"
If VIRTIO_NET_F_GUEST_CSUM is not negotiated, the device SHOULD supply
a fully checksummed packet to the driver.
"

>
> @@ -842,8 +841,7 @@ \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
> +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).

And drop this part.

Thanks

> --
> 1.8.3.1
>