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 at dpdk.org] On Behalf Of Wu, Jingjing > Sent: Friday, June 26, 2015 3:03 PM > To: 'nhorman at tuxdriver.com' > Cc: dev at 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 at 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 at 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
