I did some tests to understand the consequence of disabling "hardware LRO".

Right, I remember disabling txcsum, which didn't take affect, because hwassist wasn't updated. The patch seems to solve this issue. Thanks!

Removing the ifdefs and allow receive checksum offloading for IPv6 is a good thing. Also, enabling/disabling receive checksum pairwise for IPv4 and IPv6 make sense for VirtIO.

Addressed comment from @tuexen

The suggested changes in the summary make sense to me and the changes in sys/conf/files seem to correctly correspond to them. Looks all good to me. However, I haven't touched this file and don't know enough to review it.

In D52465#1198897, @zlei wrote:

The code looks good to me.

Well I think the document should be revised a little. Indeed for inbound traffic, if the mbuf has flag M_VLANTAG, the epair(4) end should accept but not try to discard or trim the flag, since the input path of the driver does nothing.

Also the net stack would expect the driver to do vlan header extracting, but current implementation does *NOTHING* but just pass in the mbuf as is. So IFCAP_VLAN_HWTAGGING for the input patch is more false capability.

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

Addressing comments from a chat with @tuexen.

In D51289#1194063, @p.mousavizadeh_protonmail.com wrote:

In D51289#1179499, @timo.voelker_fh-muenster.de wrote:

I don't know what that is. I can't find PRIV_NET_SETIFCAP in tuntap.c.

AFAIK, tunioctl is not called after ifhwioctl, however, I don't think it's necessary, since other cases of tunioctl are also lack privilege check.

For SIOCSIFCAP, check if requested capabilities are supported before enabling them.

Addressing @tuexen's comment by adding a sentence in the man page that explains the sync of TXCSUM and TXCSUM6.

I tested this patch with D51686 applied by looking at the csum counters per queue and the general rx_csum_offloaded counter with sysctl dev.vtnet. With nc, I sent TCP packets from a Linux host to a FreeBSD VM that uses virtio with multiple queues.

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

Addressing comments from @bcr

My only concern now is the need for the LOCK mechanism itself.

Addressing comments

Why not adding these includes in virtio_net.h?

In D21315#1188204, @vmaffione wrote:

Good, but how did you address the problem I pointed out in 2019, as explained above?
Is that not an issue anymore?

In D51291#1184658, @tuexen wrote:

@timo.voelker_fh-muenster.de Is it intended that this patch now contains the changes from D51289?

Removed the changes that are in review D51289.

Addressed @kevans's comment regarding using a lock for SIOCGIFCAP.

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

Changed type of variable isipv6 from int to bool.

I worked on a similar problem. I somehow found this related review afterwards. My code changes are in review D51688.

No functional changes. Just modified comments and code style.

For SIOCSIFCAP, ifr->ifr_reqcap is assigned directly to ifp->if_capenable instead of masking it with if_capabilities. The man page of ifnet told me that the masking is done earlier in the ioctl call chain.

My general concern here is that we could put ourselves into a weird position if we start disabling capabilities that we'd expect if tun_vhdrlen != 0 and that it'd be better to force TAPSVNETHDR to disable it instead

Currently, I don't need SIOCSIFCAP. I can just remove the code if that resolves concerns.

I think I'm OK either way- you're preserving an existing problem at most, and it's not even a major problem.

In D51639#1179521, @tuexen wrote:

Should epair also provide support for IFCAP_RXCSUM and IFCAP_RXCSUM_IPV6? I guess it will deliver packets with the corresponding mbuf header flags already...

Thanks for your review @markj

Thanks for your review @kevans

• No code changes.
• Removed epair code and documentation (will be added with a separate commit)
• Addressed @tuexen comments on documentation in mbuf man page and mbuf.h.

Did a rebase after the change of the mbuf man page via D51590.

The description from sysctl -d is probably correct. Making the man page more complete is a good thing.

In D51475#1178852, @rgrimes wrote:

Are we sure about the predict_false's being added into a file which appears to not have any predict in it? I can see that a system with assist hardware and lots of VM's this _false would actually be a pessimization as almost all packets are going to be marked with offloading capabilities. Perhaps some dtrace data might answer this.

It improves the documentation because it describes more of what is implemented.

Addressing comments from @tuexen I got from a chat. I touched only the code I added before.

• Removed the tap changes to focus this revision more on the handling of incoming packets with checksum offloading flags.
• Added code to handle SCTP packets.
• Added documentation in mbuf man page and mbuf.h.
• Extended changes in epair to be able to get and set interface capabilities.

In D51475#1175479, @kp wrote:

In D51475#1175460, @timo.voelker_fh-muenster.de wrote:

In D51475#1175448, @kp wrote:

How is CSUM_IP6_UDP different from CSUM_DATA_VALID_IPV6 now?

CSUM_DATA_VALID_IPV6 is the IPv6 version of CSUM_DATA_VALID. The mbuf man page says:

CSUM_DATA_VALID  The checksum of the data portion of the IP packet
                 has been computed and stored in the field
                 csum_data in network byte order.

With this patch, CSUM_IP6_UDP means for the input path that the checksum is unnecessary. It does not imply that the checksum has been computed. With CSUM_IP6_UDP the csum_data field instead contains the offset of the checksum field, which is important in case the packet somehow leaves the host.

But we're actively using CSUM_IP6_UDP on the input side now.

In D51475#1175448, @kp wrote:

How is CSUM_IP6_UDP different from CSUM_DATA_VALID_IPV6 now?

Thanks again @ivy for your valuable comments. As a result, I changed my approach (see D51475).

In D51298#1172514, @ivy wrote:

what i mean is if the packet is being routed (by the IP stack, not the bridge), for example if the bridge interface’s IP address is the default router for the VMs in the bridge.

in that case you’ve set the mbuf flag even though the checksum is not valid, but the checksum still needs to be calculated or at least the outgoing interface needs to be told to do the hw checksum.

is there a mechanism for that to happen?

In D51298#1172401, @ivy wrote:

what happens if a packet is received by the bridge interface, and is then routed via another interface - will the outgoing checksum be calculated if required?

Addresses @ivy's comment.