Hi Wenzhuo, > -----Original Message----- > From: Lu, Wenzhuo > Sent: Wednesday, March 29, 2017 1:53 AM > To: Iremonger, Bernard <bernard.iremon...@intel.com>; dev@dpdk.org; > Xing, Beilei <beilei.x...@intel.com>; Wu, Jingjing <jingjing...@intel.com> > Cc: Zhang, Helin <helin.zh...@intel.com>; Stroe, Laura > <laura.st...@intel.com> > Subject: RE: [dpdk-dev] [PATCH v2 1/3] net/i40e: add QinQ wrapper function > > Hi Bernard, > > > -----Original Message----- > > From: Iremonger, Bernard > > Sent: Tuesday, March 28, 2017 9:23 PM > > To: Iremonger, Bernard; Lu, Wenzhuo; dev@dpdk.org; Xing, Beilei; Wu, > > Jingjing > > Cc: Zhang, Helin; Stroe, Laura > > Subject: RE: [dpdk-dev] [PATCH v2 1/3] net/i40e: add QinQ wrapper > > function > > > > Hi Wenzhuo, > > > > <snip> > > > > -----Original Message----- > > > > > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Bernard > > > > Iremonger > > > > > Sent: Friday, March 24, 2017 12:39 AM > > > > > To: dev@dpdk.org; Xing, Beilei; Wu, Jingjing > > > > > Cc: Zhang, Helin; Iremonger, Bernard; Stroe, Laura > > > > > Subject: [dpdk-dev] [PATCH v2 1/3] net/i40e: add QinQ wrapper > > > > > function > > > > > > > > > > Add i40e_dev_cloud_filter_qinq function, and call it from > > > > > i40e_dev_consistent_tunnel_filter_set function. > > > > > Replace filter 0x1 with QinQ filter. > > > > Better not use 0x1 here. Maybe it should be change to outer IP filter? > > > Ok, I will change to out IP filter. > > > > And seems better to add more explanation about what's QinQ filter > here. > > > Ok, I will add some information. > > > > > > > > > + > > > > > +/* Create a QinQ cloud filter > > > > > + * > > > > > + * The Fortville NIC has limited resources for tunnel filters, > > > > > + * so we can only reuse existing filters. > > > > > + * > > > > > + * In step 1 we define which Field Vector fields can be used > > > > > +for > > > > > + * filter types. > > > > > + * As we do not have the inner tag defined as a field, > > > > > + * we have to define it first, by reusing one of L1 entries. > > > > > + * > > > > > + * In step 2 we are replacing one of existing filter types with > > > > > + * a new one for QinQ. > > > > > + * As we reusing L1 and replacing L2, some of the default > > > > > +filter > > > > > + * types will disappear,which depends on L1 and L2 entries we reuse. > > > > > + * > > > > > + * Step 1: Create L1 filter of outer vlan (12b) + inner vlan > > > > > +(12b) > > > > > + * > > > > > + * 1. Create L1 filter of outer vlan (12b) which will be in > > > > > use > > > > > + * later when we define the cloud filter. > > > > > + * a. Valid_flags.replace_cloud = 0 > > > > > + * b. Old_filter = 10 (Stag_Inner_Vlan) > > > > > + * c. New_filter = 0x10 > > > > > + * d. TR bit = 0xff (optional, not used here) > > > > > + * e. Buffer – 2 entries: > > > > > + * i. Byte0 = 8 (outer vlan FV index). Byte1 =0 (rsv) > > Byte 2- > > > > > 3 = 0x0fff > > > > > + * ii. Byte0 = 37 (inner vlan FV index). Byte1 =0 > (rsv) > > Byte > > > > > 2-3 = 0x0fff > > > > > + * > > > > > + * Step 2: > > > > > + * 2. Create cloud filter using two L1 filters entries: stag > > > > > and > > > > > + * new filter(outer vlan+ inner vlan) > > > > > + * a. Valid_flags.replace_cloud = 1 > > > > > + * b. Old_filter = 1 (instead of outer IP) > > > > > + * c. New_filter = 0x10 > > > > > + * d. Buffer – 2 entries: > > > > > + * i. Byte0 = 0x80 | 7 (valid | Stag). Byte13 = 0 > > > > > (rsv) > > > > > + * ii. Byte8 = 0x80 | 0x10 (valid | new l1 filter > > step1). > > > > > Byte9-11 = 0 (rsv) > > > > > + */ > > > > > +static int > > > > > +i40e_dev_cloud_filter_qinq(struct i40e_pf *pf) { > > > > Is it better to change the function name to > > > > i40e_dev_create_cloud_filter_qinq? > > > > Ok, how about i40e_cloud_filter_qinq_create()? > > It is more in line with the existing code. > It's fine to me:) > > > > > > > > > > > > diff --git a/drivers/net/i40e/i40e_ethdev.h > > > > > b/drivers/net/i40e/i40e_ethdev.h index 934c679..020c5a2 100644 > > > > > --- a/drivers/net/i40e/i40e_ethdev.h > > > > > +++ b/drivers/net/i40e/i40e_ethdev.h > > > > > > > > > > /** > > > > > + * filter type of tunneling packet */ #define > > > > > +ETH_TUNNEL_FILTER_OMAC 0x01 /**< filter by outer MAC > > > addr > > > > */ > > > > > +#define ETH_TUNNEL_FILTER_OIP 0x02 /**< filter by outer IP Addr > */ > > > > > +#define ETH_TUNNEL_FILTER_TENID 0x04 /**< filter by tenant ID > > > > > +*/ #define ETH_TUNNEL_FILTER_IMAC 0x08 /**< filter by inner > > > > > +MAC addr > > > > */ > > > > > +#define ETH_TUNNEL_FILTER_IVLAN 0x10 /**< filter by inner VLAN > > > > > +ID > > > */ > > > > > +#define ETH_TUNNEL_FILTER_IIP 0x20 /**< filter by inner IP addr > */ > > > > Seems not necessary to redefine these macros. > > > > > > > > > +#define ETH_TUNNEL_FILTER_OVLAN 0x40 /**< filter by outer > VLAN > > > > > +ID > > > > */ > > > > > + > > > > > +#define I40E_TUNNEL_FILTER_IMAC_IVLAN > > > (ETH_TUNNEL_FILTER_IMAC > > > > | \ > > > > > + ETH_TUNNEL_FILTER_IVLAN) > > > > > +#define I40E_TUNNEL_FILTER_IMAC_IVLAN_TENID > > > > > (ETH_TUNNEL_FILTER_IMAC | \ > > > > > + ETH_TUNNEL_FILTER_IVLAN > | \ > > > > > + ETH_TUNNEL_FILTER_TENID) > > > > > +#define I40E_TUNNEL_FILTER_IMAC_TENID > > > (ETH_TUNNEL_FILTER_IMAC > > > > | \ > > > > > + ETH_TUNNEL_FILTER_TENID) > > > > > +#define I40E_TUNNEL_FILTER_OMAC_TENID_IMAC > > > > > (ETH_TUNNEL_FILTER_OMAC | \ > > > > > + ETH_TUNNEL_FILTER_TENID > | \ > > > > > + ETH_TUNNEL_FILTER_IMAC) > > > > I don't think it's necessary the redefine these macros either. > > > > Maybe we only need to add I40E_TUNNEL_FILTER_CUSTOM_QINQ. > > > > > > I will check this. > > > > These are now used in the i40e_ethdev code replacing RTE_* macros. > I think these macros are not NIC specific. Better define them in RTE. But if > it's > not the good time to add them to RTE, maybe the concern of ABI change. We > need only define ETH_TUNNEL_FILTER_OVLAN and > I40E_TUNNEL_FILTER_OMAC_TENID_IMAC here.
If I just define the above filters, then the following switch statement has mixed RTE and I40E macros, it seems cleaner to me to use all I40E macros. I can change back to mixed macros if you think it is better. static int i40e_dev_get_filter_type(uint16_t filter_type, uint16_t *flag) { switch (filter_type) { case I40E_TUNNEL_FILTER_IMAC_IVLAN: *flag = I40E_AQC_ADD_CLOUD_FILTER_IMAC_IVLAN; break; case I40E_TUNNEL_FILTER_IMAC_IVLAN_TENID: *flag = I40E_AQC_ADD_CLOUD_FILTER_IMAC_IVLAN_TEN_ID; break; case I40E_TUNNEL_FILTER_CUSTOM_QINQ: *flag = I40E_AQC_ADD_CLOUD_FILTER_CUSTOM_QINQ; break; case I40E_TUNNEL_FILTER_IMAC_TENID: *flag = I40E_AQC_ADD_CLOUD_FILTER_IMAC_TEN_ID; break; case I40E_TUNNEL_FILTER_OMAC_TENID_IMAC: *flag = I40E_AQC_ADD_CLOUD_FILTER_OMAC_TEN_ID_IMAC; break; case ETH_TUNNEL_FILTER_IMAC: *flag = I40E_AQC_ADD_CLOUD_FILTER_IMAC; break; case ETH_TUNNEL_FILTER_OIP: *flag = I40E_AQC_ADD_CLOUD_FILTER_OIP; break; case ETH_TUNNEL_FILTER_IIP: *flag = I40E_AQC_ADD_CLOUD_FILTER_IIP; break; default: PMD_DRV_LOG(ERR, "invalid tunnel filter type"); return -EINVAL; } return 0; > > > > > > > > > > > > +#define I40E_TUNNEL_FILTER_CUSTOM_QINQ > > > > (ETH_TUNNEL_FILTER_OMAC > > > > > | \ > > > > > + ETH_TUNNEL_FILTER_OVLAN > | > > > > > ETH_TUNNEL_FILTER_IVLAN) > > > > > + > > > > > +/** > > > > > * Tunneling Packet filter configuration. > > > > > */ > > > > > struct i40e_tunnel_filter_conf { > > > > > -- > > > > > 2.10.1 > > > > > > Thanks for the review. > > > > > > Regards, > > > > > > Bernard. Regards, Bernard.