Hi Radu,
Thanks for making the changes. I've one more question. Please see inline.
Thanks,
Anoob
> -----Original Message-----
> From: Radu Nicolau <radu.nico...@intel.com>
> Sent: Monday, February 26, 2024 3:56 PM
> To: dev@dpdk.org
> Cc: Anoob Joseph <ano...@marvell.com>; Radu Nicolau
> <radu.nico...@intel.com>; sta...@dpdk.org; Volodymyr Fialko
> <vfia...@marvell.com>; Ting-Kai Ku <ting-kai...@intel.com>; Ciara Power
> <ciara.po...@intel.com>; Kai Ji <kai...@intel.com>; Akhil Goyal
> <gak...@marvell.com>
> Subject: [EXT] [PATCH v3] examples/ipsec-secgw: fix cryptodev to SA mapping
>
> External Email
>
> ----------------------------------------------------------------------
> There are use cases where a SA should be able to use different cryptodevs on
> different lcores, for example there can be cryptodevs with just 1 qp per VF.
> For this purpose this patch relaxes the check in create lookaside session
> function.
> Also add a check to verify that a CQP is available for the current lcore.
>
> Fixes: a8ade12123c3 ("examples/ipsec-secgw: create lookaside sessions at
> init")
> Cc: sta...@dpdk.org
> Cc: vfia...@marvell.com
>
> Signed-off-by: Radu Nicolau <radu.nico...@intel.com>
> Tested-by: Ting-Kai Ku <ting-kai...@intel.com>
> Acked-by: Ciara Power <ciara.po...@intel.com>
> Acked-by: Kai Ji <kai...@intel.com>
> ---
> v3: check if the cryptodev are not of the same type
>
> examples/ipsec-secgw/ipsec.c | 25 ++++++++++++++++++++-----
> 1 file changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c index
> f5cec4a928..b59576c049 100644
> --- a/examples/ipsec-secgw/ipsec.c
> +++ b/examples/ipsec-secgw/ipsec.c
> @@ -288,10 +288,21 @@ create_lookaside_session(struct ipsec_ctx
> *ipsec_ctx_lcore[],
> if (cdev_id == RTE_CRYPTO_MAX_DEVS)
> cdev_id = ipsec_ctx->tbl[cdev_id_qp].id;
> else if (cdev_id != ipsec_ctx->tbl[cdev_id_qp].id) {
> - RTE_LOG(ERR, IPSEC,
> - "SA mapping to multiple cryptodevs is "
> - "not supported!");
> - return -EINVAL;
> + struct rte_cryptodev_info dev_info_1, dev_info_2;
> + rte_cryptodev_info_get(cdev_id, &dev_info_1);
> + rte_cryptodev_info_get(ipsec_ctx->tbl[cdev_id_qp].id,
> + &dev_info_2);
> + if (dev_info_1.driver_id == dev_info_2.driver_id) {
> + RTE_LOG(WARNING, IPSEC,
> + "SA mapped to multiple cryptodevs for
> SPI %d\n",
> + sa->spi);
> +
> + } else {
> + RTE_LOG(WARNING, IPSEC,
> + "SA mapped to multiple cryptodevs of
> different types for SPI %d\n",
> + sa->spi);
> +
> + }
> }
>
> /* Store per core queue pair information */ @@ -908,7 +919,11
> @@ ipsec_enqueue(ipsec_xform_fn xform_func, struct ipsec_ctx *ipsec_ctx,
> continue;
> }
>
> - enqueue_cop(sa->cqp[ipsec_ctx->lcore_id], &priv->cop);
> + if (likely(sa->cqp[ipsec_ctx->lcore_id]))
> + enqueue_cop(sa->cqp[ipsec_ctx->lcore_id], &priv->cop);
> + else
> + RTE_LOG(ERR, IPSEC, "No CQP available for lcore %d\n",
> + ipsec_ctx->lcore_id);
[Anoob] Throwing an error won't be good enough, right? Won't this lead to
packet leaks? Since it is datapath, can't we assume that the configuration
would be done correctly in control path?
I would suggest drop this specific change and we can enable multiple cryptodevs
with lookaside SAs with the changes proposed.
> }
> }
>
> --
> 2.34.1
