Re: [PATCH] hw/net/net_tx_pkt: Fix virtio header without checksum offloading

[Akihiko Odaki <akihiko.odaki@daynix.com>]

On 2024/03/26 15:51, Jason Wang wrote:
> On Sun, Mar 24, 2024 at 4:32 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> It is incorrect to have the VIRTIO_NET_HDR_F_NEEDS_CSUM set when
>> checksum offloading is disabled so clear the bit. Set the
>> VIRTIO_NET_HDR_F_DATA_VALID bit instead to tell the checksum is valid.
>>
>> TCP/UDP checksum is usually offloaded when the peer requires virtio
>> headers because they can instruct the peer to compute checksum. However,
>> igb disables TX checksum offloading when a VF is enabled whether the
>> peer requires virtio headers because a transmitted packet can be routed
>> to it and it expects the packet has a proper checksum. Therefore, it
>> is necessary to have a correct virtio header even when checksum
>> offloading is disabled.
>>
>> A real TCP/UDP checksum will be computed and saved in the buffer when
>> checksum offloading is disabled. The virtio specification requires to
>> set the packet checksum stored in the buffer to the TCP/UDP pseudo
>> header when the VIRTIO_NET_HDR_F_NEEDS_CSUM bit is set so the bit must
>> be cleared in that case.
>>
>> The VIRTIO_NET_HDR_F_NEEDS_CSUM bit also tells to skip checksum
>> validation. Even if checksum offloading is disabled, it is desirable to
>> skip checksum validation because the checksum is always correct. Use the
>> VIRTIO_NET_HDR_F_DATA_VALID bit to claim the validity of the checksum.
>>
>> Fixes: ffbd2dbd8e64 ("e1000e: Perform software segmentation for loopback")
>> Buglink: https://issues.redhat.com/browse/RHEL-23067
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>>   hw/net/net_tx_pkt.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c
>> index 2e5f58b3c9cc..c225cf706513 100644
>> --- a/hw/net/net_tx_pkt.c
>> +++ b/hw/net/net_tx_pkt.c
>> @@ -833,6 +833,9 @@ bool net_tx_pkt_send_custom(struct NetTxPkt *pkt, bool offload,
>>
>>       if (offload || gso_type == VIRTIO_NET_HDR_GSO_NONE) {
>>           if (!offload && pkt->virt_hdr.flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
>> +            pkt->virt_hdr.flags =
>> +                (pkt->virt_hdr.flags & ~VIRTIO_NET_HDR_F_NEEDS_CSUM) |
>> +                VIRTIO_NET_HDR_F_DATA_VALID;
> 
> Why VIRTIO_NET_HDR_F_DATA_VALID is used in TX path?

On igb, a packet sent from a PCI function may be routed to another 
function. The virtio header updated here will be directly provided to 
the RX path in such a case.

Regards,
Akihiko Odaki

> 
> Thanks
> 
>>               net_tx_pkt_do_sw_csum(pkt, &pkt->vec[NET_TX_PKT_L2HDR_FRAG],
>>                                     pkt->payload_frags + NET_TX_PKT_PL_START_FRAG - 1,
>>                                     pkt->payload_len);
>>
>> ---
>> base-commit: ba49d760eb04630e7b15f423ebecf6c871b8f77b
>> change-id: 20240324-tx-c57d3c22ad73
>>
>> Best regards,
>> --
>> Akihiko Odaki <akihiko.odaki@daynix.com>
>>
>