[dpdk-dev] [PATCH] ethdev: add rx offload to drop error packets

[dpdk-dev] [PATCH] ethdev: add rx offload to drop error packets

[Nipun Gupta]

This change adds a RX offload capability where hardware can drop the
packets in case there is an error in the packet such as L3 checksum
error or L4 checksum.

Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
---
 lib/librte_ethdev/rte_ethdev.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index a49242bcd..bb9df2fb9 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1120,6 +1120,7 @@ struct rte_eth_conf {
 #define DEV_RX_OFFLOAD_SCTP_CKSUM	0x00020000
 #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM  0x00040000
 #define DEV_RX_OFFLOAD_RSS_HASH		0x00080000
+#define DEV_RX_OFFLOAD_ERR_PKT_DROP	0x00100000
 
 #define DEV_RX_OFFLOAD_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM | \
 				 DEV_RX_OFFLOAD_UDP_CKSUM | \
-- 
2.17.1

Re: [dpdk-dev] [PATCH] ethdev: add rx offload to drop error packets

[Ferruh Yigit]

On 8/31/2020 8:53 AM, Nipun Gupta wrote:
> This change adds a RX offload capability where hardware can drop the
> packets in case there is an error in the packet such as L3 checksum
> error or L4 checksum.
> 
> Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> ---
>  lib/librte_ethdev/rte_ethdev.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index a49242bcd..bb9df2fb9 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1120,6 +1120,7 @@ struct rte_eth_conf {
>  #define DEV_RX_OFFLOAD_SCTP_CKSUM	0x00020000
>  #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM  0x00040000
>  #define DEV_RX_OFFLOAD_RSS_HASH		0x00080000
> +#define DEV_RX_OFFLOAD_ERR_PKT_DROP	0x00100000
>  
>  #define DEV_RX_OFFLOAD_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM | \
>  				 DEV_RX_OFFLOAD_UDP_CKSUM | \
> 


Hi Nipun,

You adding a new offload type for network devices, it should have some more
description and relevant PMD/testpmd updates too.

But before that, is dropping faulty packets an offload or a config option for
the NIC? Or is this common for all NICs to have in ethdev?

Re: [dpdk-dev] [PATCH] ethdev: add rx offload to drop error packets

[Nipun Gupta]

Hi Ferruh,

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Monday, August 31, 2020 6:29 PM
> To: Nipun Gupta <nipun.gupta@nxp.com>; dev@dpdk.org
> Cc: thomas@monjalon.net; arybchenko@solarflare.com; Hemant Agrawal
> <hemant.agrawal@nxp.com>; Rohit Raj <rohit.raj@nxp.com>
> Subject: Re: [PATCH] ethdev: add rx offload to drop error packets
> 
> On 8/31/2020 8:53 AM, Nipun Gupta wrote:
> > This change adds a RX offload capability where hardware can drop the
> > packets in case there is an error in the packet such as L3 checksum
> > error or L4 checksum.
> >
> > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> > Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> > ---
> >  lib/librte_ethdev/rte_ethdev.h | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> > index a49242bcd..bb9df2fb9 100644
> > --- a/lib/librte_ethdev/rte_ethdev.h
> > +++ b/lib/librte_ethdev/rte_ethdev.h
> > @@ -1120,6 +1120,7 @@ struct rte_eth_conf {
> >  #define DEV_RX_OFFLOAD_SCTP_CKSUM	0x00020000
> >  #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM  0x00040000
> >  #define DEV_RX_OFFLOAD_RSS_HASH		0x00080000
> > +#define DEV_RX_OFFLOAD_ERR_PKT_DROP	0x00100000
> >
> >  #define DEV_RX_OFFLOAD_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM | \
> >  				 DEV_RX_OFFLOAD_UDP_CKSUM | \
> >
> 
> 
> Hi Nipun,
> 
> You adding a new offload type for network devices, it should have some more
> description and relevant PMD/testpmd updates too.

I should have added an RFC tag in the patch. I sent this for comment for error packet drop
offload support. I will send patches after proper implementation.

> 
> But before that, is dropping faulty packets an offload or a config option for
> the NIC? Or is this common for all NICs to have in ethdev?

We would like to have it as an offload capability, and on the basis of this capability
user can configure the Ethernet device to drop the error packets on the device itself
without providing them to the core.
Sorry I did not get your comment "Or is this common for all NICs to have in ethdev?"

Thanks,
Nipun

Re: [dpdk-dev] [PATCH] ethdev: add rx offload to drop error packets

[Stephen Hemminger]

On Mon, 31 Aug 2020 13:23:33 +0530
Nipun Gupta <nipun.gupta@nxp.com> wrote:

> This change adds a RX offload capability where hardware can drop the
> packets in case there is an error in the packet such as L3 checksum
> error or L4 checksum.
> 
> Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> ---
>  lib/librte_ethdev/rte_ethdev.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index a49242bcd..bb9df2fb9 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1120,6 +1120,7 @@ struct rte_eth_conf {
>  #define DEV_RX_OFFLOAD_SCTP_CKSUM	0x00020000
>  #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM  0x00040000
>  #define DEV_RX_OFFLOAD_RSS_HASH		0x00080000
> +#define DEV_RX_OFFLOAD_ERR_PKT_DROP	0x00100000
>  
>  #define DEV_RX_OFFLOAD_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM | \
>  				 DEV_RX_OFFLOAD_UDP_CKSUM | \

Could/should this be an rte_flow action as well?

Re: [dpdk-dev] [PATCH] ethdev: add rx offload to drop error packets

[Thomas Monjalon]

31/08/2020 19:00, Stephen Hemminger:
> On Mon, 31 Aug 2020 13:23:33 +0530
> Nipun Gupta <nipun.gupta@nxp.com> wrote:
> 
> > This change adds a RX offload capability where hardware can drop the
> > packets in case there is an error in the packet such as L3 checksum
> > error or L4 checksum.
> > 
> > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> > Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> > ---
> > +#define DEV_RX_OFFLOAD_ERR_PKT_DROP	0x00100000

Please add RTE_ prefix, even if older macros don't have it.
We could (in a separate effort) alias old ones with RTE_ prefixed names.

> Could/should this be an rte_flow action as well?

I feel rte_flow API is not appropriate here.
Ori, any opinion?

Re: [dpdk-dev] [PATCH] ethdev: add rx offload to drop error packets

[Nipun Gupta]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday, September 1, 2020 1:39 PM
> To: Nipun Gupta <nipun.gupta@nxp.com>; Stephen Hemminger
> <stephen@networkplumber.org>; orika@mellanox.com
> Cc: dev@dpdk.org; ferruh.yigit@intel.com; arybchenko@solarflare.com;
> Hemant Agrawal <hemant.agrawal@nxp.com>; Rohit Raj <rohit.raj@nxp.com>;
> olivier.matz@6wind.com
> Subject: Re: [dpdk-dev] [PATCH] ethdev: add rx offload to drop error packets
> 
> 31/08/2020 19:00, Stephen Hemminger:
> > On Mon, 31 Aug 2020 13:23:33 +0530
> > Nipun Gupta <nipun.gupta@nxp.com> wrote:
> >
> > > This change adds a RX offload capability where hardware can drop the
> > > packets in case there is an error in the packet such as L3 checksum
> > > error or L4 checksum.
> > >
> > > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> > > Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> > > ---
> > > +#define DEV_RX_OFFLOAD_ERR_PKT_DROP	0x00100000
> 
> Please add RTE_ prefix, even if older macros don't have it.
> We could (in a separate effort) alias old ones with RTE_ prefixed names.

Agree, will update and send the change along with testpmd and dpaa2 driver change.

> 
> > Could/should this be an rte_flow action as well?
> 
> I feel rte_flow API is not appropriate here.
> Ori, any opinion?
> 

Re: [dpdk-dev] [PATCH] ethdev: add rx offload to drop error packets

[Ori Kam]

Hi

> -----Original Message-----
> Subject: Re: [dpdk-dev] [PATCH] ethdev: add rx offload to drop error packets
> 
> 31/08/2020 19:00, Stephen Hemminger:
> > On Mon, 31 Aug 2020 13:23:33 +0530
> > Nipun Gupta <nipun.gupta@nxp.com> wrote:
> >
> > > This change adds a RX offload capability where hardware can drop the
> > > packets in case there is an error in the packet such as L3 checksum
> > > error or L4 checksum.
> > >
> > > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> > > Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> > > ---
> > > +#define DEV_RX_OFFLOAD_ERR_PKT_DROP	0x00100000
> 
> Please add RTE_ prefix, even if older macros don't have it.
> We could (in a separate effort) alias old ones with RTE_ prefixed names.
> 
> > Could/should this be an rte_flow action as well?
> 
> I feel rte_flow API is not appropriate here.
> Ori, any opinion?
> 
I also don't think this is relevant action in case of rte_flow,
If we want to support such a thing in rte_flow we should 
add item that matches on error and then use the drop action.

Best,
Ori

[dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error packets

[nipun.gupta]

From: Nipun Gupta <nipun.gupta@nxp.com>

This change adds a RX offload capability, which once enabled,
hardware will drop the packets in case there of any error in
the packet such as L3 checksum error or L4 checksum.

Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
---
These patches are based over series:
https://patchwork.dpdk.org/patch/78630/

Changes in v2:
 - Add support in DPAA1 driver (patch 2/3)
 - Add support and config parameter in testpmd (patch 3/3)

 lib/librte_ethdev/rte_ethdev.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index 645a18664..314d06c74 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1260,6 +1260,7 @@ struct rte_eth_conf {
 #define DEV_RX_OFFLOAD_SCTP_CKSUM	0x00020000
 #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM  0x00040000
 #define DEV_RX_OFFLOAD_RSS_HASH		0x00080000
+#define DEV_RX_OFFLOAD_ERR_PKT_DROP	0x00100000
 
 #define DEV_RX_OFFLOAD_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM | \
 				 DEV_RX_OFFLOAD_UDP_CKSUM | \
-- 
2.17.1

[dpdk-dev] [PATCH 2/3 v2] net/dpaa: support RX offload for error packet drop

[nipun.gupta]

From: Nipun Gupta <nipun.gupta@nxp.com>

This patch supports RX offload capability DEV_RX_OFFLOAD_ERR_PKT_DROP,
to drop error packets in the hardware

Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
---
 drivers/bus/dpaa/base/fman/fman_hw.c |  7 ++++++-
 drivers/net/dpaa/dpaa_ethdev.c       | 18 ++++++++++++++----
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/bus/dpaa/base/fman/fman_hw.c b/drivers/bus/dpaa/base/fman/fman_hw.c
index 4ab49f785..40e4e0000 100644
--- a/drivers/bus/dpaa/base/fman/fman_hw.c
+++ b/drivers/bus/dpaa/base/fman/fman_hw.c
@@ -597,7 +597,12 @@ void
 fman_if_discard_rx_errors(struct fman_if *fm_if)
 {
 	struct __fman_if *__if = container_of(fm_if, struct __fman_if, __if);
-	unsigned int *fmbm_rfsdm, *fmbm_rfsem;
+	unsigned int *fmbm_rcfg, *fmbm_rfsdm, *fmbm_rfsem;
+	unsigned int val;
+
+	fmbm_rcfg = &((struct rx_bmi_regs *)__if->bmi_map)->fmbm_rcfg;
+	val = in_be32(fmbm_rcfg);
+	out_be32(fmbm_rcfg, val & ~BMI_PORT_CFG_FDOVR);
 
 	fmbm_rfsem = &((struct rx_bmi_regs *)__if->bmi_map)->fmbm_rfsem;
 	out_be32(fmbm_rfsem, 0);
diff --git a/drivers/net/dpaa/dpaa_ethdev.c b/drivers/net/dpaa/dpaa_ethdev.c
index 1f94fc9b9..39f673db5 100644
--- a/drivers/net/dpaa/dpaa_ethdev.c
+++ b/drivers/net/dpaa/dpaa_ethdev.c
@@ -52,7 +52,8 @@
 /* Supported Rx offloads */
 static uint64_t dev_rx_offloads_sup =
 		DEV_RX_OFFLOAD_JUMBO_FRAME |
-		DEV_RX_OFFLOAD_SCATTER;
+		DEV_RX_OFFLOAD_SCATTER |
+		DEV_RX_OFFLOAD_ERR_PKT_DROP;
 
 /* Rx offloads which cannot be disabled */
 static uint64_t dev_rx_offloads_nodis =
@@ -262,6 +263,18 @@ dpaa_eth_dev_configure(struct rte_eth_dev *dev)
 		dev->data->scattered_rx = 1;
 	}
 
+	if (!(rx_offloads & DEV_RX_OFFLOAD_ERR_PKT_DROP)) {
+		struct dpaa_if *dpaa_intf = dev->data->dev_private;
+		struct qman_fq *rxq = &dpaa_intf->rx_queues[0];
+
+		DPAA_PMD_DEBUG("error packets will not be droppped on hw");
+		fman_if_receive_rx_errors(fif, FM_FD_RX_STATUS_ERR_MASK);
+		fman_if_set_err_fqid(fif, rxq->fqid);
+	} else {
+		DPAA_PMD_DEBUG("error packets will be droppped on hw");
+		fman_if_discard_rx_errors(fif);
+	}
+
 	if (!(default_q || fmc_q)) {
 		if (dpaa_fm_config(dev,
 			eth_conf->rx_adv_conf.rss_conf.rss_hf)) {
@@ -2031,9 +2044,6 @@ dpaa_dev_init(struct rte_eth_dev *eth_dev)
 		fman_intf->mac_addr.addr_bytes[5]);
 
 	if (!fman_intf->is_shared_mac) {
-		/* Configure error packet handling */
-		fman_if_receive_rx_errors(fman_intf,
-			FM_FD_RX_STATUS_ERR_MASK);
 		/* Disable RX mode */
 		fman_if_disable_rx(fman_intf);
 		/* Disable promiscuous mode */
-- 
2.17.1

[dpdk-dev] [PATCH 3/3 v2] testpmd: support hardware offload to drop error packets

[nipun.gupta]

From: Nipun Gupta <nipun.gupta@nxp.com>

With DEV_RX_OFFLOAD_ERR_PKT_DROP now defined as an offload
capability, testpmd showcases this with a new added configuration
option 'enable-hw-drop-err'.

Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
---
 app/test-pmd/parameters.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 1ead59579..ada870e2d 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -142,6 +142,7 @@ usage(char* progname)
 	printf("  --enable-hw-vlan-strip: enable hardware vlan strip.\n");
 	printf("  --enable-hw-vlan-extend: enable hardware vlan extend.\n");
 	printf("  --enable-hw-qinq-strip: enable hardware qinq strip.\n");
+	printf("  --enable-hw-drop-err: enable hardware packet drop for error packets.\n");
 	printf("  --enable-drop-en: enable per queue packet drop.\n");
 	printf("  --disable-rss: disable rss.\n");
 	printf("  --port-topology=<paired|chained|loop>: set port topology (paired "
@@ -631,6 +632,7 @@ launch_args_parse(int argc, char** argv)
 		{ "enable-hw-vlan-strip",       0, 0, 0 },
 		{ "enable-hw-vlan-extend",      0, 0, 0 },
 		{ "enable-hw-qinq-strip",       0, 0, 0 },
+		{ "enable-hw-drop-err",         0, 0, 0 },
 		{ "enable-drop-en",            0, 0, 0 },
 		{ "disable-rss",                0, 0, 0 },
 		{ "port-topology",              1, 0, 0 },
@@ -1037,6 +1039,9 @@ launch_args_parse(int argc, char** argv)
 					"enable-hw-qinq-strip"))
 				rx_offloads |= DEV_RX_OFFLOAD_QINQ_STRIP;
 
+			if (!strcmp(lgopts[opt_idx].name, "enable-hw-drop-err"))
+				rx_offloads |= DEV_RX_OFFLOAD_ERR_PKT_DROP;
+
 			if (!strcmp(lgopts[opt_idx].name, "enable-drop-en"))
 				rx_drop_en = 1;
 
-- 
2.17.1

Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error packets

[Stephen Hemminger]

On Mon,  5 Oct 2020 12:45:04 +0530
nipun.gupta@nxp.com wrote:

> From: Nipun Gupta <nipun.gupta@nxp.com>
> 
> This change adds a RX offload capability, which once enabled,
> hardware will drop the packets in case there of any error in
> the packet such as L3 checksum error or L4 checksum.
> 
> Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> ---
> These patches are based over series:
> https://patchwork.dpdk.org/patch/78630/
> 
> Changes in v2:
>  - Add support in DPAA1 driver (patch 2/3)
>  - Add support and config parameter in testpmd (patch 3/3)
> 
>  lib/librte_ethdev/rte_ethdev.h | 1 +
>  1 file changed, 1 insertion(+)

Maybe this should be an rte_flow match/action which would then make it
more flexible?

There is not much of a performance gain for this in real life and
if only one driver supports it then I am not convinced this is needed.

Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error packets

[Jerin Jacob]

On Mon, Oct 5, 2020 at 9:05 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Mon,  5 Oct 2020 12:45:04 +0530
> nipun.gupta@nxp.com wrote:
>
> > From: Nipun Gupta <nipun.gupta@nxp.com>
> >
> > This change adds a RX offload capability, which once enabled,
> > hardware will drop the packets in case there of any error in
> > the packet such as L3 checksum error or L4 checksum.

IMO, Providing additional support up to the level to choose the errors
to drops give more control to the application. Meaning,
L1 errors such as FCS error
L2 errors ..
L3 errors such checksum
i.e ethdev spec need to have  error level supported by PMD and the
application can set the layers interested to drop.

> >
> > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> > Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> > ---
> > These patches are based over series:
> > https://patchwork.dpdk.org/patch/78630/
> >
> > Changes in v2:
> >  - Add support in DPAA1 driver (patch 2/3)
> >  - Add support and config parameter in testpmd (patch 3/3)
> >
> >  lib/librte_ethdev/rte_ethdev.h | 1 +
> >  1 file changed, 1 insertion(+)
>
> Maybe this should be an rte_flow match/action which would then make it
> more flexible?

I think, it is not based on any Patten matching. So IMO, it should be best if it
is part of RX offload.

>
> There is not much of a performance gain for this in real life and
> if only one driver supports it then I am not convinced this is needed.

Marvell HW has this feature.

Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error packets

[Nipun Gupta]

> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Monday, October 5, 2020 9:40 PM
> To: Stephen Hemminger <stephen@networkplumber.org>
> Cc: Nipun Gupta <nipun.gupta@nxp.com>; dpdk-dev <dev@dpdk.org>; Thomas
> Monjalon <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>;
> Andrew Rybchenko <arybchenko@solarflare.com>; Hemant Agrawal
> <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>; Rohit
> Raj <rohit.raj@nxp.com>
> Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error
> packets
> 
> On Mon, Oct 5, 2020 at 9:05 PM Stephen Hemminger
> <stephen@networkplumber.org> wrote:
> >
> > On Mon,  5 Oct 2020 12:45:04 +0530
> > nipun.gupta@nxp.com wrote:
> >
> > > From: Nipun Gupta <nipun.gupta@nxp.com>
> > >
> > > This change adds a RX offload capability, which once enabled,
> > > hardware will drop the packets in case there of any error in
> > > the packet such as L3 checksum error or L4 checksum.
> 
> IMO, Providing additional support up to the level to choose the errors
> to drops give more control to the application. Meaning,
> L1 errors such as FCS error
> L2 errors ..
> L3 errors such checksum
> i.e ethdev spec need to have  error level supported by PMD and the
> application can set the layers interested to drop.

Agree, but 'DEV_RX_OFFLOAD_ERR_PKT_DROP' shall also be there to drop all the 
error packets? Maybe we can rename it to DEV_RX_OFFLOAD_ALL_ERR_PKT_DROP.

Currently we have not planned to add separate knobs for separate error in
the driver, maybe we can define them separately, or we need have them in
this series itself?

> 
> > >
> > > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> > > Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> > > ---
> > > These patches are based over series:
> > >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwo
> rk.dpdk.org%2Fpatch%2F78630%2F&amp;data=02%7C01%7Cnipun.gupta%40nx
> p.com%7C90b516fd465c48945e7008d869492b3e%7C686ea1d3bc2b4c6fa92cd9
> 9c5c301635%7C0%7C0%7C637375110263097933&amp;sdata=RBQswMBsfpM6
> nyKur%2FaHvOMvNK7RU%2BRyhHt%2FXBsP1OM%3D&amp;reserved=0
> > >
> > > Changes in v2:
> > >  - Add support in DPAA1 driver (patch 2/3)
> > >  - Add support and config parameter in testpmd (patch 3/3)
> > >
> > >  lib/librte_ethdev/rte_ethdev.h | 1 +
> > >  1 file changed, 1 insertion(+)
> >
> > Maybe this should be an rte_flow match/action which would then make it
> > more flexible?
> 
> I think, it is not based on any Patten matching. So IMO, it should be best if it
> is part of RX offload.
> 
> >
> > There is not much of a performance gain for this in real life and
> > if only one driver supports it then I am not convinced this is needed.
> 
> Marvell HW has this feature.

Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error packets

[Jerin Jacob]

On Tue, Oct 6, 2020 at 4:07 PM Nipun Gupta <nipun.gupta@nxp.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Jerin Jacob <jerinjacobk@gmail.com>
> > Sent: Monday, October 5, 2020 9:40 PM
> > To: Stephen Hemminger <stephen@networkplumber.org>
> > Cc: Nipun Gupta <nipun.gupta@nxp.com>; dpdk-dev <dev@dpdk.org>; Thomas
> > Monjalon <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>;
> > Andrew Rybchenko <arybchenko@solarflare.com>; Hemant Agrawal
> > <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>; Rohit
> > Raj <rohit.raj@nxp.com>
> > Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error
> > packets
> >
> > On Mon, Oct 5, 2020 at 9:05 PM Stephen Hemminger
> > <stephen@networkplumber.org> wrote:
> > >
> > > On Mon,  5 Oct 2020 12:45:04 +0530
> > > nipun.gupta@nxp.com wrote:
> > >
> > > > From: Nipun Gupta <nipun.gupta@nxp.com>
> > > >
> > > > This change adds a RX offload capability, which once enabled,
> > > > hardware will drop the packets in case there of any error in
> > > > the packet such as L3 checksum error or L4 checksum.
> >
> > IMO, Providing additional support up to the level to choose the errors
> > to drops give more control to the application. Meaning,
> > L1 errors such as FCS error
> > L2 errors ..
> > L3 errors such checksum
> > i.e ethdev spec need to have  error level supported by PMD and the
> > application can set the layers interested to drop.
>
> Agree, but 'DEV_RX_OFFLOAD_ERR_PKT_DROP' shall also be there to drop all the
> error packets? Maybe we can rename it to DEV_RX_OFFLOAD_ALL_ERR_PKT_DROP.

IMHO,  we introduce such shortcut for a single flag for all err drop
then we can not change the scheme
without an API/ABI break.

>
> Currently we have not planned to add separate knobs for separate error in
> the driver, maybe we can define them separately, or we need have them in
> this series itself?

I think, ethdev API can have the capability on what are levels it
supported, in your
driver case, you can express the same.


>
> >
> > > >
> > > > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> > > > Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> > > > ---
> > > > These patches are based over series:
> > > >
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwo
> > rk.dpdk.org%2Fpatch%2F78630%2F&amp;data=02%7C01%7Cnipun.gupta%40nx
> > p.com%7C90b516fd465c48945e7008d869492b3e%7C686ea1d3bc2b4c6fa92cd9
> > 9c5c301635%7C0%7C0%7C637375110263097933&amp;sdata=RBQswMBsfpM6
> > nyKur%2FaHvOMvNK7RU%2BRyhHt%2FXBsP1OM%3D&amp;reserved=0
> > > >
> > > > Changes in v2:
> > > >  - Add support in DPAA1 driver (patch 2/3)
> > > >  - Add support and config parameter in testpmd (patch 3/3)
> > > >
> > > >  lib/librte_ethdev/rte_ethdev.h | 1 +
> > > >  1 file changed, 1 insertion(+)
> > >
> > > Maybe this should be an rte_flow match/action which would then make it
> > > more flexible?
> >
> > I think, it is not based on any Patten matching. So IMO, it should be best if it
> > is part of RX offload.
> >
> > >
> > > There is not much of a performance gain for this in real life and
> > > if only one driver supports it then I am not convinced this is needed.
> >
> > Marvell HW has this feature.

Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error packets

[Nipun Gupta]

> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Tuesday, October 6, 2020 5:31 PM
> To: Nipun Gupta <nipun.gupta@nxp.com>
> Cc: Stephen Hemminger <stephen@networkplumber.org>; dpdk-dev
> <dev@dpdk.org>; Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
> <ferruh.yigit@intel.com>; Andrew Rybchenko <arybchenko@solarflare.com>;
> Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>
> Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error
> packets
> 
> On Tue, Oct 6, 2020 at 4:07 PM Nipun Gupta <nipun.gupta@nxp.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > Sent: Monday, October 5, 2020 9:40 PM
> > > To: Stephen Hemminger <stephen@networkplumber.org>
> > > Cc: Nipun Gupta <nipun.gupta@nxp.com>; dpdk-dev <dev@dpdk.org>;
> Thomas
> > > Monjalon <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>;
> > > Andrew Rybchenko <arybchenko@solarflare.com>; Hemant Agrawal
> > > <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>;
> Rohit
> > > Raj <rohit.raj@nxp.com>
> > > Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error
> > > packets
> > >
> > > On Mon, Oct 5, 2020 at 9:05 PM Stephen Hemminger
> > > <stephen@networkplumber.org> wrote:
> > > >
> > > > On Mon,  5 Oct 2020 12:45:04 +0530
> > > > nipun.gupta@nxp.com wrote:
> > > >
> > > > > From: Nipun Gupta <nipun.gupta@nxp.com>
> > > > >
> > > > > This change adds a RX offload capability, which once enabled,
> > > > > hardware will drop the packets in case there of any error in
> > > > > the packet such as L3 checksum error or L4 checksum.
> > >
> > > IMO, Providing additional support up to the level to choose the errors
> > > to drops give more control to the application. Meaning,
> > > L1 errors such as FCS error
> > > L2 errors ..
> > > L3 errors such checksum
> > > i.e ethdev spec need to have  error level supported by PMD and the
> > > application can set the layers interested to drop.
> >
> > Agree, but 'DEV_RX_OFFLOAD_ERR_PKT_DROP' shall also be there to drop all
> the
> > error packets? Maybe we can rename it to
> DEV_RX_OFFLOAD_ALL_ERR_PKT_DROP.
> 
> IMHO,  we introduce such shortcut for a single flag for all err drop
> then we can not change the scheme
> without an API/ABI break.

Are the following offloads fine:
	DEV_RX_OFFLOAD_L1_FCS_ERR_PKT_DROP
	DEV_RX_OFFLOAD_L3_CSUM_ERR_PKT_DROP
	DEV_RX_OFFLOAD_L4_CSUM_ERR_PKT_DROP
	DEV_RX_OFFLOAD_ALL_ERR_PKT_DROP

Please let me know in case I need to add any other too.
Ill send a v3.

Thanks,
Nipun

> 
> >
> > Currently we have not planned to add separate knobs for separate error in
> > the driver, maybe we can define them separately, or we need have them in
> > this series itself?
> 
> I think, ethdev API can have the capability on what are levels it
> supported, in your
> driver case, you can express the same.
> 
> 
> >
> > >
> > > > >
> > > > > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> > > > > Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> > > > > ---
> > > > > These patches are based over series:
> > > > >
> > >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwo
> > >
> rk.dpdk.org%2Fpatch%2F78630%2F&amp;data=02%7C01%7Cnipun.gupta%40nx
> > >
> p.com%7C90b516fd465c48945e7008d869492b3e%7C686ea1d3bc2b4c6fa92cd9
> > >
> 9c5c301635%7C0%7C0%7C637375110263097933&amp;sdata=RBQswMBsfpM6
> > > nyKur%2FaHvOMvNK7RU%2BRyhHt%2FXBsP1OM%3D&amp;reserved=0
> > > > >
> > > > > Changes in v2:
> > > > >  - Add support in DPAA1 driver (patch 2/3)
> > > > >  - Add support and config parameter in testpmd (patch 3/3)
> > > > >
> > > > >  lib/librte_ethdev/rte_ethdev.h | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > >
> > > > Maybe this should be an rte_flow match/action which would then make it
> > > > more flexible?
> > >
> > > I think, it is not based on any Patten matching. So IMO, it should be best if it
> > > is part of RX offload.
> > >
> > > >
> > > > There is not much of a performance gain for this in real life and
> > > > if only one driver supports it then I am not convinced this is needed.
> > >
> > > Marvell HW has this feature.

Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error packets

[Jerin Jacob]

On Tue, Oct 6, 2020 at 6:40 PM Nipun Gupta <nipun.gupta@nxp.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Jerin Jacob <jerinjacobk@gmail.com>
> > Sent: Tuesday, October 6, 2020 5:31 PM
> > To: Nipun Gupta <nipun.gupta@nxp.com>
> > Cc: Stephen Hemminger <stephen@networkplumber.org>; dpdk-dev
> > <dev@dpdk.org>; Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
> > <ferruh.yigit@intel.com>; Andrew Rybchenko <arybchenko@solarflare.com>;
> > Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> > <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>
> > Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error
> > packets
> >
> > On Tue, Oct 6, 2020 at 4:07 PM Nipun Gupta <nipun.gupta@nxp.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > > Sent: Monday, October 5, 2020 9:40 PM
> > > > To: Stephen Hemminger <stephen@networkplumber.org>
> > > > Cc: Nipun Gupta <nipun.gupta@nxp.com>; dpdk-dev <dev@dpdk.org>;
> > Thomas
> > > > Monjalon <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>;
> > > > Andrew Rybchenko <arybchenko@solarflare.com>; Hemant Agrawal
> > > > <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>;
> > Rohit
> > > > Raj <rohit.raj@nxp.com>
> > > > Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error
> > > > packets
> > > >
> > > > On Mon, Oct 5, 2020 at 9:05 PM Stephen Hemminger
> > > > <stephen@networkplumber.org> wrote:
> > > > >
> > > > > On Mon,  5 Oct 2020 12:45:04 +0530
> > > > > nipun.gupta@nxp.com wrote:
> > > > >
> > > > > > From: Nipun Gupta <nipun.gupta@nxp.com>
> > > > > >
> > > > > > This change adds a RX offload capability, which once enabled,
> > > > > > hardware will drop the packets in case there of any error in
> > > > > > the packet such as L3 checksum error or L4 checksum.
> > > >
> > > > IMO, Providing additional support up to the level to choose the errors
> > > > to drops give more control to the application. Meaning,
> > > > L1 errors such as FCS error
> > > > L2 errors ..
> > > > L3 errors such checksum
> > > > i.e ethdev spec need to have  error level supported by PMD and the
> > > > application can set the layers interested to drop.
> > >
> > > Agree, but 'DEV_RX_OFFLOAD_ERR_PKT_DROP' shall also be there to drop all
> > the
> > > error packets? Maybe we can rename it to
> > DEV_RX_OFFLOAD_ALL_ERR_PKT_DROP.
> >
> > IMHO,  we introduce such shortcut for a single flag for all err drop
> > then we can not change the scheme
> > without an API/ABI break.
>
> Are the following offloads fine:
>         DEV_RX_OFFLOAD_L1_FCS_ERR_PKT_DROP
>         DEV_RX_OFFLOAD_L3_CSUM_ERR_PKT_DROP
>         DEV_RX_OFFLOAD_L4_CSUM_ERR_PKT_DROP
>         DEV_RX_OFFLOAD_ALL_ERR_PKT_DROP
>
> Please let me know in case I need to add any other too.

I think, single offload flags and some config/capability structure to
define the additional
layer selection would be good, instead of adding a lot of new offload flags.


> Ill send a v3.
>
> Thanks,
> Nipun
>
> >
> > >
> > > Currently we have not planned to add separate knobs for separate error in
> > > the driver, maybe we can define them separately, or we need have them in
> > > this series itself?
> >
> > I think, ethdev API can have the capability on what are levels it
> > supported, in your
> > driver case, you can express the same.
> >
> >
> > >
> > > >
> > > > > >
> > > > > > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> > > > > > Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> > > > > > ---
> > > > > > These patches are based over series:
> > > > > >
> > > >
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwo
> > > >
> > rk.dpdk.org%2Fpatch%2F78630%2F&amp;data=02%7C01%7Cnipun.gupta%40nx
> > > >
> > p.com%7C90b516fd465c48945e7008d869492b3e%7C686ea1d3bc2b4c6fa92cd9
> > > >
> > 9c5c301635%7C0%7C0%7C637375110263097933&amp;sdata=RBQswMBsfpM6
> > > > nyKur%2FaHvOMvNK7RU%2BRyhHt%2FXBsP1OM%3D&amp;reserved=0
> > > > > >
> > > > > > Changes in v2:
> > > > > >  - Add support in DPAA1 driver (patch 2/3)
> > > > > >  - Add support and config parameter in testpmd (patch 3/3)
> > > > > >
> > > > > >  lib/librte_ethdev/rte_ethdev.h | 1 +
> > > > > >  1 file changed, 1 insertion(+)
> > > > >
> > > > > Maybe this should be an rte_flow match/action which would then make it
> > > > > more flexible?
> > > >
> > > > I think, it is not based on any Patten matching. So IMO, it should be best if it
> > > > is part of RX offload.
> > > >
> > > > >
> > > > > There is not much of a performance gain for this in real life and
> > > > > if only one driver supports it then I am not convinced this is needed.
> > > >
> > > > Marvell HW has this feature.

Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error packets

[Nipun Gupta]

> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Tuesday, October 6, 2020 6:44 PM
> To: Nipun Gupta <nipun.gupta@nxp.com>
> Cc: Stephen Hemminger <stephen@networkplumber.org>; dpdk-dev
> <dev@dpdk.org>; Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
> <ferruh.yigit@intel.com>; Andrew Rybchenko <arybchenko@solarflare.com>;
> Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>
> Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error
> packets
> 
> On Tue, Oct 6, 2020 at 6:40 PM Nipun Gupta <nipun.gupta@nxp.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > Sent: Tuesday, October 6, 2020 5:31 PM
> > > To: Nipun Gupta <nipun.gupta@nxp.com>
> > > Cc: Stephen Hemminger <stephen@networkplumber.org>; dpdk-dev
> > > <dev@dpdk.org>; Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
> > > <ferruh.yigit@intel.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>;
> > > Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> > > <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>
> > > Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error
> > > packets
> > >
> > > On Tue, Oct 6, 2020 at 4:07 PM Nipun Gupta <nipun.gupta@nxp.com> wrote:
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > > > Sent: Monday, October 5, 2020 9:40 PM
> > > > > To: Stephen Hemminger <stephen@networkplumber.org>
> > > > > Cc: Nipun Gupta <nipun.gupta@nxp.com>; dpdk-dev <dev@dpdk.org>;
> > > Thomas
> > > > > Monjalon <thomas@monjalon.net>; Ferruh Yigit
> <ferruh.yigit@intel.com>;
> > > > > Andrew Rybchenko <arybchenko@solarflare.com>; Hemant Agrawal
> > > > > <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>;
> > > Rohit
> > > > > Raj <rohit.raj@nxp.com>
> > > > > Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop
> error
> > > > > packets
> > > > >
> > > > > On Mon, Oct 5, 2020 at 9:05 PM Stephen Hemminger
> > > > > <stephen@networkplumber.org> wrote:
> > > > > >
> > > > > > On Mon,  5 Oct 2020 12:45:04 +0530
> > > > > > nipun.gupta@nxp.com wrote:
> > > > > >
> > > > > > > From: Nipun Gupta <nipun.gupta@nxp.com>
> > > > > > >
> > > > > > > This change adds a RX offload capability, which once enabled,
> > > > > > > hardware will drop the packets in case there of any error in
> > > > > > > the packet such as L3 checksum error or L4 checksum.
> > > > >
> > > > > IMO, Providing additional support up to the level to choose the errors
> > > > > to drops give more control to the application. Meaning,
> > > > > L1 errors such as FCS error
> > > > > L2 errors ..
> > > > > L3 errors such checksum
> > > > > i.e ethdev spec need to have  error level supported by PMD and the
> > > > > application can set the layers interested to drop.
> > > >
> > > > Agree, but 'DEV_RX_OFFLOAD_ERR_PKT_DROP' shall also be there to drop
> all
> > > the
> > > > error packets? Maybe we can rename it to
> > > DEV_RX_OFFLOAD_ALL_ERR_PKT_DROP.
> > >
> > > IMHO,  we introduce such shortcut for a single flag for all err drop
> > > then we can not change the scheme
> > > without an API/ABI break.
> >
> > Are the following offloads fine:
> >         DEV_RX_OFFLOAD_L1_FCS_ERR_PKT_DROP
> >         DEV_RX_OFFLOAD_L3_CSUM_ERR_PKT_DROP
> >         DEV_RX_OFFLOAD_L4_CSUM_ERR_PKT_DROP
> >         DEV_RX_OFFLOAD_ALL_ERR_PKT_DROP
> >
> > Please let me know in case I need to add any other too.
> 
> I think, single offload flags and some config/capability structure to
> define the additional
> layer selection would be good, instead of adding a lot of new offload flags.


+/**
+ * A structure used to enable/disable error packet drop on Rx.
+ */
+struct rte_rx_err_pkt_drop_conf {
+       /** enable/disable all RX error packet drop.
+        * 0 (default) - disable, 1 enable
+        */
+       uint32_t all:1;
+};
+
 /**
  * A structure used to configure an Ethernet port.
  * Depending upon the RX multi-queue mode, extra advanced
@@ -1236,6 +1246,8 @@ struct rte_eth_conf {
        uint32_t dcb_capability_en;
        struct rte_fdir_conf fdir_conf; /**< FDIR configuration. DEPRECATED */
        struct rte_intr_conf intr_conf; /**< Interrupt mode configuration. */
+       struct rte_rx_err_pkt_drop_conf err_pkt_drop_conf;
+       /**< RX error packet drop configuration. */

Is this the kind of changes you are talking about?

Also, more changes will be there in 'struct rte_eth_dev_info' structure, defining
additional separate capability something like 'uint64_t rx_err_drop_offload_capa'.

Regards,
Nipun

> 
> 
> > Ill send a v3.
> >
> > Thanks,
> > Nipun
> >
> > >
> > > >
> > > > Currently we have not planned to add separate knobs for separate error in
> > > > the driver, maybe we can define them separately, or we need have them in
> > > > this series itself?
> > >
> > > I think, ethdev API can have the capability on what are levels it
> > > supported, in your
> > > driver case, you can express the same.
> > >
> > >
> > > >
> > > > >
> > > > > > >
> > > > > > > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> > > > > > > Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> > > > > > > ---
> > > > > > > These patches are based over series:
> > > > > > >
> > > > >
> > >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwo
> > > > >
> > >
> rk.dpdk.org%2Fpatch%2F78630%2F&amp;data=02%7C01%7Cnipun.gupta%40nx
> > > > >
> > >
> p.com%7C90b516fd465c48945e7008d869492b3e%7C686ea1d3bc2b4c6fa92cd9
> > > > >
> > >
> 9c5c301635%7C0%7C0%7C637375110263097933&amp;sdata=RBQswMBsfpM6
> > > > > nyKur%2FaHvOMvNK7RU%2BRyhHt%2FXBsP1OM%3D&amp;reserved=0
> > > > > > >
> > > > > > > Changes in v2:
> > > > > > >  - Add support in DPAA1 driver (patch 2/3)
> > > > > > >  - Add support and config parameter in testpmd (patch 3/3)
> > > > > > >
> > > > > > >  lib/librte_ethdev/rte_ethdev.h | 1 +
> > > > > > >  1 file changed, 1 insertion(+)
> > > > > >
> > > > > > Maybe this should be an rte_flow match/action which would then make
> it
> > > > > > more flexible?
> > > > >
> > > > > I think, it is not based on any Patten matching. So IMO, it should be best
> if it
> > > > > is part of RX offload.
> > > > >
> > > > > >
> > > > > > There is not much of a performance gain for this in real life and
> > > > > > if only one driver supports it then I am not convinced this is needed.
> > > > >
> > > > > Marvell HW has this feature.

Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error packets

[Jerin Jacob]

On Thu, Oct 8, 2020 at 2:23 PM Nipun Gupta <nipun.gupta@nxp.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Jerin Jacob <jerinjacobk@gmail.com>
> > Sent: Tuesday, October 6, 2020 6:44 PM
> > To: Nipun Gupta <nipun.gupta@nxp.com>
> > Cc: Stephen Hemminger <stephen@networkplumber.org>; dpdk-dev
> > <dev@dpdk.org>; Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
> > <ferruh.yigit@intel.com>; Andrew Rybchenko <arybchenko@solarflare.com>;
> > Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> > <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>
> > Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error
> > packets
> >
> > On Tue, Oct 6, 2020 at 6:40 PM Nipun Gupta <nipun.gupta@nxp.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > > Sent: Tuesday, October 6, 2020 5:31 PM
> > > > To: Nipun Gupta <nipun.gupta@nxp.com>
> > > > Cc: Stephen Hemminger <stephen@networkplumber.org>; dpdk-dev
> > > > <dev@dpdk.org>; Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
> > > > <ferruh.yigit@intel.com>; Andrew Rybchenko
> > <arybchenko@solarflare.com>;
> > > > Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> > > > <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>
> > > > Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error
> > > > packets
> > > >
> > > > On Tue, Oct 6, 2020 at 4:07 PM Nipun Gupta <nipun.gupta@nxp.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > > > > Sent: Monday, October 5, 2020 9:40 PM
> > > > > > To: Stephen Hemminger <stephen@networkplumber.org>
> > > > > > Cc: Nipun Gupta <nipun.gupta@nxp.com>; dpdk-dev <dev@dpdk.org>;
> > > > Thomas
> > > > > > Monjalon <thomas@monjalon.net>; Ferruh Yigit
> > <ferruh.yigit@intel.com>;
> > > > > > Andrew Rybchenko <arybchenko@solarflare.com>; Hemant Agrawal
> > > > > > <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>;
> > > > Rohit
> > > > > > Raj <rohit.raj@nxp.com>
> > > > > > Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop
> > error
> > > > > > packets
> > > > > >
> > > > > > On Mon, Oct 5, 2020 at 9:05 PM Stephen Hemminger
> > > > > > <stephen@networkplumber.org> wrote:
> > > > > > >
> > > > > > > On Mon,  5 Oct 2020 12:45:04 +0530
> > > > > > > nipun.gupta@nxp.com wrote:
> > > > > > >
> > > > > > > > From: Nipun Gupta <nipun.gupta@nxp.com>
> > > > > > > >
> > > > > > > > This change adds a RX offload capability, which once enabled,
> > > > > > > > hardware will drop the packets in case there of any error in
> > > > > > > > the packet such as L3 checksum error or L4 checksum.
> > > > > >
> > > > > > IMO, Providing additional support up to the level to choose the errors
> > > > > > to drops give more control to the application. Meaning,
> > > > > > L1 errors such as FCS error
> > > > > > L2 errors ..
> > > > > > L3 errors such checksum
> > > > > > i.e ethdev spec need to have  error level supported by PMD and the
> > > > > > application can set the layers interested to drop.
> > > > >
> > > > > Agree, but 'DEV_RX_OFFLOAD_ERR_PKT_DROP' shall also be there to drop
> > all
> > > > the
> > > > > error packets? Maybe we can rename it to
> > > > DEV_RX_OFFLOAD_ALL_ERR_PKT_DROP.
> > > >
> > > > IMHO,  we introduce such shortcut for a single flag for all err drop
> > > > then we can not change the scheme
> > > > without an API/ABI break.
> > >
> > > Are the following offloads fine:
> > >         DEV_RX_OFFLOAD_L1_FCS_ERR_PKT_DROP
> > >         DEV_RX_OFFLOAD_L3_CSUM_ERR_PKT_DROP
> > >         DEV_RX_OFFLOAD_L4_CSUM_ERR_PKT_DROP
> > >         DEV_RX_OFFLOAD_ALL_ERR_PKT_DROP
> > >
> > > Please let me know in case I need to add any other too.
> >
> > I think, single offload flags and some config/capability structure to
> > define the additional
> > layer selection would be good, instead of adding a lot of new offload flags.
>
>
> +/**
> + * A structure used to enable/disable error packet drop on Rx.
> + */
> +struct rte_rx_err_pkt_drop_conf {
> +       /** enable/disable all RX error packet drop.
> +        * 0 (default) - disable, 1 enable
> +        */
> +       uint32_t all:1;
> +};
> +
>  /**
>   * A structure used to configure an Ethernet port.
>   * Depending upon the RX multi-queue mode, extra advanced
> @@ -1236,6 +1246,8 @@ struct rte_eth_conf {
>         uint32_t dcb_capability_en;
>         struct rte_fdir_conf fdir_conf; /**< FDIR configuration. DEPRECATED */
>         struct rte_intr_conf intr_conf; /**< Interrupt mode configuration. */
> +       struct rte_rx_err_pkt_drop_conf err_pkt_drop_conf;
> +       /**< RX error packet drop configuration. */
>
> Is this the kind of changes you are talking about?


Yes.

>
> Also, more changes will be there in 'struct rte_eth_dev_info' structure, defining
> additional separate capability something like 'uint64_t rx_err_drop_offload_capa'.
>
> Regards,
> Nipun
>
> >
> >
> > > Ill send a v3.
> > >
> > > Thanks,
> > > Nipun
> > >
> > > >
> > > > >
> > > > > Currently we have not planned to add separate knobs for separate error in
> > > > > the driver, maybe we can define them separately, or we need have them in
> > > > > this series itself?
> > > >
> > > > I think, ethdev API can have the capability on what are levels it
> > > > supported, in your
> > > > driver case, you can express the same.
> > > >
> > > >
> > > > >
> > > > > >
> > > > > > > >
> > > > > > > > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> > > > > > > > Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> > > > > > > > ---
> > > > > > > > These patches are based over series:
> > > > > > > >
> > > > > >
> > > >
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwo
> > > > > >
> > > >
> > rk.dpdk.org%2Fpatch%2F78630%2F&amp;data=02%7C01%7Cnipun.gupta%40nx
> > > > > >
> > > >
> > p.com%7C90b516fd465c48945e7008d869492b3e%7C686ea1d3bc2b4c6fa92cd9
> > > > > >
> > > >
> > 9c5c301635%7C0%7C0%7C637375110263097933&amp;sdata=RBQswMBsfpM6
> > > > > > nyKur%2FaHvOMvNK7RU%2BRyhHt%2FXBsP1OM%3D&amp;reserved=0
> > > > > > > >
> > > > > > > > Changes in v2:
> > > > > > > >  - Add support in DPAA1 driver (patch 2/3)
> > > > > > > >  - Add support and config parameter in testpmd (patch 3/3)
> > > > > > > >
> > > > > > > >  lib/librte_ethdev/rte_ethdev.h | 1 +
> > > > > > > >  1 file changed, 1 insertion(+)
> > > > > > >
> > > > > > > Maybe this should be an rte_flow match/action which would then make
> > it
> > > > > > > more flexible?
> > > > > >
> > > > > > I think, it is not based on any Patten matching. So IMO, it should be best
> > if it
> > > > > > is part of RX offload.
> > > > > >
> > > > > > >
> > > > > > > There is not much of a performance gain for this in real life and
> > > > > > > if only one driver supports it then I am not convinced this is needed.
> > > > > >
> > > > > > Marvell HW has this feature.

Re: [dpdk-dev] [PATCH 3/3 v2] testpmd: support hardware offload to drop error packets

[Asaf Penso]

>-----Original Message-----
>From: dev <dev-bounces@dpdk.org> On Behalf Of nipun.gupta@nxp.com
>Sent: Monday, October 5, 2020 10:15 AM
>To: dev@dpdk.org
>Cc: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>;
>ferruh.yigit@intel.com; arybchenko@solarflare.com;
>hemant.agrawal@nxp.com; sachin.saxena@nxp.com; rohit.raj@nxp.com;
>Nipun Gupta <nipun.gupta@nxp.com>
>Subject: [dpdk-dev] [PATCH 3/3 v2] testpmd: support hardware offload to
>drop error packets
>
>From: Nipun Gupta <nipun.gupta@nxp.com>
>
>With DEV_RX_OFFLOAD_ERR_PKT_DROP now defined as an offload
>capability, testpmd showcases this with a new added configuration option
>'enable-hw-drop-err'.
>
>Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
>---
> app/test-pmd/parameters.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
>diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c index
>1ead59579..ada870e2d 100644
>--- a/app/test-pmd/parameters.c
>+++ b/app/test-pmd/parameters.c
>@@ -142,6 +142,7 @@ usage(char* progname)
> 	printf("  --enable-hw-vlan-strip: enable hardware vlan strip.\n");
> 	printf("  --enable-hw-vlan-extend: enable hardware vlan extend.\n");
> 	printf("  --enable-hw-qinq-strip: enable hardware qinq strip.\n");
>+	printf("  --enable-hw-drop-err: enable hardware packet drop for error
>+packets.\n");
> 	printf("  --enable-drop-en: enable per queue packet drop.\n");
> 	printf("  --disable-rss: disable rss.\n");
> 	printf("  --port-topology=<paired|chained|loop>: set port topology
>(paired "
>@@ -631,6 +632,7 @@ launch_args_parse(int argc, char** argv)
> 		{ "enable-hw-vlan-strip",       0, 0, 0 },
> 		{ "enable-hw-vlan-extend",      0, 0, 0 },
> 		{ "enable-hw-qinq-strip",       0, 0, 0 },
>+		{ "enable-hw-drop-err",         0, 0, 0 },
> 		{ "enable-drop-en",            0, 0, 0 },
> 		{ "disable-rss",                0, 0, 0 },
> 		{ "port-topology",              1, 0, 0 },
>@@ -1037,6 +1039,9 @@ launch_args_parse(int argc, char** argv)
> 					"enable-hw-qinq-strip"))
> 				rx_offloads |=
>DEV_RX_OFFLOAD_QINQ_STRIP;
>
>+			if (!strcmp(lgopts[opt_idx].name, "enable-hw-drop-
>err"))
>+				rx_offloads |=
>DEV_RX_OFFLOAD_ERR_PKT_DROP;
>+
> 			if (!strcmp(lgopts[opt_idx].name, "enable-drop-en"))
> 				rx_drop_en = 1;
>
>--
>2.17.1

I think it would be good also to update testpmd run_app.rst with this new option.

Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error packets

[Asaf Penso]

>-----Original Message-----
>From: dev <dev-bounces@dpdk.org> On Behalf Of Jerin Jacob
>Sent: Thursday, October 8, 2020 11:56 AM
>To: Nipun Gupta <nipun.gupta@nxp.com>
>Cc: Stephen Hemminger <stephen@networkplumber.org>; dpdk-dev
><dev@dpdk.org>; NBU-Contact-Thomas Monjalon <thomas@monjalon.net>;
>Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
><arybchenko@solarflare.com>; Hemant Agrawal
><hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>;
>Rohit Raj <rohit.raj@nxp.com>
>Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error
>packets
>
>On Thu, Oct 8, 2020 at 2:23 PM Nipun Gupta <nipun.gupta@nxp.com> wrote:
>>
>>
>>
>> > -----Original Message-----
>> > From: Jerin Jacob <jerinjacobk@gmail.com>
>> > Sent: Tuesday, October 6, 2020 6:44 PM
>> > To: Nipun Gupta <nipun.gupta@nxp.com>
>> > Cc: Stephen Hemminger <stephen@networkplumber.org>; dpdk-dev
>> > <dev@dpdk.org>; Thomas Monjalon <thomas@monjalon.net>; Ferruh
>Yigit
>> > <ferruh.yigit@intel.com>; Andrew Rybchenko
>> > <arybchenko@solarflare.com>; Hemant Agrawal
>> > <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>;
>> > Rohit Raj <rohit.raj@nxp.com>
>> > Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to
>> > drop error packets
>> >
>> > On Tue, Oct 6, 2020 at 6:40 PM Nipun Gupta <nipun.gupta@nxp.com>
>wrote:
>> > >
>> > >
>> > >
>> > > > -----Original Message-----
>> > > > From: Jerin Jacob <jerinjacobk@gmail.com>
>> > > > Sent: Tuesday, October 6, 2020 5:31 PM
>> > > > To: Nipun Gupta <nipun.gupta@nxp.com>
>> > > > Cc: Stephen Hemminger <stephen@networkplumber.org>; dpdk-dev
>> > > > <dev@dpdk.org>; Thomas Monjalon <thomas@monjalon.net>;
>Ferruh
>> > > > Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
>> > <arybchenko@solarflare.com>;
>> > > > Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
>> > > > <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>
>> > > > Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to
>> > > > drop error packets
>> > > >
>> > > > On Tue, Oct 6, 2020 at 4:07 PM Nipun Gupta <nipun.gupta@nxp.com>
>wrote:
>> > > > >
>> > > > >
>> > > > >
>> > > > > > -----Original Message-----
>> > > > > > From: Jerin Jacob <jerinjacobk@gmail.com>
>> > > > > > Sent: Monday, October 5, 2020 9:40 PM
>> > > > > > To: Stephen Hemminger <stephen@networkplumber.org>
>> > > > > > Cc: Nipun Gupta <nipun.gupta@nxp.com>; dpdk-dev
>> > > > > > <dev@dpdk.org>;
>> > > > Thomas
>> > > > > > Monjalon <thomas@monjalon.net>; Ferruh Yigit
>> > <ferruh.yigit@intel.com>;
>> > > > > > Andrew Rybchenko <arybchenko@solarflare.com>; Hemant
>Agrawal
>> > > > > > <hemant.agrawal@nxp.com>; Sachin Saxena
>> > > > > > <sachin.saxena@nxp.com>;
>> > > > Rohit
>> > > > > > Raj <rohit.raj@nxp.com>
>> > > > > > Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx
>> > > > > > offload to drop
>> > error
>> > > > > > packets
>> > > > > >
>> > > > > > On Mon, Oct 5, 2020 at 9:05 PM Stephen Hemminger
>> > > > > > <stephen@networkplumber.org> wrote:
>> > > > > > >
>> > > > > > > On Mon,  5 Oct 2020 12:45:04 +0530 nipun.gupta@nxp.com
>> > > > > > > wrote:
>> > > > > > >
>> > > > > > > > From: Nipun Gupta <nipun.gupta@nxp.com>
>> > > > > > > >
>> > > > > > > > This change adds a RX offload capability, which once
>> > > > > > > > enabled, hardware will drop the packets in case there of
>> > > > > > > > any error in the packet such as L3 checksum error or L4
>checksum.
>> > > > > >
>> > > > > > IMO, Providing additional support up to the level to choose
>> > > > > > the errors to drops give more control to the application.
>> > > > > > Meaning,
>> > > > > > L1 errors such as FCS error
>> > > > > > L2 errors ..
>> > > > > > L3 errors such checksum
>> > > > > > i.e ethdev spec need to have  error level supported by PMD
>> > > > > > and the application can set the layers interested to drop.
>> > > > >
>> > > > > Agree, but 'DEV_RX_OFFLOAD_ERR_PKT_DROP' shall also be there
>> > > > > to drop
>> > all
>> > > > the
>> > > > > error packets? Maybe we can rename it to
>> > > > DEV_RX_OFFLOAD_ALL_ERR_PKT_DROP.
>> > > >
>> > > > IMHO,  we introduce such shortcut for a single flag for all err
>> > > > drop then we can not change the scheme without an API/ABI break.
>> > >
>> > > Are the following offloads fine:
>> > >         DEV_RX_OFFLOAD_L1_FCS_ERR_PKT_DROP
>> > >         DEV_RX_OFFLOAD_L3_CSUM_ERR_PKT_DROP
>> > >         DEV_RX_OFFLOAD_L4_CSUM_ERR_PKT_DROP
>> > >         DEV_RX_OFFLOAD_ALL_ERR_PKT_DROP
>> > >
>> > > Please let me know in case I need to add any other too.
>> >
>> > I think, single offload flags and some config/capability structure
>> > to define the additional layer selection would be good, instead of
>> > adding a lot of new offload flags.
>>
>>
>> +/**
>> + * A structure used to enable/disable error packet drop on Rx.
>> + */
>> +struct rte_rx_err_pkt_drop_conf {
>> +       /** enable/disable all RX error packet drop.
>> +        * 0 (default) - disable, 1 enable
>> +        */
>> +       uint32_t all:1;
>> +};
>> +
>>  /**
>>   * A structure used to configure an Ethernet port.
>>   * Depending upon the RX multi-queue mode, extra advanced @@ -1236,6
>> +1246,8 @@ struct rte_eth_conf {
>>         uint32_t dcb_capability_en;
>>         struct rte_fdir_conf fdir_conf; /**< FDIR configuration. DEPRECATED */
>>         struct rte_intr_conf intr_conf; /**< Interrupt mode
>> configuration. */
>> +       struct rte_rx_err_pkt_drop_conf err_pkt_drop_conf;
>> +       /**< RX error packet drop configuration. */
>>
>> Is this the kind of changes you are talking about?
>
>
>Yes.
>
>>
>> Also, more changes will be there in 'struct rte_eth_dev_info'
>> structure, defining additional separate capability something like 'uint64_t
>rx_err_drop_offload_capa'.
>>
>> Regards,
>> Nipun
>>
>> >
>> >
>> > > Ill send a v3.
>> > >
>> > > Thanks,
>> > > Nipun
>> > >
>> > > >
>> > > > >
>> > > > > Currently we have not planned to add separate knobs for
>> > > > > separate error in the driver, maybe we can define them
>> > > > > separately, or we need have them in this series itself?
>> > > >
>> > > > I think, ethdev API can have the capability on what are levels
>> > > > it supported, in your driver case, you can express the same.
>> > > >
>> > > >
>> > > > >
>> > > > > >
>> > > > > > > >
>> > > > > > > > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
>> > > > > > > > Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
>> > > > > > > > ---
>> > > > > > > > These patches are based over series:
>> > > > > > > >
>> > > > > >
>> > > >
>> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpa
>> > tchwo
>> > > > > >
>> > > >
>> >
>rk.dpdk.org%2Fpatch%2F78630%2F&amp;data=02%7C01%7Cnipun.gupta%40
>nx
>> > > > > >
>> > > >
>> >
>p.com%7C90b516fd465c48945e7008d869492b3e%7C686ea1d3bc2b4c6fa92cd9
>> > > > > >
>> > > >
>> >
>9c5c301635%7C0%7C0%7C637375110263097933&amp;sdata=RBQswMBsfpM6
>> > > > > >
>nyKur%2FaHvOMvNK7RU%2BRyhHt%2FXBsP1OM%3D&amp;reserved=0
>> > > > > > > >
>> > > > > > > > Changes in v2:
>> > > > > > > >  - Add support in DPAA1 driver (patch 2/3)
>> > > > > > > >  - Add support and config parameter in testpmd (patch
>> > > > > > > > 3/3)
>> > > > > > > >
>> > > > > > > >  lib/librte_ethdev/rte_ethdev.h | 1 +
>> > > > > > > >  1 file changed, 1 insertion(+)
>> > > > > > >
>> > > > > > > Maybe this should be an rte_flow match/action which would
>> > > > > > > then make
>> > it
>> > > > > > > more flexible?
>> > > > > >
>> > > > > > I think, it is not based on any Patten matching. So IMO, it
>> > > > > > should be best
>> > if it
>> > > > > > is part of RX offload.
>> > > > > >
>> > > > > > >
>> > > > > > > There is not much of a performance gain for this in real
>> > > > > > > life and if only one driver supports it then I am not convinced this
>is needed.
>> > > > > >
>> > > > > > Marvell HW has this feature.
Reviewed-By: Asaf Penso <asafp@nvidia.com>

Re: [dpdk-dev] [PATCH 3/3 v2] testpmd: support hardware offload to drop error packets

[Nipun Gupta]

> -----Original Message-----
> From: Asaf Penso <asafp@nvidia.com>
> Sent: Thursday, October 8, 2020 8:36 PM
> To: Nipun Gupta <nipun.gupta@nxp.com>; dev@dpdk.org
> Cc: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>;
> ferruh.yigit@intel.com; arybchenko@solarflare.com; Hemant Agrawal
> <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>; Rohit
> Raj <rohit.raj@nxp.com>
> Subject: RE: [dpdk-dev] [PATCH 3/3 v2] testpmd: support hardware offload to
> drop error packets
> 
> >-----Original Message-----
> >From: dev <dev-bounces@dpdk.org> On Behalf Of nipun.gupta@nxp.com
> >Sent: Monday, October 5, 2020 10:15 AM
> >To: dev@dpdk.org
> >Cc: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>;
> >ferruh.yigit@intel.com; arybchenko@solarflare.com;
> >hemant.agrawal@nxp.com; sachin.saxena@nxp.com; rohit.raj@nxp.com;
> >Nipun Gupta <nipun.gupta@nxp.com>
> >Subject: [dpdk-dev] [PATCH 3/3 v2] testpmd: support hardware offload to
> >drop error packets
> >
> >From: Nipun Gupta <nipun.gupta@nxp.com>
> >
> >With DEV_RX_OFFLOAD_ERR_PKT_DROP now defined as an offload
> >capability, testpmd showcases this with a new added configuration option
> >'enable-hw-drop-err'.
> >
> >Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> >---
> > app/test-pmd/parameters.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> >diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c index
> >1ead59579..ada870e2d 100644
> >--- a/app/test-pmd/parameters.c
> >+++ b/app/test-pmd/parameters.c
> >@@ -142,6 +142,7 @@ usage(char* progname)
> > 	printf("  --enable-hw-vlan-strip: enable hardware vlan strip.\n");
> > 	printf("  --enable-hw-vlan-extend: enable hardware vlan extend.\n");
> > 	printf("  --enable-hw-qinq-strip: enable hardware qinq strip.\n");
> >+	printf("  --enable-hw-drop-err: enable hardware packet drop for error
> >+packets.\n");
> > 	printf("  --enable-drop-en: enable per queue packet drop.\n");
> > 	printf("  --disable-rss: disable rss.\n");
> > 	printf("  --port-topology=<paired|chained|loop>: set port topology
> >(paired "
> >@@ -631,6 +632,7 @@ launch_args_parse(int argc, char** argv)
> > 		{ "enable-hw-vlan-strip",       0, 0, 0 },
> > 		{ "enable-hw-vlan-extend",      0, 0, 0 },
> > 		{ "enable-hw-qinq-strip",       0, 0, 0 },
> >+		{ "enable-hw-drop-err",         0, 0, 0 },
> > 		{ "enable-drop-en",            0, 0, 0 },
> > 		{ "disable-rss",                0, 0, 0 },
> > 		{ "port-topology",              1, 0, 0 },
> >@@ -1037,6 +1039,9 @@ launch_args_parse(int argc, char** argv)
> > 					"enable-hw-qinq-strip"))
> > 				rx_offloads |=
> >DEV_RX_OFFLOAD_QINQ_STRIP;
> >
> >+			if (!strcmp(lgopts[opt_idx].name, "enable-hw-drop-
> >err"))
> >+				rx_offloads |=
> >DEV_RX_OFFLOAD_ERR_PKT_DROP;
> >+
> > 			if (!strcmp(lgopts[opt_idx].name, "enable-drop-en"))
> > 				rx_drop_en = 1;
> >
> >--
> >2.17.1
> 
> I think it would be good also to update testpmd run_app.rst with this new option.

Sure. Will update in next spin.

Thanks,
Nipun

[dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error packets

[nipun.gupta]

From: Nipun Gupta <nipun.gupta@nxp.com>

This change adds a RX offload capability and configuration to
enable hardware to drop the packets in case of any error in the
packets such as L3 checksum error or L4 checksum.

Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
Reviewed-by: Asaf Penso <asafp@nvidia.com>
---

v3:
 - Add additional rx_err_drop_offload_capa, which is specific
   capability flag for RX packets error drop offload. Currently
   only 'all' error packet drops are enabled, but can be extended
   to provide capability to drop any specific errors like L1 FCS,
   L3 Checksum etc.
 - Added separate config structure to enable the drop configuration.
 - Updated doc with the new updated option in testbbdev (patch 3/3)

v2:
 - Add support in DPAA1 driver (patch 2/3)
 - Add support and config parameter in testpmd (patch 3/3)

 lib/librte_ethdev/rte_ethdev.c |  1 +
 lib/librte_ethdev/rte_ethdev.h | 22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 48d1333b1..be25e947e 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -128,6 +128,7 @@ static const struct {
 	RTE_RX_OFFLOAD_BIT2STR(SCTP_CKSUM),
 	RTE_RX_OFFLOAD_BIT2STR(OUTER_UDP_CKSUM),
 	RTE_RX_OFFLOAD_BIT2STR(RSS_HASH),
+	RTE_RX_OFFLOAD_BIT2STR(ERR_PKT_DROP),
 };
 
 #undef RTE_RX_OFFLOAD_BIT2STR
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index d2bf74f12..cb968d38a 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1194,6 +1194,16 @@ struct rte_intr_conf {
 	uint32_t rmv:1;
 };
 
+/**
+ * A structure used to enable/disable error packet drop on RX.
+ */
+struct rte_rx_err_pkt_drop_conf {
+	/** enable/disable all RX error packet drop.
+	 * 0 (default) - disable, 1 enable
+	 */
+	uint32_t all:1;
+};
+
 /**
  * A structure used to configure an Ethernet port.
  * Depending upon the RX multi-queue mode, extra advanced
@@ -1236,6 +1246,8 @@ struct rte_eth_conf {
 	uint32_t dcb_capability_en;
 	struct rte_fdir_conf fdir_conf; /**< FDIR configuration. DEPRECATED */
 	struct rte_intr_conf intr_conf; /**< Interrupt mode configuration. */
+	struct rte_rx_err_pkt_drop_conf err_pkt_drop_conf;
+	/**< RX error packet drop configuration. */
 };
 
 /**
@@ -1260,6 +1272,7 @@ struct rte_eth_conf {
 #define DEV_RX_OFFLOAD_SCTP_CKSUM	0x00020000
 #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM  0x00040000
 #define DEV_RX_OFFLOAD_RSS_HASH		0x00080000
+#define DEV_RX_OFFLOAD_ERR_PKT_DROP	0x00100000
 
 #define DEV_RX_OFFLOAD_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM | \
 				 DEV_RX_OFFLOAD_UDP_CKSUM | \
@@ -1274,6 +1287,13 @@ struct rte_eth_conf {
  * mentioned in rte_rx_offload_names in rte_ethdev.c file.
  */
 
+/**
+ * RX Error Drop offload config/capabilities of a device. These
+ * are valid only when RX capability DEV_RX_OFFLOAD_ERR_PKT_DROP
+ * is supported by the device.
+ */
+#define DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL		0x00000001
+
 /**
  * TX offload capabilities of a device.
  */
@@ -1411,6 +1431,8 @@ struct rte_eth_dev_info {
 	/**< Device per-queue RX offload capabilities. */
 	uint64_t tx_queue_offload_capa;
 	/**< Device per-queue TX offload capabilities. */
+	uint64_t rx_err_drop_offload_capa;
+	/**< RX error packet drop offload capabilities. */
 	uint16_t reta_size;
 	/**< Device redirection table size, the total number of entries. */
 	uint8_t hash_key_size; /**< Hash key size in bytes */
-- 
2.17.1

[dpdk-dev] [PATCH 2/3 v3] net/dpaa: support RX offload for error packet drop

[nipun.gupta]

From: Nipun Gupta <nipun.gupta@nxp.com>

This patch supports RX offload configuration to drop error
packets in the hardware

Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
---
 drivers/bus/dpaa/base/fman/fman_hw.c |  7 ++++++-
 drivers/net/dpaa/dpaa_ethdev.c       | 23 +++++++++++++++++++----
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/bus/dpaa/base/fman/fman_hw.c b/drivers/bus/dpaa/base/fman/fman_hw.c
index 4ab49f785..40e4e0000 100644
--- a/drivers/bus/dpaa/base/fman/fman_hw.c
+++ b/drivers/bus/dpaa/base/fman/fman_hw.c
@@ -597,7 +597,12 @@ void
 fman_if_discard_rx_errors(struct fman_if *fm_if)
 {
 	struct __fman_if *__if = container_of(fm_if, struct __fman_if, __if);
-	unsigned int *fmbm_rfsdm, *fmbm_rfsem;
+	unsigned int *fmbm_rcfg, *fmbm_rfsdm, *fmbm_rfsem;
+	unsigned int val;
+
+	fmbm_rcfg = &((struct rx_bmi_regs *)__if->bmi_map)->fmbm_rcfg;
+	val = in_be32(fmbm_rcfg);
+	out_be32(fmbm_rcfg, val & ~BMI_PORT_CFG_FDOVR);
 
 	fmbm_rfsem = &((struct rx_bmi_regs *)__if->bmi_map)->fmbm_rfsem;
 	out_be32(fmbm_rfsem, 0);
diff --git a/drivers/net/dpaa/dpaa_ethdev.c b/drivers/net/dpaa/dpaa_ethdev.c
index af47c196a..bed84a584 100644
--- a/drivers/net/dpaa/dpaa_ethdev.c
+++ b/drivers/net/dpaa/dpaa_ethdev.c
@@ -52,7 +52,8 @@
 /* Supported Rx offloads */
 static uint64_t dev_rx_offloads_sup =
 		DEV_RX_OFFLOAD_JUMBO_FRAME |
-		DEV_RX_OFFLOAD_SCATTER;
+		DEV_RX_OFFLOAD_SCATTER |
+		DEV_RX_OFFLOAD_ERR_PKT_DROP;
 
 /* Rx offloads which cannot be disabled */
 static uint64_t dev_rx_offloads_nodis =
@@ -62,6 +63,10 @@ static uint64_t dev_rx_offloads_nodis =
 		DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM |
 		DEV_RX_OFFLOAD_RSS_HASH;
 
+/* Supported Rx Error packet drop offload */
+static uint64_t dev_rx_err_drop_offloads_sup =
+		DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL;
+
 /* Supported Tx offloads */
 static uint64_t dev_tx_offloads_sup =
 		DEV_TX_OFFLOAD_MT_LOCKFREE |
@@ -262,6 +267,18 @@ dpaa_eth_dev_configure(struct rte_eth_dev *dev)
 		dev->data->scattered_rx = 1;
 	}
 
+	if (eth_conf->err_pkt_drop_conf.all) {
+		DPAA_PMD_DEBUG("error packets will be dropped on hw");
+		fman_if_discard_rx_errors(fif);
+	} else {
+		struct dpaa_if *dpaa_intf = dev->data->dev_private;
+		struct qman_fq *rxq = &dpaa_intf->rx_queues[0];
+
+		DPAA_PMD_DEBUG("error packets will not be dropped on hw");
+		fman_if_receive_rx_errors(fif, FM_FD_RX_STATUS_ERR_MASK);
+		fman_if_set_err_fqid(fif, rxq->fqid);
+	}
+
 	if (!(default_q || fmc_q)) {
 		if (dpaa_fm_config(dev,
 			eth_conf->rx_adv_conf.rss_conf.rss_hf)) {
@@ -591,6 +608,7 @@ static int dpaa_eth_dev_info(struct rte_eth_dev *dev,
 					dev_rx_offloads_nodis;
 	dev_info->tx_offload_capa = dev_tx_offloads_sup |
 					dev_tx_offloads_nodis;
+	dev_info->rx_err_drop_offload_capa = dev_rx_err_drop_offloads_sup;
 	dev_info->default_rxportconf.burst_size = DPAA_DEF_RX_BURST_SIZE;
 	dev_info->default_txportconf.burst_size = DPAA_DEF_TX_BURST_SIZE;
 	dev_info->default_rxportconf.nb_queues = 1;
@@ -2085,9 +2103,6 @@ dpaa_dev_init(struct rte_eth_dev *eth_dev)
 		fman_intf->mac_addr.addr_bytes[5]);
 
 	if (!fman_intf->is_shared_mac) {
-		/* Configure error packet handling */
-		fman_if_receive_rx_errors(fman_intf,
-			FM_FD_RX_STATUS_ERR_MASK);
 		/* Disable RX mode */
 		fman_if_disable_rx(fman_intf);
 		/* Disable promiscuous mode */
-- 
2.17.1

[dpdk-dev] [PATCH 3/3 v3] app/testpmd: support hardware offload to drop error packets

[nipun.gupta]

From: Nipun Gupta <nipun.gupta@nxp.com>

With DEV_RX_OFFLOAD_ERR_PKT_DROP now defined as an offload
capability, and separate DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL capability
to drop all error packets in hardware, testpmd showcases this
with a new added configuration option 'enable-hw-drop-err-all'.

Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
---
 app/test-pmd/parameters.c             | 7 +++++++
 app/test-pmd/testpmd.c                | 8 ++++++++
 app/test-pmd/testpmd.h                | 1 +
 doc/guides/testpmd_app_ug/run_app.rst | 4 ++++
 4 files changed, 20 insertions(+)

diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 1ead59579..508612426 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -142,6 +142,7 @@ usage(char* progname)
 	printf("  --enable-hw-vlan-strip: enable hardware vlan strip.\n");
 	printf("  --enable-hw-vlan-extend: enable hardware vlan extend.\n");
 	printf("  --enable-hw-qinq-strip: enable hardware qinq strip.\n");
+	printf("  --enable-hw-drop-err-all: enable hardware packet drop for all error packets.\n");
 	printf("  --enable-drop-en: enable per queue packet drop.\n");
 	printf("  --disable-rss: disable rss.\n");
 	printf("  --port-topology=<paired|chained|loop>: set port topology (paired "
@@ -631,6 +632,7 @@ launch_args_parse(int argc, char** argv)
 		{ "enable-hw-vlan-strip",       0, 0, 0 },
 		{ "enable-hw-vlan-extend",      0, 0, 0 },
 		{ "enable-hw-qinq-strip",       0, 0, 0 },
+		{ "enable-hw-drop-err-all",     0, 0, 0 },
 		{ "enable-drop-en",            0, 0, 0 },
 		{ "disable-rss",                0, 0, 0 },
 		{ "port-topology",              1, 0, 0 },
@@ -1283,6 +1285,11 @@ launch_args_parse(int argc, char** argv)
 				rmv_interrupt = 0;
 			if (!strcmp(lgopts[opt_idx].name, "flow-isolate-all"))
 				flow_isolate_all = 1;
+			if (!strcmp(lgopts[opt_idx].name,
+					"enable-hw-drop-err-all")) {
+				rx_err_pkt_drop_all = 1;
+			}
+
 			if (!strcmp(lgopts[opt_idx].name, "tx-offloads")) {
 				char *end = NULL;
 				n = strtoull(optarg, &end, 16);
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index ccba71c07..c9e7397e6 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -359,6 +359,11 @@ uint8_t lsc_interrupt = 1; /* enabled by default */
  */
 uint8_t rmv_interrupt = 1; /* enabled by default */
 
+/*
+ * Drop all RX error packets on HW itself.
+ */
+uint8_t rx_err_pkt_drop_all = 0; /* disabled by default */
+
 uint8_t hot_plug = 0; /**< hotplug disabled by default. */
 
 /* After attach, port setup is called on event or by iterator */
@@ -3359,6 +3364,9 @@ init_port_config(void)
 		    (rte_eth_devices[pid].data->dev_flags &
 		     RTE_ETH_DEV_INTR_RMV))
 			port->dev_conf.intr_conf.rmv = 1;
+
+		if (rx_err_pkt_drop_all)
+			port->dev_conf.err_pkt_drop_conf.all = 1;
 	}
 }
 
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index c7e7e41a9..eab154ed4 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -314,6 +314,7 @@ extern uint8_t no_device_start; /**<set by "--disable-device-start" parameter */
 extern volatile int test_done; /* stop packet forwarding when set to 1. */
 extern uint8_t lsc_interrupt; /**< disabled by "--no-lsc-interrupt" parameter */
 extern uint8_t rmv_interrupt; /**< disabled by "--no-rmv-interrupt" parameter */
+extern uint8_t rx_err_pkt_drop_all; /**< enabled by "--enable-hw-drop-err-all" parameter */
 extern uint32_t event_print_mask;
 /**< set by "--print-event xxxx" and "--mask-event xxxx parameters */
 extern bool setup_on_probe_event; /**< disabled by port setup-on iterator */
diff --git a/doc/guides/testpmd_app_ug/run_app.rst b/doc/guides/testpmd_app_ug/run_app.rst
index e2539f693..20f2f8083 100644
--- a/doc/guides/testpmd_app_ug/run_app.rst
+++ b/doc/guides/testpmd_app_ug/run_app.rst
@@ -213,6 +213,10 @@ The command line options are:
 
     Enable hardware QINQ strip.
 
+*  ``--enable-hw-drop-err-all``
+
+    Enable hardware packet drop for any error packets
+
 *   ``--enable-drop-en``
 
     Enable per-queue packet drop for packets with no descriptors.
-- 
2.17.1

Re: [dpdk-dev] [PATCH 3/3 v3] app/testpmd: support hardware offload to drop error packets

[Asaf Penso]

Regards,
Asaf Penso

>-----Original Message-----
>From: nipun.gupta@nxp.com <nipun.gupta@nxp.com>
>Sent: Friday, October 9, 2020 4:14 PM
>To: dev@dpdk.org
>Cc: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>;
>ferruh.yigit@intel.com; arybchenko@solarflare.com;
>hemant.agrawal@nxp.com; sachin.saxena@nxp.com; rohit.raj@nxp.com;
>jerinjacobk@gmail.com; stephen@networkplumber.org; Asaf Penso
><asafp@nvidia.com>; Nipun Gupta <nipun.gupta@nxp.com>
>Subject: [PATCH 3/3 v3] app/testpmd: support hardware offload to drop error
>packets
>
>From: Nipun Gupta <nipun.gupta@nxp.com>
>
>With DEV_RX_OFFLOAD_ERR_PKT_DROP now defined as an offload
>capability, and separate DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL capability to
>drop all error packets in hardware, testpmd showcases this with a new added
>configuration option 'enable-hw-drop-err-all'.
>
>Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
Reviewed-By: Asaf Penso <asafp@nvidia.com>
>---
> app/test-pmd/parameters.c             | 7 +++++++
> app/test-pmd/testpmd.c                | 8 ++++++++
> app/test-pmd/testpmd.h                | 1 +
> doc/guides/testpmd_app_ug/run_app.rst | 4 ++++
> 4 files changed, 20 insertions(+)
>
>diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c index
>1ead59579..508612426 100644
>--- a/app/test-pmd/parameters.c
>+++ b/app/test-pmd/parameters.c
>@@ -142,6 +142,7 @@ usage(char* progname)
> 	printf("  --enable-hw-vlan-strip: enable hardware vlan strip.\n");
> 	printf("  --enable-hw-vlan-extend: enable hardware vlan extend.\n");
> 	printf("  --enable-hw-qinq-strip: enable hardware qinq strip.\n");
>+	printf("  --enable-hw-drop-err-all: enable hardware packet drop for
>+all error packets.\n");
> 	printf("  --enable-drop-en: enable per queue packet drop.\n");
> 	printf("  --disable-rss: disable rss.\n");
> 	printf("  --port-topology=<paired|chained|loop>: set port topology
>(paired "
>@@ -631,6 +632,7 @@ launch_args_parse(int argc, char** argv)
> 		{ "enable-hw-vlan-strip",       0, 0, 0 },
> 		{ "enable-hw-vlan-extend",      0, 0, 0 },
> 		{ "enable-hw-qinq-strip",       0, 0, 0 },
>+		{ "enable-hw-drop-err-all",     0, 0, 0 },
> 		{ "enable-drop-en",            0, 0, 0 },
> 		{ "disable-rss",                0, 0, 0 },
> 		{ "port-topology",              1, 0, 0 },
>@@ -1283,6 +1285,11 @@ launch_args_parse(int argc, char** argv)
> 				rmv_interrupt = 0;
> 			if (!strcmp(lgopts[opt_idx].name, "flow-isolate-all"))
> 				flow_isolate_all = 1;
>+			if (!strcmp(lgopts[opt_idx].name,
>+					"enable-hw-drop-err-all")) {
>+				rx_err_pkt_drop_all = 1;
>+			}
>+
> 			if (!strcmp(lgopts[opt_idx].name, "tx-offloads")) {
> 				char *end = NULL;
> 				n = strtoull(optarg, &end, 16);
>diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>ccba71c07..c9e7397e6 100644
>--- a/app/test-pmd/testpmd.c
>+++ b/app/test-pmd/testpmd.c
>@@ -359,6 +359,11 @@ uint8_t lsc_interrupt = 1; /* enabled by default */
>  */
> uint8_t rmv_interrupt = 1; /* enabled by default */
>
>+/*
>+ * Drop all RX error packets on HW itself.
>+ */
>+uint8_t rx_err_pkt_drop_all = 0; /* disabled by default */
>+
> uint8_t hot_plug = 0; /**< hotplug disabled by default. */
>
> /* After attach, port setup is called on event or by iterator */ @@ -3359,6
>+3364,9 @@ init_port_config(void)
> 		    (rte_eth_devices[pid].data->dev_flags &
> 		     RTE_ETH_DEV_INTR_RMV))
> 			port->dev_conf.intr_conf.rmv = 1;
>+
>+		if (rx_err_pkt_drop_all)
>+			port->dev_conf.err_pkt_drop_conf.all = 1;
> 	}
> }
>
>diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h index
>c7e7e41a9..eab154ed4 100644
>--- a/app/test-pmd/testpmd.h
>+++ b/app/test-pmd/testpmd.h
>@@ -314,6 +314,7 @@ extern uint8_t no_device_start; /**<set by "--disable-
>device-start" parameter */  extern volatile int test_done; /* stop packet
>forwarding when set to 1. */  extern uint8_t lsc_interrupt; /**< disabled by "-
>-no-lsc-interrupt" parameter */  extern uint8_t rmv_interrupt; /**< disabled
>by "--no-rmv-interrupt" parameter */
>+extern uint8_t rx_err_pkt_drop_all; /**< enabled by
>+"--enable-hw-drop-err-all" parameter */
> extern uint32_t event_print_mask;
> /**< set by "--print-event xxxx" and "--mask-event xxxx parameters */
>extern bool setup_on_probe_event; /**< disabled by port setup-on iterator
>*/ diff --git a/doc/guides/testpmd_app_ug/run_app.rst
>b/doc/guides/testpmd_app_ug/run_app.rst
>index e2539f693..20f2f8083 100644
>--- a/doc/guides/testpmd_app_ug/run_app.rst
>+++ b/doc/guides/testpmd_app_ug/run_app.rst
>@@ -213,6 +213,10 @@ The command line options are:
>
>     Enable hardware QINQ strip.
>
>+*  ``--enable-hw-drop-err-all``
>+
>+    Enable hardware packet drop for any error packets
>+
> *   ``--enable-drop-en``
>
>     Enable per-queue packet drop for packets with no descriptors.
>--
>2.17.1

Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error packets

[Jerin Jacob]

On Fri, Oct 9, 2020 at 6:43 PM <nipun.gupta@nxp.com> wrote:
>
> From: Nipun Gupta <nipun.gupta@nxp.com>
>
> This change adds a RX offload capability and configuration to
> enable hardware to drop the packets in case of any error in the
> packets such as L3 checksum error or L4 checksum.
>
> Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> Reviewed-by: Asaf Penso <asafp@nvidia.com>
> ---
>
> v3:
>  - Add additional rx_err_drop_offload_capa, which is specific
>    capability flag for RX packets error drop offload. Currently
>    only 'all' error packet drops are enabled, but can be extended
>    to provide capability to drop any specific errors like L1 FCS,
>    L3 Checksum etc.
>  - Added separate config structure to enable the drop configuration.
>  - Updated doc with the new updated option in testbbdev (patch 3/3)
>
> v2:
>  - Add support in DPAA1 driver (patch 2/3)
>  - Add support and config parameter in testpmd (patch 3/3)
>
>  lib/librte_ethdev/rte_ethdev.c |  1 +
>  lib/librte_ethdev/rte_ethdev.h | 22 ++++++++++++++++++++++
>  2 files changed, 23 insertions(+)


The patch looks good to me.

1) Missed to update port_offload_cap_display() for new oflload in
/app/test-pmd/config.c
2) Please update doc/guides/nics/features.rst for this new offload

See [1] as a reference.
[1]
commit 28f6a3b88d23a348eeffd2bac79b2e0819d3b4e9
Author: Jerin Jacob <jerin.jacob@caviumnetworks.com>
Date:   Tue Oct 2 16:21:41 2018 +0530

    ethdev: support SCTP Rx checksum offload

    Added SCTP Rx checksum offload support

    Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
    Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>



>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 48d1333b1..be25e947e 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -128,6 +128,7 @@ static const struct {
>         RTE_RX_OFFLOAD_BIT2STR(SCTP_CKSUM),
>         RTE_RX_OFFLOAD_BIT2STR(OUTER_UDP_CKSUM),
>         RTE_RX_OFFLOAD_BIT2STR(RSS_HASH),
> +       RTE_RX_OFFLOAD_BIT2STR(ERR_PKT_DROP),
>  };
>
>  #undef RTE_RX_OFFLOAD_BIT2STR
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index d2bf74f12..cb968d38a 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1194,6 +1194,16 @@ struct rte_intr_conf {
>         uint32_t rmv:1;
>  };
>
> +/**
> + * A structure used to enable/disable error packet drop on RX.
> + */
> +struct rte_rx_err_pkt_drop_conf {
> +       /** enable/disable all RX error packet drop.
> +        * 0 (default) - disable, 1 enable
> +        */
> +       uint32_t all:1;
> +};
> +
>  /**
>   * A structure used to configure an Ethernet port.
>   * Depending upon the RX multi-queue mode, extra advanced
> @@ -1236,6 +1246,8 @@ struct rte_eth_conf {
>         uint32_t dcb_capability_en;
>         struct rte_fdir_conf fdir_conf; /**< FDIR configuration. DEPRECATED */
>         struct rte_intr_conf intr_conf; /**< Interrupt mode configuration. */
> +       struct rte_rx_err_pkt_drop_conf err_pkt_drop_conf;
> +       /**< RX error packet drop configuration. */
>  };
>
>  /**
> @@ -1260,6 +1272,7 @@ struct rte_eth_conf {
>  #define DEV_RX_OFFLOAD_SCTP_CKSUM      0x00020000
>  #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM  0x00040000
>  #define DEV_RX_OFFLOAD_RSS_HASH                0x00080000
> +#define DEV_RX_OFFLOAD_ERR_PKT_DROP    0x00100000
>
>  #define DEV_RX_OFFLOAD_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM | \
>                                  DEV_RX_OFFLOAD_UDP_CKSUM | \
> @@ -1274,6 +1287,13 @@ struct rte_eth_conf {
>   * mentioned in rte_rx_offload_names in rte_ethdev.c file.
>   */
>
> +/**
> + * RX Error Drop offload config/capabilities of a device. These
> + * are valid only when RX capability DEV_RX_OFFLOAD_ERR_PKT_DROP
> + * is supported by the device.
> + */
> +#define DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL                0x00000001
> +
>  /**
>   * TX offload capabilities of a device.
>   */
> @@ -1411,6 +1431,8 @@ struct rte_eth_dev_info {
>         /**< Device per-queue RX offload capabilities. */
>         uint64_t tx_queue_offload_capa;
>         /**< Device per-queue TX offload capabilities. */
> +       uint64_t rx_err_drop_offload_capa;
> +       /**< RX error packet drop offload capabilities. */
>         uint16_t reta_size;
>         /**< Device redirection table size, the total number of entries. */
>         uint8_t hash_key_size; /**< Hash key size in bytes */
> --
> 2.17.1
>

Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error packets

[Thomas Monjalon]

Hi,

The configuration of this feature is not clear to me.
Please see the comments below.

09/10/2020 15:13, nipun.gupta@nxp.com:
> From: Nipun Gupta <nipun.gupta@nxp.com>
> 
> This change adds a RX offload capability and configuration to
> enable hardware to drop the packets in case of any error in the
> packets such as L3 checksum error or L4 checksum.
> 
> Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> Reviewed-by: Asaf Penso <asafp@nvidia.com>
> ---
[...]
> +/**
> + * A structure used to enable/disable error packet drop on RX.

RX -> Rx
same for other occurences below

> + */
> +struct rte_rx_err_pkt_drop_conf {

The name should start with rte_eth_

> +	/** enable/disable all RX error packet drop.
> +	 * 0 (default) - disable, 1 enable
> +	 */
> +	uint32_t all:1;
> +};

I don't understand the meaning of this struct.
Is it just one bit to drop packets having an error?
How do you determine what is an error?

[...]
> @@ -1236,6 +1246,8 @@ struct rte_eth_conf {
>  	uint32_t dcb_capability_en;
>  	struct rte_fdir_conf fdir_conf; /**< FDIR configuration. DEPRECATED */
>  	struct rte_intr_conf intr_conf; /**< Interrupt mode configuration. */
> +	struct rte_rx_err_pkt_drop_conf err_pkt_drop_conf;
> +	/**< RX error packet drop configuration. */

Why a per-port configuration is needed in addition of the per-queue offload?

[...]
> @@ -1260,6 +1272,7 @@ struct rte_eth_conf {
>  #define DEV_RX_OFFLOAD_SCTP_CKSUM	0x00020000
>  #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM  0x00040000
>  #define DEV_RX_OFFLOAD_RSS_HASH		0x00080000
> +#define DEV_RX_OFFLOAD_ERR_PKT_DROP	0x00100000

New offload names should starte with RTE_ prefix.


> +/**
> + * RX Error Drop offload config/capabilities of a device. These
> + * are valid only when RX capability DEV_RX_OFFLOAD_ERR_PKT_DROP
> + * is supported by the device.
> + */
> +#define DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL		0x00000001

I don't understand the meaning.

> @@ -1411,6 +1431,8 @@ struct rte_eth_dev_info {
>  	/**< Device per-queue RX offload capabilities. */
>  	uint64_t tx_queue_offload_capa;
>  	/**< Device per-queue TX offload capabilities. */
> +	uint64_t rx_err_drop_offload_capa;
> +	/**< RX error packet drop offload capabilities. */

Why adding a new field here instead of reporting in rx_offload_capa?

Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error packets

[Nipun Gupta]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Monday, October 12, 2020 3:12 AM
> To: Nipun Gupta <nipun.gupta@nxp.com>
> Cc: dev@dpdk.org; ferruh.yigit@intel.com; arybchenko@solarflare.com;
> Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>;
> jerinjacobk@gmail.com; stephen@networkplumber.org; asafp@nvidia.com
> Subject: Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error
> packets
> 
> Hi,
> 
> The configuration of this feature is not clear to me.
> Please see the comments below.
> 
> 09/10/2020 15:13, nipun.gupta@nxp.com:
> > From: Nipun Gupta <nipun.gupta@nxp.com>
> >
> > This change adds a RX offload capability and configuration to
> > enable hardware to drop the packets in case of any error in the
> > packets such as L3 checksum error or L4 checksum.
> >
> > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> > Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> > Reviewed-by: Asaf Penso <asafp@nvidia.com>
> > ---
> [...]
> > +/**
> > + * A structure used to enable/disable error packet drop on RX.
> 
> RX -> Rx
> same for other occurences below

Okay.

> 
> > + */
> > +struct rte_rx_err_pkt_drop_conf {
> 
> The name should start with rte_eth_
> 
> > +	/** enable/disable all RX error packet drop.
> > +	 * 0 (default) - disable, 1 enable
> > +	 */
> > +	uint32_t all:1;
> > +};
> 
> I don't understand the meaning of this struct.
> Is it just one bit to drop packets having an error?
> How do you determine what is an error?

The error packets which are dropped on the hardware can be from
various reasons, like L1 FCS, L3 CSUM, L4 CSUM or HW can be configured
to drop ALL (or any) of the error packets received on the interface.

Currently DPAA drivers support drop of ALL the error packets, but does
not support configuration of packet drops on basis of separate error types
like dropping only L2 CSUM, L3 CSUM. So this patch is supporting only drop
of 'ALL' the errors packets received on HW.

Marvell supports drop on separate error types, thus Jerin suggested to have
this structure so that it opens the room for adding separate configuration
to drop packets on basis of separate error types.

Jerin,
maybe you can add more on this.

> 
> [...]
> > @@ -1236,6 +1246,8 @@ struct rte_eth_conf {
> >  	uint32_t dcb_capability_en;
> >  	struct rte_fdir_conf fdir_conf; /**< FDIR configuration. DEPRECATED */
> >  	struct rte_intr_conf intr_conf; /**< Interrupt mode configuration. */
> > +	struct rte_rx_err_pkt_drop_conf err_pkt_drop_conf;
> > +	/**< RX error packet drop configuration. */
> 
> Why a per-port configuration is needed in addition of the per-queue offload?
> 
> [...]
> > @@ -1260,6 +1272,7 @@ struct rte_eth_conf {
> >  #define DEV_RX_OFFLOAD_SCTP_CKSUM	0x00020000
> >  #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM  0x00040000
> >  #define DEV_RX_OFFLOAD_RSS_HASH		0x00080000
> > +#define DEV_RX_OFFLOAD_ERR_PKT_DROP	0x00100000
> 
> New offload names should starte with RTE_ prefix.

Okay

> 
> 
> > +/**
> > + * RX Error Drop offload config/capabilities of a device. These
> > + * are valid only when RX capability DEV_RX_OFFLOAD_ERR_PKT_DROP
> > + * is supported by the device.
> > + */
> > +#define DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL		0x00000001
> 
> I don't understand the meaning.

For DPAA drivers there is only one offload required, but once separate offloads
are added to drop packets on basis of separate errors, then it can bloat up the
offload capabilities. So we added new flags ' rte_rx_err_pkt_drop_conf ' instead
of adding this onto 'rx_offload_capa'.

There is only one flag ' DEV_RX_OFFLOAD_ERR_PKT_DROP' added in 'rx_offload_capa'
and if this flag is enabled, one or more of the flag in ' DEV_RX_ERR_PKT_DROP_OFFLOAD...'
can be enabled (currently only DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL) being supported
Others like:
- DEV_RX_ERR_PKT_DROP_OFFLOAD_L1_FCS
- DEV_RX_ERR_PKT_DROP_OFFLOAD_L3_CSUM
- DEV_RX_ERR_PKT_DROP_OFFLOAD_L4_CSUM etc.
Can be added once any reference driver is supporting this.

P.S. I can add these flags in this patch but without any ref driver support.

> 
> > @@ -1411,6 +1431,8 @@ struct rte_eth_dev_info {
> >  	/**< Device per-queue RX offload capabilities. */
> >  	uint64_t tx_queue_offload_capa;
> >  	/**< Device per-queue TX offload capabilities. */
> > +	uint64_t rx_err_drop_offload_capa;
> > +	/**< RX error packet drop offload capabilities. */
> 
> Why adding a new field here instead of reporting in rx_offload_capa?

This was part of v2 where flag was added in rx_offload_capa.

Both ways (v2 and v3 versions) seems to have pros and cons:
- Adding only in the rx_offload_capa' seems simpler to implement and use
- But it shall lead to increase in the offload capability types when packet
  drop configuration is added on individual error types.

Regards,
Nipun

> 
> 

Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error packets

[Andrew Rybchenko]

On 10/9/20 4:13 PM, nipun.gupta@nxp.com wrote:
> From: Nipun Gupta <nipun.gupta@nxp.com>
> 
> This change adds a RX offload capability and configuration to
> enable hardware to drop the packets in case of any error in the
> packets such as L3 checksum error or L4 checksum.
> 
> Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> Reviewed-by: Asaf Penso <asafp@nvidia.com>
> ---
> 
> v3:
>  - Add additional rx_err_drop_offload_capa, which is specific
>    capability flag for RX packets error drop offload. Currently
>    only 'all' error packet drops are enabled, but can be extended
>    to provide capability to drop any specific errors like L1 FCS,
>    L3 Checksum etc.
>  - Added separate config structure to enable the drop configuration.
>  - Updated doc with the new updated option in testbbdev (patch 3/3)
> 
> v2:
>  - Add support in DPAA1 driver (patch 2/3)
>  - Add support and config parameter in testpmd (patch 3/3)
> 
>  lib/librte_ethdev/rte_ethdev.c |  1 +
>  lib/librte_ethdev/rte_ethdev.h | 22 ++++++++++++++++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 48d1333b1..be25e947e 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -128,6 +128,7 @@ static const struct {
>  	RTE_RX_OFFLOAD_BIT2STR(SCTP_CKSUM),
>  	RTE_RX_OFFLOAD_BIT2STR(OUTER_UDP_CKSUM),
>  	RTE_RX_OFFLOAD_BIT2STR(RSS_HASH),
> +	RTE_RX_OFFLOAD_BIT2STR(ERR_PKT_DROP),
>  };
>  
>  #undef RTE_RX_OFFLOAD_BIT2STR
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index d2bf74f12..cb968d38a 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1194,6 +1194,16 @@ struct rte_intr_conf {
>  	uint32_t rmv:1;
>  };
>  
> +/**
> + * A structure used to enable/disable error packet drop on RX.
> + */
> +struct rte_rx_err_pkt_drop_conf {
> +	/** enable/disable all RX error packet drop.
> +	 * 0 (default) - disable, 1 enable
> +	 */
> +	uint32_t all:1;

"all" is bad. It should be granular and in the same terms
as mask in dev_info capability.

> +};
> +
>  /**
>   * A structure used to configure an Ethernet port.
>   * Depending upon the RX multi-queue mode, extra advanced
> @@ -1236,6 +1246,8 @@ struct rte_eth_conf {
>  	uint32_t dcb_capability_en;
>  	struct rte_fdir_conf fdir_conf; /**< FDIR configuration. DEPRECATED */
>  	struct rte_intr_conf intr_conf; /**< Interrupt mode configuration. */
> +	struct rte_rx_err_pkt_drop_conf err_pkt_drop_conf;
> +	/**< RX error packet drop configuration. */

Why not per queue?

>  };
>  
>  /**
> @@ -1260,6 +1272,7 @@ struct rte_eth_conf {
>  #define DEV_RX_OFFLOAD_SCTP_CKSUM	0x00020000
>  #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM  0x00040000
>  #define DEV_RX_OFFLOAD_RSS_HASH		0x00080000
> +#define DEV_RX_OFFLOAD_ERR_PKT_DROP	0x00100000
>  
>  #define DEV_RX_OFFLOAD_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM | \
>  				 DEV_RX_OFFLOAD_UDP_CKSUM | \
> @@ -1274,6 +1287,13 @@ struct rte_eth_conf {
>   * mentioned in rte_rx_offload_names in rte_ethdev.c file.
>   */
>  
> +/**
> + * RX Error Drop offload config/capabilities of a device. These
> + * are valid only when RX capability DEV_RX_OFFLOAD_ERR_PKT_DROP
> + * is supported by the device.
> + */
> +#define DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL		0x00000001
> +

I strictly dislike "all". It will always be bad defined.
It must be granular.

>  /**
>   * TX offload capabilities of a device.
>   */
> @@ -1411,6 +1431,8 @@ struct rte_eth_dev_info {
>  	/**< Device per-queue RX offload capabilities. */
>  	uint64_t tx_queue_offload_capa;
>  	/**< Device per-queue TX offload capabilities. */
> +	uint64_t rx_err_drop_offload_capa;
> +	/**< RX error packet drop offload capabilities. */
>  	uint16_t reta_size;
>  	/**< Device redirection table size, the total number of entries. */
>  	uint8_t hash_key_size; /**< Hash key size in bytes */
> 

Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error packets

[Nipun Gupta]

> -----Original Message-----
> From: Andrew Rybchenko <Andrew.Rybchenko@oktetlabs.ru>
> Sent: Monday, October 12, 2020 1:32 PM
> To: Nipun Gupta <nipun.gupta@nxp.com>; dev@dpdk.org
> Cc: thomas@monjalon.net; ferruh.yigit@intel.com; arybchenko@solarflare.com;
> Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>;
> jerinjacobk@gmail.com; stephen@networkplumber.org; asafp@nvidia.com
> Subject: Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error
> packets
> 
> On 10/9/20 4:13 PM, nipun.gupta@nxp.com wrote:
> > From: Nipun Gupta <nipun.gupta@nxp.com>
> >
> > This change adds a RX offload capability and configuration to
> > enable hardware to drop the packets in case of any error in the
> > packets such as L3 checksum error or L4 checksum.
> >
> > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> > Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> > Reviewed-by: Asaf Penso <asafp@nvidia.com>
> > ---
> >
> > v3:
> >  - Add additional rx_err_drop_offload_capa, which is specific
> >    capability flag for RX packets error drop offload. Currently
> >    only 'all' error packet drops are enabled, but can be extended
> >    to provide capability to drop any specific errors like L1 FCS,
> >    L3 Checksum etc.
> >  - Added separate config structure to enable the drop configuration.
> >  - Updated doc with the new updated option in testbbdev (patch 3/3)
> >
> > v2:
> >  - Add support in DPAA1 driver (patch 2/3)
> >  - Add support and config parameter in testpmd (patch 3/3)
> >
> >  lib/librte_ethdev/rte_ethdev.c |  1 +
> >  lib/librte_ethdev/rte_ethdev.h | 22 ++++++++++++++++++++++
> >  2 files changed, 23 insertions(+)
> >
> > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> > index 48d1333b1..be25e947e 100644
> > --- a/lib/librte_ethdev/rte_ethdev.c
> > +++ b/lib/librte_ethdev/rte_ethdev.c
> > @@ -128,6 +128,7 @@ static const struct {
> >  	RTE_RX_OFFLOAD_BIT2STR(SCTP_CKSUM),
> >  	RTE_RX_OFFLOAD_BIT2STR(OUTER_UDP_CKSUM),
> >  	RTE_RX_OFFLOAD_BIT2STR(RSS_HASH),
> > +	RTE_RX_OFFLOAD_BIT2STR(ERR_PKT_DROP),
> >  };
> >
> >  #undef RTE_RX_OFFLOAD_BIT2STR
> > diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> > index d2bf74f12..cb968d38a 100644
> > --- a/lib/librte_ethdev/rte_ethdev.h
> > +++ b/lib/librte_ethdev/rte_ethdev.h
> > @@ -1194,6 +1194,16 @@ struct rte_intr_conf {
> >  	uint32_t rmv:1;
> >  };
> >
> > +/**
> > + * A structure used to enable/disable error packet drop on RX.
> > + */
> > +struct rte_rx_err_pkt_drop_conf {
> > +	/** enable/disable all RX error packet drop.
> > +	 * 0 (default) - disable, 1 enable
> > +	 */
> > +	uint32_t all:1;
> 
> "all" is bad. It should be granular and in the same terms
> as mask in dev_info capability.

Consider that application do not want to receive any error packets as it will drop all
the error packets after increasing the error counter maintainer in the application.
If same functionality can be done by the hardware where hardware maintains the error
statistics, then why not have the same?
When hardware does parsing of packet headers and found error during that parsing,
this is to say to HW - hey drop any such error packets which you found during parsing
and just increase specific error counter (if present).

I agree, granular shall also be added, but as our driver does not support them, so I did
not add those bits here.

> 
> > +};
> > +
> >  /**
> >   * A structure used to configure an Ethernet port.
> >   * Depending upon the RX multi-queue mode, extra advanced
> > @@ -1236,6 +1246,8 @@ struct rte_eth_conf {
> >  	uint32_t dcb_capability_en;
> >  	struct rte_fdir_conf fdir_conf; /**< FDIR configuration. DEPRECATED */
> >  	struct rte_intr_conf intr_conf; /**< Interrupt mode configuration. */
> > +	struct rte_rx_err_pkt_drop_conf err_pkt_drop_conf;
> > +	/**< RX error packet drop configuration. */
> 
> Why not per queue?

We do not support per queue configuration for error packet drop, but only
on ethernet basis.

> 
> >  };
> >
> >  /**
> > @@ -1260,6 +1272,7 @@ struct rte_eth_conf {
> >  #define DEV_RX_OFFLOAD_SCTP_CKSUM	0x00020000
> >  #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM  0x00040000
> >  #define DEV_RX_OFFLOAD_RSS_HASH		0x00080000
> > +#define DEV_RX_OFFLOAD_ERR_PKT_DROP	0x00100000
> >
> >  #define DEV_RX_OFFLOAD_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM | \
> >  				 DEV_RX_OFFLOAD_UDP_CKSUM | \
> > @@ -1274,6 +1287,13 @@ struct rte_eth_conf {
> >   * mentioned in rte_rx_offload_names in rte_ethdev.c file.
> >   */
> >
> > +/**
> > + * RX Error Drop offload config/capabilities of a device. These
> > + * are valid only when RX capability DEV_RX_OFFLOAD_ERR_PKT_DROP
> > + * is supported by the device.
> > + */
> > +#define DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL		0x00000001
> > +
> 
> I strictly dislike "all". It will always be bad defined.
> It must be granular.
> 
> >  /**
> >   * TX offload capabilities of a device.
> >   */
> > @@ -1411,6 +1431,8 @@ struct rte_eth_dev_info {
> >  	/**< Device per-queue RX offload capabilities. */
> >  	uint64_t tx_queue_offload_capa;
> >  	/**< Device per-queue TX offload capabilities. */
> > +	uint64_t rx_err_drop_offload_capa;
> > +	/**< RX error packet drop offload capabilities. */
> >  	uint16_t reta_size;
> >  	/**< Device redirection table size, the total number of entries. */
> >  	uint8_t hash_key_size; /**< Hash key size in bytes */
> >

Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error packets

[Andrew Rybchenko]

On 10/12/20 2:30 PM, Nipun Gupta wrote:
> 
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <Andrew.Rybchenko@oktetlabs.ru>
>> Sent: Monday, October 12, 2020 1:32 PM
>> To: Nipun Gupta <nipun.gupta@nxp.com>; dev@dpdk.org
>> Cc: thomas@monjalon.net; ferruh.yigit@intel.com; arybchenko@solarflare.com;
>> Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
>> <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>;
>> jerinjacobk@gmail.com; stephen@networkplumber.org; asafp@nvidia.com
>> Subject: Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error
>> packets
>>
>> On 10/9/20 4:13 PM, nipun.gupta@nxp.com wrote:
>>> From: Nipun Gupta <nipun.gupta@nxp.com>
>>>
>>> This change adds a RX offload capability and configuration to
>>> enable hardware to drop the packets in case of any error in the
>>> packets such as L3 checksum error or L4 checksum.
>>>
>>> Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
>>> Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
>>> Reviewed-by: Asaf Penso <asafp@nvidia.com>
>>> ---
>>>
>>> v3:
>>>  - Add additional rx_err_drop_offload_capa, which is specific
>>>    capability flag for RX packets error drop offload. Currently
>>>    only 'all' error packet drops are enabled, but can be extended
>>>    to provide capability to drop any specific errors like L1 FCS,
>>>    L3 Checksum etc.
>>>  - Added separate config structure to enable the drop configuration.
>>>  - Updated doc with the new updated option in testbbdev (patch 3/3)
>>>
>>> v2:
>>>  - Add support in DPAA1 driver (patch 2/3)
>>>  - Add support and config parameter in testpmd (patch 3/3)
>>>
>>>  lib/librte_ethdev/rte_ethdev.c |  1 +
>>>  lib/librte_ethdev/rte_ethdev.h | 22 ++++++++++++++++++++++
>>>  2 files changed, 23 insertions(+)
>>>
>>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>>> index 48d1333b1..be25e947e 100644
>>> --- a/lib/librte_ethdev/rte_ethdev.c
>>> +++ b/lib/librte_ethdev/rte_ethdev.c
>>> @@ -128,6 +128,7 @@ static const struct {
>>>  	RTE_RX_OFFLOAD_BIT2STR(SCTP_CKSUM),
>>>  	RTE_RX_OFFLOAD_BIT2STR(OUTER_UDP_CKSUM),
>>>  	RTE_RX_OFFLOAD_BIT2STR(RSS_HASH),
>>> +	RTE_RX_OFFLOAD_BIT2STR(ERR_PKT_DROP),
>>>  };
>>>
>>>  #undef RTE_RX_OFFLOAD_BIT2STR
>>> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
>>> index d2bf74f12..cb968d38a 100644
>>> --- a/lib/librte_ethdev/rte_ethdev.h
>>> +++ b/lib/librte_ethdev/rte_ethdev.h
>>> @@ -1194,6 +1194,16 @@ struct rte_intr_conf {
>>>  	uint32_t rmv:1;
>>>  };
>>>
>>> +/**
>>> + * A structure used to enable/disable error packet drop on RX.
>>> + */
>>> +struct rte_rx_err_pkt_drop_conf {
>>> +	/** enable/disable all RX error packet drop.
>>> +	 * 0 (default) - disable, 1 enable
>>> +	 */
>>> +	uint32_t all:1;
>>
>> "all" is bad. It should be granular and in the same terms
>> as mask in dev_info capability.
> 
> Consider that application do not want to receive any error packets as it will drop all
> the error packets after increasing the error counter maintainer in the application.
> If same functionality can be done by the hardware where hardware maintains the error
> statistics, then why not have the same?
> When hardware does parsing of packet headers and found error during that parsing,
> this is to say to HW - hey drop any such error packets which you found during parsing
> and just increase specific error counter (if present).
> 
> I agree, granular shall also be added, but as our driver does not support them, so I did
> not add those bits here.

There is always precise definition behind "all".
You can report it in dev_info, an application can get it and
use as is (all reported capability bits) in configuration.

> 
>>
>>> +};
>>> +
>>>  /**
>>>   * A structure used to configure an Ethernet port.
>>>   * Depending upon the RX multi-queue mode, extra advanced
>>> @@ -1236,6 +1246,8 @@ struct rte_eth_conf {
>>>  	uint32_t dcb_capability_en;
>>>  	struct rte_fdir_conf fdir_conf; /**< FDIR configuration. DEPRECATED */
>>>  	struct rte_intr_conf intr_conf; /**< Interrupt mode configuration. */
>>> +	struct rte_rx_err_pkt_drop_conf err_pkt_drop_conf;
>>> +	/**< RX error packet drop configuration. */
>>
>> Why not per queue?
> 
> We do not support per queue configuration for error packet drop, but only
> on ethernet basis.
> 
>>
>>>  };
>>>
>>>  /**
>>> @@ -1260,6 +1272,7 @@ struct rte_eth_conf {
>>>  #define DEV_RX_OFFLOAD_SCTP_CKSUM	0x00020000
>>>  #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM  0x00040000
>>>  #define DEV_RX_OFFLOAD_RSS_HASH		0x00080000
>>> +#define DEV_RX_OFFLOAD_ERR_PKT_DROP	0x00100000
>>>
>>>  #define DEV_RX_OFFLOAD_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM | \
>>>  				 DEV_RX_OFFLOAD_UDP_CKSUM | \
>>> @@ -1274,6 +1287,13 @@ struct rte_eth_conf {
>>>   * mentioned in rte_rx_offload_names in rte_ethdev.c file.
>>>   */
>>>
>>> +/**
>>> + * RX Error Drop offload config/capabilities of a device. These
>>> + * are valid only when RX capability DEV_RX_OFFLOAD_ERR_PKT_DROP
>>> + * is supported by the device.
>>> + */
>>> +#define DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL		0x00000001
>>> +
>>
>> I strictly dislike "all". It will always be bad defined.
>> It must be granular.
>>
>>>  /**
>>>   * TX offload capabilities of a device.
>>>   */
>>> @@ -1411,6 +1431,8 @@ struct rte_eth_dev_info {
>>>  	/**< Device per-queue RX offload capabilities. */
>>>  	uint64_t tx_queue_offload_capa;
>>>  	/**< Device per-queue TX offload capabilities. */
>>> +	uint64_t rx_err_drop_offload_capa;
>>> +	/**< RX error packet drop offload capabilities. */
>>>  	uint16_t reta_size;
>>>  	/**< Device redirection table size, the total number of entries. */
>>>  	uint8_t hash_key_size; /**< Hash key size in bytes */
>>>
> 

Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error packets

[Nipun Gupta]

> -----Original Message-----
> From: Andrew Rybchenko <Andrew.Rybchenko@oktetlabs.ru>
> Sent: Monday, October 12, 2020 5:53 PM
> To: Nipun Gupta <nipun.gupta@nxp.com>; dev@dpdk.org
> Cc: thomas@monjalon.net; ferruh.yigit@intel.com; arybchenko@solarflare.com;
> Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>;
> jerinjacobk@gmail.com; stephen@networkplumber.org; asafp@nvidia.com
> Subject: Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error
> packets
> 
> On 10/12/20 2:30 PM, Nipun Gupta wrote:
> >
> >
> >> -----Original Message-----
> >> From: Andrew Rybchenko <Andrew.Rybchenko@oktetlabs.ru>
> >> Sent: Monday, October 12, 2020 1:32 PM
> >> To: Nipun Gupta <nipun.gupta@nxp.com>; dev@dpdk.org
> >> Cc: thomas@monjalon.net; ferruh.yigit@intel.com;
> arybchenko@solarflare.com;
> >> Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> >> <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>;
> >> jerinjacobk@gmail.com; stephen@networkplumber.org; asafp@nvidia.com
> >> Subject: Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error
> >> packets
> >>
> >> On 10/9/20 4:13 PM, nipun.gupta@nxp.com wrote:
> >>> From: Nipun Gupta <nipun.gupta@nxp.com>
> >>>
> >>> This change adds a RX offload capability and configuration to
> >>> enable hardware to drop the packets in case of any error in the
> >>> packets such as L3 checksum error or L4 checksum.
> >>>
> >>> Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> >>> Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> >>> Reviewed-by: Asaf Penso <asafp@nvidia.com>
> >>> ---
> >>>
> >>> v3:
> >>>  - Add additional rx_err_drop_offload_capa, which is specific
> >>>    capability flag for RX packets error drop offload. Currently
> >>>    only 'all' error packet drops are enabled, but can be extended
> >>>    to provide capability to drop any specific errors like L1 FCS,
> >>>    L3 Checksum etc.
> >>>  - Added separate config structure to enable the drop configuration.
> >>>  - Updated doc with the new updated option in testbbdev (patch 3/3)
> >>>
> >>> v2:
> >>>  - Add support in DPAA1 driver (patch 2/3)
> >>>  - Add support and config parameter in testpmd (patch 3/3)
> >>>
> >>>  lib/librte_ethdev/rte_ethdev.c |  1 +
> >>>  lib/librte_ethdev/rte_ethdev.h | 22 ++++++++++++++++++++++
> >>>  2 files changed, 23 insertions(+)
> >>>
> >>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> >>> index 48d1333b1..be25e947e 100644
> >>> --- a/lib/librte_ethdev/rte_ethdev.c
> >>> +++ b/lib/librte_ethdev/rte_ethdev.c
> >>> @@ -128,6 +128,7 @@ static const struct {
> >>>  	RTE_RX_OFFLOAD_BIT2STR(SCTP_CKSUM),
> >>>  	RTE_RX_OFFLOAD_BIT2STR(OUTER_UDP_CKSUM),
> >>>  	RTE_RX_OFFLOAD_BIT2STR(RSS_HASH),
> >>> +	RTE_RX_OFFLOAD_BIT2STR(ERR_PKT_DROP),
> >>>  };
> >>>
> >>>  #undef RTE_RX_OFFLOAD_BIT2STR
> >>> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> >>> index d2bf74f12..cb968d38a 100644
> >>> --- a/lib/librte_ethdev/rte_ethdev.h
> >>> +++ b/lib/librte_ethdev/rte_ethdev.h
> >>> @@ -1194,6 +1194,16 @@ struct rte_intr_conf {
> >>>  	uint32_t rmv:1;
> >>>  };
> >>>
> >>> +/**
> >>> + * A structure used to enable/disable error packet drop on RX.
> >>> + */
> >>> +struct rte_rx_err_pkt_drop_conf {
> >>> +	/** enable/disable all RX error packet drop.
> >>> +	 * 0 (default) - disable, 1 enable
> >>> +	 */
> >>> +	uint32_t all:1;
> >>
> >> "all" is bad. It should be granular and in the same terms
> >> as mask in dev_info capability.
> >
> > Consider that application do not want to receive any error packets as it will
> drop all
> > the error packets after increasing the error counter maintainer in the
> application.
> > If same functionality can be done by the hardware where hardware maintains
> the error
> > statistics, then why not have the same?
> > When hardware does parsing of packet headers and found error during that
> parsing,
> > this is to say to HW - hey drop any such error packets which you found during
> parsing
> > and just increase specific error counter (if present).
> >
> > I agree, granular shall also be added, but as our driver does not support them,
> so I did
> > not add those bits here.
> 
> There is always precise definition behind "all".
> You can report it in dev_info, an application can get it and
> use as is (all reported capability bits) in configuration.

Maybe Ill define individual error also (like l3_csum, l4_csum etc), and then 'all'
will make more sense. It will indicate that all the bits are on irrespective of
individual bits and can also be used in case if offload of individual error packet
drops type is not supported, but dropping all of them can be supported.

> 
> >
> >>
> >>> +};
> >>> +
> >>>  /**
> >>>   * A structure used to configure an Ethernet port.
> >>>   * Depending upon the RX multi-queue mode, extra advanced
> >>> @@ -1236,6 +1246,8 @@ struct rte_eth_conf {
> >>>  	uint32_t dcb_capability_en;
> >>>  	struct rte_fdir_conf fdir_conf; /**< FDIR configuration. DEPRECATED */
> >>>  	struct rte_intr_conf intr_conf; /**< Interrupt mode configuration. */
> >>> +	struct rte_rx_err_pkt_drop_conf err_pkt_drop_conf;
> >>> +	/**< RX error packet drop configuration. */
> >>
> >> Why not per queue?
> >
> > We do not support per queue configuration for error packet drop, but only
> > on ethernet basis.
> >
> >>
> >>>  };
> >>>
> >>>  /**
> >>> @@ -1260,6 +1272,7 @@ struct rte_eth_conf {
> >>>  #define DEV_RX_OFFLOAD_SCTP_CKSUM	0x00020000
> >>>  #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM  0x00040000
> >>>  #define DEV_RX_OFFLOAD_RSS_HASH		0x00080000
> >>> +#define DEV_RX_OFFLOAD_ERR_PKT_DROP	0x00100000
> >>>
> >>>  #define DEV_RX_OFFLOAD_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM |
> \
> >>>  				 DEV_RX_OFFLOAD_UDP_CKSUM | \
> >>> @@ -1274,6 +1287,13 @@ struct rte_eth_conf {
> >>>   * mentioned in rte_rx_offload_names in rte_ethdev.c file.
> >>>   */
> >>>
> >>> +/**
> >>> + * RX Error Drop offload config/capabilities of a device. These
> >>> + * are valid only when RX capability DEV_RX_OFFLOAD_ERR_PKT_DROP
> >>> + * is supported by the device.
> >>> + */
> >>> +#define DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL		0x00000001
> >>> +
> >>
> >> I strictly dislike "all". It will always be bad defined.
> >> It must be granular.
> >>
> >>>  /**
> >>>   * TX offload capabilities of a device.
> >>>   */
> >>> @@ -1411,6 +1431,8 @@ struct rte_eth_dev_info {
> >>>  	/**< Device per-queue RX offload capabilities. */
> >>>  	uint64_t tx_queue_offload_capa;
> >>>  	/**< Device per-queue TX offload capabilities. */
> >>> +	uint64_t rx_err_drop_offload_capa;
> >>> +	/**< RX error packet drop offload capabilities. */
> >>>  	uint16_t reta_size;
> >>>  	/**< Device redirection table size, the total number of entries. */
> >>>  	uint8_t hash_key_size; /**< Hash key size in bytes */
> >>>
> >

Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error packets

[Andrew Rybchenko]

On 10/12/20 3:22 PM, Andrew Rybchenko wrote:
> On 10/12/20 2:30 PM, Nipun Gupta wrote:
>>> -----Original Message-----
>>> From: Andrew Rybchenko <Andrew.Rybchenko@oktetlabs.ru>
>>> Sent: Monday, October 12, 2020 1:32 PM
>>> To: Nipun Gupta <nipun.gupta@nxp.com>; dev@dpdk.org
>>> Cc: thomas@monjalon.net; ferruh.yigit@intel.com; arybchenko@solarflare.com;
>>> Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
>>> <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>;
>>> jerinjacobk@gmail.com; stephen@networkplumber.org; asafp@nvidia.com
>>> Subject: Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error
>>> packets
>>>
>>> On 10/9/20 4:13 PM, nipun.gupta@nxp.com wrote:
>>>> From: Nipun Gupta <nipun.gupta@nxp.com>
>>>>
>>>> This change adds a RX offload capability and configuration to
>>>> enable hardware to drop the packets in case of any error in the
>>>> packets such as L3 checksum error or L4 checksum.
>>>>
>>>> Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
>>>> Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
>>>> Reviewed-by: Asaf Penso <asafp@nvidia.com>

Thinking a bit more about it I agree with Thomas idea that
it should be flow API based solution in fact.
Drop is just a one of possible actions to be done with
packets with bad checksum on one or another layer.
Such packets could be redirected to a slow path
(dedicated queue or port ID (PF, VF)).
It is just a missing feature in various layer
pattern match to say if we want to proceed with packets
with only good or only bad chehcksum (or we don't care
as we do right now). Exact match for checksums is hardly
useful except UDP with zero checksum case.

Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error packets

[Nipun Gupta]

Hi Thomas and Jerin,

There seems two views:
1. Use of existing rx_offload_capa, to have additional error packet drop offload capabilities. This is showcased in v2, and also suggested by Thomas.
2. Adding additional offload capability flag types and configuration structure for error packet drop configuration as suggested by Jerin, which is part of v3.

Please let me know which one shall I continue with for the next spin.

Thanks,
Nipun

> -----Original Message-----
> From: Nipun Gupta
> Sent: Monday, October 12, 2020 11:11 AM
> To: Thomas Monjalon <thomas@monjalon.net>; jerinjacobk@gmail.com
> Cc: dev@dpdk.org; ferruh.yigit@intel.com; arybchenko@solarflare.com;
> Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>;
> stephen@networkplumber.org; asafp@nvidia.com
> Subject: RE: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error
> packets
> 
> 
> 
> > -----Original Message-----
> > From: Thomas Monjalon <thomas@monjalon.net>
> > Sent: Monday, October 12, 2020 3:12 AM
> > To: Nipun Gupta <nipun.gupta@nxp.com>
> > Cc: dev@dpdk.org; ferruh.yigit@intel.com; arybchenko@solarflare.com;
> > Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> > <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>;
> > jerinjacobk@gmail.com; stephen@networkplumber.org; asafp@nvidia.com
> > Subject: Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error
> > packets
> >
> > Hi,
> >
> > The configuration of this feature is not clear to me.
> > Please see the comments below.
> >
> > 09/10/2020 15:13, nipun.gupta@nxp.com:
> > > From: Nipun Gupta <nipun.gupta@nxp.com>
> > >
> > > This change adds a RX offload capability and configuration to
> > > enable hardware to drop the packets in case of any error in the
> > > packets such as L3 checksum error or L4 checksum.
> > >
> > > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> > > Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> > > Reviewed-by: Asaf Penso <asafp@nvidia.com>
> > > ---
> > [...]
> > > +/**
> > > + * A structure used to enable/disable error packet drop on RX.
> >
> > RX -> Rx
> > same for other occurences below
> 
> Okay.
> 
> >
> > > + */
> > > +struct rte_rx_err_pkt_drop_conf {
> >
> > The name should start with rte_eth_
> >
> > > +	/** enable/disable all RX error packet drop.
> > > +	 * 0 (default) - disable, 1 enable
> > > +	 */
> > > +	uint32_t all:1;
> > > +};
> >
> > I don't understand the meaning of this struct.
> > Is it just one bit to drop packets having an error?
> > How do you determine what is an error?
> 
> The error packets which are dropped on the hardware can be from
> various reasons, like L1 FCS, L3 CSUM, L4 CSUM or HW can be configured
> to drop ALL (or any) of the error packets received on the interface.
> 
> Currently DPAA drivers support drop of ALL the error packets, but does
> not support configuration of packet drops on basis of separate error types
> like dropping only L2 CSUM, L3 CSUM. So this patch is supporting only drop
> of 'ALL' the errors packets received on HW.
> 
> Marvell supports drop on separate error types, thus Jerin suggested to have
> this structure so that it opens the room for adding separate configuration
> to drop packets on basis of separate error types.
> 
> Jerin,
> maybe you can add more on this.
> 
> >
> > [...]
> > > @@ -1236,6 +1246,8 @@ struct rte_eth_conf {
> > >  	uint32_t dcb_capability_en;
> > >  	struct rte_fdir_conf fdir_conf; /**< FDIR configuration. DEPRECATED */
> > >  	struct rte_intr_conf intr_conf; /**< Interrupt mode configuration. */
> > > +	struct rte_rx_err_pkt_drop_conf err_pkt_drop_conf;
> > > +	/**< RX error packet drop configuration. */
> >
> > Why a per-port configuration is needed in addition of the per-queue offload?
> >
> > [...]
> > > @@ -1260,6 +1272,7 @@ struct rte_eth_conf {
> > >  #define DEV_RX_OFFLOAD_SCTP_CKSUM	0x00020000
> > >  #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM  0x00040000
> > >  #define DEV_RX_OFFLOAD_RSS_HASH		0x00080000
> > > +#define DEV_RX_OFFLOAD_ERR_PKT_DROP	0x00100000
> >
> > New offload names should starte with RTE_ prefix.
> 
> Okay
> 
> >
> >
> > > +/**
> > > + * RX Error Drop offload config/capabilities of a device. These
> > > + * are valid only when RX capability DEV_RX_OFFLOAD_ERR_PKT_DROP
> > > + * is supported by the device.
> > > + */
> > > +#define DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL		0x00000001
> >
> > I don't understand the meaning.
> 
> For DPAA drivers there is only one offload required, but once separate offloads
> are added to drop packets on basis of separate errors, then it can bloat up the
> offload capabilities. So we added new flags ' rte_rx_err_pkt_drop_conf ' instead
> of adding this onto 'rx_offload_capa'.
> 
> There is only one flag ' DEV_RX_OFFLOAD_ERR_PKT_DROP' added in
> 'rx_offload_capa'
> and if this flag is enabled, one or more of the flag in '
> DEV_RX_ERR_PKT_DROP_OFFLOAD...'
> can be enabled (currently only DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL) being
> supported
> Others like:
> - DEV_RX_ERR_PKT_DROP_OFFLOAD_L1_FCS
> - DEV_RX_ERR_PKT_DROP_OFFLOAD_L3_CSUM
> - DEV_RX_ERR_PKT_DROP_OFFLOAD_L4_CSUM etc.
> Can be added once any reference driver is supporting this.
> 
> P.S. I can add these flags in this patch but without any ref driver support.
> 
> >
> > > @@ -1411,6 +1431,8 @@ struct rte_eth_dev_info {
> > >  	/**< Device per-queue RX offload capabilities. */
> > >  	uint64_t tx_queue_offload_capa;
> > >  	/**< Device per-queue TX offload capabilities. */
> > > +	uint64_t rx_err_drop_offload_capa;
> > > +	/**< RX error packet drop offload capabilities. */
> >
> > Why adding a new field here instead of reporting in rx_offload_capa?
> 
> This was part of v2 where flag was added in rx_offload_capa.
> 
> Both ways (v2 and v3 versions) seems to have pros and cons:
> - Adding only in the rx_offload_capa' seems simpler to implement and use
> - But it shall lead to increase in the offload capability types when packet
>   drop configuration is added on individual error types.
> 
> Regards,
> Nipun
> 
> >
> >

Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error packets

[Nipun Gupta]

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Tuesday, October 13, 2020 12:52 PM
> To: Nipun Gupta <nipun.gupta@nxp.com>; dev@dpdk.org
> Cc: thomas@monjalon.net; ferruh.yigit@intel.com; Hemant Agrawal
> <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>; Rohit
> Raj <rohit.raj@nxp.com>; jerinjacobk@gmail.com;
> stephen@networkplumber.org; asafp@nvidia.com
> Subject: Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error
> packets
> 
> On 10/12/20 3:22 PM, Andrew Rybchenko wrote:
> > On 10/12/20 2:30 PM, Nipun Gupta wrote:
> >>> -----Original Message-----
> >>> From: Andrew Rybchenko <Andrew.Rybchenko@oktetlabs.ru>
> >>> Sent: Monday, October 12, 2020 1:32 PM
> >>> To: Nipun Gupta <nipun.gupta@nxp.com>; dev@dpdk.org
> >>> Cc: thomas@monjalon.net; ferruh.yigit@intel.com;
> arybchenko@solarflare.com;
> >>> Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> >>> <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>;
> >>> jerinjacobk@gmail.com; stephen@networkplumber.org; asafp@nvidia.com
> >>> Subject: Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error
> >>> packets
> >>>
> >>> On 10/9/20 4:13 PM, nipun.gupta@nxp.com wrote:
> >>>> From: Nipun Gupta <nipun.gupta@nxp.com>
> >>>>
> >>>> This change adds a RX offload capability and configuration to
> >>>> enable hardware to drop the packets in case of any error in the
> >>>> packets such as L3 checksum error or L4 checksum.
> >>>>
> >>>> Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> >>>> Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> >>>> Reviewed-by: Asaf Penso <asafp@nvidia.com>
> 
> Thinking a bit more about it I agree with Thomas idea that
> it should be flow API based solution in fact.
> Drop is just a one of possible actions to be done with
> packets with bad checksum on one or another layer.
> Such packets could be redirected to a slow path
> (dedicated queue or port ID (PF, VF)).
> It is just a missing feature in various layer
> pattern match to say if we want to proceed with packets
> with only good or only bad chehcksum (or we don't care
> as we do right now). Exact match for checksums is hardly
> useful except UDP with zero checksum case.

I would think of it applicable to both, i.e. it could fit in config option
as well as in flow (e.g. RSS we also have as part of both config and flow).
Having this in flow would increase usage like redirecting as you mentioned,
and having in the config will increase utility when simple config like RSS is
used without flow API's.

Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error packets

[Andrew Rybchenko]

On 10/13/20 10:36 AM, Nipun Gupta wrote:
> 
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Sent: Tuesday, October 13, 2020 12:52 PM
>> To: Nipun Gupta <nipun.gupta@nxp.com>; dev@dpdk.org
>> Cc: thomas@monjalon.net; ferruh.yigit@intel.com; Hemant Agrawal
>> <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>; Rohit
>> Raj <rohit.raj@nxp.com>; jerinjacobk@gmail.com;
>> stephen@networkplumber.org; asafp@nvidia.com
>> Subject: Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error
>> packets
>>
>> On 10/12/20 3:22 PM, Andrew Rybchenko wrote:
>>> On 10/12/20 2:30 PM, Nipun Gupta wrote:
>>>>> -----Original Message-----
>>>>> From: Andrew Rybchenko <Andrew.Rybchenko@oktetlabs.ru>
>>>>> Sent: Monday, October 12, 2020 1:32 PM
>>>>> To: Nipun Gupta <nipun.gupta@nxp.com>; dev@dpdk.org
>>>>> Cc: thomas@monjalon.net; ferruh.yigit@intel.com;
>> arybchenko@solarflare.com;
>>>>> Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
>>>>> <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>;
>>>>> jerinjacobk@gmail.com; stephen@networkplumber.org; asafp@nvidia.com
>>>>> Subject: Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error
>>>>> packets
>>>>>
>>>>> On 10/9/20 4:13 PM, nipun.gupta@nxp.com wrote:
>>>>>> From: Nipun Gupta <nipun.gupta@nxp.com>
>>>>>>
>>>>>> This change adds a RX offload capability and configuration to
>>>>>> enable hardware to drop the packets in case of any error in the
>>>>>> packets such as L3 checksum error or L4 checksum.
>>>>>>
>>>>>> Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
>>>>>> Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
>>>>>> Reviewed-by: Asaf Penso <asafp@nvidia.com>
>>
>> Thinking a bit more about it I agree with Thomas idea that
>> it should be flow API based solution in fact.
>> Drop is just a one of possible actions to be done with
>> packets with bad checksum on one or another layer.
>> Such packets could be redirected to a slow path
>> (dedicated queue or port ID (PF, VF)).
>> It is just a missing feature in various layer
>> pattern match to say if we want to proceed with packets
>> with only good or only bad chehcksum (or we don't care
>> as we do right now). Exact match for checksums is hardly
>> useful except UDP with zero checksum case.
> 
> I would think of it applicable to both, i.e. it could fit in config option
> as well as in flow (e.g. RSS we also have as part of both config and flow).
> Having this in flow would increase usage like redirecting as you mentioned,
> and having in the config will increase utility when simple config like RSS is
> used without flow API's.

I dislike the idea to have it on both layers.
It adds conflicts by design. It should be no duplication.
That's why mirroring API is deprecated and will be removed
since we have the functionality via flow API now.

Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error packets

[Nipun Gupta]

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Tuesday, October 13, 2020 1:21 PM
> To: Nipun Gupta <nipun.gupta@nxp.com>; dev@dpdk.org
> Cc: thomas@monjalon.net; ferruh.yigit@intel.com; Hemant Agrawal
> <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>; Rohit
> Raj <rohit.raj@nxp.com>; jerinjacobk@gmail.com;
> stephen@networkplumber.org; asafp@nvidia.com
> Subject: Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error
> packets
> 
> On 10/13/20 10:36 AM, Nipun Gupta wrote:
> >
> >
> >> -----Original Message-----
> >> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >> Sent: Tuesday, October 13, 2020 12:52 PM
> >> To: Nipun Gupta <nipun.gupta@nxp.com>; dev@dpdk.org
> >> Cc: thomas@monjalon.net; ferruh.yigit@intel.com; Hemant Agrawal
> >> <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>;
> Rohit
> >> Raj <rohit.raj@nxp.com>; jerinjacobk@gmail.com;
> >> stephen@networkplumber.org; asafp@nvidia.com
> >> Subject: Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error
> >> packets
> >>
> >> On 10/12/20 3:22 PM, Andrew Rybchenko wrote:
> >>> On 10/12/20 2:30 PM, Nipun Gupta wrote:
> >>>>> -----Original Message-----
> >>>>> From: Andrew Rybchenko <Andrew.Rybchenko@oktetlabs.ru>
> >>>>> Sent: Monday, October 12, 2020 1:32 PM
> >>>>> To: Nipun Gupta <nipun.gupta@nxp.com>; dev@dpdk.org
> >>>>> Cc: thomas@monjalon.net; ferruh.yigit@intel.com;
> >> arybchenko@solarflare.com;
> >>>>> Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> >>>>> <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>;
> >>>>> jerinjacobk@gmail.com; stephen@networkplumber.org;
> asafp@nvidia.com
> >>>>> Subject: Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop
> error
> >>>>> packets
> >>>>>
> >>>>> On 10/9/20 4:13 PM, nipun.gupta@nxp.com wrote:
> >>>>>> From: Nipun Gupta <nipun.gupta@nxp.com>
> >>>>>>
> >>>>>> This change adds a RX offload capability and configuration to
> >>>>>> enable hardware to drop the packets in case of any error in the
> >>>>>> packets such as L3 checksum error or L4 checksum.
> >>>>>>
> >>>>>> Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> >>>>>> Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> >>>>>> Reviewed-by: Asaf Penso <asafp@nvidia.com>
> >>
> >> Thinking a bit more about it I agree with Thomas idea that
> >> it should be flow API based solution in fact.
> >> Drop is just a one of possible actions to be done with
> >> packets with bad checksum on one or another layer.
> >> Such packets could be redirected to a slow path
> >> (dedicated queue or port ID (PF, VF)).
> >> It is just a missing feature in various layer
> >> pattern match to say if we want to proceed with packets
> >> with only good or only bad chehcksum (or we don't care
> >> as we do right now). Exact match for checksums is hardly
> >> useful except UDP with zero checksum case.
> >
> > I would think of it applicable to both, i.e. it could fit in config option
> > as well as in flow (e.g. RSS we also have as part of both config and flow).
> > Having this in flow would increase usage like redirecting as you mentioned,
> > and having in the config will increase utility when simple config like RSS is
> > used without flow API's.
> 
> I dislike the idea to have it on both layers.
> It adds conflicts by design. It should be no duplication.
> That's why mirroring API is deprecated and will be removed
> since we have the functionality via flow API now.

I would like to agree, but looking at some general configuration which we have
part of both config and flow like RSS packet distribution. It seems it is part of config
too apart from flow and most sample applications using the same because of
simplicity of use?
Seems stripping of VLAN is in similar lines:  RTE_FLOW_ACTION_TYPE_OF_POP_VLAN
and DEV_RX_OFFLOAD_VLAN_STRIP?

[dpdk-dev] [PATCH 1/3 v4] ethdev: add Rx offload to drop error packets

[nipun.gupta]

From: Nipun Gupta <nipun.gupta@nxp.com>

This change adds a Rx offload capability and configuration to
enable hardware to drop the packets in case of any error in the
packets such as L3 checksum error or L4 checksum.

Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
Reviewed-by: Asaf Penso <asafp@nvidia.com>
---

v4:
 - renamed 'rte_rx_err_pkt_drop_conf' to
   'rte_eth_rx_err_pkt_drop_conf'
 - updated function 'port_offload_cap_display' to display newly
   added offloads
 - added placeholder for L1 FCS, L3 Checksum, L4 Checksum error
   packet drops
 - updated doc/guides/nics/features.rst
 - updated new added 'DEV_RX_OFFLOAD_*' to 'RTE_DEV_RX_OFFLOAD*'
 - updated RX to Rx

v3:
 - Add additional rx_err_drop_offload_capa, which is specific
   capability flag for RX packets error drop offload. Currently
   only 'all' error packet drops are enabled, but can be extended
   to provide capability to drop any specific errors like L1 FCS,
   L3 Checksum etc.
 - Added separate config structure to enable the drop configuration.
 - Updated doc with the new updated option in testbbdev (patch 3/3)

v2:
 - Add support in DPAA1 driver (patch 2/3)
 - Add support and config parameter in testpmd (patch 3/3)

 lib/librte_ethdev/rte_ethdev.c |  1 +
 lib/librte_ethdev/rte_ethdev.h | 39 +++++++++++++++++++++++++++++++++-
 2 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 48d1333b1..be25e947e 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -128,6 +128,7 @@ static const struct {
 	RTE_RX_OFFLOAD_BIT2STR(SCTP_CKSUM),
 	RTE_RX_OFFLOAD_BIT2STR(OUTER_UDP_CKSUM),
 	RTE_RX_OFFLOAD_BIT2STR(RSS_HASH),
+	RTE_RX_OFFLOAD_BIT2STR(ERR_PKT_DROP),
 };
 
 #undef RTE_RX_OFFLOAD_BIT2STR
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index d2bf74f12..0db4b4021 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1194,6 +1194,28 @@ struct rte_intr_conf {
 	uint32_t rmv:1;
 };
 
+/**
+ * A structure used to enable/disable error packet drop on Rx.
+ */
+struct rte_eth_rx_err_pkt_drop_conf {
+	/** enable/disable L1 FSC error packet drop on Rx.
+	 * 0 (default) - disable, 1 enable
+	 */
+	uint32_t l1_fcs:1;
+	/** enable/disable L3 Checksum error packet drop on Rx.
+	 * 0 (default) - disable, 1 enable
+	 */
+	uint32_t l3_csum:1;
+	/** enable/disable L4 Checksum error packet drop on Rx.
+	 * 0 (default) - disable, 1 enable
+	 */
+	uint32_t l4_csum:1;
+	/** enable/disable all Rx error packet drop.
+	 * 0 (default) - disable, 1 enable
+	 */
+	uint32_t all:1;
+};
+
 /**
  * A structure used to configure an Ethernet port.
  * Depending upon the RX multi-queue mode, extra advanced
@@ -1236,10 +1258,12 @@ struct rte_eth_conf {
 	uint32_t dcb_capability_en;
 	struct rte_fdir_conf fdir_conf; /**< FDIR configuration. DEPRECATED */
 	struct rte_intr_conf intr_conf; /**< Interrupt mode configuration. */
+	struct rte_eth_rx_err_pkt_drop_conf err_pkt_drop_conf;
+	/**< Rx error packet drop configuration. */
 };
 
 /**
- * RX offload capabilities of a device.
+ * Rx offload capabilities of a device.
  */
 #define DEV_RX_OFFLOAD_VLAN_STRIP  0x00000001
 #define DEV_RX_OFFLOAD_IPV4_CKSUM  0x00000002
@@ -1260,6 +1284,7 @@ struct rte_eth_conf {
 #define DEV_RX_OFFLOAD_SCTP_CKSUM	0x00020000
 #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM  0x00040000
 #define DEV_RX_OFFLOAD_RSS_HASH		0x00080000
+#define DEV_RX_OFFLOAD_ERR_PKT_DROP	0x00100000
 
 #define DEV_RX_OFFLOAD_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM | \
 				 DEV_RX_OFFLOAD_UDP_CKSUM | \
@@ -1274,6 +1299,16 @@ struct rte_eth_conf {
  * mentioned in rte_rx_offload_names in rte_ethdev.c file.
  */
 
+/**
+ * Rx Error Drop offload config/capabilities of a device. These
+ * are valid only when Rx capability RTE_DEV_RX_OFFLOAD_ERR_PKT_DROP
+ * is supported by the device.
+ */
+#define RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L1_FCS		0x00000001
+#define RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L3_CSUM		0x00000002
+#define RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L4_CSUM		0x00000004
+#define RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL		0x80000000
+
 /**
  * TX offload capabilities of a device.
  */
@@ -1411,6 +1446,8 @@ struct rte_eth_dev_info {
 	/**< Device per-queue RX offload capabilities. */
 	uint64_t tx_queue_offload_capa;
 	/**< Device per-queue TX offload capabilities. */
+	uint64_t rx_err_drop_offload_capa;
+	/**< RX error packet drop offload capabilities. */
 	uint16_t reta_size;
 	/**< Device redirection table size, the total number of entries. */
 	uint8_t hash_key_size; /**< Hash key size in bytes */
-- 
2.17.1

[dpdk-dev] [PATCH 2/3 v4] net/dpaa: support Rx offload for error packet drop

[nipun.gupta]

From: Nipun Gupta <nipun.gupta@nxp.com>

This patch supports Rx offload configuration to drop error
packets in the hardware

Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
---
 drivers/bus/dpaa/base/fman/fman_hw.c |  7 ++++++-
 drivers/net/dpaa/dpaa_ethdev.c       | 23 +++++++++++++++++++----
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/bus/dpaa/base/fman/fman_hw.c b/drivers/bus/dpaa/base/fman/fman_hw.c
index 4ab49f785..40e4e0000 100644
--- a/drivers/bus/dpaa/base/fman/fman_hw.c
+++ b/drivers/bus/dpaa/base/fman/fman_hw.c
@@ -597,7 +597,12 @@ void
 fman_if_discard_rx_errors(struct fman_if *fm_if)
 {
 	struct __fman_if *__if = container_of(fm_if, struct __fman_if, __if);
-	unsigned int *fmbm_rfsdm, *fmbm_rfsem;
+	unsigned int *fmbm_rcfg, *fmbm_rfsdm, *fmbm_rfsem;
+	unsigned int val;
+
+	fmbm_rcfg = &((struct rx_bmi_regs *)__if->bmi_map)->fmbm_rcfg;
+	val = in_be32(fmbm_rcfg);
+	out_be32(fmbm_rcfg, val & ~BMI_PORT_CFG_FDOVR);
 
 	fmbm_rfsem = &((struct rx_bmi_regs *)__if->bmi_map)->fmbm_rfsem;
 	out_be32(fmbm_rfsem, 0);
diff --git a/drivers/net/dpaa/dpaa_ethdev.c b/drivers/net/dpaa/dpaa_ethdev.c
index af47c196a..b9cfc4fb3 100644
--- a/drivers/net/dpaa/dpaa_ethdev.c
+++ b/drivers/net/dpaa/dpaa_ethdev.c
@@ -52,7 +52,8 @@
 /* Supported Rx offloads */
 static uint64_t dev_rx_offloads_sup =
 		DEV_RX_OFFLOAD_JUMBO_FRAME |
-		DEV_RX_OFFLOAD_SCATTER;
+		DEV_RX_OFFLOAD_SCATTER |
+		DEV_RX_OFFLOAD_ERR_PKT_DROP;
 
 /* Rx offloads which cannot be disabled */
 static uint64_t dev_rx_offloads_nodis =
@@ -62,6 +63,10 @@ static uint64_t dev_rx_offloads_nodis =
 		DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM |
 		DEV_RX_OFFLOAD_RSS_HASH;
 
+/* Supported Rx Error packet drop offload */
+static uint64_t dev_rx_err_drop_offloads_sup =
+		RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL;
+
 /* Supported Tx offloads */
 static uint64_t dev_tx_offloads_sup =
 		DEV_TX_OFFLOAD_MT_LOCKFREE |
@@ -262,6 +267,18 @@ dpaa_eth_dev_configure(struct rte_eth_dev *dev)
 		dev->data->scattered_rx = 1;
 	}
 
+	if (eth_conf->err_pkt_drop_conf.all) {
+		DPAA_PMD_DEBUG("error packets will be dropped on hw");
+		fman_if_discard_rx_errors(fif);
+	} else {
+		struct dpaa_if *dpaa_intf = dev->data->dev_private;
+		struct qman_fq *rxq = &dpaa_intf->rx_queues[0];
+
+		DPAA_PMD_DEBUG("error packets will not be dropped on hw");
+		fman_if_receive_rx_errors(fif, FM_FD_RX_STATUS_ERR_MASK);
+		fman_if_set_err_fqid(fif, rxq->fqid);
+	}
+
 	if (!(default_q || fmc_q)) {
 		if (dpaa_fm_config(dev,
 			eth_conf->rx_adv_conf.rss_conf.rss_hf)) {
@@ -591,6 +608,7 @@ static int dpaa_eth_dev_info(struct rte_eth_dev *dev,
 					dev_rx_offloads_nodis;
 	dev_info->tx_offload_capa = dev_tx_offloads_sup |
 					dev_tx_offloads_nodis;
+	dev_info->rx_err_drop_offload_capa = dev_rx_err_drop_offloads_sup;
 	dev_info->default_rxportconf.burst_size = DPAA_DEF_RX_BURST_SIZE;
 	dev_info->default_txportconf.burst_size = DPAA_DEF_TX_BURST_SIZE;
 	dev_info->default_rxportconf.nb_queues = 1;
@@ -2085,9 +2103,6 @@ dpaa_dev_init(struct rte_eth_dev *eth_dev)
 		fman_intf->mac_addr.addr_bytes[5]);
 
 	if (!fman_intf->is_shared_mac) {
-		/* Configure error packet handling */
-		fman_if_receive_rx_errors(fman_intf,
-			FM_FD_RX_STATUS_ERR_MASK);
 		/* Disable RX mode */
 		fman_if_disable_rx(fman_intf);
 		/* Disable promiscuous mode */
-- 
2.17.1

[dpdk-dev] [PATCH 3/3 v4] app/testpmd: support hardware offload to drop error packets

[nipun.gupta]

From: Nipun Gupta <nipun.gupta@nxp.com>

With DEV_RX_OFFLOAD_ERR_PKT_DROP now defined as an offload
capability, and separate RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL
capability to drop all error packets in hardware, testpmd
showcases this with a new added configuration option
'enable-hw-drop-err-all'.

Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
---
 app/test-pmd/config.c                 | 35 +++++++++++++++++++++
 app/test-pmd/parameters.c             |  7 +++++
 app/test-pmd/testpmd.c                |  8 +++++
 app/test-pmd/testpmd.h                |  1 +
 doc/guides/nics/features.rst          | 44 +++++++++++++++++++++++++++
 doc/guides/testpmd_app_ug/run_app.rst |  4 +++
 6 files changed, 99 insertions(+)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index e73dc66c8..14ef4e468 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -1027,6 +1027,41 @@ port_offload_cap_display(portid_t port_id)
 			printf("off\n");
 	}
 
+	if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_ERR_PKT_DROP) {
+		if (dev_info.rx_err_drop_offload_capa &
+			RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L1_FCS) {
+			printf("RX L1 FCS Error pkt drop:      ");
+			if (ports[port_id].dev_conf.err_pkt_drop_conf.l1_fcs)
+				printf("on\n");
+			else
+				printf("off\n");
+		}
+		if (dev_info.rx_err_drop_offload_capa &
+			RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L3_CSUM) {
+			printf("RX L3 Csum Error pkt drop:     ");
+			if (ports[port_id].dev_conf.err_pkt_drop_conf.l3_csum)
+				printf("on\n");
+			else
+				printf("off\n");
+		}
+		if (dev_info.rx_err_drop_offload_capa &
+			RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L4_CSUM) {
+			printf("RX L4 Csum Error pkt drop:     ");
+			if (ports[port_id].dev_conf.err_pkt_drop_conf.l4_csum)
+				printf("on\n");
+			else
+				printf("off\n");
+		}
+		if (dev_info.rx_err_drop_offload_capa &
+			RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL) {
+			printf("RX all Error pkt drop:         ");
+			if (ports[port_id].dev_conf.err_pkt_drop_conf.all)
+				printf("on\n");
+			else
+				printf("off\n");
+		}
+	}
+
 	if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_VLAN_INSERT) {
 		printf("VLAN insert:                   ");
 		if (ports[port_id].dev_conf.txmode.offloads &
diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 1ead59579..508612426 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -142,6 +142,7 @@ usage(char* progname)
 	printf("  --enable-hw-vlan-strip: enable hardware vlan strip.\n");
 	printf("  --enable-hw-vlan-extend: enable hardware vlan extend.\n");
 	printf("  --enable-hw-qinq-strip: enable hardware qinq strip.\n");
+	printf("  --enable-hw-drop-err-all: enable hardware packet drop for all error packets.\n");
 	printf("  --enable-drop-en: enable per queue packet drop.\n");
 	printf("  --disable-rss: disable rss.\n");
 	printf("  --port-topology=<paired|chained|loop>: set port topology (paired "
@@ -631,6 +632,7 @@ launch_args_parse(int argc, char** argv)
 		{ "enable-hw-vlan-strip",       0, 0, 0 },
 		{ "enable-hw-vlan-extend",      0, 0, 0 },
 		{ "enable-hw-qinq-strip",       0, 0, 0 },
+		{ "enable-hw-drop-err-all",     0, 0, 0 },
 		{ "enable-drop-en",            0, 0, 0 },
 		{ "disable-rss",                0, 0, 0 },
 		{ "port-topology",              1, 0, 0 },
@@ -1283,6 +1285,11 @@ launch_args_parse(int argc, char** argv)
 				rmv_interrupt = 0;
 			if (!strcmp(lgopts[opt_idx].name, "flow-isolate-all"))
 				flow_isolate_all = 1;
+			if (!strcmp(lgopts[opt_idx].name,
+					"enable-hw-drop-err-all")) {
+				rx_err_pkt_drop_all = 1;
+			}
+
 			if (!strcmp(lgopts[opt_idx].name, "tx-offloads")) {
 				char *end = NULL;
 				n = strtoull(optarg, &end, 16);
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index ccba71c07..c9e7397e6 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -359,6 +359,11 @@ uint8_t lsc_interrupt = 1; /* enabled by default */
  */
 uint8_t rmv_interrupt = 1; /* enabled by default */
 
+/*
+ * Drop all RX error packets on HW itself.
+ */
+uint8_t rx_err_pkt_drop_all = 0; /* disabled by default */
+
 uint8_t hot_plug = 0; /**< hotplug disabled by default. */
 
 /* After attach, port setup is called on event or by iterator */
@@ -3359,6 +3364,9 @@ init_port_config(void)
 		    (rte_eth_devices[pid].data->dev_flags &
 		     RTE_ETH_DEV_INTR_RMV))
 			port->dev_conf.intr_conf.rmv = 1;
+
+		if (rx_err_pkt_drop_all)
+			port->dev_conf.err_pkt_drop_conf.all = 1;
 	}
 }
 
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index c7e7e41a9..eab154ed4 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -314,6 +314,7 @@ extern uint8_t no_device_start; /**<set by "--disable-device-start" parameter */
 extern volatile int test_done; /* stop packet forwarding when set to 1. */
 extern uint8_t lsc_interrupt; /**< disabled by "--no-lsc-interrupt" parameter */
 extern uint8_t rmv_interrupt; /**< disabled by "--no-rmv-interrupt" parameter */
+extern uint8_t rx_err_pkt_drop_all; /**< enabled by "--enable-hw-drop-err-all" parameter */
 extern uint32_t event_print_mask;
 /**< set by "--print-event xxxx" and "--mask-event xxxx parameters */
 extern bool setup_on_probe_event; /**< disabled by port setup-on iterator */
diff --git a/doc/guides/nics/features.rst b/doc/guides/nics/features.rst
index dd8c9555b..cb7fdfd8b 100644
--- a/doc/guides/nics/features.rst
+++ b/doc/guides/nics/features.rst
@@ -606,6 +606,50 @@ Supports inner packet L4 checksum.
   ``tx_offload_capa,tx_queue_offload_capa:DEV_TX_OFFLOAD_OUTER_UDP_CKSUM``.
 
 
+.. _nic_features_l1_fcs_rx_error_packet_drop:
+
+L1 FCS Error Packet drop on Rx
+------------------------------
+
+Supports dropping of packets having L1 FCS error on Rx.
+
+* **[uses]     user config**: ``dev_conf.err_pkt_drop_conf.l1_fcs``.
+* **[provides] rte_eth_dev_info**: ``rx_err_drop_offload_capa:RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L1_FCS``.
+
+
+.. _nic_features_l3_csum_rx_error_packet_drop:
+
+L3 checksum Error Packet drop on Rx
+-----------------------------------
+
+Supports dropping of packets having L3 Checksum error on Rx.
+
+* **[uses]     user config**: ``dev_conf.err_pkt_drop_conf.l3_csum``.
+* **[provides] rte_eth_dev_info**: ``rx_err_drop_offload_capa:RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L3_CSUM``.
+
+
+.. _nic_features_l4_csum_rx_error_packet_drop:
+
+L4 Checksum Error Packet drop on Rx
+-----------------------------------
+
+Supports dropping of packets having L1 FCS error on Rx.
+
+* **[uses]     user config**: ``dev_conf.err_pkt_drop_conf.l4_csum``.
+* **[provides] rte_eth_dev_info**: ``rx_err_drop_offload_capa:RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L4_CSUM``.
+
+
+.. _nic_features_all_rx_error_packet_drop:
+
+All/any Error Packet drop on Rx
+-------------------------------
+
+Supports dropping of packets having any of the errors like L1 FSC, L3/L4 Checksum on Rx.
+
+* **[uses]     user config**: ``dev_conf.err_pkt_drop_conf.all``.
+* **[provides] rte_eth_dev_info**: ``rx_err_drop_offload_capa:RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL``.
+
+
 .. _nic_features_packet_type_parsing:
 
 Packet type parsing
diff --git a/doc/guides/testpmd_app_ug/run_app.rst b/doc/guides/testpmd_app_ug/run_app.rst
index e2539f693..20f2f8083 100644
--- a/doc/guides/testpmd_app_ug/run_app.rst
+++ b/doc/guides/testpmd_app_ug/run_app.rst
@@ -213,6 +213,10 @@ The command line options are:
 
     Enable hardware QINQ strip.
 
+*  ``--enable-hw-drop-err-all``
+
+    Enable hardware packet drop for any error packets
+
 *   ``--enable-drop-en``
 
     Enable per-queue packet drop for packets with no descriptors.
-- 
2.17.1

Re: [dpdk-dev] [PATCH 1/3 v4] ethdev: add Rx offload to drop error packets

[Ajit Khaparde]

On Thu, Oct 15, 2020 at 6:24 AM <nipun.gupta@nxp.com> wrote:
>
> From: Nipun Gupta <nipun.gupta@nxp.com>
>
> This change adds a Rx offload capability and configuration to
> enable hardware to drop the packets in case of any error in the
> packets such as L3 checksum error or L4 checksum.
>
> Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> Reviewed-by: Asaf Penso <asafp@nvidia.com>
> ---
>
> v4:
>  - renamed 'rte_rx_err_pkt_drop_conf' to
>    'rte_eth_rx_err_pkt_drop_conf'
>  - updated function 'port_offload_cap_display' to display newly
>    added offloads
>  - added placeholder for L1 FCS, L3 Checksum, L4 Checksum error
>    packet drops
>  - updated doc/guides/nics/features.rst
>  - updated new added 'DEV_RX_OFFLOAD_*' to 'RTE_DEV_RX_OFFLOAD*'
>  - updated RX to Rx
>
> v3:
>  - Add additional rx_err_drop_offload_capa, which is specific
>    capability flag for RX packets error drop offload. Currently
>    only 'all' error packet drops are enabled, but can be extended
>    to provide capability to drop any specific errors like L1 FCS,
>    L3 Checksum etc.
>  - Added separate config structure to enable the drop configuration.
>  - Updated doc with the new updated option in testbbdev (patch 3/3)
>
> v2:
>  - Add support in DPAA1 driver (patch 2/3)
>  - Add support and config parameter in testpmd (patch 3/3)
>
>  lib/librte_ethdev/rte_ethdev.c |  1 +
>  lib/librte_ethdev/rte_ethdev.h | 39 +++++++++++++++++++++++++++++++++-
>  2 files changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 48d1333b1..be25e947e 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -128,6 +128,7 @@ static const struct {
>         RTE_RX_OFFLOAD_BIT2STR(SCTP_CKSUM),
>         RTE_RX_OFFLOAD_BIT2STR(OUTER_UDP_CKSUM),
>         RTE_RX_OFFLOAD_BIT2STR(RSS_HASH),
> +       RTE_RX_OFFLOAD_BIT2STR(ERR_PKT_DROP),
>  };
>
>  #undef RTE_RX_OFFLOAD_BIT2STR
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index d2bf74f12..0db4b4021 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1194,6 +1194,28 @@ struct rte_intr_conf {
>         uint32_t rmv:1;
>  };
>
> +/**
> + * A structure used to enable/disable error packet drop on Rx.
> + */
> +struct rte_eth_rx_err_pkt_drop_conf {
> +       /** enable/disable L1 FSC error packet drop on Rx.
s/FSC/FCS
Its L2 FCS. FCS is a part of Ethernet frame.

> +        * 0 (default) - disable, 1 enable
> +        */
> +       uint32_t l1_fcs:1;
l2_fcs;

> +       /** enable/disable L3 Checksum error packet drop on Rx.
> +        * 0 (default) - disable, 1 enable
> +        */
> +       uint32_t l3_csum:1;
> +       /** enable/disable L4 Checksum error packet drop on Rx.
> +        * 0 (default) - disable, 1 enable
> +        */
> +       uint32_t l4_csum:1;
> +       /** enable/disable all Rx error packet drop.
> +        * 0 (default) - disable, 1 enable
> +        */
> +       uint32_t all:1;
> +};
> +
>  /**
>   * A structure used to configure an Ethernet port.
>   * Depending upon the RX multi-queue mode, extra advanced
> @@ -1236,10 +1258,12 @@ struct rte_eth_conf {
>         uint32_t dcb_capability_en;
>         struct rte_fdir_conf fdir_conf; /**< FDIR configuration. DEPRECATED */
>         struct rte_intr_conf intr_conf; /**< Interrupt mode configuration. */
> +       struct rte_eth_rx_err_pkt_drop_conf err_pkt_drop_conf;
> +       /**< Rx error packet drop configuration. */
>  };
>
>  /**
> - * RX offload capabilities of a device.
> + * Rx offload capabilities of a device.
>   */
>  #define DEV_RX_OFFLOAD_VLAN_STRIP  0x00000001
>  #define DEV_RX_OFFLOAD_IPV4_CKSUM  0x00000002
> @@ -1260,6 +1284,7 @@ struct rte_eth_conf {
>  #define DEV_RX_OFFLOAD_SCTP_CKSUM      0x00020000
>  #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM  0x00040000
>  #define DEV_RX_OFFLOAD_RSS_HASH                0x00080000
> +#define DEV_RX_OFFLOAD_ERR_PKT_DROP    0x00100000
>
>  #define DEV_RX_OFFLOAD_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM | \
>                                  DEV_RX_OFFLOAD_UDP_CKSUM | \
> @@ -1274,6 +1299,16 @@ struct rte_eth_conf {
>   * mentioned in rte_rx_offload_names in rte_ethdev.c file.
>   */
>
> +/**
> + * Rx Error Drop offload config/capabilities of a device. These
> + * are valid only when Rx capability RTE_DEV_RX_OFFLOAD_ERR_PKT_DROP
> + * is supported by the device.
> + */
> +#define RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L1_FCS         0x00000001
RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L2_FCS



> +#define RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L3_CSUM                0x00000002
> +#define RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L4_CSUM                0x00000004
> +#define RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL            0x80000000
> +
>  /**
>   * TX offload capabilities of a device.
>   */
> @@ -1411,6 +1446,8 @@ struct rte_eth_dev_info {
>         /**< Device per-queue RX offload capabilities. */
>         uint64_t tx_queue_offload_capa;
>         /**< Device per-queue TX offload capabilities. */
> +       uint64_t rx_err_drop_offload_capa;
> +       /**< RX error packet drop offload capabilities. */
>         uint16_t reta_size;
>         /**< Device redirection table size, the total number of entries. */
>         uint8_t hash_key_size; /**< Hash key size in bytes */
> --
> 2.17.1
>

Re: [dpdk-dev] [PATCH 3/3 v4] app/testpmd: support hardware offload to drop error packets

[Dharmik Thakkar]

Hi Nipun,

Some nits. Looks good otherwise.

> On Oct 15, 2020, at 8:23 AM, Nipun.gupta@nxp.com <nipun.gupta@nxp.com> wrote:
> 
> From: Nipun Gupta <nipun.gupta@nxp.com>
> 
> With DEV_RX_OFFLOAD_ERR_PKT_DROP now defined as an offload
> capability, and separate RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL
> capability to drop all error packets in hardware, testpmd
> showcases this with a new added configuration option
> 'enable-hw-drop-err-all'.
> 
> Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> ---
> app/test-pmd/config.c                 | 35 +++++++++++++++++++++
> app/test-pmd/parameters.c             |  7 +++++
> app/test-pmd/testpmd.c                |  8 +++++
> app/test-pmd/testpmd.h                |  1 +
> doc/guides/nics/features.rst          | 44 +++++++++++++++++++++++++++
> doc/guides/testpmd_app_ug/run_app.rst |  4 +++
> 6 files changed, 99 insertions(+)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index e73dc66c8..14ef4e468 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -1027,6 +1027,41 @@ port_offload_cap_display(portid_t port_id)
> 			printf("off\n");
> 	}
> 
> +	if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_ERR_PKT_DROP) {
> +		if (dev_info.rx_err_drop_offload_capa &
> +			RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L1_FCS) {
> +			printf("RX L1 FCS Error pkt drop:      ");
> +			if (ports[port_id].dev_conf.err_pkt_drop_conf.l1_fcs)
> +				printf("on\n");
> +			else
> +				printf("off\n");
> +		}
> +		if (dev_info.rx_err_drop_offload_capa &
> +			RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L3_CSUM) {
> +			printf("RX L3 Csum Error pkt drop:     ");
> +			if (ports[port_id].dev_conf.err_pkt_drop_conf.l3_csum)
> +				printf("on\n");
> +			else
> +				printf("off\n");
> +		}
> +		if (dev_info.rx_err_drop_offload_capa &
> +			RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L4_CSUM) {
> +			printf("RX L4 Csum Error pkt drop:     ");
> +			if (ports[port_id].dev_conf.err_pkt_drop_conf.l4_csum)
> +				printf("on\n");
> +			else
> +				printf("off\n");
> +		}
> +		if (dev_info.rx_err_drop_offload_capa &
> +			RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL) {
> +			printf("RX all Error pkt drop:         ");
> +			if (ports[port_id].dev_conf.err_pkt_drop_conf.all)
> +				printf("on\n");
> +			else
> +				printf("off\n");
> +		}
> +	}
> +
> 	if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_VLAN_INSERT) {
> 		printf("VLAN insert:                   ");
> 		if (ports[port_id].dev_conf.txmode.offloads &
> diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
> index 1ead59579..508612426 100644
> --- a/app/test-pmd/parameters.c
> +++ b/app/test-pmd/parameters.c
> @@ -142,6 +142,7 @@ usage(char* progname)
> 	printf("  --enable-hw-vlan-strip: enable hardware vlan strip.\n");
> 	printf("  --enable-hw-vlan-extend: enable hardware vlan extend.\n");
> 	printf("  --enable-hw-qinq-strip: enable hardware qinq strip.\n");
> +	printf("  --enable-hw-drop-err-all: enable hardware packet drop for all error packets.\n");
> 	printf("  --enable-drop-en: enable per queue packet drop.\n");
> 	printf("  --disable-rss: disable rss.\n");
> 	printf("  --port-topology=<paired|chained|loop>: set port topology (paired "
> @@ -631,6 +632,7 @@ launch_args_parse(int argc, char** argv)
> 		{ "enable-hw-vlan-strip",       0, 0, 0 },
> 		{ "enable-hw-vlan-extend",      0, 0, 0 },
> 		{ "enable-hw-qinq-strip",       0, 0, 0 },
> +		{ "enable-hw-drop-err-all",     0, 0, 0 },
> 		{ "enable-drop-en",            0, 0, 0 },
> 		{ "disable-rss",                0, 0, 0 },
> 		{ "port-topology",              1, 0, 0 },
> @@ -1283,6 +1285,11 @@ launch_args_parse(int argc, char** argv)
> 				rmv_interrupt = 0;
> 			if (!strcmp(lgopts[opt_idx].name, "flow-isolate-all"))
> 				flow_isolate_all = 1;
> +			if (!strcmp(lgopts[opt_idx].name,
> +					"enable-hw-drop-err-all")) {
> +				rx_err_pkt_drop_all = 1;
> +			}

{ } can be removed.

> +
> 			if (!strcmp(lgopts[opt_idx].name, "tx-offloads")) {
> 				char *end = NULL;
> 				n = strtoull(optarg, &end, 16);
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index ccba71c07..c9e7397e6 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -359,6 +359,11 @@ uint8_t lsc_interrupt = 1; /* enabled by default */
>  */
> uint8_t rmv_interrupt = 1; /* enabled by default */
> 
> +/*
> + * Drop all RX error packets on HW itself.
> + */
> +uint8_t rx_err_pkt_drop_all = 0; /* disabled by default */

= 0 can be removed.

> +
> uint8_t hot_plug = 0; /**< hotplug disabled by default. */
> 
> /* After attach, port setup is called on event or by iterator */
> @@ -3359,6 +3364,9 @@ init_port_config(void)
> 		    (rte_eth_devices[pid].data->dev_flags &
> 		     RTE_ETH_DEV_INTR_RMV))
> 			port->dev_conf.intr_conf.rmv = 1;
> +
> +		if (rx_err_pkt_drop_all)
> +			port->dev_conf.err_pkt_drop_conf.all = 1;
> 	}
> }
> 
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> index c7e7e41a9..eab154ed4 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -314,6 +314,7 @@ extern uint8_t no_device_start; /**<set by "--disable-device-start" parameter */
> extern volatile int test_done; /* stop packet forwarding when set to 1. */
> extern uint8_t lsc_interrupt; /**< disabled by "--no-lsc-interrupt" parameter */
> extern uint8_t rmv_interrupt; /**< disabled by "--no-rmv-interrupt" parameter */
> +extern uint8_t rx_err_pkt_drop_all; /**< enabled by "--enable-hw-drop-err-all" parameter */
> extern uint32_t event_print_mask;
> /**< set by "--print-event xxxx" and "--mask-event xxxx parameters */
> extern bool setup_on_probe_event; /**< disabled by port setup-on iterator */
> diff --git a/doc/guides/nics/features.rst b/doc/guides/nics/features.rst
> index dd8c9555b..cb7fdfd8b 100644
> --- a/doc/guides/nics/features.rst
> +++ b/doc/guides/nics/features.rst
> @@ -606,6 +606,50 @@ Supports inner packet L4 checksum.
>   ``tx_offload_capa,tx_queue_offload_capa:DEV_TX_OFFLOAD_OUTER_UDP_CKSUM``.
> 
> 
> +.. _nic_features_l1_fcs_rx_error_packet_drop:
> +
> +L1 FCS Error Packet drop on Rx
> +------------------------------
> +
> +Supports dropping of packets having L1 FCS error on Rx.
> +
> +* **[uses]     user config**: ``dev_conf.err_pkt_drop_conf.l1_fcs``.
> +* **[provides] rte_eth_dev_info**: ``rx_err_drop_offload_capa:RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L1_FCS``.
> +
> +
> +.. _nic_features_l3_csum_rx_error_packet_drop:
> +
> +L3 checksum Error Packet drop on Rx
> +-----------------------------------
> +
> +Supports dropping of packets having L3 Checksum error on Rx.
> +
> +* **[uses]     user config**: ``dev_conf.err_pkt_drop_conf.l3_csum``.
> +* **[provides] rte_eth_dev_info**: ``rx_err_drop_offload_capa:RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L3_CSUM``.
> +
> +
> +.. _nic_features_l4_csum_rx_error_packet_drop:
> +
> +L4 Checksum Error Packet drop on Rx
> +-----------------------------------
> +
> +Supports dropping of packets having L1 FCS error on Rx.
> +
> +* **[uses]     user config**: ``dev_conf.err_pkt_drop_conf.l4_csum``.
> +* **[provides] rte_eth_dev_info**: ``rx_err_drop_offload_capa:RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L4_CSUM``.
> +
> +
> +.. _nic_features_all_rx_error_packet_drop:
> +
> +All/any Error Packet drop on Rx
> +-------------------------------
> +
> +Supports dropping of packets having any of the errors like L1 FSC, L3/L4 Checksum on Rx.

s/FSC/FCS

> +
> +* **[uses]     user config**: ``dev_conf.err_pkt_drop_conf.all``.
> +* **[provides] rte_eth_dev_info**: ``rx_err_drop_offload_capa:RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL``.
> +
> +
> .. _nic_features_packet_type_parsing:
> 
> Packet type parsing
> diff --git a/doc/guides/testpmd_app_ug/run_app.rst b/doc/guides/testpmd_app_ug/run_app.rst
> index e2539f693..20f2f8083 100644
> --- a/doc/guides/testpmd_app_ug/run_app.rst
> +++ b/doc/guides/testpmd_app_ug/run_app.rst
> @@ -213,6 +213,10 @@ The command line options are:
> 
>     Enable hardware QINQ strip.
> 
> +*  ``--enable-hw-drop-err-all``
> +
> +    Enable hardware packet drop for any error packets
> +
> *   ``--enable-drop-en``
> 
>     Enable per-queue packet drop for packets with no descriptors.
> — 
> 2.17.1
> 

Reviewed-by: Dharmik Thakkar <dharmik.thakkar@arm.com>

Re: [dpdk-dev] [PATCH 3/3 v4] app/testpmd: support hardware offload to drop error packets

[Nipun Gupta]

> -----Original Message-----
> From: Dharmik Thakkar <Dharmik.Thakkar@arm.com>
> Sent: Thursday, October 29, 2020 10:52 PM
> To: Nipun Gupta <nipun.gupta@nxp.com>
> Cc: dev <dev@dpdk.org>; thomas@monjalon.net; Ferruh Yigit
> <ferruh.yigit@intel.com>; arybchenko@solarflare.com; Hemant Agrawal
> <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>; Rohit
> Raj <rohit.raj@nxp.com>; Jerin Jacob <jerinjacobk@gmail.com>; Stephen
> Hemminger <stephen@networkplumber.org>; asafp@nvidia.com; nd
> <nd@arm.com>
> Subject: Re: [dpdk-dev] [PATCH 3/3 v4] app/testpmd: support hardware offload
> to drop error packets
> 
> Hi Nipun,
> 
> Some nits. Looks good otherwise.
> 
> > On Oct 15, 2020, at 8:23 AM, Nipun.gupta@nxp.com <nipun.gupta@nxp.com>
> wrote:
> >
> > From: Nipun Gupta <nipun.gupta@nxp.com>
> >
> > With DEV_RX_OFFLOAD_ERR_PKT_DROP now defined as an offload
> > capability, and separate RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL
> > capability to drop all error packets in hardware, testpmd
> > showcases this with a new added configuration option
> > 'enable-hw-drop-err-all'.
> >
> > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> > ---
> > app/test-pmd/config.c                 | 35 +++++++++++++++++++++
> > app/test-pmd/parameters.c             |  7 +++++
> > app/test-pmd/testpmd.c                |  8 +++++
> > app/test-pmd/testpmd.h                |  1 +
> > doc/guides/nics/features.rst          | 44 +++++++++++++++++++++++++++
> > doc/guides/testpmd_app_ug/run_app.rst |  4 +++
> > 6 files changed, 99 insertions(+)
> >
> > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> > index e73dc66c8..14ef4e468 100644
> > --- a/app/test-pmd/config.c
> > +++ b/app/test-pmd/config.c
> > @@ -1027,6 +1027,41 @@ port_offload_cap_display(portid_t port_id)
> > 			printf("off\n");
> > 	}
> >
> > +	if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_ERR_PKT_DROP) {
> > +		if (dev_info.rx_err_drop_offload_capa &
> > +			RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L1_FCS) {
> > +			printf("RX L1 FCS Error pkt drop:      ");
> > +			if (ports[port_id].dev_conf.err_pkt_drop_conf.l1_fcs)
> > +				printf("on\n");
> > +			else
> > +				printf("off\n");
> > +		}
> > +		if (dev_info.rx_err_drop_offload_capa &
> > +			RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L3_CSUM) {
> > +			printf("RX L3 Csum Error pkt drop:     ");
> > +			if (ports[port_id].dev_conf.err_pkt_drop_conf.l3_csum)
> > +				printf("on\n");
> > +			else
> > +				printf("off\n");
> > +		}
> > +		if (dev_info.rx_err_drop_offload_capa &
> > +			RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L4_CSUM) {
> > +			printf("RX L4 Csum Error pkt drop:     ");
> > +			if (ports[port_id].dev_conf.err_pkt_drop_conf.l4_csum)
> > +				printf("on\n");
> > +			else
> > +				printf("off\n");
> > +		}
> > +		if (dev_info.rx_err_drop_offload_capa &
> > +			RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL) {
> > +			printf("RX all Error pkt drop:         ");
> > +			if (ports[port_id].dev_conf.err_pkt_drop_conf.all)
> > +				printf("on\n");
> > +			else
> > +				printf("off\n");
> > +		}
> > +	}
> > +
> > 	if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_VLAN_INSERT) {
> > 		printf("VLAN insert:                   ");
> > 		if (ports[port_id].dev_conf.txmode.offloads &
> > diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
> > index 1ead59579..508612426 100644
> > --- a/app/test-pmd/parameters.c
> > +++ b/app/test-pmd/parameters.c
> > @@ -142,6 +142,7 @@ usage(char* progname)
> > 	printf("  --enable-hw-vlan-strip: enable hardware vlan strip.\n");
> > 	printf("  --enable-hw-vlan-extend: enable hardware vlan extend.\n");
> > 	printf("  --enable-hw-qinq-strip: enable hardware qinq strip.\n");
> > +	printf("  --enable-hw-drop-err-all: enable hardware packet drop for all
> error packets.\n");
> > 	printf("  --enable-drop-en: enable per queue packet drop.\n");
> > 	printf("  --disable-rss: disable rss.\n");
> > 	printf("  --port-topology=<paired|chained|loop>: set port topology
> (paired "
> > @@ -631,6 +632,7 @@ launch_args_parse(int argc, char** argv)
> > 		{ "enable-hw-vlan-strip",       0, 0, 0 },
> > 		{ "enable-hw-vlan-extend",      0, 0, 0 },
> > 		{ "enable-hw-qinq-strip",       0, 0, 0 },
> > +		{ "enable-hw-drop-err-all",     0, 0, 0 },
> > 		{ "enable-drop-en",            0, 0, 0 },
> > 		{ "disable-rss",                0, 0, 0 },
> > 		{ "port-topology",              1, 0, 0 },
> > @@ -1283,6 +1285,11 @@ launch_args_parse(int argc, char** argv)
> > 				rmv_interrupt = 0;
> > 			if (!strcmp(lgopts[opt_idx].name, "flow-isolate-all"))
> > 				flow_isolate_all = 1;
> > +			if (!strcmp(lgopts[opt_idx].name,
> > +					"enable-hw-drop-err-all")) {
> > +				rx_err_pkt_drop_all = 1;
> > +			}
> 
> { } can be removed.

Agree.

> 
> > +
> > 			if (!strcmp(lgopts[opt_idx].name, "tx-offloads")) {
> > 				char *end = NULL;
> > 				n = strtoull(optarg, &end, 16);
> > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> > index ccba71c07..c9e7397e6 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -359,6 +359,11 @@ uint8_t lsc_interrupt = 1; /* enabled by default */
> >  */
> > uint8_t rmv_interrupt = 1; /* enabled by default */
> >
> > +/*
> > + * Drop all RX error packets on HW itself.
> > + */
> > +uint8_t rx_err_pkt_drop_all = 0; /* disabled by default */
> 
> = 0 can be removed.

This is on all the other default disabled parameters too probably to show that they
are disabled by default. So I would prefer to keep it like this.

> 
> > +
> > uint8_t hot_plug = 0; /**< hotplug disabled by default. */
> >
> > /* After attach, port setup is called on event or by iterator */
> > @@ -3359,6 +3364,9 @@ init_port_config(void)
> > 		    (rte_eth_devices[pid].data->dev_flags &
> > 		     RTE_ETH_DEV_INTR_RMV))
> > 			port->dev_conf.intr_conf.rmv = 1;
> > +
> > +		if (rx_err_pkt_drop_all)
> > +			port->dev_conf.err_pkt_drop_conf.all = 1;
> > 	}
> > }
> >
> > diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> > index c7e7e41a9..eab154ed4 100644
> > --- a/app/test-pmd/testpmd.h
> > +++ b/app/test-pmd/testpmd.h
> > @@ -314,6 +314,7 @@ extern uint8_t no_device_start; /**<set by "--disable-
> device-start" parameter */
> > extern volatile int test_done; /* stop packet forwarding when set to 1. */
> > extern uint8_t lsc_interrupt; /**< disabled by "--no-lsc-interrupt" parameter */
> > extern uint8_t rmv_interrupt; /**< disabled by "--no-rmv-interrupt" parameter
> */
> > +extern uint8_t rx_err_pkt_drop_all; /**< enabled by "--enable-hw-drop-err-
> all" parameter */
> > extern uint32_t event_print_mask;
> > /**< set by "--print-event xxxx" and "--mask-event xxxx parameters */
> > extern bool setup_on_probe_event; /**< disabled by port setup-on iterator */
> > diff --git a/doc/guides/nics/features.rst b/doc/guides/nics/features.rst
> > index dd8c9555b..cb7fdfd8b 100644
> > --- a/doc/guides/nics/features.rst
> > +++ b/doc/guides/nics/features.rst
> > @@ -606,6 +606,50 @@ Supports inner packet L4 checksum.
> >
> ``tx_offload_capa,tx_queue_offload_capa:DEV_TX_OFFLOAD_OUTER_UDP_CK
> SUM``.
> >
> >
> > +.. _nic_features_l1_fcs_rx_error_packet_drop:
> > +
> > +L1 FCS Error Packet drop on Rx
> > +------------------------------
> > +
> > +Supports dropping of packets having L1 FCS error on Rx.
> > +
> > +* **[uses]     user config**: ``dev_conf.err_pkt_drop_conf.l1_fcs``.
> > +* **[provides] rte_eth_dev_info**:
> ``rx_err_drop_offload_capa:RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L1_FCS``.
> > +
> > +
> > +.. _nic_features_l3_csum_rx_error_packet_drop:
> > +
> > +L3 checksum Error Packet drop on Rx
> > +-----------------------------------
> > +
> > +Supports dropping of packets having L3 Checksum error on Rx.
> > +
> > +* **[uses]     user config**: ``dev_conf.err_pkt_drop_conf.l3_csum``.
> > +* **[provides] rte_eth_dev_info**:
> ``rx_err_drop_offload_capa:RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L3_CSUM
> ``.
> > +
> > +
> > +.. _nic_features_l4_csum_rx_error_packet_drop:
> > +
> > +L4 Checksum Error Packet drop on Rx
> > +-----------------------------------
> > +
> > +Supports dropping of packets having L1 FCS error on Rx.
> > +
> > +* **[uses]     user config**: ``dev_conf.err_pkt_drop_conf.l4_csum``.
> > +* **[provides] rte_eth_dev_info**:
> ``rx_err_drop_offload_capa:RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L4_CSUM
> ``.
> > +
> > +
> > +.. _nic_features_all_rx_error_packet_drop:
> > +
> > +All/any Error Packet drop on Rx
> > +-------------------------------
> > +
> > +Supports dropping of packets having any of the errors like L1 FSC, L3/L4
> Checksum on Rx.
> 
> s/FSC/FCS

Will update in the next spin.

Thanks,
Nipun

> 
> > +
> > +* **[uses]     user config**: ``dev_conf.err_pkt_drop_conf.all``.
> > +* **[provides] rte_eth_dev_info**:
> ``rx_err_drop_offload_capa:RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL``.
> > +
> > +
> > .. _nic_features_packet_type_parsing:
> >
> > Packet type parsing
> > diff --git a/doc/guides/testpmd_app_ug/run_app.rst
> b/doc/guides/testpmd_app_ug/run_app.rst
> > index e2539f693..20f2f8083 100644
> > --- a/doc/guides/testpmd_app_ug/run_app.rst
> > +++ b/doc/guides/testpmd_app_ug/run_app.rst
> > @@ -213,6 +213,10 @@ The command line options are:
> >
> >     Enable hardware QINQ strip.
> >
> > +*  ``--enable-hw-drop-err-all``
> > +
> > +    Enable hardware packet drop for any error packets
> > +
> > *   ``--enable-drop-en``
> >
> >     Enable per-queue packet drop for packets with no descriptors.
> > —
> > 2.17.1
> >
> 
> Reviewed-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
> 

Re: [dpdk-dev] [PATCH 1/3 v4] ethdev: add Rx offload to drop error packets

[Ferruh Yigit]

On 10/15/2020 2:23 PM, nipun.gupta@nxp.com wrote:
> From: Nipun Gupta <nipun.gupta@nxp.com>
> 
> This change adds a Rx offload capability and configuration to
> enable hardware to drop the packets in case of any error in the
> packets such as L3 checksum error or L4 checksum.
> 
> Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> Reviewed-by: Asaf Penso <asafp@nvidia.com>
> ---
> 
> v4:
>   - renamed 'rte_rx_err_pkt_drop_conf' to
>     'rte_eth_rx_err_pkt_drop_conf'
>   - updated function 'port_offload_cap_display' to display newly
>     added offloads
>   - added placeholder for L1 FCS, L3 Checksum, L4 Checksum error
>     packet drops
>   - updated doc/guides/nics/features.rst
>   - updated new added 'DEV_RX_OFFLOAD_*' to 'RTE_DEV_RX_OFFLOAD*'
>   - updated RX to Rx
> 
> v3:
>   - Add additional rx_err_drop_offload_capa, which is specific
>     capability flag for RX packets error drop offload. Currently
>     only 'all' error packet drops are enabled, but can be extended
>     to provide capability to drop any specific errors like L1 FCS,
>     L3 Checksum etc.
>   - Added separate config structure to enable the drop configuration.
>   - Updated doc with the new updated option in testbbdev (patch 3/3)
> 
> v2:
>   - Add support in DPAA1 driver (patch 2/3)
>   - Add support and config parameter in testpmd (patch 3/3)
> 
>   lib/librte_ethdev/rte_ethdev.c |  1 +
>   lib/librte_ethdev/rte_ethdev.h | 39 +++++++++++++++++++++++++++++++++-
>   2 files changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c


This feature touches many main parts,
- new config item for 'rte_eth_dev_configure()'
- a new offload flag
- new capability reporting for 'rte_eth_dev_info_get()'

The feature doesn't look very mainstream to touch all these main parts and add 
complexity to them, which will affect almost all users.

And has some inconsistencies, like configuration is done via config struct, but 
capability is returned as bit-wise.
Or I think config option taken into account only if offload is requested has a 
chance to confuse people in both app and driver end.

What do you think having two specific APIs to get_capabilities and set drop config?
The responsibility of those APIs will be clear and narrowed down, which makes it 
harder to make it wrong.


Also it is an ABI break as it is and needs to wait 21.11, and even after that it 
has a potential to cause more ABI breaks, like trying to add a new error type to 
drop will need to wait 22.11 ..., but if we can have them as separate APIs we 
can have them as experimental without waiting next LTS.

Re: [dpdk-dev] [PATCH 1/3 v4] ethdev: add Rx offload to drop error packets

[Thomas Monjalon]

18/02/2021 21:32, Ferruh Yigit:
> On 10/15/2020 2:23 PM, nipun.gupta@nxp.com wrote:
> > From: Nipun Gupta <nipun.gupta@nxp.com>
> > 
> > This change adds a Rx offload capability and configuration to
> > enable hardware to drop the packets in case of any error in the
> > packets such as L3 checksum error or L4 checksum.
> > 
> > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> > Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> > Reviewed-by: Asaf Penso <asafp@nvidia.com>
> > ---
> 
> This feature touches many main parts,
> - new config item for 'rte_eth_dev_configure()'
> - a new offload flag
> - new capability reporting for 'rte_eth_dev_info_get()'
> 
> The feature doesn't look very mainstream to touch all these main parts and add 
> complexity to them, which will affect almost all users.
> 
> And has some inconsistencies, like configuration is done via config struct, but 
> capability is returned as bit-wise.
> Or I think config option taken into account only if offload is requested has a 
> chance to confuse people in both app and driver end.
> 
> What do you think having two specific APIs to get_capabilities and set drop config?
> The responsibility of those APIs will be clear and narrowed down, which makes it 
> harder to make it wrong.

I agree. In general, it is better adding new functions
instead of adding everything in rte_eth_dev_configure().

Re: [dpdk-dev] [PATCH 1/3 v4] ethdev: add Rx offload to drop error packets

[Ferruh Yigit]

On 2/18/2021 8:37 PM, Thomas Monjalon wrote:
> 18/02/2021 21:32, Ferruh Yigit:
>> On 10/15/2020 2:23 PM, nipun.gupta@nxp.com wrote:
>>> From: Nipun Gupta <nipun.gupta@nxp.com>
>>>
>>> This change adds a Rx offload capability and configuration to
>>> enable hardware to drop the packets in case of any error in the
>>> packets such as L3 checksum error or L4 checksum.
>>>
>>> Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
>>> Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
>>> Reviewed-by: Asaf Penso <asafp@nvidia.com>
>>> ---
>>
>> This feature touches many main parts,
>> - new config item for 'rte_eth_dev_configure()'
>> - a new offload flag
>> - new capability reporting for 'rte_eth_dev_info_get()'
>>
>> The feature doesn't look very mainstream to touch all these main parts and add
>> complexity to them, which will affect almost all users.
>>
>> And has some inconsistencies, like configuration is done via config struct, but
>> capability is returned as bit-wise.
>> Or I think config option taken into account only if offload is requested has a
>> chance to confuse people in both app and driver end.
>>
>> What do you think having two specific APIs to get_capabilities and set drop config?
>> The responsibility of those APIs will be clear and narrowed down, which makes it
>> harder to make it wrong.
> 
> I agree. In general, it is better adding new functions
> instead of adding everything in rte_eth_dev_configure().
> 

The set is stale, rejecting it, please send a new version if required.