vtnet: fix translation between mbuf and virtio flags

[timo.voelker_fh-muenster.de]

Summary

If using the virtio network channel, transferring packets between a host and a VM requires translating the packet's meta information between the fields in the virtio network header and, in the case of FreeBSD, the fields in the mbuf. On the VM side, this is done by the vtnet interface driver. The vtnet driver does the translation in two ways less than good:

1. When sending a packet over the virtio network channel, it does not translate the CSUM_DATA_VALID | CSUM_PSEUDO_HDR mbuf flags in VIRTIO_NET_HDR_F_DATA_VALID.

2. When receiving a packet over the virtio network channel, it translates VIRTIO_NET_HDR_F_NEEDS_CSUM in the CSUM_DATA_VALID | CSUM_PSEUDO_HDR mbuf flags.

This patch adds for the vtnet driver the translation 1. and changes the translation 2. of VIRTIO_NET_HDR_F_NEEDS_CSUM in the mbuf transmission checksum offload flag (CSUM_TCP, CSUM_TCP_IPV6, CSUM_UDP, or CSUM_UDP_IPV6).

Note that D51475 added the handling of an incoming packet with a transmission checksum offload flag set.

Test Plan

1. Run a FreeBSD VM on a KVM/qemu hypervisor. Other than with bhyve, transmission checksum offloading is enabled by default.
2. Send a TCP packet from the host to the FreeBSD VM. Due to checksum offloading, the TCP header does not contain a valid checksum.
3. Have the FreeBSD configured to send that packet out over another interface. Either by using a bridge or IP routing.

Before the patch: The FreeBSD VM translates VIRTIO_NET_HDR_F_NEEDS_CSUM in CSUM_DATA_VALID | CSUM_PSEUDO_HDR when receiving the packet from the host. The destination will drop the packet as it still has an invalid checksum. Note that this is the cause of the bug reported in https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=165059.

After the patch: The transmission checksum offload flags are preserved and the checksum will be computing on transmission (either by the hardware or by software if not supported by the hardware). This solves the reported bug.

[tuexen]

sys/dev/virtio/network/virtio_net.h

264 ↗

(On Diff #159592)

You can use bool here and later on true and false.

311 ↗

(On Diff #159592)

Would it make sense to put the return (0); before the #endif and add an #else with return(1)?

[timo.voelker_fh-muenster.de]

Changed type of variable isipv6 from int to bool.

[timo.voelker_fh-muenster.de]

sys/dev/virtio/network/virtio_net.h

264 ↗

(On Diff #159592)

That's fancy. Done.

311 ↗

(On Diff #159592)

I don't know. I chose the way with less code lines.

[tuexen]

sys/dev/virtio/network/virtio_net.h

311 ↗

(On Diff #159592)

I don't know. I chose the way with less code lines.

If neither INET nor INET6 is defined, doesn't the function now returns 0? I would expect 1 for errors.

[timo.voelker_fh-muenster.de]

virtio_net_rx_csum_by_offset now returns (1) in case INET and INET6 are not defined (as suggested by @tuexen)

[timo.voelker_fh-muenster.de]

sys/dev/virtio/network/virtio_net.h

311 ↗

(On Diff #159592)

This function returns 1 before the patch in that caste. To keep the functional changes small, I changed the code as suggested.

[tuexen]

sys/dev/virtio/network/if_vtnet.c

33

Since this is a general fix, I took it out in D52046.

[timo.voelker_fh-muenster.de]

To focus on the guest-side in this review, I removed the tuntap related code and extended the vtnet code instead.

[timo.voelker_fh-muenster.de]

sys/dev/virtio/network/if_vtnet.c

33

Thanks! Done by rebase.

[timo.voelker_fh-muenster.de]

Addressing comments from a chat with @tuexen.

• Restructure code in vtnet_rxq_csum, vtnet_rxq_csum_data_valid, and vtnet_rxq_csum_needs_csum. The basic idea is that vtnet_rxq_csum checks for errors before calling vtnet_rxq_csum_data_valid or vtnet_rxq_csum_needs_csum.
• Rename one of the unused counters in rx_csum_inaccessible_ipproto, which gives a better indication of the error than rx_csum_bad_ipproto.
• Update the description of the counters in the vtnet man page.

[tuexen]

share/man/man4/vtnet.4

156–163

The number of times a packet with a request for receive or transmit checksum offload was received and this request failed.
The different reasons for the failure are counted by
.Va dev.vtnet. Ns Ar X Ns Va .rx_csum_inaccessible_ipproto ,
.Va dev.vtnet. Ns Ar X Ns Va .rx_csum_bad_ipproto ,
.Va dev.vtnet. Ns Ar X Ns Va .rx_csum_bad_ethtype ,
or
.Va dev.vtnet. Ns Ar X Ns Va .rx_csum_bad_offset .

225

The number of times a packet with a request for receive or transmit checksum offload was received where the IP protocol was not accessible.

226

The number of times fixing the checksum required by
.Va hw.vtnet.fixup_needs_csum
or
.Va hw.vtnet. Ns Ar X Ns Va .fixup_needs_csum
was attempted for a packet where the csum is not located in the first mbuf.

227

The number of times a packet with a request for receive or transmit checksum offload was received where the IP protocol was neither TCP nor UDP.

232

The number of times a packet with a request for receive or transmit checksum offload was received where the EtherType was neither IPv4 nor IPv6.

sys/dev/virtio/network/if_vtnet.c

1810

Shouldn't we also increment some error counter here? Why not use rx_csum_bad_offset?

1854

Use %x for flags.

1880

Only declare variables at the beginning of a block.

1898

Comment should be a sentence. So start with a capital letter and end with a full stop.

[tuexen]

share/man/man4/vtnet.4

156–163

Use checksum offloading instead of checksum offload, also below.
Finally use and instead of or.

[timo.voelker_fh-muenster.de]

I rebased my changes on the current state of FreeBSD head and addressed the comments from @tuexen.

[bcr]

OK for the man page changes.