[v1] lib/cryptodev: multi-process IPC request handler
Checks
Commit Message
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
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
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
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
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
> >
> > 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
> 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.
@@ -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)
@@ -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.
*