Re: [dpdk-dev] [RFC v2, 2/2] eventdev: add crypto adapter API header

[Jerin Jacob <jerin.jacob@caviumnetworks.com>]

-----Original Message-----
> Date: Mon, 15 Jan 2018 16:23:50 +0530
> From: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
> To: jerin.jacob@caviumnetworks.com
> CC: dev@dpdk.org, narender.vangati@intel.com, Abhinandan Gujjar
>  <abhinandan.gujjar@intel.com>, Nikhil Rao <nikhil.rao@intel.com>, Gage
>  Eads <gage.eads@intel.com>
> Subject: [RFC v2, 2/2] eventdev: add crypto adapter API header
> X-Mailer: git-send-email 1.9.1
> 
> Add crypto event adapter APIs to support packet transfer
> mechanism between cryptodev and event device.
> 
> Signed-off-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
> Signed-off-by: Nikhil Rao <nikhil.rao@intel.com>
> Signed-off-by: Gage Eads <gage.eads@intel.com>
> ---

Overall it looks good to me. Though I have some confusion over
ENQ-DEQ scheme. May be it will be get cleared along with implementation.

> Notes:
> 	V2:
> 	1. Updated type as ENQ-DEQ in rte_event_crypto_adapter_type
> 	2. Removed enum rte_event_crypto_conf_type
> 	3. Updated struct rte_event_crypto_metadata
> 	4. Removed struct rte_event_crypto_queue_pair_conf
> 	5. Updated rte_event_crypto_adapter_queue_pair_add() API
> 
>  lib/librte_eventdev/Makefile                   |   1 +
>  lib/librte_eventdev/rte_event_crypto_adapter.h | 452 +++++++++++++++++++++++++

1) Please update MAINTAINERS file
2) Add meson build support
3) Update doc/api/doxy-api-index.md file and check the doxygen output
using "make doc-api-html"
4) Please add the programmers guide in doc/guides/prog_guide/

>  
> +++ b/lib/librte_eventdev/rte_event_crypto_adapter.h
> @@ -0,0 +1,452 @@
> +/*
> + *   Copyright(c) 2018 Intel Corporation. All rights reserved.
> + *   All rights reserved.
> + *
> + *   Redistribution and use in source and binary forms, with or without

Use SPDX tag scheme

> +#ifndef _RTE_EVENT_CRYPTO_ADAPTER_
> +#define _RTE_EVENT_CRYPTO_ADAPTER_
> +
> +/**
> + * This adapter adds support to enqueue crypto completions to event device.
> + * The packet flow from cryptodev to the event device can be accomplished
> + * using both SW and HW based transfer mechanisms.
> + * The adapter uses a EAL service core function for SW based packet transfer
> + * and uses the eventdev PMD functions to configure HW based packet transfer
> + * between the cryptodev and the event device.
> + *
> + * In the case of SW based transfers, application can choose to submit a

I think, we can remove "In the case of SW based transfers" as it should
be applicable for HW case too

> + * crypto operation directly to cryptodev or send it  to the cryptodev
> + * adapter via eventdev, the cryptodev adapter then submits the crypto
> + * operation to the crypto device. The first mode is known as the

The first mode (DEQ) is very clear. In the second mode(ENQ_DEQ),
- How does "worker" submits the crypto work through crypto-adapter?
If I understand it correctly, "workers" always deals with only 
cryptodev's rte_cryptodev_enqueue_burst() API and "service" function in crypto adapter 
would be responsible for dequeue() from cryptodev and enqueue to eventdev?

I understand the need for OP_NEW vs OP_FWD mode difference in both
modes. Other than that, What makes ENQ_DEQ different? Could you share the
flow for ENQ_DEQ mode with APIs.

> + * dequeue only (DEQ) mode  and the second as the enqueue - dequeue

extra space between "mode" and "and"

> + * (ENQ_DEQ) mode. The choice of mode can be specified when creating
> + * the adapter.
> + * In the latter choice, the cryptodev adapter is able to use
> + * RTE_OP_FORWARD as the event dev enqueue type, this has a performance
> + * advantage in "closed system" eventdevs like the eventdev SW PMD and

s/eventdevs/eventdev ??

> + * also, the event cannot be dropped.
> + *
> + * In the ENQ_DEQ mode the application needs to specify the cryptodev ID
> + * and queue pair ID (request information) in addition to the event
> + * information (response information) needed to enqueue an event after
> + * the crypto operation has completed. The request and response information
> + * are specified in the rte_crypto_op private_data. In the DEQ mode the
> + * the application is required to provide only the response information.

Why ENQ_DEQ modes needs cryptodev ID and queue pair ID? Which is part of
rte_cryptodev_enqueue_burst(). May be I am missing something.

If ENQ_DEQ modes help in SW case then we can add it as capability.
I am just trying to see any scope of convergence.

> + *
> + * In the ENQ_DEQ mode, application sends crypto operations as events to
> + * the adapter which dequeues events and programs cryptodev operations.
> + * The adapter then dequeues crypto completions from cryptodev and
> + * enqueue events to the event device.
> + *
> + * In the case of HW based transfers, the cryptodev PMD callback invoked
> + * from rte_cryptodev_enqueue_burst() uses the response information to
> + * setup the event for the cryptodev completion.
> + *
> + * The event crypto adapter provides common APIs to configure the packet flow
> + * from the cryptodev to event devices across both SW and HW based transfers.
> + * The crypto event adapter's functions are:
> + *  - rte_event_crypto_adapter_create_ext()
> + *  - rte_event_crypto_adapter_create()
> + *  - rte_event_crypto_adapter_free()
> + *  - rte_event_crypto_adapter_queue_pair_add()
> + *  - rte_event_crypto_adapter_queue_pair_del()
> + *  - rte_event_crypto_adapter_start()
> + *  - rte_event_crypto_adapter_stop()
> + *  - rte_event_crypto_adapter_stats_get()
> + *  - rte_event_crypto_adapter_stats_reset()
> +
> + * The applicaton creates an instance using rte_event_crypto_adapter_create()

s/applicaton/application
 
> + * or rte_event_crypto_adapter_create_ext().
> + *
> + * Cryptodev queue pair addition/deletion is done
> + * using the rte_event_crypto_adapter_queue_pair_xxx() APIs.
> + *
> + * The SW adapter or HW PMD uses rte_crypto_op::private_data_type to decide
> + * whether request/response data is located in the crypto session/crypto
> + * security session or at an offset in the rte_crypto_op.
> + * rte_crypto_op::private_data_offset is used to locate the request/response
> + * in the rte_crypto_op. If the rte_crypto_op::private_data_type
> + * indicates that the data is in the crypto session/crypto security session
> + * then the rte_crypto_op::sess_type is used to decide whether the private
> + * data is in the session or security session.
> + *
> + * For session-less it is mandatory to place the request/response data with
> + * the rte_crypto_op where as with crypto session/security session it can be
> + * placed with the rte_crypto_op or in the session/security session.

Please update(if needed) above two paragraphs based on the conclusion
from cryptodev change

> + */
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include <stdint.h>
> +#include <rte_service.h>
> +
> +#include "rte_eventdev.h"
> +
> +#define RTE_EVENT_CRYPTO_ADAPTER_MAX_INSTANCE 32
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this enum may change without prior notice

Change to new EXPERIMENTAL tag scheme.

> + *
> + * Crypto event adapter type
> + */
> +enum rte_event_crypto_adapter_type {
> +	RTE_EVENT_CRYPTO_ADAPTER_DEQ_ONLY = 1,
> +	/**< Start only dequeue part of crypto adapter.
> +	 * Packets dequeued from cryptodev are enqueued to eventdev
> +	 * as new events and events will be treated as RTE_EVENT_OP_NEW. */
> +	RTE_EVENT_CRYPTO_ADAPTER_ENQ_DEQ,
> +	/**< Start both enqueue & dequeue part of crypto adapter.
> +	 * Packet's event context will be retained and
> +	 * event will be treated as RTE_EVENT_OP_FORWARD. */

This description makes sense.

> +};
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice
> + *
> + * Crypto event request structure will be fill by application to
> + * provide event request information to the adapter.
> + */
> +struct rte_event_crypto_request {
> +	uint8_t resv[8];
> +	/**< Overlaps with first 8 bytes of struct rte_event
> +	 * that encode the response event information
> +	 */
> +	uint16_t cdev_id;
> +	/**< cryptodev ID to be used */
> +	uint16_t queue_pair_id;
> +	/**< cryptodev queue pair ID to be used */

Shouldn't we say it is need only with ENQ_DEQ mode as described earlier.
adding "@see" section would help

> +	uint32_t resv1;
> +	/**< Reserved bits */
> +};
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice
> + *
> + * Crypto event metadata structure will be filled by application
> + * to provide crypto request and event response information.
> + *
> + * If crypto events are enqueued using a HW mechanism, the cryptodev
> + * PMD will use the event response information to set up the event
> + * that is enqueued back to eventdev after completion of the crypto
> + * operation. If the transfer is done by SW, it will be used by the
> + * adapter.
> + */
> +union rte_event_crypto_metadata {
> +	struct rte_event_crypto_request request_info;
> +	struct rte_event response_info;

Perfect.

Other than above comments, everything else looks OK to me.