[dpdk-dev] [RFC] Wireless Base Band Device (bbdev)

[dpdk-dev] [RFC] Wireless Base Band Device (bbdev)

[Amr Mokhtar]

Signed-off-by: Amr Mokhtar <amr.mokhtar@intel.com>
---
 lib/librte_bbdev/rte_bbdev.h     | 636 +++++++++++++++++++++++++++++++++++++++
 lib/librte_bbdev/rte_bbdev_op.h  | 333 ++++++++++++++++++++
 lib/librte_bbdev/rte_bbdev_pmd.h | 407 +++++++++++++++++++++++++
 3 files changed, 1376 insertions(+)
 create mode 100644 lib/librte_bbdev/rte_bbdev.h
 create mode 100644 lib/librte_bbdev/rte_bbdev_op.h
 create mode 100644 lib/librte_bbdev/rte_bbdev_pmd.h

diff --git a/lib/librte_bbdev/rte_bbdev.h b/lib/librte_bbdev/rte_bbdev.h
new file mode 100644
index 0000000..557b6fb
--- /dev/null
+++ b/lib/librte_bbdev/rte_bbdev.h
@@ -0,0 +1,636 @@
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2017 Intel Corporation. All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Intel Corporation nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifndef _RTE_BBDEV_H_
+#define _RTE_BBDEV_H_
+
+/**
+ * @file rte_bbdev.h
+ *
+ * Wireless base band device application APIs.
+ *
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * This API allows an application to discover, configure and use a device to
+ * process operations. An asynchronous API (enqueue, followed by later dequeue)
+ * is used for processing operations.
+ *
+ * The functions in this API are not thread-safe when called on the same
+ * target object (a device, or a queue on a device), with the exception that
+ * one thread can enqueue operations to a queue while another thread dequeues
+ * from the same queue.
+ */
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include <stdint.h>
+#include <stdbool.h>
+#include <string.h>
+
+#include <rte_pci.h>
+#include <rte_cpuflags.h>
+#include <rte_memory.h>
+
+#include "rte_bbdev_op.h"
+
+#ifndef RTE_BBDEV_MAX_DEVS
+#define RTE_BBDEV_MAX_DEVS 128  /**< Max number of devices */
+#endif
+
+/**
+ * Get the total number of devices that have been successfully initialised.
+ *
+ * @return
+ *   The total number of usable devices.
+ */
+uint8_t
+rte_bbdev_count(void);
+
+/**
+ * Check if a device is valid.
+ *
+ * @param dev_id
+ *   The identifier of the device.
+ *
+ * @return
+ *   true if device ID is valid and device is attached, false otherwise.
+ */
+bool
+rte_bbdev_is_valid(uint8_t dev_id);
+
+/**
+ * Get the next enabled device.
+ *
+ * @param dev_id
+ *   The current device
+ *
+ * @return
+ *   - The next device, or
+ *   - RTE_BBDEV_MAX_DEVS if none found
+ */
+uint8_t
+rte_bbdev_find_next(uint8_t dev_id);
+
+/** Iterate through all enabled devices */
+#define RTE_BBDEV_FOREACH(i) for (i = rte_bbdev_find_next(-1); \
+		i < RTE_BBDEV_MAX_DEVS; \
+		i = rte_bbdev_find_next(i))
+
+/** Device configuration structure */
+struct rte_bbdev_conf {
+	int socket;  /**< NUMA socket used for memory allocation */
+};
+
+/**
+ * Configure a device.
+ * This function must be called on a device before setting up the queues and
+ * starting the device. It can also be called when a device is in the stopped
+ * state. If any device queues have been configured their configuration will be
+ * cleared by a call to this function.
+ *
+ * @param dev_id
+ *   The identifier of the device.
+ * @param num_queues
+ *   Number of queues to configure on device.
+ * @param conf
+ *   The device configuration. If NULL, a default configuration will be used.
+ *
+ * @return
+ *   - 0 on success
+ *   - EINVAL if num_queues is invalid, 0 or greater than maximum
+ *   - EBUSY if the identified device has already started
+ *   - ENOMEM if unable to allocate memory
+ */
+int
+rte_bbdev_configure(uint8_t dev_id, uint16_t num_queues,
+		const struct rte_bbdev_conf *conf);
+
+/** Device queue configuration structure */
+struct rte_bbdev_queue_conf {
+	int socket;  /**< NUMA socket used for memory allocation */
+	uint32_t queue_size;  /**< Size of queue */
+	uint8_t priority;  /**< Queue priority */
+	bool deferred_start; /**< Do not start queue when device is started. */
+	enum rte_bbdev_op_type op_type; /**< Operation type */
+};
+
+/**
+ * Configure a queue on a device.
+ * This function can be called after device configuration, and before starting.
+ * It can also be called when the device or the queue is in the stopped state.
+ *
+ * @param dev_id
+ *   The identifier of the device.
+ * @param queue_id
+ *   The index of the queue.
+ * @param conf
+ *   The queue configuration. If NULL, a default configuration will be used.
+ *
+ * @return
+ *   - 0 on success
+ *   - EINVAL if the identified queue size or priority are invalid
+ *   - EBUSY if the identified queue or its device have already started
+ */
+int
+rte_bbdev_queue_configure(uint8_t dev_id, uint16_t queue_id,
+		const struct rte_bbdev_queue_conf *conf);
+
+/**
+ * Start a device.
+ * This is the last step needed before enqueueing operations is possible.
+ *
+ * @param dev_id
+ *   The identifier of the device.
+ *
+ * @return
+ *   - 0 on success
+ *   - negative value on failure - as returned from PMD driver
+ */
+int
+rte_bbdev_start(uint8_t dev_id);
+
+/**
+ * Stop a device.
+ * The device can be reconfigured, and restarted after being stopped.
+ *
+ * @param dev_id
+ *   The identifier of the device.
+ *
+ * @return
+ *   - 0 on success
+ */
+int
+rte_bbdev_stop(uint8_t dev_id);
+
+/**
+ * Close a device.
+ * The device cannot be restarted without reconfiguration!
+ *
+ * @param dev_id
+ *   The identifier of the device.
+ *
+ * @return
+ *   - 0 on success
+ */
+int
+rte_bbdev_close(uint8_t dev_id);
+
+/**
+ * Start a specified queue on a device.
+ * This is only needed if the queue has been stopped, or if the deferred_start
+ * flag has been set when configuring the queue.
+ *
+ * @param dev_id
+ *   The identifier of the device.
+ * @param queue_id
+ *   The index of the queue.
+ *
+ * @return
+ *   - 0 on success
+ *   - negative value on failure - as returned from PMD driver
+ */
+int
+rte_bbdev_queue_start(uint8_t dev_id, uint16_t queue_id);
+
+/**
+ * Stop a specified queue on a device, to allow re configuration.
+ *
+ * @param dev_id
+ *   The identifier of the device.
+ * @param queue_id
+ *   The index of the queue.
+ *
+ * @return
+ *   - 0 on success
+ *   - negative value on failure - as returned from PMD driver
+ */
+int
+rte_bbdev_queue_stop(uint8_t dev_id, uint16_t queue_id);
+
+/** Device statistics. */
+struct rte_bbdev_stats {
+	uint64_t enqueued_count;  /**< Count of all operations enqueued */
+	uint64_t dequeued_count;  /**< Count of all operations dequeued */
+	/** Total error count on operations enqueued */
+	uint64_t enqueue_err_count;
+	/** Total error count on operations dequeued */
+	uint64_t dequeue_err_count;
+};
+
+/**
+ * Retrieve the general I/O statistics of a device.
+ *
+ * @param dev_id
+ *   The identifier of the device.
+ * @param stats
+ *   Pointer to structure to where statistics will be copied. On error, this
+ *   location may or may not have been modified.
+ *
+ * @return
+ *   - 0 on success
+ *   - EINVAL if invalid parameter pointer is provided
+ */
+int
+rte_bbdev_stats_get(uint8_t dev_id, struct rte_bbdev_stats *stats);
+
+/**
+ * Reset the statistics of a device.
+ *
+ * @param dev_id
+ *   The identifier of the device.
+ * @return
+ *   - 0 on success
+ */
+int
+rte_bbdev_stats_reset(uint8_t dev_id);
+
+/** Device information supplied by the device's driver */
+struct rte_bbdev_driver_info {
+	/** Driver name */
+	const char *driver_name;
+
+	/** Maximum number of queues supported by the device */
+	unsigned int max_num_queues;
+	/** Queue size limit (queue size must also be power of 2) */
+	uint32_t queue_size_lim;
+	/** Set if device off-loads operation to hardware  */
+	bool hardware_accelerated;
+	/** Max value supported by queue priority */
+	uint8_t max_queue_priority;
+	/** Set if device supports per-queue interrupts */
+	bool queue_intr_supported;
+	/** Minimum alignment of buffers, in bytes */
+	uint16_t min_alignment;
+	/** Default configuration used if none is supplied  */
+	struct rte_bbdev_conf default_conf;
+	/** Default queue configuration used if none is supplied  */
+	struct rte_bbdev_queue_conf default_queue_conf;
+	/** Device operation capabilities */
+	const struct rte_bbdev_op_cap *capabilities;
+	/** Device cpu_flag requirements */
+	const enum rte_cpu_flag_t *cpu_flag_reqs;
+};
+
+/** Macro used at end of bbdev PMD list */
+#define RTE_BBDEV_END_OF_CAPABILITIES_LIST() \
+	{ RTE_BBDEV_OP_NONE }
+
+/* Forward declaration */
+struct rte_pci_device;
+
+/** Device information structure used by an application to discover a devices
+ * capabilities and current configuration
+ */
+struct rte_bbdev_info {
+	int socket_id;  /**< NUMA socket that device is on */
+	const char *dev_name;  /**< Unique device name */
+	const struct rte_pci_device *pci_dev;  /**< PCI information */
+	unsigned int num_queues;  /**< Number of queues currently configured */
+	struct rte_bbdev_conf conf;  /**< Current device configuration */
+	bool started;  /**< Set if device is currently started */
+	struct rte_bbdev_driver_info drv;  /**< Info from device driver */
+};
+
+/**
+ * Retrieve information about a device.
+ *
+ * @param dev_id
+ *   The identifier of the device.
+ * @param dev_info
+ *   Pointer to structure to where information will be copied. On error, this
+ *   location may or may not have been modified.
+ *
+ * @return
+ *   - 0 on success
+ *   - EINVAL if invalid parameter pointer is provided
+ */
+int
+rte_bbdev_info_get(uint8_t dev_id, struct rte_bbdev_info *dev_info);
+
+/** Queue information */
+struct rte_bbdev_queue_info {
+	/** Current device configuration */
+	struct rte_bbdev_queue_conf conf;
+	/** Set if queue is currently started */
+	bool started;
+};
+
+/**
+ * Retrieve information about a specific queue on a device.
+ *
+ * @param dev_id
+ *   The identifier of the device.
+ * @param queue_id
+ *   The index of the queue.
+ * @param dev_info
+ *   Pointer to structure to where information will be copied. On error, this
+ *   location may or may not have been modified.
+ *
+ * @return
+ *   - 0 on success
+ *   - EINVAL if invalid parameter pointer is provided
+ */
+int
+rte_bbdev_queue_info_get(uint8_t dev_id, uint16_t queue_id,
+		struct rte_bbdev_queue_info *dev_info);
+
+/** @internal The data structure associated with each queue of a device. */
+struct rte_bbdev_queue_data {
+	void *queue_private;  /**< Driver-specific per-queue data */
+	struct rte_bbdev_queue_conf conf;  /**< Current configuration */
+	struct rte_bbdev_stats queue_stats;  /**< Queue statistics */
+	bool started;  /**< Queue state */
+};
+
+/** @internal Enqueue operations for processing on queue of a device. */
+typedef uint16_t (*rte_bbdev_enqueue_ops_t)(struct rte_bbdev_queue_data *q_data,
+		struct rte_bbdev_op **ops, uint16_t num);
+
+/** @internal Dequeue operations from a queue of a device. */
+typedef uint16_t (*rte_bbdev_dequeue_ops_t)(struct rte_bbdev_queue_data *q_data,
+		struct rte_bbdev_op **ops, uint16_t num);
+
+#ifndef RTE_BBDEV_NAME_MAX_LEN
+#define RTE_BBDEV_NAME_MAX_LEN  64  /**< Max length of device name */
+#endif
+
+/**
+ * @internal The data associated with a device, with no function pointers.
+ * This structure is safe to place in shared memory to be common among
+ * different processes in a multi-process configuration. Drivers can access
+ * these fields, but should never write to them!
+ */
+struct rte_bbdev_data {
+	char name[RTE_BBDEV_NAME_MAX_LEN]; /**< Unique identifier name */
+	void *dev_private;  /**< Driver-specific private data */
+	uint16_t num_queues;  /**< Number of currently configured queues */
+	struct rte_bbdev_queue_data *queues;  /**< Queue structures */
+	uint8_t dev_id;  /**< Device ID */
+	int socket_id;  /**< NUMA socket that device is on */
+	struct rte_bbdev_conf conf;  /**< Current configuration */
+	bool started;  /**< Device run-time state */
+};
+
+/* Forward declarations */
+struct rte_bbdev_ops;
+struct rte_bbdev_callback;
+struct rte_intr_handle;
+
+/** Structure to keep track of registered callbacks */
+TAILQ_HEAD(rte_bbdev_cb_list, rte_bbdev_callback);
+
+/**
+ * @internal The data structure associated with a device. Drivers can access
+ * these fields, but should only write to the *_ops fields.
+ */
+struct __rte_cache_aligned rte_bbdev {
+	rte_bbdev_enqueue_ops_t enqueue_ops; /**< Enqueue function */
+	rte_bbdev_dequeue_ops_t dequeue_ops;  /**< Dequeue function */
+	const struct rte_bbdev_ops *dev_ops;  /**< Functions exported by PMD */
+	struct rte_bbdev_data *data;  /**< Pointer to device data */
+	bool attached;  /**< If device is currently attached or not */
+	struct rte_device *device; /**< Backing device (HW only) */
+	/** User application callback for interrupts if present */
+	struct rte_bbdev_cb_list list_cbs;
+	struct rte_intr_handle *intr_handle; /**< Device interrupt handle */
+};
+
+/** @internal array of all devices */
+extern struct rte_bbdev rte_bbdev_devices[];
+
+/**
+ * Enqueue a burst of processed operations to a queue of the device.
+ * This functions only enqueues as many operations as currently possible and
+ * does not block until @p num_ops entries in the queue are available.
+ * This function does not provide any error notification to avoid the
+ * corresponding overhead.
+ *
+ * @param dev_id
+ *   The identifier of the device.
+ * @param queue_id
+ *   The index of the queue.
+ * @param ops
+ *   Pointer array containing operations to be enqueued Must have at least
+ *   @p num_ops entries
+ * @param num_ops
+ *   The maximum number of operations to enqueue.
+ *
+ * @return
+ *   The number of operations actually enqueued (this is the number of processed
+ *   entries in the @p ops array).
+ */
+static inline uint16_t
+rte_bbdev_enqueue_ops(uint8_t dev_id, uint16_t queue_id,
+		struct rte_bbdev_op **ops, uint16_t num_ops)
+{
+	struct rte_bbdev *dev = &rte_bbdev_devices[dev_id];
+	struct rte_bbdev_queue_data *q_data = &dev->data->queues[queue_id];
+	uint16_t n = dev->enqueue_ops(q_data, ops, num_ops);
+
+	RTE_LOG_DP(DEBUG, BBDEV, "%u ops enqueued to dev%u,q%u. Type = %s\n",
+			num_ops, dev_id, queue_id,
+			rte_bbdev_op_type_str(ops[0]->type));
+
+	return n;
+}
+
+/**
+ * Dequeue a burst of processed operations from a queue of the device.
+ * This functions returns only the current contents of the queue, and does not
+ * block until @ num_ops is available.
+ * This function does not provide any error notification to avoid the
+ * corresponding overhead.
+ *
+ * @param dev_id
+ *   The identifier of the device.
+ * @param queue_id
+ *   The index of the queue.
+ * @param ops
+ *   Pointer array where operations will be dequeued to. Must have at least
+ *   @p num_ops entries
+ * @param num_ops
+ *   The maximum number of operations to dequeue.
+ *
+ * @return
+ *   The number of operations actually dequeued (this is the number of entries
+ *   copied into the @p ops array).
+ */
+static inline uint16_t
+rte_bbdev_dequeue_ops(uint8_t dev_id, uint16_t queue_id,
+		struct rte_bbdev_op **ops, uint16_t num_ops)
+{
+	struct rte_bbdev *dev = &rte_bbdev_devices[dev_id];
+	struct rte_bbdev_queue_data *q_data = &dev->data->queues[queue_id];
+	uint16_t n = dev->dequeue_ops(q_data, ops, num_ops);
+
+	RTE_LOG_DP(DEBUG, BBDEV, "%u ops dequeued to dev%u,q%u\n",
+			n, dev_id, queue_id);
+
+	return n;
+}
+
+/** Definitions of device event types */
+enum rte_bbdev_event_type {
+	RTE_BBDEV_EVENT_UNKNOWN,  /**< unknown event type */
+	RTE_BBDEV_EVENT_ERROR,  /**< error interrupt event */
+	RTE_BBDEV_EVENT_MAX  /**< max value of this enum */
+};
+
+/**
+ * Typedef for application callback function registered by application
+ * software for notification of device events
+ *
+ * @param dev_id
+ *   Device identifier
+ * @param event
+ *   Device event to register for notification of.
+ * @param cb_arg
+ *   User specified parameter to be passed to user's callback function.
+ */
+typedef void (*rte_bbdev_cb_fn)(uint8_t dev_id,
+		enum rte_bbdev_event_type event, void *cb_arg);
+
+/**
+ * Register a callback function for specific device id. Multiple callbacks can
+ * be added and will be called in the order they are added when an event is
+ * triggered. Callbacks are called in a separate thread created by the DPDK EAL.
+ *
+ * @param dev_id
+ *   Device id.
+ * @param event
+ *   The event that the callback will be registered for.
+ * @param cb_fn
+ *   User supplied callback function to be called.
+ * @param cb_arg
+ *   Pointer to parameter that will be passed to the callback.
+ *
+ * @return
+ *   Zero on success, negative value on failure.
+ */
+int
+rte_bbdev_callback_register(uint8_t dev_id, enum rte_bbdev_event_type event,
+		rte_bbdev_cb_fn cb_fn, void *cb_arg);
+
+/**
+ * Unregister a callback function for specific device id.
+ *
+ * @param dev_id
+ *   The device identifier.
+ * @param event
+ *   The event that the callback will be unregistered for.
+ * @param cb_fn
+ *   User supplied callback function to be unregistered.
+ * @param cb_arg
+ *   Pointer to the parameter supplied when registering the callback.
+ *   (void *)-1 means to remove all registered callbacks with the specified
+ *   function address.
+ *
+ * @return
+ *   - 0 on success
+ *   - EINVAL if invalid parameter pointer is provided
+ *   - EAGAIN if the provided callback pointer does not exist
+ */
+int
+rte_bbdev_callback_unregister(uint8_t dev_id, enum rte_bbdev_event_type event,
+		rte_bbdev_cb_fn cb_fn, void *cb_arg);
+
+/**
+ * Enable a one-shot interrupt on the next operation enqueued to a particular
+ * queue. The interrupt will be triggered when the operation is ready to be
+ * dequeued. To handle the interrupt, an epoll file descriptor must be
+ * registered using rte_bbdev_queue_intr_ctl(), and then an application
+ * thread/lcore can wait for the interrupt using rte_epoll_wait().
+ *
+ * @param dev_id
+ *   The device identifier.
+ * @param queue_id
+ *   The index of the queue.
+ *
+ * @return
+ *   - 0 on success
+ *   - negative value on failure - as returned from PMD driver
+ */
+int
+rte_bbdev_queue_intr_enable(uint8_t dev_id, uint16_t queue_id);
+
+/**
+ * Disable a one-shot interrupt on the next operation enqueued to a particular
+ * queue (if it has been enabled).
+ *
+ * @param dev_id
+ *   The device identifier.
+ * @param queue_id
+ *   The index of the queue.
+ *
+ * @return
+ *   - 0 on success
+ *   - negative value on failure - as returned from PMD driver
+ */
+int
+rte_bbdev_queue_intr_disable(uint8_t dev_id, uint16_t queue_id);
+
+/**
+ * Control interface for per-queue interrupts.
+ *
+ * @param dev_id
+ *   The device identifier.
+ * @param queue_id
+ *   The index of the queue.
+ * @param epfd
+ *   Epoll file descriptor that will be associated with the interrupt source.
+ *   If the special value RTE_EPOLL_PER_THREAD is provided, a per thread epoll
+ *   file descriptor created by the EAL is used (RTE_EPOLL_PER_THREAD can also
+ *   be used when calling rte_epoll_wait()).
+ * @param op
+ *   The operation be performed for the vector.RTE_INTR_EVENT_ADD or
+ *   RTE_INTR_EVENT_DEL.
+ * @param data
+ *   User context, that will be returned in the epdata.data field of the
+ *   rte_epoll_event structure filled in by rte_epoll_wait().
+ *
+ * @return
+ *   - 0 on success
+ *   - ENOTSUP if interrupts are not supported by the identified device
+ *   - negative value on failure - as returned from PMD driver
+ */
+int
+rte_bbdev_queue_intr_ctl(uint8_t dev_id, uint16_t queue_id, int epfd, int op,
+		void *data);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* _RTE_BBDEV_H_ */
diff --git a/lib/librte_bbdev/rte_bbdev_op.h b/lib/librte_bbdev/rte_bbdev_op.h
new file mode 100644
index 0000000..1053175
--- /dev/null
+++ b/lib/librte_bbdev/rte_bbdev_op.h
@@ -0,0 +1,333 @@
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2017 Intel Corporation. All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Intel Corporation nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifndef _RTE_BBDEV_OP_H_
+#define _RTE_BBDEV_OP_H_
+
+/**
+ * @file rte_bbdev_op.h
+ *
+ * Defines wireless base band layer 1 operations and capabilities
+ *
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ */
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include <stdint.h>
+
+#include <rte_common.h>
+#include <rte_mbuf.h>
+#include <rte_memory.h>
+#include <rte_mempool.h>
+
+/** Flags for turbo decoder operation and capability structure */
+enum rte_bbdev_op_td_flag_bitmasks {
+	/** If sub block de-interleaving is to be performed. */
+	RTE_BBDEV_TURBO_SUBBLOCK_DEINTERLEAVE = (1ULL << 0),
+	/** To use CRC Type 24B (otherwise use CRC Type 24A). */
+	RTE_BBDEV_TURBO_CRC_TYPE_24B = (1ULL << 1),
+	/** If turbo equalization is to be performed. */
+	RTE_BBDEV_TURBO_EQUALIZER = (1ULL << 2),
+	/** If set, saturate soft output to +/-127 */
+	RTE_BBDEV_TURBO_SOFT_OUT_SATURATE = (1ULL << 3),
+	/**
+	 * Set to 1 to start iteration from even, else odd; one iteration =
+	 * max_iteration + 0.5
+	 */
+	RTE_BBDEV_TURBO_HALF_ITERATION_EVEN = (1ULL << 4),
+	/**
+	 * If 0, TD stops after CRC matches; else if 1, runs to end of next
+	 * odd iteration after CRC matches
+	 */
+	RTE_BBDEV_TURBO_CONTINUE_CRC_MATCH = (1ULL << 5),
+	/** Set if soft output is required to be output  */
+	RTE_BBDEV_TURBO_SOFT_OUTPUT = (1ULL << 6),
+	/** Set to enable early termination mode */
+	RTE_BBDEV_TURBO_EARLY_TERMINATION = (1ULL << 7),
+	/**
+	 * Set if the input is a raw data (E bytes, no NULL bytes). If not set
+	 * the input is a full circular buffer with data (Kw bytes) as decribed
+	 * in spec. 36.212, chapter 5.1.4.1.2.
+	 */
+	RTE_BBDEV_TURBO_RAW_INPUT_DATA = (1ULL << 8),
+};
+
+/** Flags for turbo encoder operation and capability structure */
+enum rte_bbdev_op_te_flag_bitmasks {
+	/** Ignore rv_index and set K0 = 0 */
+	RTE_BBDEV_TURBO_RV_INDEX_BYPASS = (1ULL << 0),
+	/** If rate matching is to be performed */
+	RTE_BBDEV_TURBO_RATE_MATCH = (1ULL << 1),
+	/** This bit must be set to enable CRC-24B generation */
+	RTE_BBDEV_TURBO_CRC_24B_ATTACH = (1ULL << 2),
+	/** This bit must be set to enable CRC-24A generation */
+	RTE_BBDEV_TURBO_CRC_24A_ATTACH = (1ULL << 3)
+};
+
+/** Data input and output buffer for Turbo operations */
+struct rte_bbdev_op_data {
+	struct rte_mbuf *data;
+	/**< First mbuf segment with input/output data. */
+	uint32_t offset;
+	/**< The starting point for the Turbo input/output, in bytes, from the
+	 * start of the data in the data buffer. It must be smaller than
+	 * data_len of the mbuf's first segment!
+	 */
+	uint32_t length;
+	/**< For input operations: the length, in bytes, of the source buffer
+	 * on which the Turbo encode/decode will be computed.
+	 * For output operations: the length, in bytes, of the output buffer
+	 * of the Turbo operation.
+	 */
+};
+
+/** Operation structure for the Turbo Decoder */
+struct rte_bbdev_op_turbo_dec {
+	struct rte_bbdev_op_data input; /**< input src data */
+	struct rte_bbdev_op_data hard_output; /**< hard output buffer */
+	struct rte_bbdev_op_data soft_output; /**< soft output buffer */
+
+	uint32_t op_flags;  /**< Flags from rte_bbdev_op_td_flag_bitmasks */
+	uint32_t e;  /**< E parameter for TEQ rate matching */
+	uint16_t k;  /**< size of the input code block in bits (40 - 6144) */
+	uint8_t rv_index;  /**< Rv index for rate matching (0 - 3) */
+	uint8_t iter_min:4;  /**< min number of iterations */
+	uint8_t iter_max:4;  /**< max number of iterations */
+	uint8_t iter_count;  /**< Actual num. of iterations performed */
+	/** 5 bit extrinsic scale (scale factor on extrinsic info) */
+	uint8_t ext_scale;
+	/** Number of MAP engines, must be power of 2 (or 0 to auto-select) */
+	uint8_t num_maps;
+};
+
+/** Operation structure for the Turbo Encoder */
+struct rte_bbdev_op_turbo_enc {
+	struct rte_bbdev_op_data input; /**< input src data */
+	struct rte_bbdev_op_data output; /**< output buffer */
+
+	uint32_t op_flags;  /**< Flags from rte_bbdev_op_te_flag_bitmasks */
+	uint16_t k;  /**< size of the input code block in bits (40 - 6144) */
+	uint32_t e;  /**< length in bits of the rate match output (17 bits) */
+	int32_t n_soft;  /**< total number of soft bits according to UE cat. */
+	int32_t k_mimo;  /**< MIMO type */
+	int32_t mdl_harq;  /**< the maximum number of DL HARQ processes */
+	/** total number of bits available for transmission of one TB */
+	int32_t g;
+	int32_t nl;  /**< number of layer */
+	int32_t qm;  /**< modulation type */
+	/** Ncb parameter for rate matching, range [k : 3(k+4)] */
+	uint16_t ncb;
+	uint8_t rv_index;  /**< Rv index for rate matching (0 - 3) */
+};
+
+/** List of the capabilities for the Turbo Decoder */
+struct rte_bbdev_op_cap_turbo_dec {
+	/** Flags from rte_bbdev_op_td_flag_bitmasks */
+	uint32_t capability_flags;
+	uint8_t num_buffers_src;  /**< Num scatter-gather buffers */
+	uint8_t num_buffers_hard_out;  /**< Num scatter-gather buffers */
+	uint8_t num_buffers_soft_out;  /**< Num scatter-gather buffers */
+};
+
+/** List of the capabilities for the Turbo Encoder */
+struct rte_bbdev_op_cap_turbo_enc {
+	/** Flags from rte_bbdev_op_te_flag_bitmasks */
+	uint32_t capability_flags;
+	uint8_t num_buffers_src;  /**< Num scatter-gather buffers */
+	uint8_t num_buffers_dst;  /**< Num scatter-gather buffers */
+};
+
+/** Different operation types supported by the device */
+enum rte_bbdev_op_type {
+	RTE_BBDEV_OP_NONE = 0,  /**< Dummy operation that does nothing */
+	RTE_BBDEV_OP_TURBO_DEC,  /**< Turbo decode */
+	RTE_BBDEV_OP_TURBO_ENC,  /**< Turbo encode */
+	RTE_BBDEV_OP_TYPE_COUNT,  /**< Count of different op types */
+};
+
+/** Bit indexes of possible errors reported through status field */
+enum {
+	RTE_BBDEV_DRV_ERROR = 0,
+	RTE_BBDEV_DATA_ERROR,
+	RTE_BBDEV_CRC_ERROR,
+};
+
+/** Structure specifying a single operation */
+struct rte_bbdev_op {
+	enum rte_bbdev_op_type type;  /**< Type of this operation */
+	int status;  /**< Status of operation that was performed */
+	struct rte_mempool *mempool;  /**< Mempool which op instance is in */
+	void *opaque_data;  /**< Opaque pointer for user data */
+	/**
+	 * Anonymous union of operation-type specific parameters. When allocated
+	 * using rte_bbdev_op_pool_create(), space is allocated for the
+	 * parameters at the end of each rte_bbdev_op structure, and the
+	 * pointers here point to it.
+	 */
+	RTE_STD_C11
+	union {
+		void *generic;
+		struct rte_bbdev_op_turbo_dec *turbo_dec;
+		struct rte_bbdev_op_turbo_enc *turbo_enc;
+	};
+};
+
+/** Operation capabilities supported by a device */
+struct rte_bbdev_op_cap {
+	enum rte_bbdev_op_type type;  /**< Type of operation */
+	union {
+		struct rte_bbdev_op_cap_turbo_dec turbo_dec;
+		struct rte_bbdev_op_cap_turbo_enc turbo_enc;
+	} cap;  /**< Operation-type specific capabilities */
+};
+
+/** @internal Private data structure stored with operation pool. */
+struct rte_bbdev_op_pool_private {
+	enum rte_bbdev_op_type type;  /**< Type of operations in a pool */
+};
+
+/**
+ * Converts queue operation type from enum to string
+ *
+ * @param op_type
+ *   Operation type as enum
+ *
+ * @returns
+ *   Operation type as string
+ *
+ */
+const char*
+rte_bbdev_op_type_str(enum rte_bbdev_op_type op_type);
+
+/**
+ * Creates a bbdev operation mempool
+ *
+ * @param name
+ *   Pool name.
+ * @param type
+ *   Operation type, use RTE_BBDEV_OP_NONE for a pool which supports all
+ *   operation types.
+ * @param num_elements
+ *   Number of elements in the pool.
+ * @param cache_size
+ *   Number of elements to cache on an lcore, see rte_mempool_create() for
+ *   further details about cache size.
+ * @param socket_id
+ *   Socket to allocate memory on.
+ *
+ * @return
+ *   - Pointer to a mempool on success,
+ *   - NULL pointer on failure.
+ */
+struct rte_mempool *
+rte_bbdev_op_pool_create(const char *name, enum rte_bbdev_op_type type,
+		unsigned int num_elements, unsigned int cache_size,
+		int socket_id);
+
+/**
+ * Bulk allocate operations from a mempool with parameter defaults reset.
+ *
+ * @param mempool
+ *   Operation mempool, created by rte_bbdev_op_pool_create().
+ * @param type
+ *   Operation type to allocate
+ * @param ops
+ *   Output array to place allocated operations
+ * @param num_ops
+ *   Number of operations to allocate
+ *
+ * @returns
+ *   - 0 on success
+ *   - EINVAL if invalid mempool is provided
+ */
+static inline int
+rte_bbdev_op_alloc_bulk(struct rte_mempool *mempool,
+		enum rte_bbdev_op_type type, struct rte_bbdev_op **ops,
+		uint16_t num_ops)
+{
+	struct rte_bbdev_op_pool_private *priv;
+	uint16_t i;
+	int ret;
+
+	/* Check type */
+	priv = (struct rte_bbdev_op_pool_private *)
+			rte_mempool_get_priv(mempool);
+	if (unlikely((priv->type != type) &&
+			(priv->type != RTE_BBDEV_OP_NONE)))
+		return -EINVAL;
+
+	/* Get elements */
+	ret = rte_mempool_get_bulk(mempool, (void **)ops, num_ops);
+	if (unlikely(ret < 0))
+		return ret;
+
+	/* Reset to default */
+	for (i = 0; i < num_ops; i++) {
+		struct rte_bbdev_op *op = ops[i];
+		op->type = type;
+	}
+
+	RTE_LOG_DP(DEBUG, BBDEV, "%u ops allocated from %s, type = %s\n",
+			num_ops, mempool->name,
+			rte_bbdev_op_type_str(type));
+
+	return 0;
+}
+
+/**
+ * Free operation structures that were allocated by rte_bbdev_op_alloc_bulk().
+ * All structures must belong to the same mempool.
+ *
+ * @param ops
+ *   Operation structures
+ * @param num_ops
+ *   Number of structures
+ */
+static inline void
+rte_bbdev_op_free_bulk(struct rte_bbdev_op **ops, unsigned int num_ops)
+{
+	if (num_ops > 0) {
+		rte_mempool_put_bulk(ops[0]->mempool, (void **)ops, num_ops);
+		RTE_LOG_DP(DEBUG, BBDEV, "%u ops freed to %s\n", num_ops,
+				ops[0]->mempool->name);
+	}
+}
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* _RTE_BBDEV_OP_H_ */
diff --git a/lib/librte_bbdev/rte_bbdev_pmd.h b/lib/librte_bbdev/rte_bbdev_pmd.h
new file mode 100644
index 0000000..8b816a6
--- /dev/null
+++ b/lib/librte_bbdev/rte_bbdev_pmd.h
@@ -0,0 +1,407 @@
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2017 Intel Corporation. All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Intel Corporation nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifndef _RTE_BBDEV_PMD_H_
+#define _RTE_BBDEV_PMD_H_
+
+/**
+ * @file rte_bbdev_pmd.h
+ *
+ * Wireless base band driver-facing APIs.
+ *
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * This API provides the mechanism for device drivers to register with the
+ * bbdev interface. User applications should not use this API.
+ */
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include <stdint.h>
+#include <rte_pci.h>
+#include <rte_log.h>
+
+#include "rte_bbdev.h"
+
+/**
+ * Helper macro for logging
+ *
+ * @param level
+ *   Log level: EMERG, ALERT, CRIT, ERR, WARNING, NOTICE, INFO, or DEBUG
+ * @param fmt
+ *   The format string, as in printf(3).
+ * @param ...
+ *   The variable arguments required by the format string.
+ *
+ * @return
+ *   - 0 on success
+ *   - Negative on error
+ */
+#define rte_bbdev_log(level, fmt, ...) \
+		RTE_LOG(level, BBDEV, fmt "\n", ##__VA_ARGS__)
+
+/**
+ * Helper macro for debug logging with extra source info
+ *
+ * @param fmt
+ *   The format string, as in printf(3).
+ * @param ...
+ *   The variable arguments required by the format string.
+ *
+ * @return
+ *   - 0 on success
+ *   - Negative on error
+ */
+#define rte_bbdev_log_debug(fmt, ...) \
+		rte_bbdev_log(DEBUG, RTE_STR(__LINE__) ":%s() " fmt, __func__, \
+			##__VA_ARGS__)
+
+/**
+ * Helper macro for extra conditional logging from datapath
+ *
+ * @param fmt
+ *   The format string, as in printf(3).
+ * @param ...
+ *   The variable arguments required by the format string.
+ *
+ * @return
+ *   - 0 on success
+ *   - Negative on error
+ */
+#ifdef RTE_LIBRTE_BBDEV_DEBUG
+#define rte_bbdev_log_verbose(fmt, ...)  rte_bbdev_log_debug(fmt, ##__VA_ARGS__)
+#else
+#define rte_bbdev_log_verbose(fmt, ...)
+#endif
+
+/** Suggested value for SW based devices */
+#define RTE_BBDEV_DEFAULT_MAX_NB_QUEUES RTE_MAX_LCORE
+
+/** Suggested value for SW based devices */
+#define RTE_BBDEV_QUEUE_SIZE_LIMIT 16384
+
+/**
+ * Initialisation function of a HW driver invoked for each matching HW device
+ * detected during the EAL initialisation phase, or when a new device is
+ * attached. The driver should initialise the device and its own software
+ * context.
+ *
+ * @param dev
+ *   This is a new device structure instance that is associated with the
+ *   matching device.
+ *   The driver *must* populate the following fields:
+ *    - dev_ops
+ *    - enqueue_ops
+ *    - dequeue_ops
+ *
+ * @return
+ *   - 0 on success
+ */
+typedef int (*rte_bbdev_init_t)(struct rte_bbdev *dev);
+
+/**
+ * Finalization function of a HW driver invoked for each matching HW device
+ * detected during the closing phase, or when a device is detached.
+ *
+ * @param dev
+ *   The device structure instance that is associated with the matching device.
+ *
+ * @return
+ *   - 0 on success
+ */
+typedef int (*rte_bbdev_uninit_t)(struct rte_bbdev *dev);
+
+/**
+ * @internal
+ * Wrapper for use by pci drivers as a .probe function to attach to a bbdev
+ * interface.
+ */
+int
+rte_bbdev_pci_generic_probe(struct rte_pci_device *pci_dev,
+		size_t private_data_size,
+		rte_bbdev_init_t dev_init);
+
+/**
+ * @internal
+ * Wrapper for use by pci drivers as a .remove function to detach a bbdev
+ * interface.
+ */
+int
+rte_bbdev_pci_generic_remove(struct rte_pci_device *pci_dev,
+		rte_bbdev_uninit_t dev_uninit);
+
+/**
+ * Creates and initialises a new device. This function should be called by the
+ * .probe callback defined in "struct rte_vdev_driver" for virtual drivers and
+ * in "struct rte_pci_driver" for hardware drivers. Since HW should use
+ * rte_bbdev_hw_probe() this function must be called by a specific virtual
+ * device probe() function.
+ *
+ * Example usage:
+ * @code
+ * static int
+ * my_vdevice_driver_probe(const char *name, const char *args)
+ * {
+ *     ...
+ *     vdev = rte_bbdev_driver_init(name,
+ *         sizeof(struct my_device_private_data), socket)
+ *     ...
+ * }
+ *
+ * static struct rte_vdev_driver my_vdevice_eal_driver = {
+ *     .probe = my_vdevice_driver_probe,
+ *     .remove = my_vdevice_driver_remove,
+ * };
+ *
+ * RTE_PMD_REGISTER_VDEV(driver_name, my_vdevice_eal_driver);
+ * RTE_PMD_REGISTER_ALIAS(driver_name, alias);
+ * RTE_PMD_REGISTER_PARAM_STRING(driver_name, custom_params_format);
+ * @endcode
+ *
+ * @param name
+ *   Unique device name.
+ * @param dev_private_size
+ *   Size of device private data.
+ * @param socket_id
+ *   Socket to allocate resources on.
+ *
+ * @return
+ *   - Pointer to the new device.
+ *     The caller of this function *must* then populate the following fields
+ *     and only these fields before returning.
+ *      - dev_ops
+ *      - enqueue_ops
+ *      - dequeue_ops
+ *   - NULL otherwise
+ */
+struct rte_bbdev *
+rte_bbdev_driver_init(const char *name, size_t dev_private_size,
+		int socket_id);
+
+/**
+ * Destroys a previously created device. This function should be called by the
+ * .remove callback defined in "struct rte_vdev_driver" for virtual drivers and
+ * in "struct rte_pci_driver" for hardware drivers. Since HW should use
+ * rte_bbdev_hw_remove() this function must be called by a specific virtual
+ * device remove() function.
+ *
+ * Example usage:
+ * @code
+ * static int
+ * my_vdevice_driver_remove(const char *name)
+ * {
+ *     ...
+ *     vdev = rte_bbdev_driver_uninit(name)
+ *     ...
+ * }
+ *
+ * static struct rte_vdev_driver my_vdevice_eal_driver = {
+ *     .probe = my_vdevice_driver_probe,
+ *     .remove = my_vdevice_driver_remove,
+ * };
+ *
+ * RTE_PMD_REGISTER_VDEV(driver_name, my_vdevice_eal_driver);
+ * RTE_PMD_REGISTER_ALIAS(driver_name, alias);
+ * RTE_PMD_REGISTER_PARAM_STRING(driver_name, custom_params_format);
+ * @endcode
+ *
+ * @param name
+ *   Unique device name.
+ *
+ * @return
+ *   - 0 on success
+ *   - EINVAL if invalid parameter pointer is provided
+ *   - ENODEV if unable to find the named device
+ */
+int
+rte_bbdev_driver_uninit(const char *name);
+
+/**
+ * Get the device structure for a named device.
+ *
+ * @param name
+ *   Name of the device
+ *
+ * @return
+ *   - The device structure pointer, or
+ *   - NULL otherwise
+ *
+ */
+struct rte_bbdev *
+rte_bbdev_get_named_dev(const char *name);
+
+/**
+ * Definitions of all functions exported by a driver through the the generic
+ * structure of type *rte_bbdev_ops* supplied in the *rte_bbdev* structure
+ * associated with a device.
+ */
+
+/** @internal Function used to configure a device. */
+typedef int (*rte_bbdev_configure_t)(struct rte_bbdev *dev, uint16_t num_queues,
+		const struct rte_bbdev_conf *conf);
+
+/** @internal Function to allocate and configure a device queue. */
+typedef int (*rte_bbdev_queue_setup_t)(struct rte_bbdev *dev,
+		uint16_t queue_id, const struct rte_bbdev_queue_conf *conf);
+
+/* @internal
+ * Function to release memory resources allocated for a device queue.
+ */
+typedef int (*rte_bbdev_queue_release_t)(struct rte_bbdev *dev,
+		uint16_t queue_id);
+
+/** @internal Function to start a configured device. */
+typedef int (*rte_bbdev_start_t)(struct rte_bbdev *dev);
+
+/** @internal Function to stop a device. */
+typedef void (*rte_bbdev_stop_t)(struct rte_bbdev *dev);
+
+/** @internal Function to close a device. */
+typedef int (*rte_bbdev_close_t)(struct rte_bbdev *dev);
+
+/** @internal Function to start a device queue. */
+typedef int (*rte_bbdev_queue_start_t)(struct rte_bbdev *dev,
+		uint16_t queue_id);
+
+/** @internal Function to stop a device queue. */
+typedef int (*rte_bbdev_queue_stop_t)(struct rte_bbdev *dev, uint16_t queue_id);
+
+/** @internal Function to read stats from a device. */
+typedef void (*rte_bbdev_stats_get_t)(struct rte_bbdev *dev,
+		struct rte_bbdev_stats *stats);
+
+/** @internal Function to reset stats on a device. */
+typedef void (*rte_bbdev_stats_reset_t)(struct rte_bbdev *dev);
+
+/** @internal Function to retrieve specific information of a device. */
+typedef void (*rte_bbdev_info_get_t)(struct rte_bbdev *dev,
+		struct rte_bbdev_driver_info *dev_info);
+
+/** @internal Function to retrieve specific information of a device. */
+typedef void (*rte_bbdev_info_get_t)(struct rte_bbdev *dev,
+		struct rte_bbdev_driver_info *dev_info);
+
+/* @internal
+ * Function to enable interrupt for next op on a queue of a device.
+ */
+typedef int (*rte_bbdev_queue_intr_enable_t)(struct rte_bbdev *dev,
+				    uint16_t queue_id);
+
+/* @internal
+ * Function to disable interrupt for next op on a queue of a device.
+ */
+typedef int (*rte_bbdev_queue_intr_disable_t)(struct rte_bbdev *dev,
+				    uint16_t queue_id);
+
+/**
+ * Operations implemented by drivers. Fields marked as "Required" must be
+ * provided by a driver for a device to have basic functionality. "Optional"
+ * fields are for non-vital operations
+ */
+struct rte_bbdev_ops {
+	/**< Configure device. Optional. */
+	rte_bbdev_configure_t configure;
+	/**< Start device. Optional. */
+	rte_bbdev_start_t start;
+	/**< Stop device. Optional. */
+	rte_bbdev_stop_t stop;
+	/**< Close device. Optional. */
+	rte_bbdev_close_t close;
+
+	/**< Get device info. Required. */
+	rte_bbdev_info_get_t info_get;
+	/** Get device statistics. Optional. */
+	rte_bbdev_stats_get_t stats_get;
+	/** Reset device statistics. Optional. */
+	rte_bbdev_stats_reset_t stats_reset;
+
+	/** Set up a device queue. Required. */
+	rte_bbdev_queue_setup_t queue_setup;
+	/** Release a queue. Required. */
+	rte_bbdev_queue_release_t queue_release;
+	/** Start a queue. Optional. */
+	rte_bbdev_queue_start_t queue_start;
+	/**< Stop a queue pair. Optional. */
+	rte_bbdev_queue_stop_t queue_stop;
+
+	/** Enable queue interrupt. Optional */
+	rte_bbdev_queue_intr_enable_t queue_intr_enable;
+	/** Disable queue interrupt. Optional */
+	rte_bbdev_queue_intr_disable_t queue_intr_disable;
+};
+
+/**
+ * Executes all the user application registered callbacks for the specific
+ * device and event type.
+ *
+ * @param dev
+ *   Pointer to the device structure.
+ * @param event
+ *   Event type.
+ */
+void
+rte_bbdev_pmd_callback_process(struct rte_bbdev *dev,
+	enum rte_bbdev_event_type event);
+
+/**
+ *  Initialisation params structure that can be used by software based drivers
+ */
+struct rte_bbdev_init_params {
+	int socket_id;  /**< Base band null device socket */
+	uint16_t queues_num;  /**< Base band null device queues number */
+};
+
+/**
+ * Parse generic parameters that could be used for software based devices.
+ *
+ * @param params
+ *   Pointer to structure that will hold the parsed parameters.
+ * @param input_args
+ *   Pointer to arguments to be parsed.
+ *
+ * @return
+ *   - 0 on success
+ *   - EINVAL if invalid parameter pointer is provided
+ *   - EFAULT if unable to parse provided arguments
+ */
+int
+rte_bbdev_parse_params(struct rte_bbdev_init_params *params,
+		const char *input_args);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* _RTE_BBDEV_PMD_H_ */
-- 
1.9.1

[dpdk-dev] [RFC] Wireless Base Band Device (bbdev)

[Amr Mokhtar]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 9259 bytes --]

This RFC describes a proposal for the Wireless Base Band Device (bbdev) in DPDK 
that abstracts HW accelerators based on FPGA and/or Fixed Function Accelerators 
that assist with LTE Physical Layer processing. Furthermore, it decouples the 
application from the compute-intensive wireless functions by abstracting their 
optimized libraries to appear as virtual bbdev devices. 
This makes bbdev a common programming framework that enables the same 
application code to be run on different systems with a single software 
architecture and programming model. If the system has hardware accelerators, 
they will be used, but if the system does not have hardware accelerators, 
software implementations can be used.

The proposed bbdev is designed in a lookaside model where the operation to be 
processed is first enqueued asynchronously, and then the result is later 
dequeued similar to existing lookaside models.

The proposed DPDK Base Band device framework meets the following requirements:
 1. Enumerates bbdev hardware devices and load corresponding drivers.
 2. Abstracts the same functionality through the optimized software libraries in 
    case HW device is not existent.
 3. Seamless interface for underlying operations (software or hardware)
 4. Pluggable drivers for various parts of the packet processing
 5. APIs to:
    - Probe wireless device capabilities and resources
    - Configure, start, stop, close and retrieve information of wireless devices
    - Configure, start, stop and retrieve information of operating queues
    - Enqueue/dequeue operations to wireless device queues
    - Reset and retrieve device and queue statistics
    - Support interrupts from HW

The proposed approach is to have a single wireless device interface (bbdev) that 
is used for CRC, Rate (De)Matching and Turbo coding functionality supporting LTE 
physical Layer use cases. A general term for base band is used in the naming to 
allow for a combination of functions to be deployed for flexible and 
programmable devices are used such as FPGAs.  

The wireless Base Band device interface (bbdev) cannot be looked at as a 
subsidiary device class of cryptodev framework, bbdev follows similar design 
approach but it is different in definition and from operation perspective.  
The bbdev does not require the session management approach that cryptodev needs 
to function. Furthermore, bbdev is an abstraction framework that abstracts 
various device functions for numerous operations in the LTE physical layer like 
Turbo coding and rate matching, which cannot be considered operations of 
cryptography. Also, a bbdev device can support multiple wireless functions under 
one device ID.

Other design approaches where considered during design selection phase like a 
wireless device interface is to have individual interfaces for each operation 
type within the LTE physical layer. This somewhat follows the cryptodev model, 
where the device interface can be used to perform a single class of operation 
type. However, it is difficult to map a device which performs multiple 
operations into this model. Consider the hardware accelerator that performs 
Rate (De)Matching, Turbo encoding/decoding (Turbo is a Forward Error Correction 
algorithm) and CRC handling. The device would have to register with three 
interfaces, and it would need three look-aside operations to do the processing 
(impacting performance).
It is not correct to use it with a FEC (Forward Error Correction) device 
interface, as the device does more than that. Also, there is a wide range of 
FEC algorithms, many of which have no parameters or use-cases in common with 
Turbo (for example Reed-Solomon used in storage), so the usefulness of such an 
interface is questionable.

The initial release of the bbdev includes CRC attachment, Turbo Coding and 
Rate (De)Matching supported in software.

A device reports its capabilities when registering itself in the bbdev framework. 
With the aid of this capabilities mechanism, an application can query devices to 
discover which operations within the LTE physical layer they are capable of 
performing.

Turbo code software library can be added as a virtual device to simulate the 
functionality being performed by the HW in case it is not existent or in case 
the application needs to use the two flavors to suit different types of 
workloads, for example: short payloads to use software Turbo and the HW for 
larger payloads. This software library is not part of DPDK mainstream, but it 
can be downloaded from an external link and linked to DPDK at compile time.

For simplicity, the initial software devices are designed to perform the 
operation in the thread context that calls the enqueue function. The result of 
the operation will be put onto an internal rte_ring based queue, waiting for 
dequeue to be called. As device queues are not thread safe, the single-producer, 
single-consumer version of rte_ring queue can be used.

A device ID represents a handle that is used by the application to refer to a 
specific instance of a bbdev device. The range of device IDs for bbdevs is not 
linked or related in any way with device IDs used for cryptodevs or ethdevs.

The application can query how many bbdevs were discovered by the EAL through 
rte_bbdev_count() and then knows the range of valid device IDs that can be used 
for further device interaction.

Once a device is present in the applications context, the application can 
discover some information about the exact device type and capabilities by 
calling rte_bbdev_info_get(dev_id, &info). Capabilities (in terms of operations 
supported, max number of queues, etc.) may be different for each device type so 
this is an important step for an application that is not highly coupled to a 
specific device type.

>From the application point of view, each instance of a bbdev device consists of 
one or more queues identified by queue IDs. While different devices may have 
different capabilities (e.g. support different operation types), all queues on 
a device support identical configuration possibilities. A queue is configured 
for only one type of operation and is configured at initializations time. 
When an operation is enqueued to a specific queue ID, the result is dequeued 
from the same queue ID.

Configuration of a device has two different levels: configuration that applies 
to the whole device, and configuration that applies to a single queue. 
Device configuration is applied with rte_bbdev_configure(dev_id,num_queues,conf) 
and queue configuration is applied with 
rte_bbdev_queue_configure(dev_id, queue_id,conf). Note that, although all queues 
on a device support same capabilities, they can be configured differently and 
will then behave differently.

After initialization, devices are in a stopped state, so must be started by the 
application. By default, all queues are started when the device is started, but 
they can be stopped individually. If an application is finished using a device 
it can close the device. Once closed, it cannot be restarted without 
reconfiguration.

rte_bbdev_start(dev_id)
rte_bbdev_queue_start(dev_id, queue_id) 
rte_bbdev_queue_stop(dev_id, queue_id) 
rte_bbdev_stop(dev_id) 
rte_bbdev_close(dev_id)

Operations on a buffer (or buffers) are performed by an asynchronous API. 
Operations are enqueued to the device, and then dequeued later. 
Ordering of operations is maintained between the enqueue and dequeue.

rte_bbdev_enqueue_ops(dev_id, queue_id, **ops, num_ops)
rte_bbdev_dequeue_ops(dev_id, queue_id, **ops, num_ops)

Queues are not thread-safe and the use of multiple queues or application-level 
locking is required for multi-threaded applications that share a device. 
Note that it is however acceptable for one thread to enqueue to a queue ID and 
another thread to dequeue from the same queue ID.  This could be used to 
implement a multithreaded pipeline where each thread dequeues from a bbdev 
device, before enqueueing to the next.
The number of queues supported by the device can be queried through 
rte_bbdev_info_get().

**ops is an array of pointers to struct rte_bbdev_op structures which contain 
all the details needed by the device to perform a single operation. 
As the bbdev interface supports different operation types (although individual 
devices may only support a subset of these), it contains a type field, and a 
union of parameters for each operation type.

struct rte_bbdev_op {
	enum rte_bbdev_op_type type;
	…
	union {
		void *generic;
		struct rte_bbdev_op_turbo_dec *turbo_dec;
		struct rte_bbdev_op_turbo_enc *turbo_enc;
	};
};


Find the enclosed patch for the complete API specification, application- and 
driver-facing APIs.

Looking forward to getting comments from both the application and driver 


Amr Mokhtar (1):
  Wireless Base Band Device (bbdev)

 lib/librte_bbdev/rte_bbdev.h     | 636 +++++++++++++++++++++++++++++++++++++++
 lib/librte_bbdev/rte_bbdev_op.h  | 333 ++++++++++++++++++++
 lib/librte_bbdev/rte_bbdev_pmd.h | 407 +++++++++++++++++++++++++
 3 files changed, 1376 insertions(+)
 create mode 100644 lib/librte_bbdev/rte_bbdev.h
 create mode 100644 lib/librte_bbdev/rte_bbdev_op.h
 create mode 100644 lib/librte_bbdev/rte_bbdev_pmd.h

-- 
1.9.1

Re: [dpdk-dev] [RFC] Wireless Base Band Device (bbdev)

[Mokhtar, Amr]

Hello,
Following on the proposal of bbdev, is there any comments. You feedback is highly appreciated.
May I propose to take this topic up to discussion by the Technical Board at next convenient time?

Regards,
Amr

> -----Original Message-----
> From: Mokhtar, Amr
> Sent: Friday 25 August 2017 14:47
> To: dev@dpdk.org
> Cc: Mokhtar, Amr <amr.mokhtar@intel.com>
> Subject: [RFC] Wireless Base Band Device (bbdev)
> 
> This RFC describes a proposal for the Wireless Base Band Device (bbdev) in DPDK
> that abstracts HW accelerators based on FPGA and/or Fixed Function
> Accelerators
> that assist with LTE Physical Layer processing. Furthermore, it decouples the
> application from the compute-intensive wireless functions by abstracting their
> optimized libraries to appear as virtual bbdev devices.
> This makes bbdev a common programming framework that enables the same
> application code to be run on different systems with a single software
> architecture and programming model. If the system has hardware accelerators,
> they will be used, but if the system does not have hardware accelerators,
> software implementations can be used.
> 
> The proposed bbdev is designed in a lookaside model where the operation to be
> processed is first enqueued asynchronously, and then the result is later
> dequeued similar to existing lookaside models.
> 
> The proposed DPDK Base Band device framework meets the following
> requirements:
>  1. Enumerates bbdev hardware devices and load corresponding drivers.
>  2. Abstracts the same functionality through the optimized software libraries in
>     case HW device is not existent.
>  3. Seamless interface for underlying operations (software or hardware)
>  4. Pluggable drivers for various parts of the packet processing
>  5. APIs to:
>     - Probe wireless device capabilities and resources
>     - Configure, start, stop, close and retrieve information of wireless devices
>     - Configure, start, stop and retrieve information of operating queues
>     - Enqueue/dequeue operations to wireless device queues
>     - Reset and retrieve device and queue statistics
>     - Support interrupts from HW
> 
> The proposed approach is to have a single wireless device interface (bbdev) that
> is used for CRC, Rate (De)Matching and Turbo coding functionality supporting
> LTE
> physical Layer use cases. A general term for base band is used in the naming to
> allow for a combination of functions to be deployed for flexible and
> programmable devices are used such as FPGAs.
> 
> The wireless Base Band device interface (bbdev) cannot be looked at as a
> subsidiary device class of cryptodev framework, bbdev follows similar design
> approach but it is different in definition and from operation perspective.
> The bbdev does not require the session management approach that cryptodev
> needs
> to function. Furthermore, bbdev is an abstraction framework that abstracts
> various device functions for numerous operations in the LTE physical layer like
> Turbo coding and rate matching, which cannot be considered operations of
> cryptography. Also, a bbdev device can support multiple wireless functions under
> one device ID.
> 
> Other design approaches where considered during design selection phase like a
> wireless device interface is to have individual interfaces for each operation
> type within the LTE physical layer. This somewhat follows the cryptodev model,
> where the device interface can be used to perform a single class of operation
> type. However, it is difficult to map a device which performs multiple
> operations into this model. Consider the hardware accelerator that performs
> Rate (De)Matching, Turbo encoding/decoding (Turbo is a Forward Error
> Correction
> algorithm) and CRC handling. The device would have to register with three
> interfaces, and it would need three look-aside operations to do the processing
> (impacting performance).
> It is not correct to use it with a FEC (Forward Error Correction) device
> interface, as the device does more than that. Also, there is a wide range of
> FEC algorithms, many of which have no parameters or use-cases in common
> with
> Turbo (for example Reed-Solomon used in storage), so the usefulness of such an
> interface is questionable.
> 
> The initial release of the bbdev includes CRC attachment, Turbo Coding and
> Rate (De)Matching supported in software.
> 
> A device reports its capabilities when registering itself in the bbdev framework.
> With the aid of this capabilities mechanism, an application can query devices to
> discover which operations within the LTE physical layer they are capable of
> performing.
> 
> Turbo code software library can be added as a virtual device to simulate the
> functionality being performed by the HW in case it is not existent or in case
> the application needs to use the two flavors to suit different types of
> workloads, for example: short payloads to use software Turbo and the HW for
> larger payloads. This software library is not part of DPDK mainstream, but it
> can be downloaded from an external link and linked to DPDK at compile time.
> 
> For simplicity, the initial software devices are designed to perform the
> operation in the thread context that calls the enqueue function. The result of
> the operation will be put onto an internal rte_ring based queue, waiting for
> dequeue to be called. As device queues are not thread safe, the single-producer,
> single-consumer version of rte_ring queue can be used.
> 
> A device ID represents a handle that is used by the application to refer to a
> specific instance of a bbdev device. The range of device IDs for bbdevs is not
> linked or related in any way with device IDs used for cryptodevs or ethdevs.
> 
> The application can query how many bbdevs were discovered by the EAL through
> rte_bbdev_count() and then knows the range of valid device IDs that can be used
> for further device interaction.
> 
> Once a device is present in the applications context, the application can
> discover some information about the exact device type and capabilities by
> calling rte_bbdev_info_get(dev_id, &info). Capabilities (in terms of operations
> supported, max number of queues, etc.) may be different for each device type
> so
> this is an important step for an application that is not highly coupled to a
> specific device type.
> 
> From the application point of view, each instance of a bbdev device consists of
> one or more queues identified by queue IDs. While different devices may have
> different capabilities (e.g. support different operation types), all queues on
> a device support identical configuration possibilities. A queue is configured
> for only one type of operation and is configured at initializations time.
> When an operation is enqueued to a specific queue ID, the result is dequeued
> from the same queue ID.
> 
> Configuration of a device has two different levels: configuration that applies
> to the whole device, and configuration that applies to a single queue.
> Device configuration is applied with
> rte_bbdev_configure(dev_id,num_queues,conf)
> and queue configuration is applied with
> rte_bbdev_queue_configure(dev_id, queue_id,conf). Note that, although all
> queues
> on a device support same capabilities, they can be configured differently and
> will then behave differently.
> 
> After initialization, devices are in a stopped state, so must be started by the
> application. By default, all queues are started when the device is started, but
> they can be stopped individually. If an application is finished using a device
> it can close the device. Once closed, it cannot be restarted without
> reconfiguration.
> 
> rte_bbdev_start(dev_id)
> rte_bbdev_queue_start(dev_id, queue_id)
> rte_bbdev_queue_stop(dev_id, queue_id)
> rte_bbdev_stop(dev_id)
> rte_bbdev_close(dev_id)
> 
> Operations on a buffer (or buffers) are performed by an asynchronous API.
> Operations are enqueued to the device, and then dequeued later.
> Ordering of operations is maintained between the enqueue and dequeue.
> 
> rte_bbdev_enqueue_ops(dev_id, queue_id, **ops, num_ops)
> rte_bbdev_dequeue_ops(dev_id, queue_id, **ops, num_ops)
> 
> Queues are not thread-safe and the use of multiple queues or application-level
> locking is required for multi-threaded applications that share a device.
> Note that it is however acceptable for one thread to enqueue to a queue ID and
> another thread to dequeue from the same queue ID.  This could be used to
> implement a multithreaded pipeline where each thread dequeues from a bbdev
> device, before enqueueing to the next.
> The number of queues supported by the device can be queried through
> rte_bbdev_info_get().
> 
> **ops is an array of pointers to struct rte_bbdev_op structures which contain
> all the details needed by the device to perform a single operation.
> As the bbdev interface supports different operation types (although individual
> devices may only support a subset of these), it contains a type field, and a
> union of parameters for each operation type.
> 
> struct rte_bbdev_op {
> 	enum rte_bbdev_op_type type;
> 	…
> 	union {
> 		void *generic;
> 		struct rte_bbdev_op_turbo_dec *turbo_dec;
> 		struct rte_bbdev_op_turbo_enc *turbo_enc;
> 	};
> };
> 
> 
> Find the enclosed patch for the complete API specification, application- and
> driver-facing APIs.
> 
> Looking forward to getting comments from both the application and driver
> 
> 
> Amr Mokhtar (1):
>   Wireless Base Band Device (bbdev)
> 
>  lib/librte_bbdev/rte_bbdev.h     | 636
> +++++++++++++++++++++++++++++++++++++++
>  lib/librte_bbdev/rte_bbdev_op.h  | 333 ++++++++++++++++++++
>  lib/librte_bbdev/rte_bbdev_pmd.h | 407 +++++++++++++++++++++++++
>  3 files changed, 1376 insertions(+)
>  create mode 100644 lib/librte_bbdev/rte_bbdev.h
>  create mode 100644 lib/librte_bbdev/rte_bbdev_op.h
>  create mode 100644 lib/librte_bbdev/rte_bbdev_pmd.h
> 
> --
> 1.9.1

Re: [dpdk-dev] [RFC] Wireless Base Band Device (bbdev)

[Thomas Monjalon]

25/08/2017 15:46, Amr Mokhtar:
> This RFC describes a proposal for the Wireless Base Band Device (bbdev) in DPDK 
> that abstracts HW accelerators based on FPGA and/or Fixed Function Accelerators 
> that assist with LTE Physical Layer processing. Furthermore, it decouples the 
> application from the compute-intensive wireless functions by abstracting their 
> optimized libraries to appear as virtual bbdev devices. 
> This makes bbdev a common programming framework that enables the same 
> application code to be run on different systems with a single software 
> architecture and programming model. If the system has hardware accelerators, 
> they will be used, but if the system does not have hardware accelerators, 
> software implementations can be used.

Looks interesting.
When do you plan to send the first version?

The description done in this RFC is very complete and clear. Thanks
I will ask few questions below.

I assume packets are mbuf and Rx/Tx is done by ethdev. Right?

Did you think about the API required for inline processing
(i.e. bbdev combined with ethdev Rx/Tx)?

[...]
> Other design approaches where considered during design selection phase like a 
> wireless device interface is to have individual interfaces for each operation 
> type within the LTE physical layer. This somewhat follows the cryptodev model, 
> where the device interface can be used to perform a single class of operation 
> type. However, it is difficult to map a device which performs multiple 
> operations into this model. Consider the hardware accelerator that performs 
> Rate (De)Matching, Turbo encoding/decoding (Turbo is a Forward Error Correction 
> algorithm) and CRC handling. The device would have to register with three 
> interfaces, and it would need three look-aside operations to do the processing 
> (impacting performance).
> It is not correct to use it with a FEC (Forward Error Correction) device 
> interface, as the device does more than that. Also, there is a wide range of 
> FEC algorithms, many of which have no parameters or use-cases in common with 
> Turbo (for example Reed-Solomon used in storage), so the usefulness of such an 
> interface is questionable.

So you decided to dedicate an API to the wireless base band functions,
which is a functional split.
I think the cryptodev API is a different split: it is targetting all
processing which falls into the crypto category.
Some crypto algorithms are associated to the wireless function,
while others are more generic.

It is very difficult to know how to split the scope of an API.
With the proposed scheme, if a wireless LTE device implements ZUC algorithm,
we will have to support it in a cryptodev PMD while having a bbdev PMD
for other wireless functions.
Thoughts?

> The initial release of the bbdev includes CRC attachment, Turbo Coding and 
> Rate (De)Matching supported in software.
> 
> A device reports its capabilities when registering itself in the bbdev framework. 
> With the aid of this capabilities mechanism, an application can query devices to 
> discover which operations within the LTE physical layer they are capable of 
> performing.
> 
> Turbo code software library can be added as a virtual device to simulate the 
> functionality being performed by the HW in case it is not existent or in case 
> the application needs to use the two flavors to suit different types of 
> workloads, for example: short payloads to use software Turbo and the HW for 
> larger payloads. This software library is not part of DPDK mainstream, but it 
> can be downloaded from an external link and linked to DPDK at compile time.

Do you mean that you do not plan to integrate the SW fallback for
the turbo coding feature?
Even if it is packaged separately, it could be integrated with bbdev API.

[...]
> The application can query how many bbdevs were discovered by the EAL through 
> rte_bbdev_count() and then knows the range of valid device IDs that can be used 
> for further device interaction.

You must have an iterator macro to be able to manage id range with holes.
I see it is already implemented as RTE_BBDEV_FOREACH.

[...]
> rte_bbdev_enqueue_ops(dev_id, queue_id, **ops, num_ops)
> rte_bbdev_dequeue_ops(dev_id, queue_id, **ops, num_ops)
[...]
> 
> **ops is an array of pointers to struct rte_bbdev_op structures which contain 
> all the details needed by the device to perform a single operation. 
> As the bbdev interface supports different operation types (although individual 
> devices may only support a subset of these), it contains a type field, and a 
> union of parameters for each operation type.
> 
> struct rte_bbdev_op {
> 	enum rte_bbdev_op_type type;
> 	union {
> 		void *generic;
> 		struct rte_bbdev_op_turbo_dec *turbo_dec;
> 		struct rte_bbdev_op_turbo_enc *turbo_enc;
> 	};
> };

I do not understand this part.
It seems you want only two generic function to perform processing.
I do not see the benefit.
It is usually easier to have one function per type of processing.

I will continue the review with the code itself.

Re: [dpdk-dev] [RFC] Wireless Base Band Device (bbdev)

[Mokhtar, Amr]

Hi Thomas,
Kindly find my inline replies below..


> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday 21 September 2017 15:35
> To: Mokhtar, Amr <amr.mokhtar@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [RFC] Wireless Base Band Device (bbdev)
> 
> 25/08/2017 15:46, Amr Mokhtar:
> > This RFC describes a proposal for the Wireless Base Band Device
> > (bbdev) in DPDK that abstracts HW accelerators based on FPGA and/or
> > Fixed Function Accelerators that assist with LTE Physical Layer
> > processing. Furthermore, it decouples the application from the
> > compute-intensive wireless functions by abstracting their optimized libraries to
> appear as virtual bbdev devices.
> > This makes bbdev a common programming framework that enables the same
> > application code to be run on different systems with a single software
> > architecture and programming model. If the system has hardware
> > accelerators, they will be used, but if the system does not have
> > hardware accelerators, software implementations can be used.
> 
> Looks interesting.
> When do you plan to send the first version?

Initial release has been pushed out. Feel free to have a look..
http://dpdk.org/ml/archives/dev/2017-September/077027.html
http://dpdk.org/ml/archives/dev/2017-September/077028.html
http://dpdk.org/ml/archives/dev/2017-September/077029.html
http://dpdk.org/ml/archives/dev/2017-September/077030.html
http://dpdk.org/ml/archives/dev/2017-September/077031.html
http://dpdk.org/ml/archives/dev/2017-September/077033.html
http://dpdk.org/ml/archives/dev/2017-September/077032.html

> 
> The description done in this RFC is very complete and clear. Thanks I will ask few
> questions below.
> 
> I assume packets are mbuf and Rx/Tx is done by ethdev. Right?

Right.

> 
> Did you think about the API required for inline processing (i.e. bbdev combined
> with ethdev Rx/Tx)?

The current programming model is that ethdev is being used for input/output and
the bbdev is offloaded for lookaside acceleration.
The inline processing topic sounds interesting, this is something we can look at in
the future. Appreciate if you can share your thoughts in that regard.

> 
> [...]
> > Other design approaches where considered during design selection phase
> > like a wireless device interface is to have individual interfaces for
> > each operation type within the LTE physical layer. This somewhat
> > follows the cryptodev model, where the device interface can be used to
> > perform a single class of operation type. However, it is difficult to
> > map a device which performs multiple operations into this model.
> > Consider the hardware accelerator that performs Rate (De)Matching,
> > Turbo encoding/decoding (Turbo is a Forward Error Correction
> > algorithm) and CRC handling. The device would have to register with
> > three interfaces, and it would need three look-aside operations to do
> > the processing (impacting performance).
> > It is not correct to use it with a FEC (Forward Error Correction)
> > device interface, as the device does more than that. Also, there is a
> > wide range of FEC algorithms, many of which have no parameters or
> > use-cases in common with Turbo (for example Reed-Solomon used in
> > storage), so the usefulness of such an interface is questionable.
> 
> So you decided to dedicate an API to the wireless base band functions, which is a
> functional split.
> I think the cryptodev API is a different split: it is targetting all processing which
> falls into the crypto category.
> Some crypto algorithms are associated to the wireless function, while others are
> more generic.
> 
> It is very difficult to know how to split the scope of an API.

The way we view this functional split is:
 - If it is a cryptographic function -> cryptodev
 - If it is a Wireless L1 function, or in other words, more related to signal processing
   (coding, scrambling, modulation, ...) -> bbdev

> With the proposed scheme, if a wireless LTE device implements ZUC algorithm,
> we will have to support it in a cryptodev PMD while having a bbdev PMD for
> other wireless functions.
> Thoughts?

ZUC stays a cryptographic algorithm, it should go to cryptodev. It's true that ZUC
is a crypto algorithm used in the mobile wireless domain, but it is not related to signal
processing (Layer 1).
bbdev is targeting all or some of those wireless L1 functions.

> 
> > The initial release of the bbdev includes CRC attachment, Turbo Coding
> > and Rate (De)Matching supported in software.
> >
> > A device reports its capabilities when registering itself in the bbdev framework.
> > With the aid of this capabilities mechanism, an application can query
> > devices to discover which operations within the LTE physical layer
> > they are capable of performing.
> >
> > Turbo code software library can be added as a virtual device to
> > simulate the functionality being performed by the HW in case it is not
> > existent or in case the application needs to use the two flavors to
> > suit different types of workloads, for example: short payloads to use
> > software Turbo and the HW for larger payloads. This software library
> > is not part of DPDK mainstream, but it can be downloaded from an external
> link and linked to DPDK at compile time.
> 
> Do you mean that you do not plan to integrate the SW fallback for the turbo
> coding feature?

We DO actually plan to support SW-fallback of Turbo FEC in bbdev.

> Even if it is packaged separately, it could be integrated with bbdev API.

Right. It is packaged separately and link-able to bbdev library.

> 
> [...]
> > The application can query how many bbdevs were discovered by the EAL
> > through
> > rte_bbdev_count() and then knows the range of valid device IDs that
> > can be used for further device interaction.
> 
> You must have an iterator macro to be able to manage id range with holes.
> I see it is already implemented as RTE_BBDEV_FOREACH.

Right. bbdev has the iterator macro.
But, sequential (gapless) device IDs only are currently supported in the first release.

> 
> [...]
> > rte_bbdev_enqueue_ops(dev_id, queue_id, **ops, num_ops)
> > rte_bbdev_dequeue_ops(dev_id, queue_id, **ops, num_ops)
> [...]
> >
> > **ops is an array of pointers to struct rte_bbdev_op structures which
> > contain all the details needed by the device to perform a single operation.
> > As the bbdev interface supports different operation types (although
> > individual devices may only support a subset of these), it contains a
> > type field, and a union of parameters for each operation type.
> >
> > struct rte_bbdev_op {
> > 	enum rte_bbdev_op_type type;
> > 	union {
> > 		void *generic;
> > 		struct rte_bbdev_op_turbo_dec *turbo_dec;
> > 		struct rte_bbdev_op_turbo_enc *turbo_enc;
> > 	};
> > };
> 
> I do not understand this part.
> It seems you want only two generic function to perform processing.
> I do not see the benefit.
> It is usually easier to have one function per type of processing.
> 

Bbdev devices support Turbo encode and Turbo decode operations.
Both have separate sets of parameters and different functionalities, but
each queue is capable of doing either encode or decode (keep in mind
that this freedom of choice is only applicable before the q gets configured).

There is only one enqueue function for both enc/dec, and similarly one
dequeue function for both. The pmd internally interprets its argument
array of type "rte_bbdev_op" differently based on whether this q was set
up for enc or dec.
See this pseudo-code to give more projection of the idea:

enqueue(struct rte_bbdev_queue_data *q_data, struct rte_bbdev_op *op) {
    void *queue = q_data->queue_private;
    struct pmd_prv_queue *q = queue;

    switch (q->type) {
    case RTE_BBDEV_OP_TURBO_ENC:
        struct rte_bbdev_op_turbo_enc *enc = op->turbo_enc;
        /* do encode */
        encode(enc);
        break;
    case RTE_BBDEV_OP_TURBO_DEC:
        struct rte_bbdev_op_turbo_dec *dec = op->turbo_edec;
        /* do decode */
        decode(dec);
        break;
    }

Since an enqueue was received on a decode queue, then the union is interpreted
as (rte_bbdev_op_turbo_dec), and vice versa for the encode (rte_bbdev_op_turbo_enc).

> I will continue the review with the code itself.

Thanks!

Re: [dpdk-dev] [RFC] Wireless Base Band Device (bbdev)

[Thomas Monjalon]

05/10/2017 22:06, Mokhtar, Amr:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 25/08/2017 15:46, Amr Mokhtar:
> > Did you think about the API required for inline processing (i.e. bbdev combined
> > with ethdev Rx/Tx)?
> 
> The current programming model is that ethdev is being used for input/output and
> the bbdev is offloaded for lookaside acceleration.
> The inline processing topic sounds interesting, this is something we can look at in
> the future. Appreciate if you can share your thoughts in that regard.

When inlining processing in ethdev, it is possible to use rte_flow to
configure processing to be applied on some flows.
However it requires to define the specific action (i.e. processing).
The problem is that you are going to duplicate the API for bbdev processing
if the original API does not fit into a rte_flow action.
The rte_flow actions are still a new idea.
I think it would be nice to have a common structure for
enqueue/dequeue functions and rte_flow configuration.
With this idea, configuring an inline processing would be just
calling an ethdev rte_flow function instead of a bbdev enqueue/dequeue.

Opinions?

[...]
> The way we view this functional split is:
>  - If it is a cryptographic function -> cryptodev
>  - If it is a Wireless L1 function, or in other words, more related to signal processing
>    (coding, scrambling, modulation, ...) -> bbdev
> 
> > With the proposed scheme, if a wireless LTE device implements ZUC algorithm,
> > we will have to support it in a cryptodev PMD while having a bbdev PMD for
> > other wireless functions.
> > Thoughts?
> 
> ZUC stays a cryptographic algorithm, it should go to cryptodev. It's true that ZUC
> is a crypto algorithm used in the mobile wireless domain, but it is not related to signal
> processing (Layer 1).
> bbdev is targeting all or some of those wireless L1 functions.

OK, this definition of the bbdev scope is very interesting.
Please could you explain it in the first lines of bbdev doxygen?

[...]
> > > rte_bbdev_enqueue_ops(dev_id, queue_id, **ops, num_ops)
> > > rte_bbdev_dequeue_ops(dev_id, queue_id, **ops, num_ops)
> > [...]
> > >
> > > **ops is an array of pointers to struct rte_bbdev_op structures which
> > > contain all the details needed by the device to perform a single operation.
> > > As the bbdev interface supports different operation types (although
> > > individual devices may only support a subset of these), it contains a
> > > type field, and a union of parameters for each operation type.
> > >
> > > struct rte_bbdev_op {
> > > 	enum rte_bbdev_op_type type;
> > > 	union {
> > > 		void *generic;
> > > 		struct rte_bbdev_op_turbo_dec *turbo_dec;
> > > 		struct rte_bbdev_op_turbo_enc *turbo_enc;
> > > 	};
> > > };
> > 
> > I do not understand this part.
> > It seems you want only two generic function to perform processing.
> > I do not see the benefit.
> > It is usually easier to have one function per type of processing.
> > 
> 
> Bbdev devices support Turbo encode and Turbo decode operations.
> Both have separate sets of parameters and different functionalities, but
> each queue is capable of doing either encode or decode (keep in mind
> that this freedom of choice is only applicable before the q gets configured).
> 
> There is only one enqueue function for both enc/dec, and similarly one
> dequeue function for both. The pmd internally interprets its argument
> array of type "rte_bbdev_op" differently based on whether this q was set
> up for enc or dec.
> See this pseudo-code to give more projection of the idea:
> 
> enqueue(struct rte_bbdev_queue_data *q_data, struct rte_bbdev_op *op) {
>     void *queue = q_data->queue_private;
>     struct pmd_prv_queue *q = queue;
> 
>     switch (q->type) {
>     case RTE_BBDEV_OP_TURBO_ENC:
>         struct rte_bbdev_op_turbo_enc *enc = op->turbo_enc;
>         /* do encode */
>         encode(enc);
>         break;
>     case RTE_BBDEV_OP_TURBO_DEC:
>         struct rte_bbdev_op_turbo_dec *dec = op->turbo_edec;
>         /* do decode */
>         decode(dec);
>         break;
>     }
> 
> Since an enqueue was received on a decode queue, then the union is interpreted
> as (rte_bbdev_op_turbo_dec), and vice versa for the encode (rte_bbdev_op_turbo_enc).

I still do not see the benefit.
Why not 4 functions?
	enqueue_enc
	enqueue_dec
	dequeue_enc
	dequeue_dec
Sorry if the question is very basic.

Re: [dpdk-dev] [RFC] Wireless Base Band Device (bbdev)

[Stephen Hemminger]

> +/* Forward declaration */
> +struct rte_pci_device;
> +
> +/** Device information structure used by an application to discover a devices
> + * capabilities and current configuration
> + */
> +struct rte_bbdev_info {
> +	int socket_id;  /**< NUMA socket that device is on */
> +	const char *dev_name;  /**< Unique device name */
> +	const struct rte_pci_device *pci_dev;  /**< PCI information */
> +	unsigned int num_queues;  /**< Number of queues currently configured */
> +	struct rte_bbdev_conf conf;  /**< Current device configuration */
> +	bool started;  /**< Set if device is currently started */
> +	struct rte_bbdev_driver_info drv;  /**< Info from device driver */
> +};

Please don't build in dependency on PCI from the beginning.
Number of queues can be uint16_t ?

Re: [dpdk-dev] [RFC] Wireless Base Band Device (bbdev)

[Mokhtar, Amr]

Thanks Stephen.
Agree. Will remove dependency on PCI for now. And num_queues should have been declared as uint16_t.

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Friday 1 September 2017 21:04
> To: Mokhtar, Amr <amr.mokhtar@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [RFC] Wireless Base Band Device (bbdev)
> 
> 
> > +/* Forward declaration */
> > +struct rte_pci_device;
> > +
> > +/** Device information structure used by an application to discover a
> > +devices
> > + * capabilities and current configuration  */ struct rte_bbdev_info {
> > +	int socket_id;  /**< NUMA socket that device is on */
> > +	const char *dev_name;  /**< Unique device name */
> > +	const struct rte_pci_device *pci_dev;  /**< PCI information */
> > +	unsigned int num_queues;  /**< Number of queues currently configured
> */
> > +	struct rte_bbdev_conf conf;  /**< Current device configuration */
> > +	bool started;  /**< Set if device is currently started */
> > +	struct rte_bbdev_driver_info drv;  /**< Info from device driver */
> > +};
> 
> Please don't build in dependency on PCI from the beginning.
> Number of queues can be uint16_t ?

Re: [dpdk-dev] [RFC] Wireless Base Band Device (bbdev)

[Thomas Monjalon]

25/08/2017 15:46, Amr Mokhtar:
> +/**
> + * Configure a device.
> + * This function must be called on a device before setting up the queues and
> + * starting the device. It can also be called when a device is in the stopped
> + * state. If any device queues have been configured their configuration will be
> + * cleared by a call to this function.
> + *
> + * @param dev_id
> + *   The identifier of the device.
> + * @param num_queues
> + *   Number of queues to configure on device.
> + * @param conf
> + *   The device configuration. If NULL, a default configuration will be used.
> + *
> + * @return
> + *   - 0 on success
> + *   - EINVAL if num_queues is invalid, 0 or greater than maximum
> + *   - EBUSY if the identified device has already started
> + *   - ENOMEM if unable to allocate memory
> + */
> +int
> +rte_bbdev_configure(uint8_t dev_id, uint16_t num_queues,
> +		const struct rte_bbdev_conf *conf);

I am not convinced by the "configure all" function in ethdev.
We break the ABI each time we add a new feature to configure.
And it does not really help to have all configurations in one struct.
Would you mind to split the struct rte_bbdev_conf and split
the function accordingly?

[...]
> +struct rte_bbdev_info {
> +	int socket_id;  /**< NUMA socket that device is on */
> +	const char *dev_name;  /**< Unique device name */
> +	const struct rte_pci_device *pci_dev;  /**< PCI information */
> +	unsigned int num_queues;  /**< Number of queues currently configured */
> +	struct rte_bbdev_conf conf;  /**< Current device configuration */
> +	bool started;  /**< Set if device is currently started */
> +	struct rte_bbdev_driver_info drv;  /**< Info from device driver */
> +};

As Stephen said, PCI must not appear in this API.
Please use the bus abstraction.

[...]
> +struct __rte_cache_aligned rte_bbdev {
> +	rte_bbdev_enqueue_ops_t enqueue_ops; /**< Enqueue function */
> +	rte_bbdev_dequeue_ops_t dequeue_ops;  /**< Dequeue function */
> +	const struct rte_bbdev_ops *dev_ops;  /**< Functions exported by PMD */
> +	struct rte_bbdev_data *data;  /**< Pointer to device data */
> +	bool attached;  /**< If device is currently attached or not */

What "attached" means?
I'm afraid you are trying to manage hotplug in the wrong layer.

> +	struct rte_device *device; /**< Backing device (HW only) */

SW port should have also a rte_device (vdev).

[...]
> +/** Data input and output buffer for Turbo operations */
> +struct rte_bbdev_op_data {

Why there is no "turbo" word in the name of this struct?

> +	struct rte_mbuf *data;
> +	/**< First mbuf segment with input/output data. */
> +	uint32_t offset;
> +	/**< The starting point for the Turbo input/output, in bytes, from the
> +	 * start of the data in the data buffer. It must be smaller than
> +	 * data_len of the mbuf's first segment!
> +	 */
> +	uint32_t length;
> +	/**< For input operations: the length, in bytes, of the source buffer
> +	 * on which the Turbo encode/decode will be computed.
> +	 * For output operations: the length, in bytes, of the output buffer
> +	 * of the Turbo operation.
> +	 */
> +};

[...]
> +/** Structure specifying a single operation */
> +struct rte_bbdev_op {
> +	enum rte_bbdev_op_type type;  /**< Type of this operation */
> +	int status;  /**< Status of operation that was performed */
> +	struct rte_mempool *mempool;  /**< Mempool which op instance is in */
> +	void *opaque_data;  /**< Opaque pointer for user data */
> +	/**
> +	 * Anonymous union of operation-type specific parameters. When allocated
> +	 * using rte_bbdev_op_pool_create(), space is allocated for the
> +	 * parameters at the end of each rte_bbdev_op structure, and the
> +	 * pointers here point to it.
> +	 */
> +	RTE_STD_C11
> +	union {
> +		void *generic;
> +		struct rte_bbdev_op_turbo_dec *turbo_dec;
> +		struct rte_bbdev_op_turbo_enc *turbo_enc;
> +	};
> +};

I am not sure it is a good idea to fit every operations in the
same struct and the same functions.

[...]
> +/**
> + * Helper macro for logging
> + *
> + * @param level
> + *   Log level: EMERG, ALERT, CRIT, ERR, WARNING, NOTICE, INFO, or DEBUG
> + * @param fmt
> + *   The format string, as in printf(3).
> + * @param ...
> + *   The variable arguments required by the format string.
> + *
> + * @return
> + *   - 0 on success
> + *   - Negative on error
> + */
> +#define rte_bbdev_log(level, fmt, ...) \
> +		RTE_LOG(level, BBDEV, fmt "\n", ##__VA_ARGS__)

This is the legacy log system.
Please use dynamic log type.

[...]
> +#ifdef RTE_LIBRTE_BBDEV_DEBUG
> +#define rte_bbdev_log_verbose(fmt, ...)  rte_bbdev_log_debug(fmt, ##__VA_ARGS__)
> +#else
> +#define rte_bbdev_log_verbose(fmt, ...)
> +#endif

With the new log functions, you do not need to disable debug log
at compilation time.

> +/**
> + *  Initialisation params structure that can be used by software based drivers
> + */
> +struct rte_bbdev_init_params {
> +	int socket_id;  /**< Base band null device socket */
> +	uint16_t queues_num;  /**< Base band null device queues number */
> +};
> +
> +/**
> + * Parse generic parameters that could be used for software based devices.
> + *
> + * @param params
> + *   Pointer to structure that will hold the parsed parameters.
> + * @param input_args
> + *   Pointer to arguments to be parsed.
> + *
> + * @return
> + *   - 0 on success
> + *   - EINVAL if invalid parameter pointer is provided
> + *   - EFAULT if unable to parse provided arguments
> + */
> +int
> +rte_bbdev_parse_params(struct rte_bbdev_init_params *params,
> +		const char *input_args);

I do not understand the intent of these parameters.
Are they common to every PMDs?
Or could they be moved in software PMDs?

End of this first review pass :)

Re: [dpdk-dev] [RFC] Wireless Base Band Device (bbdev)

[Mokhtar, Amr]

Hi Thomas,
Thanks for reviewing.. Kindly find my reply in-line below..

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday 21 September 2017 15:56
> To: Mokhtar, Amr <amr.mokhtar@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [RFC] Wireless Base Band Device (bbdev)
> 
> 25/08/2017 15:46, Amr Mokhtar:
> > +/**
> > + * Configure a device.
> > + * This function must be called on a device before setting up the
> > +queues and
> > + * starting the device. It can also be called when a device is in the
> > +stopped
> > + * state. If any device queues have been configured their
> > +configuration will be
> > + * cleared by a call to this function.
> > + *
> > + * @param dev_id
> > + *   The identifier of the device.
> > + * @param num_queues
> > + *   Number of queues to configure on device.
> > + * @param conf
> > + *   The device configuration. If NULL, a default configuration will be used.
> > + *
> > + * @return
> > + *   - 0 on success
> > + *   - EINVAL if num_queues is invalid, 0 or greater than maximum
> > + *   - EBUSY if the identified device has already started
> > + *   - ENOMEM if unable to allocate memory
> > + */
> > +int
> > +rte_bbdev_configure(uint8_t dev_id, uint16_t num_queues,
> > +		const struct rte_bbdev_conf *conf);
> 
> I am not convinced by the "configure all" function in ethdev.
> We break the ABI each time we add a new feature to configure.
> And it does not really help to have all configurations in one struct.
> Would you mind to split the struct rte_bbdev_conf and split the function
> accordingly?

There is nothing to split tbh. The only parameter it has is the socket_id.
And in fact, it's optional, can be null. The only config we need is num_queues.
I don't see in the near future that we may need to add more config params.
As a side, in the time of the implementation we were trying to avoid any
diversions from the current design ideology of ethdev and cryptodev.
Can we leave it for consideration with future releases?

> 
> [...]
> > +struct rte_bbdev_info {
> > +	int socket_id;  /**< NUMA socket that device is on */
> > +	const char *dev_name;  /**< Unique device name */
> > +	const struct rte_pci_device *pci_dev;  /**< PCI information */
> > +	unsigned int num_queues;  /**< Number of queues currently configured
> */
> > +	struct rte_bbdev_conf conf;  /**< Current device configuration */
> > +	bool started;  /**< Set if device is currently started */
> > +	struct rte_bbdev_driver_info drv;  /**< Info from device driver */
> > +};
> 
> As Stephen said, PCI must not appear in this API.
> Please use the bus abstraction.

Done.

> 
> [...]
> > +struct __rte_cache_aligned rte_bbdev {
> > +	rte_bbdev_enqueue_ops_t enqueue_ops; /**< Enqueue function */
> > +	rte_bbdev_dequeue_ops_t dequeue_ops;  /**< Dequeue function */
> > +	const struct rte_bbdev_ops *dev_ops;  /**< Functions exported by PMD
> */
> > +	struct rte_bbdev_data *data;  /**< Pointer to device data */
> > +	bool attached;  /**< If device is currently attached or not */
> 
> What "attached" means?
> I'm afraid you are trying to manage hotplug in the wrong layer.

Hotplug is not supported in the current release.

> 
> > +	struct rte_device *device; /**< Backing device (HW only) */
> 
> SW port should have also a rte_device (vdev).

Right. Fixed the comment.

> 
> [...]
> > +/** Data input and output buffer for Turbo operations */ struct
> > +rte_bbdev_op_data {
> 
> Why there is no "turbo" word in the name of this struct?

To keep it a generic input/output data descriptor,
that suits any type of baseband operation not only for turbo.

> 
> > +	struct rte_mbuf *data;
> > +	/**< First mbuf segment with input/output data. */
> > +	uint32_t offset;
> > +	/**< The starting point for the Turbo input/output, in bytes, from the
> > +	 * start of the data in the data buffer. It must be smaller than
> > +	 * data_len of the mbuf's first segment!
> > +	 */
> > +	uint32_t length;
> > +	/**< For input operations: the length, in bytes, of the source buffer
> > +	 * on which the Turbo encode/decode will be computed.
> > +	 * For output operations: the length, in bytes, of the output buffer
> > +	 * of the Turbo operation.
> > +	 */
> > +};
> 
> [...]
> > +/** Structure specifying a single operation */ struct rte_bbdev_op {
> > +	enum rte_bbdev_op_type type;  /**< Type of this operation */
> > +	int status;  /**< Status of operation that was performed */
> > +	struct rte_mempool *mempool;  /**< Mempool which op instance is in
> */
> > +	void *opaque_data;  /**< Opaque pointer for user data */
> > +	/**
> > +	 * Anonymous union of operation-type specific parameters. When
> allocated
> > +	 * using rte_bbdev_op_pool_create(), space is allocated for the
> > +	 * parameters at the end of each rte_bbdev_op structure, and the
> > +	 * pointers here point to it.
> > +	 */
> > +	RTE_STD_C11
> > +	union {
> > +		void *generic;
> > +		struct rte_bbdev_op_turbo_dec *turbo_dec;
> > +		struct rte_bbdev_op_turbo_enc *turbo_enc;
> > +	};
> > +};
> 
> I am not sure it is a good idea to fit every operations in the same struct and the
> same functions.

Due to the fact that our design adopts this idea that a device can support both
the encode and decode operations.
Then, at the time of PMD registration, the enqueue functions is allocated.
This enqueue() function is common for both operations.
This fitted operation structure is essential for the driver to decide on the operation.

> 
> [...]
> > +/**
> > + * Helper macro for logging
> > + *
> > + * @param level
> > + *   Log level: EMERG, ALERT, CRIT, ERR, WARNING, NOTICE, INFO, or
> DEBUG
> > + * @param fmt
> > + *   The format string, as in printf(3).
> > + * @param ...
> > + *   The variable arguments required by the format string.
> > + *
> > + * @return
> > + *   - 0 on success
> > + *   - Negative on error
> > + */
> > +#define rte_bbdev_log(level, fmt, ...) \
> > +		RTE_LOG(level, BBDEV, fmt "\n", ##__VA_ARGS__)
> 
> This is the legacy log system.
> Please use dynamic log type.

Done.

> 
> [...]
> > +#ifdef RTE_LIBRTE_BBDEV_DEBUG
> > +#define rte_bbdev_log_verbose(fmt, ...)  rte_bbdev_log_debug(fmt,
> > +##__VA_ARGS__) #else #define rte_bbdev_log_verbose(fmt, ...) #endif
> 
> With the new log functions, you do not need to disable debug log at compilation
> time.

Right.

> 
> > +/**
> > + *  Initialisation params structure that can be used by software
> > +based drivers  */ struct rte_bbdev_init_params {
> > +	int socket_id;  /**< Base band null device socket */
> > +	uint16_t queues_num;  /**< Base band null device queues number */ };
> > +
> > +/**
> > + * Parse generic parameters that could be used for software based devices.
> > + *
> > + * @param params
> > + *   Pointer to structure that will hold the parsed parameters.
> > + * @param input_args
> > + *   Pointer to arguments to be parsed.
> > + *
> > + * @return
> > + *   - 0 on success
> > + *   - EINVAL if invalid parameter pointer is provided
> > + *   - EFAULT if unable to parse provided arguments
> > + */
> > +int
> > +rte_bbdev_parse_params(struct rte_bbdev_init_params *params,
> > +		const char *input_args);
> 
> I do not understand the intent of these parameters.
> Are they common to every PMDs?
> Or could they be moved in software PMDs?

That was an old design approach, but this now moved and became the
responsibility of the soft PMD.

> 
> End of this first review pass :)

Thanks!!

Re: [dpdk-dev] [RFC] Wireless Base Band Device (bbdev)

[Thomas Monjalon]

03/10/2017 16:29, Mokhtar, Amr:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 25/08/2017 15:46, Amr Mokhtar:
> > > +int
> > > +rte_bbdev_configure(uint8_t dev_id, uint16_t num_queues,
> > > +		const struct rte_bbdev_conf *conf);
> > 
> > I am not convinced by the "configure all" function in ethdev.
> > We break the ABI each time we add a new feature to configure.
> > And it does not really help to have all configurations in one struct.
> > Would you mind to split the struct rte_bbdev_conf and split the function
> > accordingly?
> 
> There is nothing to split tbh. The only parameter it has is the socket_id.
> And in fact, it's optional, can be null. The only config we need is num_queues.

Indeed, there is nothing in this struct.
If you need only to allocate queues, you just have to rename this function.

> I don't see in the near future that we may need to add more config params.
> As a side, in the time of the implementation we were trying to avoid any
> diversions from the current design ideology of ethdev and cryptodev.

There is no ideology in ethdev, just some mistakes ;)

> Can we leave it for consideration with future releases?

No it should be addressed from the beginning.

When you will need to add something more to configure port-wise,
you should add a new function instead of breaking the ABI
of the global conf struct.
That's why the configure option should be more specialized.

Distro people were complaining about ABI breakage last week.
This is exactly an example of how to avoid it from the beginning.


> > [...]
> > > +struct __rte_cache_aligned rte_bbdev {
> > > +	rte_bbdev_enqueue_ops_t enqueue_ops; /**< Enqueue function */
> > > +	rte_bbdev_dequeue_ops_t dequeue_ops;  /**< Dequeue function */
> > > +	const struct rte_bbdev_ops *dev_ops;  /**< Functions exported by PMD
> > */
> > > +	struct rte_bbdev_data *data;  /**< Pointer to device data */
> > > +	bool attached;  /**< If device is currently attached or not */
> > 
> > What "attached" means?
> > I'm afraid you are trying to manage hotplug in the wrong layer.
> 
> Hotplug is not supported in the current release.

It is not answering the question.
What is an "attached" device?


> > [...]
> > > +/** Structure specifying a single operation */ struct rte_bbdev_op {
> > > +	enum rte_bbdev_op_type type;  /**< Type of this operation */
> > > +	int status;  /**< Status of operation that was performed */
> > > +	struct rte_mempool *mempool;  /**< Mempool which op instance is in
> > */
> > > +	void *opaque_data;  /**< Opaque pointer for user data */
> > > +	/**
> > > +	 * Anonymous union of operation-type specific parameters. When
> > allocated
> > > +	 * using rte_bbdev_op_pool_create(), space is allocated for the
> > > +	 * parameters at the end of each rte_bbdev_op structure, and the
> > > +	 * pointers here point to it.
> > > +	 */
> > > +	RTE_STD_C11
> > > +	union {
> > > +		void *generic;
> > > +		struct rte_bbdev_op_turbo_dec *turbo_dec;
> > > +		struct rte_bbdev_op_turbo_enc *turbo_enc;
> > > +	};
> > > +};
> > 
> > I am not sure it is a good idea to fit every operations
> > in the same struct and the same functions.
> 
> Due to the fact that our design adopts this idea that a device can support both
> the encode and decode operations.
> Then, at the time of PMD registration, the enqueue functions is allocated.
> This enqueue() function is common for both operations.
> This fitted operation structure is essential for the driver to decide on the operation.

Sorry I do not understand why you must have a "generic operation".
Please, could you try again to explain this design to someone
not fully understanding how turbo enc/dec works?

Re: [dpdk-dev] [RFC] Wireless Base Band Device (bbdev)

[Flavio Leitner]

On Tue, 03 Oct 2017 17:17:53 +0200
Thomas Monjalon <thomas@monjalon.net> wrote:

> 03/10/2017 16:29, Mokhtar, Amr:
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]  
> > > 25/08/2017 15:46, Amr Mokhtar:  
> > > > +int
> > > > +rte_bbdev_configure(uint8_t dev_id, uint16_t num_queues,
> > > > +		const struct rte_bbdev_conf *conf);  
> > > 
> > > I am not convinced by the "configure all" function in ethdev.
> > > We break the ABI each time we add a new feature to configure.
> > > And it does not really help to have all configurations in one struct.
> > > Would you mind to split the struct rte_bbdev_conf and split the function
> > > accordingly?  
> > 
> > There is nothing to split tbh. The only parameter it has is the socket_id.
> > And in fact, it's optional, can be null. The only config we need is num_queues.  
> 
> Indeed, there is nothing in this struct.
> If you need only to allocate queues, you just have to rename this function.
> 
> > I don't see in the near future that we may need to add more config params.
> > As a side, in the time of the implementation we were trying to avoid any
> > diversions from the current design ideology of ethdev and cryptodev.  
> 
> There is no ideology in ethdev, just some mistakes ;)
> 
> > Can we leave it for consideration with future releases?  
> 
> No it should be addressed from the beginning.
> 
> When you will need to add something more to configure port-wise,
> you should add a new function instead of breaking the ABI
> of the global conf struct.
> That's why the configure option should be more specialized.
> 
> Distro people were complaining about ABI breakage last week.
> This is exactly an example of how to avoid it from the beginning.

Exactly, and fixing in future is unlikely to happen or could be
more difficult and even if it happens, it will cause another breakage.

-- 
Flavio

Re: [dpdk-dev] [RFC] Wireless Base Band Device (bbdev)

[Mokhtar, Amr]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Tuesday 3 October 2017 16:18
> To: Mokhtar, Amr <amr.mokhtar@intel.com>
> Cc: dev@dpdk.org; fbl@redhat.com; aconole@redhat.com; bluca@debian.org
> Subject: Re: [dpdk-dev] [RFC] Wireless Base Band Device (bbdev)
> 
> 03/10/2017 16:29, Mokhtar, Amr:
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > 25/08/2017 15:46, Amr Mokhtar:
> > > > +int
> > > > +rte_bbdev_configure(uint8_t dev_id, uint16_t num_queues,
> > > > +		const struct rte_bbdev_conf *conf);
> > >
> > > I am not convinced by the "configure all" function in ethdev.
> > > We break the ABI each time we add a new feature to configure.
> > > And it does not really help to have all configurations in one struct.
> > > Would you mind to split the struct rte_bbdev_conf and split the
> > > function accordingly?
> >
> > There is nothing to split tbh. The only parameter it has is the socket_id.
> > And in fact, it's optional, can be null. The only config we need is num_queues.
> 
> Indeed, there is nothing in this struct.
> If you need only to allocate queues, you just have to rename this function.
> 
> > I don't see in the near future that we may need to add more config params.
> > As a side, in the time of the implementation we were trying to avoid
> > any diversions from the current design ideology of ethdev and cryptodev.
> 
> There is no ideology in ethdev, just some mistakes ;)
> 
> > Can we leave it for consideration with future releases?
> 
> No it should be addressed from the beginning.
> 
> When you will need to add something more to configure port-wise, you should
> add a new function instead of breaking the ABI of the global conf struct.
> That's why the configure option should be more specialized.
> 
> Distro people were complaining about ABI breakage last week.
> This is exactly an example of how to avoid it from the beginning.
> 

Ok, got your point. I was looking at it from an API-only standpoint.
How about modifying it into?
int
rte_bbdev_setup_queues(uint16_t dev_id, uint16_t num_queues, int socket_id);

> 
> > > [...]
> > > > +struct __rte_cache_aligned rte_bbdev {
> > > > +	rte_bbdev_enqueue_ops_t enqueue_ops; /**< Enqueue function */
> > > > +	rte_bbdev_dequeue_ops_t dequeue_ops;  /**< Dequeue function */
> > > > +	const struct rte_bbdev_ops *dev_ops;  /**< Functions exported by
> > > > +PMD
> > > */
> > > > +	struct rte_bbdev_data *data;  /**< Pointer to device data */
> > > > +	bool attached;  /**< If device is currently attached or not */
> > >
> > > What "attached" means?
> > > I'm afraid you are trying to manage hotplug in the wrong layer.
> >
> > Hotplug is not supported in the current release.
> 
> It is not answering the question.
> What is an "attached" device?

"Attached" means that the PCI device was probed and the bbdev device slot is allocated.
For software devices, means that a virtual bbdev device (vdev) is allocated for bbdev.
Same way the "attached" approach used in cryptodev.

> 
> 
> > > [...]
> > > > +/** Structure specifying a single operation */ struct rte_bbdev_op {
> > > > +	enum rte_bbdev_op_type type;  /**< Type of this operation */
> > > > +	int status;  /**< Status of operation that was performed */
> > > > +	struct rte_mempool *mempool;  /**< Mempool which op instance is
> > > > +in
> > > */
> > > > +	void *opaque_data;  /**< Opaque pointer for user data */
> > > > +	/**
> > > > +	 * Anonymous union of operation-type specific parameters. When
> > > allocated
> > > > +	 * using rte_bbdev_op_pool_create(), space is allocated for the
> > > > +	 * parameters at the end of each rte_bbdev_op structure, and the
> > > > +	 * pointers here point to it.
> > > > +	 */
> > > > +	RTE_STD_C11
> > > > +	union {
> > > > +		void *generic;
> > > > +		struct rte_bbdev_op_turbo_dec *turbo_dec;
> > > > +		struct rte_bbdev_op_turbo_enc *turbo_enc;
> > > > +	};
> > > > +};
> > >
> > > I am not sure it is a good idea to fit every operations in the same
> > > struct and the same functions.
> >
> > Due to the fact that our design adopts this idea that a device can
> > support both the encode and decode operations.
> > Then, at the time of PMD registration, the enqueue functions is allocated.
> > This enqueue() function is common for both operations.
> > This fitted operation structure is essential for the driver to decide on the
> operation.
> 
> Sorry I do not understand why you must have a "generic operation".
> Please, could you try again to explain this design to someone not fully
> understanding how turbo enc/dec works?

Oh, sorry, I was not paying attention that you're referring to "void *generic"
It is just a place-holder for any other operation types. Can be removed if you like.

Re: [dpdk-dev] [RFC] Wireless Base Band Device (bbdev)

[Thomas Monjalon]

05/10/2017 23:55, Mokhtar, Amr:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 03/10/2017 16:29, Mokhtar, Amr:
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > 25/08/2017 15:46, Amr Mokhtar:
> > > > > +int
> > > > > +rte_bbdev_configure(uint8_t dev_id, uint16_t num_queues,
> > > > > +		const struct rte_bbdev_conf *conf);
> > > >
> > > > I am not convinced by the "configure all" function in ethdev.
> > > > We break the ABI each time we add a new feature to configure.
> > > > And it does not really help to have all configurations in one struct.
> > > > Would you mind to split the struct rte_bbdev_conf and split the
> > > > function accordingly?
> > >
> > > There is nothing to split tbh. The only parameter it has is the socket_id.
> > > And in fact, it's optional, can be null. The only config we need is num_queues.
> > 
> > Indeed, there is nothing in this struct.
> > If you need only to allocate queues, you just have to rename this function.
> > 
> > > I don't see in the near future that we may need to add more config params.
> > > As a side, in the time of the implementation we were trying to avoid
> > > any diversions from the current design ideology of ethdev and cryptodev.
> > 
> > There is no ideology in ethdev, just some mistakes ;)
> > 
> > > Can we leave it for consideration with future releases?
> > 
> > No it should be addressed from the beginning.
> > 
> > When you will need to add something more to configure port-wise, you should
> > add a new function instead of breaking the ABI of the global conf struct.
> > That's why the configure option should be more specialized.
> > 
> > Distro people were complaining about ABI breakage last week.
> > This is exactly an example of how to avoid it from the beginning.
> > 
> 
> Ok, got your point. I was looking at it from an API-only standpoint.
> How about modifying it into?
> int
> rte_bbdev_setup_queues(uint16_t dev_id, uint16_t num_queues, int socket_id);

Yes OK

[...]
> > > > > +struct __rte_cache_aligned rte_bbdev {
> > > > > +	rte_bbdev_enqueue_ops_t enqueue_ops; /**< Enqueue function */
> > > > > +	rte_bbdev_dequeue_ops_t dequeue_ops;  /**< Dequeue function */
> > > > > +	const struct rte_bbdev_ops *dev_ops;  /**< Functions exported by
> > > > > +PMD
> > > > */
> > > > > +	struct rte_bbdev_data *data;  /**< Pointer to device data */
> > > > > +	bool attached;  /**< If device is currently attached or not */
> > > >
> > > > What "attached" means?
> > > > I'm afraid you are trying to manage hotplug in the wrong layer.
> > >
> > > Hotplug is not supported in the current release.
> > 
> > It is not answering the question.
> > What is an "attached" device?
> 
> "Attached" means that the PCI device was probed and the bbdev device slot is allocated.
> For software devices, means that a virtual bbdev device (vdev) is allocated for bbdev.
> Same way the "attached" approach used in cryptodev.

Not sure to understand.
If "attached" means "allocated", when is it false?

[...]
> > > > > +/** Structure specifying a single operation */ struct rte_bbdev_op {
> > > > > +	enum rte_bbdev_op_type type;  /**< Type of this operation */
> > > > > +	int status;  /**< Status of operation that was performed */
> > > > > +	struct rte_mempool *mempool;  /**< Mempool which op instance is
> > > > > +in
> > > > */
> > > > > +	void *opaque_data;  /**< Opaque pointer for user data */
> > > > > +	/**
> > > > > +	 * Anonymous union of operation-type specific parameters. When
> > > > allocated
> > > > > +	 * using rte_bbdev_op_pool_create(), space is allocated for the
> > > > > +	 * parameters at the end of each rte_bbdev_op structure, and the
> > > > > +	 * pointers here point to it.
> > > > > +	 */
> > > > > +	RTE_STD_C11
> > > > > +	union {
> > > > > +		void *generic;
> > > > > +		struct rte_bbdev_op_turbo_dec *turbo_dec;
> > > > > +		struct rte_bbdev_op_turbo_enc *turbo_enc;
> > > > > +	};
> > > > > +};
> > > >
> > > > I am not sure it is a good idea to fit every operations in the same
> > > > struct and the same functions.
> > >
> > > Due to the fact that our design adopts this idea that a device can
> > > support both the encode and decode operations.
> > > Then, at the time of PMD registration, the enqueue functions is allocated.
> > > This enqueue() function is common for both operations.
> > > This fitted operation structure is essential for the driver to decide on the
> > operation.
> > 
> > Sorry I do not understand why you must have a "generic operation".
> > Please, could you try again to explain this design to someone not fully
> > understanding how turbo enc/dec works?
> 
> Oh, sorry, I was not paying attention that you're referring to "void *generic"
> It is just a place-holder for any other operation types. Can be removed if you like.

No I was not referring to void *generic.
It is the same question as in the RFC.
I don't understand the benefit of grouping different things in an union.

Re: [dpdk-dev] [RFC] Wireless Base Band Device (bbdev)

[Mokhtar, Amr]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday 5 October 2017 23:23
> To: Mokhtar, Amr <amr.mokhtar@intel.com>
> Cc: dev@dpdk.org; fbl@redhat.com; aconole@redhat.com; bluca@debian.org
> Subject: Re: [dpdk-dev] [RFC] Wireless Base Band Device (bbdev)
> 
> 05/10/2017 23:55, Mokhtar, Amr:
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > 03/10/2017 16:29, Mokhtar, Amr:
> > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > 25/08/2017 15:46, Amr Mokhtar:
> > > > > > +int
> > > > > > +rte_bbdev_configure(uint8_t dev_id, uint16_t num_queues,
> > > > > > +		const struct rte_bbdev_conf *conf);
> > > > >
> > > > > I am not convinced by the "configure all" function in ethdev.
> > > > > We break the ABI each time we add a new feature to configure.
> > > > > And it does not really help to have all configurations in one struct.
> > > > > Would you mind to split the struct rte_bbdev_conf and split the
> > > > > function accordingly?
> > > >
> > > > There is nothing to split tbh. The only parameter it has is the socket_id.
> > > > And in fact, it's optional, can be null. The only config we need is
> num_queues.
> > >
> > > Indeed, there is nothing in this struct.
> > > If you need only to allocate queues, you just have to rename this function.
> > >
> > > > I don't see in the near future that we may need to add more config params.
> > > > As a side, in the time of the implementation we were trying to
> > > > avoid any diversions from the current design ideology of ethdev and
> cryptodev.
> > >
> > > There is no ideology in ethdev, just some mistakes ;)
> > >
> > > > Can we leave it for consideration with future releases?
> > >
> > > No it should be addressed from the beginning.
> > >
> > > When you will need to add something more to configure port-wise, you
> > > should add a new function instead of breaking the ABI of the global conf
> struct.
> > > That's why the configure option should be more specialized.
> > >
> > > Distro people were complaining about ABI breakage last week.
> > > This is exactly an example of how to avoid it from the beginning.
> > >
> >
> > Ok, got your point. I was looking at it from an API-only standpoint.
> > How about modifying it into?
> > int
> > rte_bbdev_setup_queues(uint16_t dev_id, uint16_t num_queues, int
> > socket_id);
> 
> Yes OK
> 
> [...]
> > > > > > +struct __rte_cache_aligned rte_bbdev {
> > > > > > +	rte_bbdev_enqueue_ops_t enqueue_ops; /**< Enqueue
> function */
> > > > > > +	rte_bbdev_dequeue_ops_t dequeue_ops;  /**< Dequeue
> function */
> > > > > > +	const struct rte_bbdev_ops *dev_ops;  /**< Functions
> > > > > > +exported by PMD
> > > > > */
> > > > > > +	struct rte_bbdev_data *data;  /**< Pointer to device data */
> > > > > > +	bool attached;  /**< If device is currently attached or not
> > > > > > +*/
> > > > >
> > > > > What "attached" means?
> > > > > I'm afraid you are trying to manage hotplug in the wrong layer.
> > > >
> > > > Hotplug is not supported in the current release.
> > >
> > > It is not answering the question.
> > > What is an "attached" device?
> >
> > "Attached" means that the PCI device was probed and the bbdev device slot is
> allocated.
> > For software devices, means that a virtual bbdev device (vdev) is allocated for
> bbdev.
> > Same way the "attached" approach used in cryptodev.
> 
> Not sure to understand.
> If "attached" means "allocated", when is it false?

Currently in bbdev, it is set to true and never goes false.
As I said the Hotplug feature is not fully supported in the current version. I can remove that flag for now.

But generally, it should be cleared to false when rte_pci_driver->remove function is called. (Hotplug?)

> 
> [...]
> > > > > > +/** Structure specifying a single operation */ struct rte_bbdev_op {
> > > > > > +	enum rte_bbdev_op_type type;  /**< Type of this operation */
> > > > > > +	int status;  /**< Status of operation that was performed */
> > > > > > +	struct rte_mempool *mempool;  /**< Mempool which op
> instance
> > > > > > +is in
> > > > > */
> > > > > > +	void *opaque_data;  /**< Opaque pointer for user data */
> > > > > > +	/**
> > > > > > +	 * Anonymous union of operation-type specific parameters.
> > > > > > +When
> > > > > allocated
> > > > > > +	 * using rte_bbdev_op_pool_create(), space is allocated for the
> > > > > > +	 * parameters at the end of each rte_bbdev_op structure, and
> the
> > > > > > +	 * pointers here point to it.
> > > > > > +	 */
> > > > > > +	RTE_STD_C11
> > > > > > +	union {
> > > > > > +		void *generic;
> > > > > > +		struct rte_bbdev_op_turbo_dec *turbo_dec;
> > > > > > +		struct rte_bbdev_op_turbo_enc *turbo_enc;
> > > > > > +	};
> > > > > > +};
> > > > >
> > > > > I am not sure it is a good idea to fit every operations in the
> > > > > same struct and the same functions.
> > > >
> > > > Due to the fact that our design adopts this idea that a device can
> > > > support both the encode and decode operations.
> > > > Then, at the time of PMD registration, the enqueue functions is allocated.
> > > > This enqueue() function is common for both operations.
> > > > This fitted operation structure is essential for the driver to
> > > > decide on the
> > > operation.
> > >
> > > Sorry I do not understand why you must have a "generic operation".
> > > Please, could you try again to explain this design to someone not
> > > fully understanding how turbo enc/dec works?
> >
> > Oh, sorry, I was not paying attention that you're referring to "void *generic"
> > It is just a place-holder for any other operation types. Can be removed if you
> like.
> 
> No I was not referring to void *generic.
> It is the same question as in the RFC.
> I don't understand the benefit of grouping different things in an union.

There is no benefit, this is a restriction because there is only one function pointer
for enq and another one for deq in the ops structure. Again for the same reason
of trying to keep things in sync with ethdev and cryptodev.
I've always wanted to make it as you proposed, that way it is more performant
(no checking for the type of operation.) If this is agreed, I will do it with all my pleasure :)

The optimum solution though IMHO would be to make the generic enq/deq function pointers
per queue, instead of being per device; that way every enqueue goes straight to
the queue-specific function that matches its operation type.
Notice that currently we have turbo_enc/turbo_dec, but in the future we may have more..

Re: [dpdk-dev] [RFC] Wireless Base Band Device (bbdev)

[Thomas Monjalon]

07/10/2017 01:27, Mokhtar, Amr:
> 
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Sent: Thursday 5 October 2017 23:23
> > To: Mokhtar, Amr <amr.mokhtar@intel.com>
> > Cc: dev@dpdk.org; fbl@redhat.com; aconole@redhat.com; bluca@debian.org
> > Subject: Re: [dpdk-dev] [RFC] Wireless Base Band Device (bbdev)
> > 
> > 05/10/2017 23:55, Mokhtar, Amr:
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > 03/10/2017 16:29, Mokhtar, Amr:
> > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > > 25/08/2017 15:46, Amr Mokhtar:
> > > > > > > +int
> > > > > > > +rte_bbdev_configure(uint8_t dev_id, uint16_t num_queues,
> > > > > > > +		const struct rte_bbdev_conf *conf);
> > > > > >
> > > > > > I am not convinced by the "configure all" function in ethdev.
> > > > > > We break the ABI each time we add a new feature to configure.
> > > > > > And it does not really help to have all configurations in one struct.
> > > > > > Would you mind to split the struct rte_bbdev_conf and split the
> > > > > > function accordingly?
> > > > >
> > > > > There is nothing to split tbh. The only parameter it has is the socket_id.
> > > > > And in fact, it's optional, can be null. The only config we need is
> > num_queues.
> > > >
> > > > Indeed, there is nothing in this struct.
> > > > If you need only to allocate queues, you just have to rename this function.
> > > >
> > > > > I don't see in the near future that we may need to add more config params.
> > > > > As a side, in the time of the implementation we were trying to
> > > > > avoid any diversions from the current design ideology of ethdev and
> > cryptodev.
> > > >
> > > > There is no ideology in ethdev, just some mistakes ;)
> > > >
> > > > > Can we leave it for consideration with future releases?
> > > >
> > > > No it should be addressed from the beginning.
> > > >
> > > > When you will need to add something more to configure port-wise, you
> > > > should add a new function instead of breaking the ABI of the global conf
> > struct.
> > > > That's why the configure option should be more specialized.
> > > >
> > > > Distro people were complaining about ABI breakage last week.
> > > > This is exactly an example of how to avoid it from the beginning.
> > > >
> > >
> > > Ok, got your point. I was looking at it from an API-only standpoint.
> > > How about modifying it into?
> > > int
> > > rte_bbdev_setup_queues(uint16_t dev_id, uint16_t num_queues, int
> > > socket_id);
> > 
> > Yes OK
> > 
> > [...]
> > > > > > > +struct __rte_cache_aligned rte_bbdev {
> > > > > > > +	rte_bbdev_enqueue_ops_t enqueue_ops; /**< Enqueue
> > function */
> > > > > > > +	rte_bbdev_dequeue_ops_t dequeue_ops;  /**< Dequeue
> > function */
> > > > > > > +	const struct rte_bbdev_ops *dev_ops;  /**< Functions
> > > > > > > +exported by PMD
> > > > > > */
> > > > > > > +	struct rte_bbdev_data *data;  /**< Pointer to device data */
> > > > > > > +	bool attached;  /**< If device is currently attached or not
> > > > > > > +*/
> > > > > >
> > > > > > What "attached" means?
> > > > > > I'm afraid you are trying to manage hotplug in the wrong layer.
> > > > >
> > > > > Hotplug is not supported in the current release.
> > > >
> > > > It is not answering the question.
> > > > What is an "attached" device?
> > >
> > > "Attached" means that the PCI device was probed and the bbdev device slot is
> > allocated.
> > > For software devices, means that a virtual bbdev device (vdev) is allocated for
> > bbdev.
> > > Same way the "attached" approach used in cryptodev.
> > 
> > Not sure to understand.
> > If "attached" means "allocated", when is it false?
> 
> Currently in bbdev, it is set to true and never goes false.
> As I said the Hotplug feature is not fully supported in the current version. I can remove that flag for now.
> 
> But generally, it should be cleared to false when rte_pci_driver->remove function is called. (Hotplug?)

Hotplug is still a work in progress in DPDK.
Please remove this flag if it is useless.
We will add something if needed when hotplug support will be better designed.

> > [...]
> > > > > > > +/** Structure specifying a single operation */ struct rte_bbdev_op {
> > > > > > > +	enum rte_bbdev_op_type type;  /**< Type of this operation */
> > > > > > > +	int status;  /**< Status of operation that was performed */
> > > > > > > +	struct rte_mempool *mempool;  /**< Mempool which op
> > instance
> > > > > > > +is in
> > > > > > */
> > > > > > > +	void *opaque_data;  /**< Opaque pointer for user data */
> > > > > > > +	/**
> > > > > > > +	 * Anonymous union of operation-type specific parameters.
> > > > > > > +When
> > > > > > allocated
> > > > > > > +	 * using rte_bbdev_op_pool_create(), space is allocated for the
> > > > > > > +	 * parameters at the end of each rte_bbdev_op structure, and
> > the
> > > > > > > +	 * pointers here point to it.
> > > > > > > +	 */
> > > > > > > +	RTE_STD_C11
> > > > > > > +	union {
> > > > > > > +		void *generic;
> > > > > > > +		struct rte_bbdev_op_turbo_dec *turbo_dec;
> > > > > > > +		struct rte_bbdev_op_turbo_enc *turbo_enc;
> > > > > > > +	};
> > > > > > > +};
> > > > > >
> > > > > > I am not sure it is a good idea to fit every operations in the
> > > > > > same struct and the same functions.
> > > > >
> > > > > Due to the fact that our design adopts this idea that a device can
> > > > > support both the encode and decode operations.
> > > > > Then, at the time of PMD registration, the enqueue functions is allocated.
> > > > > This enqueue() function is common for both operations.
> > > > > This fitted operation structure is essential for the driver to
> > > > > decide on the
> > > > operation.
> > > >
> > > > Sorry I do not understand why you must have a "generic operation".
> > > > Please, could you try again to explain this design to someone not
> > > > fully understanding how turbo enc/dec works?
> > >
> > > Oh, sorry, I was not paying attention that you're referring to "void *generic"
> > > It is just a place-holder for any other operation types. Can be removed if you
> > like.
> > 
> > No I was not referring to void *generic.
> > It is the same question as in the RFC.
> > I don't understand the benefit of grouping different things in an union.
> 
> There is no benefit, this is a restriction because there is only one function pointer
> for enq and another one for deq in the ops structure. Again for the same reason
> of trying to keep things in sync with ethdev and cryptodev.
> I've always wanted to make it as you proposed, that way it is more performant
> (no checking for the type of operation.) If this is agreed, I will do it with all my pleasure :)
> 
> The optimum solution though IMHO would be to make the generic enq/deq function pointers
> per queue, instead of being per device; that way every enqueue goes straight to
> the queue-specific function that matches its operation type.
> Notice that currently we have turbo_enc/turbo_dec, but in the future we may have more..

Please do not impose some restrictions to your API just because you want
to mimic ethdev.
Feel free to innovate :)