Re: [dpdk-dev] [PATCH 1/3] cryptodev: add new APIs to assist PMD initialisation

[Tomasz Duszynski <tdu@semihalf.com>]

Hi Declan,

Some comments inline.

On Fri, Oct 20, 2017 at 10:21:11PM +0100, Declan Doherty wrote:
> Adds new PMD assist functions which are bus independent for driver to
> create and destroy new device instances.
>
> Also includes function to parse parameters which can be passed to
> driver on device initialisation.
>
> Signed-off-by: Declan Doherty <declan.doherty@intel.com>
> ---
>  lib/librte_cryptodev/rte_cryptodev.h           |   8 +-
>  lib/librte_cryptodev/rte_cryptodev_pmd.c       | 169 +++++++++++++++++++++++++
>  lib/librte_cryptodev/rte_cryptodev_pmd.h       |  88 +++++++++++++
>  lib/librte_cryptodev/rte_cryptodev_version.map |   3 +
>  4 files changed, 264 insertions(+), 4 deletions(-)
>
> diff --git a/lib/librte_cryptodev/rte_cryptodev.h b/lib/librte_cryptodev/rte_cryptodev.h
> index fd0e3f1..86257b0 100644
> --- a/lib/librte_cryptodev/rte_cryptodev.h
> +++ b/lib/librte_cryptodev/rte_cryptodev.h
> @@ -60,10 +60,10 @@ extern const char **rte_cyptodev_names;
>  		RTE_FMT("%s() line %u: " RTE_FMT_HEAD(__VA_ARGS__,) "\n", \
>  			__func__, __LINE__, RTE_FMT_TAIL(__VA_ARGS__,)))
>
> -#define CDEV_PMD_LOG_ERR(dev, ...) \
> -	RTE_LOG(ERR, CRYPTODEV, \
> -		RTE_FMT("[%s] %s() line %u: " RTE_FMT_HEAD(__VA_ARGS__,) "\n", \
> -			dev, __func__, __LINE__, RTE_FMT_TAIL(__VA_ARGS__,)))
> +#define CDEV_LOG_INFO(...) \
> +	RTE_LOG(INFO, CRYPTODEV, \
> +		RTE_FMT("%s() line %u: " RTE_FMT_HEAD(__VA_ARGS__,) "\n", \
> +			__func__, __LINE__, RTE_FMT_TAIL(__VA_ARGS__,)))
>
>  #ifdef RTE_LIBRTE_CRYPTODEV_DEBUG
>  #define CDEV_LOG_DEBUG(...) \
> diff --git a/lib/librte_cryptodev/rte_cryptodev_pmd.c b/lib/librte_cryptodev/rte_cryptodev_pmd.c
> index a57faad..6cb4419 100644
> --- a/lib/librte_cryptodev/rte_cryptodev_pmd.c
> +++ b/lib/librte_cryptodev/rte_cryptodev_pmd.c
> @@ -40,6 +40,175 @@
>   * Parse name from argument
>   */
>  static int
> +rte_cryptodev_pmd_parse_name_arg(const char *key __rte_unused,
> +		const char *value, void *extra_args)
> +{
> +	struct rte_cryptodev_pmd_init_params *params = extra_args;
> +
> +	if (strlen(value) >= RTE_CRYPTODEV_NAME_MAX_LEN - 1) {
> +		CDEV_LOG_ERR("Invalid name %s, should be less than "
> +				"%u bytes", value,
> +				RTE_CRYPTODEV_NAME_MAX_LEN - 1);
> +		return -1;
> +	}
> +
> +	strncpy(params->name, value, RTE_CRYPTODEV_NAME_MAX_LEN);

Would strcpy() do here? At this point we already know that name will
fit into params->name.

> +
> +	return 0;
> +}
> +
> +/**
> + * Parse integer from argument
> + */
> +static int
> +rte_cryptodev_pmd_parse_integer_arg(const char *key __rte_unused,
> +		const char *value, void *extra_args)
> +{
> +	int *i = extra_args;
> +
> +	*i = atoi(value);
> +	if (*i < 0) {
> +		CDEV_LOG_ERR("Argument has to be positive.");

Perhaps s/positive/non-negative ?
Number 0 looks like a valid argument.

> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +int
> +rte_cryptodev_pmd_parse_input_args(
> +		struct rte_cryptodev_pmd_init_params *params,
> +		const char *args)
> +{
> +	struct rte_kvargs *kvlist = NULL;
> +	int ret = 0;
> +
> +	if (params == NULL)
> +		return -EINVAL;
> +
> +	if (args) {
> +		kvlist = rte_kvargs_parse(args,	cryptodev_pmd_valid_params);
> +		if (kvlist == NULL)
> +			return -EINVAL;
> +
> +		ret = rte_kvargs_process(kvlist,
> +				RTE_CRYPTODEV_PMD_MAX_NB_QP_ARG,
> +				&rte_cryptodev_pmd_parse_integer_arg,
> +				&params->max_nb_queue_pairs);
> +		if (ret < 0)
> +			goto free_kvlist;
> +
> +		ret = rte_kvargs_process(kvlist,
> +				RTE_CRYPTODEV_PMD_MAX_NB_SESS_ARG,
> +				&rte_cryptodev_pmd_parse_integer_arg,
> +				&params->max_nb_sessions);
> +		if (ret < 0)
> +			goto free_kvlist;
> +
> +		ret = rte_kvargs_process(kvlist,
> +				RTE_CRYPTODEV_PMD_SOCKET_ID_ARG,
> +				&rte_cryptodev_pmd_parse_integer_arg,
> +				&params->socket_id);
> +		if (ret < 0)
> +			goto free_kvlist;
> +
> +		ret = rte_kvargs_process(kvlist,
> +				RTE_CRYPTODEV_PMD_NAME_ARG,
> +				&rte_cryptodev_pmd_parse_name_arg,
> +				params);
> +		if (ret < 0)
> +			goto free_kvlist;
> +	}
> +
> +free_kvlist:
> +	rte_kvargs_free(kvlist);
> +	return ret;
> +}
> +
> +struct rte_cryptodev *
> +rte_cryptodev_pmd_create(const char *name,
> +		struct rte_device *device,
> +		struct rte_cryptodev_pmd_init_params *params)
> +{
> +	struct rte_cryptodev *cryptodev;
> +
> +	if (params->name[0] != '\0') {
> +		CDEV_LOG_INFO("[%s] User specified device name = %s\n",
> +				device->driver->name, params->name);
> +		name = params->name;
> +	}
> +
> +	CDEV_LOG_INFO("[%s] Creating cryptodev %s\n",
> +			device->driver->name, name);
> +
> +	CDEV_LOG_INFO("[%s] Initialisation parameters [ name: %s, "
> +			"private data size:  %zu, "
> +			"socket id: %d,"
> +			"max queue pairs: %u, "
> +			"max sessions: %u ]\n",
> +			device->driver->name, name, params->private_data_size,
> +			params->socket_id, params->max_nb_queue_pairs,
> +			params->max_nb_sessions);
> +
> +	/* allocate device structure */
> +	cryptodev = rte_cryptodev_pmd_allocate(name, params->socket_id);
> +	if (cryptodev == NULL) {
> +		CDEV_LOG_ERR("[%s] Failed to allocate crypto device for %s",
> +				device->driver->name, name);
> +		return NULL;
> +	}
> +
> +	/* allocate private device structure */
> +	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> +		cryptodev->data->dev_private =
> +				rte_zmalloc_socket("cryptodev device private",
> +						params->private_data_size,
> +						RTE_CACHE_LINE_SIZE,
> +						params->socket_id);
> +
> +		if (cryptodev->data->dev_private == NULL) {
> +			CDEV_LOG_ERR("[%s] Cannot allocate memory for "
> +					"cryptodev %s private data",
> +					device->driver->name, name);
> +
> +			rte_cryptodev_pmd_release_device(cryptodev);
> +			return NULL;
> +		}
> +	}
> +
> +	cryptodev->device = device;
> +
> +	/* initialise user call-back tail queue */
> +	TAILQ_INIT(&(cryptodev->link_intr_cbs));
> +
> +	return cryptodev;
> +}
> +
> +
> +
> +int
> +rte_cryptodev_pmd_destroy(struct rte_cryptodev *cryptodev)
> +{
> +	CDEV_LOG_INFO("Closing crypto device %s [%s]", cryptodev->data->name,
> +			cryptodev->device->name);
> +
> +	/* free crypto device */
> +	rte_cryptodev_pmd_release_device(cryptodev);
> +
> +	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> +		rte_free(cryptodev->data->dev_private);
> +
> +
> +	cryptodev->device = NULL;
> +	cryptodev->data = NULL;
> +
> +	return 0;
> +}
> +
> +/**
> + * Parse name from argument
> + */
> +static int
>  rte_cryptodev_vdev_parse_name_arg(const char *key __rte_unused,
>  		const char *value, void *extra_args)
>  {
> diff --git a/lib/librte_cryptodev/rte_cryptodev_pmd.h b/lib/librte_cryptodev/rte_cryptodev_pmd.h
> index ba074e1..be9eb80 100644
> --- a/lib/librte_cryptodev/rte_cryptodev_pmd.h
> +++ b/lib/librte_cryptodev/rte_cryptodev_pmd.h
> @@ -56,6 +56,35 @@ extern "C" {
>  #include "rte_crypto.h"
>  #include "rte_cryptodev.h"
>
> +
> +#define RTE_CRYPTODEV_PMD_DEFAULT_MAX_NB_QUEUE_PAIRS	8
> +#define RTE_CRYPTODEV_PMD_DEFAULT_MAX_NB_SESSIONS	2048
> +
> +#define RTE_CRYPTODEV_PMD_NAME_ARG			("name")
> +#define RTE_CRYPTODEV_PMD_MAX_NB_QP_ARG			("max_nb_queue_pairs")
> +#define RTE_CRYPTODEV_PMD_MAX_NB_SESS_ARG		("max_nb_sessions")
> +#define RTE_CRYPTODEV_PMD_SOCKET_ID_ARG			("socket_id")
> +
> +
> +static const char * const cryptodev_pmd_valid_params[] = {
> +	RTE_CRYPTODEV_PMD_NAME_ARG,
> +	RTE_CRYPTODEV_PMD_MAX_NB_QP_ARG,
> +	RTE_CRYPTODEV_PMD_MAX_NB_SESS_ARG,
> +	RTE_CRYPTODEV_PMD_SOCKET_ID_ARG
> +};
> +
> +/**
> + * @internal
> + * Initialisation parameters for crypto devices
> + */
> +struct rte_cryptodev_pmd_init_params {
> +	char name[RTE_CRYPTODEV_NAME_MAX_LEN];
> +	size_t private_data_size;
> +	int socket_id;
> +	unsigned int max_nb_queue_pairs;
> +	unsigned int max_nb_sessions;
> +};
> +
>  /** Global structure used for maintaining state of allocated crypto devices */
>  struct rte_cryptodev_global {
>  	struct rte_cryptodev *devs;	/**< Device information array */
> @@ -392,6 +421,65 @@ rte_cryptodev_pmd_allocate(const char *name, int socket_id);
>  extern int
>  rte_cryptodev_pmd_release_device(struct rte_cryptodev *cryptodev);
>
> +
> +/**
> + * @internal
> + *
> + * PMD assist function to parse initialisation arguments for crypto driver
> + * when creating a new crypto PMD device instance.
> + *
> + * PMD driver should set default values for that PMD before calling function,
> + * these default values will be over-written with successfully parsed values
> + * from args string.
> + *
> + * @param	params	parsed PMD initialisation parameters
> + * @param	args	input argument string to parse
> + *
> + * @return
> + *  - 0 on success
> + *  - errno on failure
> + */
> +int
> +rte_cryptodev_pmd_parse_input_args(
> +		struct rte_cryptodev_pmd_init_params *params,
> +		const char *args);
> +
> +/**
> + * @internal
> + *
> + * PMD assist function to provide boiler plate code for crypto driver to create
> + * and allocate resources for a new crypto PMD device instance.
> + *
> + * @param	name	crypto device name.
> + * @param	device	base device instance
> + * @param	params	PMD initialisation parameters
> + *
> + * @return
> + *  - crypto device instance on success
> + *  - NULL on creation failure
> + */
> +struct rte_cryptodev *
> +rte_cryptodev_pmd_create(const char *name,
> +		struct rte_device *device,
> +		struct rte_cryptodev_pmd_init_params *params);
> +
> +/**
> + * @internal
> + *
> + * PMD assist function to provide boiler plate code for crypto driver to
> + * destroy and free resources associated with a crypto PMD device instance.
> + *
> + * @param	name	crypto device name.
> + * @param	device	base device instance
> + * @param	params	PMD initialisation parameters
> + *
> + * @return
> + *  - crypto device instance on success
> + *  - NULL on creation failure
> + */
> +int
> +rte_cryptodev_pmd_destroy(struct rte_cryptodev *cryptodev);
> +
>  /**
>   * Executes all the user application registered callbacks for the specific
>   * device.
> diff --git a/lib/librte_cryptodev/rte_cryptodev_version.map b/lib/librte_cryptodev/rte_cryptodev_version.map
> index 919b6cc..a0ea7bf 100644
> --- a/lib/librte_cryptodev/rte_cryptodev_version.map
> +++ b/lib/librte_cryptodev/rte_cryptodev_version.map
> @@ -84,5 +84,8 @@ DPDK_17.11 {
>  	global:
>
>  	rte_cryptodev_name_get;
> +	rte_cryptodev_pmd_create;
> +	rte_cryptodev_pmd_destroy;
> +	rte_cryptodev_pmd_parse_input_args;
>
>  } DPDK_17.08;
> --
> 2.9.4
>

--
- Tomasz Duszyński