[dpdk-dev] [PATCH v3 1/5] net/iavf: add support for FDIR basic rule
Su, Simei
simei.su at intel.com
Tue Apr 14 10:31:55 CEST 2020
Hi, Xiaolong
> -----Original Message-----
> From: Ye, Xiaolong <xiaolong.ye at intel.com>
> Sent: Tuesday, April 14, 2020 3:37 PM
> To: Su, Simei <simei.su at intel.com>
> Cc: Zhang, Qi Z <qi.z.zhang at intel.com>; Wu, Jingjing <jingjing.wu at intel.com>;
> dev at dpdk.org; Cao, Yahui <yahui.cao at 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 at 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
More information about the dev
mailing list