Addresses @bcr's comment.

To check the impact of IPv4 header checksum, I ran tests.

Offloading of IP header and TCP/UDP checksum need to work independently. This solves a bug.

Addresses @kp's comment.

Leave the lines that initialize ip_sum with 0, which some NICs require.

Change option syntax from -Bw to -W, add -R, and revise the code.

In D56564#1296307, @zlei wrote:

I'd suggest to add links to the discussing about defer calculating the checksum. I believe I saw that before, but I can not find it now.

Addresses @gallatin's comments.

Addresses @gallatin's comment.

In D51289#1292473, @tuexen wrote:

In D51289#1292049, @pouria wrote:

Since @markj accepted this revision too. I'll commit this one.

Let me double check with Timo. This is part of a series and I am not sure about that state.

In D55915#1281289, @kib wrote:

In D55915#1281101, @timo.voelker_fh-muenster.de wrote:

In D55915#1280245, @kib wrote:

You only write a single value into the specified offset, right?
This should be clearly explained in the man page addition.
Or consider adding a possibility of writing the series of values starting at the given offset, of the specified width for each value (IMO it is fine as is, but needs to be properly documented).

In my use case, I only had to write a single value. Do you know use cases where a series of values needs to be written to a BAR region?

I tried to make it similar to -w (write to config space). This is the man page part about -w:

The -w option writes the value into a configuration space register at byte offset addr of device selector.

Do you find this more clear?

Yes

In D55915#1280245, @kib wrote:

You only write a single value into the specified offset, right?
This should be clearly explained in the man page addition.
Or consider adding a possibility of writing the series of values starting at the given offset, of the specified width for each value (IMO it is fine as is, but needs to be properly documented).

In D55915#1279706, @jhb wrote:

I think rather than trying to overload -B, it would be better to just use a different getopt() letter for this mode. I would suggest using -R for reading from a BAR but keep 'D' as an alias for backwards compatibility, and using -W for writing into a BAR.

Why changing hw.virtio.pci.transitional to 1 by default:

Addressed comments from @tuexen and @ziaee.

In D55534#1270843, @tuexen wrote:

Does this patch solve any problem?

Looks good to me. It improves readability, in my opinion. Also, it eliminates unnecessary m_freem calls with a null pointer.

I agree on @pouria's comment.

Solves this bug:

Allow enabling/disabling the new capabilities: IFCAP_TXCSUM, IFCAP_TXCSUM_IPV6, and IFCAP_VLAN_HWCSUM.

Use CSUM_BITS string defined in sys/mbuf.h for output.

In D52465#1257581, @zarychtam_plan-b.pwste.edu.pl wrote:

Please bear in mind that the above change, as well as "epair: add support for checksum offloading" (https://reviews.freebsd.org/D51639) are incompatible with netmap(4). If the epair(4) interface is used in netmap mode, communication is broken. It never happens on physical interfaces with netmap support, like for example ix(4) where these NIC capabilities don't need to be disabled.

In D54948#1257019, @afedorov wrote:

Maybe someone remembers this project: https://lists.freebsd.org/pipermail/freebsd-current/2014-September/052226.html

Addressed @tuxen's style comments.

Without this patch, transmit checksum offloading for TCP/IPv6 and UDP/IPv6 is never used and transmit checksum offloading for IPv4, TCP/IPv4 and UDP/IPv4 is always used, even if disabled via ifconfig ue? -txcsum.

I see the code of this driver the first time, but incrementing IFCOUNTER_OERRORS everywhere where tx_dropped is incremented makes sense. The positions of the lines where the other IFCOUNTER_* counters are incremented seems correct to me as well.

Based on Dmitry's explanation

Seems to improve the code. Less lines and it looks less complex. This changes the functionality only if IFCAP_RXCSUM and IFCAP_RXCSUM_IPV6 or IFCAP_TXCSUM and IFCAP_TXCSUM_IPV6 get out of sync but, in my understanding of the code, this cannot happen.

Looks good to me. I see no functional change but less lines of code and a more consistent way to change capenable and hwassist.

Looks good to me.

Looks good. It makes sense to add TXCSUM_IPV6 since the interface supports it.

To address @tuexen's and @p.mousavizadeh_protonmail.com's comments, I restrict the code changes in this review to the changes required to process incoming packets with IP checksum offloading and leave code improvements for another review.

The question is, what means TXCSUM?

I agree @markj. It is not easy to understand the code. However, before this patch, without any other options, optlen is 0 and, thus, the jumbo option will never be added.

The different checksum offloading features should work independently.

Use the same semantic for cklen as in the UDP case.

Enabling TSO without TXCSUM is not good and should not be possible.

It's strange that the spec does not allow the driver to set the VIRTIO_NET_HDR_F_DATA_VALID flag. If the host receives a packet over a physical interface that validates the checksum, forwards that packet to a VM, and the VM sends it out again (e. g., because of routing), we now lose the information that the checksum has already been validated. However, the spec appears to be clear on that, and I guess FreeBSD should follow it.

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