diff mbox series

net/iavf: fix IAVF_TX_OFFLOAD_MASK definition

Message ID 20231024091328.11933-1-radu.nicolau@intel.com (mailing list archive)
State Accepted, archived
Delegated to: Qi Zhang
Headers
Series net/iavf: fix IAVF_TX_OFFLOAD_MASK definition |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-compile-amd64-testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-unit-arm64-testing success Testing PASS
ci/intel-Functional success Functional PASS
ci/iol-unit-amd64-testing success Testing PASS
ci/iol-compile-arm64-testing success Testing PASS
ci/github-robot: build success github build: passed

Commit Message

Radu Nicolau Oct. 24, 2023, 9:13 a.m. UTC 
  IAVF_TX_OFFLOAD_MASK definition contained
RTE_ETH_TX_OFFLOAD_SECURITY instead of
RTE_MBUF_F_TX_SEC_OFFLOAD.

Fixes: 6bc987ecb860 ("net/iavf: support IPsec inline crypto")
Cc: stable@dpdk.org

Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
---
 drivers/net/iavf/iavf_rxtx.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

David Marchand Oct. 24, 2023, 9:49 a.m. UTC | #1 
On Tue, Oct 24, 2023 at 11:13 AM Radu Nicolau <radu.nicolau@intel.com> wrote:
>
> IAVF_TX_OFFLOAD_MASK definition contained
> RTE_ETH_TX_OFFLOAD_SECURITY instead of
> RTE_MBUF_F_TX_SEC_OFFLOAD.
>
> Fixes: 6bc987ecb860 ("net/iavf: support IPsec inline crypto")
> Cc: stable@dpdk.org
>
> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>

Something is not clear to me.
How was the IPsec inline crypto feature supposed to work with this
driver so far?

Any packet with the RTE_MBUF_F_TX_SEC_OFFLOAD flag should have been
refused in iavf_prep_pkts.
Radu Nicolau Oct. 24, 2023, 10:22 a.m. UTC | #2 
On 24-Oct-23 10:49 AM, David Marchand wrote:
> On Tue, Oct 24, 2023 at 11:13 AM Radu Nicolau <radu.nicolau@intel.com> wrote:
>> IAVF_TX_OFFLOAD_MASK definition contained
>> RTE_ETH_TX_OFFLOAD_SECURITY instead of
>> RTE_MBUF_F_TX_SEC_OFFLOAD.
>>
>> Fixes: 6bc987ecb860 ("net/iavf: support IPsec inline crypto")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
> Something is not clear to me.
> How was the IPsec inline crypto feature supposed to work with this
> driver so far?
>
> Any packet with the RTE_MBUF_F_TX_SEC_OFFLOAD flag should have been
> refused in iavf_prep_pkts.
>
It worked because the IPsec sample app doesn't call rte_eth_tx_prepare, 
and from what I can see no other sample app does.
Qi Zhang Oct. 24, 2023, 11:24 a.m. UTC | #3 
> -----Original Message-----
> From: Radu Nicolau <radu.nicolau@intel.com>
> Sent: Tuesday, October 24, 2023 6:23 PM
> To: Marchand, David <david.marchand@redhat.com>
> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>;
> dev@dpdk.org; stable@dpdk.org
> Subject: Re: [PATCH] net/iavf: fix IAVF_TX_OFFLOAD_MASK definition
> 
> 
> On 24-Oct-23 10:49 AM, David Marchand wrote:
> > On Tue, Oct 24, 2023 at 11:13 AM Radu Nicolau <radu.nicolau@intel.com>
> wrote:
> >> IAVF_TX_OFFLOAD_MASK definition contained
> RTE_ETH_TX_OFFLOAD_SECURITY
> >> instead of RTE_MBUF_F_TX_SEC_OFFLOAD.
> >>
> >> Fixes: 6bc987ecb860 ("net/iavf: support IPsec inline crypto")
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
> > Something is not clear to me.
> > How was the IPsec inline crypto feature supposed to work with this
> > driver so far?
> >
> > Any packet with the RTE_MBUF_F_TX_SEC_OFFLOAD flag should have been
> > refused in iavf_prep_pkts.
> >
> It worked because the IPsec sample app doesn't call rte_eth_tx_prepare, and
> from what I can see no other sample app does.

To keep consistent, its better to refine the IAVF_TX_OFFLOAD_NOTSUP_MASK definition.
Radu Nicolau Oct. 24, 2023, 2:48 p.m. UTC | #4 
On 24-Oct-23 12:24 PM, Zhang, Qi Z wrote:
>
>> -----Original Message-----
>> From: Radu Nicolau <radu.nicolau@intel.com>
>> Sent: Tuesday, October 24, 2023 6:23 PM
>> To: Marchand, David <david.marchand@redhat.com>
>> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>;
>> dev@dpdk.org; stable@dpdk.org
>> Subject: Re: [PATCH] net/iavf: fix IAVF_TX_OFFLOAD_MASK definition
>>
>>
>> On 24-Oct-23 10:49 AM, David Marchand wrote:
>>> On Tue, Oct 24, 2023 at 11:13 AM Radu Nicolau <radu.nicolau@intel.com>
>> wrote:
>>>> IAVF_TX_OFFLOAD_MASK definition contained
>> RTE_ETH_TX_OFFLOAD_SECURITY
>>>> instead of RTE_MBUF_F_TX_SEC_OFFLOAD.
>>>>
>>>> Fixes: 6bc987ecb860 ("net/iavf: support IPsec inline crypto")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
>>> Something is not clear to me.
>>> How was the IPsec inline crypto feature supposed to work with this
>>> driver so far?
>>>
>>> Any packet with the RTE_MBUF_F_TX_SEC_OFFLOAD flag should have been
>>> refused in iavf_prep_pkts.
>>>
>> It worked because the IPsec sample app doesn't call rte_eth_tx_prepare, and
>> from what I can see no other sample app does.
> To keep consistent, its better to refine the IAVF_TX_OFFLOAD_NOTSUP_MASK definition.

You mean like this?


#define IAVF_TX_OFFLOAD_NOTSUP_MASK ( \
         RTE_MBUF_F_TX_OFFLOAD_MASK ^ (  \
             RTE_MBUF_F_TX_OUTER_IPV6 |         \
             RTE_MBUF_F_TX_OUTER_IPV4 |         \
             RTE_MBUF_F_TX_IPV6 |             \
             RTE_MBUF_F_TX_IPV4 |             \
             RTE_MBUF_F_TX_VLAN |         \
             RTE_MBUF_F_TX_IP_CKSUM |         \
             RTE_MBUF_F_TX_L4_MASK |         \
             RTE_MBUF_F_TX_TCP_SEG |         \
             RTE_MBUF_F_TX_UDP_SEG |      \
             RTE_MBUF_F_TX_TUNNEL_MASK |    \
             RTE_MBUF_F_TX_OUTER_IP_CKSUM |  \
             RTE_MBUF_F_TX_OUTER_UDP_CKSUM | \
             RTE_MBUF_F_TX_SEC_OFFLOAD))
Qi Zhang Oct. 24, 2023, 11:30 p.m. UTC | #5 
> -----Original Message-----
> From: Nicolau, Radu <radu.nicolau@intel.com>
> Sent: Tuesday, October 24, 2023 10:49 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Marchand, David
> <david.marchand@redhat.com>
> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>;
> dev@dpdk.org; stable@dpdk.org
> Subject: Re: [PATCH] net/iavf: fix IAVF_TX_OFFLOAD_MASK definition
> 
> 
> On 24-Oct-23 12:24 PM, Zhang, Qi Z wrote:
> >
> >> -----Original Message-----
> >> From: Radu Nicolau <radu.nicolau@intel.com>
> >> Sent: Tuesday, October 24, 2023 6:23 PM
> >> To: Marchand, David <david.marchand@redhat.com>
> >> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> >> <beilei.xing@intel.com>; dev@dpdk.org; stable@dpdk.org
> >> Subject: Re: [PATCH] net/iavf: fix IAVF_TX_OFFLOAD_MASK definition
> >>
> >>
> >> On 24-Oct-23 10:49 AM, David Marchand wrote:
> >>> On Tue, Oct 24, 2023 at 11:13 AM Radu Nicolau
> >>> <radu.nicolau@intel.com>
> >> wrote:
> >>>> IAVF_TX_OFFLOAD_MASK definition contained
> >> RTE_ETH_TX_OFFLOAD_SECURITY
> >>>> instead of RTE_MBUF_F_TX_SEC_OFFLOAD.
> >>>>
> >>>> Fixes: 6bc987ecb860 ("net/iavf: support IPsec inline crypto")
> >>>> Cc: stable@dpdk.org
> >>>>
> >>>> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
> >>> Something is not clear to me.
> >>> How was the IPsec inline crypto feature supposed to work with this
> >>> driver so far?
> >>>
> >>> Any packet with the RTE_MBUF_F_TX_SEC_OFFLOAD flag should have
> been
> >>> refused in iavf_prep_pkts.
> >>>
> >> It worked because the IPsec sample app doesn't call
> >> rte_eth_tx_prepare, and from what I can see no other sample app does.
> > To keep consistent, its better to refine the
> IAVF_TX_OFFLOAD_NOTSUP_MASK definition.
> 
> You mean like this?
> 
> 
> #define IAVF_TX_OFFLOAD_NOTSUP_MASK ( \
>          RTE_MBUF_F_TX_OFFLOAD_MASK ^ (  \
>              RTE_MBUF_F_TX_OUTER_IPV6 |         \
>              RTE_MBUF_F_TX_OUTER_IPV4 |         \
>              RTE_MBUF_F_TX_IPV6 |             \
>              RTE_MBUF_F_TX_IPV4 |             \
>              RTE_MBUF_F_TX_VLAN |         \
>              RTE_MBUF_F_TX_IP_CKSUM |         \
>              RTE_MBUF_F_TX_L4_MASK |         \
>              RTE_MBUF_F_TX_TCP_SEG |         \
>              RTE_MBUF_F_TX_UDP_SEG |      \
>              RTE_MBUF_F_TX_TUNNEL_MASK |    \
>              RTE_MBUF_F_TX_OUTER_IP_CKSUM |  \
>              RTE_MBUF_F_TX_OUTER_UDP_CKSUM | \
>              RTE_MBUF_F_TX_SEC_OFFLOAD))

Sorry, I miss understanding this code change, actually you didn't remove a flag, but just replace it,  NOTSUP_MASK no need to be changed

Then I don't understand why "Any packet with the RTE_MBUF_F_TX_SEC_OFFLOAD flag should have refused in iavf_prep_pkts"
But I assume tx_pkt_prepare should reject only invalid packets while still functioning correctly with inline IPsec.
Qi Zhang Oct. 25, 2023, 1:50 a.m. UTC | #6 
> -----Original Message-----
> From: Radu Nicolau <radu.nicolau@intel.com>
> Sent: Tuesday, October 24, 2023 5:13 PM
> To: Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>
> Cc: dev@dpdk.org; Nicolau, Radu <radu.nicolau@intel.com>;
> stable@dpdk.org
> Subject: [PATCH] net/iavf: fix IAVF_TX_OFFLOAD_MASK definition
> 
> IAVF_TX_OFFLOAD_MASK definition contained
> RTE_ETH_TX_OFFLOAD_SECURITY instead of RTE_MBUF_F_TX_SEC_OFFLOAD.
> 
> Fixes: 6bc987ecb860 ("net/iavf: support IPsec inline crypto")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
> ---
>  drivers/net/iavf/iavf_rxtx.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/iavf/iavf_rxtx.h b/drivers/net/iavf/iavf_rxtx.h index
> 605ea3f824..f29c90fb21 100644
> --- a/drivers/net/iavf/iavf_rxtx.h
> +++ b/drivers/net/iavf/iavf_rxtx.h
> @@ -98,7 +98,7 @@
>  		RTE_MBUF_F_TX_TUNNEL_MASK |	\
>  		RTE_MBUF_F_TX_OUTER_IP_CKSUM |  \
>  		RTE_MBUF_F_TX_OUTER_UDP_CKSUM | \
> -		RTE_ETH_TX_OFFLOAD_SECURITY)
> +		RTE_MBUF_F_TX_SEC_OFFLOAD)
> 
>  #define IAVF_TX_OFFLOAD_NOTSUP_MASK \
>  		(RTE_MBUF_F_TX_OFFLOAD_MASK ^
> IAVF_TX_OFFLOAD_MASK)
> --
> 2.25.1

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

Applied to dpdk-next-net-intel.

Thanks
Qi
Radu Nicolau Oct. 25, 2023, 9:01 a.m. UTC | #7 
On 25-Oct-23 12:30 AM, Zhang, Qi Z wrote:
>
>> -----Original Message-----
>> From: Nicolau, Radu <radu.nicolau@intel.com>
>> Sent: Tuesday, October 24, 2023 10:49 PM
>> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Marchand, David
>> <david.marchand@redhat.com>
>> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>;
>> dev@dpdk.org; stable@dpdk.org
>> Subject: Re: [PATCH] net/iavf: fix IAVF_TX_OFFLOAD_MASK definition
>>
>>
>> On 24-Oct-23 12:24 PM, Zhang, Qi Z wrote:
>>>> -----Original Message-----
>>>> From: Radu Nicolau <radu.nicolau@intel.com>
>>>> Sent: Tuesday, October 24, 2023 6:23 PM
>>>> To: Marchand, David <david.marchand@redhat.com>
>>>> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
>>>> <beilei.xing@intel.com>; dev@dpdk.org; stable@dpdk.org
>>>> Subject: Re: [PATCH] net/iavf: fix IAVF_TX_OFFLOAD_MASK definition
>>>>
>>>>
>>>> On 24-Oct-23 10:49 AM, David Marchand wrote:
>>>>> On Tue, Oct 24, 2023 at 11:13 AM Radu Nicolau
>>>>> <radu.nicolau@intel.com>
>>>> wrote:
>>>>>> IAVF_TX_OFFLOAD_MASK definition contained
>>>> RTE_ETH_TX_OFFLOAD_SECURITY
>>>>>> instead of RTE_MBUF_F_TX_SEC_OFFLOAD.
>>>>>>
>>>>>> Fixes: 6bc987ecb860 ("net/iavf: support IPsec inline crypto")
>>>>>> Cc: stable@dpdk.org
>>>>>>
>>>>>> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
>>>>> Something is not clear to me.
>>>>> How was the IPsec inline crypto feature supposed to work with this
>>>>> driver so far?
>>>>>
>>>>> Any packet with the RTE_MBUF_F_TX_SEC_OFFLOAD flag should have
>> been
>>>>> refused in iavf_prep_pkts.
>>>>>
>>>> It worked because the IPsec sample app doesn't call
>>>> rte_eth_tx_prepare, and from what I can see no other sample app does.
>>> To keep consistent, its better to refine the
>> IAVF_TX_OFFLOAD_NOTSUP_MASK definition.
>>
>> You mean like this?
>>
>>
>> #define IAVF_TX_OFFLOAD_NOTSUP_MASK ( \
>>           RTE_MBUF_F_TX_OFFLOAD_MASK ^ (  \
>>               RTE_MBUF_F_TX_OUTER_IPV6 |         \
>>               RTE_MBUF_F_TX_OUTER_IPV4 |         \
>>               RTE_MBUF_F_TX_IPV6 |             \
>>               RTE_MBUF_F_TX_IPV4 |             \
>>               RTE_MBUF_F_TX_VLAN |         \
>>               RTE_MBUF_F_TX_IP_CKSUM |         \
>>               RTE_MBUF_F_TX_L4_MASK |         \
>>               RTE_MBUF_F_TX_TCP_SEG |         \
>>               RTE_MBUF_F_TX_UDP_SEG |      \
>>               RTE_MBUF_F_TX_TUNNEL_MASK |    \
>>               RTE_MBUF_F_TX_OUTER_IP_CKSUM |  \
>>               RTE_MBUF_F_TX_OUTER_UDP_CKSUM | \
>>               RTE_MBUF_F_TX_SEC_OFFLOAD))
> Sorry, I miss understanding this code change, actually you didn't remove a flag, but just replace it,  NOTSUP_MASK no need to be changed
>
> Then I don't understand why "Any packet with the RTE_MBUF_F_TX_SEC_OFFLOAD flag should have refused in iavf_prep_pkts"
> But I assume tx_pkt_prepare should reject only invalid packets while still functioning correctly with inline IPsec.

rte_eth_tx_prepare would have rejected the packets before this fix, but 
no app calls rte_eth_tx_prepare. The only app that calls it is testpmd.
David Marchand Oct. 25, 2023, 9:07 a.m. UTC | #8 
On Wed, Oct 25, 2023 at 11:02 AM Radu Nicolau <radu.nicolau@intel.com> wrote:
>
>
> On 25-Oct-23 12:30 AM, Zhang, Qi Z wrote:
> >
> >> -----Original Message-----
> >> From: Nicolau, Radu <radu.nicolau@intel.com>
> >> Sent: Tuesday, October 24, 2023 10:49 PM
> >> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Marchand, David
> >> <david.marchand@redhat.com>
> >> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>;
> >> dev@dpdk.org; stable@dpdk.org
> >> Subject: Re: [PATCH] net/iavf: fix IAVF_TX_OFFLOAD_MASK definition
> >>
> >>
> >> On 24-Oct-23 12:24 PM, Zhang, Qi Z wrote:
> >>>> -----Original Message-----
> >>>> From: Radu Nicolau <radu.nicolau@intel.com>
> >>>> Sent: Tuesday, October 24, 2023 6:23 PM
> >>>> To: Marchand, David <david.marchand@redhat.com>
> >>>> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> >>>> <beilei.xing@intel.com>; dev@dpdk.org; stable@dpdk.org
> >>>> Subject: Re: [PATCH] net/iavf: fix IAVF_TX_OFFLOAD_MASK definition
> >>>>
> >>>>
> >>>> On 24-Oct-23 10:49 AM, David Marchand wrote:
> >>>>> On Tue, Oct 24, 2023 at 11:13 AM Radu Nicolau
> >>>>> <radu.nicolau@intel.com>
> >>>> wrote:
> >>>>>> IAVF_TX_OFFLOAD_MASK definition contained
> >>>> RTE_ETH_TX_OFFLOAD_SECURITY
> >>>>>> instead of RTE_MBUF_F_TX_SEC_OFFLOAD.
> >>>>>>
> >>>>>> Fixes: 6bc987ecb860 ("net/iavf: support IPsec inline crypto")
> >>>>>> Cc: stable@dpdk.org
> >>>>>>
> >>>>>> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
> >>>>> Something is not clear to me.
> >>>>> How was the IPsec inline crypto feature supposed to work with this
> >>>>> driver so far?
> >>>>>
> >>>>> Any packet with the RTE_MBUF_F_TX_SEC_OFFLOAD flag should have
> >> been
> >>>>> refused in iavf_prep_pkts.
> >>>>>
> >>>> It worked because the IPsec sample app doesn't call
> >>>> rte_eth_tx_prepare, and from what I can see no other sample app does.
> >>> To keep consistent, its better to refine the
> >> IAVF_TX_OFFLOAD_NOTSUP_MASK definition.
> >>
> >> You mean like this?
> >>
> >>
> >> #define IAVF_TX_OFFLOAD_NOTSUP_MASK ( \
> >>           RTE_MBUF_F_TX_OFFLOAD_MASK ^ (  \
> >>               RTE_MBUF_F_TX_OUTER_IPV6 |         \
> >>               RTE_MBUF_F_TX_OUTER_IPV4 |         \
> >>               RTE_MBUF_F_TX_IPV6 |             \
> >>               RTE_MBUF_F_TX_IPV4 |             \
> >>               RTE_MBUF_F_TX_VLAN |         \
> >>               RTE_MBUF_F_TX_IP_CKSUM |         \
> >>               RTE_MBUF_F_TX_L4_MASK |         \
> >>               RTE_MBUF_F_TX_TCP_SEG |         \
> >>               RTE_MBUF_F_TX_UDP_SEG |      \
> >>               RTE_MBUF_F_TX_TUNNEL_MASK |    \
> >>               RTE_MBUF_F_TX_OUTER_IP_CKSUM |  \
> >>               RTE_MBUF_F_TX_OUTER_UDP_CKSUM | \
> >>               RTE_MBUF_F_TX_SEC_OFFLOAD))
> > Sorry, I miss understanding this code change, actually you didn't remove a flag, but just replace it,  NOTSUP_MASK no need to be changed
> >
> > Then I don't understand why "Any packet with the RTE_MBUF_F_TX_SEC_OFFLOAD flag should have refused in iavf_prep_pkts"
> > But I assume tx_pkt_prepare should reject only invalid packets while still functioning correctly with inline IPsec.
>
> rte_eth_tx_prepare would have rejected the packets before this fix, but
> no app calls rte_eth_tx_prepare. The only app that calls it is testpmd.

From my understanding, applications that want checksum offload are
required to call rte_eth_tx_prepare.
Radu Nicolau Oct. 25, 2023, 10:14 a.m. UTC | #9 
On 25-Oct-23 10:07 AM, David Marchand wrote:
> On Wed, Oct 25, 2023 at 11:02 AM Radu Nicolau <radu.nicolau@intel.com> wrote:
>>
>> On 25-Oct-23 12:30 AM, Zhang, Qi Z wrote:
>>>> -----Original Message-----
>>>> From: Nicolau, Radu <radu.nicolau@intel.com>
>>>> Sent: Tuesday, October 24, 2023 10:49 PM
>>>> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Marchand, David
>>>> <david.marchand@redhat.com>
>>>> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>;
>>>> dev@dpdk.org; stable@dpdk.org
>>>> Subject: Re: [PATCH] net/iavf: fix IAVF_TX_OFFLOAD_MASK definition
>>>>
>>>>
>>>> On 24-Oct-23 12:24 PM, Zhang, Qi Z wrote:
>>>>>> -----Original Message-----
>>>>>> From: Radu Nicolau <radu.nicolau@intel.com>
>>>>>> Sent: Tuesday, October 24, 2023 6:23 PM
>>>>>> To: Marchand, David <david.marchand@redhat.com>
>>>>>> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
>>>>>> <beilei.xing@intel.com>; dev@dpdk.org; stable@dpdk.org
>>>>>> Subject: Re: [PATCH] net/iavf: fix IAVF_TX_OFFLOAD_MASK definition
>>>>>>
>>>>>>
>>>>>> On 24-Oct-23 10:49 AM, David Marchand wrote:
>>>>>>> On Tue, Oct 24, 2023 at 11:13 AM Radu Nicolau
>>>>>>> <radu.nicolau@intel.com>
>>>>>> wrote:
>>>>>>>> IAVF_TX_OFFLOAD_MASK definition contained
>>>>>> RTE_ETH_TX_OFFLOAD_SECURITY
>>>>>>>> instead of RTE_MBUF_F_TX_SEC_OFFLOAD.
>>>>>>>>
>>>>>>>> Fixes: 6bc987ecb860 ("net/iavf: support IPsec inline crypto")
>>>>>>>> Cc: stable@dpdk.org
>>>>>>>>
>>>>>>>> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
>>>>>>> Something is not clear to me.
>>>>>>> How was the IPsec inline crypto feature supposed to work with this
>>>>>>> driver so far?
>>>>>>>
>>>>>>> Any packet with the RTE_MBUF_F_TX_SEC_OFFLOAD flag should have
>>>> been
>>>>>>> refused in iavf_prep_pkts.
>>>>>>>
>>>>>> It worked because the IPsec sample app doesn't call
>>>>>> rte_eth_tx_prepare, and from what I can see no other sample app does.
>>>>> To keep consistent, its better to refine the
>>>> IAVF_TX_OFFLOAD_NOTSUP_MASK definition.
>>>>
>>>> You mean like this?
>>>>
>>>>
>>>> #define IAVF_TX_OFFLOAD_NOTSUP_MASK ( \
>>>>            RTE_MBUF_F_TX_OFFLOAD_MASK ^ (  \
>>>>                RTE_MBUF_F_TX_OUTER_IPV6 |         \
>>>>                RTE_MBUF_F_TX_OUTER_IPV4 |         \
>>>>                RTE_MBUF_F_TX_IPV6 |             \
>>>>                RTE_MBUF_F_TX_IPV4 |             \
>>>>                RTE_MBUF_F_TX_VLAN |         \
>>>>                RTE_MBUF_F_TX_IP_CKSUM |         \
>>>>                RTE_MBUF_F_TX_L4_MASK |         \
>>>>                RTE_MBUF_F_TX_TCP_SEG |         \
>>>>                RTE_MBUF_F_TX_UDP_SEG |      \
>>>>                RTE_MBUF_F_TX_TUNNEL_MASK |    \
>>>>                RTE_MBUF_F_TX_OUTER_IP_CKSUM |  \
>>>>                RTE_MBUF_F_TX_OUTER_UDP_CKSUM | \
>>>>                RTE_MBUF_F_TX_SEC_OFFLOAD))
>>> Sorry, I miss understanding this code change, actually you didn't remove a flag, but just replace it,  NOTSUP_MASK no need to be changed
>>>
>>> Then I don't understand why "Any packet with the RTE_MBUF_F_TX_SEC_OFFLOAD flag should have refused in iavf_prep_pkts"
>>> But I assume tx_pkt_prepare should reject only invalid packets while still functioning correctly with inline IPsec.
>> rte_eth_tx_prepare would have rejected the packets before this fix, but
>> no app calls rte_eth_tx_prepare. The only app that calls it is testpmd.
>  From my understanding, applications that want checksum offload are
> required to call rte_eth_tx_prepare.

TBH I don't understand much about it and looking at the implementation 
actually made things worse: for example from what I can see calling it 
when RTE_MBUF_F_TX_TCP_CKSUM is set will result in having the TCP 
checksum being computed (in software) in the prepare function.
diff mbox series

Patch

diff --git a/drivers/net/iavf/iavf_rxtx.h b/drivers/net/iavf/iavf_rxtx.h
index 605ea3f824..f29c90fb21 100644
--- a/drivers/net/iavf/iavf_rxtx.h
+++ b/drivers/net/iavf/iavf_rxtx.h
@@ -98,7 +98,7 @@ 
 		RTE_MBUF_F_TX_TUNNEL_MASK |	\
 		RTE_MBUF_F_TX_OUTER_IP_CKSUM |  \
 		RTE_MBUF_F_TX_OUTER_UDP_CKSUM | \
-		RTE_ETH_TX_OFFLOAD_SECURITY)
+		RTE_MBUF_F_TX_SEC_OFFLOAD)
 
 #define IAVF_TX_OFFLOAD_NOTSUP_MASK \
 		(RTE_MBUF_F_TX_OFFLOAD_MASK ^ IAVF_TX_OFFLOAD_MASK)