[dpdk-dev] [PATCH] net/ixgbe: remove hardcoded CRC STRIP config from ixgbe

[dpdk-dev] [PATCH] net/ixgbe: remove hardcoded CRC STRIP config from ixgbe

[Wei Zhao]

There is CRC related ifdefs for ixgbe:
CONFIG_RTE_LIBRTE_IXGBE_PF_DISABLE_STRIP_CRC=n
It is used in VF drivers ixgbevf_dev_configure() functions.
VF cannot change the CRC strip behavior and based on what PF
configured it needs to response proper to user
ixgbevf_dev_configure() request. Right now what PF set is
defined by above config options but this method is too static.

Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
---
 app/test-pmd/parameters.c        |  6 ++++++
 app/test-pmd/testpmd.c           |  2 ++
 app/test-pmd/testpmd.h           |  1 +
 drivers/net/ixgbe/ixgbe_ethdev.c | 20 ++++++++++----------
 lib/librte_ethdev/rte_ethdev.h   |  1 +
 5 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 962fad7..b981b0f 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -188,6 +188,7 @@ usage(char* progname)
 	printf("  --tx-offloads=0xXXXXXXXX: hexadecimal bitmask of TX queue offloads\n");
 	printf("  --hot-plug: enable hot plug for device.\n");
 	printf("  --vxlan-gpe-port=N: UPD port of tunnel VXLAN-GPE\n");
+	printf("  --pf-crc-keep: disable pf CRC strip function for device\n");
 	printf("  --mlockall: lock all memory\n");
 	printf("  --no-mlockall: do not lock all memory\n");
 }
@@ -623,6 +624,7 @@ launch_args_parse(int argc, char** argv)
 		{ "tx-offloads",		1, 0, 0 },
 		{ "hot-plug",			0, 0, 0 },
 		{ "vxlan-gpe-port",		1, 0, 0 },
+		{ "pf-crc-keep",		0, 0, 0 },
 		{ "mlockall",			0, 0, 0 },
 		{ "no-mlockall",		0, 0, 0 },
 		{ 0, 0, 0, 0 },
@@ -1131,6 +1133,9 @@ launch_args_parse(int argc, char** argv)
 					rte_exit(EXIT_FAILURE,
 						 "vxlan-gpe-port must be >= 0\n");
 			}
+			if (!strcmp(lgopts[opt_idx].name, "pf-crc-keep")) {
+				rx_offloads_disable |= DEV_RX_OFFLOAD_CRC_STRIP;
+			}
 			if (!strcmp(lgopts[opt_idx].name, "print-event"))
 				if (parse_event_printing_config(optarg, 1)) {
 					rte_exit(EXIT_FAILURE,
@@ -1163,4 +1168,5 @@ launch_args_parse(int argc, char** argv)
 	/* Set offload configuration from command line parameters. */
 	rx_mode.offloads = rx_offloads;
 	tx_mode.offloads = tx_offloads;
+	rx_mode.offloads_disable = rx_offloads_disable;
 }
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index d3ce92f..c94328a 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -287,6 +287,8 @@ uint8_t rmv_interrupt = 1; /* enabled by default */
 
 uint8_t hot_plug = 0; /**< hotplug disabled by default. */
 
+uint64_t rx_offloads_disable = 0;  /**< rx offload enabled by default. */
+
 /*
  * Display or mask ether events
  * Default to all events except VF_MBOX
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 4fc30a8..d9734d3 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -313,6 +313,7 @@ extern uint32_t event_print_mask;
 /**< set by "--print-event xxxx" and "--mask-event xxxx parameters */
 extern uint8_t hot_plug; /**< enable by "--hot-plug" parameter */
 extern int do_mlockall; /**< set by "--mlockall" or "--no-mlockall" parameter */
+extern uint64_t rx_offloads_disable;
 
 #ifdef RTE_LIBRTE_IXGBE_BYPASS
 extern uint32_t bypass_timeout; /**< Store the NIC bypass watchdog timeout */
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 26b1927..25c1187 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -5007,17 +5007,17 @@ ixgbevf_dev_configure(struct rte_eth_dev *dev)
 	 * VF has no ability to enable/disable HW CRC
 	 * Keep the persistent behavior the same as Host PF
 	 */
-#ifndef RTE_LIBRTE_IXGBE_PF_DISABLE_STRIP_CRC
-	if (rte_eth_dev_must_keep_crc(conf->rxmode.offloads)) {
-		PMD_INIT_LOG(NOTICE, "VF can't disable HW CRC Strip");
-		conf->rxmode.offloads |= DEV_RX_OFFLOAD_CRC_STRIP;
-	}
-#else
-	if (!rte_eth_dev_must_keep_crc(conf->rxmode.offloads)) {
-		PMD_INIT_LOG(NOTICE, "VF can't enable HW CRC Strip");
-		conf->rxmode.offloads &= ~DEV_RX_OFFLOAD_CRC_STRIP;
+	if (conf->rxmode.offloads_disable & DEV_RX_OFFLOAD_CRC_STRIP) {
+		if (rte_eth_dev_must_keep_crc(conf->rxmode.offloads)) {
+			PMD_INIT_LOG(NOTICE, "VF can't disable HW CRC Strip");
+			conf->rxmode.offloads |= DEV_RX_OFFLOAD_CRC_STRIP;
+		}
+	} else {
+		if (!rte_eth_dev_must_keep_crc(conf->rxmode.offloads)) {
+			PMD_INIT_LOG(NOTICE, "VF can't enable HW CRC Strip");
+			conf->rxmode.offloads &= ~DEV_RX_OFFLOAD_CRC_STRIP;
+		}
 	}
-#endif
 
 	/*
 	 * Initialize to TRUE. If any of Rx queues doesn't meet the bulk
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index f5f593b..35a2dd1 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -334,6 +334,7 @@ struct rte_eth_rxmode {
 	 * structure are allowed to be set.
 	 */
 	uint64_t offloads;
+	uint64_t offloads_disable;
 };
 
 /**
-- 
2.7.5

Re: [dpdk-dev] [PATCH] net/ixgbe: remove hardcoded CRC STRIP config from ixgbe

[Zhang, Qi Z]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wei Zhao
> Sent: Tuesday, July 24, 2018 10:37 AM
> To: dev@dpdk.org
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; stable@dpdk.org; Zhao1, Wei
> <wei.zhao1@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: [dpdk-dev] [PATCH] net/ixgbe: remove hardcoded CRC STRIP config
> from ixgbe
> 
> There is CRC related ifdefs for ixgbe:
> CONFIG_RTE_LIBRTE_IXGBE_PF_DISABLE_STRIP_CRC=n
> It is used in VF drivers ixgbevf_dev_configure() functions.
> VF cannot change the CRC strip behavior and based on what PF configured it
> needs to response proper to user
> ixgbevf_dev_configure() request. Right now what PF set is defined by above
> config options but this method is too static.
> 
> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> ---
>  app/test-pmd/parameters.c        |  6 ++++++
>  app/test-pmd/testpmd.c           |  2 ++
>  app/test-pmd/testpmd.h           |  1 +
>  drivers/net/ixgbe/ixgbe_ethdev.c | 20 ++++++++++----------
>  lib/librte_ethdev/rte_ethdev.h   |  1 +
>  5 files changed, 20 insertions(+), 10 deletions(-)

Its better separate the patch for ethdev, ixgbe, and testpmd.

Re: [dpdk-dev] [PATCH] net/ixgbe: remove hardcoded CRC STRIP config from ixgbe

[Zhao1, Wei]

Hi,  Ferruh
	What do you think of the patch implementation?

> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Thursday, August 2, 2018 10:28 AM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; stable@dpdk.org; Zhao1, Wei
> <wei.zhao1@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: RE: [dpdk-dev] [PATCH] net/ixgbe: remove hardcoded CRC STRIP
> config from ixgbe
> 
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wei Zhao
> > Sent: Tuesday, July 24, 2018 10:37 AM
> > To: dev@dpdk.org
> > Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; stable@dpdk.org; Zhao1,
> > Wei <wei.zhao1@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> > Subject: [dpdk-dev] [PATCH] net/ixgbe: remove hardcoded CRC STRIP
> > config from ixgbe
> >
> > There is CRC related ifdefs for ixgbe:
> > CONFIG_RTE_LIBRTE_IXGBE_PF_DISABLE_STRIP_CRC=n
> > It is used in VF drivers ixgbevf_dev_configure() functions.
> > VF cannot change the CRC strip behavior and based on what PF
> > configured it needs to response proper to user
> > ixgbevf_dev_configure() request. Right now what PF set is defined by
> > above config options but this method is too static.
> >
> > Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> > Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> > ---
> >  app/test-pmd/parameters.c        |  6 ++++++
> >  app/test-pmd/testpmd.c           |  2 ++
> >  app/test-pmd/testpmd.h           |  1 +
> >  drivers/net/ixgbe/ixgbe_ethdev.c | 20 ++++++++++----------
> >  lib/librte_ethdev/rte_ethdev.h   |  1 +
> >  5 files changed, 20 insertions(+), 10 deletions(-)
> 
> Its better separate the patch for ethdev, ixgbe, and testpmd.
> 

Re: [dpdk-dev] [PATCH] net/ixgbe: remove hardcoded CRC STRIP config from ixgbe

[Ferruh Yigit]

On 7/24/2018 3:36 AM, Wei Zhao wrote:
> There is CRC related ifdefs for ixgbe:
> CONFIG_RTE_LIBRTE_IXGBE_PF_DISABLE_STRIP_CRC=n
> It is used in VF drivers ixgbevf_dev_configure() functions.
> VF cannot change the CRC strip behavior and based on what PF
> configured it needs to response proper to user
> ixgbevf_dev_configure() request. Right now what PF set is
> defined by above config options but this method is too static.
> 
> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>

<...>
> @@ -334,6 +334,7 @@ struct rte_eth_rxmode {
>  	 * structure are allowed to be set.
>  	 */
>  	uint64_t offloads;
> +	uint64_t offloads_disable;

Do we need a disable flag in ethdev, an offload not enabled is disabled by
default isn't it. This conflicts with offloads flag and makes is confusing.

For igb/e1000 VF case, VF driver can't change what PF set and VF driver can't
learn the PF setting dynamically, so this information needs to be passed to VF
driver by application/user.
Currently this information passed by compile time config option, my suggestion
was using devargs.

In your implementation testpmd parameter added to get this information and pass
to driver, but this means all applications needs to do this, instead adding this
support to driver looks better to me.

Re: [dpdk-dev] [PATCH] net/ixgbe: remove hardcoded CRC STRIP config from ixgbe

[Shahaf Shuler]

Thursday, August 9, 2018 11:32 AM, Ferruh Yigit:
> Subject: Re: [PATCH] net/ixgbe: remove hardcoded CRC STRIP config from
> ixgbe
> 
> On 7/24/2018 3:36 AM, Wei Zhao wrote:
> > There is CRC related ifdefs for ixgbe:
> > CONFIG_RTE_LIBRTE_IXGBE_PF_DISABLE_STRIP_CRC=n
> > It is used in VF drivers ixgbevf_dev_configure() functions.
> > VF cannot change the CRC strip behavior and based on what PF
> > configured it needs to response proper to user
> > ixgbevf_dev_configure() request. Right now what PF set is defined by
> > above config options but this method is too static.
> >
> > Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> > Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> 
> <...>
> > @@ -334,6 +334,7 @@ struct rte_eth_rxmode {
> >  	 * structure are allowed to be set.
> >  	 */
> >  	uint64_t offloads;
> > +	uint64_t offloads_disable;
> 
> Do we need a disable flag in ethdev, an offload not enabled is disabled by
> default isn't it. This conflicts with offloads flag and makes is confusing.

+1.

**all** the offloads are disabled by default. 

> 
> For igb/e1000 VF case, VF driver can't change what PF set and VF driver can't
> learn the PF setting dynamically, so this information needs to be passed to VF
> driver by application/user.
> Currently this information passed by compile time config option, my
> suggestion was using devargs.
> 
> In your implementation testpmd parameter added to get this information
> and pass to driver, but this means all applications needs to do this, instead
> adding this support to driver looks better to me.

Re: [dpdk-dev] [PATCH] net/ixgbe: remove hardcoded CRC STRIP config from ixgbe

[Andrew Rybchenko]

On 12.08.2018 09:28, Shahaf Shuler wrote:
> Thursday, August 9, 2018 11:32 AM, Ferruh Yigit:
>> Subject: Re: [PATCH] net/ixgbe: remove hardcoded CRC STRIP config from
>> ixgbe
>>
>> On 7/24/2018 3:36 AM, Wei Zhao wrote:
>>> There is CRC related ifdefs for ixgbe:
>>> CONFIG_RTE_LIBRTE_IXGBE_PF_DISABLE_STRIP_CRC=n
>>> It is used in VF drivers ixgbevf_dev_configure() functions.
>>> VF cannot change the CRC strip behavior and based on what PF
>>> configured it needs to response proper to user
>>> ixgbevf_dev_configure() request. Right now what PF set is defined by
>>> above config options but this method is too static.
>>>
>>> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
>>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
>> <...>
>>> @@ -334,6 +334,7 @@ struct rte_eth_rxmode {
>>>   	 * structure are allowed to be set.
>>>   	 */
>>>   	uint64_t offloads;
>>> +	uint64_t offloads_disable;
>> Do we need a disable flag in ethdev, an offload not enabled is disabled by
>> default isn't it. This conflicts with offloads flag and makes is confusing.
> +1.
>
> **all** the offloads are disabled by default.
>
>> For igb/e1000 VF case, VF driver can't change what PF set and VF driver can't
>> learn the PF setting dynamically, so this information needs to be passed to VF
>> driver by application/user.
>> Currently this information passed by compile time config option, my
>> suggestion was using devargs.
>>
>> In your implementation testpmd parameter added to get this information
>> and pass to driver, but this means all applications needs to do this, instead
>> adding this support to driver looks better to me.

I think we should add fixed offloads to dev_info. I.e. if offlload is 
supported,
it could be marked as fixed (i.e. always enabled).
If offload is not supported, it is always disabled (and cannot/should not be
marked as fixed).
May be the right name for it is not "fixed", but "always_enabled".
Also it should be persistent. It should not be allowed in above ixgbe case
to change the offload state on PF if there are other users (drivers 
attached).
Otherwise, we need mechanism to notify apps about these changes -
overcomplicated.

Re: [dpdk-dev] [PATCH] net/ixgbe: remove hardcoded CRC STRIP config from ixgbe

[Shahaf Shuler]

Sunday, August 12, 2018 10:53 AM, Andrew Rybchenko:
> Subject: Re: [PATCH] net/ixgbe: remove hardcoded CRC STRIP config from
> ixgbe
> 
> On 12.08.2018 09:28, Shahaf Shuler wrote:
> > Thursday, August 9, 2018 11:32 AM, Ferruh Yigit:
> >> Subject: Re: [PATCH] net/ixgbe: remove hardcoded CRC STRIP config
> >> from ixgbe
> >>
> >> On 7/24/2018 3:36 AM, Wei Zhao wrote:
> >>> There is CRC related ifdefs for ixgbe:
> >>> CONFIG_RTE_LIBRTE_IXGBE_PF_DISABLE_STRIP_CRC=n
> >>> It is used in VF drivers ixgbevf_dev_configure() functions.
> >>> VF cannot change the CRC strip behavior and based on what PF
> >>> configured it needs to response proper to user
> >>> ixgbevf_dev_configure() request. Right now what PF set is defined by
> >>> above config options but this method is too static.
> >>>
> >>> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> >>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> >> <...>
> >>> @@ -334,6 +334,7 @@ struct rte_eth_rxmode {
> >>>   	 * structure are allowed to be set.
> >>>   	 */
> >>>   	uint64_t offloads;
> >>> +	uint64_t offloads_disable;
> >> Do we need a disable flag in ethdev, an offload not enabled is
> >> disabled by default isn't it. This conflicts with offloads flag and makes is
> confusing.
> > +1.
> >
> > **all** the offloads are disabled by default.
> >
> >> For igb/e1000 VF case, VF driver can't change what PF set and VF
> >> driver can't learn the PF setting dynamically, so this information
> >> needs to be passed to VF driver by application/user.
> >> Currently this information passed by compile time config option, my
> >> suggestion was using devargs.
> >>
> >> In your implementation testpmd parameter added to get this
> >> information and pass to driver, but this means all applications needs
> >> to do this, instead adding this support to driver looks better to me.
> 
> I think we should add fixed offloads to dev_info. I.e. if offlload is supported,
> it could be marked as fixed (i.e. always enabled).
> If offload is not supported, it is always disabled (and cannot/should not be
> marked as fixed).
> May be the right name for it is not "fixed", but "always_enabled".

I think it will over complicate applications. Those limitation should be expressed as part of the "limitation" section of the corresponding PMD guide. 

PMDs with such limitation can also put some warning message to notify or even fail the device configuration if the needed permanent offload is not set by the application. 

> Also it should be persistent. It should not be allowed in above ixgbe case to
> change the offload state on PF if there are other users (drivers attached).
> Otherwise, we need mechanism to notify apps about these changes -
> overcomplicated.

Re: [dpdk-dev] [PATCH] net/ixgbe: remove hardcoded CRC STRIP config from ixgbe

[Ferruh Yigit]

On 8/12/2018 9:46 AM, Shahaf Shuler wrote:
> Sunday, August 12, 2018 10:53 AM, Andrew Rybchenko:
>> Subject: Re: [PATCH] net/ixgbe: remove hardcoded CRC STRIP config from
>> ixgbe
>>
>> On 12.08.2018 09:28, Shahaf Shuler wrote:
>>> Thursday, August 9, 2018 11:32 AM, Ferruh Yigit:
>>>> Subject: Re: [PATCH] net/ixgbe: remove hardcoded CRC STRIP config
>>>> from ixgbe
>>>>
>>>> On 7/24/2018 3:36 AM, Wei Zhao wrote:
>>>>> There is CRC related ifdefs for ixgbe:
>>>>> CONFIG_RTE_LIBRTE_IXGBE_PF_DISABLE_STRIP_CRC=n
>>>>> It is used in VF drivers ixgbevf_dev_configure() functions.
>>>>> VF cannot change the CRC strip behavior and based on what PF
>>>>> configured it needs to response proper to user
>>>>> ixgbevf_dev_configure() request. Right now what PF set is defined by
>>>>> above config options but this method is too static.
>>>>>
>>>>> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
>>>>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
>>>> <...>
>>>>> @@ -334,6 +334,7 @@ struct rte_eth_rxmode {
>>>>>   	 * structure are allowed to be set.
>>>>>   	 */
>>>>>   	uint64_t offloads;
>>>>> +	uint64_t offloads_disable;
>>>> Do we need a disable flag in ethdev, an offload not enabled is
>>>> disabled by default isn't it. This conflicts with offloads flag and makes is
>> confusing.
>>> +1.
>>>
>>> **all** the offloads are disabled by default.
>>>
>>>> For igb/e1000 VF case, VF driver can't change what PF set and VF
>>>> driver can't learn the PF setting dynamically, so this information
>>>> needs to be passed to VF driver by application/user.
>>>> Currently this information passed by compile time config option, my
>>>> suggestion was using devargs.
>>>>
>>>> In your implementation testpmd parameter added to get this
>>>> information and pass to driver, but this means all applications needs
>>>> to do this, instead adding this support to driver looks better to me.
>>
>> I think we should add fixed offloads to dev_info. I.e. if offlload is supported,
>> it could be marked as fixed (i.e. always enabled).
>> If offload is not supported, it is always disabled (and cannot/should not be
>> marked as fixed).
>> May be the right name for it is not "fixed", but "always_enabled".
> 
> I think it will over complicate applications. Those limitation should be expressed as part of the "limitation" section of the corresponding PMD guide. 
> 
> PMDs with such limitation can also put some warning message to notify or even fail the device configuration if the needed permanent offload is not set by the application. 

Some PMDs already have these warning messages, for the case device supports an
offload, so it is in advertised capabilities, but doesn't support disabling it.

"Can't disable"/"always on" information from PMD is missing now, it would be
nice to get it from PMD but I agree that it will complicate things.

And this won't help the ixgbe VF case anyway, for that case if offload can be
enabled/disable in VF depends on PF configuration, so it is not a fixed
information for VF that you can put into driver code.

> 
>> Also it should be persistent. It should not be allowed in above ixgbe case to
>> change the offload state on PF if there are other users (drivers attached).
>> Otherwise, we need mechanism to notify apps about these changes -
>> overcomplicated.