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

[Akihiko Odaki]

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>