diff mbox series

[v1] lib/cryptodev: multi-process IPC request handler

Message ID 20220726230804.90036-1-kai.ji@intel.com (mailing list archive)
State Changes Requested
Delegated to: akhil goyal
Headers show
Series [v1] lib/cryptodev: multi-process IPC request handler | expand

Checks

Context Check Description
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-x86_64-compile-testing warning Testing issues
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/intel-Testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Ji, Kai July 26, 2022, 11:08 p.m. UTC
This patch add in multi-process IPC request handler function in rte
cryptodev. This function intend to support a queue-pair configuration
request to allow the secondary process to reconfigure the queue-pair
setup'ed by the primary process.

Signed-off-by: Kai Ji <kai.ji@intel.com>
---
 lib/cryptodev/rte_cryptodev.c | 45 +++++++++++++++++++++++++++++++++++
 lib/cryptodev/rte_cryptodev.h | 14 +++++++++++
 2 files changed, 59 insertions(+)

Comments

Akhil Goyal July 27, 2022, 4:25 a.m. UTC | #1
This is a library change you should cc all PMD owners while sending patch.

> This patch add in multi-process IPC request handler function in rte
> cryptodev. This function intend to support a queue-pair configuration
> request to allow the secondary process to reconfigure the queue-pair
> setup'ed by the primary process.

Who will release the queue pair already setup by primary in the first place?
Currently, all queues are setup by primary and secondary uses them.
So if a queue is re-initialized by secondary, and if it is being used in primary process,
Wont that drop packets abruptly if the queue is re-initialized?

Also, I see register API but not deregister.
> 
> Signed-off-by: Kai Ji <kai.ji@intel.com>
> ---
>  lib/cryptodev/rte_cryptodev.c | 45 +++++++++++++++++++++++++++++++++++
>  lib/cryptodev/rte_cryptodev.h | 14 +++++++++++
>  2 files changed, 59 insertions(+)
> 
> diff --git a/lib/cryptodev/rte_cryptodev.c b/lib/cryptodev/rte_cryptodev.c
> index 42f3221052..18fdbf6db6 100644
> --- a/lib/cryptodev/rte_cryptodev.c
> +++ b/lib/cryptodev/rte_cryptodev.c
> @@ -1202,6 +1202,51 @@ rte_cryptodev_get_qp_status(uint8_t dev_id,
> uint16_t queue_pair_id)
>  	return 0;
>  }
> 
> +/* crypto queue pair config */
> +#define CRYPTODEV_MP_REQ "cryptodev_mp_request"
> +
> +static int
> +rte_cryptodev_mp_request(const struct rte_mp_msg *mp_msg, const void
> *peer)
> +{
> +	struct rte_mp_msg mp_res;
> +	struct rte_cryptodev_mp_param *res =
> +		(struct rte_cryptodev_mp_param *)mp_res.param;
> +	const struct rte_cryptodev_mp_param *param =
> +		(const struct rte_cryptodev_mp_param *)mp_msg->param;
> +
> +	int ret;
> +	int dev_id = 0;
> +	int port_id = 0, socket_id = 1;
> +	struct rte_cryptodev_qp_conf queue_conf;
> +	queue_conf.nb_descriptors = 2;
> +
> +	RTE_ASSERT(rte_eal_process_type() == RTE_PROC_PRIMARY);
> +	switch (param->type) {
> +	case RTE_CRYPTODEV_MP_REQ_QP:
> +		ret = rte_cryptodev_queue_pair_setup(dev_id, port_id,
> +				&queue_conf, socket_id);
> +		res->result = ret;
> +
> +		ret = rte_mp_reply(&mp_res, peer);
> +		break;
> +	default:
> +		CDEV_LOG_ERR("invalid mp request type\n");
> +		return -EINVAL;
> +	}
> +	return ret;
> +
> +}
> +
> +int rte_cryptodev_mp_request_register(void)
> +{
> +	int ret;
> +
> +	RTE_ASSERT(rte_eal_process_type() == RTE_PROC_PRIMARY);
> +	ret = rte_mp_action_register(CRYPTODEV_MP_REQ,
> +				rte_cryptodev_mp_request);
> +	return ret;
> +}
> +
>  int
>  rte_cryptodev_queue_pair_setup(uint8_t dev_id, uint16_t queue_pair_id,
>  		const struct rte_cryptodev_qp_conf *qp_conf, int socket_id)
> diff --git a/lib/cryptodev/rte_cryptodev.h b/lib/cryptodev/rte_cryptodev.h
> index 56f459c6a0..901465ca65 100644
> --- a/lib/cryptodev/rte_cryptodev.h
> +++ b/lib/cryptodev/rte_cryptodev.h
> @@ -539,6 +539,18 @@ enum rte_cryptodev_event_type {
>  	RTE_CRYPTODEV_EVENT_MAX		/**< max value of this enum */
>  };
> 
> +/* Request types for IPC. */
> +enum rte_cryptodev_mp_req_type {
> +	RTE_CRYPTODEV_MP_REQ_NONE,
> +	RTE_CRYPTODEV_MP_REQ_QP
> +};
> +
> +/* Parameters for IPC. */
> +struct rte_cryptodev_mp_param {
> +	enum rte_cryptodev_mp_req_type type;
> +	int result;
> +};
> +
>  /** Crypto device queue pair configuration structure. */
>  struct rte_cryptodev_qp_conf {
>  	uint32_t nb_descriptors; /**< Number of descriptors per queue pair */
> @@ -744,6 +756,8 @@ rte_cryptodev_stop(uint8_t dev_id);
>  extern int
>  rte_cryptodev_close(uint8_t dev_id);
> 
> +extern int rte_cryptodev_mp_request_register(void);
> +
>  /**
>   * Allocate and set up a receive queue pair for a device.
>   *
> --
> 2.17.1
Fan Zhang Aug. 5, 2022, 8:51 a.m. UTC | #2
Hi Akhil,

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Wednesday, July 27, 2022 5:26 AM
> To: Ji, Kai <kai.ji@intel.com>; dev@dpdk.org
> Cc: Anoob Joseph <anoobj@marvell.com>; hemant.agrawal@nxp.com;
> Zhang, Roy Fan <roy.fan.zhang@intel.com>; Chandubabu Namburu
> <chandu@amd.com>; Ruifeng Wang <ruifeng.wang@arm.com>;
> ajit.khaparde@broadcom.com; Michael Shamis <michaelsh@marvell.com>;
> Nagadheeraj Rottela <rnagadheeraj@marvell.com>; matan@nvidia.com; Jay
> Zhou <jianjay.zhou@huawei.com>
> Subject: RE: [EXT] [dpdk-dev v1] lib/cryptodev: multi-process IPC request
> handler
> 
> This is a library change you should cc all PMD owners while sending patch.
Kai is in holiday at the moment and will be back in a week. I will sync with him then.
> 
> > This patch add in multi-process IPC request handler function in rte
> > cryptodev. This function intend to support a queue-pair configuration
> > request to allow the secondary process to reconfigure the queue-pair
> > setup'ed by the primary process.
> 
> Who will release the queue pair already setup by primary in the first place?

Fan: If the queue pair already setup by primary the secondary shall not recreate it
but use it instead.

> Currently, all queues are setup by primary and secondary uses them.
> So if a queue is re-initialized by secondary, and if it is being used in primary
> process,
> Wont that drop packets abruptly if the queue is re-initialized?

You are right. What about creating a variable in the queue pair with either PID
or thread id who own the queue pair?

> 
> Also, I see register API but not deregister.

There will be V2 for that.

Regards,
Fan
Akhil Goyal Aug. 8, 2022, 7:43 a.m. UTC | #3
Hi Fan,
> Hi Akhil,
> 
> > This is a library change you should cc all PMD owners while sending patch.
> Kai is in holiday at the moment and will be back in a week. I will sync with him
> then.
> >
> > > This patch add in multi-process IPC request handler function in rte
> > > cryptodev. This function intend to support a queue-pair configuration
> > > request to allow the secondary process to reconfigure the queue-pair
> > > setup'ed by the primary process.
> >
> > Who will release the queue pair already setup by primary in the first place?
> 
> Fan: If the queue pair already setup by primary the secondary shall not recreate
> it
> but use it instead.

OK but the description says secondary would reconfigure the qp setup by primary.

> 
> > Currently, all queues are setup by primary and secondary uses them.
> > So if a queue is re-initialized by secondary, and if it is being used in primary
> > process,
> > Wont that drop packets abruptly if the queue is re-initialized?
> 
> You are right. What about creating a variable in the queue pair with either PID
> or thread id who own the queue pair?

I believe we should not expose the PID/thread id via queue to the user application.
This may be security issue.

Instead an "in_use" parameter can be added which can tell if sone other process is using it or not.
And this in_use param also need not be exposed to user. It can be completely hidden in the PMD.
User will get an error number(probably -EUSERS) indicating the queue pair is already in use.

Regards,
Akhil
Fan Zhang Aug. 12, 2022, 8:06 a.m. UTC | #4
HI Akhil,

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Monday, August 8, 2022 8:43 AM
> To: Zhang, Roy Fan <roy.fan.zhang@intel.com>; Ji, Kai <kai.ji@intel.com>;
> dev@dpdk.org
> Cc: Anoob Joseph <anoobj@marvell.com>; hemant.agrawal@nxp.com;
> Chandubabu Namburu <chandu@amd.com>; Ruifeng Wang
> <ruifeng.wang@arm.com>; ajit.khaparde@broadcom.com; Michael Shamis
> <michaelsh@marvell.com>; Nagadheeraj Rottela
> <rnagadheeraj@marvell.com>; matan@nvidia.com; Jay Zhou
> <jianjay.zhou@huawei.com>
> Subject: RE: [EXT] [dpdk-dev v1] lib/cryptodev: multi-process IPC request
> handler
> 
> Hi Fan,
> > Hi Akhil,
> >
> > > This is a library change you should cc all PMD owners while sending patch.
> > Kai is in holiday at the moment and will be back in a week. I will sync with
> him
> > then.
> > >
> > > > This patch add in multi-process IPC request handler function in rte
> > > > cryptodev. This function intend to support a queue-pair configuration
> > > > request to allow the secondary process to reconfigure the queue-pair
> > > > setup'ed by the primary process.
> > >
> > > Who will release the queue pair already setup by primary in the first place?
> >
> > Fan: If the queue pair already setup by primary the secondary shall not
> recreate
> > it
> > but use it instead.
> 
> OK but the description says secondary would reconfigure the qp setup by
> primary.
> 
> >
> > > Currently, all queues are setup by primary and secondary uses them.
> > > So if a queue is re-initialized by secondary, and if it is being used in
> primary
> > > process,
> > > Wont that drop packets abruptly if the queue is re-initialized?
> >
> > You are right. What about creating a variable in the queue pair with either
> PID
> > or thread id who own the queue pair?
> 
> I believe we should not expose the PID/thread id via queue to the user
> application.
> This may be security issue.

You have a point.

> 
> Instead an "in_use" parameter can be added which can tell if sone other
> process is using it or not.
> And this in_use param also need not be exposed to user. It can be
> completely hidden in the PMD.
> User will get an error number(probably -EUSERS) indicating the queue pair is
> already in use.

Great idea. That's what I am after too. So can I sum up the following change?

- each queue pair has a "in_use" param. I believe we can refine this a bit by
a "not_in_use", "in_use_by_primary" and "in_use_by_secondary" enum.
- the secondary process may request to configure a queue pair by sending
  message to primary
- as of requesting freeing a queue pair
	- primary can free any queue pair.
	- but for secondary to free a queue pair, we have a problem:
		- we may allow secondary to request freeing the queue pair if it
		  is "in_use_by_secondary". But then there may be a security
		  issue as a secondary can free a queue pair used by different
		  secondary process.
		- or we may not allow secondary process to request freeing
		  any queue pair, it is securer, but less flexible.


> 
> Regards,
> Akhil

Regards,
Fan
Akhil Goyal Aug. 12, 2022, 8:25 a.m. UTC | #5
> >
> > Instead an "in_use" parameter can be added which can tell if sone other
> > process is using it or not.
> > And this in_use param also need not be exposed to user. It can be
> > completely hidden in the PMD.
> > User will get an error number(probably -EUSERS) indicating the queue pair is
> > already in use.
> 
> Great idea. That's what I am after too. So can I sum up the following change?
> 
> - each queue pair has a "in_use" param. I believe we can refine this a bit by
> a "not_in_use", "in_use_by_primary" and "in_use_by_secondary" enum.
This is specific to the PMD. Each PMD may have its own implementation.

> - the secondary process may request to configure a queue pair by sending
>   message to primary
> - as of requesting freeing a queue pair
> 	- primary can free any queue pair.
> 	- but for secondary to free a queue pair, we have a problem:
> 		- we may allow secondary to request freeing the queue pair if it
> 		  is "in_use_by_secondary". But then there may be a security
> 		  issue as a secondary can free a queue pair used by different
> 		  secondary process.
Is it not possible to save pid(inside PMD) of the requesting process in queue private data
which is being configured?

> 		- or we may not allow secondary process to request freeing
> 		  any queue pair, it is securer, but less flexible.
> 
> 
> >
> > Regards,
> > Akhil
> 
> Regards,
> Fan
Akhil Goyal Sept. 21, 2022, 6:37 p.m. UTC | #6
> Subject: RE: [EXT] [dpdk-dev v1] lib/cryptodev: multi-process IPC request handler
> 
> > >
> > > Instead an "in_use" parameter can be added which can tell if sone other
> > > process is using it or not.
> > > And this in_use param also need not be exposed to user. It can be
> > > completely hidden in the PMD.
> > > User will get an error number(probably -EUSERS) indicating the queue pair is
> > > already in use.
> >
> > Great idea. That's what I am after too. So can I sum up the following change?
> >
> > - each queue pair has a "in_use" param. I believe we can refine this a bit by
> > a "not_in_use", "in_use_by_primary" and "in_use_by_secondary" enum.
> This is specific to the PMD. Each PMD may have its own implementation.
> 
> > - the secondary process may request to configure a queue pair by sending
> >   message to primary
> > - as of requesting freeing a queue pair
> > 	- primary can free any queue pair.
> > 	- but for secondary to free a queue pair, we have a problem:
> > 		- we may allow secondary to request freeing the queue pair if it
> > 		  is "in_use_by_secondary". But then there may be a security
> > 		  issue as a secondary can free a queue pair used by different
> > 		  secondary process.
> Is it not possible to save pid(inside PMD) of the requesting process in queue
> private data
> which is being configured?
> 
> > 		- or we may not allow secondary process to request freeing
> > 		  any queue pair, it is securer, but less flexible.
> >
Any update on this?

Marking this as Changes requested.
Please see that this change can only go in RC1 or else defer to next cycle.
diff mbox series

Patch

diff --git a/lib/cryptodev/rte_cryptodev.c b/lib/cryptodev/rte_cryptodev.c
index 42f3221052..18fdbf6db6 100644
--- a/lib/cryptodev/rte_cryptodev.c
+++ b/lib/cryptodev/rte_cryptodev.c
@@ -1202,6 +1202,51 @@  rte_cryptodev_get_qp_status(uint8_t dev_id, uint16_t queue_pair_id)
 	return 0;
 }
 
+/* crypto queue pair config */
+#define CRYPTODEV_MP_REQ "cryptodev_mp_request"
+
+static int
+rte_cryptodev_mp_request(const struct rte_mp_msg *mp_msg, const void *peer)
+{
+	struct rte_mp_msg mp_res;
+	struct rte_cryptodev_mp_param *res =
+		(struct rte_cryptodev_mp_param *)mp_res.param;
+	const struct rte_cryptodev_mp_param *param =
+		(const struct rte_cryptodev_mp_param *)mp_msg->param;
+
+	int ret;
+	int dev_id = 0;
+	int port_id = 0, socket_id = 1;
+	struct rte_cryptodev_qp_conf queue_conf;
+	queue_conf.nb_descriptors = 2;
+
+	RTE_ASSERT(rte_eal_process_type() == RTE_PROC_PRIMARY);
+	switch (param->type) {
+	case RTE_CRYPTODEV_MP_REQ_QP:
+		ret = rte_cryptodev_queue_pair_setup(dev_id, port_id,
+				&queue_conf, socket_id);
+		res->result = ret;
+
+		ret = rte_mp_reply(&mp_res, peer);
+		break;
+	default:
+		CDEV_LOG_ERR("invalid mp request type\n");
+		return -EINVAL;
+	}
+	return ret;
+
+}
+
+int rte_cryptodev_mp_request_register(void)
+{
+	int ret;
+
+	RTE_ASSERT(rte_eal_process_type() == RTE_PROC_PRIMARY);
+	ret = rte_mp_action_register(CRYPTODEV_MP_REQ,
+				rte_cryptodev_mp_request);
+	return ret;
+}
+
 int
 rte_cryptodev_queue_pair_setup(uint8_t dev_id, uint16_t queue_pair_id,
 		const struct rte_cryptodev_qp_conf *qp_conf, int socket_id)
diff --git a/lib/cryptodev/rte_cryptodev.h b/lib/cryptodev/rte_cryptodev.h
index 56f459c6a0..901465ca65 100644
--- a/lib/cryptodev/rte_cryptodev.h
+++ b/lib/cryptodev/rte_cryptodev.h
@@ -539,6 +539,18 @@  enum rte_cryptodev_event_type {
 	RTE_CRYPTODEV_EVENT_MAX		/**< max value of this enum */
 };
 
+/* Request types for IPC. */
+enum rte_cryptodev_mp_req_type {
+	RTE_CRYPTODEV_MP_REQ_NONE,
+	RTE_CRYPTODEV_MP_REQ_QP
+};
+
+/* Parameters for IPC. */
+struct rte_cryptodev_mp_param {
+	enum rte_cryptodev_mp_req_type type;
+	int result;
+};
+
 /** Crypto device queue pair configuration structure. */
 struct rte_cryptodev_qp_conf {
 	uint32_t nb_descriptors; /**< Number of descriptors per queue pair */
@@ -744,6 +756,8 @@  rte_cryptodev_stop(uint8_t dev_id);
 extern int
 rte_cryptodev_close(uint8_t dev_id);
 
+extern int rte_cryptodev_mp_request_register(void);
+
 /**
  * Allocate and set up a receive queue pair for a device.
  *