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

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

[Harold Huang]

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.

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;
 	}
@@ -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;
 	}
-- 
2.27.0

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

[Maxime Coquelin]

Hi Harold,

On 1/7/22 12:53, Harold Huang 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.
> 
> 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;
>   	}
> @@ -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;
>   	}

I'm not sure to understand why this is needed, as Vhost lib will always
ensure the IP csum has been calculated. Could you please elaborate?

Thanks,
Maxime

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

[Harold Huang]

Hi, Maxime,

Maxime Coquelin <maxime.coquelin@redhat.com> 于2022年1月31日周一 18:04写道：
>
> Hi Harold,
>
> On 1/7/22 12:53, Harold Huang 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.
> >
> > 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;
> >       }
> > @@ -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;
> >       }
>
> I'm not sure to understand why this is needed, as Vhost lib will always
> ensure the IP csum has been calculated. Could you please elaborate?

Thanks for your comments. Previously I want to enable tx checksum
offload for the tap device when I use DPDK virtio-user driver with
OVS. OVS assume checksum offload capability includes tcp, udp and ipv4
checksum offload:
https://github.com/openvswitch/ovs/blob/master/lib/netdev-dpdk.c#L1097.
But  AFAIK,  ipv4 checksum has always been calculated in the kernel.
And according to the virito-spec
(https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.html),
VIRTIO_NET_F_GUEST_CSUM feature bit may only indicate TCP or UDP
checksum offload, right? Maybe I need to change OVS to adapt to
virtio-user driver to enable checksum offload.

>
> Thanks,
> Maxime
>

Thanks,
Harold

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

[David Marchand]

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