Re: [dpdk-dev] [PATCH v3 1/5] net/iavf: add support for FDIR basic rule

["Su, Simei" <simei.su@intel.com>]

Hi, Xiaolong

> -----Original Message-----
> From: Ye, Xiaolong <xiaolong.ye@intel.com>
> Sent: Tuesday, April 14, 2020 3:37 PM
> To: Su, Simei <simei.su@intel.com>
> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
> dev@dpdk.org; Cao, Yahui <yahui.cao@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.su@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