Message ID | 20231214043449.15835-3-lukasz.plachno@intel.com (mailing list archive) |
---|---|
State | Awaiting Upstream |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | ice: Support flow director ether type filters | expand |
On Thu, Dec 14, 2023 at 05:34:49AM +0100, Lukasz Plachno wrote: ... > @@ -1199,6 +1212,99 @@ ice_set_fdir_ip6_usr_seg(struct ice_flow_seg_info *seg, > return 0; > } > > +/** > + * ice_fdir_vlan_valid - validate VLAN data for Flow Director rule > + * @fsp: pointer to ethtool Rx flow specification > + * > + * Return: true if vlan data is valid, false otherwise > + */ > +static bool ice_fdir_vlan_valid(struct ethtool_rx_flow_spec *fsp) > +{ > + if (fsp->m_ext.vlan_etype && > + ntohs(fsp->h_ext.vlan_etype) & ~(ETH_P_8021Q | ETH_P_8021AD)) > + return false; Hi Jakub and Lukasz, It is not obvious to me that a bitwise comparison of the vlan_ethtype is correct. Possibly naively I expected something more like (completely untested!): if (!eth_type_vlan(sp->m_ext.vlan_etype)) return false: > + > + if (fsp->m_ext.vlan_tci && > + ntohs(fsp->h_ext.vlan_tci) >= VLAN_N_VID) > + return false; > + > + return true; > +} ...
On 12/16/2023 11:03 AM, Simon Horman wrote: > On Thu, Dec 14, 2023 at 05:34:49AM +0100, Lukasz Plachno wrote: > > ... > >> @@ -1199,6 +1212,99 @@ ice_set_fdir_ip6_usr_seg(struct ice_flow_seg_info *seg, >> return 0; >> } >> >> +/** >> + * ice_fdir_vlan_valid - validate VLAN data for Flow Director rule >> + * @fsp: pointer to ethtool Rx flow specification >> + * >> + * Return: true if vlan data is valid, false otherwise >> + */ >> +static bool ice_fdir_vlan_valid(struct ethtool_rx_flow_spec *fsp) >> +{ >> + if (fsp->m_ext.vlan_etype && >> + ntohs(fsp->h_ext.vlan_etype) & ~(ETH_P_8021Q | ETH_P_8021AD)) >> + return false; > > Hi Jakub and Lukasz, > > It is not obvious to me that a bitwise comparison of the vlan_ethtype is > correct. Possibly naively I expected something more like > (completely untested!): > > if (!eth_type_vlan(sp->m_ext.vlan_etype)) > return false: > >> + >> + if (fsp->m_ext.vlan_tci && >> + ntohs(fsp->h_ext.vlan_tci) >= VLAN_N_VID) >> + return false; >> + >> + return true; >> +} eth_type_vlan() does what is needed here and is much more readable, I will switch to it in V4 Thanks, Łukasz
On 12/13/2023 8:34 PM, Lukasz Plachno wrote: > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding. > > > From: Jakub Buchocki <jakubx.buchocki@intel.com> > > Add support for 'flow-type ether' Flow Director rules via ethtool. > > Rules not containing masks are processed by the Flow Director, > and support the following set of input parameters in all combinations: > src, dst, proto, vlan-etype, vlan, action. > > It is possible to specify address mask in ethtool parameters but only > 00:00:00:00:00 and FF:FF:FF:FF:FF are valid. > The same applies to proto, vlan-etype and vlan masks: > only 0x0000 and 0xffff masks are valid. Would it be useful to have user facing error messages for invalid masks stating what the valid masks are? > > Signed-off-by: Jakub Buchocki <jakubx.buchocki@intel.com> > Co-developed-by: Mateusz Pacuszka <mateuszx.pacuszka@intel.com> > Signed-off-by: Mateusz Pacuszka <mateuszx.pacuszka@intel.com> > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > Signed-off-by: Lukasz Plachno <lukasz.plachno@intel.com> > --- > .../net/ethernet/intel/ice/ice_ethtool_fdir.c | 128 +++++++++++++++++- > drivers/net/ethernet/intel/ice/ice_fdir.c | 27 ++++ > drivers/net/ethernet/intel/ice/ice_fdir.h | 11 ++ > drivers/net/ethernet/intel/ice/ice_type.h | 1 + > 4 files changed, 166 insertions(+), 1 deletion(-) > [...]
On 12/19/2023 6:35 PM, Brett Creeley wrote: > > > On 12/13/2023 8:34 PM, Lukasz Plachno wrote: >> Caution: This message originated from an External Source. Use proper >> caution when opening attachments, clicking links, or responding. >> >> >> From: Jakub Buchocki <jakubx.buchocki@intel.com> >> >> Add support for 'flow-type ether' Flow Director rules via ethtool. >> >> Rules not containing masks are processed by the Flow Director, >> and support the following set of input parameters in all combinations: >> src, dst, proto, vlan-etype, vlan, action. >> >> It is possible to specify address mask in ethtool parameters but only >> 00:00:00:00:00 and FF:FF:FF:FF:FF are valid. >> The same applies to proto, vlan-etype and vlan masks: >> only 0x0000 and 0xffff masks are valid. > > Would it be useful to have user facing error messages for invalid masks > stating what the valid masks are? > Do you think about something like: dev_warn("Driver only supports masks 00:00:00:00:00:00 and FF:FF:FF:FF:FF:FF"), or there is a way to pass custom message to ethtool to print it to user? >> >> Signed-off-by: Jakub Buchocki <jakubx.buchocki@intel.com> >> Co-developed-by: Mateusz Pacuszka <mateuszx.pacuszka@intel.com> >> Signed-off-by: Mateusz Pacuszka <mateuszx.pacuszka@intel.com> >> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> >> Signed-off-by: Lukasz Plachno <lukasz.plachno@intel.com> >> --- >> .../net/ethernet/intel/ice/ice_ethtool_fdir.c | 128 +++++++++++++++++- >> drivers/net/ethernet/intel/ice/ice_fdir.c | 27 ++++ >> drivers/net/ethernet/intel/ice/ice_fdir.h | 11 ++ >> drivers/net/ethernet/intel/ice/ice_type.h | 1 + >> 4 files changed, 166 insertions(+), 1 deletion(-) >> > > [...]
On 12/20/2023 6:19 AM, Plachno, Lukasz wrote: > Caution: This message originated from an External Source. Use proper > caution when opening attachments, clicking links, or responding. > > > On 12/19/2023 6:35 PM, Brett Creeley wrote: >> >> >> On 12/13/2023 8:34 PM, Lukasz Plachno wrote: >>> Caution: This message originated from an External Source. Use proper >>> caution when opening attachments, clicking links, or responding. >>> >>> >>> From: Jakub Buchocki <jakubx.buchocki@intel.com> >>> >>> Add support for 'flow-type ether' Flow Director rules via ethtool. >>> >>> Rules not containing masks are processed by the Flow Director, >>> and support the following set of input parameters in all combinations: >>> src, dst, proto, vlan-etype, vlan, action. >>> >>> It is possible to specify address mask in ethtool parameters but only >>> 00:00:00:00:00 and FF:FF:FF:FF:FF are valid. >>> The same applies to proto, vlan-etype and vlan masks: >>> only 0x0000 and 0xffff masks are valid. >> >> Would it be useful to have user facing error messages for invalid masks >> stating what the valid masks are? >> > > Do you think about something like: > dev_warn("Driver only supports masks 00:00:00:00:00:00 and > FF:FF:FF:FF:FF:FF"), > or there is a way to pass custom message to ethtool to print it to user? Using a dev_err()/dev_warn() was more along the lines of what I was thinking. Brett > >>> >>> Signed-off-by: Jakub Buchocki <jakubx.buchocki@intel.com> >>> Co-developed-by: Mateusz Pacuszka <mateuszx.pacuszka@intel.com> >>> Signed-off-by: Mateusz Pacuszka <mateuszx.pacuszka@intel.com> >>> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> >>> Signed-off-by: Lukasz Plachno <lukasz.plachno@intel.com> >>> --- >>> .../net/ethernet/intel/ice/ice_ethtool_fdir.c | 128 +++++++++++++++++- >>> drivers/net/ethernet/intel/ice/ice_fdir.c | 27 ++++ >>> drivers/net/ethernet/intel/ice/ice_fdir.h | 11 ++ >>> drivers/net/ethernet/intel/ice/ice_type.h | 1 + >>> 4 files changed, 166 insertions(+), 1 deletion(-) >>> >> >> [...]
diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool_fdir.c b/drivers/net/ethernet/intel/ice/ice_ethtool_fdir.c index d151e5bacfec..6951a5b02ac7 100644 --- a/drivers/net/ethernet/intel/ice/ice_ethtool_fdir.c +++ b/drivers/net/ethernet/intel/ice/ice_ethtool_fdir.c @@ -41,6 +41,8 @@ static struct in6_addr zero_ipv6_addr_mask = { static int ice_fltr_to_ethtool_flow(enum ice_fltr_ptype flow) { switch (flow) { + case ICE_FLTR_PTYPE_NONF_ETH: + return ETHER_FLOW; case ICE_FLTR_PTYPE_NONF_IPV4_TCP: return TCP_V4_FLOW; case ICE_FLTR_PTYPE_NONF_IPV4_UDP: @@ -72,6 +74,8 @@ static int ice_fltr_to_ethtool_flow(enum ice_fltr_ptype flow) static enum ice_fltr_ptype ice_ethtool_flow_to_fltr(int eth) { switch (eth) { + case ETHER_FLOW: + return ICE_FLTR_PTYPE_NONF_ETH; case TCP_V4_FLOW: return ICE_FLTR_PTYPE_NONF_IPV4_TCP; case UDP_V4_FLOW: @@ -137,6 +141,15 @@ int ice_get_ethtool_fdir_entry(struct ice_hw *hw, struct ethtool_rxnfc *cmd) memset(&fsp->m_ext, 0, sizeof(fsp->m_ext)); switch (fsp->flow_type) { + case ETHER_FLOW: + fsp->h_u.ether_spec.h_proto = rule->eth.type; + fsp->m_u.ether_spec.h_proto = rule->eth_mask.type; + ether_addr_copy(fsp->h_u.ether_spec.h_dest, rule->eth.dst); + ether_addr_copy(fsp->m_u.ether_spec.h_dest, rule->eth_mask.dst); + ether_addr_copy(fsp->h_u.ether_spec.h_source, rule->eth.src); + ether_addr_copy(fsp->m_u.ether_spec.h_source, + rule->eth_mask.src); + break; case IPV4_USER_FLOW: fsp->h_u.usr_ip4_spec.ip_ver = ETH_RX_NFC_IP4; fsp->h_u.usr_ip4_spec.proto = 0; @@ -1199,6 +1212,99 @@ ice_set_fdir_ip6_usr_seg(struct ice_flow_seg_info *seg, return 0; } +/** + * ice_fdir_vlan_valid - validate VLAN data for Flow Director rule + * @fsp: pointer to ethtool Rx flow specification + * + * Return: true if vlan data is valid, false otherwise + */ +static bool ice_fdir_vlan_valid(struct ethtool_rx_flow_spec *fsp) +{ + if (fsp->m_ext.vlan_etype && + ntohs(fsp->h_ext.vlan_etype) & ~(ETH_P_8021Q | ETH_P_8021AD)) + return false; + + if (fsp->m_ext.vlan_tci && + ntohs(fsp->h_ext.vlan_tci) >= VLAN_N_VID) + return false; + + return true; +} + +/** + * ice_set_ether_flow_seg + * @seg: flow segment for programming + * @eth_spec: mask data from ethtool + * + * Return: 0 on success and errno in case of error. + */ +static int ice_set_ether_flow_seg(struct ice_flow_seg_info *seg, + struct ethhdr *eth_spec) +{ + ICE_FLOW_SET_HDRS(seg, ICE_FLOW_SEG_HDR_ETH); + + /* Ethertype */ + if (eth_spec->h_proto == htons(0xFFFF)) + ice_flow_set_fld(seg, ICE_FLOW_FIELD_IDX_ETH_TYPE, + ICE_FLOW_FLD_OFF_INVAL, + ICE_FLOW_FLD_OFF_INVAL, + ICE_FLOW_FLD_OFF_INVAL, false); + else if (eth_spec->h_proto) + return -EOPNOTSUPP; + + /* Source MAC address */ + if (is_broadcast_ether_addr(eth_spec->h_source)) + ice_flow_set_fld(seg, ICE_FLOW_FIELD_IDX_ETH_SA, + ICE_FLOW_FLD_OFF_INVAL, ICE_FLOW_FLD_OFF_INVAL, + ICE_FLOW_FLD_OFF_INVAL, false); + else if (!is_zero_ether_addr(eth_spec->h_source)) + return -EOPNOTSUPP; + + /* Destination MAC address */ + if (is_broadcast_ether_addr(eth_spec->h_dest)) + ice_flow_set_fld(seg, ICE_FLOW_FIELD_IDX_ETH_DA, + ICE_FLOW_FLD_OFF_INVAL, ICE_FLOW_FLD_OFF_INVAL, + ICE_FLOW_FLD_OFF_INVAL, false); + else if (!is_zero_ether_addr(eth_spec->h_dest)) + return -EOPNOTSUPP; + + return 0; +} + +/** + * ice_set_fdir_vlan_seg + * @seg: flow segment for programming + * @ext_masks: masks for additional RX flow fields + */ +static int +ice_set_fdir_vlan_seg(struct ice_flow_seg_info *seg, + struct ethtool_flow_ext *ext_masks) +{ + ICE_FLOW_SET_HDRS(seg, ICE_FLOW_SEG_HDR_VLAN); + + if (ext_masks->vlan_etype) { + if (ext_masks->vlan_etype != htons(0xFFFF)) + return -EOPNOTSUPP; + + ice_flow_set_fld(seg, ICE_FLOW_FIELD_IDX_S_VLAN, + ICE_FLOW_FLD_OFF_INVAL, + ICE_FLOW_FLD_OFF_INVAL, + ICE_FLOW_FLD_OFF_INVAL, false); + } + + if (ext_masks->vlan_tci) { + if (ext_masks->vlan_tci != htons(0xFFFF)) + return -EOPNOTSUPP; + + ice_flow_set_fld(seg, ICE_FLOW_FIELD_IDX_C_VLAN, + ICE_FLOW_FLD_OFF_INVAL, + ICE_FLOW_FLD_OFF_INVAL, + ICE_FLOW_FLD_OFF_INVAL, false); + } + + return 0; +} + /** * ice_cfg_fdir_xtrct_seq - Configure extraction sequence for the given filter * @pf: PF structure @@ -1215,7 +1321,7 @@ ice_cfg_fdir_xtrct_seq(struct ice_pf *pf, struct ethtool_rx_flow_spec *fsp, struct device *dev = ice_pf_to_dev(pf); enum ice_fltr_ptype fltr_idx; struct ice_hw *hw = &pf->hw; - bool perfect_filter; + bool perfect_filter = false; int ret; seg = devm_kzalloc(dev, sizeof(*seg), GFP_KERNEL); @@ -1268,6 +1374,16 @@ ice_cfg_fdir_xtrct_seq(struct ice_pf *pf, struct ethtool_rx_flow_spec *fsp, ret = ice_set_fdir_ip6_usr_seg(seg, &fsp->m_u.usr_ip6_spec, &perfect_filter); break; + case ETHER_FLOW: + ret = ice_set_ether_flow_seg(seg, &fsp->m_u.ether_spec); + if (!ret && (fsp->m_ext.vlan_etype || fsp->m_ext.vlan_tci)) { + if (!ice_fdir_vlan_valid(fsp)) { + ret = -EINVAL; + break; + } + ret = ice_set_fdir_vlan_seg(seg, &fsp->m_ext); + } + break; default: ret = -EINVAL; } @@ -1829,6 +1945,16 @@ ice_set_fdir_input_set(struct ice_vsi *vsi, struct ethtool_rx_flow_spec *fsp, input->mask.v6.tc = fsp->m_u.usr_ip6_spec.tclass; input->mask.v6.proto = fsp->m_u.usr_ip6_spec.l4_proto; break; + case ETHER_FLOW: + ether_addr_copy(input->eth.dst, fsp->h_u.ether_spec.h_dest); + ether_addr_copy(input->eth.src, fsp->h_u.ether_spec.h_source); + ether_addr_copy(input->eth_mask.dst, + fsp->m_u.ether_spec.h_dest); + ether_addr_copy(input->eth_mask.src, + fsp->m_u.ether_spec.h_source); + input->eth.type = fsp->h_u.ether_spec.h_proto; + input->eth_mask.type = fsp->m_u.ether_spec.h_proto; + break; default: /* not doing un-parsed flow types */ return -EINVAL; diff --git a/drivers/net/ethernet/intel/ice/ice_fdir.c b/drivers/net/ethernet/intel/ice/ice_fdir.c index d6053cdad686..790f28ccce35 100644 --- a/drivers/net/ethernet/intel/ice/ice_fdir.c +++ b/drivers/net/ethernet/intel/ice/ice_fdir.c @@ -4,6 +4,8 @@ #include "ice_common.h" /* These are training packet headers used to program flow director filters. */ +static const u8 ice_fdir_eth_pkt[22] = {0}; + static const u8 ice_fdir_tcpv4_pkt[] = { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x08, 0x00, 0x45, 0x00, @@ -416,6 +418,11 @@ static const u8 ice_fdir_ip6_tun_pkt[] = { /* Flow Director no-op training packet table */ static const struct ice_fdir_base_pkt ice_fdir_pkt[] = { + { + ICE_FLTR_PTYPE_NONF_ETH, + sizeof(ice_fdir_eth_pkt), ice_fdir_eth_pkt, + sizeof(ice_fdir_eth_pkt), ice_fdir_eth_pkt, + }, { ICE_FLTR_PTYPE_NONF_IPV4_TCP, sizeof(ice_fdir_tcpv4_pkt), ice_fdir_tcpv4_pkt, @@ -937,6 +944,22 @@ ice_fdir_get_gen_prgm_pkt(struct ice_hw *hw, struct ice_fdir_fltr *input, * perspective. The input from user is from Rx filter perspective. */ switch (flow) { + case ICE_FLTR_PTYPE_NONF_ETH: + ice_pkt_insert_mac_addr(loc, input->eth.dst); + ice_pkt_insert_mac_addr(loc + ETH_ALEN, input->eth.src); + if (input->ext_data.vlan_tag || + input->ext_data.vlan_type) { + ice_pkt_insert_u16(loc, ICE_ETH_TYPE_F_OFFSET, + input->ext_data.vlan_type); + ice_pkt_insert_u16(loc, ICE_ETH_VLAN_TCI_OFFSET, + input->ext_data.vlan_tag); + ice_pkt_insert_u16(loc, ICE_ETH_TYPE_VLAN_OFFSET, + input->eth.type); + } else { + ice_pkt_insert_u16(loc, ICE_ETH_TYPE_F_OFFSET, + input->eth.type); + } + break; case ICE_FLTR_PTYPE_NONF_IPV4_TCP: ice_pkt_insert_u32(loc, ICE_IPV4_DST_ADDR_OFFSET, input->ip.v4.src_ip); @@ -1224,6 +1247,10 @@ ice_fdir_comp_rules(struct ice_fdir_fltr *a, struct ice_fdir_fltr *b) * same flow_type. */ switch (flow_type) { + case ICE_FLTR_PTYPE_NONF_ETH: + if (!memcmp(&a->eth, &b->eth, sizeof(a->eth))) + return true; + break; case ICE_FLTR_PTYPE_NONF_IPV4_TCP: case ICE_FLTR_PTYPE_NONF_IPV4_UDP: case ICE_FLTR_PTYPE_NONF_IPV4_SCTP: diff --git a/drivers/net/ethernet/intel/ice/ice_fdir.h b/drivers/net/ethernet/intel/ice/ice_fdir.h index 1b9b84490689..0c90865a36c5 100644 --- a/drivers/net/ethernet/intel/ice/ice_fdir.h +++ b/drivers/net/ethernet/intel/ice/ice_fdir.h @@ -8,6 +8,9 @@ #define ICE_FDIR_MAX_RAW_PKT_SIZE (512 + ICE_FDIR_TUN_PKT_OFF) /* macros for offsets into packets for flow director programming */ +#define ICE_ETH_TYPE_F_OFFSET 12 +#define ICE_ETH_VLAN_TCI_OFFSET 14 +#define ICE_ETH_TYPE_VLAN_OFFSET 16 #define ICE_IPV4_SRC_ADDR_OFFSET 26 #define ICE_IPV4_DST_ADDR_OFFSET 30 #define ICE_IPV4_TCP_SRC_PORT_OFFSET 34 @@ -97,6 +100,12 @@ struct ice_rx_flow_userdef { u16 flex_fltr; }; +struct ice_fdir_eth { + u8 dst[ETH_ALEN]; + u8 src[ETH_ALEN]; + __be16 type; +}; + struct ice_fdir_v4 { __be32 dst_ip; __be32 src_ip; @@ -159,6 +168,8 @@ struct ice_fdir_fltr { struct list_head fltr_node; enum ice_fltr_ptype flow_type; + struct ice_fdir_eth eth, eth_mask; + union { struct ice_fdir_v4 v4; struct ice_fdir_v6 v6; diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h index a18ca0ff879f..4311b14ab3b8 100644 --- a/drivers/net/ethernet/intel/ice/ice_type.h +++ b/drivers/net/ethernet/intel/ice/ice_type.h @@ -202,6 +202,7 @@ struct ice_phy_info { enum ice_fltr_ptype { /* NONE - used for undef/error */ ICE_FLTR_PTYPE_NONF_NONE = 0, + ICE_FLTR_PTYPE_NONF_ETH, ICE_FLTR_PTYPE_NONF_IPV4_UDP, ICE_FLTR_PTYPE_NONF_IPV4_TCP, ICE_FLTR_PTYPE_NONF_IPV4_SCTP,