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

[Ye Xiaolong <xiaolong.ye@intel.com>]

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?

>+
> /* 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.

>+ */
>+
>+#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.

>+		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) ?

>+}
>+
>+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."

>+		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.


Thanks,
Xiaolong