From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id 0EAD12E83 for ; Sun, 24 Sep 2017 18:02:03 +0200 (CEST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga104.jf.intel.com with ESMTP; 24 Sep 2017 09:01:59 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,432,1500966000"; d="scan'208";a="138861372" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga002.jf.intel.com with ESMTP; 24 Sep 2017 09:01:59 -0700 Received: from fmsmsx155.amr.corp.intel.com (10.18.116.71) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.319.2; Sun, 24 Sep 2017 09:01:59 -0700 Received: from shsmsx102.ccr.corp.intel.com (10.239.4.154) by FMSMSX155.amr.corp.intel.com (10.18.116.71) with Microsoft SMTP Server (TLS) id 14.3.319.2; Sun, 24 Sep 2017 09:01:58 -0700 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.213]) by shsmsx102.ccr.corp.intel.com ([169.254.2.175]) with mapi id 14.03.0319.002; Mon, 25 Sep 2017 00:01:56 +0800 From: "Wu, Jingjing" To: "Zhao1, Wei" , "dev@dpdk.org" CC: "Zhao1, Wei" Thread-Topic: [dpdk-dev] [PATCH v3 1/2] net/i40e: queue region set and flush Thread-Index: AQHTLdIy+pOYRIF3eU295iDKk2U4GaLEMLmg Date: Sun, 24 Sep 2017 16:01:55 +0000 Message-ID: <9BB6961774997848B5B42BEC655768F810E7BE31@SHSMSX103.ccr.corp.intel.com> References: <1505282644-40415-1-git-send-email-wei.zhao1@intel.com> <1505445188-70251-1-git-send-email-wei.zhao1@intel.com> <1505445188-70251-2-git-send-email-wei.zhao1@intel.com> In-Reply-To: <1505445188-70251-2-git-send-email-wei.zhao1@intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiOTY1OGFhNDItNDE0OS00MmE1LThhZWEtNDhkOGEzOWRhNTFkIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6IndhZjRtZTVcL2tpYnhFSUkzWjZzbmZnQjFXb0trb0JJKzJyblhsTmtvVkdnPSJ9 x-ctpclassification: CTP_IC dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v3 1/2] net/i40e: queue region set and flush X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 24 Sep 2017 16:02:05 -0000 > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wei Zhao > Sent: Friday, September 15, 2017 11:13 AM > To: dev@dpdk.org > Cc: Zhao1, Wei > Subject: [dpdk-dev] [PATCH v3 1/2] net/i40e: queue region set and flush >=20 > This feature enable queue regions configuration for RSS in PF/VF, > so that different traffic classes or different packet > classification types can be separated to different queues in > different queue regions.This patch can set queue region range, > it include queue number in a region and the index of first queue. > This patch enable mapping between different priorities (UP) and > different traffic classes.It also enable mapping between a region > index and a sepcific flowtype(PCTYPE).It also provide the solution > of flush all configuration about queue region the above described. >=20 > Signed-off-by: Wei Zhao > --- > drivers/net/i40e/i40e_ethdev.c | 19 +- > drivers/net/i40e/i40e_ethdev.h | 30 ++ > drivers/net/i40e/rte_pmd_i40e.c | 482 > ++++++++++++++++++++++++++++++ > drivers/net/i40e/rte_pmd_i40e.h | 38 +++ > drivers/net/i40e/rte_pmd_i40e_version.map | 1 + > 5 files changed, 566 insertions(+), 4 deletions(-) >=20 > diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethde= v.c > index 8e0580c..1663fc0 100644 > --- a/drivers/net/i40e/i40e_ethdev.c > +++ b/drivers/net/i40e/i40e_ethdev.c > @@ -137,10 +137,6 @@ > #define I40E_PRTTSYN_TSYNTYPE 0x0e000000 > #define I40E_CYCLECOUNTER_MASK 0xffffffffffffffffULL >=20 > -#define I40E_MAX_PERCENT 100 > -#define I40E_DEFAULT_DCB_APP_NUM 1 > -#define I40E_DEFAULT_DCB_APP_PRIO 3 > - > /** > * Below are values for writing un-exposed registers suggested > * by silicon experts > @@ -1034,6 +1030,15 @@ i40e_init_fdir_filter_list(struct rte_eth_dev *dev= ) > return ret; > } >=20 > +static void > +i40e_init_queue_region_conf(struct rte_eth_dev *dev) > +{ > + struct i40e_pf *pf =3D I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private); > + struct i40e_queue_region_info *info =3D &pf->queue_region; > + > + memset(info, 0, sizeof(struct i40e_queue_region_info)); > +} > + > static int > eth_i40e_dev_init(struct rte_eth_dev *dev) > { > @@ -1309,6 +1314,9 @@ eth_i40e_dev_init(struct rte_eth_dev *dev) > if (ret < 0) > goto err_init_fdir_filter_list; >=20 > + /* initialize queue region configuration */ > + i40e_init_queue_region_conf(dev); > + If this func i40e_init_queue_region_conf is only called here, then it is Not necessary? Because all the pf struct has been zeroed at probe time. > return 0; >=20 > err_init_fdir_filter_list: > @@ -1464,6 +1472,9 @@ eth_i40e_dev_uninit(struct rte_eth_dev *dev) > /* Remove all Traffic Manager configuration */ > i40e_tm_conf_uninit(dev); >=20 > + /* Remove all the queue region configuration */ > + i40e_flush_region_all_conf(hw, pf, 0); > + > return 0; > } >=20 > diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethde= v.h > index ad80f0f..d612886 100644 > --- a/drivers/net/i40e/i40e_ethdev.h > +++ b/drivers/net/i40e/i40e_ethdev.h > @@ -260,6 +260,16 @@ enum i40e_flxpld_layer_idx { > #define I40E_QOS_BW_WEIGHT_MIN 1 > /* The max bandwidth weight is 127. */ > #define I40E_QOS_BW_WEIGHT_MAX 127 > +/* The max queue region index is 7. */ > +#define I40E_REGION_MAX_INDEX 7 > +/* The max queue region userpriority is 7. */ > +#define I40E_REGION_USERPRIORITY_MAX_INDEX 7 You can reuse I40E_MAX_USER_PRIORITY. > +/* The max pctype index is 63. */ > +#define I40E_REGION_PCTYPE_MAX_INDEX 63 > + You can use I40E_FILTER_PCTYPE_MAX > +#define I40E_MAX_PERCENT 100 > +#define I40E_DEFAULT_DCB_APP_NUM 1 > +#define I40E_DEFAULT_DCB_APP_PRIO 3 >=20 > /** > * The overhead from MTU to max frame size. > @@ -541,6 +551,22 @@ struct i40e_ethertype_rule { > struct rte_hash *hash_table; > }; >=20 > +/* queue region info */ > +struct i40e_region_info { > + uint8_t region_id; > + uint8_t queue_start_index; > + uint8_t queue_num; > + uint8_t user_priority_num; > + uint8_t user_priority[I40E_MAX_USER_PRIORITY]; Please comment what is the meaning of this array? > + uint8_t flowtype_num; > + uint8_t hw_flowtype[I40E_FILTER_PCTYPE_MAX]; The same, please comment it. > +}; > + > +struct i40e_queue_region_info { > + uint16_t queue_region_number; > + struct i40e_region_info region[I40E_REGION_MAX_INDEX + 1]; > +}; > + > /* Tunnel filter number HW supports */ > #define I40E_MAX_TUNNEL_FILTER_NUM 400 >=20 > @@ -776,6 +802,7 @@ struct i40e_pf { > struct i40e_fdir_info fdir; /* flow director info */ > struct i40e_ethertype_rule ethertype; /* Ethertype filter rule */ > struct i40e_tunnel_rule tunnel; /* Tunnel filter rule */ > + struct i40e_queue_region_info queue_region; /* queue region info */ > struct i40e_fc_conf fc_conf; /* Flow control conf */ > struct i40e_mirror_rule_list mirror_list; > uint16_t nb_mirror_rule; /* The number of mirror rules */ > @@ -1003,6 +1030,9 @@ void i40e_check_write_reg(struct i40e_hw *hw, uint3= 2_t addr, > uint32_t val); > int i40e_tm_ops_get(struct rte_eth_dev *dev, void *ops); > void i40e_tm_conf_init(struct rte_eth_dev *dev); > void i40e_tm_conf_uninit(struct rte_eth_dev *dev); > +int i40e_flush_region_all_conf(struct i40e_hw *hw, > + struct i40e_pf *pf, uint16_t on); > + >=20 > #define I40E_DEV_TO_PCI(eth_dev) \ > RTE_DEV_TO_PCI((eth_dev)->device) > diff --git a/drivers/net/i40e/rte_pmd_i40e.c b/drivers/net/i40e/rte_pmd_i= 40e.c > index d69a472..9a75f21 100644 > --- a/drivers/net/i40e/rte_pmd_i40e.c > +++ b/drivers/net/i40e/rte_pmd_i40e.c > @@ -35,6 +35,7 @@ > #include >=20 > #include "base/i40e_prototype.h" > +#include "base/i40e_dcb.h" > #include "i40e_ethdev.h" > #include "i40e_pf.h" > #include "i40e_rxtx.h" > @@ -2161,3 +2162,484 @@ rte_pmd_i40e_add_vf_mac_addr(uint8_t port, uint16= _t > vf_id, >=20 > return 0; > } > + > +static int > +i40e_vsi_update_queue_region_mapping(struct i40e_hw *hw, > + struct i40e_pf *pf) > +{ > + uint16_t i; > + struct i40e_vsi *vsi =3D pf->main_vsi; > + uint16_t queue_offset, bsf, tc_index; > + struct i40e_vsi_context ctxt; > + struct i40e_aqc_vsi_properties_data *vsi_info; > + struct i40e_queue_region_info *region_info =3D > + &pf->queue_region; > + uint32_t ret =3D -EINVAL; > + > + if (!region_info->queue_region_number) { > + PMD_INIT_LOG(ERR, "there is no that region id been set before"); > + return ret; > + } > + > + memset(&ctxt, 0, sizeof(struct i40e_vsi_context)); > + > + /* Update Queue Pairs Mapping for currently enabled UPs */ > + ctxt.seid =3D vsi->seid; > + ctxt.pf_num =3D hw->pf_id; > + ctxt.vf_num =3D 0; > + ctxt.uplink_seid =3D vsi->uplink_seid; > + ctxt.info =3D vsi->info; > + vsi_info =3D &ctxt.info; > + Please use the vsi_info stored in i40e_vsi to avoid missing parameters. > + memset(vsi_info->tc_mapping, 0, sizeof(uint16_t) * 8); > + memset(vsi_info->queue_mapping, 0, sizeof(uint16_t) * 16); > + Ctxt has been zeroed, no need to memset tc_mapping and queue_mapping again. > + /** > + * Configure queue region and queue mapping parameters, > + * for enabled queue region, allocate queues to this region. > + */ > + > + for (i =3D 0; i < region_info->queue_region_number; i++) { > + tc_index =3D region_info->region[i].region_id; > + bsf =3D rte_bsf32(region_info->region[i].queue_num); > + queue_offset =3D region_info->region[i].queue_start_index; > + vsi_info->tc_mapping[tc_index] =3D rte_cpu_to_le_16( > + (queue_offset << I40E_AQ_VSI_TC_QUE_OFFSET_SHIFT) | > + (bsf << I40E_AQ_VSI_TC_QUE_NUMBER_SHIFT)); > + } > + > + /* Associate queue number with VSI, Keep vsi->nb_qps unchanged */ > + if (vsi->type =3D=3D I40E_VSI_SRIOV) { Vsi is main vsi, it is impossible that the type to be SRIOV. If you want to= configure Queue mapping on sriov VSI, you need to scan all the SRIOV VSIs. > + vsi_info->mapping_flags |=3D > + rte_cpu_to_le_16(I40E_AQ_VSI_QUE_MAP_NONCONTIG); > + for (i =3D 0; i < vsi->nb_qps; i++) > + vsi_info->queue_mapping[i] =3D > + rte_cpu_to_le_16(vsi->base_queue + i); > + } else { > + vsi_info->mapping_flags |=3D > + rte_cpu_to_le_16(I40E_AQ_VSI_QUE_MAP_CONTIG); > + vsi_info->queue_mapping[0] =3D rte_cpu_to_le_16(vsi->base_queue); > + } > + vsi_info->valid_sections |=3D > + rte_cpu_to_le_16(I40E_AQ_VSI_PROP_QUEUE_MAP_VALID); > + > + /* Update the VSI after updating the VSI queue-mapping information */ > + ret =3D i40e_aq_update_vsi_params(hw, &ctxt, NULL); > + if (ret) { > + PMD_INIT_LOG(ERR, "Failed to configure queue region " PMD_INIT_LOG -> PMD_DRV_LOG > + "mapping =3D %d ", hw->aq.asq_last_status); > + return ret; > + } > + /* update the local VSI info with updated queue map */ > + (void)rte_memcpy(&vsi->info.tc_mapping, &ctxt.info.tc_mapping, > + sizeof(vsi->info.tc_mapping)); > + (void)rte_memcpy(&vsi->info.queue_mapping, > + &ctxt.info.queue_mapping, > + sizeof(vsi->info.queue_mapping)); > + vsi->info.mapping_flags =3D ctxt.info.mapping_flags; > + vsi->info.valid_sections =3D 0; > + > + return 0; > +} > + > + > +static int > +i40e_set_queue_region(struct i40e_pf *pf, > + struct rte_i40e_rss_region_conf *conf_ptr) > +{ > + uint16_t tc_size_tb[7] =3D {1, 2, 4, 8, 16, 32, 64}; > + uint16_t i; > + struct i40e_vsi *main_vsi =3D pf->main_vsi; > + struct i40e_queue_region_info *info =3D &pf->queue_region; > + uint32_t ret =3D -EINVAL; > + > + for (i =3D 0; i < I40E_REGION_MAX_INDEX; i++) > + if (conf_ptr->queue_num =3D=3D tc_size_tb[i]) > + break; > + > + if (i =3D=3D I40E_REGION_MAX_INDEX) { > + printf("The region sizes should be any of the following " > + "values: 1, 2, 4, 8, 16, 32, 64 as long as the " > + "total number of queues do not exceed the VSI allocation"); > + return ret; > + } > + If you just want to check if the queue_nume is one of {1, 2,4,8,16,32,64} You can use=20 If (rte_is_power_of_2(queue_num) && queue_num <=3D 64) > + if (conf_ptr->region_id >=3D I40E_REGION_MAX_INDEX) { > + PMD_INIT_LOG(ERR, "the queue region max index is 7"); > + return ret; > + } > + > + if ((conf_ptr->queue_start_index + conf_ptr->queue_num) > + > main_vsi->nb_used_qps) { > + PMD_INIT_LOG(ERR, "the queue index exceeds the VSI range"); > + return ret; > + } > + > + if (info->queue_region_number) { > + for (i =3D 0; i < info->queue_region_number; i++) > + if (conf_ptr->region_id =3D=3D info->region[i].region_id) > + break; > + > + if ((i =3D=3D info->queue_region_number) && > + (i <=3D I40E_REGION_MAX_INDEX)) { It is impossible that i > I40E_REGION_MAX_INDEX, right? > + info->region[i].region_id =3D conf_ptr->region_id; > + info->region[i].queue_num =3D conf_ptr->queue_num; > + info->region[i].queue_start_index =3D > + conf_ptr->queue_start_index; > + info->queue_region_number++; > + } else { > + PMD_INIT_LOG(ERR, "queue region number " > + "exceeds maxnum 8 or the " > + "queue region id has " > + "been set before"); Does it mean we can not change the set of one queue region whose id is set = before? > + return ret; > + } > + } else { > + info->region[0].region_id =3D conf_ptr->region_id; > + info->region[0].queue_num =3D conf_ptr->queue_num; > + info->region[0].queue_start_index =3D > + conf_ptr->queue_start_index; > + info->queue_region_number++; > + } I think this part of code can be refined.=20 > + > + return 0; > +} > + > +static int > +i40e_set_region_flowtype_pf(struct i40e_hw *hw, > + struct i40e_pf *pf, struct rte_i40e_rss_region_conf *conf_ptr) > +{ > + uint32_t pfqf_hregion; > + uint32_t ret =3D -EINVAL; > + struct i40e_queue_region_info *info =3D &pf->queue_region; > + uint16_t i, j, index, flowtype_set =3D 0; > + uint16_t region_index, flowtype_index; > + > + if (conf_ptr->region_id > I40E_PFQF_HREGION_MAX_INDEX) { > + PMD_INIT_LOG(ERR, "the queue region max index is 7"); > + return ret; > + } > + > + if (conf_ptr->hw_flowtype >=3D I40E_FILTER_PCTYPE_MAX) { > + PMD_INIT_LOG(ERR, "the hw_flowtype or PCTYPE max index is 63"); > + return ret; > + } > + > + if (info->queue_region_number) { > + for (i =3D 0; i < info->queue_region_number; i++) > + if (conf_ptr->region_id =3D=3D info->region[i].region_id) > + break; > + > + if (i =3D=3D info->queue_region_number) { > + PMD_INIT_LOG(ERR, "that region id has not " > + "been set before"); > + return ret; > + } If info->queue_region_number=3D=3D0, then i would be 0. So, the outer wrap if (info->queue_region_number) {} can be omit. The same at above func, I think this part of code can be refined. > + region_index =3D i; > +=09 > + for (i =3D 0; i < info->queue_region_number; i++) { > + for (j =3D 0; j < info->region[i].flowtype_num; j++) { > + if (conf_ptr->hw_flowtype =3D=3D > + info->region[i].hw_flowtype[j]) { > + flowtype_set =3D 1; > + break; > + } > + } > + } > + > + if (flowtype_set) { > + PMD_INIT_LOG(ERR, "that hw_flowtype " > + "has been set before"); > + return ret; Is that right to return -EINVAL when it has been set. And it seems In this function, the error code is only -EINVAL, if so, why we define ret? > + } > + flowtype_index =3D info->region[region_index].flowtype_num; > + info->region[region_index].hw_flowtype[flowtype_index] =3D > + conf_ptr->hw_flowtype; > + info->region[region_index].flowtype_num++; > + } else { > + PMD_INIT_LOG(ERR, "there is no that region id been set before"); Again, please use PMD_DRV_LOG. Because the func is not called at initializa= tion time. Please replace it in all the patch. > + return ret; > + } > + > + index =3D conf_ptr->hw_flowtype >> 3; > + pfqf_hregion =3D i40e_read_rx_ctl(hw, I40E_PFQF_HREGION(index)); > + > + if ((conf_ptr->hw_flowtype & 0x7) =3D=3D 0) { > + pfqf_hregion |=3D conf_ptr->region_id << > + I40E_PFQF_HREGION_REGION_0_SHIFT; > + pfqf_hregion |=3D 1 << I40E_PFQF_HREGION_OVERRIDE_ENA_0_SHIFT; > + } else if ((conf_ptr->hw_flowtype & 0x7) =3D=3D 1) { > + pfqf_hregion |=3D conf_ptr->region_id << > + I40E_PFQF_HREGION_REGION_1_SHIFT; > + pfqf_hregion |=3D 1 << I40E_PFQF_HREGION_OVERRIDE_ENA_1_SHIFT; > + } else if ((conf_ptr->hw_flowtype & 0x7) =3D=3D 2) { > + pfqf_hregion |=3D conf_ptr->region_id << > + I40E_PFQF_HREGION_REGION_2_SHIFT; > + pfqf_hregion |=3D 1 << I40E_PFQF_HREGION_OVERRIDE_ENA_2_SHIFT; > + } else if ((conf_ptr->hw_flowtype & 0x7) =3D=3D 3) { > + pfqf_hregion |=3D conf_ptr->region_id << > + I40E_PFQF_HREGION_REGION_3_SHIFT; > + pfqf_hregion |=3D 1 << I40E_PFQF_HREGION_OVERRIDE_ENA_3_SHIFT; > + } else if ((conf_ptr->hw_flowtype & 0x7) =3D=3D 4) { > + pfqf_hregion |=3D conf_ptr->region_id << > + I40E_PFQF_HREGION_REGION_4_SHIFT; > + pfqf_hregion |=3D 1 << I40E_PFQF_HREGION_OVERRIDE_ENA_4_SHIFT; > + } else if ((conf_ptr->hw_flowtype & 0x7) =3D=3D 5) { > + pfqf_hregion |=3D conf_ptr->region_id << > + I40E_PFQF_HREGION_REGION_5_SHIFT; > + pfqf_hregion |=3D 1 << I40E_PFQF_HREGION_OVERRIDE_ENA_5_SHIFT; > + } else if ((conf_ptr->hw_flowtype & 0x7) =3D=3D 6) { > + pfqf_hregion |=3D conf_ptr->region_id << > + I40E_PFQF_HREGION_REGION_6_SHIFT; > + pfqf_hregion |=3D 1 << I40E_PFQF_HREGION_OVERRIDE_ENA_6_SHIFT; What is the (conf_ptr->hw_flowtype & 0x7) =3D=3D 1/2/3/4/5/6 meanings? Plea= se avoid coding with integer number. Can not understand the relationship with flow t= ype. And the hw_flowtype is API to users, you need to make it clear enough. > + } else { > + pfqf_hregion |=3D conf_ptr->region_id << > + I40E_PFQF_HREGION_REGION_7_SHIFT; > + pfqf_hregion |=3D 1 << I40E_PFQF_HREGION_OVERRIDE_ENA_7_SHIFT; > + } > + > + i40e_write_rx_ctl(hw, I40E_PFQF_HREGION(index), pfqf_hregion); > + > + return 0; > +} > + > +static int > +i40e_set_up_region(struct i40e_pf *pf, > + struct rte_i40e_rss_region_conf *conf_ptr) > +{ > + struct i40e_queue_region_info *info =3D &pf->queue_region; > + uint32_t ret =3D -EINVAL; > + uint16_t i, j, region_index, up_set =3D 0; > + > + if (conf_ptr->user_priority > I40E_REGION_USERPRIORITY_MAX_INDEX) { > + PMD_INIT_LOG(ERR, "the queue region max index is 7"); > + return ret; > + } > + > + if (conf_ptr->region_id >=3D I40E_REGION_MAX_INDEX) { > + PMD_INIT_LOG(ERR, "the region_id max index is 7"); > + return ret; > + } > + > + if (info->queue_region_number) { > + for (i =3D 0; i < info->queue_region_number; i++) > + if (conf_ptr->region_id =3D=3D info->region[i].region_id) > + break; > + > + if (i =3D=3D info->queue_region_number) { > + PMD_INIT_LOG(ERR, "that region id " > + "has not been set before"); > + return ret; > + } > + region_index =3D i; > + > + for (i =3D 0; i < info->queue_region_number; i++) { > + for (j =3D 0; j < > + info->region[i].user_priority_num; j++) { > + if (info->region[i].user_priority[j] =3D=3D > + conf_ptr->user_priority) { > + up_set =3D 1; > + break; > + } > + } > + } > + > + if (up_set) { > + PMD_INIT_LOG(ERR, "that user priority " > + "has been set before"); > + return ret; > + } > + > + j =3D info->region[region_index].user_priority_num; > + info->region[region_index].user_priority[j] =3D > + conf_ptr->user_priority; > + info->region[region_index].user_priority_num++; > + } else { > + PMD_INIT_LOG(ERR, "there is no that region id been set before"); > + return ret; > + } > + > + return 0; > +} > + > +static int > +i40e_queue_region_dcb_configure(struct i40e_hw *hw, > + struct i40e_pf *pf) > +{ > + struct i40e_dcbx_config dcb_cfg_local; > + struct i40e_dcbx_config *dcb_cfg; > + struct i40e_queue_region_info *info =3D &pf->queue_region; > + struct i40e_dcbx_config *old_cfg =3D &hw->local_dcbx_config; > + uint32_t ret =3D -EINVAL; > + uint16_t i, j, prio_index, region_index; > + uint8_t tc_map, tc_bw, bw_lf; > + > + if (!info->queue_region_number) { > + PMD_INIT_LOG(ERR, "there is no that region id been set before"); > + return ret; > + } > + > + dcb_cfg =3D &dcb_cfg_local; > + memset(dcb_cfg, 0, sizeof(struct i40e_dcbx_config)); > + > + /* assume each tc has the same bw */ > + tc_bw =3D I40E_MAX_PERCENT / info->queue_region_number; > + for (i =3D 0; i < info->queue_region_number; i++) > + dcb_cfg->etscfg.tcbwtable[i] =3D tc_bw; > + /* to ensure the sum of tcbw is equal to 100 */ > + bw_lf =3D I40E_MAX_PERCENT % info->queue_region_number; > + for (i =3D 0; i < bw_lf; i++) > + dcb_cfg->etscfg.tcbwtable[i]++; > + > + /* assume each tc has the same Transmission Selection Algorithm */ > + for (i =3D 0; i < info->queue_region_number; i++) > + dcb_cfg->etscfg.tsatable[i] =3D I40E_IEEE_TSA_ETS; > + > + for (i =3D 0; i < info->queue_region_number; i++) { > + for (j =3D 0; j < info->region[i].user_priority_num; j++) { > + prio_index =3D info->region[i].user_priority[j]; > + region_index =3D info->region[i].region_id; > + dcb_cfg->etscfg.prioritytable[prio_index] =3D > + region_index; > + } > + } > + > + /* FW needs one App to configure HW */ > + dcb_cfg->numapps =3D I40E_DEFAULT_DCB_APP_NUM; > + dcb_cfg->app[0].selector =3D I40E_APP_SEL_ETHTYPE; > + dcb_cfg->app[0].priority =3D I40E_DEFAULT_DCB_APP_PRIO; > + dcb_cfg->app[0].protocolid =3D I40E_APP_PROTOID_FCOE; > + > + tc_map =3D RTE_LEN2MASK(info->queue_region_number, uint8_t); > + > + dcb_cfg->pfc.willing =3D 0; > + dcb_cfg->pfc.pfccap =3D I40E_MAX_TRAFFIC_CLASS; > + dcb_cfg->pfc.pfcenable =3D tc_map; > + > + /* Copy the new config to the current config */ > + *old_cfg =3D *dcb_cfg; > + old_cfg->etsrec =3D old_cfg->etscfg; > + ret =3D i40e_set_dcb_config(hw); > + > + if (ret) { > + PMD_INIT_LOG(ERR, "Set queue region DCB Config failed, err" > + " %s aq_err %s", > + i40e_stat_str(hw, ret), > + i40e_aq_str(hw, hw->aq.asq_last_status)); > + return ret; > + } > + > + return 0; > +} > + > +static int > +i40e_queue_region_dcb_configure_default(struct i40e_hw *hw) Can i40e_dcb_init_configure do what this func did here? > +{ > + uint16_t i; > + uint32_t ret =3D -EINVAL; > + > + memset(&hw->local_dcbx_config, 0, > + sizeof(struct i40e_dcbx_config)); > + /* set dcb default configuration */ > + hw->local_dcbx_config.etscfg.willing =3D 0; > + hw->local_dcbx_config.etscfg.maxtcs =3D 0; > + hw->local_dcbx_config.etscfg.tcbwtable[0] =3D 100; > + hw->local_dcbx_config.etscfg.tsatable[0] =3D > + I40E_IEEE_TSA_ETS; > + /* all UPs mapping to region 0 */ > + for (i =3D 0; i < I40E_MAX_USER_PRIORITY; i++) > + hw->local_dcbx_config.etscfg.prioritytable[i] =3D 0; > + hw->local_dcbx_config.etsrec =3D > + hw->local_dcbx_config.etscfg; > + hw->local_dcbx_config.pfc.willing =3D 0; > + hw->local_dcbx_config.pfc.pfccap =3D > + I40E_MAX_TRAFFIC_CLASS; > + /* FW needs one App to configure HW */ > + hw->local_dcbx_config.numapps =3D 1; > + hw->local_dcbx_config.app[0].selector =3D > + I40E_APP_SEL_ETHTYPE; > + hw->local_dcbx_config.app[0].priority =3D 3; > + hw->local_dcbx_config.app[0].protocolid =3D > + I40E_APP_PROTOID_FCOE; > + ret =3D i40e_set_dcb_config(hw); > + if (ret) { > + PMD_INIT_LOG(ERR, > + "default dcb config fails. err =3D %d, aq_err =3D %d.", > + ret, hw->aq.asq_last_status); > + return ret; > + } > + > + return 0; > +} > + > +int > +i40e_flush_region_all_conf(struct i40e_hw *hw, struct i40e_pf *pf, > + uint16_t on) > +{ > + uint16_t i; > + struct i40e_queue_region_info *info =3D &pf->queue_region; > + > + if (on) { > + i40e_vsi_update_queue_region_mapping(hw, pf); > + i40e_queue_region_dcb_configure(hw, pf); Don't check all the return code? > + return 0; > + } > + > + info->queue_region_number =3D 1; > + info->region[0].queue_num =3D 64; > + info->region[0].queue_start_index =3D 0; > + > + i40e_vsi_update_queue_region_mapping(hw, pf); > + i40e_queue_region_dcb_configure_default(hw); > + > + for (i =3D 0; i < I40E_PFQF_HREGION_MAX_INDEX; i++) > + i40e_write_rx_ctl(hw, I40E_PFQF_HREGION(i), 0); > + memset(info, 0, sizeof(struct i40e_queue_region_info)); > + > + return 0; > +} > + > +int rte_pmd_i40e_queue_region_conf(uint8_t port, > + struct rte_i40e_rss_region_conf *conf_ptr) > +{ > + struct rte_eth_dev *dev =3D &rte_eth_devices[port]; > + struct i40e_pf *pf =3D I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private); > + struct i40e_hw *hw =3D I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private); > + enum rte_pmd_i40e_queue_region_op op_type =3D conf_ptr->op; > + uint32_t ret; > + > + if (!is_i40e_supported(dev)) > + return -ENOTSUP; > + > + switch (op_type) { > + case RTE_PMD_I40E_QUEUE_REGION_SET: > + ret =3D i40e_set_queue_region(pf, conf_ptr); > + break; > + case RTE_PMD_I40E_REGION_FLOWTYPE_PF_SET: > + ret =3D i40e_set_region_flowtype_pf(hw, pf, conf_ptr); > + break; > + case RTE_PMD_I40E_REGION_FLOWTYPE_VF_SET: > + ret =3D -EINVAL; > + break; If it is not supported, why to define this op? > + case RTE_PMD_I40E_UP_REGION_SET: > + ret =3D i40e_set_up_region(pf, conf_ptr); > + break; > + case RTE_PMD_I40E_REGION_ALL_FLUSH_ON: > + ret =3D i40e_flush_region_all_conf(hw, pf, 1); > + break; What is the FLUSH on meaning? Commit all configure to HW? If so, what makes me confused that: RTE_PMD_I40E_QUEUE_REGION_SET only touch the software stored in driver. But RTE_PMD_I40E_REGION_FLOWTYPE_PF_SET set the I40E_PFQF_HREGION register. Those behaviors are not consist. And FLUSH OFF means delete all? You need to add more comments. > + case RTE_PMD_I40E_REGION_ALL_FLUSH_OFF: > + ret =3D i40e_flush_region_all_conf(hw, pf, 0); > + break; > + > + default: > + PMD_DRV_LOG(WARNING, "op type (%d) not supported", > + op_type); > + ret =3D -EINVAL; > + break; > + } > + > + I40E_WRITE_FLUSH(hw); > + > + return ret; > +} > diff --git a/drivers/net/i40e/rte_pmd_i40e.h b/drivers/net/i40e/rte_pmd_i= 40e.h > index 155b7e8..a1329f4 100644 > --- a/drivers/net/i40e/rte_pmd_i40e.h > +++ b/drivers/net/i40e/rte_pmd_i40e.h > @@ -91,6 +91,20 @@ enum rte_pmd_i40e_package_info { > RTE_PMD_I40E_PKG_INFO_MAX =3D 0xFFFFFFFF > }; >=20 > +/** > + * Option types of queue region. > + */ > +enum rte_pmd_i40e_queue_region_op { > + RTE_PMD_I40E_REGION_UNDEFINED =3D 0, > + RTE_PMD_I40E_QUEUE_REGION_SET, /**< add queue region set*/ > + RTE_PMD_I40E_REGION_FLOWTYPE_PF_SET, /**< add pf region pctype set */ > + RTE_PMD_I40E_REGION_FLOWTYPE_VF_SET, /**< add vf region pctype set */ > + RTE_PMD_I40E_UP_REGION_SET, /**< add queue region pctype set */ > + RTE_PMD_I40E_REGION_ALL_FLUSH_ON, /**< flush on all configuration */ > + RTE_PMD_I40E_REGION_ALL_FLUSH_OFF, /**< flush off all configuration *= / > + RTE_PMD_I40E_QUEUE_REGION_OP_MAX > +}; > + > #define RTE_PMD_I40E_DDP_NAME_SIZE 32 >=20 > /** > @@ -146,6 +160,18 @@ struct rte_pmd_i40e_ptype_mapping { > }; >=20 > /** > + * Queue region information get from CLI. > + */ > +struct rte_i40e_rss_region_conf { > + uint8_t region_id; > + uint8_t hw_flowtype; > + uint8_t queue_start_index; > + uint8_t queue_num; > + uint8_t user_priority; > + enum rte_pmd_i40e_queue_region_op op; > +}; This is API struct, you need to comment them in detail Thanks Jingjing