[dpdk-dev] [PATCH] net/i40e: disable source pruning

[dpdk-dev] [PATCH] net/i40e: disable source pruning

[Alvin Zhang]

VRRP advertisement packets are dropped on i40e PF devices because
when a MAC address is added to a device, packets originating from
that MAC address are dropped. This patch disables source pruning to
work around that issue.

Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 1fc3d89..33550e1 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -5812,6 +5812,10 @@ struct i40e_vsi *
 		ctxt.pf_num = hw->pf_id;
 		ctxt.uplink_seid = vsi->uplink_seid;
 		ctxt.vf_num = 0;
+		ctxt.info.valid_sections |=
+				cpu_to_le16(I40E_AQ_VSI_PROP_SWITCH_VALID);
+		ctxt.info.switch_id |=
+				cpu_to_le16(I40E_AQ_VSI_SW_ID_FLAG_LOCAL_LB);
 
 		/* Update VSI parameters */
 		ret = i40e_aq_update_vsi_params(hw, &ctxt, NULL);
-- 
1.8.3.1

[dpdk-dev] [PATCH v2] net/i40e: disable source pruning

[Alvin Zhang]

VRRP advertisement packets are dropped on i40e PF devices because
when a MAC address is added to a device, packets originating from
that MAC address are dropped.

This patch adds a devarg to support disabling source pruning to work
around above issue.

Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
---
 doc/guides/nics/i40e.rst       |  9 ++++++
 drivers/net/i40e/i40e_ethdev.c | 65 ++++++++++++++++++++++++++++++++++++++++++
 drivers/net/i40e/i40e_ethdev.h |  1 +
 3 files changed, 75 insertions(+)

diff --git a/doc/guides/nics/i40e.rst b/doc/guides/nics/i40e.rst
index ef91b3a..1089a3a 100644
--- a/doc/guides/nics/i40e.rst
+++ b/doc/guides/nics/i40e.rst
@@ -236,6 +236,15 @@ Runtime Config Options
 
   -a 84:00.0,vf_msg_cfg=80@120:180
 
+- ``Disable source pruning on PF`` (default ``not disabled``)
+
+  When a MAC address is added to a device, packets originating from that MAC
+  address will be dropped for source pruning. To disable source pruning on PF
+  we can add ``devargs`` parameter ``disable_source_pruning=[0,1]`` and set its
+  value to 1, for example::
+
+  -a 84:00.0,disable_source_pruning=1
+
 Vector RX Pre-conditions
 ~~~~~~~~~~~~~~~~~~~~~~~~
 For Vector RX it is assumed that the number of descriptor rings will be a power
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 1fc3d89..3722e87 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -47,6 +47,7 @@
 #define ETH_I40E_SUPPORT_MULTI_DRIVER	"support-multi-driver"
 #define ETH_I40E_QUEUE_NUM_PER_VF_ARG	"queue-num-per-vf"
 #define ETH_I40E_VF_MSG_CFG		"vf_msg_cfg"
+#define ETH_I40E_DISABLE_SOURCE_PRUNING_ARG	"disable_source_pruning"
 
 #define I40E_CLEAR_PXE_WAIT_MS     200
 #define I40E_VSI_TSR_QINQ_STRIP		0x4010
@@ -411,6 +412,7 @@ static int i40e_sw_tunnel_filter_insert(struct i40e_pf *pf,
 	ETH_I40E_SUPPORT_MULTI_DRIVER,
 	ETH_I40E_QUEUE_NUM_PER_VF_ARG,
 	ETH_I40E_VF_MSG_CFG,
+	ETH_I40E_DISABLE_SOURCE_PRUNING_ARG,
 	NULL};
 
 static const struct rte_pci_id pci_id_i40e_map[] = {
@@ -1368,6 +1370,23 @@ static inline void i40e_clear_automask(struct i40e_pf *pf)
 }
 
 static int
+i40e_parse_bool(__rte_unused const char *key, const char *value, void *opaque)
+{
+	char *end;
+	unsigned long i;
+
+	errno = 0;
+	i = strtoul(value, &end, 10);
+	if (errno != 0 || end == value || *end != 0 || i > 1) {
+		PMD_DRV_LOG(ERR, "Wrong bool value: %s", value);
+		return -EINVAL;
+	}
+
+	*(bool *)opaque = (bool)i;
+	return 0;
+}
+
+static int
 i40e_parse_vf_msg_config(struct rte_eth_dev *dev,
 		struct i40e_vf_msg_cfg *msg_cfg)
 {
@@ -1404,6 +1423,42 @@ static inline void i40e_clear_automask(struct i40e_pf *pf)
 	return ret;
 }
 
+static int
+i40e_parse_source_pruning_config(struct rte_eth_dev *dev)
+{
+	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
+	struct rte_kvargs *kvlist;
+	int kvargs_count;
+	int ret = -EINVAL;
+
+	if (!dev->device->devargs)
+		return 0;
+
+	kvlist = rte_kvargs_parse(dev->device->devargs->args, valid_keys);
+	if (!kvlist)
+		return 0;
+
+	kvargs_count = rte_kvargs_count(kvlist,
+					ETH_I40E_DISABLE_SOURCE_PRUNING_ARG);
+	if (kvargs_count > 1) {
+		PMD_DRV_LOG(ERR, "More than one argument \"%s\"!",
+			    ETH_I40E_DISABLE_SOURCE_PRUNING_ARG);
+		goto free_end;
+	}
+
+	if (kvargs_count &&
+	    rte_kvargs_process(kvlist, ETH_I40E_DISABLE_SOURCE_PRUNING_ARG,
+			       i40e_parse_bool,
+			       &pf->disable_source_pruning) < 0)
+		goto free_end;
+
+	ret = 0;
+
+free_end:
+	rte_kvargs_free(kvlist);
+	return ret;
+}
+
 #define I40E_ALARM_INTERVAL 50000 /* us */
 
 static int
@@ -1487,6 +1542,10 @@ static inline void i40e_clear_automask(struct i40e_pf *pf)
 	/* Check if need to support multi-driver */
 	i40e_support_multi_driver(dev);
 
+	ret = i40e_parse_source_pruning_config(dev);
+	if (ret)
+		return ret;
+
 	/* Make sure all is clean before doing PF reset */
 	i40e_clear_hw(hw);
 
@@ -5812,6 +5871,12 @@ struct i40e_vsi *
 		ctxt.pf_num = hw->pf_id;
 		ctxt.uplink_seid = vsi->uplink_seid;
 		ctxt.vf_num = 0;
+		if (pf->disable_source_pruning) {
+			ctxt.info.valid_sections |=
+				cpu_to_le16(I40E_AQ_VSI_PROP_SWITCH_VALID);
+			ctxt.info.switch_id |=
+				cpu_to_le16(I40E_AQ_VSI_SW_ID_FLAG_LOCAL_LB);
+		}
 
 		/* Update VSI parameters */
 		ret = i40e_aq_update_vsi_params(hw, &ctxt, NULL);
diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
index 8a68b36..b441153 100644
--- a/drivers/net/i40e/i40e_ethdev.h
+++ b/drivers/net/i40e/i40e_ethdev.h
@@ -1171,6 +1171,7 @@ struct i40e_pf {
 	bool dport_replace_flag;   /* Destination port replace is done */
 	struct i40e_tm_conf tm_conf;
 	bool support_multi_driver; /* 1 - support multiple driver */
+	bool disable_source_pruning; /* 1 - disable source pruning */
 
 	/* Dynamic Device Personalization */
 	bool gtp_support; /* 1 - support GTP-C and GTP-U */
-- 
1.8.3.1

[dpdk-dev] [PATCH v3] net/i40e: disable source pruning

[Alvin Zhang]

VRRP advertisement packets are dropped on i40e PF devices because
when a MAC address is added to a device, packets originating from
that MAC address are dropped.

This patch adds a devarg to support disabling source pruning to work
around above issue.

Bugzilla ID: 648

Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
---

v3: Rebase to 69a3c6319140. Add bugzilla ID in commit log.
---
 doc/guides/nics/i40e.rst       |  9 ++++++
 drivers/net/i40e/i40e_ethdev.c | 65 ++++++++++++++++++++++++++++++++++++++++++
 drivers/net/i40e/i40e_ethdev.h |  1 +
 3 files changed, 75 insertions(+)

diff --git a/doc/guides/nics/i40e.rst b/doc/guides/nics/i40e.rst
index ef91b3a..1089a3a 100644
--- a/doc/guides/nics/i40e.rst
+++ b/doc/guides/nics/i40e.rst
@@ -236,6 +236,15 @@ Runtime Config Options
 
   -a 84:00.0,vf_msg_cfg=80@120:180
 
+- ``Disable source pruning on PF`` (default ``not disabled``)
+
+  When a MAC address is added to a device, packets originating from that MAC
+  address will be dropped for source pruning. To disable source pruning on PF
+  we can add ``devargs`` parameter ``disable_source_pruning=[0,1]`` and set its
+  value to 1, for example::
+
+  -a 84:00.0,disable_source_pruning=1
+
 Vector RX Pre-conditions
 ~~~~~~~~~~~~~~~~~~~~~~~~
 For Vector RX it is assumed that the number of descriptor rings will be a power
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 1fc3d89..3722e87 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -47,6 +47,7 @@
 #define ETH_I40E_SUPPORT_MULTI_DRIVER	"support-multi-driver"
 #define ETH_I40E_QUEUE_NUM_PER_VF_ARG	"queue-num-per-vf"
 #define ETH_I40E_VF_MSG_CFG		"vf_msg_cfg"
+#define ETH_I40E_DISABLE_SOURCE_PRUNING_ARG	"disable_source_pruning"
 
 #define I40E_CLEAR_PXE_WAIT_MS     200
 #define I40E_VSI_TSR_QINQ_STRIP		0x4010
@@ -411,6 +412,7 @@ static int i40e_sw_tunnel_filter_insert(struct i40e_pf *pf,
 	ETH_I40E_SUPPORT_MULTI_DRIVER,
 	ETH_I40E_QUEUE_NUM_PER_VF_ARG,
 	ETH_I40E_VF_MSG_CFG,
+	ETH_I40E_DISABLE_SOURCE_PRUNING_ARG,
 	NULL};
 
 static const struct rte_pci_id pci_id_i40e_map[] = {
@@ -1368,6 +1370,23 @@ static inline void i40e_clear_automask(struct i40e_pf *pf)
 }
 
 static int
+i40e_parse_bool(__rte_unused const char *key, const char *value, void *opaque)
+{
+	char *end;
+	unsigned long i;
+
+	errno = 0;
+	i = strtoul(value, &end, 10);
+	if (errno != 0 || end == value || *end != 0 || i > 1) {
+		PMD_DRV_LOG(ERR, "Wrong bool value: %s", value);
+		return -EINVAL;
+	}
+
+	*(bool *)opaque = (bool)i;
+	return 0;
+}
+
+static int
 i40e_parse_vf_msg_config(struct rte_eth_dev *dev,
 		struct i40e_vf_msg_cfg *msg_cfg)
 {
@@ -1404,6 +1423,42 @@ static inline void i40e_clear_automask(struct i40e_pf *pf)
 	return ret;
 }
 
+static int
+i40e_parse_source_pruning_config(struct rte_eth_dev *dev)
+{
+	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
+	struct rte_kvargs *kvlist;
+	int kvargs_count;
+	int ret = -EINVAL;
+
+	if (!dev->device->devargs)
+		return 0;
+
+	kvlist = rte_kvargs_parse(dev->device->devargs->args, valid_keys);
+	if (!kvlist)
+		return 0;
+
+	kvargs_count = rte_kvargs_count(kvlist,
+					ETH_I40E_DISABLE_SOURCE_PRUNING_ARG);
+	if (kvargs_count > 1) {
+		PMD_DRV_LOG(ERR, "More than one argument \"%s\"!",
+			    ETH_I40E_DISABLE_SOURCE_PRUNING_ARG);
+		goto free_end;
+	}
+
+	if (kvargs_count &&
+	    rte_kvargs_process(kvlist, ETH_I40E_DISABLE_SOURCE_PRUNING_ARG,
+			       i40e_parse_bool,
+			       &pf->disable_source_pruning) < 0)
+		goto free_end;
+
+	ret = 0;
+
+free_end:
+	rte_kvargs_free(kvlist);
+	return ret;
+}
+
 #define I40E_ALARM_INTERVAL 50000 /* us */
 
 static int
@@ -1487,6 +1542,10 @@ static inline void i40e_clear_automask(struct i40e_pf *pf)
 	/* Check if need to support multi-driver */
 	i40e_support_multi_driver(dev);
 
+	ret = i40e_parse_source_pruning_config(dev);
+	if (ret)
+		return ret;
+
 	/* Make sure all is clean before doing PF reset */
 	i40e_clear_hw(hw);
 
@@ -5812,6 +5871,12 @@ struct i40e_vsi *
 		ctxt.pf_num = hw->pf_id;
 		ctxt.uplink_seid = vsi->uplink_seid;
 		ctxt.vf_num = 0;
+		if (pf->disable_source_pruning) {
+			ctxt.info.valid_sections |=
+				cpu_to_le16(I40E_AQ_VSI_PROP_SWITCH_VALID);
+			ctxt.info.switch_id |=
+				cpu_to_le16(I40E_AQ_VSI_SW_ID_FLAG_LOCAL_LB);
+		}
 
 		/* Update VSI parameters */
 		ret = i40e_aq_update_vsi_params(hw, &ctxt, NULL);
diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
index 8a68b36..b441153 100644
--- a/drivers/net/i40e/i40e_ethdev.h
+++ b/drivers/net/i40e/i40e_ethdev.h
@@ -1171,6 +1171,7 @@ struct i40e_pf {
 	bool dport_replace_flag;   /* Destination port replace is done */
 	struct i40e_tm_conf tm_conf;
 	bool support_multi_driver; /* 1 - support multiple driver */
+	bool disable_source_pruning; /* 1 - disable source pruning */
 
 	/* Dynamic Device Personalization */
 	bool gtp_support; /* 1 - support GTP-C and GTP-U */
-- 
1.8.3.1

RE: [dpdk-dev] [PATCH v3] net/i40e: disable source pruning

[Jiang, YuX]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Alvin Zhang
> Sent: Wednesday, October 20, 2021 9:29 AM
> To: Xing, Beilei <beilei.xing@intel.com>; Guo, Junfeng
> <junfeng.guo@intel.com>
> Cc: dev@dpdk.org; Zhang, AlvinX <alvinx.zhang@intel.com>
> Subject: [dpdk-dev] [PATCH v3] net/i40e: disable source pruning
> 
> VRRP advertisement packets are dropped on i40e PF devices because when
> a MAC address is added to a device, packets originating from that MAC
> address are dropped.
> 
> This patch adds a devarg to support disabling source pruning to work around
> above issue.
> 
> Bugzilla ID: 648
> 
> Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
> ---
Tested-by:  Yu Jiang <YuX.Jiang@intel.com>

Verified patchset http://patches.dpdk.org/project/dpdk/patch/20211020012831.8480-1-alvinx.zhang@intel.com/ on baseline dpdk22.03-rc1:ecc0dd455e "raw/cnxk_gpio: add option to select subset of GPIOs"
Tested pass on Ethernet Controller XL710 for 40GbE QSFP+ 1583, OS: Fedora Linux 35/5.14.10-300.fc35.x86_64
Test step as below:
 ./dpdk-testpmd -l 1,2 -n 1 -a 18:00.0,disable_source_pruning=1 -- -i
 pkt = Ether(src="00:00:5E:00:01:0A")/IP()/Raw("x"*60)
 test steps:
1). testpmd>set verbose 1
    testpmd>start
2). Send the pkt, the pkt can be received by testpmd
3). testpmd>mac_addr add 0 00:00:5E:00:01:0A 
4). Re-send the pkt, the pkt still can be received by testpmd.

RE: [dpdk-dev] [PATCH v3] net/i40e: disable source pruning

[Morten Brørup]

> From: Jiang, YuX [mailto:yux.jiang@intel.com]
> Sent: Monday, 21 February 2022 09.31
> 
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Alvin Zhang
> > Sent: Wednesday, October 20, 2021 9:29 AM
> >
> > VRRP advertisement packets are dropped on i40e PF devices because
> when
> > a MAC address is added to a device, packets originating from that MAC
> > address are dropped.
> >
> > This patch adds a devarg to support disabling source pruning to work
> around
> > above issue.
> >
> > Bugzilla ID: 648
> >
> > Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
> > ---
> Tested-by:  Yu Jiang <YuX.Jiang@intel.com>
> 
> Verified patchset
> http://patches.dpdk.org/project/dpdk/patch/20211020012831.8480-1-
> alvinx.zhang@intel.com/ on baseline dpdk22.03-rc1:ecc0dd455e
> "raw/cnxk_gpio: add option to select subset of GPIOs"
> Tested pass on Ethernet Controller XL710 for 40GbE QSFP+ 1583, OS:
> Fedora Linux 35/5.14.10-300.fc35.x86_64
> Test step as below:
>  ./dpdk-testpmd -l 1,2 -n 1 -a 18:00.0,disable_source_pruning=1 -- -i
>  pkt = Ether(src="00:00:5E:00:01:0A")/IP()/Raw("x"*60)
>  test steps:
> 1). testpmd>set verbose 1
>     testpmd>start
> 2). Send the pkt, the pkt can be received by testpmd
> 3). testpmd>mac_addr add 0 00:00:5E:00:01:0A
> 4). Re-send the pkt, the pkt still can be received by testpmd.

If source pruning is not the default behavior of all NICs, it should be disabled by default in the i40e NIC too.

A NIC shouldn't drop any packets unless it has explicitly been configured for it! And a NIC shouldn't treat any packets differently than other NICs do, unless the NIC has explicitly been configured so!

Furthermore, I would prefer that configurations for explicitly dropping certain types of packets is available through runtime APIs, e.g. RTE_FLOWS, or dedicated functions like rte_eth_promiscuous_enable/disable(). This patch doesn't support runtime detection of installed NICs performed by the application.

I am very surprised by this default behavior of a NIC. Please confirm that Source Pruning is at least disabled in Promiscuous mode?

-Morten

RE: [dpdk-dev] [PATCH v3] net/i40e: disable source pruning

[Zhang, Ke1X]

> -----Original Message-----
> From: Morten Brørup <mb@smartsharesystems.com>
> Sent: Monday, February 21, 2022 5:35 PM
> To: Jiang, YuX <yux.jiang@intel.com>; Alvin Zhang <alvinx.zhang@intel.com>;
> Xing, Beilei <beilei.xing@intel.com>; Guo, Junfeng <junfeng.guo@intel.com>
> Cc: dev@dpdk.org; Zhang, AlvinX <alvinx.zhang@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v3] net/i40e: disable source pruning
> 
> > From: Jiang, YuX [mailto:yux.jiang@intel.com]
> > Sent: Monday, 21 February 2022 09.31
> >
> > > From: dev <dev-bounces@dpdk.org> On Behalf Of Alvin Zhang
> > > Sent: Wednesday, October 20, 2021 9:29 AM
> > >
> > > VRRP advertisement packets are dropped on i40e PF devices because
> > when
> > > a MAC address is added to a device, packets originating from that
> > > MAC address are dropped.
> > >
> > > This patch adds a devarg to support disabling source pruning to work
> > around
> > > above issue.
> > >
> > > Bugzilla ID: 648
> > >
> > > Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
> > > ---
> > Tested-by:  Yu Jiang <YuX.Jiang@intel.com>
> >
> > Verified patchset
> > http://patches.dpdk.org/project/dpdk/patch/20211020012831.8480-1-
> > alvinx.zhang@intel.com/ on baseline dpdk22.03-rc1:ecc0dd455e
> > "raw/cnxk_gpio: add option to select subset of GPIOs"
> > Tested pass on Ethernet Controller XL710 for 40GbE QSFP+ 1583, OS:
> > Fedora Linux 35/5.14.10-300.fc35.x86_64 Test step as below:
> >  ./dpdk-testpmd -l 1,2 -n 1 -a 18:00.0,disable_source_pruning=1 -- -i
> > pkt = Ether(src="00:00:5E:00:01:0A")/IP()/Raw("x"*60)
> >  test steps:
> > 1). testpmd>set verbose 1
> >     testpmd>start
> > 2). Send the pkt, the pkt can be received by testpmd 3).
> > testpmd>mac_addr add 0 00:00:5E:00:01:0A 4). Re-send the pkt, the pkt
> > still can be received by testpmd.
> 
> If source pruning is not the default behavior of all NICs, it should be disabled
> by default in the i40e NIC too.
> 
> A NIC shouldn't drop any packets unless it has explicitly been configured for it!
> And a NIC shouldn't treat any packets differently than other NICs do, unless
> the NIC has explicitly been configured so!
> 
> Furthermore, I would prefer that configurations for explicitly dropping
> certain types of packets is available through runtime APIs, e.g. RTE_FLOWS,
> or dedicated functions like rte_eth_promiscuous_enable/disable(). This
> patch doesn't support runtime detection of installed NICs performed by the
> application.
> 
> I am very surprised by this default behavior of a NIC. Please confirm that
> Source Pruning is at least disabled in Promiscuous mode?
> 
> -Morten

Thanks for your comments @Morten Brørup
After testing with other NIC like ice, source pruning is the default action, it means it is the same action
for both ice and i40e when receiving in default.
In this patch, the default is "not disable", it is same with other NICs.

 

RE: [dpdk-dev] [PATCH v3] net/i40e: disable source pruning

[Morten Brørup]

+CC Ethernet API maintainers, you might have an opinion about expected default behavior

> From: Zhang, Ke1X [mailto:ke1x.zhang@intel.com]
> Sent: Monday, 26 December 2022 10.04
> 
> > From: Morten Brørup <mb@smartsharesystems.com>
> > Sent: Monday, February 21, 2022 5:35 PM
> >
> > > From: Jiang, YuX [mailto:yux.jiang@intel.com]
> > > Sent: Monday, 21 February 2022 09.31
> > >
> > > > From: dev <dev-bounces@dpdk.org> On Behalf Of Alvin Zhang
> > > > Sent: Wednesday, October 20, 2021 9:29 AM
> > > >
> > > > VRRP advertisement packets are dropped on i40e PF devices because
> > > when
> > > > a MAC address is added to a device, packets originating from that
> > > > MAC address are dropped.
> > > >
> > > > This patch adds a devarg to support disabling source pruning to
> work
> > > around
> > > > above issue.
> > > >
> > > > Bugzilla ID: 648
> > > >
> > > > Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
> > > > ---
> > > Tested-by:  Yu Jiang <YuX.Jiang@intel.com>
> > >
> > > Verified patchset
> > > http://patches.dpdk.org/project/dpdk/patch/20211020012831.8480-1-
> > > alvinx.zhang@intel.com/ on baseline dpdk22.03-rc1:ecc0dd455e
> > > "raw/cnxk_gpio: add option to select subset of GPIOs"
> > > Tested pass on Ethernet Controller XL710 for 40GbE QSFP+ 1583, OS:
> > > Fedora Linux 35/5.14.10-300.fc35.x86_64 Test step as below:
> > >  ./dpdk-testpmd -l 1,2 -n 1 -a 18:00.0,disable_source_pruning=1 --
> -i
> > > pkt = Ether(src="00:00:5E:00:01:0A")/IP()/Raw("x"*60)
> > >  test steps:
> > > 1). testpmd>set verbose 1
> > >     testpmd>start
> > > 2). Send the pkt, the pkt can be received by testpmd 3).
> > > testpmd>mac_addr add 0 00:00:5E:00:01:0A 4). Re-send the pkt, the
> pkt
> > > still can be received by testpmd.
> >
> > If source pruning is not the default behavior of all NICs, it should
> be disabled
> > by default in the i40e NIC too.
> >
> > A NIC shouldn't drop any packets unless it has explicitly been
> configured for it!
> > And a NIC shouldn't treat any packets differently than other NICs do,
> unless
> > the NIC has explicitly been configured so!
> >
> > Furthermore, I would prefer that configurations for explicitly
> dropping
> > certain types of packets is available through runtime APIs, e.g.
> RTE_FLOWS,
> > or dedicated functions like rte_eth_promiscuous_enable/disable().
> This
> > patch doesn't support runtime detection of installed NICs performed
> by the
> > application.
> >
> > I am very surprised by this default behavior of a NIC. Please confirm
> that
> > Source Pruning is at least disabled in Promiscuous mode?
> >
> > -Morten
> 
> Thanks for your comments @Morten Brørup
> After testing with other NIC like ice, source pruning is the default
> action, it means it is the same action
> for both ice and i40e when receiving in default.

I meant NICs in general, not just Intel NICs. Is it standard behavior for NICs in general (incl. NVIDIA, Broadcom, etc.) to perform source pruning?

They way I read the bug report, the source pruning behavior is specific to Intel NICs. Applications should not be required to pass specific command line parameters to make one vendor's PMDs behave like the general behavior of PMDs from other vendors.

> In this patch, the default is "not disable", it is same with other
> NICs.

Not other NICs in general, only other Intel NICs.

I acknowledge that this patch is a workaround, and certainly an improvement, but why not provide a fix instead? It could be backported to older DPDK releases.

If you want to stick with this workaround instead of providing a proper fix, please also update the documentation to mention that disable_source_pruning=1 is required.

Please also describe how to disable source pruning at runtime. Some applications detect the installed NICs at runtime, so passing NIC specific parameters to the application at startup is not possible.

-Morten

RE: [dpdk-dev] [PATCH v3] net/i40e: disable source pruning

[Morten Brørup]

> From: Zhang, Ke1X [mailto:ke1x.zhang@intel.com]
> Sent: Monday, 26 December 2022 10.04
> 
> > From: Morten Brørup <mb@smartsharesystems.com>
> > Sent: Monday, February 21, 2022 5:35 PM
> >
> > > From: Jiang, YuX [mailto:yux.jiang@intel.com]
> > > Sent: Monday, 21 February 2022 09.31
> > >
> > > > From: dev <dev-bounces@dpdk.org> On Behalf Of Alvin Zhang
> > > > Sent: Wednesday, October 20, 2021 9:29 AM
> > > >
> > > > VRRP advertisement packets are dropped on i40e PF devices because
> > > when
> > > > a MAC address is added to a device, packets originating from that
> > > > MAC address are dropped.
> > > >
> > > > This patch adds a devarg to support disabling source pruning to
> work
> > > around
> > > > above issue.
> > > >
> > > > Bugzilla ID: 648
> > > >
> > > > Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
> > > > ---
> > > Tested-by:  Yu Jiang <YuX.Jiang@intel.com>
> > >
> > > Verified patchset
> > > http://patches.dpdk.org/project/dpdk/patch/20211020012831.8480-1-
> > > alvinx.zhang@intel.com/ on baseline dpdk22.03-rc1:ecc0dd455e
> > > "raw/cnxk_gpio: add option to select subset of GPIOs"
> > > Tested pass on Ethernet Controller XL710 for 40GbE QSFP+ 1583, OS:
> > > Fedora Linux 35/5.14.10-300.fc35.x86_64 Test step as below:
> > >  ./dpdk-testpmd -l 1,2 -n 1 -a 18:00.0,disable_source_pruning=1 --
> -i
> > > pkt = Ether(src="00:00:5E:00:01:0A")/IP()/Raw("x"*60)
> > >  test steps:
> > > 1). testpmd>set verbose 1
> > >     testpmd>start
> > > 2). Send the pkt, the pkt can be received by testpmd 3).
> > > testpmd>mac_addr add 0 00:00:5E:00:01:0A 4). Re-send the pkt, the
> pkt
> > > still can be received by testpmd.
> >
> > If source pruning is not the default behavior of all NICs, it should
> be disabled
> > by default in the i40e NIC too.
> >
> > A NIC shouldn't drop any packets unless it has explicitly been
> configured for it!
> > And a NIC shouldn't treat any packets differently than other NICs do,
> unless
> > the NIC has explicitly been configured so!
> >
> > Furthermore, I would prefer that configurations for explicitly
> dropping
> > certain types of packets is available through runtime APIs, e.g.
> RTE_FLOWS,
> > or dedicated functions like rte_eth_promiscuous_enable/disable().
> This
> > patch doesn't support runtime detection of installed NICs performed
> by the
> > application.
> >
> > I am very surprised by this default behavior of a NIC. Please confirm
> that
> > Source Pruning is at least disabled in Promiscuous mode?
> >
> > -Morten
> 
> Thanks for your comments @Morten Brørup
> After testing with other NIC like ice, source pruning is the default
> action, it means it is the same action
> for both ice and i40e when receiving in default.

I meant NICs in general, not just Intel NICs. Is it standard behavior for NICs in general (incl. NVIDIA, Broadcom, etc.) to perform source pruning?

They way I read the bug report, the source pruning behavior is specific to Intel NICs. Applications should not be required to pass specific command line parameters to make one vendor's PMDs behave like the general behavior of PMDs from other vendors.

> In this patch, the default is "not disable", it is same with other
> NICs.

Not other NICs in general, only other Intel NICs.

I acknowledge that this patch is a workaround, and certainly an improvement, but why not provide a fix instead? It could be backported to older DPDK releases.

If you want to stick with this workaround instead of providing a proper fix, please also update the documentation to mention that disable_source_pruning=1 is required.

Please also describe how to disable source pruning at runtime. Some applications detect the installed NICs at runtime, so passing NIC specific parameters to the application at startup is not possible.

-Morten

[PATCH] net/i40e: disable source pruning

[Ke Zhang]

VRRP advertisement packets are dropped on i40e PF devices because
when a MAC address is added to a device, packets originating from
that MAC address are dropped.

This patch adds a interface in lib/ethdev to support disabling
source pruning to work around above issue.

Bugzilla ID: 648

Signed-off-by: Ke Zhang <ke1x.zhang@intel.com>
---
 app/test-pmd/cmdline.c         | 77 ++++++++++++++++++++++++++++++++++
 drivers/net/i40e/i40e_ethdev.c | 53 +++++++++++++++++++++++
 lib/ethdev/ethdev_driver.h     |  6 +++
 lib/ethdev/rte_ethdev.c        | 16 +++++++
 lib/ethdev/rte_ethdev.h        | 15 +++++++
 5 files changed, 167 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index cb8c174020..a9602a2ed0 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -482,6 +482,9 @@ static void cmd_help_long_parsed(void *parsed_result,
 			"set promisc (port_id|all) (on|off)\n"
 			"    Set the promiscuous mode on port_id, or all.\n\n"
 
+			"set llb (port_id|all) (on|off)\n"
+			"    Set vsi local loopback on port_id, or all.\n\n"
+
 			"set allmulti (port_id|all) (on|off)\n"
 			"    Set the allmulti mode on port_id, or all.\n\n"
 
@@ -5775,6 +5778,78 @@ static cmdline_parse_inst_t cmd_set_promisc_mode_one = {
 	},
 };
 
+/* *** SET VSI LOCAL LOOPBACK *** */
+struct cmd_set_vsi_local_lb_result {
+	cmdline_fixed_string_t set;
+	cmdline_fixed_string_t llb;
+	cmdline_fixed_string_t port_all; /* valid if "allports" argument == 1 */
+	uint16_t port_num;               /* valid if "allports" argument == 0 */
+	cmdline_fixed_string_t enable;
+};
+
+static void cmd_set_vsi_llb_parsed(void *parsed_result,
+					__rte_unused struct cmdline *cl,
+					void *allports)
+{
+	struct cmd_set_vsi_local_lb_result *res = parsed_result;
+	int enable;
+	portid_t i;
+
+	if (!strcmp(res->enable, "on"))
+		enable = 1;
+	else
+		enable = 0;
+
+	/* all ports */
+	if (allports) {
+		RTE_ETH_FOREACH_DEV(i)
+			rte_eth_dev_enable_local_lb(i, enable);
+	} else {
+		rte_eth_dev_enable_local_lb(res->port_num, enable);
+	}
+}
+
+static cmdline_parse_token_string_t cmd_setllb_set =
+	TOKEN_STRING_INITIALIZER(struct cmd_set_vsi_local_lb_result, set, "set");
+static cmdline_parse_token_string_t cmd_setllb_llb =
+	TOKEN_STRING_INITIALIZER(struct cmd_set_vsi_local_lb_result, llb,
+				 "llb");
+static cmdline_parse_token_string_t cmd_setllb_portall =
+	TOKEN_STRING_INITIALIZER(struct cmd_set_vsi_local_lb_result, port_all,
+				 "all");
+static cmdline_parse_token_num_t cmd_setllb_portnum =
+	TOKEN_NUM_INITIALIZER(struct cmd_set_vsi_local_lb_result, port_num,
+			      RTE_UINT16);
+static cmdline_parse_token_string_t cmd_setllb_enalbe =
+	TOKEN_STRING_INITIALIZER(struct cmd_set_vsi_local_lb_result, enable,
+				 "on#off");
+
+static cmdline_parse_inst_t cmd_set_vsi_enable_all = {
+	.f = cmd_set_vsi_llb_parsed,
+	.data = (void *)1,
+	.help_str = "set llb all on|off: Set vsi local loopback for all ports",
+	.tokens = {
+		(void *)&cmd_setllb_set,
+		(void *)&cmd_setllb_llb,
+		(void *)&cmd_setllb_portall,
+		(void *)&cmd_setllb_enalbe,
+		NULL,
+	},
+};
+
+static cmdline_parse_inst_t cmd_set_vsi_enable_one = {
+	.f = cmd_set_vsi_llb_parsed,
+	.data = (void *)0,
+	.help_str = "set llb <port_id> on|off: Set vsi local loopback on port id",
+	.tokens = {
+		(void *)&cmd_setllb_set,
+		(void *)&cmd_setllb_llb,
+		(void *)&cmd_setllb_portnum,
+		(void *)&cmd_setllb_enalbe,
+		NULL,
+	},
+};
+
 /* *** SET ALLMULTI MODE *** */
 struct cmd_set_allmulti_mode_result {
 	cmdline_fixed_string_t set;
@@ -12866,6 +12941,8 @@ static cmdline_parse_ctx_t builtin_ctx[] = {
 	(cmdline_parse_inst_t *)&cmd_show_port_cman_capa,
 	(cmdline_parse_inst_t *)&cmd_show_port_cman_config,
 	(cmdline_parse_inst_t *)&cmd_set_port_cman_config,
+	(cmdline_parse_inst_t *)&cmd_set_vsi_enable_all,
+	(cmdline_parse_inst_t *)&cmd_set_vsi_enable_one,
 	NULL,
 };
 
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 7726a89d99..dd07c38f0f 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -406,6 +406,7 @@ static void i40e_ethertype_filter_restore(struct i40e_pf *pf);
 static void i40e_tunnel_filter_restore(struct i40e_pf *pf);
 static void i40e_filter_restore(struct i40e_pf *pf);
 static void i40e_notify_all_vfs_link_status(struct rte_eth_dev *dev);
+static int i40e_enable_pf_local_lb(struct rte_eth_dev *dev, int on);
 
 static const char *const valid_keys[] = {
 	ETH_I40E_FLOATING_VEB_ARG,
@@ -517,6 +518,7 @@ static const struct eth_dev_ops i40e_eth_dev_ops = {
 	.tm_ops_get                   = i40e_tm_ops_get,
 	.tx_done_cleanup              = i40e_tx_done_cleanup,
 	.get_monitor_addr             = i40e_get_monitor_addr,
+	.enable_local_lb              = i40e_enable_pf_local_lb,
 };
 
 /* store statistics names and its offset in stats structure */
@@ -5634,6 +5636,57 @@ i40e_enable_pf_lb(struct i40e_pf *pf)
 			    hw->aq.asq_last_status);
 }
 
+/* i40e_enable_pf_local_lb
+ * @pf: pointer to the pf structure
+ *
+ * allow local loopback on pf
+ */
+static int
+i40e_enable_pf_local_lb(struct rte_eth_dev *dev, int on)
+{
+	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
+	struct i40e_hw *hw = I40E_PF_TO_HW(pf);
+	struct i40e_vsi_context ctxt;
+	int ret;
+
+	/* Use the FW API if FW >= v5.0 */
+	if (hw->aq.fw_maj_ver < 5 && hw->mac.type != I40E_MAC_X722) {
+#ifdef TREX_PATCH
+		/* Most of our customers do not have latest FW */
+		PMD_INIT_LOG(INFO, "FW < v5.0, cannot enable local loopback");
+#else
+		PMD_INIT_LOG(ERR, "FW < v5.0, cannot enable local loopback");
+#endif
+		return I40E_NOT_SUPPORTED;
+	}
+
+	memset(&ctxt, 0, sizeof(ctxt));
+	ctxt.seid = pf->main_vsi_seid;
+	ctxt.pf_num = hw->pf_id;
+	ret = i40e_aq_get_vsi_params(hw, &ctxt, NULL);
+	if (ret) {
+		PMD_DRV_LOG(ERR, "cannot get pf vsi config, err %d, aq_err %d",
+			    ret, hw->aq.asq_last_status);
+		return ret;
+	}
+	ctxt.flags = I40E_AQ_VSI_TYPE_PF;
+	ctxt.info.valid_sections =
+		rte_cpu_to_le_16(I40E_AQ_VSI_PROP_SWITCH_VALID);
+	if (on)
+		ctxt.info.switch_id |=
+			rte_cpu_to_le_16(I40E_AQ_VSI_SW_ID_FLAG_LOCAL_LB);
+	else
+		ctxt.info.switch_id &=
+			~rte_cpu_to_le_16(I40E_AQ_VSI_SW_ID_FLAG_LOCAL_LB);
+
+	ret = i40e_aq_update_vsi_params(hw, &ctxt, NULL);
+	if (ret)
+		PMD_DRV_LOG(ERR, "update vsi switch failed, aq_err=%d",
+			    hw->aq.asq_last_status);
+
+	return ret;
+}
+
 /* Setup a VSI */
 struct i40e_vsi *
 i40e_vsi_setup(struct i40e_pf *pf,
diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index 6a550cfc83..3a11a7fab4 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -668,6 +668,10 @@ typedef int (*eth_get_module_info_t)(struct rte_eth_dev *dev,
 typedef int (*eth_get_module_eeprom_t)(struct rte_eth_dev *dev,
 				       struct rte_dev_eeprom_info *info);
 
+/** @internal Function used to enable/disable the vsi loop back of an Ethernet device. */
+typedef int (*eth_enable_local_lb_t)(struct rte_eth_dev *dev,
+				  int on);
+
 struct rte_flow_ops;
 /**
  * @internal
@@ -1403,6 +1407,8 @@ struct eth_dev_ops {
 	eth_cman_config_set_t cman_config_set;
 	/** Retrieve congestion management configuration */
 	eth_cman_config_get_t cman_config_get;
+	/** Enable/disable the vsi loop back of an Ethernet device */
+	eth_enable_local_lb_t enable_local_lb;
 };
 
 /**
diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 5d5e18db1e..3991688e2b 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -6318,6 +6318,22 @@ rte_eth_buffer_split_get_supported_hdr_ptypes(uint16_t port_id, uint32_t *ptypes
 	return j;
 }
 
+int
+rte_eth_dev_enable_local_lb(uint16_t port_id, int on)
+{
+	struct rte_eth_dev *dev;
+	int ret;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+	dev = &rte_eth_devices[port_id];
+
+	if (*dev->dev_ops->enable_local_lb == NULL)
+		return -ENOTSUP;
+
+	ret = (*dev->dev_ops->enable_local_lb)(dev, on);
+	return eth_err(port_id, ret);
+}
+
 RTE_LOG_REGISTER_DEFAULT(rte_eth_dev_logtype, INFO);
 
 RTE_INIT(ethdev_init_telemetry)
diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index c129ca1eaf..c4888ebd62 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -3437,6 +3437,21 @@ int rte_eth_dev_set_mtu(uint16_t port_id, uint16_t mtu);
  */
 int rte_eth_dev_vlan_filter(uint16_t port_id, uint16_t vlan_id, int on);
 
+/**
+ * Enable/Disable local Loopback for VSIs that are used as uplink of a software
+ * (cascaded) VEB, VEPA or Port Virtualizer.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param on
+ *   If > 0, enable local Loopback.
+ *   Otherwise, disable local Loopback.
+ * @return
+ *   - (0) if successful.
+ *   - negative if failed.
+ */
+int rte_eth_dev_enable_local_lb(uint16_t port_id, int on);
+
 /**
  * Enable/Disable hardware VLAN Strip by a Rx queue of an Ethernet device.
  *
-- 
2.25.1

RE: [PATCH] net/i40e: disable source pruning

[Morten Brørup]

+CC: Andrew, as Ethernet API maintainer

> From: Ke Zhang [mailto:ke1x.zhang@intel.com]
> Sent: Monday, 9 January 2023 03.20
> 
> VRRP advertisement packets are dropped on i40e PF devices because
> when a MAC address is added to a device, packets originating from
> that MAC address are dropped.
> 
> This patch adds a interface in lib/ethdev to support disabling
> source pruning to work around above issue.

Thank you for the improved patch.

> 
> Bugzilla ID: 648
> 
> Signed-off-by: Ke Zhang <ke1x.zhang@intel.com>
> ---

[...]

+static cmdline_parse_token_string_t cmd_setllb_enalbe =

Typo: enalbe -> enable.

[...]

> +/* i40e_enable_pf_local_lb
> + * @pf: pointer to the pf structure
> + *
> + * allow local loopback on pf
> + */
> +static int
> +i40e_enable_pf_local_lb(struct rte_eth_dev *dev, int on)
> +{
> +	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data-
> >dev_private);
> +	struct i40e_hw *hw = I40E_PF_TO_HW(pf);
> +	struct i40e_vsi_context ctxt;
> +	int ret;
> +
> +	/* Use the FW API if FW >= v5.0 */
> +	if (hw->aq.fw_maj_ver < 5 && hw->mac.type != I40E_MAC_X722) {
> +#ifdef TREX_PATCH
> +		/* Most of our customers do not have latest FW */
> +		PMD_INIT_LOG(INFO, "FW < v5.0, cannot enable local
> loopback");
> +#else
> +		PMD_INIT_LOG(ERR, "FW < v5.0, cannot enable local
> loopback");
> +#endif
> +		return I40E_NOT_SUPPORTED;
> +	}

Can this bug not be fixed without requiring FW >= 5.0?

If VRRP is not supported with older FW, perhaps you could also log a warning when a VRRP MAC address is added to the NIC (and the FW is too old, or local_lb is disabled). Just an idea; it will help people debug issues with VRRP in production.

> +
> +	memset(&ctxt, 0, sizeof(ctxt));
> +	ctxt.seid = pf->main_vsi_seid;
> +	ctxt.pf_num = hw->pf_id;
> +	ret = i40e_aq_get_vsi_params(hw, &ctxt, NULL);
> +	if (ret) {
> +		PMD_DRV_LOG(ERR, "cannot get pf vsi config, err %d, aq_err
> %d",
> +			    ret, hw->aq.asq_last_status);
> +		return ret;
> +	}
> +	ctxt.flags = I40E_AQ_VSI_TYPE_PF;
> +	ctxt.info.valid_sections =
> +		rte_cpu_to_le_16(I40E_AQ_VSI_PROP_SWITCH_VALID);
> +	if (on)
> +		ctxt.info.switch_id |=
> +			rte_cpu_to_le_16(I40E_AQ_VSI_SW_ID_FLAG_LOCAL_LB);
> +	else
> +		ctxt.info.switch_id &=
> +			~rte_cpu_to_le_16(I40E_AQ_VSI_SW_ID_FLAG_LOCAL_LB);
> +
> +	ret = i40e_aq_update_vsi_params(hw, &ctxt, NULL);
> +	if (ret)
> +		PMD_DRV_LOG(ERR, "update vsi switch failed, aq_err=%d",
> +			    hw->aq.asq_last_status);
> +
> +	return ret;
> +}

[...]

> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index c129ca1eaf..c4888ebd62 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -3437,6 +3437,21 @@ int rte_eth_dev_set_mtu(uint16_t port_id,
> uint16_t mtu);
>   */
>  int rte_eth_dev_vlan_filter(uint16_t port_id, uint16_t vlan_id, int
> on);
> 
> +/**
> + * Enable/Disable local Loopback for VSIs that are used as uplink of a
> software
> + * (cascaded) VEB, VEPA or Port Virtualizer.
> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param on
> + *   If > 0, enable local Loopback.
> + *   Otherwise, disable local Loopback.
> + * @return
> + *   - (0) if successful.
> + *   - negative if failed.
> + */
> +int rte_eth_dev_enable_local_lb(uint16_t port_id, int on);
> +

Local Loopback usually means that packets queued for TX are looped back into RX by the NIC or PHY.

If the Intel X710/XXV710/XL710 "Source Pruning" feature only applies to egressing packets internally looped back to ingress by VEB, this patch makes some sense, although I wish you could come up with a better name for it than Local Loopback.

However, if the "Source Pruning" feature also applies to packets ingressing from the physical Ethernet interface, this naming and behavior is counterintuitive. In this case, "Source Pruning" is a special filter, which other NICs don't apply to ingressing packets, so the PMD should not enable the filter (and thus behave differently than all other NICs) unless explicitly enabled by the application.

Re: [PATCH] net/i40e: disable source pruning

[Ferruh Yigit]

On 1/9/2023 2:20 AM, Ke Zhang wrote:
> VRRP advertisement packets are dropped on i40e PF devices because
> when a MAC address is added to a device, packets originating from
> that MAC address are dropped.
> 
> This patch adds a interface in lib/ethdev to support disabling
> source pruning to work around above issue.
> 

Hi Ke,

We have a dilemma between enabling as much HW config/feature as possible
and having common/portable APIs.

If we add a new API for each specific HW config/feature, it will be
confusing for users and DPDK APIs won't be portable. We may end up
slightly specific version of an API specific to a NIC.
For this we used PMD specific APIs time to time (like
drivers/net/i40e/rte_pmd_i40e.h), although is not also a good solution.


If this is a common config/feature for more than one NIC, I am OK to
procced with the patch as it is, for this I have cc'ed a few more folks,
but first can you please describe the config/feature a little more
detailed please.

What I understand is if packet target MAC address is same as device MAC
address, packet is dropped, which is called 'source pruning' feature.
What is the use case for this, is the expectation send a packet to
router/switch and expect to receive it back?

Is all egress packets dropped as described? Becase feture referred as
"local loopback" in the code, is this only applied to packets switched
locally in NIC?

Can it be solution to always disable source pruning by default?


If this is specific to i40e, I suggest adding it as PMD specific API.
I put some comments below but first need to clarify how to procced with
the patch.



> Bugzilla ID: 648
> 
> Signed-off-by: Ke Zhang <ke1x.zhang@intel.com>
> ---
>  app/test-pmd/cmdline.c         | 77 ++++++++++++++++++++++++++++++++++
>  drivers/net/i40e/i40e_ethdev.c | 53 +++++++++++++++++++++++
>  lib/ethdev/ethdev_driver.h     |  6 +++
>  lib/ethdev/rte_ethdev.c        | 16 +++++++
>  lib/ethdev/rte_ethdev.h        | 15 +++++++
>  5 files changed, 167 insertions(+)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index cb8c174020..a9602a2ed0 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -482,6 +482,9 @@ static void cmd_help_long_parsed(void *parsed_result,
>  			"set promisc (port_id|all) (on|off)\n"
>  			"    Set the promiscuous mode on port_id, or all.\n\n"
>  
> +			"set llb (port_id|all) (on|off)\n"
> +			"    Set vsi local loopback on port_id, or all.\n\n"
> +

Please don't use abreviations, specially for not commonly knows
features, 'llb' won't make sense to many people, and as number of this
unkown commonds increase it has a risk to make testpmd unusable,
please prefer 'local_loopback'.

And please don't refer to 'vsi' here.

And personally I don't prefer using 'set xxx' commands to configure
device, what I prefer:

"set xxx"                   --> set testpmd specific configuration
"port config <port_id> xxx" --> configure device


Although I am aware this separation is not clear and there is a mixuture
of usage, some of them historical.


>  			"set allmulti (port_id|all) (on|off)\n"
>  			"    Set the allmulti mode on port_id, or all.\n\n"
>  
> @@ -5775,6 +5778,78 @@ static cmdline_parse_inst_t cmd_set_promisc_mode_one = {
>  	},
>  };
>  
> +/* *** SET VSI LOCAL LOOPBACK *** *> +struct cmd_set_vsi_local_lb_result {
> +	cmdline_fixed_string_t set;
> +	cmdline_fixed_string_t llb;
> +	cmdline_fixed_string_t port_all; /* valid if "allports" argument == 1 */
> +	uint16_t port_num;               /* valid if "allports" argument == 0 */
> +	cmdline_fixed_string_t enable;
> +};
> +
> +static void cmd_set_vsi_llb_parsed(void *parsed_result,
> +					__rte_unused struct cmdline *cl,
> +					void *allports)
> +{
> +	struct cmd_set_vsi_local_lb_result *res = parsed_result;
> +	int enable;
> +	portid_t i;
> +
> +	if (!strcmp(res->enable, "on"))
> +		enable = 1;
> +	else
> +		enable = 0;
> +
> +	/* all ports */
> +	if (allports) {
> +		RTE_ETH_FOREACH_DEV(i)
> +			rte_eth_dev_enable_local_lb(i, enable);
> +	} else {
> +		rte_eth_dev_enable_local_lb(res->port_num, enable);
> +	}
> +}
> +
> +static cmdline_parse_token_string_t cmd_setllb_set =
> +	TOKEN_STRING_INITIALIZER(struct cmd_set_vsi_local_lb_result, set, "set");
> +static cmdline_parse_token_string_t cmd_setllb_llb =
> +	TOKEN_STRING_INITIALIZER(struct cmd_set_vsi_local_lb_result, llb,
> +				 "llb");
> +static cmdline_parse_token_string_t cmd_setllb_portall =
> +	TOKEN_STRING_INITIALIZER(struct cmd_set_vsi_local_lb_result, port_all,
> +				 "all");
> +static cmdline_parse_token_num_t cmd_setllb_portnum =
> +	TOKEN_NUM_INITIALIZER(struct cmd_set_vsi_local_lb_result, port_num,
> +			      RTE_UINT16);
> +static cmdline_parse_token_string_t cmd_setllb_enalbe =
> +	TOKEN_STRING_INITIALIZER(struct cmd_set_vsi_local_lb_result, enable,
> +				 "on#off");
> +
> +static cmdline_parse_inst_t cmd_set_vsi_enable_all = {
> +	.f = cmd_set_vsi_llb_parsed,
> +	.data = (void *)1,
> +	.help_str = "set llb all on|off: Set vsi local loopback for all ports",
> +	.tokens = {
> +		(void *)&cmd_setllb_set,
> +		(void *)&cmd_setllb_llb,
> +		(void *)&cmd_setllb_portall,
> +		(void *)&cmd_setllb_enalbe,
> +		NULL,
> +	},
> +};
> +
> +static cmdline_parse_inst_t cmd_set_vsi_enable_one = {
> +	.f = cmd_set_vsi_llb_parsed,
> +	.data = (void *)0,
> +	.help_str = "set llb <port_id> on|off: Set vsi local loopback on port id",
> +	.tokens = {
> +		(void *)&cmd_setllb_set,
> +		(void *)&cmd_setllb_llb,
> +		(void *)&cmd_setllb_portnum,
> +		(void *)&cmd_setllb_enalbe,
> +		NULL,
> +	},
> +};

please dont use 'vsi' in the scope of testpmd commands.

> +
>  /* *** SET ALLMULTI MODE *** */
>  struct cmd_set_allmulti_mode_result {
>  	cmdline_fixed_string_t set;
> @@ -12866,6 +12941,8 @@ static cmdline_parse_ctx_t builtin_ctx[] = {
>  	(cmdline_parse_inst_t *)&cmd_show_port_cman_capa,
>  	(cmdline_parse_inst_t *)&cmd_show_port_cman_config,
>  	(cmdline_parse_inst_t *)&cmd_set_port_cman_config,
> +	(cmdline_parse_inst_t *)&cmd_set_vsi_enable_all,
> +	(cmdline_parse_inst_t *)&cmd_set_vsi_enable_one,

Instead of adding to end better to try it grouping with relevant commands.

>  	NULL,
>  };
>  
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 7726a89d99..dd07c38f0f 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -406,6 +406,7 @@ static void i40e_ethertype_filter_restore(struct i40e_pf *pf);
>  static void i40e_tunnel_filter_restore(struct i40e_pf *pf);
>  static void i40e_filter_restore(struct i40e_pf *pf);
>  static void i40e_notify_all_vfs_link_status(struct rte_eth_dev *dev);
> +static int i40e_enable_pf_local_lb(struct rte_eth_dev *dev, int on);
>  
>  static const char *const valid_keys[] = {
>  	ETH_I40E_FLOATING_VEB_ARG,
> @@ -517,6 +518,7 @@ static const struct eth_dev_ops i40e_eth_dev_ops = {
>  	.tm_ops_get                   = i40e_tm_ops_get,
>  	.tx_done_cleanup              = i40e_tx_done_cleanup,
>  	.get_monitor_addr             = i40e_get_monitor_addr,
> +	.enable_local_lb              = i40e_enable_pf_local_lb,
>  };
>  
>  /* store statistics names and its offset in stats structure */
> @@ -5634,6 +5636,57 @@ i40e_enable_pf_lb(struct i40e_pf *pf)
>  			    hw->aq.asq_last_status);
>  }
>  
> +/* i40e_enable_pf_local_lb
> + * @pf: pointer to the pf structure
> + *
> + * allow local loopback on pf
> + */
> +static int
> +i40e_enable_pf_local_lb(struct rte_eth_dev *dev, int on)
> +{
> +	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
> +	struct i40e_hw *hw = I40E_PF_TO_HW(pf);
> +	struct i40e_vsi_context ctxt;
> +	int ret;
> +
> +	/* Use the FW API if FW >= v5.0 */
> +	if (hw->aq.fw_maj_ver < 5 && hw->mac.type != I40E_MAC_X722) {
> +#ifdef TREX_PATCH
> +		/* Most of our customers do not have latest FW */
> +		PMD_INIT_LOG(INFO, "FW < v5.0, cannot enable local loopback");
> +#else
> +		PMD_INIT_LOG(ERR, "FW < v5.0, cannot enable local loopback");
> +#endif
> +		return I40E_NOT_SUPPORTED;
> +	}
> +
> +	memset(&ctxt, 0, sizeof(ctxt));
> +	ctxt.seid = pf->main_vsi_seid;
> +	ctxt.pf_num = hw->pf_id;
> +	ret = i40e_aq_get_vsi_params(hw, &ctxt, NULL);
> +	if (ret) {
> +		PMD_DRV_LOG(ERR, "cannot get pf vsi config, err %d, aq_err %d",
> +			    ret, hw->aq.asq_last_status);
> +		return ret;
> +	}
> +	ctxt.flags = I40E_AQ_VSI_TYPE_PF;
> +	ctxt.info.valid_sections =
> +		rte_cpu_to_le_16(I40E_AQ_VSI_PROP_SWITCH_VALID);
> +	if (on)
> +		ctxt.info.switch_id |=
> +			rte_cpu_to_le_16(I40E_AQ_VSI_SW_ID_FLAG_LOCAL_LB);
> +	else
> +		ctxt.info.switch_id &=
> +			~rte_cpu_to_le_16(I40E_AQ_VSI_SW_ID_FLAG_LOCAL_LB);
> +
> +	ret = i40e_aq_update_vsi_params(hw, &ctxt, NULL);
> +	if (ret)
> +		PMD_DRV_LOG(ERR, "update vsi switch failed, aq_err=%d",
> +			    hw->aq.asq_last_status);
> +
> +	return ret;
> +}
> +
>  /* Setup a VSI */
>  struct i40e_vsi *
>  i40e_vsi_setup(struct i40e_pf *pf,
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index 6a550cfc83..3a11a7fab4 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -668,6 +668,10 @@ typedef int (*eth_get_module_info_t)(struct rte_eth_dev *dev,
>  typedef int (*eth_get_module_eeprom_t)(struct rte_eth_dev *dev,
>  				       struct rte_dev_eeprom_info *info);
>  
> +/** @internal Function used to enable/disable the vsi loop back of an Ethernet device. */
> +typedef int (*eth_enable_local_lb_t)(struct rte_eth_dev *dev,
> +				  int on);
> +

What is 'VSI'? Is it common dpdk or ethdev concept? If not please don't
refer it in ethdev.



>  struct rte_flow_ops;
>  /**
>   * @internal
> @@ -1403,6 +1407,8 @@ struct eth_dev_ops {
>  	eth_cman_config_set_t cman_config_set;
>  	/** Retrieve congestion management configuration */
>  	eth_cman_config_get_t cman_config_get;
> +	/** Enable/disable the vsi loop back of an Ethernet device */
> +	eth_enable_local_lb_t enable_local_lb;

The name 'enable_local_lb' needs to refined.
Why it is called as 'local_lb'?
And again please use loopback instead of 'lb'.

>  };
>  
>  /**
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 5d5e18db1e..3991688e2b 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -6318,6 +6318,22 @@ rte_eth_buffer_split_get_supported_hdr_ptypes(uint16_t port_id, uint32_t *ptypes
>  	return j;
>  }
>  
> +int
> +rte_eth_dev_enable_local_lb(uint16_t port_id, int on)
> +{
> +	struct rte_eth_dev *dev;
> +	int ret;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +	dev = &rte_eth_devices[port_id];
> +
> +	if (*dev->dev_ops->enable_local_lb == NULL)
> +		return -ENOTSUP;
> +
> +	ret = (*dev->dev_ops->enable_local_lb)(dev, on);
> +	return eth_err(port_id, ret);
> +}
> +
>  RTE_LOG_REGISTER_DEFAULT(rte_eth_dev_logtype, INFO);
>  
>  RTE_INIT(ethdev_init_telemetry)
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index c129ca1eaf..c4888ebd62 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -3437,6 +3437,21 @@ int rte_eth_dev_set_mtu(uint16_t port_id, uint16_t mtu);
>   */
>  int rte_eth_dev_vlan_filter(uint16_t port_id, uint16_t vlan_id, int on);
>  
> +/**
> + * Enable/Disable local Loopback for VSIs that are used as uplink of a software
> + * (cascaded) VEB, VEPA or Port Virtualizer.
> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param on
> + *   If > 0, enable local Loopback.
> + *   Otherwise, disable local Loopback.
> + * @return
> + *   - (0) if successful.
> + *   - negative if failed.
> + */
> +int rte_eth_dev_enable_local_lb(uint16_t port_id, int on);
> +

API enable/disable the feature but API name has 'enable',
so it will be calling enable_X API to disable X,
if API is to enable/disable, perhaps it can be names as 'set_X'?

[PATCH v2] net/i40e: support enabling/disabling source pruning

[Ke Zhang]

VRRP advertisement packets are dropped on i40e PF devices because
when a MAC address is added to a device, packets originating from
that MAC address are dropped.

This patch adds a PMD specific API to enable/disable source
pruning to fix above issue.

Bugzilla ID: 648

Signed-off-by: Ke Zhang <ke1x.zhang@intel.com>
---
 app/test-pmd/cmdline.c          | 84 +++++++++++++++++++++++++++++++++
 drivers/net/i40e/i40e_ethdev.c  | 43 +++++++++++++++++
 drivers/net/i40e/i40e_ethdev.h  |  1 +
 drivers/net/i40e/rte_pmd_i40e.c | 20 ++++++++
 drivers/net/i40e/rte_pmd_i40e.h | 16 +++++++
 5 files changed, 164 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index cb8c174020..76a574affd 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -776,6 +776,9 @@ static void cmd_help_long_parsed(void *parsed_result,
 
 			"port cleanup (port_id) txq (queue_id) (free_cnt)\n"
 			"    Cleanup txq mbufs for a specific Tx queue\n\n"
+
+			"port config (port_id|all) src_prune (on|off)\n"
+			"    Set source prune on port_id, or all.\n\n"
 		);
 	}
 
@@ -1619,6 +1622,84 @@ static cmdline_parse_inst_t cmd_config_loopback_specific = {
 	},
 };
 
+/* *** configure source prune for port *** */
+struct cmd_config_src_prune_result {
+	cmdline_fixed_string_t port;
+	cmdline_fixed_string_t keyword;
+	cmdline_fixed_string_t port_all; /* valid if "allports" argument == 1 */
+	uint16_t port_num;               /* valid if "allports" argument == 0 */
+	cmdline_fixed_string_t item;
+	cmdline_fixed_string_t enable;
+};
+
+static void cmd_config_src_prune_parsed(void *parsed_result,
+					__rte_unused struct cmdline *cl,
+					void *allports)
+{
+	struct cmd_config_src_prune_result *res = parsed_result;
+	int enable;
+	portid_t i;
+
+	if (!strcmp(res->enable, "on"))
+		enable = 1;
+	else
+		enable = 0;
+
+	/* all ports */
+	if (allports) {
+		RTE_ETH_FOREACH_DEV(i)
+			rte_pmd_i40e_set_src_prune(i, enable);
+	} else {
+		rte_pmd_i40e_set_src_prune(res->port_num, enable);
+	}
+}
+
+static cmdline_parse_token_string_t cmd_config_src_prune_port =
+	TOKEN_STRING_INITIALIZER(struct cmd_config_src_prune_result, port, "port");
+static cmdline_parse_token_string_t cmd_config_src_prune_keyword =
+	TOKEN_STRING_INITIALIZER(struct cmd_config_src_prune_result, keyword,
+				 "config");
+static cmdline_parse_token_string_t cmd_config_src_prune_portall =
+	TOKEN_STRING_INITIALIZER(struct cmd_config_src_prune_result, port_all,
+				 "all");
+static cmdline_parse_token_num_t cmd_config_src_prune_portnum =
+	TOKEN_NUM_INITIALIZER(struct cmd_config_src_prune_result, port_num,
+			      RTE_UINT16);
+static cmdline_parse_token_string_t cmd_config_src_prune_item =
+	TOKEN_STRING_INITIALIZER(struct cmd_config_src_prune_result,
+			item, "src_prune");
+static cmdline_parse_token_string_t cmd_config_src_prune_enable =
+	TOKEN_STRING_INITIALIZER(struct cmd_config_src_prune_result, enable,
+				 "on#off");
+
+static cmdline_parse_inst_t cmd_config_src_prune_all = {
+	.f = cmd_config_src_prune_parsed,
+	.data = (void *)1,
+	.help_str = "port config all src_prune (on|off): Set source prune on all ports.",
+	.tokens = {
+		(void *)&cmd_config_src_prune_port,
+		(void *)&cmd_config_src_prune_keyword,
+		(void *)&cmd_config_src_prune_portall,
+		(void *)&cmd_config_src_prune_item,
+		(void *)&cmd_config_src_prune_enable,
+		NULL,
+	},
+};
+
+static cmdline_parse_inst_t cmd_config_src_prune_specific = {
+	.f = cmd_config_src_prune_parsed,
+	.data = (void *)0,
+	.help_str = "port config all src_prune (on|off): Set source prune on specific port.",
+	.tokens = {
+		(void *)&cmd_config_src_prune_port,
+		(void *)&cmd_config_src_prune_keyword,
+		(void *)&cmd_config_src_prune_portnum,
+		(void *)&cmd_config_src_prune_item,
+		(void *)&cmd_config_src_prune_enable,
+		NULL,
+	},
+};
+
 /* *** configure txq/rxq, txd/rxd *** */
 struct cmd_config_rx_tx {
 	cmdline_fixed_string_t port;
@@ -12731,6 +12812,8 @@ static cmdline_parse_ctx_t builtin_ctx[] = {
 	(cmdline_parse_inst_t *)&cmd_config_speed_specific,
 	(cmdline_parse_inst_t *)&cmd_config_loopback_all,
 	(cmdline_parse_inst_t *)&cmd_config_loopback_specific,
+	(cmdline_parse_inst_t *)&cmd_config_src_prune_all,
+	(cmdline_parse_inst_t *)&cmd_config_src_prune_specific,
 	(cmdline_parse_inst_t *)&cmd_config_rx_tx,
 	(cmdline_parse_inst_t *)&cmd_config_mtu,
 	(cmdline_parse_inst_t *)&cmd_config_max_pkt_len,
@@ -12850,6 +12933,7 @@ static cmdline_parse_ctx_t builtin_ctx[] = {
 	(cmdline_parse_inst_t *)&cmd_operate_bpf_unld_parse,
 #endif
 	(cmdline_parse_inst_t *)&cmd_config_tx_metadata_specific,
+
 	(cmdline_parse_inst_t *)&cmd_show_tx_metadata,
 	(cmdline_parse_inst_t *)&cmd_show_rx_tx_desc_status,
 	(cmdline_parse_inst_t *)&cmd_show_rx_queue_desc_used_count,
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 7726a89d99..0f993b958e 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -5634,6 +5634,46 @@ i40e_enable_pf_lb(struct i40e_pf *pf)
 			    hw->aq.asq_last_status);
 }
 
+/* i40e_set_pf_source_prune
+ * @pf: pointer to the pf structure
+ * @on: Enable/disable source prune
+ *
+ * set source prune on pf
+ */
+int
+i40e_set_pf_source_prune(struct i40e_pf *pf, int on)
+{
+	struct i40e_hw *hw = I40E_PF_TO_HW(pf);
+	struct i40e_vsi_context ctxt;
+	int ret;
+
+	memset(&ctxt, 0, sizeof(ctxt));
+	ctxt.seid = pf->main_vsi_seid;
+	ctxt.pf_num = hw->pf_id;
+	ret = i40e_aq_get_vsi_params(hw, &ctxt, NULL);
+	if (ret) {
+		PMD_DRV_LOG(ERR, "cannot get pf vsi config, err %d, aq_err %d",
+			    ret, hw->aq.asq_last_status);
+		return ret;
+	}
+	ctxt.flags = I40E_AQ_VSI_TYPE_PF;
+	ctxt.info.valid_sections =
+		rte_cpu_to_le_16(I40E_AQ_VSI_PROP_SWITCH_VALID);
+	if (on)
+		ctxt.info.switch_id &=
+			~rte_cpu_to_le_16(I40E_AQ_VSI_SW_ID_FLAG_LOCAL_LB);
+	else
+		ctxt.info.switch_id |=
+			rte_cpu_to_le_16(I40E_AQ_VSI_SW_ID_FLAG_LOCAL_LB);
+
+	ret = i40e_aq_update_vsi_params(hw, &ctxt, NULL);
+	if (ret)
+		PMD_DRV_LOG(ERR, "update vsi switch failed, aq_err=%d",
+			    hw->aq.asq_last_status);
+
+	return ret;
+}
+
 /* Setup a VSI */
 struct i40e_vsi *
 i40e_vsi_setup(struct i40e_pf *pf,
@@ -5691,6 +5731,9 @@ i40e_vsi_setup(struct i40e_pf *pf,
 		}
 	}
 
+	/* source prune is disabled to support VRRP in default*/
+	i40e_set_pf_source_prune(pf, 0);
+
 	vsi = rte_zmalloc("i40e_vsi", sizeof(struct i40e_vsi), 0);
 	if (!vsi) {
 		PMD_DRV_LOG(ERR, "Failed to allocate memory for vsi");
diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
index 7c4cc44a27..df1591584b 100644
--- a/drivers/net/i40e/i40e_ethdev.h
+++ b/drivers/net/i40e/i40e_ethdev.h
@@ -1427,6 +1427,7 @@ int i40e_pf_calc_configured_queues_num(struct i40e_pf *pf);
 int i40e_pf_reset_rss_reta(struct i40e_pf *pf);
 int i40e_pf_reset_rss_key(struct i40e_pf *pf);
 int i40e_pf_config_rss(struct i40e_pf *pf);
+int i40e_set_pf_source_prune(struct i40e_pf *pf, int on);
 int i40e_set_rss_key(struct i40e_vsi *vsi, uint8_t *key, uint8_t key_len);
 int i40e_set_rss_lut(struct i40e_vsi *vsi, uint8_t *lut, uint16_t lut_size);
 int i40e_vf_representor_init(struct rte_eth_dev *ethdev, void *init_params);
diff --git a/drivers/net/i40e/rte_pmd_i40e.c b/drivers/net/i40e/rte_pmd_i40e.c
index 35829a1eea..b9fc3c24a1 100644
--- a/drivers/net/i40e/rte_pmd_i40e.c
+++ b/drivers/net/i40e/rte_pmd_i40e.c
@@ -445,6 +445,26 @@ rte_pmd_i40e_set_tx_loopback(uint16_t port, uint8_t on)
 	return ret;
 }
 
+int
+rte_pmd_i40e_set_src_prune(uint16_t port, uint8_t on)
+{
+	struct rte_eth_dev *dev;
+	struct i40e_pf *pf;
+	int ret;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
+
+	dev = &rte_eth_devices[port];
+
+	if (!is_i40e_supported(dev))
+		return -ENOTSUP;
+
+	pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
+
+	ret = i40e_set_pf_source_prune(pf, on);
+	return ret;
+}
+
 int
 rte_pmd_i40e_set_vf_unicast_promisc(uint16_t port, uint16_t vf_id, uint8_t on)
 {
diff --git a/drivers/net/i40e/rte_pmd_i40e.h b/drivers/net/i40e/rte_pmd_i40e.h
index 4cb21c3713..76c9296413 100644
--- a/drivers/net/i40e/rte_pmd_i40e.h
+++ b/drivers/net/i40e/rte_pmd_i40e.h
@@ -400,6 +400,22 @@ int rte_pmd_i40e_set_vf_vlan_anti_spoof(uint16_t port,
 int rte_pmd_i40e_set_tx_loopback(uint16_t port,
 				 uint8_t on);
 
+/**
+ * Enable/Disable source prune on all the PF.
+ *
+ * @param port
+ *    The port identifier of the Ethernet device.
+ * @param on
+ *    1 - Enable source prune.
+ *    0 - Disable source prune.
+ * @return
+ *   - (0) if successful.
+ *   - (-ENODEV) if *port* invalid.
+ *   - (-EINVAL) if bad parameter.
+ */
+int rte_pmd_i40e_set_src_prune(uint16_t port,
+				 uint8_t on);
+
 /**
  * Enable/Disable VF unicast promiscuous mode.
  *
-- 
2.25.1

RE: [PATCH v2] net/i40e: support enabling/disabling source pruning

[Morten Brørup]

> From: Ke Zhang [mailto:ke1x.zhang@intel.com]
> Sent: Monday, 30 January 2023 09.09
> 
> VRRP advertisement packets are dropped on i40e PF devices because
> when a MAC address is added to a device, packets originating from
> that MAC address are dropped.
> 
> This patch adds a PMD specific API to enable/disable source
> pruning to fix above issue.
> 
> Bugzilla ID: 648
> 
> Signed-off-by: Ke Zhang <ke1x.zhang@intel.com>
> ---

This fixes what I considered a bug in the driver, so source pruning is now disabled by default. Thank you.

Acked-by: Morten Brørup <mb@smartsharesystems.com>

Should rte_pmd_i40e_set_src_prune be added to a maps file? (I'm not sure about a PMD specific APIs, so please check.)

Re: [PATCH v2] net/i40e: support enabling/disabling source pruning

[David Marchand]

On Mon, Jan 30, 2023 at 9:23 AM Ke Zhang <ke1x.zhang@intel.com> wrote:
>
> VRRP advertisement packets are dropped on i40e PF devices because
> when a MAC address is added to a device, packets originating from
> that MAC address are dropped.
>
> This patch adds a PMD specific API to enable/disable source
> pruning to fix above issue.
>
> Bugzilla ID: 648
>
> Signed-off-by: Ke Zhang <ke1x.zhang@intel.com>
> ---
>  app/test-pmd/cmdline.c          | 84 +++++++++++++++++++++++++++++++++
>  drivers/net/i40e/i40e_ethdev.c  | 43 +++++++++++++++++
>  drivers/net/i40e/i40e_ethdev.h  |  1 +
>  drivers/net/i40e/rte_pmd_i40e.c | 20 ++++++++
>  drivers/net/i40e/rte_pmd_i40e.h | 16 +++++++
>  5 files changed, 164 insertions(+)
>
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index cb8c174020..76a574affd 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -776,6 +776,9 @@ static void cmd_help_long_parsed(void *parsed_result,
>
>                         "port cleanup (port_id) txq (queue_id) (free_cnt)\n"
>                         "    Cleanup txq mbufs for a specific Tx queue\n\n"
> +
> +                       "port config (port_id|all) src_prune (on|off)\n"
> +                       "    Set source prune on port_id, or all.\n\n"
>                 );
>         }
>

- This seems i40e specific, please move to drivers/net/i40e/i40e_testpmd.c.

- Besides, I would prefer that something in the command name clearly
states this is driver (here, i40e) specific.
Like "port config XX i40e_src_prune" or maybe the other way around,
start with a "driver i40e" prefix.

Maybe others have an opinion.


-- 
David Marchand

RE: [PATCH v2] net/i40e: support enabling/disabling source pruning

[Zhang, Ke1X]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Monday, January 30, 2023 4:58 PM
> To: Zhang, Ke1X <ke1x.zhang@intel.com>; mb@smartsharesystems.com;
> thomas@monjalon.net; ferruh.yigit@amd.com
> Cc: dev@dpdk.org; Matz, Olivier <olivier.matz@6wind.com>; Zhang, Yuying
> <yuying.zhang@intel.com>; Xing, Beilei <beilei.xing@intel.com>
> Subject: Re: [PATCH v2] net/i40e: support enabling/disabling source pruning
> 
> On Mon, Jan 30, 2023 at 9:23 AM Ke Zhang <ke1x.zhang@intel.com> wrote:
> >
> > VRRP advertisement packets are dropped on i40e PF devices because
> when
> > a MAC address is added to a device, packets originating from that MAC
> > address are dropped.
> >
> > This patch adds a PMD specific API to enable/disable source pruning to
> > fix above issue.
> >
> > Bugzilla ID: 648
> >
> > Signed-off-by: Ke Zhang <ke1x.zhang@intel.com>
> > ---
> >  app/test-pmd/cmdline.c          | 84
> +++++++++++++++++++++++++++++++++
> >  drivers/net/i40e/i40e_ethdev.c  | 43 +++++++++++++++++
> > drivers/net/i40e/i40e_ethdev.h  |  1 +
> > drivers/net/i40e/rte_pmd_i40e.c | 20 ++++++++
> > drivers/net/i40e/rte_pmd_i40e.h | 16 +++++++
> >  5 files changed, 164 insertions(+)
> >
> > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > cb8c174020..76a574affd 100644
> > --- a/app/test-pmd/cmdline.c
> > +++ b/app/test-pmd/cmdline.c
> > @@ -776,6 +776,9 @@ static void cmd_help_long_parsed(void
> > *parsed_result,
> >
> >                         "port cleanup (port_id) txq (queue_id) (free_cnt)\n"
> >                         "    Cleanup txq mbufs for a specific Tx queue\n\n"
> > +
> > +                       "port config (port_id|all) src_prune (on|off)\n"
> > +                       "    Set source prune on port_id, or all.\n\n"
> >                 );
> >         }
> >
> 
> - This seems i40e specific, please move to drivers/net/i40e/i40e_testpmd.c.
> 
> - Besides, I would prefer that something in the command name clearly states
> this is driver (here, i40e) specific.
> Like "port config XX i40e_src_prune" or maybe the other way around, start
> with a "driver i40e" prefix.
> 
> Maybe others have an opinion.
> 
> 
> --
> David Marchand

Thanks for your comments, this command could be used for other Intel NIC like ice,
And It is only finished for i40e in this story: https://jira.devtools.intel.com/browse/DPDK-29564

Re: [PATCH v2] net/i40e: support enabling/disabling source pruning

[Thomas Monjalon]

31/01/2023 04:28, Zhang, Ke1X:
> From: David Marchand <david.marchand@redhat.com>
> > On Mon, Jan 30, 2023 at 9:23 AM Ke Zhang <ke1x.zhang@intel.com> wrote:
> > >
> > > VRRP advertisement packets are dropped on i40e PF devices because
> > when
> > > a MAC address is added to a device, packets originating from that MAC
> > > address are dropped.
> > >
> > > This patch adds a PMD specific API to enable/disable source pruning to
> > > fix above issue.
> > >
> > > Bugzilla ID: 648
> > >
> > > Signed-off-by: Ke Zhang <ke1x.zhang@intel.com>
> > > ---
> > >  app/test-pmd/cmdline.c          | 84
> > +++++++++++++++++++++++++++++++++
> > >  drivers/net/i40e/i40e_ethdev.c  | 43 +++++++++++++++++
> > > drivers/net/i40e/i40e_ethdev.h  |  1 +
> > > drivers/net/i40e/rte_pmd_i40e.c | 20 ++++++++
> > > drivers/net/i40e/rte_pmd_i40e.h | 16 +++++++
> > >  5 files changed, 164 insertions(+)
> > >
> > > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > > cb8c174020..76a574affd 100644
> > > --- a/app/test-pmd/cmdline.c
> > > +++ b/app/test-pmd/cmdline.c
> > > @@ -776,6 +776,9 @@ static void cmd_help_long_parsed(void
> > > *parsed_result,
> > >
> > >                         "port cleanup (port_id) txq (queue_id) (free_cnt)\n"
> > >                         "    Cleanup txq mbufs for a specific Tx queue\n\n"
> > > +
> > > +                       "port config (port_id|all) src_prune (on|off)\n"
> > > +                       "    Set source prune on port_id, or all.\n\n"
> > >                 );
> > >         }
> > >
> > 
> > - This seems i40e specific, please move to drivers/net/i40e/i40e_testpmd.c.
> > 
> > - Besides, I would prefer that something in the command name clearly states
> > this is driver (here, i40e) specific.
> > Like "port config XX i40e_src_prune" or maybe the other way around, start
> > with a "driver i40e" prefix.
> > 
> > Maybe others have an opinion.
> > 
> > 
> > --
> > David Marchand
> 
> Thanks for your comments, this command could be used for other Intel NIC like ice,
> And It is only finished for i40e in this story: https://jira.devtools.intel.com/browse/DPDK-29564

Only Intel has access to this URL.

On the principle, you should not update testpmd commands in a patch for i40e.
We add testpmd commands when adding new features in ethdev.

Here specifically, you are adding rte_pmd_i40e_set_src_prune()
with a vague explanation: "Enable/Disable source prune on all the PF."
Without more information, I cannot judge whether the feature is generic or not.
So for now, this is a nack.

RE: [PATCH v2] net/i40e: support enabling/disabling source pruning

[Zhang, Ke1X]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, February 1, 2023 7:11 PM
> To: David Marchand <david.marchand@redhat.com>;
> mb@smartsharesystems.com; ferruh.yigit@amd.com; Zhang, Ke1X
> <ke1x.zhang@intel.com>
> Cc: dev@dpdk.org; Matz, Olivier <olivier.matz@6wind.com>; Zhang, Yuying
> <yuying.zhang@intel.com>; Xing, Beilei <beilei.xing@intel.com>
> Subject: Re: [PATCH v2] net/i40e: support enabling/disabling source pruning
> 
> 31/01/2023 04:28, Zhang, Ke1X:
> > From: David Marchand <david.marchand@redhat.com>
> > > On Mon, Jan 30, 2023 at 9:23 AM Ke Zhang <ke1x.zhang@intel.com>
> wrote:
> > > >
> > > > VRRP advertisement packets are dropped on i40e PF devices because
> > > when
> > > > a MAC address is added to a device, packets originating from that
> > > > MAC address are dropped.
> > > >
> > > > This patch adds a PMD specific API to enable/disable source
> > > > pruning to fix above issue.
> > > >
> > > > Bugzilla ID: 648
> > > >
> > > > Signed-off-by: Ke Zhang <ke1x.zhang@intel.com>
> > > > ---
> > > >  app/test-pmd/cmdline.c          | 84
> > > +++++++++++++++++++++++++++++++++
> > > >  drivers/net/i40e/i40e_ethdev.c  | 43 +++++++++++++++++
> > > > drivers/net/i40e/i40e_ethdev.h  |  1 +
> > > > drivers/net/i40e/rte_pmd_i40e.c | 20 ++++++++
> > > > drivers/net/i40e/rte_pmd_i40e.h | 16 +++++++
> > > >  5 files changed, 164 insertions(+)
> > > >
> > > > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > > > cb8c174020..76a574affd 100644
> > > > --- a/app/test-pmd/cmdline.c
> > > > +++ b/app/test-pmd/cmdline.c
> > > > @@ -776,6 +776,9 @@ static void cmd_help_long_parsed(void
> > > > *parsed_result,
> > > >
> > > >                         "port cleanup (port_id) txq (queue_id) (free_cnt)\n"
> > > >                         "    Cleanup txq mbufs for a specific Tx queue\n\n"
> > > > +
> > > > +                       "port config (port_id|all) src_prune (on|off)\n"
> > > > +                       "    Set source prune on port_id, or all.\n\n"
> > > >                 );
> > > >         }
> > > >
> > >
> > > - This seems i40e specific, please move to
> drivers/net/i40e/i40e_testpmd.c.
> > >
> > > - Besides, I would prefer that something in the command name clearly
> > > states this is driver (here, i40e) specific.
> > > Like "port config XX i40e_src_prune" or maybe the other way around,
> > > start with a "driver i40e" prefix.
> > >
> > > Maybe others have an opinion.
> > >
> > >
> > > --
> > > David Marchand
> >
> > Thanks for your comments, this command could be used for other Intel
> > NIC like ice, And It is only finished for i40e in this story:
> > https://jira.devtools.intel.com/browse/DPDK-29564
> 
> Only Intel has access to this URL.
> 
> On the principle, you should not update testpmd commands in a patch for
> i40e.
> We add testpmd commands when adding new features in ethdev.
> 
> Here specifically, you are adding rte_pmd_i40e_set_src_prune() with a
> vague explanation: "Enable/Disable source prune on all the PF."
> Without more information, I cannot judge whether the feature is generic or
> not.
> So for now, this is a nack.
> 
Thanks for your comments, this feature is only for intel NIC.
Would you please share a solution for this case?
Thanks.

Re: [PATCH v2] net/i40e: support enabling/disabling source pruning

[Thomas Monjalon]

07/02/2023 02:40, Zhang, Ke1X:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 31/01/2023 04:28, Zhang, Ke1X:
> > > From: David Marchand <david.marchand@redhat.com>
> > > > On Mon, Jan 30, 2023 at 9:23 AM Ke Zhang <ke1x.zhang@intel.com>
> > wrote:
> > > > >
> > > > > VRRP advertisement packets are dropped on i40e PF devices because
> > > > when
> > > > > a MAC address is added to a device, packets originating from that
> > > > > MAC address are dropped.
> > > > >
> > > > > This patch adds a PMD specific API to enable/disable source
> > > > > pruning to fix above issue.
> > > > >
> > > > > Bugzilla ID: 648
> > > > >
> > > > > Signed-off-by: Ke Zhang <ke1x.zhang@intel.com>
> > > > > ---
> > > > >  app/test-pmd/cmdline.c          | 84
> > > > +++++++++++++++++++++++++++++++++
> > > > >  drivers/net/i40e/i40e_ethdev.c  | 43 +++++++++++++++++
> > > > > drivers/net/i40e/i40e_ethdev.h  |  1 +
> > > > > drivers/net/i40e/rte_pmd_i40e.c | 20 ++++++++
> > > > > drivers/net/i40e/rte_pmd_i40e.h | 16 +++++++
> > > > >  5 files changed, 164 insertions(+)
> > > > >
> > > > > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > > > > cb8c174020..76a574affd 100644
> > > > > --- a/app/test-pmd/cmdline.c
> > > > > +++ b/app/test-pmd/cmdline.c
> > > > > @@ -776,6 +776,9 @@ static void cmd_help_long_parsed(void
> > > > > *parsed_result,
> > > > >
> > > > >                         "port cleanup (port_id) txq (queue_id) (free_cnt)\n"
> > > > >                         "    Cleanup txq mbufs for a specific Tx queue\n\n"
> > > > > +
> > > > > +                       "port config (port_id|all) src_prune (on|off)\n"
> > > > > +                       "    Set source prune on port_id, or all.\n\n"
> > > > >                 );
> > > > >         }
> > > > >
> > > >
> > > > - This seems i40e specific, please move to
> > drivers/net/i40e/i40e_testpmd.c.
> > > >
> > > > - Besides, I would prefer that something in the command name clearly
> > > > states this is driver (here, i40e) specific.
> > > > Like "port config XX i40e_src_prune" or maybe the other way around,
> > > > start with a "driver i40e" prefix.
> > > >
> > > > Maybe others have an opinion.
> > > >
> > > >
> > > > --
> > > > David Marchand
> > >
> > > Thanks for your comments, this command could be used for other Intel
> > > NIC like ice, And It is only finished for i40e in this story:
> > > https://jira.devtools.intel.com/browse/DPDK-29564
> > 
> > Only Intel has access to this URL.
> > 
> > On the principle, you should not update testpmd commands in a patch for
> > i40e.
> > We add testpmd commands when adding new features in ethdev.
> > 
> > Here specifically, you are adding rte_pmd_i40e_set_src_prune() with a
> > vague explanation: "Enable/Disable source prune on all the PF."
> > Without more information, I cannot judge whether the feature is generic or
> > not.
> > So for now, this is a nack.
> > 
> Thanks for your comments, this feature is only for intel NIC.
> Would you please share a solution for this case?

Host testpmd code in your driver directory
with a command name starting with the name of your driver: "i40e port ...".
You can look at what is done for mlx5 in drivers/net/mlx5/mlx5_testpmd.c