[dpdk-dev,v4,1/4] ethdev: rename rte_eth_vmdq_mirror_conf

Message ID 1433917473-21508-2-git-send-email-jingjing.wu@intel.com (mailing list archive)
State Accepted, archived
Headers

Commit Message

Jingjing Wu June 10, 2015, 6:24 a.m. UTC
  rename rte_eth_vmdq_mirror_conf to rte_eth_mirror_conf and move
the maximum rule id check from ethdev level to driver

Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
---
 app/test-pmd/cmdline.c           | 22 +++++++++++-----------
 drivers/net/ixgbe/ixgbe_ethdev.c | 11 +++++++----
 drivers/net/ixgbe/ixgbe_ethdev.h |  4 +++-
 lib/librte_ether/rte_ethdev.c    | 18 ++----------------
 lib/librte_ether/rte_ethdev.h    | 19 ++++++++++---------
 5 files changed, 33 insertions(+), 41 deletions(-)
  

Comments

Jingjing Wu June 26, 2015, 7:03 a.m. UTC | #1
Hi, Neil

About this patch I have an ABI concern about it. 
This patch just renamed a struct rte_eth_vmdq_mirror_conf to rte_eth_mirror_conf, the size and its elements don't change. As my understanding, it will not break the ABI. And I also tested it. 
But when I use the script ./scripts/validate-abi.sh to check. A low severity problem is reported in symbol "rte_eth_mirror_rule_set"
 - Change: "Base type of 2nd parameter mirror_conf has been changed from struct rte_eth_vmdq_mirror_conf to struct rte_eth_mirror_conf."
 - Effect: "Replacement of parameter base type may indicate a change in its semantic meaning."

So, I'm not sure whether this patch meet the ABI policy?

Additional, about the validate-abi.sh, does it mean we need to fix all the problems it reports? Or we can decide case by case. Can a Low Severity problem be acceptable?

Look forward to your reply.

Thanks

Jingjing

> -----Original Message-----
> From: Wu, Jingjing
> Sent: Wednesday, June 10, 2015 2:25 PM
> To: dev@dpdk.org
> Cc: Wu, Jingjing; Liu, Jijiang; Jiajia, SunX; Zhang, Helin
> Subject: [PATCH v4 1/4] ethdev: rename rte_eth_vmdq_mirror_conf
> 
> rename rte_eth_vmdq_mirror_conf to rte_eth_mirror_conf and move the
> maximum rule id check from ethdev level to driver
> 
> Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
> ---
>  app/test-pmd/cmdline.c           | 22 +++++++++++-----------
>  drivers/net/ixgbe/ixgbe_ethdev.c | 11 +++++++----
> drivers/net/ixgbe/ixgbe_ethdev.h |  4 +++-
>  lib/librte_ether/rte_ethdev.c    | 18 ++----------------
>  lib/librte_ether/rte_ethdev.h    | 19 ++++++++++---------
>  5 files changed, 33 insertions(+), 41 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> f01db2a..d693bde 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -6604,11 +6604,11 @@ cmd_set_mirror_mask_parsed(void
> *parsed_result,  {
>  	int ret,nb_item,i;
>  	struct cmd_set_mirror_mask_result *res = parsed_result;
> -	struct rte_eth_vmdq_mirror_conf mr_conf;
> +	struct rte_eth_mirror_conf mr_conf;
> 
> -	memset(&mr_conf,0,sizeof(struct rte_eth_vmdq_mirror_conf));
> +	memset(&mr_conf, 0, sizeof(struct rte_eth_mirror_conf));
> 
> -	unsigned int vlan_list[ETH_VMDQ_MAX_VLAN_FILTERS];
> +	unsigned int vlan_list[ETH_MIRROR_MAX_VLANS];
> 
>  	mr_conf.dst_pool = res->dstpool_id;
> 
> @@ -6618,11 +6618,11 @@ cmd_set_mirror_mask_parsed(void
> *parsed_result,
>  	} else if(!strcmp(res->what, "vlan-mirror")) {
>  		mr_conf.rule_type_mask = ETH_VMDQ_VLAN_MIRROR;
>  		nb_item = parse_item_list(res->value, "core",
> -
> 	ETH_VMDQ_MAX_VLAN_FILTERS,vlan_list,1);
> +					ETH_MIRROR_MAX_VLANS, vlan_list,
> 1);
>  		if (nb_item <= 0)
>  			return;
> 
> -		for(i=0; i < nb_item; i++) {
> +		for (i = 0; i < nb_item; i++) {
>  			if (vlan_list[i] > ETHER_MAX_VLAN_ID) {
>  				printf("Invalid vlan_id: must be < 4096\n");
>  				return;
> @@ -6634,10 +6634,10 @@ cmd_set_mirror_mask_parsed(void
> *parsed_result,
>  	}
> 
>  	if(!strcmp(res->on, "on"))
> -		ret = rte_eth_mirror_rule_set(res->port_id,&mr_conf,
> +		ret = rte_eth_mirror_rule_set(res->port_id, &mr_conf,
>  						res->rule_id, 1);
>  	else
> -		ret = rte_eth_mirror_rule_set(res->port_id,&mr_conf,
> +		ret = rte_eth_mirror_rule_set(res->port_id, &mr_conf,
>  						res->rule_id, 0);
>  	if(ret < 0)
>  		printf("mirror rule add error: (%s)\n", strerror(-ret)); @@ -
> 6711,9 +6711,9 @@ cmd_set_mirror_link_parsed(void *parsed_result,  {
>  	int ret;
>  	struct cmd_set_mirror_link_result *res = parsed_result;
> -	struct rte_eth_vmdq_mirror_conf mr_conf;
> +	struct rte_eth_mirror_conf mr_conf;
> 
> -	memset(&mr_conf,0,sizeof(struct rte_eth_vmdq_mirror_conf));
> +	memset(&mr_conf, 0, sizeof(struct rte_eth_mirror_conf));
>  	if(!strcmp(res->what, "uplink-mirror")) {
>  		mr_conf.rule_type_mask = ETH_VMDQ_UPLINK_MIRROR;
>  	}else if(!strcmp(res->what, "downlink-mirror")) @@ -6722,10
> +6722,10 @@ cmd_set_mirror_link_parsed(void *parsed_result,
>  	mr_conf.dst_pool = res->dstpool_id;
> 
>  	if(!strcmp(res->on, "on"))
> -		ret = rte_eth_mirror_rule_set(res->port_id,&mr_conf,
> +		ret = rte_eth_mirror_rule_set(res->port_id, &mr_conf,
>  						res->rule_id, 1);
>  	else
> -		ret = rte_eth_mirror_rule_set(res->port_id,&mr_conf,
> +		ret = rte_eth_mirror_rule_set(res->port_id, &mr_conf,
>  						res->rule_id, 0);
> 
>  	/* check the return value and print it if is < 0 */ diff --git
> a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index 0d9f9b2..9e767fa 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -209,7 +209,7 @@ static int ixgbe_set_pool_tx(struct rte_eth_dev
> *dev,uint16_t pool,uint8_t on);  static int ixgbe_set_pool_vlan_filter(struct
> rte_eth_dev *dev, uint16_t vlan,
>  		uint64_t pool_mask,uint8_t vlan_on);
>  static int ixgbe_mirror_rule_set(struct rte_eth_dev *dev,
> -		struct rte_eth_vmdq_mirror_conf *mirror_conf,
> +		struct rte_eth_mirror_conf *mirror_conf,
>  		uint8_t rule_id, uint8_t on);
>  static int ixgbe_mirror_rule_reset(struct rte_eth_dev *dev,
>  		uint8_t	rule_id);
> @@ -3388,7 +3388,7 @@ ixgbe_set_pool_vlan_filter(struct rte_eth_dev
> *dev, uint16_t vlan,
> 
>  static int
>  ixgbe_mirror_rule_set(struct rte_eth_dev *dev,
> -			struct rte_eth_vmdq_mirror_conf *mirror_conf,
> +			struct rte_eth_mirror_conf *mirror_conf,
>  			uint8_t rule_id, uint8_t on)
>  {
>  	uint32_t mr_ctl,vlvf;
> @@ -3412,7 +3412,10 @@ ixgbe_mirror_rule_set(struct rte_eth_dev *dev,
>  		IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> 
>  	if (ixgbe_vmdq_mode_check(hw) < 0)
> -		return (-ENOTSUP);
> +		return -ENOTSUP;
> +
> +	if (rule_id >= IXGBE_MAX_MIRROR_RULES)
> +		return -EINVAL;
> 
>  	/* Check if vlan mask is valid */
>  	if ((mirror_conf->rule_type_mask & ETH_VMDQ_VLAN_MIRROR)
> && (on)) { @@ -3526,7 +3529,7 @@ ixgbe_mirror_rule_reset(struct
> rte_eth_dev *dev, uint8_t rule_id)
>  		return (-ENOTSUP);
> 
>  	memset(&mr_info->mr_conf[rule_id], 0,
> -		sizeof(struct rte_eth_vmdq_mirror_conf));
> +		sizeof(struct rte_eth_mirror_conf));
> 
>  	/* clear PFVMCTL register */
>  	IXGBE_WRITE_REG(hw, IXGBE_MRCTL(rule_id), mr_ctl); diff --git
> a/drivers/net/ixgbe/ixgbe_ethdev.h b/drivers/net/ixgbe/ixgbe_ethdev.h
> index 19237b8..755b674 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.h
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.h
> @@ -177,8 +177,10 @@ struct ixgbe_uta_info {
>  	uint32_t uta_shadow[IXGBE_MAX_UTA];
>  };
> 
> +#define IXGBE_MAX_MIRROR_RULES 4  /* Maximum nb. of mirror rules. */
> +
>  struct ixgbe_mirror_info {
> -	struct rte_eth_vmdq_mirror_conf
> mr_conf[ETH_VMDQ_NUM_MIRROR_RULE];
> +	struct rte_eth_mirror_conf mr_conf[IXGBE_MAX_MIRROR_RULES];
>  	/**< store PF mirror rules configuration*/  };
> 
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index 024fe8b..43c7295 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -3034,7 +3034,7 @@ int rte_eth_set_vf_rate_limit(uint8_t port_id,
> uint16_t vf, uint16_t tx_rate,
> 
>  int
>  rte_eth_mirror_rule_set(uint8_t port_id,
> -			struct rte_eth_vmdq_mirror_conf *mirror_conf,
> +			struct rte_eth_mirror_conf *mirror_conf,
>  			uint8_t rule_id, uint8_t on)
>  {
>  	struct rte_eth_dev *dev = &rte_eth_devices[port_id]; @@ -3051,7
> +3051,7 @@ rte_eth_mirror_rule_set(uint8_t port_id,
> 
>  	if (mirror_conf->dst_pool >= ETH_64_POOLS) {
>  		PMD_DEBUG_TRACE("Invalid dst pool, pool id must"
> -			"be 0-%d\n",ETH_64_POOLS - 1);
> +			"be 0-%d\n", ETH_64_POOLS - 1);
>  		return -EINVAL;
>  	}
> 
> @@ -3062,13 +3062,6 @@ rte_eth_mirror_rule_set(uint8_t port_id,
>  		return -EINVAL;
>  	}
> 
> -	if(rule_id >= ETH_VMDQ_NUM_MIRROR_RULE)
> -	{
> -		PMD_DEBUG_TRACE("Invalid rule_id, rule_id must be 0-
> %d\n",
> -			ETH_VMDQ_NUM_MIRROR_RULE - 1);
> -		return -EINVAL;
> -	}
> -
>  	dev = &rte_eth_devices[port_id];
>  	FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mirror_rule_set, -
> ENOTSUP);
> 
> @@ -3085,13 +3078,6 @@ rte_eth_mirror_rule_reset(uint8_t port_id,
> uint8_t rule_id)
>  		return -ENODEV;
>  	}
> 
> -	if(rule_id >= ETH_VMDQ_NUM_MIRROR_RULE)
> -	{
> -		PMD_DEBUG_TRACE("Invalid rule_id, rule_id must be 0-
> %d\n",
> -			ETH_VMDQ_NUM_MIRROR_RULE-1);
> -		return -EINVAL;
> -	}
> -
>  	dev = &rte_eth_devices[port_id];
>  	FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mirror_rule_reset, -
> ENOTSUP);
> 
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index 16dbe00..ae22fea 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -467,8 +467,8 @@ struct rte_eth_rss_conf {
>  #define ETH_VMDQ_ACCEPT_BROADCAST   0x0008 /**< accept broadcast
> packets. */
>  #define ETH_VMDQ_ACCEPT_MULTICAST   0x0010 /**< multicast
> promiscuous. */
> 
> -/* Definitions used for VMDQ mirror rules setting */
> -#define ETH_VMDQ_NUM_MIRROR_RULE     4 /**< Maximum nb. of mirror
> rules. . */
> +/** Maximum nb. of vlan per mirror rule */
> +#define ETH_MIRROR_MAX_VLANS       64
> 
>  #define ETH_VMDQ_POOL_MIRROR    0x0001 /**< Virtual Pool Mirroring. */
>  #define ETH_VMDQ_UPLINK_MIRROR  0x0002 /**< Uplink Port Mirroring.
> */ @@ -480,18 +480,19 @@ struct rte_eth_rss_conf {
>   */
>  struct rte_eth_vlan_mirror {
>  	uint64_t vlan_mask; /**< mask for valid VLAN ID. */
> -	uint16_t vlan_id[ETH_VMDQ_MAX_VLAN_FILTERS];
> -	/** VLAN ID list for vlan mirror. */
> +	/** VLAN ID list for vlan mirroring. */
> +	uint16_t vlan_id[ETH_MIRROR_MAX_VLANS];
>  };
> 
>  /**
>   * A structure used to configure traffic mirror of an Ethernet port.
>   */
> -struct rte_eth_vmdq_mirror_conf {
> +struct rte_eth_mirror_conf {
>  	uint8_t rule_type_mask; /**< Mirroring rule type mask we want to
> set */
> -	uint8_t dst_pool; /**< Destination pool for this mirror rule. */
> +	uint8_t dst_pool;  /**< Destination pool for this mirror rule. */
>  	uint64_t pool_mask; /**< Bitmap of pool for pool mirroring */
> -	struct rte_eth_vlan_mirror vlan; /**< VLAN ID setting for VLAN
> mirroring */
> +	/** VLAN ID setting for VLAN mirroring. */
> +	struct rte_eth_vlan_mirror vlan;
>  };
> 
>  /**
> @@ -1211,7 +1212,7 @@ typedef int (*eth_set_vf_rate_limit_t)(struct
> rte_eth_dev *dev,  /**< @internal Set VF TX rate */
> 
>  typedef int (*eth_mirror_rule_set_t)(struct rte_eth_dev *dev,
> -				  struct rte_eth_vmdq_mirror_conf
> *mirror_conf,
> +				  struct rte_eth_mirror_conf *mirror_conf,
>  				  uint8_t rule_id,
>  				  uint8_t on);
>  /**< @internal Add a traffic mirroring rule on an Ethernet device */ @@ -
> 3168,7 +3169,7 @@ rte_eth_dev_set_vf_vlan_filter(uint8_t port, uint16_t
> vlan_id,
>   *   - (-EINVAL) if the mr_conf information is not correct.
>   */
>  int rte_eth_mirror_rule_set(uint8_t port_id,
> -			struct rte_eth_vmdq_mirror_conf *mirror_conf,
> +			struct rte_eth_mirror_conf *mirror_conf,
>  			uint8_t rule_id,
>  			uint8_t on);
> 
> --
> 1.9.3
  
Jingjing Wu July 6, 2015, 1:27 a.m. UTC | #2
Hi, Thomas

Any suggestions about this patch? Do I need rework it by using macro RTE_NEXT_ABI?
Thanks a lot!

Jingjing

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wu, Jingjing
> Sent: Friday, June 26, 2015 3:03 PM
> To: 'nhorman@tuxdriver.com'
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v4 1/4] ethdev: rename
> rte_eth_vmdq_mirror_conf
> 
> Hi, Neil
> 
> About this patch I have an ABI concern about it.
> This patch just renamed a struct rte_eth_vmdq_mirror_conf to
> rte_eth_mirror_conf, the size and its elements don't change. As my
> understanding, it will not break the ABI. And I also tested it.
> But when I use the script ./scripts/validate-abi.sh to check. A low severity
> problem is reported in symbol "rte_eth_mirror_rule_set"
>  - Change: "Base type of 2nd parameter mirror_conf has been changed from
> struct rte_eth_vmdq_mirror_conf to struct rte_eth_mirror_conf."
>  - Effect: "Replacement of parameter base type may indicate a change in its
> semantic meaning."
> 
> So, I'm not sure whether this patch meet the ABI policy?
> 
> Additional, about the validate-abi.sh, does it mean we need to fix all the
> problems it reports? Or we can decide case by case. Can a Low Severity
> problem be acceptable?
> 
> Look forward to your reply.
> 
> Thanks
> 
> Jingjing
> 
> > -----Original Message-----
> > From: Wu, Jingjing
> > Sent: Wednesday, June 10, 2015 2:25 PM
> > To: dev@dpdk.org
> > Cc: Wu, Jingjing; Liu, Jijiang; Jiajia, SunX; Zhang, Helin
> > Subject: [PATCH v4 1/4] ethdev: rename rte_eth_vmdq_mirror_conf
> >
> > rename rte_eth_vmdq_mirror_conf to rte_eth_mirror_conf and move the
> > maximum rule id check from ethdev level to driver
> >
> > Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
> > ---
> >  app/test-pmd/cmdline.c           | 22 +++++++++++-----------
> >  drivers/net/ixgbe/ixgbe_ethdev.c | 11 +++++++----
> > drivers/net/ixgbe/ixgbe_ethdev.h |  4 +++-
> >  lib/librte_ether/rte_ethdev.c    | 18 ++----------------
> >  lib/librte_ether/rte_ethdev.h    | 19 ++++++++++---------
> >  5 files changed, 33 insertions(+), 41 deletions(-)
> >
> > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > f01db2a..d693bde 100644
> > --- a/app/test-pmd/cmdline.c
> > +++ b/app/test-pmd/cmdline.c
> > @@ -6604,11 +6604,11 @@ cmd_set_mirror_mask_parsed(void
> > *parsed_result,  {
> >  	int ret,nb_item,i;
> >  	struct cmd_set_mirror_mask_result *res = parsed_result;
> > -	struct rte_eth_vmdq_mirror_conf mr_conf;
> > +	struct rte_eth_mirror_conf mr_conf;
> >
> > -	memset(&mr_conf,0,sizeof(struct rte_eth_vmdq_mirror_conf));
> > +	memset(&mr_conf, 0, sizeof(struct rte_eth_mirror_conf));
> >
> > -	unsigned int vlan_list[ETH_VMDQ_MAX_VLAN_FILTERS];
> > +	unsigned int vlan_list[ETH_MIRROR_MAX_VLANS];
> >
> >  	mr_conf.dst_pool = res->dstpool_id;
> >
> > @@ -6618,11 +6618,11 @@ cmd_set_mirror_mask_parsed(void
> > *parsed_result,
> >  	} else if(!strcmp(res->what, "vlan-mirror")) {
> >  		mr_conf.rule_type_mask = ETH_VMDQ_VLAN_MIRROR;
> >  		nb_item = parse_item_list(res->value, "core",
> > -
> > 	ETH_VMDQ_MAX_VLAN_FILTERS,vlan_list,1);
> > +					ETH_MIRROR_MAX_VLANS, vlan_list,
> > 1);
> >  		if (nb_item <= 0)
> >  			return;
> >
> > -		for(i=0; i < nb_item; i++) {
> > +		for (i = 0; i < nb_item; i++) {
> >  			if (vlan_list[i] > ETHER_MAX_VLAN_ID) {
> >  				printf("Invalid vlan_id: must be < 4096\n");
> >  				return;
> > @@ -6634,10 +6634,10 @@ cmd_set_mirror_mask_parsed(void
> > *parsed_result,
> >  	}
> >
> >  	if(!strcmp(res->on, "on"))
> > -		ret = rte_eth_mirror_rule_set(res->port_id,&mr_conf,
> > +		ret = rte_eth_mirror_rule_set(res->port_id, &mr_conf,
> >  						res->rule_id, 1);
> >  	else
> > -		ret = rte_eth_mirror_rule_set(res->port_id,&mr_conf,
> > +		ret = rte_eth_mirror_rule_set(res->port_id, &mr_conf,
> >  						res->rule_id, 0);
> >  	if(ret < 0)
> >  		printf("mirror rule add error: (%s)\n", strerror(-ret)); @@ -
> > 6711,9 +6711,9 @@ cmd_set_mirror_link_parsed(void *parsed_result,  {
> >  	int ret;
> >  	struct cmd_set_mirror_link_result *res = parsed_result;
> > -	struct rte_eth_vmdq_mirror_conf mr_conf;
> > +	struct rte_eth_mirror_conf mr_conf;
> >
> > -	memset(&mr_conf,0,sizeof(struct rte_eth_vmdq_mirror_conf));
> > +	memset(&mr_conf, 0, sizeof(struct rte_eth_mirror_conf));
> >  	if(!strcmp(res->what, "uplink-mirror")) {
> >  		mr_conf.rule_type_mask = ETH_VMDQ_UPLINK_MIRROR;
> >  	}else if(!strcmp(res->what, "downlink-mirror")) @@ -6722,10
> > +6722,10 @@ cmd_set_mirror_link_parsed(void *parsed_result,
> >  	mr_conf.dst_pool = res->dstpool_id;
> >
> >  	if(!strcmp(res->on, "on"))
> > -		ret = rte_eth_mirror_rule_set(res->port_id,&mr_conf,
> > +		ret = rte_eth_mirror_rule_set(res->port_id, &mr_conf,
> >  						res->rule_id, 1);
> >  	else
> > -		ret = rte_eth_mirror_rule_set(res->port_id,&mr_conf,
> > +		ret = rte_eth_mirror_rule_set(res->port_id, &mr_conf,
> >  						res->rule_id, 0);
> >
> >  	/* check the return value and print it if is < 0 */ diff --git
> > a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> > index 0d9f9b2..9e767fa 100644
> > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > @@ -209,7 +209,7 @@ static int ixgbe_set_pool_tx(struct rte_eth_dev
> > *dev,uint16_t pool,uint8_t on);  static int
> > ixgbe_set_pool_vlan_filter(struct rte_eth_dev *dev, uint16_t vlan,
> >  		uint64_t pool_mask,uint8_t vlan_on);  static int
> > ixgbe_mirror_rule_set(struct rte_eth_dev *dev,
> > -		struct rte_eth_vmdq_mirror_conf *mirror_conf,
> > +		struct rte_eth_mirror_conf *mirror_conf,
> >  		uint8_t rule_id, uint8_t on);
> >  static int ixgbe_mirror_rule_reset(struct rte_eth_dev *dev,
> >  		uint8_t	rule_id);
> > @@ -3388,7 +3388,7 @@ ixgbe_set_pool_vlan_filter(struct rte_eth_dev
> > *dev, uint16_t vlan,
> >
> >  static int
> >  ixgbe_mirror_rule_set(struct rte_eth_dev *dev,
> > -			struct rte_eth_vmdq_mirror_conf *mirror_conf,
> > +			struct rte_eth_mirror_conf *mirror_conf,
> >  			uint8_t rule_id, uint8_t on)
> >  {
> >  	uint32_t mr_ctl,vlvf;
> > @@ -3412,7 +3412,10 @@ ixgbe_mirror_rule_set(struct rte_eth_dev *dev,
> >  		IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> >
> >  	if (ixgbe_vmdq_mode_check(hw) < 0)
> > -		return (-ENOTSUP);
> > +		return -ENOTSUP;
> > +
> > +	if (rule_id >= IXGBE_MAX_MIRROR_RULES)
> > +		return -EINVAL;
> >
> >  	/* Check if vlan mask is valid */
> >  	if ((mirror_conf->rule_type_mask & ETH_VMDQ_VLAN_MIRROR)
> && (on)) {
> > @@ -3526,7 +3529,7 @@ ixgbe_mirror_rule_reset(struct rte_eth_dev
> *dev,
> > uint8_t rule_id)
> >  		return (-ENOTSUP);
> >
> >  	memset(&mr_info->mr_conf[rule_id], 0,
> > -		sizeof(struct rte_eth_vmdq_mirror_conf));
> > +		sizeof(struct rte_eth_mirror_conf));
> >
> >  	/* clear PFVMCTL register */
> >  	IXGBE_WRITE_REG(hw, IXGBE_MRCTL(rule_id), mr_ctl); diff --git
> > a/drivers/net/ixgbe/ixgbe_ethdev.h b/drivers/net/ixgbe/ixgbe_ethdev.h
> > index 19237b8..755b674 100644
> > --- a/drivers/net/ixgbe/ixgbe_ethdev.h
> > +++ b/drivers/net/ixgbe/ixgbe_ethdev.h
> > @@ -177,8 +177,10 @@ struct ixgbe_uta_info {
> >  	uint32_t uta_shadow[IXGBE_MAX_UTA];
> >  };
> >
> > +#define IXGBE_MAX_MIRROR_RULES 4  /* Maximum nb. of mirror rules.
> */
> > +
> >  struct ixgbe_mirror_info {
> > -	struct rte_eth_vmdq_mirror_conf
> > mr_conf[ETH_VMDQ_NUM_MIRROR_RULE];
> > +	struct rte_eth_mirror_conf mr_conf[IXGBE_MAX_MIRROR_RULES];
> >  	/**< store PF mirror rules configuration*/  };
> >
> > diff --git a/lib/librte_ether/rte_ethdev.c
> > b/lib/librte_ether/rte_ethdev.c index 024fe8b..43c7295 100644
> > --- a/lib/librte_ether/rte_ethdev.c
> > +++ b/lib/librte_ether/rte_ethdev.c
> > @@ -3034,7 +3034,7 @@ int rte_eth_set_vf_rate_limit(uint8_t port_id,
> > uint16_t vf, uint16_t tx_rate,
> >
> >  int
> >  rte_eth_mirror_rule_set(uint8_t port_id,
> > -			struct rte_eth_vmdq_mirror_conf *mirror_conf,
> > +			struct rte_eth_mirror_conf *mirror_conf,
> >  			uint8_t rule_id, uint8_t on)
> >  {
> >  	struct rte_eth_dev *dev = &rte_eth_devices[port_id]; @@ -3051,7
> > +3051,7 @@ rte_eth_mirror_rule_set(uint8_t port_id,
> >
> >  	if (mirror_conf->dst_pool >= ETH_64_POOLS) {
> >  		PMD_DEBUG_TRACE("Invalid dst pool, pool id must"
> > -			"be 0-%d\n",ETH_64_POOLS - 1);
> > +			"be 0-%d\n", ETH_64_POOLS - 1);
> >  		return -EINVAL;
> >  	}
> >
> > @@ -3062,13 +3062,6 @@ rte_eth_mirror_rule_set(uint8_t port_id,
> >  		return -EINVAL;
> >  	}
> >
> > -	if(rule_id >= ETH_VMDQ_NUM_MIRROR_RULE)
> > -	{
> > -		PMD_DEBUG_TRACE("Invalid rule_id, rule_id must be 0-
> > %d\n",
> > -			ETH_VMDQ_NUM_MIRROR_RULE - 1);
> > -		return -EINVAL;
> > -	}
> > -
> >  	dev = &rte_eth_devices[port_id];
> >  	FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mirror_rule_set, -
> ENOTSUP);
> >
> > @@ -3085,13 +3078,6 @@ rte_eth_mirror_rule_reset(uint8_t port_id,
> > uint8_t rule_id)
> >  		return -ENODEV;
> >  	}
> >
> > -	if(rule_id >= ETH_VMDQ_NUM_MIRROR_RULE)
> > -	{
> > -		PMD_DEBUG_TRACE("Invalid rule_id, rule_id must be 0-
> > %d\n",
> > -			ETH_VMDQ_NUM_MIRROR_RULE-1);
> > -		return -EINVAL;
> > -	}
> > -
> >  	dev = &rte_eth_devices[port_id];
> >  	FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mirror_rule_reset, -
> ENOTSUP);
> >
> > diff --git a/lib/librte_ether/rte_ethdev.h
> > b/lib/librte_ether/rte_ethdev.h index 16dbe00..ae22fea 100644
> > --- a/lib/librte_ether/rte_ethdev.h
> > +++ b/lib/librte_ether/rte_ethdev.h
> > @@ -467,8 +467,8 @@ struct rte_eth_rss_conf {
> >  #define ETH_VMDQ_ACCEPT_BROADCAST   0x0008 /**< accept broadcast
> > packets. */
> >  #define ETH_VMDQ_ACCEPT_MULTICAST   0x0010 /**< multicast
> > promiscuous. */
> >
> > -/* Definitions used for VMDQ mirror rules setting */
> > -#define ETH_VMDQ_NUM_MIRROR_RULE     4 /**< Maximum nb. of
> mirror
> > rules. . */
> > +/** Maximum nb. of vlan per mirror rule */
> > +#define ETH_MIRROR_MAX_VLANS       64
> >
> >  #define ETH_VMDQ_POOL_MIRROR    0x0001 /**< Virtual Pool Mirroring.
> */
> >  #define ETH_VMDQ_UPLINK_MIRROR  0x0002 /**< Uplink Port Mirroring.
> > */ @@ -480,18 +480,19 @@ struct rte_eth_rss_conf {
> >   */
> >  struct rte_eth_vlan_mirror {
> >  	uint64_t vlan_mask; /**< mask for valid VLAN ID. */
> > -	uint16_t vlan_id[ETH_VMDQ_MAX_VLAN_FILTERS];
> > -	/** VLAN ID list for vlan mirror. */
> > +	/** VLAN ID list for vlan mirroring. */
> > +	uint16_t vlan_id[ETH_MIRROR_MAX_VLANS];
> >  };
> >
> >  /**
> >   * A structure used to configure traffic mirror of an Ethernet port.
> >   */
> > -struct rte_eth_vmdq_mirror_conf {
> > +struct rte_eth_mirror_conf {
> >  	uint8_t rule_type_mask; /**< Mirroring rule type mask we want to
> set
> > */
> > -	uint8_t dst_pool; /**< Destination pool for this mirror rule. */
> > +	uint8_t dst_pool;  /**< Destination pool for this mirror rule. */
> >  	uint64_t pool_mask; /**< Bitmap of pool for pool mirroring */
> > -	struct rte_eth_vlan_mirror vlan; /**< VLAN ID setting for VLAN
> > mirroring */
> > +	/** VLAN ID setting for VLAN mirroring. */
> > +	struct rte_eth_vlan_mirror vlan;
> >  };
> >
> >  /**
> > @@ -1211,7 +1212,7 @@ typedef int (*eth_set_vf_rate_limit_t)(struct
> > rte_eth_dev *dev,  /**< @internal Set VF TX rate */
> >
> >  typedef int (*eth_mirror_rule_set_t)(struct rte_eth_dev *dev,
> > -				  struct rte_eth_vmdq_mirror_conf
> > *mirror_conf,
> > +				  struct rte_eth_mirror_conf *mirror_conf,
> >  				  uint8_t rule_id,
> >  				  uint8_t on);
> >  /**< @internal Add a traffic mirroring rule on an Ethernet device */
> > @@ -
> > 3168,7 +3169,7 @@ rte_eth_dev_set_vf_vlan_filter(uint8_t port,
> > uint16_t vlan_id,
> >   *   - (-EINVAL) if the mr_conf information is not correct.
> >   */
> >  int rte_eth_mirror_rule_set(uint8_t port_id,
> > -			struct rte_eth_vmdq_mirror_conf *mirror_conf,
> > +			struct rte_eth_mirror_conf *mirror_conf,
> >  			uint8_t rule_id,
> >  			uint8_t on);
> >
> > --
> > 1.9.3
  
Thomas Monjalon July 7, 2015, 2:51 p.m. UTC | #3
2015-06-26 07:03, Wu, Jingjing:
> Hi, Neil
> 
> About this patch I have an ABI concern about it. 
> This patch just renamed a struct rte_eth_vmdq_mirror_conf to
> rte_eth_mirror_conf, the size and its elements don't change.
> As my understanding, it will not break the ABI. And I also tested it.
> But when I use the script ./scripts/validate-abi.sh to check.
> A low severity problem is reported in symbol "rte_eth_mirror_rule_set"
>  - Change: "Base type of 2nd parameter mirror_conf has been changed
>  from struct rte_eth_vmdq_mirror_conf to struct rte_eth_mirror_conf."
>  - Effect: "Replacement of parameter base type may indicate a change
>  in its semantic meaning."
> 
> So, I'm not sure whether this patch meet the ABI policy?

I think it's OK.

> Additional, about the validate-abi.sh, does it mean we need to fix
> all the problems it reports? Or we can decide case by case.
> Can a Low Severity problem be acceptable?

We have to decide case by case.
It makes ABI checking impossible to automate.
That's why any help is welcome to check the git HEAD for ABI violation.
  
Jingjing Wu July 8, 2015, 12:58 a.m. UTC | #4
Thanks for the clarification.

Jingjing

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Tuesday, July 07, 2015 10:51 PM
> To: Wu, Jingjing
> Cc: dev@dpdk.org; nhorman@tuxdriver.com
> Subject: Re: [dpdk-dev] [PATCH v4 1/4] ethdev: rename
> rte_eth_vmdq_mirror_conf
> 
> 2015-06-26 07:03, Wu, Jingjing:
> > Hi, Neil
> >
> > About this patch I have an ABI concern about it.
> > This patch just renamed a struct rte_eth_vmdq_mirror_conf to
> > rte_eth_mirror_conf, the size and its elements don't change.
> > As my understanding, it will not break the ABI. And I also tested it.
> > But when I use the script ./scripts/validate-abi.sh to check.
> > A low severity problem is reported in symbol "rte_eth_mirror_rule_set"
> >  - Change: "Base type of 2nd parameter mirror_conf has been changed
> > from struct rte_eth_vmdq_mirror_conf to struct rte_eth_mirror_conf."
> >  - Effect: "Replacement of parameter base type may indicate a change
> > in its semantic meaning."
> >
> > So, I'm not sure whether this patch meet the ABI policy?
> 
> I think it's OK.
> 
> > Additional, about the validate-abi.sh, does it mean we need to fix all
> > the problems it reports? Or we can decide case by case.
> > Can a Low Severity problem be acceptable?
> 
> We have to decide case by case.
> It makes ABI checking impossible to automate.
> That's why any help is welcome to check the git HEAD for ABI violation.
  
John McNamara July 8, 2015, 10:31 a.m. UTC | #5
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Tuesday, July 7, 2015 3:51 PM
> To: Wu, Jingjing
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v4 1/4] ethdev: rename
> rte_eth_vmdq_mirror_conf
> 
> > Additional, about the validate-abi.sh, does it mean we need to fix all
> > the problems it reports? Or we can decide case by case.
> > Can a Low Severity problem be acceptable?
> 
> We have to decide case by case.
> It makes ABI checking impossible to automate.
> That's why any help is welcome to check the git HEAD for ABI violation.

Hi,

Automated testing for ABI breaks is tricky since it requires some interpretation of the errors and the severity level.

However, each report file contains a 2 line summary at the top that can be read and parsed. For example (with long lines wrapped):

    head -2 compat_reports/librte_mbuf.so/v2.0.0_to_ABI_CHECK/compat_report.html

    <!-- kind:binary;verdict:compatible;affected:0;added:1;removed:0;
         type_problems_high:0;type_problems_medium:0;type_problems_low:0;
         interface_problems_high:0;interface_problems_medium:0;
         interface_problems_low:0;changed_constants:0;tool_version:1.99.8.5 -->
    <!-- kind:source;verdict:compatible;affected:0;added:1;removed:0;
         type_problems_high:0;type_problems_medium:0;type_problems_low:0;
         interface_problems_high:0;interface_problems_medium:0;
         interface_problems_low:0;changed_constants:0;tool_version:1.99.8.5 -->

This still requires interpretation but it can be used to search for categories of incompatibility.

To test if a patchset breaks the API it is possible to do something like the following. Say, for example that you have committed 3 commits to your local master, you could tag and run the checker like this:

    git checkout master

    git tag -f ABI_CHECK_AFTER
    git tag -f ABI_CHECK_BEFORE HEAD~3

    ./scripts/validate-abi.sh ABI_CHECK_BEFORE ABI_CHECK_AFTER x86_64-native-linuxapp-gcc

    grep -lr "kind:binary;verdict:incompatible" compat_reports/

The grep could be extended to match other categories that you are interested in. However, it is still probably better to examine the reports manually.

You can also use something this to run a git bisect to find a commit that introduced an ABI incompatibility:

    # Tag the head and some known good point in the history.
    git checkout master
    git tag -f ABI_CHECK_END
    git tag -f ABI_CHECK_START v2.0.0

    # Start git bisect with bad and good.
    git bisect start ABI_CHECK_END ABI_CHECK_START

    # Run bisect with a script.
    git bisect run ~/abi_bisect.sh

Where the ~/abi_bisect.sh script that searches for ABI incompatibilities looks like this:

    #!/bin/sh

    git tag -f ABI_CHECK_END

    rm -rf compat_reports/

    ./scripts/validate-abi.sh ABI_CHECK_START ABI_CHECK_END x86_64-native-linuxapp-gcc

    ! grep -lr "kind:binary;verdict:incompatible" compat_reports/

John.
--
  
Bruce Richardson July 8, 2015, 10:35 a.m. UTC | #6
On Wed, Jul 08, 2015 at 10:31:15AM +0000, Mcnamara, John wrote:
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> > Sent: Tuesday, July 7, 2015 3:51 PM
> > To: Wu, Jingjing
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v4 1/4] ethdev: rename
> > rte_eth_vmdq_mirror_conf
> > 
> > > Additional, about the validate-abi.sh, does it mean we need to fix all
> > > the problems it reports? Or we can decide case by case.
> > > Can a Low Severity problem be acceptable?
> > 
> > We have to decide case by case.
> > It makes ABI checking impossible to automate.
> > That's why any help is welcome to check the git HEAD for ABI violation.
> 
> Hi,
> 
> Automated testing for ABI breaks is tricky since it requires some interpretation of the errors and the severity level.
> 
> However, each report file contains a 2 line summary at the top that can be read and parsed. For example (with long lines wrapped):
> 
>     head -2 compat_reports/librte_mbuf.so/v2.0.0_to_ABI_CHECK/compat_report.html
> 
>     <!-- kind:binary;verdict:compatible;affected:0;added:1;removed:0;
>          type_problems_high:0;type_problems_medium:0;type_problems_low:0;
>          interface_problems_high:0;interface_problems_medium:0;
>          interface_problems_low:0;changed_constants:0;tool_version:1.99.8.5 -->
>     <!-- kind:source;verdict:compatible;affected:0;added:1;removed:0;
>          type_problems_high:0;type_problems_medium:0;type_problems_low:0;
>          interface_problems_high:0;interface_problems_medium:0;
>          interface_problems_low:0;changed_constants:0;tool_version:1.99.8.5 -->
> 
> This still requires interpretation but it can be used to search for categories of incompatibility.
> 
> To test if a patchset breaks the API it is possible to do something like the following. Say, for example that you have committed 3 commits to your local master, you could tag and run the checker like this:
> 
>     git checkout master
> 
>     git tag -f ABI_CHECK_AFTER
>     git tag -f ABI_CHECK_BEFORE HEAD~3
> 
>     ./scripts/validate-abi.sh ABI_CHECK_BEFORE ABI_CHECK_AFTER x86_64-native-linuxapp-gcc
> 
>     grep -lr "kind:binary;verdict:incompatible" compat_reports/
> 
> The grep could be extended to match other categories that you are interested in. However, it is still probably better to examine the reports manually.
> 
> You can also use something this to run a git bisect to find a commit that introduced an ABI incompatibility:
> 
>     # Tag the head and some known good point in the history.
>     git checkout master
>     git tag -f ABI_CHECK_END
>     git tag -f ABI_CHECK_START v2.0.0
> 
>     # Start git bisect with bad and good.
>     git bisect start ABI_CHECK_END ABI_CHECK_START
> 
>     # Run bisect with a script.
>     git bisect run ~/abi_bisect.sh
> 
> Where the ~/abi_bisect.sh script that searches for ABI incompatibilities looks like this:
> 
>     #!/bin/sh
> 
>     git tag -f ABI_CHECK_END
> 
>     rm -rf compat_reports/
> 
>     ./scripts/validate-abi.sh ABI_CHECK_START ABI_CHECK_END x86_64-native-linuxapp-gcc
> 
>     ! grep -lr "kind:binary;verdict:incompatible" compat_reports/
> 
> John.
> -- 

I'd mod this post up as "+1 informative", except that this isn't slashdot. :-)

Very helpful, John. Thanks.

/Bruce
  

Patch

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index f01db2a..d693bde 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -6604,11 +6604,11 @@  cmd_set_mirror_mask_parsed(void *parsed_result,
 {
 	int ret,nb_item,i;
 	struct cmd_set_mirror_mask_result *res = parsed_result;
-	struct rte_eth_vmdq_mirror_conf mr_conf;
+	struct rte_eth_mirror_conf mr_conf;
 
-	memset(&mr_conf,0,sizeof(struct rte_eth_vmdq_mirror_conf));
+	memset(&mr_conf, 0, sizeof(struct rte_eth_mirror_conf));
 
-	unsigned int vlan_list[ETH_VMDQ_MAX_VLAN_FILTERS];
+	unsigned int vlan_list[ETH_MIRROR_MAX_VLANS];
 
 	mr_conf.dst_pool = res->dstpool_id;
 
@@ -6618,11 +6618,11 @@  cmd_set_mirror_mask_parsed(void *parsed_result,
 	} else if(!strcmp(res->what, "vlan-mirror")) {
 		mr_conf.rule_type_mask = ETH_VMDQ_VLAN_MIRROR;
 		nb_item = parse_item_list(res->value, "core",
-					ETH_VMDQ_MAX_VLAN_FILTERS,vlan_list,1);
+					ETH_MIRROR_MAX_VLANS, vlan_list, 1);
 		if (nb_item <= 0)
 			return;
 
-		for(i=0; i < nb_item; i++) {
+		for (i = 0; i < nb_item; i++) {
 			if (vlan_list[i] > ETHER_MAX_VLAN_ID) {
 				printf("Invalid vlan_id: must be < 4096\n");
 				return;
@@ -6634,10 +6634,10 @@  cmd_set_mirror_mask_parsed(void *parsed_result,
 	}
 
 	if(!strcmp(res->on, "on"))
-		ret = rte_eth_mirror_rule_set(res->port_id,&mr_conf,
+		ret = rte_eth_mirror_rule_set(res->port_id, &mr_conf,
 						res->rule_id, 1);
 	else
-		ret = rte_eth_mirror_rule_set(res->port_id,&mr_conf,
+		ret = rte_eth_mirror_rule_set(res->port_id, &mr_conf,
 						res->rule_id, 0);
 	if(ret < 0)
 		printf("mirror rule add error: (%s)\n", strerror(-ret));
@@ -6711,9 +6711,9 @@  cmd_set_mirror_link_parsed(void *parsed_result,
 {
 	int ret;
 	struct cmd_set_mirror_link_result *res = parsed_result;
-	struct rte_eth_vmdq_mirror_conf mr_conf;
+	struct rte_eth_mirror_conf mr_conf;
 
-	memset(&mr_conf,0,sizeof(struct rte_eth_vmdq_mirror_conf));
+	memset(&mr_conf, 0, sizeof(struct rte_eth_mirror_conf));
 	if(!strcmp(res->what, "uplink-mirror")) {
 		mr_conf.rule_type_mask = ETH_VMDQ_UPLINK_MIRROR;
 	}else if(!strcmp(res->what, "downlink-mirror"))
@@ -6722,10 +6722,10 @@  cmd_set_mirror_link_parsed(void *parsed_result,
 	mr_conf.dst_pool = res->dstpool_id;
 
 	if(!strcmp(res->on, "on"))
-		ret = rte_eth_mirror_rule_set(res->port_id,&mr_conf,
+		ret = rte_eth_mirror_rule_set(res->port_id, &mr_conf,
 						res->rule_id, 1);
 	else
-		ret = rte_eth_mirror_rule_set(res->port_id,&mr_conf,
+		ret = rte_eth_mirror_rule_set(res->port_id, &mr_conf,
 						res->rule_id, 0);
 
 	/* check the return value and print it if is < 0 */
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 0d9f9b2..9e767fa 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -209,7 +209,7 @@  static int ixgbe_set_pool_tx(struct rte_eth_dev *dev,uint16_t pool,uint8_t on);
 static int ixgbe_set_pool_vlan_filter(struct rte_eth_dev *dev, uint16_t vlan,
 		uint64_t pool_mask,uint8_t vlan_on);
 static int ixgbe_mirror_rule_set(struct rte_eth_dev *dev,
-		struct rte_eth_vmdq_mirror_conf *mirror_conf,
+		struct rte_eth_mirror_conf *mirror_conf,
 		uint8_t rule_id, uint8_t on);
 static int ixgbe_mirror_rule_reset(struct rte_eth_dev *dev,
 		uint8_t	rule_id);
@@ -3388,7 +3388,7 @@  ixgbe_set_pool_vlan_filter(struct rte_eth_dev *dev, uint16_t vlan,
 
 static int
 ixgbe_mirror_rule_set(struct rte_eth_dev *dev,
-			struct rte_eth_vmdq_mirror_conf *mirror_conf,
+			struct rte_eth_mirror_conf *mirror_conf,
 			uint8_t rule_id, uint8_t on)
 {
 	uint32_t mr_ctl,vlvf;
@@ -3412,7 +3412,10 @@  ixgbe_mirror_rule_set(struct rte_eth_dev *dev,
 		IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
 	if (ixgbe_vmdq_mode_check(hw) < 0)
-		return (-ENOTSUP);
+		return -ENOTSUP;
+
+	if (rule_id >= IXGBE_MAX_MIRROR_RULES)
+		return -EINVAL;
 
 	/* Check if vlan mask is valid */
 	if ((mirror_conf->rule_type_mask & ETH_VMDQ_VLAN_MIRROR) && (on)) {
@@ -3526,7 +3529,7 @@  ixgbe_mirror_rule_reset(struct rte_eth_dev *dev, uint8_t rule_id)
 		return (-ENOTSUP);
 
 	memset(&mr_info->mr_conf[rule_id], 0,
-		sizeof(struct rte_eth_vmdq_mirror_conf));
+		sizeof(struct rte_eth_mirror_conf));
 
 	/* clear PFVMCTL register */
 	IXGBE_WRITE_REG(hw, IXGBE_MRCTL(rule_id), mr_ctl);
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h b/drivers/net/ixgbe/ixgbe_ethdev.h
index 19237b8..755b674 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.h
+++ b/drivers/net/ixgbe/ixgbe_ethdev.h
@@ -177,8 +177,10 @@  struct ixgbe_uta_info {
 	uint32_t uta_shadow[IXGBE_MAX_UTA];
 };
 
+#define IXGBE_MAX_MIRROR_RULES 4  /* Maximum nb. of mirror rules. */
+
 struct ixgbe_mirror_info {
-	struct rte_eth_vmdq_mirror_conf mr_conf[ETH_VMDQ_NUM_MIRROR_RULE];
+	struct rte_eth_mirror_conf mr_conf[IXGBE_MAX_MIRROR_RULES];
 	/**< store PF mirror rules configuration*/
 };
 
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 024fe8b..43c7295 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -3034,7 +3034,7 @@  int rte_eth_set_vf_rate_limit(uint8_t port_id, uint16_t vf, uint16_t tx_rate,
 
 int
 rte_eth_mirror_rule_set(uint8_t port_id,
-			struct rte_eth_vmdq_mirror_conf *mirror_conf,
+			struct rte_eth_mirror_conf *mirror_conf,
 			uint8_t rule_id, uint8_t on)
 {
 	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
@@ -3051,7 +3051,7 @@  rte_eth_mirror_rule_set(uint8_t port_id,
 
 	if (mirror_conf->dst_pool >= ETH_64_POOLS) {
 		PMD_DEBUG_TRACE("Invalid dst pool, pool id must"
-			"be 0-%d\n",ETH_64_POOLS - 1);
+			"be 0-%d\n", ETH_64_POOLS - 1);
 		return -EINVAL;
 	}
 
@@ -3062,13 +3062,6 @@  rte_eth_mirror_rule_set(uint8_t port_id,
 		return -EINVAL;
 	}
 
-	if(rule_id >= ETH_VMDQ_NUM_MIRROR_RULE)
-	{
-		PMD_DEBUG_TRACE("Invalid rule_id, rule_id must be 0-%d\n",
-			ETH_VMDQ_NUM_MIRROR_RULE - 1);
-		return -EINVAL;
-	}
-
 	dev = &rte_eth_devices[port_id];
 	FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mirror_rule_set, -ENOTSUP);
 
@@ -3085,13 +3078,6 @@  rte_eth_mirror_rule_reset(uint8_t port_id, uint8_t rule_id)
 		return -ENODEV;
 	}
 
-	if(rule_id >= ETH_VMDQ_NUM_MIRROR_RULE)
-	{
-		PMD_DEBUG_TRACE("Invalid rule_id, rule_id must be 0-%d\n",
-			ETH_VMDQ_NUM_MIRROR_RULE-1);
-		return -EINVAL;
-	}
-
 	dev = &rte_eth_devices[port_id];
 	FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mirror_rule_reset, -ENOTSUP);
 
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 16dbe00..ae22fea 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -467,8 +467,8 @@  struct rte_eth_rss_conf {
 #define ETH_VMDQ_ACCEPT_BROADCAST   0x0008 /**< accept broadcast packets. */
 #define ETH_VMDQ_ACCEPT_MULTICAST   0x0010 /**< multicast promiscuous. */
 
-/* Definitions used for VMDQ mirror rules setting */
-#define ETH_VMDQ_NUM_MIRROR_RULE     4 /**< Maximum nb. of mirror rules. . */
+/** Maximum nb. of vlan per mirror rule */
+#define ETH_MIRROR_MAX_VLANS       64
 
 #define ETH_VMDQ_POOL_MIRROR    0x0001 /**< Virtual Pool Mirroring. */
 #define ETH_VMDQ_UPLINK_MIRROR  0x0002 /**< Uplink Port Mirroring. */
@@ -480,18 +480,19 @@  struct rte_eth_rss_conf {
  */
 struct rte_eth_vlan_mirror {
 	uint64_t vlan_mask; /**< mask for valid VLAN ID. */
-	uint16_t vlan_id[ETH_VMDQ_MAX_VLAN_FILTERS];
-	/** VLAN ID list for vlan mirror. */
+	/** VLAN ID list for vlan mirroring. */
+	uint16_t vlan_id[ETH_MIRROR_MAX_VLANS];
 };
 
 /**
  * A structure used to configure traffic mirror of an Ethernet port.
  */
-struct rte_eth_vmdq_mirror_conf {
+struct rte_eth_mirror_conf {
 	uint8_t rule_type_mask; /**< Mirroring rule type mask we want to set */
-	uint8_t dst_pool; /**< Destination pool for this mirror rule. */
+	uint8_t dst_pool;  /**< Destination pool for this mirror rule. */
 	uint64_t pool_mask; /**< Bitmap of pool for pool mirroring */
-	struct rte_eth_vlan_mirror vlan; /**< VLAN ID setting for VLAN mirroring */
+	/** VLAN ID setting for VLAN mirroring. */
+	struct rte_eth_vlan_mirror vlan;
 };
 
 /**
@@ -1211,7 +1212,7 @@  typedef int (*eth_set_vf_rate_limit_t)(struct rte_eth_dev *dev,
 /**< @internal Set VF TX rate */
 
 typedef int (*eth_mirror_rule_set_t)(struct rte_eth_dev *dev,
-				  struct rte_eth_vmdq_mirror_conf *mirror_conf,
+				  struct rte_eth_mirror_conf *mirror_conf,
 				  uint8_t rule_id,
 				  uint8_t on);
 /**< @internal Add a traffic mirroring rule on an Ethernet device */
@@ -3168,7 +3169,7 @@  rte_eth_dev_set_vf_vlan_filter(uint8_t port, uint16_t vlan_id,
  *   - (-EINVAL) if the mr_conf information is not correct.
  */
 int rte_eth_mirror_rule_set(uint8_t port_id,
-			struct rte_eth_vmdq_mirror_conf *mirror_conf,
+			struct rte_eth_mirror_conf *mirror_conf,
 			uint8_t rule_id,
 			uint8_t on);