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

["Wu, Jingjing" <jingjing.wu@intel.com>]

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