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

[Jason Wang <jasowang@redhat.com>]

On Tue, Mar 26, 2024 at 3:04 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> 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.

But I meant for example net_tx_pkt_send_custom() is used in
e1000e_tx_pkt_send() which is the tx path on the host.

VIRTIO_NET_HDR_F_DATA_VALID is not necessary in the tx path.

Thanks

>
> 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>
> >>
> >
>