In D51688#1381752, @cc wrote:

In D51688#1381454, @cc wrote:

In D51688#1381014, @timo.voelker_fh-muenster.de wrote:

The throughput is about 60 Mbit/s higher with transmit checksum offloading (txcsum). That's something. I expect a bigger win with TSO and checksum offloading is a requirement for TSO.

How to enable TSO in the vtnet0 of VM? I can enable TSO in the tap0 of the host. But didn't find out TSO enabled in the VM. And the VM sysctl dev.vtnet.0.txq0 below shows TSO not used in the test traffic.

In D51688#1378630, @cc wrote:

In your test, will this checksum offloading save some CPU cycles. If so, is there any benchmark showing that stats?

Looks like code improvements to me. I also don't see a functional change.

vtnet defined the receive-side offload functions only if INET or INET6 were defined. While I copied this to virtio_net.h, I missed checking if INET or INET6 was defined when calling one of these functions from tuntap.

Changes how virtio_net.h does byte conversion. Instead of using virtio_gtoh16, which is unintuitive when called from the host, it uses the new macro VIRTIO_NET_TX_MODERN_LE.

Rebased the patch on the current FreeBSD main branch.

In D57988#1330151, @glebius wrote:

I still can't understand :(

It is not the loopback that does checksum validation. It has never validated the checksum.
Before my change (D57518), loopback lied. For each packet, loopback just indicated that it has validated the checksum and that the checksum is correct. In consequence, IP skipped checksum validation.
In general, this kind of lying for a packet received is problematic. If the packet is not for the host and the host sends it out (e.g., due to IP routing), the packet goes on the wire without a valid checksum.

How is it possible that a packet received on loopback would go outside? I can imagine only policy routing by ipfw/pf.

After applying this patch, the ip_reass_test tests pass on my system.

In D57988#1329787, @glebius wrote:

Thanks! Stupid question: how did that happen that after Timo's change the loopback actually does check checksums?

It seems this bug shows only after D57518 because, before that change, the loopback interface with IFCAP_RXCSUM enabled just told IP for each packet that the checksum has been checked and is valid. After that change, the loopback interface does not pretend that anymore, and IP validates the checksum.

In D55915#1324504, @jhb wrote:

Yes, this looks good to me. I'm about to land my style fixes change and can land this one next. Do you have a git commit I can cherry-pick?

In D57392#1323920, @jrtc27 wrote:

The change itself looks fine but I think the commit message needs rewriting. It is quite verbose on irrelevant details (like how bus_(space_)* foo work under the hood), and talks about data on the PCI bus (which is implementation, not architecture). Also the bug reference should follow the proper format (see tools/tools/git/hooks/prepare-commit-msg). Honestly the commit message would be better if it was entirely replaced with something like:

virtio_pci_modern: Remove endianness conversion for config space

The bus_* functions already handle converting from PCI endianness (i.e.
little-endian) to native endianness when accessing the config space (see
ofw_pcib_bus_get_bus_tag), so converting again with virtio_htogX/virtio_gtohX
undoes any byte-swapping and breaks big-endian systems. They should only be
used for operating on shared memory.

Note part of this reverts commit fb53b42e36a9 ("virtio-modern: fix PCI
common read/write functions on big endian targets").

PR		294706
Fixes:		fb53b42e36a9 ("virtio-modern: fix PCI common read/write functions on big endian targets")
Fixes:		9da9560c4dd3 ("virtio: Add VirtIO PCI modern (V1) support")
MFC after:	1 week

@jhb's comments addressed.

@pouria, thanks for the clarifications. Your inline comments in if_offload.c and if_offload.h disappeared after I renamed them to if_tx_offload.c and if_tx_offload.h. I created new inline comments and quoted your text.

Addresses @pouria's comments.

Plan changed. Instead of computing the checksum in the bridge, do it right before the driver gets the packet: D56564

Thanks for your review!

In D57392#1315045, @adrian wrote:

Neat! So hm what devices are on the virtio pci bus? storage, net, anything else? I'm just trying to imagine the device testing map for this change.

Thanks for chasing this down!

Call virtio_net_rx_csum() only if VIRTIO_NET_HDR_F_NEEDS_CSUM or VIRTIO_NET_HDR_F_DATA_VALID is set.

This patch was needed for D51688, but D51688 has changed and now uses the fact that the capabilities are added when a vnet_hdr_len > 0 is set, which obsoletes this patch.

Update the vnet_hdr_len in tap_set_cap().

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.