Hi Anoob, Thank you.
Please see my answers below. Regards, Praveen -----Original Message----- From: Anoob Joseph <ano...@marvell.com> Sent: Thursday, March 12, 2020 4:31 PM To: Shetty, Praveen <praveen.she...@intel.com>; dev@dpdk.org; Doherty, Declan <declan.dohe...@intel.com>; Iremonger, Bernard <bernard.iremon...@intel.com>; Ananyev, Konstantin <konstantin.anan...@intel.com> Subject: RE: [dpdk-dev] [PATCH v1] examples/ipsec-secgw: support flow director feature Hi Praveen, I do have some review comments on the code. Before that, can you give a brief overview of what is being targeted? My understanding is that the primary objective is to use rte_flow (or flow director) to redirect a specific flow(/SA) to a specific queue. Can you confirm? >>>> Yes, your understanding is correct, the main objective is to support load >>>> distribution in ipsec-secgw application. >>>> flow director and RSS features are used achieve the load distribution. >>>> flow director is used to redirect the specified inbound ipsec flow to a >>>> specified queue. Couple of questions, 1. I would assume the new option of "flow-direction" is optional and is determined per SA. In that case, can I assume that RSS would be active for the other flows (or SAs). Let's say, I just want to add a SA for which I would like to enable "flow-direction" but leave the rest as is. How is that handled? [Praveen] >>>> We are using fdir_flag to differentiate the mix of SA's(SA's with and >>>> without flow-direction). >>>> fdir_flag will be "set" for the SA which has configured with >>>> flow-direction option(SA rule syntax is extended to add new options >>>> <action_type> <portid> <queueid> ). >>>> flow creation is called only for the SA's with fdir_flag is set. 2. I see that the changes are only applicable for LOOKASIDE_PROTOCOL. The same feature would be useful for other modes as well, right? [Praveen] >>>> We are adding this feature for i40e NIC and the i40e NIC doesn't support >>>> either encryption or decryption, that's why we used only >>>> LOOKASIDE_PROTOCOL in this case. 3. I'm not sure "flow-direction" is the right wording for the option. This is just specifying the "rx-queue" per SA. @Akhil, Konstantin, comments? >>>> @Declan, @Konstantin , @Bernard, @Akhil Could you please suggest a name >>>> on which we can all agree upon? Thanks, Anoob > -----Original Message----- > From: dev <dev-boun...@dpdk.org> On Behalf Of Praveen Shetty > Sent: Wednesday, March 11, 2020 8:25 PM > To: dev@dpdk.org; declan.dohe...@intel.com; > bernard.iremon...@intel.com; konstantin.anan...@intel.com > Subject: [dpdk-dev] [PATCH v1] examples/ipsec-secgw: support flow > director feature > > Modified Secuirty gateway application to support configuration of flow > director rule to direct inbound IPsec SA to a specified queue. > > Signed-off-by: Praveen Shetty <praveen.she...@intel.com> > --- > examples/ipsec-secgw/ep0.cfg | 11 +++++ > examples/ipsec-secgw/ipsec-secgw.c | 56 ++++++++++++++++++++++++- > examples/ipsec-secgw/ipsec.c | 67 ++++++++++++++++++++++++++++++ > examples/ipsec-secgw/ipsec.h | 11 +++++ > examples/ipsec-secgw/sa.c | 50 +++++++++++++++++++++- > 5 files changed, 192 insertions(+), 3 deletions(-) > > diff --git a/examples/ipsec-secgw/ep0.cfg > b/examples/ipsec-secgw/ep0.cfg index dfd4aca7d..c9f80e81b 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 port_id 0 type > +lookaside-protocol-offload > + > 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 port_id > +0 type lookaside-protocol-offload > + > #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 4799bc90c..132484422 100644 > --- a/examples/ipsec-secgw/ipsec-secgw.c > +++ b/examples/ipsec-secgw/ipsec-secgw.c > @@ -166,7 +166,6 @@ static const struct option lgopts[] = { > {CMD_LINE_OPT_FRAG_TTL, 1, 0, CMD_LINE_OPT_FRAG_TTL_NUM}, > {NULL, 0, 0, 0} > }; > - > /* mask of enabled ports */ > static uint32_t enabled_port_mask; > static uint64_t enabled_cryptodev_mask = UINT64_MAX; @@ -259,6 > +258,30 @@ static struct rte_eth_conf port_conf = { > .txmode = { > .mq_mode = ETH_MQ_TX_NONE, > }, > + .fdir_conf = { > + .mode = RTE_FDIR_MODE_NONE, > + .pballoc = RTE_FDIR_PBALLOC_64K, > + .status = RTE_FDIR_REPORT_STATUS, > + .mask = { > + .vlan_tci_mask = 0xFFEF, > + .ipv4_mask = { > + .src_ip = 0xFFFFFFFF, > + .dst_ip = 0xFFFFFFFF, > + }, > + .ipv6_mask = { > + .src_ip = {0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, > + 0xFFFFFFFF}, > + .dst_ip = {0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, > + 0xFFFFFFFF}, > + }, > + .src_port_mask = 0xFFFF, > + .dst_port_mask = 0xFFFF, > + .mac_addr_byte_mask = 0xFF, > + .tunnel_type_mask = 1, > + .tunnel_id_mask = 0xFFFFFFFF, > + }, > + .drop_queue = 127, > + } > }; > > static struct socket_ctx socket_ctx[NB_SOCKETS]; @@ -1184,7 +1207,6 > @@ > main_loop(__attribute__((unused)) void *dummy) > > if (nb_rx > 0) > process_pkts(qconf, pkts, nb_rx, portid); > - > /* dequeue and process completed crypto-ops */ > if (UNPROTECTED_PORT(portid)) > drain_inbound_crypto_queues(qconf, > @@ -1196,6 +1218,27 @@ main_loop(__attribute__((unused)) void *dummy) > } > } > > +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_params(void) > { > @@ -2503,6 +2546,15 @@ main(int32_t argc, char **argv) > continue; > > sa_check_offloads(portid, &req_rx_offloads, &req_tx_offloads); > + /* check if FDIR is configured on the port */ > + if (check_fdir_configured(portid)) { > + /* Enable FDIR */ > + port_conf.fdir_conf.mode = > RTE_FDIR_MODE_PERFECT; > + /* Disable RSS */ > + port_conf.rxmode.mq_mode = ETH_MQ_RX_NONE; > + port_conf.rx_adv_conf.rss_conf.rss_hf = 0; > + port_conf.rx_adv_conf.rss_conf.rss_key = NULL; > + } > port_init(portid, req_rx_offloads, req_tx_offloads); > } > > diff --git a/examples/ipsec-secgw/ipsec.c > b/examples/ipsec-secgw/ipsec.c index 6e8120702..363809cfd 100644 > --- a/examples/ipsec-secgw/ipsec.c > +++ b/examples/ipsec-secgw/ipsec.c > @@ -415,6 +415,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) > + return 0; /* No Flow director rules for Egress traffic */ > + if (sa->flags == TRANSPORT) { > + RTE_LOG(ERR, IPSEC, > + "No Flow director rule for transport mode:"); > + 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, > + }; > + 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, IPV6_ADDR_LEN); > + memcpy(sa->ipv6_spec.hdr.src_addr, > + sa->src.ip.ip6.ip6_b, IPV6_ADDR_LEN); > + 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->fdir_portid, &sa->attr, > + sa->pattern, sa->action, > + &err); > + if (ret < 0) { > + RTE_LOG(ERR, IPSEC, > + "Flow Validation failed\n"); > + return ret; > + } > + sa->flow = rte_flow_create(sa->fdir_portid, > + &sa->attr, sa->pattern, sa->action, > + &err); > + if (!sa->flow) { > + RTE_LOG(ERR, IPSEC, > + "Flow Creation failed\n"); > + 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 4f2fd6184..00147895a 100644 > --- a/examples/ipsec-secgw/ipsec.h > +++ b/examples/ipsec-secgw/ipsec.h > @@ -46,6 +46,8 @@ > > #define IP6_VERSION (6) > > +#define IPV6_ADDR_LEN 16 > + > struct rte_crypto_xform; > struct ipsec_xform; > struct rte_mbuf; > @@ -138,6 +140,9 @@ struct ipsec_sa { > }; > enum rte_security_ipsec_sa_direction direction; > uint16_t portid; > + uint16_t fdir_portid; > + uint8_t fdir_qid; > + uint8_t fdir_flag; > > #define MAX_RTE_FLOW_PATTERN (4) > #define MAX_RTE_FLOW_ACTIONS (3) > @@ -383,5 +388,11 @@ 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); > > +int check_fdir_configured(uint16_t portid); > #endif /* __IPSEC_H__ */ > diff --git a/examples/ipsec-secgw/sa.c b/examples/ipsec-secgw/sa.c > index 4822d6bda..9955dfcbe 100644 > --- a/examples/ipsec-secgw/sa.c > +++ b/examples/ipsec-secgw/sa.c > @@ -20,6 +20,9 @@ > #include <rte_random.h> > #include <rte_ethdev.h> > #include <rte_malloc.h> > +#include <rte_common.h> > +#include <rte_string_fns.h> > +#include <rte_ethdev_driver.h> > > #include "ipsec.h" > #include "esp.h" > @@ -271,6 +274,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; > @@ -681,6 +685,25 @@ parse_sa_tokens(char **tokens, uint32_t n_tokens, > fallback_p = 1; > continue; > } > + if (strcmp(tokens[ti], "flow-direction") == 0) { > + rule->fdir_flag = 1; > + INCREMENT_TOKEN_INDEX(ti, n_tokens, status); > + if (status->status < 0) > + return; > + rule->fdir_portid = atoi(tokens[ti]); > + 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->fdir_portid, > + rule->fdir_qid); > + if (status_p < 0) { > + printf("port id %u / queue id %u is not > valid\n", > + rule->fdir_portid, rule->fdir_qid); > + } > + continue; > + } > > /* unrecognizeable input */ > APP_CHECK(0, status, "unrecognized input \"%s\"", @@ -823,6 > +846,9 @@ print_one_sa_rule(const struct ipsec_sa *sa, int inbound) > break; > } > } > + if (sa->fdir_flag == 1) > + printf("flow-direction %d %d", sa->fdir_portid, sa->fdir_qid); > + > printf("\n"); > } > > @@ -1153,7 +1179,15 @@ sa_add_rules(struct sa_ctx *sa_ctx, const > struct ipsec_sa entries[], > return -EINVAL; > } > } > - > + if (sa->fdir_flag && > + ips->type == > + RTE_SECURITY_ACTION_TYPE_LOOKASIDE_PROTOCOL > && > + inbound) { > + rc = create_ipsec_esp_flow(sa); > + if (rc != 0) > + RTE_LOG(ERR, IPSEC_ESP, > + "create_ipsec_esp flow failed\n"); > + } > print_one_sa_rule(sa, inbound); > } > > @@ -1256,6 +1290,20 @@ fill_ipsec_session(struct rte_ipsec_session > *ss, struct rte_ipsec_sa *sa) > return rc; > } > > +int > +check_fdir_configured(uint16_t portid) { > + struct ipsec_sa *sa = NULL; > + uint32_t idx_sa = 0; > + > + for (idx_sa = 0; idx_sa < nb_sa_in; idx_sa++) { > + sa = &sa_in[idx_sa]; > + if (sa->fdir_portid == portid) > + return sa->fdir_flag; > + } > + return 0; > +} > + > /* > * Initialise related rte_ipsec_sa object. > */ > -- > 2.17.1