Hi Praveen,
[Anoob] Rather than calling PMD specific functions from all applications, it
might be better to introduce the same via 'devargs' to the PMD.
[Praveen Shetty] We will investigate further on this.
Probably let's converge on this before sending v2. I don't think we should add
a new option just specific to one PMD. Devargs already provides the required
framework and other PMDs are already using it.
@Konstatin, @Akhil,
What is your take on this?
Thanks,
Anoob
> -----Original Message-----
> From: Shetty, Praveen <praveen.she...@intel.com>
> Sent: Tuesday, March 10, 2020 5:11 PM
> To: Anoob Joseph <ano...@marvell.com>; dev@dpdk.org; Doherty, Declan
> <declan.dohe...@intel.com>; Iremonger, Bernard
> <bernard.iremon...@intel.com>; Ananyev, Konstantin
> <konstantin.anan...@intel.com>
> Subject: [EXT] RE: [dpdk-dev] [PATCH v1] examples/ipsec-secgw: load/unload
> esp-ah DDP file
>
> External Email
>
> ----------------------------------------------------------------------
> Hi Anoob,
>
> Thanks for the review.
> Please see my response inline.
>
> Regards,
> Praveen
>
> -----Original Message-----
> From: Anoob Joseph <ano...@marvell.com>
> Sent: Friday, March 6, 2020 3:30 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: load/unload esp-ah
> DDP file
>
> Hi Praveen,
>
> Please see inline.
>
> Thanks,
> Anoob
>
> > -----Original Message-----
> > From: dev <dev-boun...@dpdk.org> On Behalf Of Praveen Shetty
> > Sent: Friday, March 6, 2020 11:26 AM
> > To: dev@dpdk.org; declan.dohe...@intel.com;
> > bernard.iremon...@intel.com; konstantin.anan...@intel.com
> > Subject: [dpdk-dev] [PATCH v1] examples/ipsec-secgw: load/unload
> > esp-ah DDP file
> >
> > Modified Secuirty gateway application to support load/unload esp-ah
> > ddp package on i40e NIC from ipsec-secgw application.
> >
> > Signed-off-by: Praveen Shetty <praveen.she...@intel.com>
> > ---
> > examples/ipsec-secgw/ipsec-secgw.c | 67 +++++++++++-
> > examples/ipsec-secgw/ipsec.c | 158
> > +++++++++++++++++++++++++++++
> > examples/ipsec-secgw/ipsec.h | 6 +-
> > 3 files changed, 228 insertions(+), 3 deletions(-)
> >
> > diff --git a/examples/ipsec-secgw/ipsec-secgw.c b/examples/ipsec-
> > secgw/ipsec-secgw.c index 4799bc90c..214b8a625 100644
> > --- a/examples/ipsec-secgw/ipsec-secgw.c
> > +++ b/examples/ipsec-secgw/ipsec-secgw.c
> > @@ -198,7 +198,10 @@ struct app_sa_prm app_sa_prm = {
> > .cache_sz = SA_CACHE_SZ
> > };
> > static const char *cfgfile;
> > -
> > +static const char *ddp_file;
> > +static const char *ddp_bkp_file;
> > +static uint16_t fdir_portid;
> > +static int16_t ddp_unload_flag;
> > struct lcore_rx_queue {
> > uint16_t port_id;
> > uint8_t queue_id;
> > @@ -1286,6 +1289,8 @@ print_usage(const char *prgname)
> > " [-e]"
> > " [-a]"
> > " [-c]"
> > + " [-L (portid,ddp_file_path,ddp_backup_file_path)]"
> > + " [-U (portid,ddp_backup_file_path)]"
>
> [Anoob] This is applicable only to i40e NIC, right? Probably mention that.
> [Praveen Shetty] Yes. We will do this in V2.
>
> > " -f CONFIG_FILE"
> > " --config (port,queue,lcore)[,(port,queue,lcore)]"
> > " [--single-sa SAIDX]"
> > @@ -1307,6 +1312,8 @@ print_usage(const char *prgname)
> > " -a enables SA SQN atomic behaviour\n"
> > " -c specifies inbound SAD cache size,\n"
> > " zero value disables the cache (default value: 128)\n"
> > + " -L To load ddp profile on NIC\n"
> > + " -U To unload ddp profile from NIC\n"
> > " -f CONFIG_FILE: Configuration file\n"
> > " --config (port,queue,lcore): Rx queue configuration\n"
> > " --single-sa SAIDX: Use single SA index for outbound
> > traffic,\n"
> > @@ -1379,6 +1386,33 @@ parse_decimal(const char *str)
> > return num;
> > }
> >
> > +static int32_t
> > +parse_ddp_load_args(char *str)
> > +{
> > + int16_t num_args = 0;
> > + char *file_fld[3];
> > + num_args = rte_strsplit(str, strlen(str), file_fld, 3, ',');
> > + if (num_args != 3)
> > + return -1;
> > + fdir_portid = atoi(file_fld[0]);
> > + ddp_file = file_fld[1];
> > + ddp_bkp_file = file_fld[2];
> > + return 0;
> > +}
> > +
> > +static int32_t
> > +parse_ddp_unload_args(char *str)
> > +{
> > + int16_t num_args = 0;
> > + char *file_fld[2];
> > + num_args = rte_strsplit(str, strlen(str), file_fld, 2, ',');
> > + if (num_args != 2)
> > + return -1;
> > + fdir_portid = atoi(file_fld[0]);
> > + ddp_bkp_file = file_fld[1];
> > + return 0;
> > +}
> > +
> > static int32_t
> > parse_config(const char *q_arg)
> > {
> > @@ -1459,7 +1493,7 @@ parse_args(int32_t argc, char **argv)
> >
> > argvopt = argv;
> >
> > - while ((opt = getopt_long(argc, argvopt, "aelp:Pu:f:j:w:c:",
> > + while ((opt = getopt_long(argc, argvopt, "aelp:Pu:L:U:f:j:w:c:",
> > lgopts, &option_index)) != EOF) {
> >
> > switch (opt) {
> > @@ -1483,6 +1517,25 @@ parse_args(int32_t argc, char **argv)
> > return -1;
> > }
> > break;
> > + case 'L':
> > + printf("Loading ddp package selected\n");
> > + ret = parse_ddp_load_args(optarg);
> > + if (ret < 0) {
> > + printf("Invalid ddp load args: %s\n", optarg);
> > + print_usage(prgname);
> > + return -1;
> > + }
> > + break;
> > + case 'U':
> > + printf("Unloading ddp package selected\n");
> > + ret = parse_ddp_unload_args(optarg);
> > + if (ret < 0) {
> > + printf("Invalid ddp unload args: %s\n",
> > optarg);
> > + print_usage(prgname);
> > + return -1;
> > + }
> > + ddp_unload_flag = 1;
> > + break;
> > case 'f':
> > if (f_present == 1) {
> > printf("\"-f\" option present more than "
> > @@ -2505,6 +2558,16 @@ 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);
> > }
>
> [Anoob] Following checks are not valid for non i40e PMDs, right? Is there a
> capability we can check before doing the below?
> [Praveen Shetty] Yes, that's true. We can skip this for non i40e PMD by
> adding
> #ifdef RTE_LIBRTE_I40E_PMD before the following checks. We will do this in
> V2.
>
> > + if (ddp_file != NULL && ddp_bkp_file != NULL) {
> > + ret = load_ddp_esp_package(fdir_portid, ddp_file,
> > ddp_bkp_file);
> > + if (ret < 0)
> > + printf("loading ddp package failed\n");
> > + }
> > + if (ddp_unload_flag && ddp_bkp_file != NULL) {
> > + ret = unload_ddp_esp_package(fdir_portid, ddp_bkp_file);
> > + if (ret < 0)
> > + printf("unloading ddp package failed\n");
> > + }
> >
> > cryptodevs_init();
> >
> > diff --git a/examples/ipsec-secgw/ipsec.c
> > b/examples/ipsec-secgw/ipsec.c index 6e8120702..3840a0848 100644
> > --- a/examples/ipsec-secgw/ipsec.c
> > +++ b/examples/ipsec-secgw/ipsec.c
> > @@ -4,6 +4,9 @@
> > #include <sys/types.h>
> > #include <netinet/in.h>
> > #include <netinet/ip.h>
> > +#include <sys/stat.h>
> > +#include <fcntl.h>
> > +#include <unistd.h>
> >
> > #include <rte_branch_prediction.h>
> > #include <rte_log.h>
> > @@ -14,6 +17,9 @@
> > #include <rte_ethdev.h>
> > #include <rte_mbuf.h>
> > #include <rte_hash.h>
> > +#ifdef RTE_LIBRTE_I40E_PMD
> > +#include <rte_pmd_i40e.h>
> > +#endif
> >
> > #include "ipsec.h"
> > #include "esp.h"
> > @@ -155,6 +161,158 @@ create_lookaside_session(struct ipsec_ctx
> > *ipsec_ctx, struct ipsec_sa *sa,
> > return 0;
> > }
> >
> > +static int
> > +close_file(uint8_t *buf)
> > +{
> > + if (buf) {
> > + free((void *)buf);
> > + return 0;
> > + }
> > +
> > + return -1;
> > +}
> > +
> > +static uint8_t *
> > +open_file(const char *file_path, uint32_t *size) {
> > + int fd = open(file_path, O_RDONLY);
> > + off_t pkg_size;
> > + uint8_t *buf = NULL;
> > + int ret = 0;
> > + struct stat st_buf;
> > +
> > + if (size)
> > + *size = 0;
> > +
> > + if (fd == -1) {
> > + printf("%s: Failed to open %s\n", __func__, file_path);
> > + return buf;
> > + }
> > +
> > + if ((fstat(fd, &st_buf) != 0) || (!S_ISREG(st_buf.st_mode))) {
> > + close(fd);
> > + printf("%s: File operations failed\n", __func__);
> > + return buf;
> > + }
> > +
> > + pkg_size = st_buf.st_size;
> > + if (pkg_size < 0) {
> > + close(fd);
> > + printf("%s: File operations failed\n", __func__);
> > + return buf;
> > + }
> > +
> > + buf = (uint8_t *)malloc(pkg_size);
>
> [Anoob] The cast is not required.
> [Praveen Shetty] We will fix this in V2.
>
> > + if (!buf) {
> > + close(fd);
> > + printf("%s: Failed to malloc memory\n", __func__);
> > + return buf;
> > + }
> > +
> > + ret = read(fd, buf, pkg_size);
> > + if (ret < 0) {
> > + close(fd);
> > + printf("%s: File read operation failed\n", __func__);
> > + close_file(buf);
>
> [Anoob] close_file() just frees the buffer and returns 0 is the buffer is not
> NULL,
> and returns -1 if NULL. Do we need a separate function just for that? Also,
> the
> return value of close_file is ignored in all cases.
>
> [Praveen Shetty] Yes, We will address this in V2.
>
> > + return NULL;
> > + }
> > +
> > + if (size)
> > + *size = pkg_size;
> > +
> > + close(fd);
> > +
> > + return buf;
> > +}
> > +
> > +static int
> > +save_file(const char *file_path, uint8_t *buf, uint32_t size) {
> > + FILE *fh = fopen(file_path, "wb");
> > +
> > + if (fh == NULL) {
> > + printf("%s: Failed to open %s\n", __func__, file_path);
> > + return -1;
> > + }
> > +
> > + if (fwrite(buf, 1, size, fh) != size) {
> > + fclose(fh);
> > + printf("%s: File write operation failed\n", __func__);
> > + return -1;
> > + }
> > +
> > + fclose(fh);
> > +
> > + return 0;
> > +}
> > +
> > +/* Load dynamic device personalization profile */ int
> > +load_ddp_esp_package(uint16_t fdir_portid, const char *ddp_file,
> > + const char *ddp_bkp_file)
> > +{
> > + uint8_t *buff;
> > + uint32_t size;
> > + int ret = -ENOTSUP;
> > +
> > + buff = open_file(ddp_file, &size);
> > + if (!buff)
> > + return -1;
> > +
> > +#ifdef RTE_LIBRTE_I40E_PMD
> > + ret = rte_pmd_i40e_process_ddp_package(fdir_portid,
> > + buff, size,
> > +
> > RTE_PMD_I40E_PKG_OP_WR_ADD); #endif
> > +
>
> [Anoob] Rather than calling PMD specific functions from all applications, it
> might be better to introduce the same via 'devargs' to the PMD.
>
> [Praveen Shetty] We will investigate further on this.
>
> > + if (ret == -EEXIST)
> > + printf("Profile has already existed.\n");
> > + else if (ret < 0) {
>
> [Anoob] When i40e is not enabled (using RTE_LIBRTE_I40E_PMD), the error
> message would be "Failed to load profile", though the right reason is feature
> being not supported.
>
> [Praveen Shetty] In V2 the following code will be executed only when i40e is
> enabled. In that case error message can be intact.
>
> > + printf("Failed to load profile.\n");
> > + close_file(buff);
> > + return -1;
> > + } else
> > + printf("loading ddp profile success\n");
> > + if (ddp_bkp_file != NULL)
> > + save_file(ddp_bkp_file, buff, size);
> > +
> > + close_file(buff);
> > + return 0;
> > +}
> > +
> > +/* Unload dynamic device personalization profile */ int
> > +unload_ddp_esp_package(uint16_t fdir_portid, const char
> > +*ddp_bkp_file) {
> > +
> > + uint8_t *buff;
> > + uint32_t size;
> > + int ret = -ENOTSUP;
> > +
> > + buff = open_file(ddp_bkp_file, &size);
> > + if (!buff)
> > + return -1;
> > +
> > +#ifdef RTE_LIBRTE_I40E_PMD
> > + ret = rte_pmd_i40e_process_ddp_package(fdir_portid,
> > + buff, size,
> > +
> > RTE_PMD_I40E_PKG_OP_WR_DEL); #endif
> > +
> > + if (ret == -EACCES) {
> > + printf("Profile does not exist.\n");
> > + close_file(buff);
> > + return -1;
> > + } else if (ret < 0) {
> > + printf("Failed to delete profile.\n");
> > + close_file(buff);
> > + return -1;
> > + } else
> > + printf("ddp profile deleted successfully\n");
> > + close_file(buff);
> > + return 0;
> > +}
> > +
>
> [Anoob] All the above functions are just about loading the ddp package. May be
> we can move all that into a new file.
>
> [Praveen Shetty ] Yes we can do this. We will address this in V2.
>
> > int
> > create_inline_session(struct socket_ctx *skt_ctx, struct ipsec_sa *sa,
> > struct rte_ipsec_session *ips)
> > diff --git a/examples/ipsec-secgw/ipsec.h
> > b/examples/ipsec-secgw/ipsec.h index 4f2fd6184..73eec47ab 100644
> > --- a/examples/ipsec-secgw/ipsec.h
> > +++ b/examples/ipsec-secgw/ipsec.h
> > @@ -383,5 +383,9 @@ 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
> > +load_ddp_esp_package(uint16_t fdir_portid, const char *ddp_file,
> > + const char *ddp_bkp_file);
> > +int
> > +unload_ddp_esp_package(uint16_t fdir_portid, const char
> > +*ddp_bkp_file);
> > #endif /* __IPSEC_H__ */
> > --
> > 2.17.1
