> >>>
> >>> Signed-off-by: Ankur Dwivedi <adwiv...@marvell.com>
> >>> Signed-off-by: Anoob Joseph <ano...@marvell.com>
> >>> ---
> >>> examples/ipsec-secgw/ipsec-secgw.c | 56
> >> ++++++++++++++++++++++++++++++++++++++
> >>> examples/ipsec-secgw/ipsec.c | 8 ++++++
> >>> examples/ipsec-secgw/ipsec.h | 6 ++++
> >>> 3 files changed, 70 insertions(+)
> >>>
> >>> diff --git a/examples/ipsec-secgw/ipsec-secgw.c
> >>> b/examples/ipsec-secgw/ipsec-secgw.c
> >>> index 3b5aaf6..7506922 100644
> >>> --- a/examples/ipsec-secgw/ipsec-secgw.c
> >>> +++ b/examples/ipsec-secgw/ipsec-secgw.c
> >>> @@ -128,6 +128,8 @@ struct ethaddr_info
> >> ethaddr_tbl[RTE_MAX_ETHPORTS] = {
> >>> { 0, ETHADDR(0x00, 0x16, 0x3e, 0x49, 0x9e, 0xdd) } };
> >>>
> >>> +struct flow_info flow_info_tbl[RTE_MAX_ETHPORTS];
> >>
> >> Need to be initialized with zeroes somewhere.
> >
> > [Anoob] Will add it in v2.
>
> [Lukasz] Is there any reason to initialize flow_info_tbl explicitly with
> zeros ? As a global array it will be automatically
> zeroized by the compiler.
I think, it wouldn't.
Only static ones will be silently initialized by compiler.
Otherwise it could be anything.
>
> >>
> >>> +
> >>> #define CMD_LINE_OPT_CONFIG "config"
> >>> #define CMD_LINE_OPT_SINGLE_SA "single-sa"
> >>> #define CMD_LINE_OPT_CRYPTODEV_MASK "cryptodev_mask"
> >>> @@ -2406,6 +2408,55 @@ reassemble_init(void)
> >>> return rc;
> >>> }
> >>>
> >>> +static int
> >>> +create_default_ipsec_flow(uint16_t port_id, uint64_t rx_offloads) {
> >>> + int ret = 0;
> >>> +
> >>> + /* Add the default ipsec flow to detect all ESP packets for rx */
> >>> + if (rx_offloads & DEV_RX_OFFLOAD_SECURITY) {
> >>> + struct rte_flow_action action[2];
> >>> + struct rte_flow_item pattern[2];
> >>> + struct rte_flow_attr attr = {0};
> >>> + struct rte_flow_error err;
> >>> + struct rte_flow *flow;
> >>> +
> >>> + pattern[0].type = RTE_FLOW_ITEM_TYPE_ESP;
> >>> + pattern[0].spec = NULL;
> >>> + pattern[0].mask = NULL;
> >>> + pattern[0].last = NULL;
> >>> + pattern[1].type = RTE_FLOW_ITEM_TYPE_END;
> >>> +
> >>> + action[0].type = RTE_FLOW_ACTION_TYPE_SECURITY;
> >>> + action[0].conf = NULL;
> >>> + action[1].type = RTE_FLOW_ACTION_TYPE_END;
> >>> + action[1].conf = NULL;
> >>> +
> >>> + attr.egress = 0;
> >>> + attr.ingress = 1;
> >>> +
> >>> + ret = rte_flow_validate(port_id, &attr, pattern, action, &err);
> >>> + if (ret) {
> >>
> >> As I understand, flow_validate() is used here to query does this capability
> >> (multiple security sessions for same flow) is supported by PMD/HW?
> >> If so, then probably no need for error message if it doesn't.
> >
> > [Anoob] Yes. Will remove the error log.
> >
> >>
> >>> + RTE_LOG(ERR, IPSEC,
> >>> + "Failed to validate ipsec flow %s\n",
> >>> + err.message);
> >>> + goto exit;
> >>> + }
> >>> +
> >>> + flow = rte_flow_create(port_id, &attr, pattern, action, &err);
> >>
> >> Same question as for http://patches.dpdk.org/patch/63621/ , why do you
> >> need it at all?
> >> What it will enable/disable?
> >
> > [Anoob] Your followup question there accurately describes the usage. If the
> > application wants to enable H/w IPsec processing only on a
> specific SPI range, it will be allowed so with this kind of flow.
> >
> > Let's say, application wants to allow H/w processing only for SPI 1-8192.
> > In that case, either 8192 rte_flows need to be created, or one
> rte_flow rule with SPI 1-8192 range can be created. Any SPI outside the range
> won't match the rule and rte_flow could have further rules to
> act on such packets.
> >
> >>
> >>> + if (flow == NULL) {
> >>> + RTE_LOG(ERR, IPSEC,
> >>> + "Failed to create ipsec flow %s\n",
> >>> + err.message);
> >>> + ret = -rte_errno;
> >>> + goto exit;
> >>
> >> Why not just 'return ret;' here?
> >
> > [Anoob] Will fix in v2.
> >
> >>
> >>> + }
> >>> + flow_info_tbl[port_id].rx_def_flow = flow;
> >>> + }
> >>> +exit:
> >>> + return ret;
> >>> +}
> >>> +
> >>> int32_t
> >>> main(int32_t argc, char **argv)
> >>> {
> >>> @@ -2478,6 +2529,11 @@ main(int32_t argc, char **argv)
> >>>
> >>> sa_check_offloads(portid, &req_rx_offloads,
> >> &req_tx_offloads);
> >>> port_init(portid, req_rx_offloads, req_tx_offloads);
> >>> + /* Create default ipsec flow for the ethernet device */
> >>> + ret = create_default_ipsec_flow(portid, req_rx_offloads);
> >>> + if (ret)
> >>> + printf("Cannot create default flow, err=%d,
> >> port=%d\n",
> >>> + ret, portid);
> >>
> >> Again it is an optional feature, so not sure if we need to report it for
> >> every port.
> >> Might be better to do visa-versa: LOG(INFO, ...) when create_default() was
> >> successfull.
> >
> > [Anoob] Will update in v2.
> >
> >>
> >>> }
> >>>
> >>> cryptodevs_init();
> >>> diff --git a/examples/ipsec-secgw/ipsec.c
> >>> b/examples/ipsec-secgw/ipsec.c index d4b5712..e529f68 100644
> >>> --- a/examples/ipsec-secgw/ipsec.c
> >>> +++ b/examples/ipsec-secgw/ipsec.c
> >>> @@ -261,6 +261,12 @@ create_inline_session(struct socket_ctx *skt_ctx,
> >> struct ipsec_sa *sa,
> >>> unsigned int i;
> >>> unsigned int j;
> >>>
> >>> + /*
> >>> + * Don't create flow if default flow is already created
> >>> + */
> >>> + if (flow_info_tbl[sa->portid].rx_def_flow)
> >>> + goto set_cdev_id;
> >>
> >> As a nit: would be great to avoid introducing extra gotos.
> >
> > [Anoob] So, set the cdev_id and return here itself?
> >
> > Will make that change in v2.
> >
> >>
> >>> +
> >>
> >> As I can see, that block of code is for
> >> RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO only.
> >> Is that what intended?
> >
> > [Anoob] Yes
> >
> >> BTW, for RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL, it seems rte_flow
> >> is never created anyway inside that function.
> >
> > [Anoob] Yes. Current ipsec-secgw doesn't have rte_flow creation for inline
> > protocol. It is done only for inline crypto. The default flow that
> we are adding is applicable for both inline crypto & inline protocol. Hence
> adding the extra check in inline crypto path to avoid creating
> duplicate rte_flows.
> >
> >>
> >>> ret = rte_eth_dev_info_get(sa->portid, &dev_info);
> >>> if (ret != 0) {
> >>> RTE_LOG(ERR, IPSEC,
> >>> @@ -396,6 +402,8 @@ create_inline_session(struct socket_ctx *skt_ctx,
> >> struct ipsec_sa *sa,
> >>> ips->security.ol_flags = sec_cap->ol_flags;
> >>> ips->security.ctx = sec_ctx;
> >>> }
> >>> +
> >>> +set_cdev_id:
> >>> sa->cdev_id_qp = 0;
> >>>
> >>> return 0;
> >>> diff --git a/examples/ipsec-secgw/ipsec.h
> >>> b/examples/ipsec-secgw/ipsec.h index 8e07521..28ff07d 100644
> >>> --- a/examples/ipsec-secgw/ipsec.h
> >>> +++ b/examples/ipsec-secgw/ipsec.h
> >>> @@ -81,6 +81,12 @@ struct app_sa_prm {
> >>>
> >>> extern struct app_sa_prm app_sa_prm;
> >>>
> >>> +struct flow_info {
> >>> + struct rte_flow *rx_def_flow;
> >>> +};
> >>> +
> >>> +extern struct flow_info flow_info_tbl[RTE_MAX_ETHPORTS];
> >>> +
> >>> enum {
> >>> IPSEC_SESSION_PRIMARY = 0,
> >>> IPSEC_SESSION_FALLBACK = 1,
> >>> --
> >>> 2.7.4
> >
