[dpdk-dev] [PATCH 01/14] examples/ipsec-secgw: add default rte_flow for inline Rx
Anoob Joseph
anoobj at marvell.com
Mon Dec 16 16:58:24 CET 2019
Hi Konstantin,
Thanks for the review. Please see inline.
Thanks,
Anoob
> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev at intel.com>
> Sent: Monday, December 16, 2019 7:51 PM
> To: Anoob Joseph <anoobj at marvell.com>; Akhil Goyal <akhil.goyal at nxp.com>;
> Nicolau, Radu <radu.nicolau at intel.com>; Thomas Monjalon
> <thomas at monjalon.net>
> Cc: Ankur Dwivedi <adwivedi at marvell.com>; Jerin Jacob Kollanukkaran
> <jerinj at marvell.com>; Narayana Prasad Raju Athreya
> <pathreya at marvell.com>; Archana Muniganti <marchana at marvell.com>;
> Tejasree Kondoj <ktejasree at marvell.com>; Vamsi Krishna Attunuru
> <vattunuru at marvell.com>; Lukas Bartosik <lbartosik at marvell.com>;
> dev at dpdk.org
> Subject: [EXT] RE: [PATCH 01/14] examples/ipsec-secgw: add default rte_flow
> for inline Rx
>
> External Email
>
> ----------------------------------------------------------------------
>
> > From: Ankur Dwivedi <adwivedi at marvell.com>
> >
> > The default flow created would enable security processing on all ESP
> > packets. If the default flow is created, SA based rte_flow creation
> > would be skipped.
>
> I suppose that one depends on:
> http://patches.dpdk.org/patch/63621/
> http://patches.dpdk.org/cover/63625/
> to work as expected?
> If so probably worth to mention in that header or in cover letter (or both).
[Anoob] Yes. Usually the dependency is not added in the commit header. I'll update the v2 cover letter with such details.
>
> >
> > Signed-off-by: Ankur Dwivedi <adwivedi at marvell.com>
> > Signed-off-by: Anoob Joseph <anoobj at 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.
>
> > +
> > #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
More information about the dev
mailing list