| Message ID | 20240324-tx-v1-1-a3b4135749ec@daynix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/net/net_tx_pkt: Fix virtio header without checksum offloading | expand |
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? 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> >
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> >> >
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> > >> > > >
On 2024/03/27 11:50, Jason Wang wrote: > 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. igb passes igb_tx_pkt_vmdq_callback to net_tx_pkt_send_custom(). igb_tx_pkt_vmdq_callback() passes the packet to its rx path for loopback. Regards, Akihiko Odaki > > 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> >>>> >>> >> >
On Wed, Mar 27, 2024 at 10:53 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > On 2024/03/27 11:50, Jason Wang wrote: > > 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. > > igb passes igb_tx_pkt_vmdq_callback to net_tx_pkt_send_custom(). > igb_tx_pkt_vmdq_callback() passes the packet to its rx path for loopback. > You are right, how about igb_tx_pkt_vmdq_callback()? We probably need to tweak the name if it is only used in rx path. Thanks > Regards, > Akihiko Odaki > > > > > 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> > >>>> > >>> > >> > > >
On 2024/03/27 11:59, Jason Wang wrote: > On Wed, Mar 27, 2024 at 10:53 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: >> >> On 2024/03/27 11:50, Jason Wang wrote: >>> 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. >> >> igb passes igb_tx_pkt_vmdq_callback to net_tx_pkt_send_custom(). >> igb_tx_pkt_vmdq_callback() passes the packet to its rx path for loopback. >> > > You are right, how about igb_tx_pkt_vmdq_callback()? > > We probably need to tweak the name if it is only used in rx path. igb_tx_pkt_vmdq_callback() itself is part of the tx path of a PCI function, and invokes the rx path of another PCI function in case of loopback, or triggers the transmission to the external peer. Regards, Akihiko Odaki > > Thanks > >> Regards, >> Akihiko Odaki >> >>> >>> 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> >>>>>> >>>>> >>>> >>> >> >
On Wed, Mar 27, 2024 at 11:05 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > On 2024/03/27 11:59, Jason Wang wrote: > > On Wed, Mar 27, 2024 at 10:53 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > >> > >> On 2024/03/27 11:50, Jason Wang wrote: > >>> 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. > >> > >> igb passes igb_tx_pkt_vmdq_callback to net_tx_pkt_send_custom(). > >> igb_tx_pkt_vmdq_callback() passes the packet to its rx path for loopback. > >> > > > > You are right, how about igb_tx_pkt_vmdq_callback()? > > > > We probably need to tweak the name if it is only used in rx path. > > igb_tx_pkt_vmdq_callback() itself is part of the tx path of a PCI > function, and invokes the rx path of another PCI function in case of > loopback, or triggers the transmission to the external peer. Right, so if it's an external TX, VIRTIO_NET_HDR_F_DATA_VALID may not work there. Thanks > > Regards, > Akihiko Odaki > > > > > Thanks > > > >> Regards, > >> Akihiko Odaki > >> > >>> > >>> 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> > >>>>>> > >>>>> > >>>> > >>> > >> > > >
On 2024/03/27 12:06, Jason Wang wrote: > On Wed, Mar 27, 2024 at 11:05 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: >> >> On 2024/03/27 11:59, Jason Wang wrote: >>> On Wed, Mar 27, 2024 at 10:53 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: >>>> >>>> On 2024/03/27 11:50, Jason Wang wrote: >>>>> 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. >>>> >>>> igb passes igb_tx_pkt_vmdq_callback to net_tx_pkt_send_custom(). >>>> igb_tx_pkt_vmdq_callback() passes the packet to its rx path for loopback. >>>> >>> >>> You are right, how about igb_tx_pkt_vmdq_callback()? >>> >>> We probably need to tweak the name if it is only used in rx path. >> >> igb_tx_pkt_vmdq_callback() itself is part of the tx path of a PCI >> function, and invokes the rx path of another PCI function in case of >> loopback, or triggers the transmission to the external peer. > > Right, so if it's an external TX, VIRTIO_NET_HDR_F_DATA_VALID may not > work there. It should be fine since it's just a hint. Regards, Akihiko Odaki > > Thanks > >> >> Regards, >> Akihiko Odaki >> >>> >>> Thanks >>> >>>> Regards, >>>> Akihiko Odaki >>>> >>>>> >>>>> 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> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
On Wed, Mar 27, 2024 at 11:11 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > On 2024/03/27 12:06, Jason Wang wrote: > > On Wed, Mar 27, 2024 at 11:05 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > >> > >> On 2024/03/27 11:59, Jason Wang wrote: > >>> On Wed, Mar 27, 2024 at 10:53 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > >>>> > >>>> On 2024/03/27 11:50, Jason Wang wrote: > >>>>> 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. > >>>> > >>>> igb passes igb_tx_pkt_vmdq_callback to net_tx_pkt_send_custom(). > >>>> igb_tx_pkt_vmdq_callback() passes the packet to its rx path for loopback. > >>>> > >>> > >>> You are right, how about igb_tx_pkt_vmdq_callback()? > >>> > >>> We probably need to tweak the name if it is only used in rx path. > >> > >> igb_tx_pkt_vmdq_callback() itself is part of the tx path of a PCI > >> function, and invokes the rx path of another PCI function in case of > >> loopback, or triggers the transmission to the external peer. > > > > Right, so if it's an external TX, VIRTIO_NET_HDR_F_DATA_VALID may not > > work there. > > It should be fine since it's just a hint. It is not defined in the spec AFAIK. So we should try our best to avoid that. For example vnet header might be hardened by failing a TX packet with that by kernel I would bother now than bother it in the future for safety if it's not too hard. Thanks Thanks > > Regards, > Akihiko Odaki > > > > > Thanks > > > >> > >> Regards, > >> Akihiko Odaki > >> > >>> > >>> Thanks > >>> > >>>> Regards, > >>>> Akihiko Odaki > >>>> > >>>>> > >>>>> 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> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > > >
On 2024/03/27 12:24, Jason Wang wrote: > On Wed, Mar 27, 2024 at 11:11 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: >> >> On 2024/03/27 12:06, Jason Wang wrote: >>> On Wed, Mar 27, 2024 at 11:05 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: >>>> >>>> On 2024/03/27 11:59, Jason Wang wrote: >>>>> On Wed, Mar 27, 2024 at 10:53 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: >>>>>> >>>>>> On 2024/03/27 11:50, Jason Wang wrote: >>>>>>> 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. >>>>>> >>>>>> igb passes igb_tx_pkt_vmdq_callback to net_tx_pkt_send_custom(). >>>>>> igb_tx_pkt_vmdq_callback() passes the packet to its rx path for loopback. >>>>>> >>>>> >>>>> You are right, how about igb_tx_pkt_vmdq_callback()? >>>>> >>>>> We probably need to tweak the name if it is only used in rx path. >>>> >>>> igb_tx_pkt_vmdq_callback() itself is part of the tx path of a PCI >>>> function, and invokes the rx path of another PCI function in case of >>>> loopback, or triggers the transmission to the external peer. >>> >>> Right, so if it's an external TX, VIRTIO_NET_HDR_F_DATA_VALID may not >>> work there. >> >> It should be fine since it's just a hint. > > It is not defined in the spec AFAIK. So we should try our best to avoid that. > > For example vnet header might be hardened by failing a TX packet with > that by kernel > > I would bother now than bother it in the future for safety if it's not too hard. I checked the specification and found: https://docs.oasis-open.org/virtio/virtio/v1.2/csd01/virtio-v1.2-csd01.html#x1-2310001 > 5.1.6.2.1 Driver Requirements: Packet Transmission > The driver MUST NOT set the VIRTIO_NET_HDR_F_DATA_VALID and > VIRTIO_NET_HDR_F_RSC_INFO bits in flags. So it's explicitly prohibited. I'll send v2, which removes VIRTIO_NET_HDR_F_DATA_VALID. Regards, Akihiko Odaki > > Thanks > > > > Thanks > >> >> Regards, >> Akihiko Odaki >> >>> >>> Thanks >>> >>>> >>>> Regards, >>>> Akihiko Odaki >>>> >>>>> >>>>> Thanks >>>>> >>>>>> Regards, >>>>>> Akihiko Odaki >>>>>> >>>>>>> >>>>>>> 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> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
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; 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);
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(+) --- base-commit: ba49d760eb04630e7b15f423ebecf6c871b8f77b change-id: 20240324-tx-c57d3c22ad73 Best regards,