Hi, Xiaolong > -----Original Message----- > From: Ye, Xiaolong <xiaolong...@intel.com> > Sent: Tuesday, April 14, 2020 3:37 PM > To: Su, Simei <simei...@intel.com> > Cc: Zhang, Qi Z <qi.z.zh...@intel.com>; Wu, Jingjing <jingjing...@intel.com>; > dev@dpdk.org; Cao, Yahui <yahui....@intel.com> > Subject: Re: [PATCH v3 1/5] net/iavf: add support for FDIR basic rule > > On 04/10, Simei Su wrote: > >This patch adds FDIR create/destroy/validate function in AVF. > >Common pattern and queue/qgroup/passthru/drop actions are supported. > > > >Signed-off-by: Simei Su <simei...@intel.com> > >--- > > doc/guides/rel_notes/release_20_05.rst | 1 + > > drivers/net/iavf/Makefile | 1 + > > drivers/net/iavf/iavf.h | 17 + > > drivers/net/iavf/iavf_fdir.c | 749 > +++++++++++++++++++++++++++++++++ > > drivers/net/iavf/iavf_vchnl.c | 154 ++++++- > > drivers/net/iavf/meson.build | 1 + > > 6 files changed, 922 insertions(+), 1 deletion(-) create mode 100644 > >drivers/net/iavf/iavf_fdir.c > > > >diff --git a/doc/guides/rel_notes/release_20_05.rst > >b/doc/guides/rel_notes/release_20_05.rst > >index b5962d8..17299ef 100644 > >--- a/doc/guides/rel_notes/release_20_05.rst > >+++ b/doc/guides/rel_notes/release_20_05.rst > >@@ -99,6 +99,7 @@ New Features > > > > * Added generic filter support. > > * Added advanced RSS configuration for VFs. > >+ * Added advanced iavf with FDIR capability. > > > > > > Removed Items > >diff --git a/drivers/net/iavf/Makefile b/drivers/net/iavf/Makefile > >index 7b0093a..b2b75d7 100644 > >--- a/drivers/net/iavf/Makefile > >+++ b/drivers/net/iavf/Makefile > >@@ -25,6 +25,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_IAVF_PMD) += > iavf_vchnl.c > > SRCS-$(CONFIG_RTE_LIBRTE_IAVF_PMD) += iavf_rxtx.c > > SRCS-$(CONFIG_RTE_LIBRTE_IAVF_PMD) += iavf_generic_flow.c > > SRCS-$(CONFIG_RTE_LIBRTE_IAVF_PMD) += iavf_hash.c > >+SRCS-$(CONFIG_RTE_LIBRTE_IAVF_PMD) += iavf_fdir.c > > ifeq ($(CONFIG_RTE_ARCH_X86), y) > > SRCS-$(CONFIG_RTE_LIBRTE_IAVF_PMD) += iavf_rxtx_vec_sse.c endif > diff > >--git a/drivers/net/iavf/iavf.h b/drivers/net/iavf/iavf.h index > >d813296..2f84a1f 100644 > >--- a/drivers/net/iavf/iavf.h > >+++ b/drivers/net/iavf/iavf.h > >@@ -92,6 +92,17 @@ struct iavf_vsi { > > struct iavf_flow_parser_node; > > TAILQ_HEAD(iavf_parser_list, iavf_flow_parser_node); > > > >+struct iavf_fdir_conf { > >+ struct virtchnl_fdir_add add_fltr; > >+ struct virtchnl_fdir_del del_fltr; > >+ uint64_t input_set; > >+ uint32_t flow_id; > >+}; > >+ > >+struct iavf_fdir_info { > >+ struct iavf_fdir_conf conf; > >+}; > > Why we need struct iavf_fdir_info since it has only one member?
In 20.05, it doesn't support flow director query counter. It will support query counter feature later, Which may need related counter config. So I write it in this form in case other configuration will be added in the future. > > >+ > > /* TODO: is that correct to assume the max number to be 16 ?*/ > > #define IAVF_MAX_MSIX_VECTORS 16 > > > >@@ -131,6 +142,8 @@ struct iavf_info { > > rte_spinlock_t flow_ops_lock; > > struct iavf_parser_list rss_parser_list; > > struct iavf_parser_list dist_parser_list; > >+ > >+ struct iavf_fdir_info fdir; /* flow director info */ > > }; > > > > #define IAVF_MAX_PKT_TYPE 1024 > >@@ -254,4 +267,8 @@ int iavf_add_del_eth_addr(struct iavf_adapter > >*adapter, int iavf_add_del_vlan(struct iavf_adapter *adapter, uint16_t > >vlanid, bool add); int iavf_add_del_rss_cfg(struct iavf_adapter *adapter, > > struct virtchnl_rss_cfg *rss_cfg, bool add); > >+int iavf_fdir_add(struct iavf_adapter *adapter, struct iavf_fdir_conf > >+*filter); int iavf_fdir_del(struct iavf_adapter *adapter, struct > >+iavf_fdir_conf *filter); int iavf_fdir_check(struct iavf_adapter *adapter, > >+ struct iavf_fdir_conf *filter); > > #endif /* _IAVF_ETHDEV_H_ */ > >diff --git a/drivers/net/iavf/iavf_fdir.c > >b/drivers/net/iavf/iavf_fdir.c new file mode 100644 index > >0000000..f2b10d7 > >--- /dev/null > >+++ b/drivers/net/iavf/iavf_fdir.c > >@@ -0,0 +1,749 @@ > >+/* SPDX-License-Identifier: BSD-3-Clause > >+ * Copyright(c) 2019 Intel Corporation > > Should be 2020. Yes, wrote it wrong. > > >+ */ > >+ > >+#include <sys/queue.h> > >+#include <stdio.h> > >+#include <errno.h> > >+#include <stdint.h> > >+#include <string.h> > >+#include <unistd.h> > >+#include <stdarg.h> > >+ > >+#include <rte_ether.h> > >+#include <rte_ethdev_driver.h> > >+#include <rte_malloc.h> > >+#include <rte_tailq.h> > >+ > >+#include "iavf.h" > >+#include "iavf_generic_flow.h" > >+#include "virtchnl.h" > >+ > >+#define IAVF_FDIR_MAX_QREGION_SIZE 128 > >+ > >+#define IAVF_FDIR_IPV6_TC_OFFSET 20 > >+#define IAVF_IPV6_TC_MASK (0xFF << IAVF_FDIR_IPV6_TC_OFFSET) > >+ > >+#define IAVF_FDIR_INSET_ETH (\ > >+ IAVF_INSET_ETHERTYPE) > >+ > >+#define IAVF_FDIR_INSET_ETH_IPV4 (\ > >+ IAVF_INSET_IPV4_SRC | IAVF_INSET_IPV4_DST | \ > >+ IAVF_INSET_IPV4_PROTO | IAVF_INSET_IPV4_TOS | \ > >+ IAVF_INSET_IPV4_TTL) > >+ > >+#define IAVF_FDIR_INSET_ETH_IPV4_UDP (\ > >+ IAVF_INSET_IPV4_SRC | IAVF_INSET_IPV4_DST | \ > >+ IAVF_INSET_IPV4_TOS | IAVF_INSET_IPV4_TTL | \ > >+ IAVF_INSET_UDP_SRC_PORT | IAVF_INSET_UDP_DST_PORT) > >+ > >+#define IAVF_FDIR_INSET_ETH_IPV4_TCP (\ > >+ IAVF_INSET_IPV4_SRC | IAVF_INSET_IPV4_DST | \ > >+ IAVF_INSET_IPV4_TOS | IAVF_INSET_IPV4_TTL | \ > >+ IAVF_INSET_TCP_SRC_PORT | IAVF_INSET_TCP_DST_PORT) > >+ > >+#define IAVF_FDIR_INSET_ETH_IPV4_SCTP (\ > >+ IAVF_INSET_IPV4_SRC | IAVF_INSET_IPV4_DST | \ > >+ IAVF_INSET_IPV4_TOS | IAVF_INSET_IPV4_TTL | \ > >+ IAVF_INSET_SCTP_SRC_PORT | IAVF_INSET_SCTP_DST_PORT) > >+ > >+#define IAVF_FDIR_INSET_ETH_IPV6 (\ > >+ IAVF_INSET_IPV6_SRC | IAVF_INSET_IPV6_DST | \ > >+ IAVF_INSET_IPV6_NEXT_HDR | IAVF_INSET_IPV6_TC | \ > >+ IAVF_INSET_IPV6_HOP_LIMIT) > >+ > >+#define IAVF_FDIR_INSET_ETH_IPV6_UDP (\ > >+ IAVF_INSET_IPV6_SRC | IAVF_INSET_IPV6_DST | \ > >+ IAVF_INSET_IPV6_TC | IAVF_INSET_IPV6_HOP_LIMIT | \ > >+ IAVF_INSET_UDP_SRC_PORT | IAVF_INSET_UDP_DST_PORT) > >+ > >+#define IAVF_FDIR_INSET_ETH_IPV6_TCP (\ > >+ IAVF_INSET_IPV6_SRC | IAVF_INSET_IPV6_DST | \ > >+ IAVF_INSET_IPV6_TC | IAVF_INSET_IPV6_HOP_LIMIT | \ > >+ IAVF_INSET_TCP_SRC_PORT | IAVF_INSET_TCP_DST_PORT) > >+ > >+#define IAVF_FDIR_INSET_ETH_IPV6_SCTP (\ > >+ IAVF_INSET_IPV6_SRC | IAVF_INSET_IPV6_DST | \ > >+ IAVF_INSET_IPV6_TC | IAVF_INSET_IPV6_HOP_LIMIT | \ > >+ IAVF_INSET_SCTP_SRC_PORT | IAVF_INSET_SCTP_DST_PORT) > >+ > >+static struct iavf_pattern_match_item iavf_fdir_pattern[] = { > >+ {iavf_pattern_ethertype, IAVF_FDIR_INSET_ETH, > IAVF_INSET_NONE}, > >+ {iavf_pattern_eth_ipv4, IAVF_FDIR_INSET_ETH_IPV4, > IAVF_INSET_NONE}, > >+ {iavf_pattern_eth_ipv4_udp, IAVF_FDIR_INSET_ETH_IPV4_UDP, > IAVF_INSET_NONE}, > >+ {iavf_pattern_eth_ipv4_tcp, IAVF_FDIR_INSET_ETH_IPV4_TCP, > IAVF_INSET_NONE}, > >+ {iavf_pattern_eth_ipv4_sctp, IAVF_FDIR_INSET_ETH_IPV4_SCTP, > IAVF_INSET_NONE}, > >+ {iavf_pattern_eth_ipv6, IAVF_FDIR_INSET_ETH_IPV6, > IAVF_INSET_NONE}, > >+ {iavf_pattern_eth_ipv6_udp, IAVF_FDIR_INSET_ETH_IPV6_UDP, > IAVF_INSET_NONE}, > >+ {iavf_pattern_eth_ipv6_tcp, IAVF_FDIR_INSET_ETH_IPV6_TCP, > IAVF_INSET_NONE}, > >+ {iavf_pattern_eth_ipv6_sctp, IAVF_FDIR_INSET_ETH_IPV6_SCTP, > IAVF_INSET_NONE}, > >+}; > >+ > >+static struct iavf_flow_parser iavf_fdir_parser; > >+ > >+static int > >+iavf_fdir_init(struct iavf_adapter *ad) { > >+ struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(ad); > >+ struct iavf_flow_parser *parser; > >+ > >+ if (vf->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_FDIR_PF) > > Need to check whether vf->vf_res is NULL, otherwise it may cause segfault. > Ok, I will think it over. > >+ parser = &iavf_fdir_parser; > >+ else > >+ return -ENOTSUP; > >+ > >+ return iavf_register_parser(parser, ad); } > >+ > >+static void > >+iavf_fdir_uninit(struct iavf_adapter *ad) { > >+ struct iavf_flow_parser *parser; > >+ > >+ parser = &iavf_fdir_parser; > >+ > >+ iavf_unregister_parser(parser, ad); > > Simplify to iavf_unregister_parser(&iavf_fdir_parser, ad) ? Ok, will simplify it. > > >+} > >+ > >+static int > >+iavf_fdir_create(struct iavf_adapter *ad, > >+ struct rte_flow *flow, > >+ void *meta, > >+ struct rte_flow_error *error) > >+{ > >+ struct iavf_fdir_conf *filter = meta; > >+ struct iavf_fdir_conf *rule; > >+ int ret; > >+ > >+ rule = rte_zmalloc("fdir_entry", sizeof(*rule), 0); > >+ if (!rule) { > >+ rte_flow_error_set(error, ENOMEM, > >+ RTE_FLOW_ERROR_TYPE_HANDLE, NULL, > >+ "Failed to allocate memory"); > > Better to be more specific, like "Failed to allocate memory for fdir rule." Ok, it makes sense. > > >+ return -rte_errno; > >+ } > >+ > >+ ret = iavf_fdir_add(ad, filter); > >+ if (ret) { > >+ rte_flow_error_set(error, -ret, > >+ RTE_FLOW_ERROR_TYPE_HANDLE, NULL, > >+ "Add filter rule failed."); > > What about "Failed to add filter rule" to make it consistent with other error > log. > And same for other error logs below. Ok, I will modify it. Thanks. > > > Thanks, > Xiaolong Br Simei