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

[Jason Wang <jasowang@redhat.com>]

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