[PATCH 0/2] enable VF untag drop

[PATCH 0/2] enable VF untag drop

[Qi Zhang]

Add a new API to enable untag drop for a specific VF. 
This is an experimental API.

Qi Zhang (2):
  net/i40e: enable VF untag drop
  app/testpmd: enable VF untag drop in testpmd

 app/test-pmd/cmdline.c          | 104 ++++++++++++++++++++++++++++++++++++++++
 drivers/net/i40e/i40e_ethdev.c  |  49 +++++++++++++++++++
 drivers/net/i40e/rte_pmd_i40e.h |  18 +++++++
 3 files changed, 171 insertions(+)

-- 
2.9.3

[PATCH 1/2] net/i40e: enable VF untag drop

[Qi Zhang]

Add a new private API to support the untag drop enable/disable
for specific VF.

Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c  | 49 +++++++++++++++++++++++++++++++++++++++++
 drivers/net/i40e/rte_pmd_i40e.h | 18 +++++++++++++++
 2 files changed, 67 insertions(+)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 303027b..9915cb2 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -11212,3 +11212,52 @@ rte_pmd_i40e_reset_vf_stats(uint8_t port,
 
 	return 0;
 }
+
+int rte_pmd_i40e_set_vf_vlan_untag_drop(uint8_t port,
+					uint16_t vf_id,
+					uint8_t on)
+{
+	struct rte_eth_dev *dev;
+	struct i40e_pf *pf;
+	struct i40e_vsi *vsi;
+	struct i40e_aqc_add_remove_vlan_element_data vlan_data = {0};
+	struct i40e_hw *hw;
+	int ret;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
+
+	dev = &rte_eth_devices[port];
+
+	if (!is_device_supported(dev, &rte_i40e_pmd))
+		return -ENOTSUP;
+
+	pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
+
+	if (vf_id >= pf->vf_num || !pf->vfs) {
+		PMD_DRV_LOG(ERR, "Invalid VF ID.");
+		return -EINVAL;
+	}
+
+	vsi = pf->vfs[vf_id].vsi;
+	if (!vsi) {
+		PMD_DRV_LOG(ERR, "Invalid VSI.");
+		return -EINVAL;
+	}
+
+	hw = I40E_VSI_TO_HW(vsi);
+	vlan_data.vlan_tag = 0;
+	vlan_data.vlan_flags = 0x1;
+
+	if (on) {
+		ret = i40e_aq_add_vlan(hw, vsi->seid, &vlan_data, 1, NULL);
+		if (ret != I40E_SUCCESS)
+			PMD_DRV_LOG(ERR, "Failed to add vlan filter");
+	} else {
+		ret = i40e_aq_remove_vlan(hw, vsi->seid,
+					&vlan_data, 1, NULL);
+		if (ret != I40E_SUCCESS)
+			PMD_DRV_LOG(ERR, "Failed to remove vlan filter");
+	}
+
+	return ret;
+}
diff --git a/drivers/net/i40e/rte_pmd_i40e.h b/drivers/net/i40e/rte_pmd_i40e.h
index a0ad88c..895e2cc 100644
--- a/drivers/net/i40e/rte_pmd_i40e.h
+++ b/drivers/net/i40e/rte_pmd_i40e.h
@@ -332,4 +332,22 @@ int rte_pmd_i40e_get_vf_stats(uint8_t port,
 int rte_pmd_i40e_reset_vf_stats(uint8_t port,
 				uint16_t vf_id);
 
+/**
+ * Enable/Disable VF untag drop
+ *
+ * @param port
+ *    The port identifier of the Ethernet device.
+ * @param vf_id
+ *    VF on witch to enable/disable
+ * @param on
+ *    Enable or Disable
+ * @retura
+ *  - (0) if successful.
+ *  -(-ENODEVE) if *port* invalid
+ *  -(-EINVAL) if bad parameter.
+ */
+int rte_pmd_i40e_set_vf_vlan_untag_drop(uint8_t port,
+					uint16_t vf_id,
+					uint8_t on);
+
 #endif /* _PMD_I40E_H_ */
-- 
2.9.3

[PATCH 2/2] app/testpmd: enable VF untag drop in testpmd

[Qi Zhang]

Add command line to support untag drop to specific VF in
testpmd.

Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
---
 app/test-pmd/cmdline.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 104 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 43fc636..4ddc2c9 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -311,6 +311,10 @@ static void cmd_help_long_parsed(void *parsed_result,
 
 			"set vf vlan antispoof (port_id) (vf_id) (on|off)\n"
 			"    Set VLAN antispoof for a VF from the PF.\n\n"
+#ifdef RTE_LIBRTE_I40E_PMD
+			"set vf vlan untagdrop (port_id) (vf_id) (on|off)\n"
+			"    Set VLAN untag drop for a VF from the PF.\n\n"
+#endif
 
 			"set vf vlan tag (port_id) (vf_id) (on|off)\n"
 			"    Set VLAN tag for a VF from the PF.\n\n"
@@ -10995,6 +10999,103 @@ cmdline_parse_inst_t cmd_set_vf_vlan_anti_spoof = {
 	},
 };
 
+#ifdef RTE_LIBRTE_I40E_PMD
+/* vf vlan untag drop configuration */
+
+/* Common result structure for vf vlan untag drop */
+struct cmd_vf_vlan_untag_drop_result {
+	cmdline_fixed_string_t set;
+	cmdline_fixed_string_t vf;
+	cmdline_fixed_string_t vlan;
+	cmdline_fixed_string_t untagdrop;
+	uint8_t port_id;
+	uint32_t vf_id;
+	cmdline_fixed_string_t on_off;
+};
+
+/* Common CLI fields for vf vlan untag drop enable disable */
+cmdline_parse_token_string_t cmd_vf_vlan_untag_drop_set =
+	TOKEN_STRING_INITIALIZER
+		(struct cmd_vf_vlan_untag_drop_result,
+		 set, "set");
+cmdline_parse_token_string_t cmd_vf_vlan_untag_drop_vf =
+	TOKEN_STRING_INITIALIZER
+		(struct cmd_vf_vlan_untag_drop_result,
+		 vf, "vf");
+cmdline_parse_token_string_t cmd_vf_vlan_untag_drop_vlan =
+	TOKEN_STRING_INITIALIZER
+		(struct cmd_vf_vlan_untag_drop_result,
+		 vlan, "vlan");
+cmdline_parse_token_string_t cmd_vf_vlan_untag_drop_untagdrop =
+	TOKEN_STRING_INITIALIZER
+		(struct cmd_vf_vlan_untag_drop_result,
+		 untagdrop, "untagdrop");
+cmdline_parse_token_num_t cmd_vf_vlan_untag_drop_port_id =
+	TOKEN_NUM_INITIALIZER
+		(struct cmd_vf_vlan_untag_drop_result,
+		 port_id, UINT8);
+cmdline_parse_token_num_t cmd_vf_vlan_untag_drop_vf_id =
+	TOKEN_NUM_INITIALIZER
+		(struct cmd_vf_vlan_untag_drop_result,
+		 vf_id, UINT32);
+cmdline_parse_token_string_t cmd_vf_vlan_untag_drop_on_off =
+	TOKEN_STRING_INITIALIZER
+		(struct cmd_vf_vlan_untag_drop_result,
+		 on_off, "on#off");
+
+static void
+cmd_set_vf_vlan_untag_drop_parsed(
+	void *parsed_result,
+	__attribute__((unused)) struct cmdline *cl,
+	__attribute__((unused)) void *data)
+{
+	struct cmd_vf_vlan_untag_drop_result *res = parsed_result;
+	int ret = -ENOTSUP;
+
+	__rte_unused int is_on = (strcmp(res->on_off, "on") == 0) ? 1 : 0;
+
+	if (port_id_is_invalid(res->port_id, ENABLED_WARN))
+		return;
+
+	if (ret == -ENOTSUP)
+		ret = rte_pmd_i40e_set_vf_vlan_untag_drop(res->port_id,
+				res->vf_id, is_on);
+
+	switch (ret) {
+	case 0:
+		break;
+	case -EINVAL:
+		printf("invalid vf_id %d\n", res->vf_id);
+		break;
+	case -ENODEV:
+		printf("invalid port_id %d\n", res->port_id);
+		break;
+	case -ENOTSUP:
+		printf("function not implemented\n");
+		break;
+	default:
+		printf("programming error: (%s)\n", strerror(-ret));
+	}
+}
+
+cmdline_parse_inst_t cmd_set_vf_vlan_untag_drop = {
+	.f = cmd_set_vf_vlan_untag_drop_parsed,
+	.data = NULL,
+	.help_str = "set vf vlan untagdrop <port_id> <vf_id> on|off",
+	.tokens = {
+		(void *)&cmd_vf_vlan_untag_drop_set,
+		(void *)&cmd_vf_vlan_untag_drop_vf,
+		(void *)&cmd_vf_vlan_untag_drop_vlan,
+		(void *)&cmd_vf_vlan_untag_drop_untagdrop,
+		(void *)&cmd_vf_vlan_untag_drop_port_id,
+		(void *)&cmd_vf_vlan_untag_drop_vf_id,
+		(void *)&cmd_vf_vlan_untag_drop_on_off,
+		NULL,
+	},
+};
+
+#endif
+
 /* vf mac anti spoof configuration */
 
 /* Common result structure for vf mac anti spoof */
@@ -12549,6 +12650,9 @@ cmdline_parse_ctx_t main_ctx[] = {
 	(cmdline_parse_inst_t *)&cmd_config_e_tag_filter_add,
 	(cmdline_parse_inst_t *)&cmd_config_e_tag_filter_del,
 	(cmdline_parse_inst_t *)&cmd_set_vf_vlan_anti_spoof,
+#ifdef RTE_LIBRTE_I40E_PMD
+	(cmdline_parse_inst_t *)&cmd_set_vf_vlan_untag_drop,
+#endif
 	(cmdline_parse_inst_t *)&cmd_set_vf_mac_anti_spoof,
 	(cmdline_parse_inst_t *)&cmd_set_vf_vlan_stripq,
 	(cmdline_parse_inst_t *)&cmd_set_vf_vlan_insert,
-- 
2.9.3

Re: [PATCH 1/2] net/i40e: enable VF untag drop

[Ferruh Yigit]

On 3/3/2017 1:59 AM, Qi Zhang wrote:
> Add a new private API to support the untag drop enable/disable
> for specific VF.
> 
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> ---
>  drivers/net/i40e/i40e_ethdev.c  | 49 +++++++++++++++++++++++++++++++++++++++++
>  drivers/net/i40e/rte_pmd_i40e.h | 18 +++++++++++++++

Shared library is giving build error because of API is missing in
*version.map file

>  2 files changed, 67 insertions(+)
> 

<...>

> diff --git a/drivers/net/i40e/rte_pmd_i40e.h b/drivers/net/i40e/rte_pmd_i40e.h
> index a0ad88c..895e2cc 100644
> --- a/drivers/net/i40e/rte_pmd_i40e.h
> +++ b/drivers/net/i40e/rte_pmd_i40e.h
> @@ -332,4 +332,22 @@ int rte_pmd_i40e_get_vf_stats(uint8_t port,
>  int rte_pmd_i40e_reset_vf_stats(uint8_t port,
>  				uint16_t vf_id);
>  
> +/**
> + * Enable/Disable VF untag drop
> + *
> + * @param port
> + *    The port identifier of the Ethernet device.
> + * @param vf_id
> + *    VF on witch to enable/disable
> + * @param on
> + *    Enable or Disable
> + * @retura

@return

> + *  - (0) if successful.
> + *  -(-ENODEVE) if *port* invalid
> + *  -(-EINVAL) if bad parameter.
> + */
> +int rte_pmd_i40e_set_vf_vlan_untag_drop(uint8_t port,
> +					uint16_t vf_id,
> +					uint8_t on);

As discussed previously, I believe it is good to keep following syntax
in API:
<name_space>_<object>_<action>, for this API it becomes:

rte_pmd_i40e_vf_vlan_untag_drop_set(), and perhaps "set" can be removed?

> +
>  #endif /* _PMD_I40E_H_ */
> 

Re: [PATCH 2/2] app/testpmd: enable VF untag drop in testpmd

[Ferruh Yigit]

On 3/3/2017 1:59 AM, Qi Zhang wrote:
> Add command line to support untag drop to specific VF in
> testpmd.
> 
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> ---
>  app/test-pmd/cmdline.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 104 insertions(+)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 43fc636..4ddc2c9 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -311,6 +311,10 @@ static void cmd_help_long_parsed(void *parsed_result,
>  
>  			"set vf vlan antispoof (port_id) (vf_id) (on|off)\n"
>  			"    Set VLAN antispoof for a VF from the PF.\n\n"
> +#ifdef RTE_LIBRTE_I40E_PMD
> +			"set vf vlan untagdrop (port_id) (vf_id) (on|off)\n"
> +			"    Set VLAN untag drop for a VF from the PF.\n\n"
> +#endif

We should decide how to implement PMD specific APIs in testpmd, and be
consistent about it.

Currently there are two approaches:

1- Wrap PMD specific feature and API with and PMD #ifdef, as done here.

2- Enable feature by default, return -ENOTSUP for port_id that does not
support it. Ex: cmd_vf_rxvlan_filter.

I am for second option. And explicitly not disabling I40E driver does
not mean you should have those NICs in your runtime environment, so the
effect will be same as always enabling it.


And since number of PMD specific APIs are increasing, perhaps we should
find a better approach for testpmd to prevent them corrupting testpmd.

Also it may worth to discuss why number of PMD specific APIs are increasing.

>  
>  			"set vf vlan tag (port_id) (vf_id) (on|off)\n"
>  			"    Set VLAN tag for a VF from the PF.\n\n"
> @@ -10995,6 +10999,103 @@ cmdline_parse_inst_t cmd_set_vf_vlan_anti_spoof = {
>  	},
>  };
>  
<...>

Re: [PATCH 2/2] app/testpmd: enable VF untag drop in testpmd

[Zhang, Qi Z]

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Tuesday, March 7, 2017 7:14 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
> Zhang, Helin <helin.zhang@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 2/2] app/testpmd: enable VF untag drop in
> testpmd
> 
> On 3/3/2017 1:59 AM, Qi Zhang wrote:
> > Add command line to support untag drop to specific VF in testpmd.
> >
> > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > ---
> >  app/test-pmd/cmdline.c | 104
> > +++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 104 insertions(+)
> >
> > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > 43fc636..4ddc2c9 100644
> > --- a/app/test-pmd/cmdline.c
> > +++ b/app/test-pmd/cmdline.c
> > @@ -311,6 +311,10 @@ static void cmd_help_long_parsed(void
> > *parsed_result,
> >
> >  			"set vf vlan antispoof (port_id) (vf_id) (on|off)\n"
> >  			"    Set VLAN antispoof for a VF from the PF.\n\n"
> > +#ifdef RTE_LIBRTE_I40E_PMD
> > +			"set vf vlan untagdrop (port_id) (vf_id) (on|off)\n"
> > +			"    Set VLAN untag drop for a VF from the PF.\n\n"
> > +#endif
> 
> We should decide how to implement PMD specific APIs in testpmd, and be
> consistent about it.
> 
> Currently there are two approaches:
> 
> 1- Wrap PMD specific feature and API with and PMD #ifdef, as done here.
> 
> 2- Enable feature by default, return -ENOTSUP for port_id that does not support
> it. Ex: cmd_vf_rxvlan_filter.
>
> I am for second option. And explicitly not disabling I40E driver does not mean
> you should have those NICs in your runtime environment, so the effect will be
> same as always enabling it.
>
Yes, I notice this problem, during implementation, I saw both patterns exist, so I have to choose one of them
We'd better align this.
Both option ok for me, but a little bit prefer option 1 , since it's not necessary to explore a command if no backend device, that make the hint more clean.
> 
> And since number of PMD specific APIs are increasing, perhaps we should find a
> better approach for testpmd to prevent them corrupting testpmd.
Will think about this, also like to know if you or anyone have any good suggestion.
> 
> Also it may worth to discuss why number of PMD specific APIs are increasing.
> 
> >
> >  			"set vf vlan tag (port_id) (vf_id) (on|off)\n"
> >  			"    Set VLAN tag for a VF from the PF.\n\n"
> > @@ -10995,6 +10999,103 @@ cmdline_parse_inst_t
> cmd_set_vf_vlan_anti_spoof = {
> >  	},
> >  };
> >
> <...>

Re: [PATCH 1/2] net/i40e: enable VF untag drop

[Zhang, Qi Z]

Hi Ferruh:

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Tuesday, March 7, 2017 6:51 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
> Zhang, Helin <helin.zhang@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 1/2] net/i40e: enable VF untag drop
> 
> On 3/3/2017 1:59 AM, Qi Zhang wrote:
> > Add a new private API to support the untag drop enable/disable for
> > specific VF.
> >
> > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > ---
> >  drivers/net/i40e/i40e_ethdev.c  | 49
> > +++++++++++++++++++++++++++++++++++++++++
> >  drivers/net/i40e/rte_pmd_i40e.h | 18 +++++++++++++++
> 
> Shared library is giving build error because of API is missing in *version.map file
> 
> >  2 files changed, 67 insertions(+)
> >
> 
> <...>
> 
> > diff --git a/drivers/net/i40e/rte_pmd_i40e.h
> > b/drivers/net/i40e/rte_pmd_i40e.h index a0ad88c..895e2cc 100644
> > --- a/drivers/net/i40e/rte_pmd_i40e.h
> > +++ b/drivers/net/i40e/rte_pmd_i40e.h
> > @@ -332,4 +332,22 @@ int rte_pmd_i40e_get_vf_stats(uint8_t port,  int
> > rte_pmd_i40e_reset_vf_stats(uint8_t port,
> >  				uint16_t vf_id);
> >
> > +/**
> > + * Enable/Disable VF untag drop
> > + *
> > + * @param port
> > + *    The port identifier of the Ethernet device.
> > + * @param vf_id
> > + *    VF on witch to enable/disable
> > + * @param on
> > + *    Enable or Disable
> > + * @retura
> 
> @return
> 
> > + *  - (0) if successful.
> > + *  -(-ENODEVE) if *port* invalid
> > + *  -(-EINVAL) if bad parameter.
> > + */
> > +int rte_pmd_i40e_set_vf_vlan_untag_drop(uint8_t port,
> > +					uint16_t vf_id,
> > +					uint8_t on);
> 
> As discussed previously, I believe it is good to keep following syntax in API:
> <name_space>_<object>_<action>, for this API it becomes:
I think, current naming rule is <name_space>_<action>_<object> right? See below
		rte_pmd_i40e_set_vf_vlan_anti_spoof;
        rte_pmd_i40e_set_vf_vlan_filter;
        rte_pmd_i40e_set_vf_vlan_insert;
        rte_pmd_i40e_set_vf_vlan_stripq;
        rte_pmd_i40e_set_vf_vlan_tag;
so what's wrong with this?
> 
> rte_pmd_i40e_vf_vlan_untag_drop_set(), and perhaps "set" can be removed?
> 
> > +
> >  #endif /* _PMD_I40E_H_ */
> >

[PATCH v2 0/2] net/i40e: enable VF untag drop

[Qi Zhang]

Add a new API to enable untag drop for a specific VF. 
This is an experimental API.

v2:
- Add a flag "untag_drop" in i40e_vsi to help track the untag drop
  status, so it prevent error from i40e_aq_remove_vlan when call
  rte_pmd_i40e_set_vf_vlan_untag_drop twice.
  Due to hardware limitation, the error code from i40e_aq_remove_vlan
  is not able to help distinct a specific double removal error from
  other errors.

Qi Zhang (2):
  net/i40e: enable VF untag drop
  app/testpmd: enable VF untag drop in testpmd

 app/test-pmd/cmdline.c          | 104 ++++++++++++++++++++++++++++++++++++++++
 drivers/net/i40e/i40e_ethdev.c  |  54 +++++++++++++++++++++
 drivers/net/i40e/i40e_ethdev.h  |   1 +
 drivers/net/i40e/rte_pmd_i40e.h |  18 +++++++
 4 files changed, 177 insertions(+)

-- 
2.9.3

[PATCH v2 1/2] net/i40e: enable VF untag drop

[Qi Zhang]

Add a new private API to support the untag drop enable/disable
for specific VF.
Add a flag "untag_drop" in i40e_vsi to help track the untag drop
status, so it prevent error from i40e_aq_remove_vlan when call
rte_pmd_i40e_set_vf_vlan_untag_drop twice.

Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
---
V2:
- Add a flag "untag_drop" in i40e_vsi to track the untag drop status.

 drivers/net/i40e/i40e_ethdev.c  | 54 +++++++++++++++++++++++++++++++++++++++++
 drivers/net/i40e/i40e_ethdev.h  |  1 +
 drivers/net/i40e/rte_pmd_i40e.h | 18 ++++++++++++++
 3 files changed, 73 insertions(+)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 303027b..304b02b 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -11212,3 +11212,57 @@ rte_pmd_i40e_reset_vf_stats(uint8_t port,
 
 	return 0;
 }
+
+int rte_pmd_i40e_set_vf_vlan_untag_drop(uint8_t port,
+					uint16_t vf_id,
+					uint8_t on)
+{
+	struct rte_eth_dev *dev;
+	struct i40e_pf *pf;
+	struct i40e_vsi *vsi;
+	struct i40e_aqc_add_remove_vlan_element_data vlan_data = {0};
+	struct i40e_hw *hw;
+	int ret = 0;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
+
+	dev = &rte_eth_devices[port];
+
+	if (!is_device_supported(dev, &rte_i40e_pmd))
+		return -ENOTSUP;
+
+	pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
+
+	if (vf_id >= pf->vf_num || !pf->vfs) {
+		PMD_DRV_LOG(ERR, "Invalid VF ID.");
+		return -EINVAL;
+	}
+
+	vsi = pf->vfs[vf_id].vsi;
+	if (!vsi) {
+		PMD_DRV_LOG(ERR, "Invalid VSI.");
+		return -EINVAL;
+	}
+
+
+	hw = I40E_VSI_TO_HW(vsi);
+	vlan_data.vlan_tag = 0;
+	vlan_data.vlan_flags = 0x1;
+
+	if (on && !vsi->untag_drop) {
+		ret = i40e_aq_add_vlan(hw, vsi->seid, &vlan_data, 1, NULL);
+		if (ret != I40E_SUCCESS)
+			PMD_DRV_LOG(ERR, "Failed to add vlan filter");
+		else
+			vsi->untag_drop = 1;
+	} else if (!on && vsi->untag_drop) {
+		ret = i40e_aq_remove_vlan(hw, vsi->seid,
+					&vlan_data, 1, NULL);
+		if (ret != I40E_SUCCESS)
+			PMD_DRV_LOG(ERR, "Failed to remove vlan filter");
+		else
+			vsi->untag_drop = 0;
+	}
+
+	return ret;
+}
diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
index f545850..25d0c49 100644
--- a/drivers/net/i40e/i40e_ethdev.h
+++ b/drivers/net/i40e/i40e_ethdev.h
@@ -365,6 +365,7 @@ struct i40e_vsi {
 	uint8_t enabled_tc; /* The traffic class enabled */
 	uint8_t vlan_anti_spoof_on; /* The VLAN anti-spoofing enabled */
 	struct i40e_bw_info bw_info; /* VSI bandwidth information */
+	uint8_t untag_drop; /* VLAN untagged drop enable/disable status */
 };
 
 struct pool_entry {
diff --git a/drivers/net/i40e/rte_pmd_i40e.h b/drivers/net/i40e/rte_pmd_i40e.h
index a0ad88c..895e2cc 100644
--- a/drivers/net/i40e/rte_pmd_i40e.h
+++ b/drivers/net/i40e/rte_pmd_i40e.h
@@ -332,4 +332,22 @@ int rte_pmd_i40e_get_vf_stats(uint8_t port,
 int rte_pmd_i40e_reset_vf_stats(uint8_t port,
 				uint16_t vf_id);
 
+/**
+ * Enable/Disable VF untag drop
+ *
+ * @param port
+ *    The port identifier of the Ethernet device.
+ * @param vf_id
+ *    VF on witch to enable/disable
+ * @param on
+ *    Enable or Disable
+ * @retura
+ *  - (0) if successful.
+ *  -(-ENODEVE) if *port* invalid
+ *  -(-EINVAL) if bad parameter.
+ */
+int rte_pmd_i40e_set_vf_vlan_untag_drop(uint8_t port,
+					uint16_t vf_id,
+					uint8_t on);
+
 #endif /* _PMD_I40E_H_ */
-- 
2.9.3

[PATCH v2 2/2] app/testpmd: enable VF untag drop in testpmd

[Qi Zhang]

Add command line to support untag drop to specific VF in
testpmd.

Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
---
v2:
- adjust compile option to align with others.

 app/test-pmd/cmdline.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 99 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 43fc636..527a0f6 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -312,6 +312,9 @@ static void cmd_help_long_parsed(void *parsed_result,
 			"set vf vlan antispoof (port_id) (vf_id) (on|off)\n"
 			"    Set VLAN antispoof for a VF from the PF.\n\n"
 
+			"set vf vlan untagdrop (port_id) (vf_id) (on|off)\n"
+			"    Set VLAN untag drop for a VF from the PF.\n\n"
+
 			"set vf vlan tag (port_id) (vf_id) (on|off)\n"
 			"    Set VLAN tag for a VF from the PF.\n\n"
 
@@ -10995,6 +10998,101 @@ cmdline_parse_inst_t cmd_set_vf_vlan_anti_spoof = {
 	},
 };
 
+/* vf vlan untag drop configuration */
+
+/* Common result structure for vf vlan untag drop */
+struct cmd_vf_vlan_untag_drop_result {
+	cmdline_fixed_string_t set;
+	cmdline_fixed_string_t vf;
+	cmdline_fixed_string_t vlan;
+	cmdline_fixed_string_t untagdrop;
+	uint8_t port_id;
+	uint32_t vf_id;
+	cmdline_fixed_string_t on_off;
+};
+
+/* Common CLI fields for vf vlan untag drop enable disable */
+cmdline_parse_token_string_t cmd_vf_vlan_untag_drop_set =
+	TOKEN_STRING_INITIALIZER
+		(struct cmd_vf_vlan_untag_drop_result,
+		 set, "set");
+cmdline_parse_token_string_t cmd_vf_vlan_untag_drop_vf =
+	TOKEN_STRING_INITIALIZER
+		(struct cmd_vf_vlan_untag_drop_result,
+		 vf, "vf");
+cmdline_parse_token_string_t cmd_vf_vlan_untag_drop_vlan =
+	TOKEN_STRING_INITIALIZER
+		(struct cmd_vf_vlan_untag_drop_result,
+		 vlan, "vlan");
+cmdline_parse_token_string_t cmd_vf_vlan_untag_drop_untagdrop =
+	TOKEN_STRING_INITIALIZER
+		(struct cmd_vf_vlan_untag_drop_result,
+		 untagdrop, "untagdrop");
+cmdline_parse_token_num_t cmd_vf_vlan_untag_drop_port_id =
+	TOKEN_NUM_INITIALIZER
+		(struct cmd_vf_vlan_untag_drop_result,
+		 port_id, UINT8);
+cmdline_parse_token_num_t cmd_vf_vlan_untag_drop_vf_id =
+	TOKEN_NUM_INITIALIZER
+		(struct cmd_vf_vlan_untag_drop_result,
+		 vf_id, UINT32);
+cmdline_parse_token_string_t cmd_vf_vlan_untag_drop_on_off =
+	TOKEN_STRING_INITIALIZER
+		(struct cmd_vf_vlan_untag_drop_result,
+		 on_off, "on#off");
+
+static void
+cmd_set_vf_vlan_untag_drop_parsed(
+	void *parsed_result,
+	__attribute__((unused)) struct cmdline *cl,
+	__attribute__((unused)) void *data)
+{
+	struct cmd_vf_vlan_untag_drop_result *res = parsed_result;
+	int ret = -ENOTSUP;
+
+	__rte_unused int is_on = (strcmp(res->on_off, "on") == 0) ? 1 : 0;
+
+	if (port_id_is_invalid(res->port_id, ENABLED_WARN))
+		return;
+
+#ifdef RTE_LIBRTE_I40E_PMD
+	ret = rte_pmd_i40e_set_vf_vlan_untag_drop(res->port_id,
+				res->vf_id, is_on);
+#endif
+
+	switch (ret) {
+	case 0:
+		break;
+	case -EINVAL:
+		printf("invalid vf_id %d\n", res->vf_id);
+		break;
+	case -ENODEV:
+		printf("invalid port_id %d\n", res->port_id);
+		break;
+	case -ENOTSUP:
+		printf("function not implemented\n");
+		break;
+	default:
+		printf("programming error: (%s)\n", strerror(-ret));
+	}
+}
+
+cmdline_parse_inst_t cmd_set_vf_vlan_untag_drop = {
+	.f = cmd_set_vf_vlan_untag_drop_parsed,
+	.data = NULL,
+	.help_str = "set vf vlan untagdrop <port_id> <vf_id> on|off",
+	.tokens = {
+		(void *)&cmd_vf_vlan_untag_drop_set,
+		(void *)&cmd_vf_vlan_untag_drop_vf,
+		(void *)&cmd_vf_vlan_untag_drop_vlan,
+		(void *)&cmd_vf_vlan_untag_drop_untagdrop,
+		(void *)&cmd_vf_vlan_untag_drop_port_id,
+		(void *)&cmd_vf_vlan_untag_drop_vf_id,
+		(void *)&cmd_vf_vlan_untag_drop_on_off,
+		NULL,
+	},
+};
+
 /* vf mac anti spoof configuration */
 
 /* Common result structure for vf mac anti spoof */
@@ -12549,6 +12647,7 @@ cmdline_parse_ctx_t main_ctx[] = {
 	(cmdline_parse_inst_t *)&cmd_config_e_tag_filter_add,
 	(cmdline_parse_inst_t *)&cmd_config_e_tag_filter_del,
 	(cmdline_parse_inst_t *)&cmd_set_vf_vlan_anti_spoof,
+	(cmdline_parse_inst_t *)&cmd_set_vf_vlan_untag_drop,
 	(cmdline_parse_inst_t *)&cmd_set_vf_mac_anti_spoof,
 	(cmdline_parse_inst_t *)&cmd_set_vf_vlan_stripq,
 	(cmdline_parse_inst_t *)&cmd_set_vf_vlan_insert,
-- 
2.9.3

Re: [PATCH 1/2] net/i40e: enable VF untag drop

[Ferruh Yigit]

On 3/9/2017 3:24 AM, Zhang, Qi Z wrote:
> Hi Ferruh:
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Tuesday, March 7, 2017 6:51 PM
>> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
>> Zhang, Helin <helin.zhang@intel.com>
>> Cc: dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH 1/2] net/i40e: enable VF untag drop
>>
>> On 3/3/2017 1:59 AM, Qi Zhang wrote:
>>> Add a new private API to support the untag drop enable/disable for
>>> specific VF.
>>>
>>> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
>>> ---
>>>  drivers/net/i40e/i40e_ethdev.c  | 49
>>> +++++++++++++++++++++++++++++++++++++++++
>>>  drivers/net/i40e/rte_pmd_i40e.h | 18 +++++++++++++++
>>
>> Shared library is giving build error because of API is missing in *version.map file
>>
>>>  2 files changed, 67 insertions(+)
>>>
>>
>> <...>
>>
>>> diff --git a/drivers/net/i40e/rte_pmd_i40e.h
>>> b/drivers/net/i40e/rte_pmd_i40e.h index a0ad88c..895e2cc 100644
>>> --- a/drivers/net/i40e/rte_pmd_i40e.h
>>> +++ b/drivers/net/i40e/rte_pmd_i40e.h
>>> @@ -332,4 +332,22 @@ int rte_pmd_i40e_get_vf_stats(uint8_t port,  int
>>> rte_pmd_i40e_reset_vf_stats(uint8_t port,
>>>  				uint16_t vf_id);
>>>
>>> +/**
>>> + * Enable/Disable VF untag drop
>>> + *
>>> + * @param port
>>> + *    The port identifier of the Ethernet device.
>>> + * @param vf_id
>>> + *    VF on witch to enable/disable
>>> + * @param on
>>> + *    Enable or Disable
>>> + * @retura
>>
>> @return
>>
>>> + *  - (0) if successful.
>>> + *  -(-ENODEVE) if *port* invalid
>>> + *  -(-EINVAL) if bad parameter.
>>> + */
>>> +int rte_pmd_i40e_set_vf_vlan_untag_drop(uint8_t port,
>>> +					uint16_t vf_id,
>>> +					uint8_t on);
>>
>> As discussed previously, I believe it is good to keep following syntax in API:
>> <name_space>_<object>_<action>, for this API it becomes:
> I think, current naming rule is <name_space>_<action>_<object> right? 

Overall, I am not aware of any defined naming rule, I am for defining one.

> See below
> 		rte_pmd_i40e_set_vf_vlan_anti_spoof;
>         rte_pmd_i40e_set_vf_vlan_filter;
>         rte_pmd_i40e_set_vf_vlan_insert;
>         rte_pmd_i40e_set_vf_vlan_stripq;
>         rte_pmd_i40e_set_vf_vlan_tag;
> so what's wrong with this?

This breaks hierarchical approach, if you think API name as tree. Easier
to see this when you sort the APIs, ns_set_x, ns_reset_x, ns_del_x will
spread to different locations.

This looks OK when you work on one type of object already, but with all
APIs in concern, I believe object based grouping is better than action
based grouping.

And why do you think above one is better? Again, as long as one is
agreed on, I am OK.

>>
>> rte_pmd_i40e_vf_vlan_untag_drop_set(), and perhaps "set" can be removed?
>>
>>> +
>>>  #endif /* _PMD_I40E_H_ */
>>>
> 

Re: [PATCH 2/2] app/testpmd: enable VF untag drop in testpmd

[Ferruh Yigit]

On 3/9/2017 2:59 AM, Zhang, Qi Z wrote:
> 
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Tuesday, March 7, 2017 7:14 PM
>> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
>> Zhang, Helin <helin.zhang@intel.com>
>> Cc: dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH 2/2] app/testpmd: enable VF untag drop in
>> testpmd
>>
>> On 3/3/2017 1:59 AM, Qi Zhang wrote:
>>> Add command line to support untag drop to specific VF in testpmd.
>>>
>>> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
>>> ---
>>>  app/test-pmd/cmdline.c | 104
>>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 104 insertions(+)
>>>
>>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
>>> 43fc636..4ddc2c9 100644
>>> --- a/app/test-pmd/cmdline.c
>>> +++ b/app/test-pmd/cmdline.c
>>> @@ -311,6 +311,10 @@ static void cmd_help_long_parsed(void
>>> *parsed_result,
>>>
>>>  			"set vf vlan antispoof (port_id) (vf_id) (on|off)\n"
>>>  			"    Set VLAN antispoof for a VF from the PF.\n\n"
>>> +#ifdef RTE_LIBRTE_I40E_PMD
>>> +			"set vf vlan untagdrop (port_id) (vf_id) (on|off)\n"
>>> +			"    Set VLAN untag drop for a VF from the PF.\n\n"
>>> +#endif
>>
>> We should decide how to implement PMD specific APIs in testpmd, and be
>> consistent about it.
>>
>> Currently there are two approaches:
>>
>> 1- Wrap PMD specific feature and API with and PMD #ifdef, as done here.
>>
>> 2- Enable feature by default, return -ENOTSUP for port_id that does not support
>> it. Ex: cmd_vf_rxvlan_filter.
>>
>> I am for second option. And explicitly not disabling I40E driver does not mean
>> you should have those NICs in your runtime environment, so the effect will be
>> same as always enabling it.
>>
> Yes, I notice this problem, during implementation, I saw both patterns exist, so I have to choose one of them
> We'd better align this.
> Both option ok for me, but a little bit prefer option 1 , since it's not necessary to explore a command if no backend device, that make the hint more clean.

I agree it's not necessary to explore a command if no backend device,
but first option does not guaranties it. What it checks is compile time
option, not runtime device detection.
I40E driver is enabled by default, and unless user explicitly disables
it, testpmd command will be there independent from device is there or not.

>>
>> And since number of PMD specific APIs are increasing, perhaps we should find a
>> better approach for testpmd to prevent them corrupting testpmd.
> Will think about this, also like to know if you or anyone have any good suggestion.
>>
>> Also it may worth to discuss why number of PMD specific APIs are increasing.
>>
>>>
>>>  			"set vf vlan tag (port_id) (vf_id) (on|off)\n"
>>>  			"    Set VLAN tag for a VF from the PF.\n\n"
>>> @@ -10995,6 +10999,103 @@ cmdline_parse_inst_t
>> cmd_set_vf_vlan_anti_spoof = {
>>>  	},
>>>  };
>>>
>> <...>

Re: [PATCH 1/2] net/i40e: enable VF untag drop

[Zhang, Qi Z]

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Tuesday, March 14, 2017 9:30 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Zhang, Helin <helin.zhang@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 1/2] net/i40e: enable VF untag drop
> 
> On 3/9/2017 3:24 AM, Zhang, Qi Z wrote:
> > Hi Ferruh:
> >
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Tuesday, March 7, 2017 6:51 PM
> >> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Jingjing
> >> <jingjing.wu@intel.com>; Zhang, Helin <helin.zhang@intel.com>
> >> Cc: dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH 1/2] net/i40e: enable VF untag drop
> >>
> >> On 3/3/2017 1:59 AM, Qi Zhang wrote:
> >>> Add a new private API to support the untag drop enable/disable for
> >>> specific VF.
> >>>
> >>> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> >>> ---
> >>>  drivers/net/i40e/i40e_ethdev.c  | 49
> >>> +++++++++++++++++++++++++++++++++++++++++
> >>>  drivers/net/i40e/rte_pmd_i40e.h | 18 +++++++++++++++
> >>
> >> Shared library is giving build error because of API is missing in
> >> *version.map file
> >>
> >>>  2 files changed, 67 insertions(+)
> >>>
> >>
> >> <...>
> >>
> >>> diff --git a/drivers/net/i40e/rte_pmd_i40e.h
> >>> b/drivers/net/i40e/rte_pmd_i40e.h index a0ad88c..895e2cc 100644
> >>> --- a/drivers/net/i40e/rte_pmd_i40e.h
> >>> +++ b/drivers/net/i40e/rte_pmd_i40e.h
> >>> @@ -332,4 +332,22 @@ int rte_pmd_i40e_get_vf_stats(uint8_t port,
> >>> int rte_pmd_i40e_reset_vf_stats(uint8_t port,
> >>>  				uint16_t vf_id);
> >>>
> >>> +/**
> >>> + * Enable/Disable VF untag drop
> >>> + *
> >>> + * @param port
> >>> + *    The port identifier of the Ethernet device.
> >>> + * @param vf_id
> >>> + *    VF on witch to enable/disable
> >>> + * @param on
> >>> + *    Enable or Disable
> >>> + * @retura
> >>
> >> @return
> >>
> >>> + *  - (0) if successful.
> >>> + *  -(-ENODEVE) if *port* invalid
> >>> + *  -(-EINVAL) if bad parameter.
> >>> + */
> >>> +int rte_pmd_i40e_set_vf_vlan_untag_drop(uint8_t port,
> >>> +					uint16_t vf_id,
> >>> +					uint8_t on);
> >>
> >> As discussed previously, I believe it is good to keep following syntax in
> API:	
> >> <name_space>_<object>_<action>, for this API it becomes:
> > I think, current naming rule is <name_space>_<action>_<object> right?
> 
> Overall, I am not aware of any defined naming rule, I am for defining one.
> 
> > See below
> > 		rte_pmd_i40e_set_vf_vlan_anti_spoof;
> >         rte_pmd_i40e_set_vf_vlan_filter;
> >         rte_pmd_i40e_set_vf_vlan_insert;
> >         rte_pmd_i40e_set_vf_vlan_stripq;
> >         rte_pmd_i40e_set_vf_vlan_tag;
> > so what's wrong with this 
> 
> This breaks hierarchical approach, if you think API name as tree. Easier to
> see this when you sort the APIs, ns_set_x, ns_reset_x, ns_del_x will spread
> to different locations.
I agree with your point, I had thought your concern is only about this patch, but actually it's not.
> 
> This looks OK when you work on one type of object already, but with all APIs
> in concern, I believe object based grouping is better than action based
> grouping.

> 
> And why do you think above one is better? Again, as long as one is agreed on,
I don't, sorry for make you misunderstand
> 
> >>
> >> rte_pmd_i40e_vf_vlan_untag_drop_set(), and perhaps "set" can be
> removed?
> >>
> >>> +
> >>>  #endif /* _PMD_I40E_H_ */
> >>>
> >

Re: [PATCH 2/2] app/testpmd: enable VF untag drop in testpmd

[Zhang, Qi Z]

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Tuesday, March 14, 2017 9:32 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Zhang, Helin <helin.zhang@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 2/2] app/testpmd: enable VF untag drop in
> testpmd
> 
> On 3/9/2017 2:59 AM, Zhang, Qi Z wrote:
> >
> >
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Tuesday, March 7, 2017 7:14 PM
> >> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Jingjing
> >> <jingjing.wu@intel.com>; Zhang, Helin <helin.zhang@intel.com>
> >> Cc: dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH 2/2] app/testpmd: enable VF untag drop
> >> in testpmd
> >>
> >> On 3/3/2017 1:59 AM, Qi Zhang wrote:
> >>> Add command line to support untag drop to specific VF in testpmd.
> >>>
> >>> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> >>> ---
> >>>  app/test-pmd/cmdline.c | 104
> >>> +++++++++++++++++++++++++++++++++++++++++++++++++
> >>>  1 file changed, 104 insertions(+)
> >>>
> >>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> >>> 43fc636..4ddc2c9 100644
> >>> --- a/app/test-pmd/cmdline.c
> >>> +++ b/app/test-pmd/cmdline.c
> >>> @@ -311,6 +311,10 @@ static void cmd_help_long_parsed(void
> >>> *parsed_result,
> >>>
> >>>  			"set vf vlan antispoof (port_id) (vf_id) (on|off)\n"
> >>>  			"    Set VLAN antispoof for a VF from the PF.\n\n"
> >>> +#ifdef RTE_LIBRTE_I40E_PMD
> >>> +			"set vf vlan untagdrop (port_id) (vf_id) (on|off)\n"
> >>> +			"    Set VLAN untag drop for a VF from the PF.\n\n"
> >>> +#endif
> >>
> >> We should decide how to implement PMD specific APIs in testpmd, and
> >> be consistent about it.
> >>
> >> Currently there are two approaches:
> >>
> >> 1- Wrap PMD specific feature and API with and PMD #ifdef, as done here.
> >>
> >> 2- Enable feature by default, return -ENOTSUP for port_id that does
> >> not support it. Ex: cmd_vf_rxvlan_filter.
> >>
> >> I am for second option. And explicitly not disabling I40E driver does
> >> not mean you should have those NICs in your runtime environment, so
> >> the effect will be same as always enabling it.
> >>
> > Yes, I notice this problem, during implementation, I saw both patterns
> > exist, so I have to choose one of them We'd better align this.
> > Both option ok for me, but a little bit prefer option 1 , since it's not
> necessary to explore a command if no backend device, that make the hint
> more clean.
> 
> I agree it's not necessary to explore a command if no backend device, but
> first option does not guaranties it. What it checks is compile time option, not
> runtime device detection.
> I40E driver is enabled by default, and unless user explicitly disables it,
> testpmd command will be there independent from device is there or not.
So this still benefit custom build, right?
Anyway, as I said, both are ok for me, I followed option 2 since most APIs follow this.
Please check v2.
> 
> >>
> >> And since number of PMD specific APIs are increasing, perhaps we
> >> should find a better approach for testpmd to prevent them corrupting
> testpmd.
> > Will think about this, also like to know if you or anyone have any good
> suggestion.
> >>
> >> Also it may worth to discuss why number of PMD specific APIs are
> increasing.
> >>
> >>>
> >>>  			"set vf vlan tag (port_id) (vf_id) (on|off)\n"
> >>>  			"    Set VLAN tag for a VF from the PF.\n\n"
> >>> @@ -10995,6 +10999,103 @@ cmdline_parse_inst_t
> >> cmd_set_vf_vlan_anti_spoof = {
> >>>  	},
> >>>  };
> >>>
> >> <...>

Re: [PATCH 1/2] net/i40e: enable VF untag drop

[Iremonger, Bernard]

Hi Ferruh, Qi,

<snip>

> > >> Subject: Re: [dpdk-dev] [PATCH 1/2] net/i40e: enable VF untag drop
> > >>
> > >> On 3/3/2017 1:59 AM, Qi Zhang wrote:
> > >>> Add a new private API to support the untag drop enable/disable for
> > >>> specific VF.
> > >>>
> > >>> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > >>> ---
> > >>>  drivers/net/i40e/i40e_ethdev.c  | 49
> > >>> +++++++++++++++++++++++++++++++++++++++++
> > >>>  drivers/net/i40e/rte_pmd_i40e.h | 18 +++++++++++++++
> > >>
> > >> Shared library is giving build error because of API is missing in
> > >> *version.map file
> > >>
> > >>>  2 files changed, 67 insertions(+)
> > >>>
> > >>
> > >> <...>
> > >>
> > >>> diff --git a/drivers/net/i40e/rte_pmd_i40e.h
> > >>> b/drivers/net/i40e/rte_pmd_i40e.h index a0ad88c..895e2cc 100644
> > >>> --- a/drivers/net/i40e/rte_pmd_i40e.h
> > >>> +++ b/drivers/net/i40e/rte_pmd_i40e.h
> > >>> @@ -332,4 +332,22 @@ int rte_pmd_i40e_get_vf_stats(uint8_t port,
> > >>> int rte_pmd_i40e_reset_vf_stats(uint8_t port,
> > >>>  				uint16_t vf_id);
> > >>>
> > >>> +/**
> > >>> + * Enable/Disable VF untag drop
> > >>> + *
> > >>> + * @param port
> > >>> + *    The port identifier of the Ethernet device.
> > >>> + * @param vf_id
> > >>> + *    VF on witch to enable/disable
> > >>> + * @param on
> > >>> + *    Enable or Disable
> > >>> + * @retura
> > >>
> > >> @return
> > >>
> > >>> + *  - (0) if successful.
> > >>> + *  -(-ENODEVE) if *port* invalid
> > >>> + *  -(-EINVAL) if bad parameter.
> > >>> + */
> > >>> +int rte_pmd_i40e_set_vf_vlan_untag_drop(uint8_t port,
> > >>> +					uint16_t vf_id,
> > >>> +					uint8_t on);
> > >>
> > >> As discussed previously, I believe it is good to keep following
> > >> syntax in
> > API:
> > >> <name_space>_<object>_<action>, for this API it becomes:
> > > I think, current naming rule is <name_space>_<action>_<object> right?

 This seems to be the existing naming convention.

> >
> > Overall, I am not aware of any defined naming rule, I am for defining one.
> >
> > > See below
> > > 		rte_pmd_i40e_set_vf_vlan_anti_spoof;
> > >         rte_pmd_i40e_set_vf_vlan_filter;
> > >         rte_pmd_i40e_set_vf_vlan_insert;
> > >         rte_pmd_i40e_set_vf_vlan_stripq;
> > >         rte_pmd_i40e_set_vf_vlan_tag; so what's wrong with this
> >
> > This breaks hierarchical approach, if you think API name as tree.
> > Easier to see this when you sort the APIs, ns_set_x, ns_reset_x,
> > ns_del_x will spread to different locations.
> I agree with your point, I had thought your concern is only about this patch,
> but actually it's not.
> >
> > This looks OK when you work on one type of object already, but with
> > all APIs in concern, I believe object based grouping is better than
> > action based grouping.
> 
> >
> > And why do you think above one is better? Again, as long as one is
> > agreed on,
> I don't, sorry for make you misunderstand

I don't think changing the name convention at this point is a good idea.
It would be better to remain consistent with the existing naming convention.
Otherwise both naming conventions will exist for the rte_pmd_i40e_* API's.
 

> > >> rte_pmd_i40e_vf_vlan_untag_drop_set(), and perhaps "set" can be
> > removed?
> > >>
> > >>> +
> > >>>  #endif /* _PMD_I40E_H_ */
> > >>>
> > >

Regards,

Bernard.

Re: [PATCH 1/2] net/i40e: enable VF untag drop

[Ferruh Yigit]

On 3/14/2017 6:16 PM, Iremonger, Bernard wrote:
> Hi Ferruh, Qi,
> 
> <snip>
> 
>>>>> Subject: Re: [dpdk-dev] [PATCH 1/2] net/i40e: enable VF untag drop
>>>>>
>>>>> On 3/3/2017 1:59 AM, Qi Zhang wrote:
>>>>>> Add a new private API to support the untag drop enable/disable for
>>>>>> specific VF.
>>>>>>
>>>>>> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
>>>>>> ---
>>>>>>  drivers/net/i40e/i40e_ethdev.c  | 49
>>>>>> +++++++++++++++++++++++++++++++++++++++++
>>>>>>  drivers/net/i40e/rte_pmd_i40e.h | 18 +++++++++++++++
>>>>>
>>>>> Shared library is giving build error because of API is missing in
>>>>> *version.map file
>>>>>
>>>>>>  2 files changed, 67 insertions(+)
>>>>>>
>>>>>
>>>>> <...>
>>>>>
>>>>>> diff --git a/drivers/net/i40e/rte_pmd_i40e.h
>>>>>> b/drivers/net/i40e/rte_pmd_i40e.h index a0ad88c..895e2cc 100644
>>>>>> --- a/drivers/net/i40e/rte_pmd_i40e.h
>>>>>> +++ b/drivers/net/i40e/rte_pmd_i40e.h
>>>>>> @@ -332,4 +332,22 @@ int rte_pmd_i40e_get_vf_stats(uint8_t port,
>>>>>> int rte_pmd_i40e_reset_vf_stats(uint8_t port,
>>>>>>  				uint16_t vf_id);
>>>>>>
>>>>>> +/**
>>>>>> + * Enable/Disable VF untag drop
>>>>>> + *
>>>>>> + * @param port
>>>>>> + *    The port identifier of the Ethernet device.
>>>>>> + * @param vf_id
>>>>>> + *    VF on witch to enable/disable
>>>>>> + * @param on
>>>>>> + *    Enable or Disable
>>>>>> + * @retura
>>>>>
>>>>> @return
>>>>>
>>>>>> + *  - (0) if successful.
>>>>>> + *  -(-ENODEVE) if *port* invalid
>>>>>> + *  -(-EINVAL) if bad parameter.
>>>>>> + */
>>>>>> +int rte_pmd_i40e_set_vf_vlan_untag_drop(uint8_t port,
>>>>>> +					uint16_t vf_id,
>>>>>> +					uint8_t on);
>>>>>
>>>>> As discussed previously, I believe it is good to keep following
>>>>> syntax in
>>> API:
>>>>> <name_space>_<object>_<action>, for this API it becomes:
>>>> I think, current naming rule is <name_space>_<action>_<object> right?
> 
>  This seems to be the existing naming convention.
> 
>>>
>>> Overall, I am not aware of any defined naming rule, I am for defining one.
>>>
>>>> See below
>>>> 		rte_pmd_i40e_set_vf_vlan_anti_spoof;
>>>>         rte_pmd_i40e_set_vf_vlan_filter;
>>>>         rte_pmd_i40e_set_vf_vlan_insert;
>>>>         rte_pmd_i40e_set_vf_vlan_stripq;
>>>>         rte_pmd_i40e_set_vf_vlan_tag; so what's wrong with this
>>>
>>> This breaks hierarchical approach, if you think API name as tree.
>>> Easier to see this when you sort the APIs, ns_set_x, ns_reset_x,
>>> ns_del_x will spread to different locations.
>> I agree with your point, I had thought your concern is only about this patch,
>> but actually it's not.
>>>
>>> This looks OK when you work on one type of object already, but with
>>> all APIs in concern, I believe object based grouping is better than
>>> action based grouping.
>>
>>>
>>> And why do you think above one is better? Again, as long as one is
>>> agreed on,
>> I don't, sorry for make you misunderstand
> 
> I don't think changing the name convention at this point is a good idea.

I am not suggesting changing existing ones, for the sake compatibility.
But that can also be an option, since these are PMD specific API, I
expect usage will be limited and these does not carry as high standard
as library APIs.

> It would be better to remain consistent with the existing naming convention.

Existing i40e ones added this way to be compatible with existing ixgbe
ones. But I don't think we need to follow old usage with new APIs.

> Otherwise both naming conventions will exist for the rte_pmd_i40e_* API's.

It can be for a while, later all can be fixed. If you think proposed
convention is not better, that is something else.

>  
> 
>>>>> rte_pmd_i40e_vf_vlan_untag_drop_set(), and perhaps "set" can be
>>> removed?
>>>>>
>>>>>> +
>>>>>>  #endif /* _PMD_I40E_H_ */
>>>>>>
>>>>
> 
> Regards,
> 
> Bernard.
> 

Re: [PATCH v2 2/2] app/testpmd: enable VF untag drop in testpmd

[Ferruh Yigit]

On 3/13/2017 4:55 AM, Qi Zhang wrote:
> Add command line to support untag drop to specific VF in
> testpmd.
> 
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> ---
> v2:
> - adjust compile option to align with others.
> 
>  app/test-pmd/cmdline.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++

doc/guides/testpmd_app_ug/testpmd_funcs.rst also should be updated with
new command.

<...>

Re: [PATCH 1/2] net/i40e: enable VF untag drop

[Iremonger, Bernard]

Hi Ferruh,
<snip>
> >>>>> Subject: Re: [dpdk-dev] [PATCH 1/2] net/i40e: enable VF untag drop
> >>>>>
> >>>>> On 3/3/2017 1:59 AM, Qi Zhang wrote:
> >>>>>> Add a new private API to support the untag drop enable/disable
> >>>>>> for specific VF.
> >>>>>>
> >>>>>> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> >>>>>> ---
> >>>>>>  drivers/net/i40e/i40e_ethdev.c  | 49
> >>>>>> +++++++++++++++++++++++++++++++++++++++++
> >>>>>>  drivers/net/i40e/rte_pmd_i40e.h | 18 +++++++++++++++
> >>>>>
> >>>>> Shared library is giving build error because of API is missing in
> >>>>> *version.map file
> >>>>>
> >>>>>>  2 files changed, 67 insertions(+)
> >>>>>>
> >>>>>
> >>>>> <...>
> >>>>>
> >>>>>> diff --git a/drivers/net/i40e/rte_pmd_i40e.h
> >>>>>> b/drivers/net/i40e/rte_pmd_i40e.h index a0ad88c..895e2cc 100644
> >>>>>> --- a/drivers/net/i40e/rte_pmd_i40e.h
> >>>>>> +++ b/drivers/net/i40e/rte_pmd_i40e.h
> >>>>>> @@ -332,4 +332,22 @@ int rte_pmd_i40e_get_vf_stats(uint8_t
> port,
> >>>>>> int rte_pmd_i40e_reset_vf_stats(uint8_t port,
> >>>>>>  				uint16_t vf_id);
> >>>>>>
> >>>>>> +/**
> >>>>>> + * Enable/Disable VF untag drop
> >>>>>> + *
> >>>>>> + * @param port
> >>>>>> + *    The port identifier of the Ethernet device.
> >>>>>> + * @param vf_id
> >>>>>> + *    VF on witch to enable/disable
> >>>>>> + * @param on
> >>>>>> + *    Enable or Disable
> >>>>>> + * @retura
> >>>>>
> >>>>> @return
> >>>>>
> >>>>>> + *  - (0) if successful.
> >>>>>> + *  -(-ENODEVE) if *port* invalid
> >>>>>> + *  -(-EINVAL) if bad parameter.
> >>>>>> + */
> >>>>>> +int rte_pmd_i40e_set_vf_vlan_untag_drop(uint8_t port,
> >>>>>> +					uint16_t vf_id,
> >>>>>> +					uint8_t on);
> >>>>>
> >>>>> As discussed previously, I believe it is good to keep following
> >>>>> syntax in
> >>> API:
> >>>>> <name_space>_<object>_<action>, for this API it becomes:
> >>>> I think, current naming rule is <name_space>_<action>_<object>
> right?
> >
> >  This seems to be the existing naming convention.
> >
> >>>
> >>> Overall, I am not aware of any defined naming rule, I am for defining
> one.
> >>>
> >>>> See below
> >>>> 		rte_pmd_i40e_set_vf_vlan_anti_spoof;
> >>>>         rte_pmd_i40e_set_vf_vlan_filter;
> >>>>         rte_pmd_i40e_set_vf_vlan_insert;
> >>>>         rte_pmd_i40e_set_vf_vlan_stripq;
> >>>>         rte_pmd_i40e_set_vf_vlan_tag; so what's wrong with this
> >>>
> >>> This breaks hierarchical approach, if you think API name as tree.
> >>> Easier to see this when you sort the APIs, ns_set_x, ns_reset_x,
> >>> ns_del_x will spread to different locations.
> >> I agree with your point, I had thought your concern is only about
> >> this patch, but actually it's not.
> >>>
> >>> This looks OK when you work on one type of object already, but with
> >>> all APIs in concern, I believe object based grouping is better than
> >>> action based grouping.
> >>
> >>>
> >>> And why do you think above one is better? Again, as long as one is
> >>> agreed on,
> >> I don't, sorry for make you misunderstand
> >
> > I don't think changing the name convention at this point is a good idea.
> 
> I am not suggesting changing existing ones, for the sake compatibility.
> But that can also be an option, since these are PMD specific API, I expect
> usage will be limited and these does not carry as high standard as library APIs.

These API's are already in use with users. 

> > It would be better to remain consistent with the existing naming
> convention.
> 
> Existing i40e ones added this way to be compatible with existing ixgbe ones.
> But I don't think we need to follow old usage with new APIs.
> 
> > Otherwise both naming conventions will exist for the rte_pmd_i40e_*
> API's.
> 
> It can be for a while, later all can be fixed. If you think proposed convention is
> not better, that is something else.

I don't see anything to be gained by using two naming conventions side by side.
I don't have a strong view on which name convention is better, however the existing naming convention is what is in use with users at present. It is also the naming convention used librte_ether.

Changing API naming conventions is something that should probably be discussed in a separate thread, as it can have a wider impact than the i40e and ixgbe PMD's.
 
> >
> >
> >>>>> rte_pmd_i40e_vf_vlan_untag_drop_set(), and perhaps "set" can be
> >>> removed?
> >>>>>
> >>>>>> +
> >>>>>>  #endif /* _PMD_I40E_H_ */
> >>>>>>
> >>>>

Regards,

Bernard.