DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH v1] examples/ipsec-secgw: load/unload esp-ah DDP file
@ 2020-03-06  5:56 Praveen Shetty
  2020-03-06  9:59 ` Anoob Joseph
  0 siblings, 1 reply; 6+ messages in thread
From: Praveen Shetty @ 2020-03-06  5:56 UTC (permalink / raw)
  To: dev, declan.doherty, bernard.iremonger, konstantin.ananyev

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.shetty@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)]"
 		" -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);
 	}
+	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);
+	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);
+		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
+
+	if (ret == -EEXIST)
+		printf("Profile has already existed.\n");
+	else if (ret < 0) {
+		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;
+}
+
 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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dpdk-dev] [PATCH v1] examples/ipsec-secgw: load/unload esp-ah DDP file
  2020-03-06  5:56 [dpdk-dev] [PATCH v1] examples/ipsec-secgw: load/unload esp-ah DDP file Praveen Shetty
@ 2020-03-06  9:59 ` Anoob Joseph
  2020-03-10 11:41   ` Shetty, Praveen
  0 siblings, 1 reply; 6+ messages in thread
From: Anoob Joseph @ 2020-03-06  9:59 UTC (permalink / raw)
  To: Praveen Shetty, dev, declan.doherty, bernard.iremonger,
	konstantin.ananyev

Hi Praveen,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Praveen Shetty
> Sent: Friday, March 6, 2020 11:26 AM
> To: dev@dpdk.org; declan.doherty@intel.com;
> bernard.iremonger@intel.com; konstantin.ananyev@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.shetty@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.
 
>  		" -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?
 
> +	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.

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

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dpdk-dev] [PATCH v1] examples/ipsec-secgw: load/unload esp-ah DDP file
  2020-03-06  9:59 ` Anoob Joseph
@ 2020-03-10 11:41   ` Shetty, Praveen
  2020-03-10 16:24     ` Anoob Joseph
  0 siblings, 1 reply; 6+ messages in thread
From: Shetty, Praveen @ 2020-03-10 11:41 UTC (permalink / raw)
  To: Anoob Joseph, dev, Doherty, Declan, Iremonger, Bernard, Ananyev,
	Konstantin

Hi Anoob,

Thanks for the review.
Please see my response inline.

Regards,
Praveen

-----Original Message-----
From: Anoob Joseph <anoobj@marvell.com> 
Sent: Friday, March 6, 2020 3:30 PM
To: Shetty, Praveen <praveen.shetty@intel.com>; dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>; Iremonger, Bernard <bernard.iremonger@intel.com>; Ananyev, Konstantin <konstantin.ananyev@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-bounces@dpdk.org> On Behalf Of Praveen Shetty
> Sent: Friday, March 6, 2020 11:26 AM
> To: dev@dpdk.org; declan.doherty@intel.com; 
> bernard.iremonger@intel.com; konstantin.ananyev@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.shetty@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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dpdk-dev] [PATCH v1] examples/ipsec-secgw: load/unload esp-ah DDP file
  2020-03-10 11:41   ` Shetty, Praveen
@ 2020-03-10 16:24     ` Anoob Joseph
  2020-03-11 13:35       ` Doherty, Declan
  0 siblings, 1 reply; 6+ messages in thread
From: Anoob Joseph @ 2020-03-10 16:24 UTC (permalink / raw)
  To: Shetty, Praveen, dev, Doherty, Declan, Iremonger, Bernard,
	Ananyev, Konstantin

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.shetty@intel.com>
> Sent: Tuesday, March 10, 2020 5:11 PM
> To: Anoob Joseph <anoobj@marvell.com>; dev@dpdk.org; Doherty, Declan
> <declan.doherty@intel.com>; Iremonger, Bernard
> <bernard.iremonger@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@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 <anoobj@marvell.com>
> Sent: Friday, March 6, 2020 3:30 PM
> To: Shetty, Praveen <praveen.shetty@intel.com>; dev@dpdk.org; Doherty,
> Declan <declan.doherty@intel.com>; Iremonger, Bernard
> <bernard.iremonger@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@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-bounces@dpdk.org> On Behalf Of Praveen Shetty
> > Sent: Friday, March 6, 2020 11:26 AM
> > To: dev@dpdk.org; declan.doherty@intel.com;
> > bernard.iremonger@intel.com; konstantin.ananyev@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.shetty@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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dpdk-dev] [PATCH v1] examples/ipsec-secgw: load/unload esp-ah DDP file
  2020-03-10 16:24     ` Anoob Joseph
@ 2020-03-11 13:35       ` Doherty, Declan
  2020-03-11 15:03         ` Ananyev, Konstantin
  0 siblings, 1 reply; 6+ messages in thread
From: Doherty, Declan @ 2020-03-11 13:35 UTC (permalink / raw)
  To: Anoob Joseph, Shetty, Praveen, dev, Iremonger, Bernard, Ananyev,
	Konstantin

On 10/03/2020 4:24 PM, Anoob Joseph wrote:
> 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.


I agree this would be better handled by driver specific devargs

> 
> @Konstatin, @Akhil,
> What is your take on this?
> 
> Thanks,
> Anoob
> 
>> -----Original Message-----
>> From: Shetty, Praveen <praveen.shetty@intel.com>
>> Sent: Tuesday, March 10, 2020 5:11 PM
>> To: Anoob Joseph <anoobj@marvell.com>; dev@dpdk.org; Doherty, Declan
>> <declan.doherty@intel.com>; Iremonger, Bernard
>> <bernard.iremonger@intel.com>; Ananyev, Konstantin
>> <konstantin.ananyev@intel.com>
>> Subject: [EXT] RE: [dpdk-dev] [PATCH v1] examples/ipsec-secgw: load/unload
>> esp-ah DDP file
>>
...
>>> --
>>> 2.17.1
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dpdk-dev] [PATCH v1] examples/ipsec-secgw: load/unload esp-ah DDP file
  2020-03-11 13:35       ` Doherty, Declan
@ 2020-03-11 15:03         ` Ananyev, Konstantin
  0 siblings, 0 replies; 6+ messages in thread
From: Ananyev, Konstantin @ 2020-03-11 15:03 UTC (permalink / raw)
  To: Doherty, Declan, Anoob Joseph, Shetty, Praveen, dev, Iremonger, Bernard



> -----Original Message-----
> From: Doherty, Declan <declan.doherty@intel.com>
> Sent: Wednesday, March 11, 2020 1:36 PM
> To: Anoob Joseph <anoobj@marvell.com>; Shetty, Praveen <praveen.shetty@intel.com>; dev@dpdk.org; Iremonger, Bernard
> <bernard.iremonger@intel.com>; Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v1] examples/ipsec-secgw: load/unload esp-ah DDP file
> 
> On 10/03/2020 4:24 PM, Anoob Joseph wrote:
> > 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.
> 
> 
> I agree this would be better handled by driver specific devargs

No objections from my side too.

> 
> >
> > @Konstatin, @Akhil,
> > What is your take on this?
> >
> > Thanks,
> > Anoob
> >
> >> -----Original Message-----
> >> From: Shetty, Praveen <praveen.shetty@intel.com>
> >> Sent: Tuesday, March 10, 2020 5:11 PM
> >> To: Anoob Joseph <anoobj@marvell.com>; dev@dpdk.org; Doherty, Declan
> >> <declan.doherty@intel.com>; Iremonger, Bernard
> >> <bernard.iremonger@intel.com>; Ananyev, Konstantin
> >> <konstantin.ananyev@intel.com>
> >> Subject: [EXT] RE: [dpdk-dev] [PATCH v1] examples/ipsec-secgw: load/unload
> >> esp-ah DDP file
> >>
> ...
> >>> --
> >>> 2.17.1
> >


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-03-11 15:04 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-06  5:56 [dpdk-dev] [PATCH v1] examples/ipsec-secgw: load/unload esp-ah DDP file Praveen Shetty
2020-03-06  9:59 ` Anoob Joseph
2020-03-10 11:41   ` Shetty, Praveen
2020-03-10 16:24     ` Anoob Joseph
2020-03-11 13:35       ` Doherty, Declan
2020-03-11 15:03         ` Ananyev, Konstantin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).