Hi Praveen,

Sorry for being late to reply on this, Please delegate the patches properly 
from next time in patchworks.
This patch was neither delegated to me, nor I was in to/cc. So it got missed.

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

Please add documentation (doc/guides/sample_app_ug/ipsec_secgw.rst) changes
to explain the new parameter.

> Signed-off-by: Praveen Shetty <praveen.she...@intel.com>
> ---
> v3 changes:
> Incorporated Anoob review comments on v2.
> 



> diff --git a/examples/ipsec-secgw/ipsec-secgw.c b/examples/ipsec-
> secgw/ipsec-secgw.c
> index ce36e6d9c..4400b075c 100644
> --- a/examples/ipsec-secgw/ipsec-secgw.c
> +++ b/examples/ipsec-secgw/ipsec-secgw.c
> @@ -246,6 +246,30 @@ static struct rte_eth_conf port_conf = {
>       .txmode = {
>               .mq_mode = ETH_MQ_TX_NONE,
>       },
> +     .fdir_conf = {

Fdir_conf is a deprecated parameter. It is not good to introduce 
Something new in the application with a deprecated parameter.
Please use the recommended way to configure flows.

> +     .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,
> +     }
>  };
> 
>  struct socket_ctx socket_ctx[NB_SOCKETS];
> @@ -1183,6 +1207,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)
>  {
> @@ -2813,6 +2859,15 @@ main(int32_t argc, char **argv)
> 
>               sa_check_offloads(portid, &req_rx_offloads[portid],
>                               &req_tx_offloads[portid]);
> +              /* 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[portid],
>                               req_tx_offloads[portid]);
>       }
> diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c
> index d40657102..76ee9dbcf 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)
> +             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, 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\n");
> +             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\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 f8f29f9b1..b0e9f45cb 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,12 @@ 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 0eb52d141..ddd275142 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);
>                               return;
> +                     }
>                       portid_p = 1;
>                       continue;
>               }
> @@ -681,6 +688,43 @@ 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_INLINE_PROTOCOL ||
> +                             ips->type ==
> +
>       RTE_SECURITY_ACTION_TYPE_LOOKASIDE_PROTOCOL ||
> +                             ips->type ==
> +
>       RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO) {
> +                             APP_CHECK(0, status, "Flow Director not "
> +                                     "supported for security session "
> +                                     "type:%d", ips->type);
> +                                     return;
> +                     }
It means it is supported in cpu crypto as well? Better to have a check for the 
supported
Action types, as in the future there may be some other action types.

> +                     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);
> +                             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);
> +                     }
> +                     continue;
> +             }
> 
>               /* unrecognizeable input */
>               APP_CHECK(0, status, "unrecognized input \"%s\"",
> @@ -719,7 +763,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;
> @@ -823,6 +866,9 @@ print_one_sa_rule(const struct ipsec_sa *sa, int inbound)
>                       break;
>               }
>       }
> +     if (sa->fdir_flag == 1)
> +             printf("flow-direction %d %d", sa->portid, sa->fdir_qid);

Better to print like below.
printf("flow-direction port %d queue %d ", sa->portid, sa->fdir_qid)

> +
>       printf("\n");
>  }
> 

Reply via email to