Hi Akhil,
Thank you.
Please see my response inline.
-----Original Message-----
From: Akhil Goyal <akhil.go...@nxp.com>
Sent: Thursday, April 16, 2020 12:30 AM
To: Shetty, Praveen <praveen.she...@intel.com>; dev@dpdk.org; Doherty, Declan
<declan.dohe...@intel.com>; ano...@marvell.com
Cc: Iremonger, Bernard <bernard.iremon...@intel.com>; Ananyev, Konstantin
<konstantin.anan...@intel.com>
Subject: RE: [PATCH v5] examples/ipsec-secgw: support flow director feature
Hi Praveen,
>
> Support load distribution in security gateway application using NIC
> load distribution feature(Flow Director).
> Flow Director is used to redirect the specified inbound ipsec flow to
> a specified queue.This is achieved by extending the SA rule syntax to
> support specification by adding new action_type of <flow-direction> to
> a specified <port_id> <queue_id>.
>
> Signed-off-by: Praveen Shetty <praveen.she...@intel.com>
Please fix a few minor comments below.
Apart from that,
Acked-by: Akhil Goyal <akhil.go...@nxp.com>
> ---
> v5 changes:
> Small change in condtional handling in sa.c file.
>
> v4 changes:
> 1. Removed flow director configuration changes as they are not
> required anymore.
> 2. Addressed all the review comments from Akhil and Anoob.
> 3. Included update to the ipsec-secgw user guide and update to release notes.
>
> doc/guides/rel_notes/release_20_05.rst | 4 ++
> doc/guides/sample_app_ug/ipsec_secgw.rst | 16 ++++++
> examples/ipsec-secgw/ep0.cfg | 11 ++++
> examples/ipsec-secgw/ipsec-secgw.c | 22 ++++++++
> examples/ipsec-secgw/ipsec.c | 67 ++++++++++++++++++++++++
> examples/ipsec-secgw/ipsec.h | 7 +++
> examples/ipsec-secgw/sa.c | 63 ++++++++++++++++++++--
> 7 files changed, 186 insertions(+), 4 deletions(-)
>
> diff --git a/doc/guides/rel_notes/release_20_05.rst
> b/doc/guides/rel_notes/release_20_05.rst
> index 6b1a7c58c..a9ec163cf 100644
> --- a/doc/guides/rel_notes/release_20_05.rst
> +++ b/doc/guides/rel_notes/release_20_05.rst
> @@ -81,6 +81,10 @@ New Features
> by making use of the event device capabilities. The event mode
> currently supports
> only inline IPsec protocol offload.
>
> +* **Updated ipsec-secgw application.**
> +
> + Added IPsec inbound load-distribution support for ipsec-secgw
> + application
> using NIC
> + load distribution feature(Flow Director).
There is one more item in release notes for ipsec-secgw for event mode.
You should add a bullet in that for this feature.
[Praveen] okay. Will do it in v6.
>
> Removed Items
> -------------
> diff --git a/doc/guides/sample_app_ug/ipsec_secgw.rst
> b/doc/guides/sample_app_ug/ipsec_secgw.rst
> index 038f593f4..12f921d76 100644
> --- a/doc/guides/sample_app_ug/ipsec_secgw.rst
> +++ b/doc/guides/sample_app_ug/ipsec_secgw.rst
> @@ -506,6 +506,7 @@ The SA rule syntax is shown as follows:
>
> sa <dir> <spi> <cipher_algo> <cipher_key> <auth_algo> <auth_key>
> <mode> <src_ip> <dst_ip> <action_type> <port_id> <fallback>
> + <flow-direction> <port_id> <queue_id>
>
> where each options means:
>
> @@ -698,6 +699,18 @@ where each options means:
>
> * *fallback lookaside-none*
>
> +``<flow-direction>``
> +
> + * Action type is for redirecting the specific inbound ipsec-flow to
> + a specified queue.
Flow direction is not an action type. You can re-phrase it as Option for
redirecting a specific inbound ipsec flow of a port to a specific queue of that
port.
[Praveen] okay. Will update this in v6.
> +
> + * Optional: Yes.
> +
> + * Available options:
> +
> + * *port_id*: Port ID of the NIC for which the SA is configured.
> + * *queue_id*: Queue ID to which traffic should be redirected.
> +
> Example SA rules:
>
> .. code-block:: console
> @@ -727,6 +740,9 @@ Example SA rules:
> mode ipv4-tunnel src 172.16.2.5 dst 172.16.1.5 \
> type inline-crypto-offload port_id 0
>
> + sa in 117 cipher_algo null auth_algo null mode ipv4-tunnel src
> 172.16.2.7 \
> + dst 172.16.1.7 flow-direction 0 2
> +
> Routing rule syntax
> ^^^^^^^^^^^^^^^^^^^
>
> diff --git a/examples/ipsec-secgw/ep0.cfg
> b/examples/ipsec-secgw/ep0.cfg index dfd4aca7d..6f8d5aa53 100644
> --- a/examples/ipsec-secgw/ep0.cfg
> +++ b/examples/ipsec-secgw/ep0.cfg
> @@ -29,6 +29,7 @@ sp ipv4 in esp protect 111 pri 1 dst
> 192.168.186.0/24 sport
> 0:65535 dport 0:6553
> sp ipv4 in esp protect 115 pri 1 dst 192.168.210.0/24 sport 0:65535
> dport
> 0:65535
> sp ipv4 in esp protect 116 pri 1 dst 192.168.211.0/24 sport 0:65535
> dport
> 0:65535
> sp ipv4 in esp protect 115 pri 1 dst 192.168.210.0/24 sport 0:65535
> dport
> 0:65535
> +sp ipv4 in esp protect 117 pri 1 dst 192.168.212.0/24 sport 0:65535
> +dport
> 0:65535
> sp ipv4 in esp protect 125 pri 1 dst 192.168.65.0/24 sport 0:65535
> dport 0:65535 sp ipv4 in esp protect 125 pri 1 dst 192.168.65.0/24
> sport 0:65535 dport 0:65535 sp ipv4 in esp protect 126 pri 1 dst
> 192.168.66.0/24 sport 0:65535 dport 0:65535 @@ -61,6 +62,8 @@ sp ipv6
> in esp protect 125 pri 1 dst
> ffff:0000:0000:0000:aaaa:aaaa:0000:0000/96
> sport 0:65535 dport 0:65535
> sp ipv6 in esp protect 126 pri 1 dst
> ffff:0000:0000:0000:bbbb:bbbb:0000:0000/96 \ sport 0:65535 dport
> 0:65535
> +sp ipv6 in esp protect 127 pri 1 dst
> ffff:0000:0000:0000:cccc:dddd:0000:0000/96 \
> +sport 0:65535 dport 0:65535
>
> #SA rules
> sa out 5 cipher_algo aes-128-cbc cipher_key
> 0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0 \ @@ -118,6 +121,9 @@ dst 172.16.1.5
>
> sa in 116 cipher_algo null auth_algo null mode ipv4-tunnel src
> 172.16.2.6 dst
> 172.16.1.6
>
> +sa in 117 cipher_algo null auth_algo null mode ipv4-tunnel src
> +172.16.2.7 \ dst 172.16.1.7 flow-direction 0 2
> +
> sa in 125 cipher_algo aes-128-cbc cipher_key
> c3:c3:c3:c3:c3:c3:c3:c3:c3:c3:c3:\
> c3:c3:c3:c3:c3 auth_algo sha1-hmac auth_key
> c3:c3:c3:c3:c3:c3:c3:c3:c3:c3:c3:\
> c3:c3:c3:c3:c3:c3:c3:c3:c3 mode ipv6-tunnel \ @@ -130,6 +136,11 @@ sa
> in 126 cipher_algo aes-128-cbc cipher_key
> 4d:4d:4d:4d:4d:4d:4d:4d:4d:4d:4d:\
> src 2222:2222:2222:2222:2222:2222:2222:6666 \ dst
> 1111:1111:1111:1111:1111:1111:1111:6666
>
> +sa in 127 cipher_algo null auth_algo null mode ipv6-tunnel \ src
> +2222:2222:2222:2222:2222:2222:2222:7777 \ dst
> +1111:1111:1111:1111:1111:1111:1111:7777 \ flow-direction 0 3
> +
> #Routing rules
> rt ipv4 dst 172.16.2.5/32 port 0
> rt ipv4 dst 172.16.2.6/32 port 1
> diff --git a/examples/ipsec-secgw/ipsec-secgw.c b/examples/ipsec-
> secgw/ipsec-secgw.c index 5fde4f728..6d02341de 100644
> --- a/examples/ipsec-secgw/ipsec-secgw.c
> +++ b/examples/ipsec-secgw/ipsec-secgw.c
> @@ -1183,6 +1183,28 @@ ipsec_poll_mode_worker(void)
> }
> }
>
> +int
> +check_flow_params(uint16_t fdir_portid, uint8_t fdir_qid) {
> + uint16_t i;
> + uint16_t portid;
> + uint8_t queueid;
> +
> + for (i = 0; i < nb_lcore_params; ++i) {
> + portid = lcore_params_array[i].port_id;
> + if (portid == fdir_portid) {
> + queueid = lcore_params_array[i].queue_id;
> + if (queueid == fdir_qid)
> + break;
> + }
> +
> + if (i == nb_lcore_params - 1)
> + return -1;
> + }
> +
> + return 1;
> +}
> +
> static int32_t
> check_poll_mode_params(struct eh_conf *eh_conf) { diff --git
> a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c index
> d40657102..461fae461 100644
> --- a/examples/ipsec-secgw/ipsec.c
> +++ b/examples/ipsec-secgw/ipsec.c
> @@ -418,6 +418,73 @@ create_inline_session(struct socket_ctx *skt_ctx,
> struct ipsec_sa *sa,
> return 0;
> }
>
> +int
> +create_ipsec_esp_flow(struct ipsec_sa *sa) {
> + int ret = 0;
> + struct rte_flow_error err;
> + if (sa->direction == RTE_SECURITY_IPSEC_SA_DIR_EGRESS) {
> + RTE_LOG(ERR, IPSEC,
> + "No Flow director rule for Egress traffic\n");
> + return -1;
> + }
> + if (sa->flags == TRANSPORT) {
> + RTE_LOG(ERR, IPSEC,
> + "No Flow director rule for transport mode\n");
> + return -1;
> + }
> + sa->action[0].type = RTE_FLOW_ACTION_TYPE_QUEUE;
> + sa->pattern[0].type = RTE_FLOW_ITEM_TYPE_ETH;
> + sa->action[0].conf =
> + &(struct rte_flow_action_queue){
> + .index = sa->fdir_qid,
> + };
Unnecessary line break, spacing before '{'
[Praveen] will fix this in v6.
> + sa->attr.egress = 0;
> + sa->attr.ingress = 1;
> + if (IS_IP6(sa->flags)) {
> + sa->pattern[1].mask = &rte_flow_item_ipv6_mask;
> + sa->pattern[1].type = RTE_FLOW_ITEM_TYPE_IPV6;
> + sa->pattern[1].spec = &sa->ipv6_spec;
> + memcpy(sa->ipv6_spec.hdr.dst_addr,
> + sa->dst.ip.ip6.ip6_b, sizeof(sa->dst.ip.ip6.ip6_b));
> + memcpy(sa->ipv6_spec.hdr.src_addr,
> + sa->src.ip.ip6.ip6_b, sizeof(sa->src.ip.ip6.ip6_b));
> + sa->pattern[2].type = RTE_FLOW_ITEM_TYPE_ESP;
> + sa->pattern[2].spec = &sa->esp_spec;
> + sa->pattern[2].mask = &rte_flow_item_esp_mask;
> + sa->esp_spec.hdr.spi = rte_cpu_to_be_32(sa->spi);
> + sa->pattern[3].type = RTE_FLOW_ITEM_TYPE_END;
> + } else if (IS_IP4(sa->flags)) {
> + sa->pattern[1].mask = &rte_flow_item_ipv4_mask;
> + sa->pattern[1].type = RTE_FLOW_ITEM_TYPE_IPV4;
> + sa->pattern[1].spec = &sa->ipv4_spec;
> + sa->ipv4_spec.hdr.dst_addr = sa->dst.ip.ip4;
> + sa->ipv4_spec.hdr.src_addr = sa->src.ip.ip4;
> + sa->pattern[2].type = RTE_FLOW_ITEM_TYPE_ESP;
> + sa->pattern[2].spec = &sa->esp_spec;
> + sa->pattern[2].mask = &rte_flow_item_esp_mask;
> + sa->esp_spec.hdr.spi = rte_cpu_to_be_32(sa->spi);
> + sa->pattern[3].type = RTE_FLOW_ITEM_TYPE_END;
> + }
> + sa->action[1].type = RTE_FLOW_ACTION_TYPE_END;
> +
> + ret = rte_flow_validate(sa->portid, &sa->attr, sa->pattern, sa->action,
> + &err);
> + if (ret < 0) {
> + RTE_LOG(ERR, IPSEC, "Flow validation failed %s\n",
> err.message);
> + return ret;
> + }
> +
> + sa->flow = rte_flow_create(sa->portid, &sa->attr, sa->pattern,
> + sa->action, &err);
> + if (!sa->flow) {
> + RTE_LOG(ERR, IPSEC, "Flow creation failed %s\n", err.message);
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> /*
> * queue crypto-ops into PMD queue.
> */
> diff --git a/examples/ipsec-secgw/ipsec.h
> b/examples/ipsec-secgw/ipsec.h index f8f29f9b1..1790a93ca 100644
> --- a/examples/ipsec-secgw/ipsec.h
> +++ b/examples/ipsec-secgw/ipsec.h
> @@ -144,6 +144,8 @@ struct ipsec_sa {
> };
> enum rte_security_ipsec_sa_direction direction;
> uint16_t portid;
> + uint8_t fdir_qid;
> + uint8_t fdir_flag;
>
> #define MAX_RTE_FLOW_PATTERN (4)
> #define MAX_RTE_FLOW_ACTIONS (3)
> @@ -408,5 +410,10 @@ create_lookaside_session(struct ipsec_ctx
> *ipsec_ctx, struct ipsec_sa *sa, int create_inline_session(struct
> socket_ctx *skt_ctx, struct ipsec_sa *sa,
> struct rte_ipsec_session *ips);
> +int
> +check_flow_params(uint16_t fdir_portid, uint8_t fdir_qid);
> +
> +int
> +create_ipsec_esp_flow(struct ipsec_sa *sa);
>
> #endif /* __IPSEC_H__ */
> diff --git a/examples/ipsec-secgw/sa.c b/examples/ipsec-secgw/sa.c
> index a6bf5e8b1..06bb25b3e 100644
> --- a/examples/ipsec-secgw/sa.c
> +++ b/examples/ipsec-secgw/sa.c
> @@ -271,6 +271,7 @@ parse_sa_tokens(char **tokens, uint32_t n_tokens,
> uint32_t type_p = 0;
> uint32_t portid_p = 0;
> uint32_t fallback_p = 0;
> + int16_t status_p = 0;
>
> if (strcmp(tokens[0], "in") == 0) {
> ri = &nb_sa_in;
> @@ -295,6 +296,7 @@ parse_sa_tokens(char **tokens, uint32_t n_tokens,
> if (atoi(tokens[1]) == INVALID_SPI)
> return;
> rule->spi = atoi(tokens[1]);
> + rule->portid = UINT16_MAX;
> ips = ipsec_get_primary_session(rule);
>
> for (ti = 2; ti < n_tokens; ti++) {
> @@ -636,9 +638,14 @@ parse_sa_tokens(char **tokens, uint32_t n_tokens,
> INCREMENT_TOKEN_INDEX(ti, n_tokens, status);
> if (status->status < 0)
> return;
> - rule->portid = atoi(tokens[ti]);
> - if (status->status < 0)
> + if (rule->portid == UINT16_MAX)
> + rule->portid = atoi(tokens[ti]);
> + else if (rule->portid != atoi(tokens[ti])) {
> + APP_CHECK(0, status, "portid %s not matching
> "
> + "with already assigned portid %u",
> + tokens[ti], rule->portid);
Please do not split strings which are printed as the output on console.
It will make it difficult to grep while debugging.
[Praveen] okay. Will fix this in v6.
> return;
> + }
> portid_p = 1;
> continue;
> }
> @@ -683,6 +690,44 @@ parse_sa_tokens(char **tokens, uint32_t n_tokens,
> fallback_p = 1;
> continue;
> }
> + if (strcmp(tokens[ti], "flow-direction") == 0) {
> + if (ips->type ==
> + RTE_SECURITY_ACTION_TYPE_NONE ||
> + ips->type ==
> + RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO) {
The code is not readable here.
Either a switch case or a separate function if the indentation it going beyond.
[Praveen] okay. Will do this in v6.
> + rule->fdir_flag = 1;
> + INCREMENT_TOKEN_INDEX(ti, n_tokens,
> status);
> + if (status->status < 0)
> + return;
> + if (rule->portid == UINT16_MAX)
> + rule->portid = atoi(tokens[ti]);
> + else if (rule->portid != atoi(tokens[ti])) {
> + APP_CHECK(0, status, "portid %s "
> + "not matching with already "
> + "assigned portid %u",
> + tokens[ti], rule->portid);
Same here, do not split strings which are printed.
You should have the complete string on a separate line. Checkpatch will not
trouble for more than 80.
[Praveen] Okay. Will do this in v6.
> + return;
> + }
> + INCREMENT_TOKEN_INDEX(ti, n_tokens,
> status);
> + if (status->status < 0)
> + return;
> + rule->fdir_qid = atoi(tokens[ti]);
> + /* validating portid and queueid */
> + status_p = check_flow_params(rule->portid,
> + rule->fdir_qid);
> + if (status_p < 0) {
> + printf("port id %u / queue id %u is "
> + "not valid\n", rule->portid,
> + rule->fdir_qid);
> + }
> + } else {
> + APP_CHECK(0, status, "flow director not "
> + "supported for security session "
> + "type:%d", ips->type);
> + return;
> + }
> + continue;
> + }
>
> /* unrecognizeable input */
> APP_CHECK(0, status, "unrecognized input \"%s\"", @@ -721,7
> +766,6
> @@ parse_sa_tokens(char **tokens, uint32_t n_tokens,
> if (!type_p || (!portid_p && ips->type !=
> RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO)) {
> ips->type = RTE_SECURITY_ACTION_TYPE_NONE;
> - rule->portid = -1;
> }
>
> *ri = *ri + 1;
> @@ -806,7 +850,7 @@ print_one_sa_rule(const struct ipsec_sa *sa, int inbound)
> printf("lookaside-protocol-offload ");
> break;
> case RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO:
> - printf("cpu-crypto-accelerated");
> + printf("cpu-crypto-accelerated ");
> break;
> }
>
> @@ -825,6 +869,10 @@ print_one_sa_rule(const struct ipsec_sa *sa, int
> inbound)
> break;
> }
> }
> + if (sa->fdir_flag == 1)
> + printf("flow-direction port %d queue %d", sa->portid,
> + sa->fdir_qid);
> +
> printf("\n");
> }
>
> @@ -1143,6 +1191,13 @@ sa_add_rules(struct sa_ctx *sa_ctx, const
> struct ipsec_sa entries[],
> }
> }
>
> + if (sa->fdir_flag && inbound) {
> + rc = create_ipsec_esp_flow(sa);
> + if (rc != 0)
> + RTE_LOG(ERR, IPSEC_ESP,
> + "create_ipsec_esp_flow() failed %s\n",
> + strerror(rc));
> + }
> print_one_sa_rule(sa, inbound);
> }
>
> --
> 2.17.1
Regards,
Praveen
