Re: [PATCH] net/virtio: include ipv4 cksum to support cksum offload capability

[David Marchand <david.marchand@redhat.com>]

Adding Olivier who, iirc, worked on those offloads a long time ago.

On Fri, Jan 7, 2022 at 12:54 PM Harold Huang <baymaxhuang@gmail.com> wrote:
>
> Device cksum offload capability usually include ipv4 cksum, tcp and udp
> cksum offload capability. The application such as OVS usually negotiate
> with the drive like this to enable cksum offload.

OVS checksum offloading is still being worked on.
By this, I mean neither that it is fully functional nor completely broken.
But important to keep in mind that it is still a work in progress.


>
> Signed-off-by: Harold Huang <baymaxhuang@gmail.com>
> ---
>  drivers/net/virtio/virtio_ethdev.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index c2588369b2..65b03bf0e4 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -3041,6 +3041,7 @@ virtio_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
>                 dev_info->rx_offload_capa |= RTE_ETH_RX_OFFLOAD_SCATTER;
>         if (host_features & (1ULL << VIRTIO_NET_F_GUEST_CSUM)) {
>                 dev_info->rx_offload_capa |=
> +                       RTE_ETH_RX_OFFLOAD_IPV4_CKSUM |
>                         RTE_ETH_RX_OFFLOAD_TCP_CKSUM |
>                         RTE_ETH_RX_OFFLOAD_UDP_CKSUM;
>         }

Adding this capability alone probably has no effect visible to a dpdk
application.

For L3, virtio rx handlers never sets anything but
RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN.
https://git.dpdk.org/dpdk/tree/drivers/net/virtio/virtio_rxtx.c#n920

As a consequence, the application can't tell if the ip checksum is
correct and the application must validate the checksum of packets on
its own.
Maybe we could add some RTE_MBUF_F_RX_IP_CKSUM_GOOD in case of TCP/UDP (?)
https://git.dpdk.org/dpdk/tree/drivers/net/virtio/virtio_rxtx.c#n932


But simply reporting the capability looks wrong to me.


> @@ -3055,6 +3056,7 @@ virtio_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
>                                     RTE_ETH_TX_OFFLOAD_VLAN_INSERT;
>         if (host_features & (1ULL << VIRTIO_NET_F_CSUM)) {
>                 dev_info->tx_offload_capa |=
> +                       RTE_ETH_TX_OFFLOAD_IPV4_CKSUM |
>                         RTE_ETH_TX_OFFLOAD_UDP_CKSUM |
>                         RTE_ETH_TX_OFFLOAD_TCP_CKSUM;
>         }

For the tx part, I am a bit unsure, but here is what I understand.


With such a capability, the application may think it can send the
packet with no checksum, neither ip, nor tcp.
The virtio specification writes that the driver must make sure the ip
checksum is correct if it wants to ask for L4 checksum.

"""
If the VIRTIO_NET_F_CSUM feature has been negotiated, the driver MAY
set the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in flags, if so:

the driver MUST validate the packet checksum at offset csum_offset
from csum_start as well as all preceding offsets;
the driver MUST set the packet checksum stored in the buffer to the
TCP/UDP pseudo header;
the driver MUST set csum_start and csum_offset such that calculating a
ones’ complement checksum from csum_start up until the end of the
packet and storing the result at offset csum_offset from csum_start
will result in a fully checksummed packet;
"""

But, nothing in the virtio tx handlers deals with ip cheksum offloading.
https://git.dpdk.org/dpdk/tree/drivers/net/virtio/virtqueue.h#n664

So here too, I think it is wrong to report such a capability.


-- 
David Marchand