[PATCH] net/iavf: fix checksum offloading

[PATCH] net/iavf: fix checksum offloading

[David Marchand]

The only presence of RTE_MBUF_F_TX_IPV4 can't be used as an indicator
that a checksum offload has been requested by an application.
Check that RTE_MBUF_F_TX_IP_CKSUM or others flags have been set.

Fixes: 1e728b01120c ("net/iavf: rework Tx path")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/iavf/iavf_rxtx.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/iavf/iavf_rxtx.c b/drivers/net/iavf/iavf_rxtx.c
index f7df4665d1..b9e2879764 100644
--- a/drivers/net/iavf/iavf_rxtx.c
+++ b/drivers/net/iavf/iavf_rxtx.c
@@ -2652,6 +2652,9 @@ iavf_build_data_desc_cmd_offset_fields(volatile uint64_t *qw1,
 		offset |= (m->l2_len >> 1)
 			<< IAVF_TX_DESC_LENGTH_MACLEN_SHIFT;
 
+	if ((m->ol_flags & IAVF_TX_CKSUM_OFFLOAD_MASK) == 0)
+		goto skip_cksum;
+
 	/* Enable L3 checksum offloading inner */
 	if (m->ol_flags & RTE_MBUF_F_TX_IP_CKSUM) {
 		if (m->ol_flags & RTE_MBUF_F_TX_IPV4) {
@@ -2702,6 +2705,7 @@ iavf_build_data_desc_cmd_offset_fields(volatile uint64_t *qw1,
 		break;
 	}
 
+skip_cksum:
 	*qw1 = rte_cpu_to_le_64((((uint64_t)command <<
 		IAVF_TXD_DATA_QW1_CMD_SHIFT) & IAVF_TXD_DATA_QW1_CMD_MASK) |
 		(((uint64_t)offset << IAVF_TXD_DATA_QW1_OFFSET_SHIFT) &
-- 
2.41.0

Re: [PATCH] net/iavf: fix checksum offloading

[Eelco Chaudron]

On 18 Aug 2023, at 11:03, David Marchand wrote:

> The only presence of RTE_MBUF_F_TX_IPV4 can't be used as an indicator
> that a checksum offload has been requested by an application.
> Check that RTE_MBUF_F_TX_IP_CKSUM or others flags have been set.
>
> Fixes: 1e728b01120c ("net/iavf: rework Tx path")
> Cc: stable@dpdk.org

Thanks for fixing this David! I tested and reviewed this change and it works now.

Tested-by: Eelco Chaudron <echaudro@redhat.com>
Acked-by: Eelco Chaudron <echaudro@redhat.com>

> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  drivers/net/iavf/iavf_rxtx.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/iavf/iavf_rxtx.c b/drivers/net/iavf/iavf_rxtx.c
> index f7df4665d1..b9e2879764 100644
> --- a/drivers/net/iavf/iavf_rxtx.c
> +++ b/drivers/net/iavf/iavf_rxtx.c
> @@ -2652,6 +2652,9 @@ iavf_build_data_desc_cmd_offset_fields(volatile uint64_t *qw1,
>  		offset |= (m->l2_len >> 1)
>  			<< IAVF_TX_DESC_LENGTH_MACLEN_SHIFT;
>
> +	if ((m->ol_flags & IAVF_TX_CKSUM_OFFLOAD_MASK) == 0)
> +		goto skip_cksum;
> +
>  	/* Enable L3 checksum offloading inner */
>  	if (m->ol_flags & RTE_MBUF_F_TX_IP_CKSUM) {
>  		if (m->ol_flags & RTE_MBUF_F_TX_IPV4) {
> @@ -2702,6 +2705,7 @@ iavf_build_data_desc_cmd_offset_fields(volatile uint64_t *qw1,
>  		break;
>  	}
>
> +skip_cksum:
>  	*qw1 = rte_cpu_to_le_64((((uint64_t)command <<
>  		IAVF_TXD_DATA_QW1_CMD_SHIFT) & IAVF_TXD_DATA_QW1_CMD_MASK) |
>  		(((uint64_t)offset << IAVF_TXD_DATA_QW1_OFFSET_SHIFT) &
> -- 
> 2.41.0

Re: [PATCH] net/iavf: fix checksum offloading

[David Marchand]

On Mon, Aug 21, 2023 at 10:03 AM Eelco Chaudron <echaudro@redhat.com> wrote:
> On 18 Aug 2023, at 11:03, David Marchand wrote:
>
> > The only presence of RTE_MBUF_F_TX_IPV4 can't be used as an indicator
> > that a checksum offload has been requested by an application.
> > Check that RTE_MBUF_F_TX_IP_CKSUM or others flags have been set.
> >
> > Fixes: 1e728b01120c ("net/iavf: rework Tx path")
> > Cc: stable@dpdk.org
>
> Thanks for fixing this David! I tested and reviewed this change and it works now.

For the record, Eelco encountered an issue with OVS 3.2 which now uses
ip checksum offloading.
Packets requiring such offloading were dropped by either the net/iavf
driver or the i40e VF hw itself.


-- 
David Marchand

RE: [PATCH] net/iavf: fix checksum offloading

[Zhang, Qi Z]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Friday, August 18, 2023 5:04 PM
> To: dev@dpdk.org
> Cc: echaudro@redhat.com; mkp@redhat.com; stable@dpdk.org; Wu, Jingjing
> <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Doherty, Declan
> <declan.doherty@intel.com>; Sinha, Abhijit <abhijit.sinha@intel.com>; Nicolau,
> Radu <radu.nicolau@intel.com>
> Subject: [PATCH] net/iavf: fix checksum offloading
> 
> The only presence of RTE_MBUF_F_TX_IPV4 can't be used as an indicator that
> a checksum offload has been requested by an application.

According to current implementation, actually the only presence of RTE_MBUF_F_TX_IPV4 will cause IIPT = 10b, this scenario corresponds to an 'IPv4 packet with no IP checksum offload,' according to datasheet.
So, I assume in this situation, the PMD  continues to operate under the assumption that the application has not requested checksum offloading.

Could you share more insight what is the failure,  maybe we can perform a more comprehensive investigation?

Thanks
Qi



> Check that RTE_MBUF_F_TX_IP_CKSUM or others flags have been set. 
> 
> Fixes: 1e728b01120c ("net/iavf: rework Tx path")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  drivers/net/iavf/iavf_rxtx.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/iavf/iavf_rxtx.c b/drivers/net/iavf/iavf_rxtx.c index
> f7df4665d1..b9e2879764 100644
> --- a/drivers/net/iavf/iavf_rxtx.c
> +++ b/drivers/net/iavf/iavf_rxtx.c
> @@ -2652,6 +2652,9 @@ iavf_build_data_desc_cmd_offset_fields(volatile
> uint64_t *qw1,
>  		offset |= (m->l2_len >> 1)
>  			<< IAVF_TX_DESC_LENGTH_MACLEN_SHIFT;
> 
> +	if ((m->ol_flags & IAVF_TX_CKSUM_OFFLOAD_MASK) == 0)
> +		goto skip_cksum;
> +
>  	/* Enable L3 checksum offloading inner */
>  	if (m->ol_flags & RTE_MBUF_F_TX_IP_CKSUM) {
>  		if (m->ol_flags & RTE_MBUF_F_TX_IPV4) { @@ -2702,6
> +2705,7 @@ iavf_build_data_desc_cmd_offset_fields(volatile uint64_t *qw1,
>  		break;
>  	}
> 
> +skip_cksum:
>  	*qw1 = rte_cpu_to_le_64((((uint64_t)command <<
>  		IAVF_TXD_DATA_QW1_CMD_SHIFT) &
> IAVF_TXD_DATA_QW1_CMD_MASK) |
>  		(((uint64_t)offset << IAVF_TXD_DATA_QW1_OFFSET_SHIFT) &
> --
> 2.41.0

Re: [PATCH] net/iavf: fix checksum offloading

[David Marchand]

On Mon, Aug 21, 2023 at 1:54 PM Zhang, Qi Z <qi.z.zhang@intel.com> wrote:
> > Subject: [PATCH] net/iavf: fix checksum offloading
> >
> > The only presence of RTE_MBUF_F_TX_IPV4 can't be used as an indicator that
> > a checksum offload has been requested by an application.
>
> According to current implementation, actually the only presence of RTE_MBUF_F_TX_IPV4 will cause IIPT = 10b, this scenario corresponds to an 'IPv4 packet with no IP checksum offload,' according to datasheet.
> So, I assume in this situation, the PMD  continues to operate under the assumption that the application has not requested checksum offloading.
>
> Could you share more insight what is the failure,  maybe we can perform a more comprehensive investigation?

I think the missing piece is that OVS passes a l2_len == l3_len == 0.
In our tests, we could see that tx_errors get incremented for each
failed packet to transmit.


-- 
David Marchand

RE: [PATCH] net/iavf: fix checksum offloading

[Zhang, Qi Z]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Tuesday, August 22, 2023 1:29 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; echaudro@redhat.com; mkp@redhat.com;
> stable@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Doherty, Declan <declan.doherty@intel.com>; Sinha,
> Abhijit <abhijit.sinha@intel.com>; Nicolau, Radu <radu.nicolau@intel.com>
> Subject: Re: [PATCH] net/iavf: fix checksum offloading
> 
> On Mon, Aug 21, 2023 at 1:54 PM Zhang, Qi Z <qi.z.zhang@intel.com> wrote:
> > > Subject: [PATCH] net/iavf: fix checksum offloading
> > >
> > > The only presence of RTE_MBUF_F_TX_IPV4 can't be used as an
> > > indicator that a checksum offload has been requested by an application.
> >
> > According to current implementation, actually the only presence of
> RTE_MBUF_F_TX_IPV4 will cause IIPT = 10b, this scenario corresponds to an
> 'IPv4 packet with no IP checksum offload,' according to datasheet.
> > So, I assume in this situation, the PMD  continues to operate under the
> assumption that the application has not requested checksum offloading.
> >
> > Could you share more insight what is the failure,  maybe we can perform a
> more comprehensive investigation?
> 
> I think the missing piece is that OVS passes a l2_len == l3_len == 0.
> In our tests, we could see that tx_errors get incremented for each failed packet
> to transmit.

OK, do you think to ignore RTE_MBUF_F_TX_IPV4 when l3_len = 0 is a better fix?

> 
> 
> --
> David Marchand

Re: [PATCH] net/iavf: fix checksum offloading

[David Marchand]

On Tue, Aug 22, 2023 at 3:52 AM Zhang, Qi Z <qi.z.zhang@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: David Marchand <david.marchand@redhat.com>
> > Sent: Tuesday, August 22, 2023 1:29 AM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > Cc: dev@dpdk.org; echaudro@redhat.com; mkp@redhat.com;
> > stable@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> > <beilei.xing@intel.com>; Doherty, Declan <declan.doherty@intel.com>; Sinha,
> > Abhijit <abhijit.sinha@intel.com>; Nicolau, Radu <radu.nicolau@intel.com>
> > Subject: Re: [PATCH] net/iavf: fix checksum offloading
> >
> > On Mon, Aug 21, 2023 at 1:54 PM Zhang, Qi Z <qi.z.zhang@intel.com> wrote:
> > > > Subject: [PATCH] net/iavf: fix checksum offloading
> > > >
> > > > The only presence of RTE_MBUF_F_TX_IPV4 can't be used as an
> > > > indicator that a checksum offload has been requested by an application.
> > >
> > > According to current implementation, actually the only presence of
> > RTE_MBUF_F_TX_IPV4 will cause IIPT = 10b, this scenario corresponds to an
> > 'IPv4 packet with no IP checksum offload,' according to datasheet.
> > > So, I assume in this situation, the PMD  continues to operate under the
> > assumption that the application has not requested checksum offloading.
> > >
> > > Could you share more insight what is the failure,  maybe we can perform a
> > more comprehensive investigation?
> >
> > I think the missing piece is that OVS passes a l2_len == l3_len == 0.
> > In our tests, we could see that tx_errors get incremented for each failed packet
> > to transmit.
>
> OK, do you think to ignore RTE_MBUF_F_TX_IPV4 when l3_len = 0 is a better fix?

Looking at the mbuf API, l2_len and l3_len should be considered by a
driver if ol_flags contains at least one of RTE_MBUF_F_TX_SEC_OFFLOAD,
RTE_MBUF_F_TX_TUNNEL_*, RTE_MBUF_F_TX_TCP_SEG,
RTE_MBUF_F_TX_(IP|TCP|UDP|SCTP)_CKSUM.
Here, it is not the case.

If the driver reads l2_len or l3_len, this is an undefined behavior:
for example, OVS might have been using l2_len or l3_len for its
internal uses (though I agree it would be risky for an application to
do so).

We probably need to fix access to l2_len a few lines before my patch.

        if (m->ol_flags & RTE_MBUF_F_TX_TUNNEL_MASK &&
                        !(m->ol_flags & RTE_MBUF_F_TX_SEC_OFFLOAD))
                offset |= (m->outer_l2_len >> 1)
                        << IAVF_TX_DESC_LENGTH_MACLEN_SHIFT;
        else
                offset |= (m->l2_len >> 1)
                        << IAVF_TX_DESC_LENGTH_MACLEN_SHIFT;


But to be clear, no I don't think looking at l3_len value is better:
it should not be read at all.


-- 
David Marchand

RE: [PATCH] net/iavf: fix checksum offloading

[Zhang, Qi Z]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Tuesday, August 22, 2023 2:12 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; echaudro@redhat.com; mkp@redhat.com;
> stable@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Doherty, Declan <declan.doherty@intel.com>; Sinha,
> Abhijit <abhijit.sinha@intel.com>; Nicolau, Radu <radu.nicolau@intel.com>
> Subject: Re: [PATCH] net/iavf: fix checksum offloading
> 
> On Tue, Aug 22, 2023 at 3:52 AM Zhang, Qi Z <qi.z.zhang@intel.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: David Marchand <david.marchand@redhat.com>
> > > Sent: Tuesday, August 22, 2023 1:29 AM
> > > To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > > Cc: dev@dpdk.org; echaudro@redhat.com; mkp@redhat.com;
> > > stable@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> > > <beilei.xing@intel.com>; Doherty, Declan <declan.doherty@intel.com>;
> > > Sinha, Abhijit <abhijit.sinha@intel.com>; Nicolau, Radu
> > > <radu.nicolau@intel.com>
> > > Subject: Re: [PATCH] net/iavf: fix checksum offloading
> > >
> > > On Mon, Aug 21, 2023 at 1:54 PM Zhang, Qi Z <qi.z.zhang@intel.com>
> wrote:
> > > > > Subject: [PATCH] net/iavf: fix checksum offloading
> > > > >
> > > > > The only presence of RTE_MBUF_F_TX_IPV4 can't be used as an
> > > > > indicator that a checksum offload has been requested by an application.
> > > >
> > > > According to current implementation, actually the only presence of
> > > RTE_MBUF_F_TX_IPV4 will cause IIPT = 10b, this scenario corresponds
> > > to an
> > > 'IPv4 packet with no IP checksum offload,' according to datasheet.
> > > > So, I assume in this situation, the PMD  continues to operate
> > > > under the
> > > assumption that the application has not requested checksum offloading.
> > > >
> > > > Could you share more insight what is the failure,  maybe we can
> > > > perform a
> > > more comprehensive investigation?
> > >
> > > I think the missing piece is that OVS passes a l2_len == l3_len == 0.
> > > In our tests, we could see that tx_errors get incremented for each
> > > failed packet to transmit.
> >
> > OK, do you think to ignore RTE_MBUF_F_TX_IPV4 when l3_len = 0 is a better
> fix?
> 
> Looking at the mbuf API, l2_len and l3_len should be considered by a driver if
> ol_flags contains at least one of RTE_MBUF_F_TX_SEC_OFFLOAD,
> RTE_MBUF_F_TX_TUNNEL_*, RTE_MBUF_F_TX_TCP_SEG,
> RTE_MBUF_F_TX_(IP|TCP|UDP|SCTP)_CKSUM.
> Here, it is not the case.
> 
> If the driver reads l2_len or l3_len, this is an undefined behavior:
> for example, OVS might have been using l2_len or l3_len for its internal uses
> (though I agree it would be risky for an application to do so).
> 
> We probably need to fix access to l2_len a few lines before my patch.
> 
>         if (m->ol_flags & RTE_MBUF_F_TX_TUNNEL_MASK &&
>                         !(m->ol_flags & RTE_MBUF_F_TX_SEC_OFFLOAD))
>                 offset |= (m->outer_l2_len >> 1)
>                         << IAVF_TX_DESC_LENGTH_MACLEN_SHIFT;
>         else
>                 offset |= (m->l2_len >> 1)
>                         << IAVF_TX_DESC_LENGTH_MACLEN_SHIFT;
> 
> 
> But to be clear, no I don't think looking at l3_len value is better:
> it should not be read at all.

Yes, you may be correct; it appears that this issue is unrelated to l3_len. The primary concern is to prevent the configuration of Tx descriptors with incorrect values.

Based on your description, it seems the problem arises when the PMD sets MACLEN to 0 and also configures IIPT as 01b, Is this correct?

To prevent this issue, we could implement a check where, if l2_len is 0, we simply ignore the IIPT configuration and keep it at 0. (which leads to same configure with your patch)

Regarding your mention of 'fix access to l2_len,' if l2_len is 0, there's no change in the offset regardless of whether l2_len is accessed or not. Did you mean setting a fixed value of MACLEN to 14?"









> 
> 
> --
> David Marchand

Re: [PATCH] net/iavf: fix checksum offloading

[David Marchand]

On Tue, Aug 22, 2023 at 9:33 AM Zhang, Qi Z <qi.z.zhang@intel.com> wrote:
> > If the driver reads l2_len or l3_len, this is an undefined behavior:
> > for example, OVS might have been using l2_len or l3_len for its internal uses
> > (though I agree it would be risky for an application to do so).
> >
> > We probably need to fix access to l2_len a few lines before my patch.
> >
> >         if (m->ol_flags & RTE_MBUF_F_TX_TUNNEL_MASK &&
> >                         !(m->ol_flags & RTE_MBUF_F_TX_SEC_OFFLOAD))
> >                 offset |= (m->outer_l2_len >> 1)
> >                         << IAVF_TX_DESC_LENGTH_MACLEN_SHIFT;
> >         else
> >                 offset |= (m->l2_len >> 1)
> >                         << IAVF_TX_DESC_LENGTH_MACLEN_SHIFT;
> >
> >
> > But to be clear, no I don't think looking at l3_len value is better:
> > it should not be read at all.
>
> Yes, you may be correct; it appears that this issue is unrelated to l3_len. The primary concern is to prevent the configuration of Tx descriptors with incorrect values.
>
> Based on your description, it seems the problem arises when the PMD sets MACLEN to 0 and also configures IIPT as 01b, Is this correct?
>
> To prevent this issue, we could implement a check where, if l2_len is 0, we simply ignore the IIPT configuration and keep it at 0. (which leads to same configure with your patch)

The driver is _not_ supposed to read l2_len or l3_len if no valid
ol_flags is set.
I will reject any suggestion where you consider their values.


-- 
David Marchand

RE: [PATCH] net/iavf: fix checksum offloading

[Zhang, Qi Z]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Tuesday, August 22, 2023 3:40 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; echaudro@redhat.com; mkp@redhat.com;
> stable@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Doherty, Declan <declan.doherty@intel.com>; Sinha,
> Abhijit <abhijit.sinha@intel.com>; Nicolau, Radu <radu.nicolau@intel.com>
> Subject: Re: [PATCH] net/iavf: fix checksum offloading
> 
> On Tue, Aug 22, 2023 at 9:33 AM Zhang, Qi Z <qi.z.zhang@intel.com> wrote:
> > > If the driver reads l2_len or l3_len, this is an undefined behavior:
> > > for example, OVS might have been using l2_len or l3_len for its
> > > internal uses (though I agree it would be risky for an application to do so).
> > >
> > > We probably need to fix access to l2_len a few lines before my patch.
> > >
> > >         if (m->ol_flags & RTE_MBUF_F_TX_TUNNEL_MASK &&
> > >                         !(m->ol_flags & RTE_MBUF_F_TX_SEC_OFFLOAD))
> > >                 offset |= (m->outer_l2_len >> 1)
> > >                         << IAVF_TX_DESC_LENGTH_MACLEN_SHIFT;
> > >         else
> > >                 offset |= (m->l2_len >> 1)
> > >                         << IAVF_TX_DESC_LENGTH_MACLEN_SHIFT;
> > >
> > >
> > > But to be clear, no I don't think looking at l3_len value is better:
> > > it should not be read at all.
> >
> > Yes, you may be correct; it appears that this issue is unrelated to l3_len. The
> primary concern is to prevent the configuration of Tx descriptors with incorrect
> values.
> >
> > Based on your description, it seems the problem arises when the PMD sets
> MACLEN to 0 and also configures IIPT as 01b, Is this correct?
> >
> > To prevent this issue, we could implement a check where, if l2_len is
> > 0, we simply ignore the IIPT configuration and keep it at 0. (which
> > leads to same configure with your patch)
> 
> The driver is _not_ supposed to read l2_len or l3_len if no valid ol_flags is set.
> I will reject any suggestion where you consider their values.

Alright, we could directly verify the MACLEN value in offset and decide if IIPT should be configured or not, regardless of the values of l2_len and l3_len
And before that, l2_len / l3_len should not be accessed by the driver if no related offload is required.

> 
> 
> --
> David Marchand

RE: [PATCH] net/iavf: fix checksum offloading

[Zhang, Qi Z]

> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Tuesday, August 22, 2023 3:59 PM
> To: 'David Marchand' <david.marchand@redhat.com>
> Cc: dev@dpdk.org; echaudro@redhat.com; mkp@redhat.com;
> stable@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Doherty, Declan <declan.doherty@intel.com>; Sinha,
> Abhijit <abhijit.sinha@intel.com>; Nicolau, Radu <radu.nicolau@intel.com>
> Subject: RE: [PATCH] net/iavf: fix checksum offloading
> 
> 
> 
> > -----Original Message-----
> > From: David Marchand <david.marchand@redhat.com>
> > Sent: Tuesday, August 22, 2023 3:40 PM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > Cc: dev@dpdk.org; echaudro@redhat.com; mkp@redhat.com;
> > stable@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> > <beilei.xing@intel.com>; Doherty, Declan <declan.doherty@intel.com>;
> > Sinha, Abhijit <abhijit.sinha@intel.com>; Nicolau, Radu
> > <radu.nicolau@intel.com>
> > Subject: Re: [PATCH] net/iavf: fix checksum offloading
> >
> > On Tue, Aug 22, 2023 at 9:33 AM Zhang, Qi Z <qi.z.zhang@intel.com> wrote:
> > > > If the driver reads l2_len or l3_len, this is an undefined behavior:
> > > > for example, OVS might have been using l2_len or l3_len for its
> > > > internal uses (though I agree it would be risky for an application to do so).
> > > >
> > > > We probably need to fix access to l2_len a few lines before my patch.
> > > >
> > > >         if (m->ol_flags & RTE_MBUF_F_TX_TUNNEL_MASK &&
> > > >                         !(m->ol_flags & RTE_MBUF_F_TX_SEC_OFFLOAD))
> > > >                 offset |= (m->outer_l2_len >> 1)
> > > >                         << IAVF_TX_DESC_LENGTH_MACLEN_SHIFT;
> > > >         else
> > > >                 offset |= (m->l2_len >> 1)
> > > >                         << IAVF_TX_DESC_LENGTH_MACLEN_SHIFT;
> > > >

After second thought, I think your patch looks good, will you submit a new version to cover above fix?

[PATCH v2] net/iavf: fix checksum offloading

[David Marchand]

l2_len and l3_len fields are related to Tx offloading features.

It is undefined in the DPDK API what those fields may contain if an
application did not request a Tx offload.

Skip reading them if no Tx offloads has been requested.

Fixes: 1e728b01120c ("net/iavf: rework Tx path")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v1:
- updated commitlog and patch to focus on undefined behavior of net/iavf
  wrt reading l2_len/l3_len,

---
 drivers/net/iavf/iavf_rxtx.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/iavf/iavf_rxtx.c b/drivers/net/iavf/iavf_rxtx.c
index f7df4665d1..33b0f9f482 100644
--- a/drivers/net/iavf/iavf_rxtx.c
+++ b/drivers/net/iavf/iavf_rxtx.c
@@ -2643,6 +2643,9 @@ iavf_build_data_desc_cmd_offset_fields(volatile uint64_t *qw1,
 		l2tag1 |= m->vlan_tci;
 	}
 
+	if ((m->ol_flags & IAVF_TX_CKSUM_OFFLOAD_MASK) == 0)
+		goto skip_cksum;
+
 	/* Set MACLEN */
 	if (m->ol_flags & RTE_MBUF_F_TX_TUNNEL_MASK &&
 			!(m->ol_flags & RTE_MBUF_F_TX_SEC_OFFLOAD))
@@ -2702,6 +2705,7 @@ iavf_build_data_desc_cmd_offset_fields(volatile uint64_t *qw1,
 		break;
 	}
 
+skip_cksum:
 	*qw1 = rte_cpu_to_le_64((((uint64_t)command <<
 		IAVF_TXD_DATA_QW1_CMD_SHIFT) & IAVF_TXD_DATA_QW1_CMD_MASK) |
 		(((uint64_t)offset << IAVF_TXD_DATA_QW1_OFFSET_SHIFT) &
-- 
2.41.0

RE: [PATCH v2] net/iavf: fix checksum offloading

[Zhang, Qi Z]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Wednesday, August 23, 2023 2:29 PM
> To: dev@dpdk.org
> Cc: echaudro@redhat.com; mkp@redhat.com; Zhang, Qi Z
> <qi.z.zhang@intel.com>; stable@dpdk.org; Wu, Jingjing
> <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Sinha, Abhijit
> <abhijit.sinha@intel.com>; Doherty, Declan <declan.doherty@intel.com>;
> Nicolau, Radu <radu.nicolau@intel.com>
> Subject: [PATCH v2] net/iavf: fix checksum offloading
> 
> l2_len and l3_len fields are related to Tx offloading features.
> 
> It is undefined in the DPDK API what those fields may contain if an application
> did not request a Tx offload.
> 
> Skip reading them if no Tx offloads has been requested.
> 
> Fixes: 1e728b01120c ("net/iavf: rework Tx path")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>

Acked-by: Qi Zhang <qi.z.zhang@intel.com>

Applied to dpdk-next-net-intel.

Thanks
Qi

Re: [PATCH v2] net/iavf: fix checksum offloading

[Patrick Robb]

[-- Attachment #1: Type: text/plain, Size: 1563 bytes --]

Hi David, you can ignore the failures UNH reported for this patch - we made
a mistake reconfiguring jenkins for the refactored unit test suite. As
discussed we are not running the arm64 unit tests until one of the
solutions we talked about is in place.

On Wed, Aug 23, 2023 at 4:33 AM Zhang, Qi Z <qi.z.zhang@intel.com> wrote:

>
>
> > -----Original Message-----
> > From: David Marchand <david.marchand@redhat.com>
> > Sent: Wednesday, August 23, 2023 2:29 PM
> > To: dev@dpdk.org
> > Cc: echaudro@redhat.com; mkp@redhat.com; Zhang, Qi Z
> > <qi.z.zhang@intel.com>; stable@dpdk.org; Wu, Jingjing
> > <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Sinha,
> Abhijit
> > <abhijit.sinha@intel.com>; Doherty, Declan <declan.doherty@intel.com>;
> > Nicolau, Radu <radu.nicolau@intel.com>
> > Subject: [PATCH v2] net/iavf: fix checksum offloading
> >
> > l2_len and l3_len fields are related to Tx offloading features.
> >
> > It is undefined in the DPDK API what those fields may contain if an
> application
> > did not request a Tx offload.
> >
> > Skip reading them if no Tx offloads has been requested.
> >
> > Fixes: 1e728b01120c ("net/iavf: rework Tx path")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
>
> Acked-by: Qi Zhang <qi.z.zhang@intel.com>
>
> Applied to dpdk-next-net-intel.
>
> Thanks
> Qi
>
>

-- 

Patrick Robb

Technical Service Manager

UNH InterOperability Laboratory

21 Madbury Rd, Suite 100, Durham, NH 03824

www.iol.unh.edu

[-- Attachment #2: Type: text/html, Size: 4743 bytes --]