[v3,6/8] raw/ioat: add configure, start and stop functions

Message ID 20190627104055.8244-7-bruce.richardson@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series raw/ioat: driver for Intel QuickData Technology |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Bruce Richardson June 27, 2019, 10:40 a.m. UTC
  Allow initializing a driver instance. Include selftest to validate these
functions.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>

---

V3: don't add a new descriptor format struct, reuse from rte_ioat_spec.h
V2: test cases placed in self-test routine
---
 app/test/test_rawdev.c              |  2 +-
 doc/guides/rawdevs/ioat_rawdev.rst  | 32 ++++++++++++
 drivers/raw/ioat/Makefile           |  1 +
 drivers/raw/ioat/ioat_rawdev.c      | 78 +++++++++++++++++++++++++++++
 drivers/raw/ioat/ioat_rawdev_test.c | 41 +++++++++++++++
 drivers/raw/ioat/meson.build        |  3 +-
 drivers/raw/ioat/rte_ioat_rawdev.h  |  4 +-
 7 files changed, 158 insertions(+), 3 deletions(-)
 create mode 100644 drivers/raw/ioat/ioat_rawdev_test.c
  

Comments

Anatoly Burakov June 27, 2019, 12:29 p.m. UTC | #1
On 27-Jun-19 11:40 AM, Bruce Richardson wrote:
> Allow initializing a driver instance. Include selftest to validate these
> functions.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> 
> ---
> 
> V3: don't add a new descriptor format struct, reuse from rte_ioat_spec.h
> V2: test cases placed in self-test routine
> ---
>   app/test/test_rawdev.c              |  2 +-
>   doc/guides/rawdevs/ioat_rawdev.rst  | 32 ++++++++++++
>   drivers/raw/ioat/Makefile           |  1 +
>   drivers/raw/ioat/ioat_rawdev.c      | 78 +++++++++++++++++++++++++++++
>   drivers/raw/ioat/ioat_rawdev_test.c | 41 +++++++++++++++
>   drivers/raw/ioat/meson.build        |  3 +-
>   drivers/raw/ioat/rte_ioat_rawdev.h  |  4 +-
>   7 files changed, 158 insertions(+), 3 deletions(-)
>   create mode 100644 drivers/raw/ioat/ioat_rawdev_test.c
> 
> diff --git a/app/test/test_rawdev.c b/app/test/test_rawdev.c
> index 4db762b4c..731e51717 100644
> --- a/app/test/test_rawdev.c
> +++ b/app/test/test_rawdev.c
> @@ -36,7 +36,7 @@ test_rawdev_selftest_ioat(void)
>   		struct rte_rawdev_info info = { .dev_private = NULL };
>   		if (rte_rawdev_info_get(i, &info) == 0 &&
>   				strstr(info.driver_name, "ioat") != NULL)
> -			return 0;
> +			return rte_rawdev_selftest(i);

Even though it doesn't matter in practice, technically, we can't pass a 
raw return value to the test caller. It should be TEST_SUCCESS or 
TEST_FAILURE.

>   	}
>   
>   	printf("No IOAT rawdev found, skipping tests\n");
> diff --git a/doc/guides/rawdevs/ioat_rawdev.rst b/doc/guides/rawdevs/ioat_rawdev.rst
> index 0ce984490..9ab97e2aa 100644
> --- a/doc/guides/rawdevs/ioat_rawdev.rst
> +++ b/doc/guides/rawdevs/ioat_rawdev.rst
> @@ -117,3 +117,35 @@ the ``dev_private`` field in the ``rte_rawdev_info`` struct should either
>   be NULL, or else be set to point to a structure of type
>   ``rte_ioat_rawdev_config``, in which case the size of the configured device
>   input ring will be returned in that structure.
> +
> +Device Configuration
> +~~~~~~~~~~~~~~~~~~~~~
> +
> +Configuring an IOAT rawdev device is done using the
> +``rte_rawdev_configure()`` API, which takes the same structure parameters
> +as the, previously referenced, ``rte_rawdev_info_get()`` API. The main
> +difference is that, because the parameter is used as input rather than
> +output, the ``dev_private`` structure element cannot be NULL, and must
> +point to a valid ``rte_ioat_rawdev_config`` structure, containing the ring
> +size to be used by the device. The ring size must be a power of two,
> +between 64 and 4096.

<snip>

> +	if (params->ring_size > 4096 || params->ring_size < 64 ||
> +			!rte_is_power_of_2(params->ring_size))
> +		return -EINVAL;
> +
> +	ioat->ring_size = params->ring_size;
> +	if (ioat->desc_ring != NULL) {
> +		rte_free(ioat->desc_ring);
> +		ioat->desc_ring = NULL;
> +	}
> +
> +	/* allocate one block of memory for both descriptors
> +	 * and completion handles.
> +	 */
> +	ioat->desc_ring = rte_zmalloc_socket(NULL,
> +			(DESC_SZ + COMPLETION_SZ) * ioat->ring_size,
> +			0, /* alignment, default to 64Byte */
> +			dev->device->numa_node);

Using rte_zmalloc for hardware structures seems suspect. Do you not need 
IOVA-contiguous memory here?

> +	if (ioat->desc_ring == NULL)
> +		return -ENOMEM;
> +	ioat->hdls = (void *)&ioat->desc_ring[ioat->ring_size];
> +
> +	ioat->ring_addr = rte_malloc_virt2iova(ioat->desc_ring);
> +
> +	/* configure descriptor ring - each one points to next */
> +	for (i = 0; i < ioat->ring_size; i++) {
> +		ioat->desc_ring[i].next = ioat->ring_addr +
> +				(((i + 1) % ioat->ring_size) * DESC_SZ);
> +	}

OK, this *definitely* looks suspect :) with rte_zmalloc(), there's no 
guarantee that the entire allocated block resides on the same physical 
page, so you can't assume IOVA addresses will be contiguous either, 
unless you only intend to operate in IOVA as VA mode (which i didn't 
notice).
  
Pattan, Reshma June 27, 2019, 4:37 p.m. UTC | #2
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
> Sent: Thursday, June 27, 2019 11:41 AM
> To: dev@dpdk.org
> Cc: thomas@monjalon.net; jerinj@marvell.com; Richardson, Bruce
> <bruce.richardson@intel.com>
> Subject: [dpdk-dev] [PATCH v3 6/8] raw/ioat: add configure, start and stop
> functions

<snip>

> +int ioat_rawdev_test(uint16_t dev_id);
> +
>  static int
>  ioat_rawdev_create(const char *name, struct rte_pci_device *dev)  {
>  	static const struct rte_rawdev_ops ioat_rawdev_ops = {
> +			.dev_configure = ioat_dev_configure,
> +			.dev_start = ioat_dev_start,
> +			.dev_stop = ioat_dev_stop,
>  			.dev_info_get = ioat_dev_info_get,
> +			.dev_selftest = ioat_rawdev_test,

Build fail for  ./devtools/test-build.sh x86_64-native-linux-gcc+next+shared
/drivers/raw/ioat/ioat_rawdev.c: In function 'ioat_rawdev_create':
drivers/raw/ioat/ioat_rawdev.c:163:20: error: initialization of 'int (*)(void)' from incompatible pointer type 'int (*)(uint16_t)' {aka 'int (*)(short unsigned int)'} [-Werror=incompatible-pointer-types]
  163 |    .dev_selftest = ioat_rawdev_test,

Thanks,
Reshma
  
Bruce Richardson June 28, 2019, 9:21 p.m. UTC | #3
On Thu, Jun 27, 2019 at 05:37:33PM +0100, Pattan, Reshma wrote:
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
> > Sent: Thursday, June 27, 2019 11:41 AM
> > To: dev@dpdk.org
> > Cc: thomas@monjalon.net; jerinj@marvell.com; Richardson, Bruce
> > <bruce.richardson@intel.com>
> > Subject: [dpdk-dev] [PATCH v3 6/8] raw/ioat: add configure, start and stop
> > functions
> 
> <snip>
> 
> > +int ioat_rawdev_test(uint16_t dev_id);
> > +
> >  static int
> >  ioat_rawdev_create(const char *name, struct rte_pci_device *dev)  {
> >  	static const struct rte_rawdev_ops ioat_rawdev_ops = {
> > +			.dev_configure = ioat_dev_configure,
> > +			.dev_start = ioat_dev_start,
> > +			.dev_stop = ioat_dev_stop,
> >  			.dev_info_get = ioat_dev_info_get,
> > +			.dev_selftest = ioat_rawdev_test,
> 
> Build fail for  ./devtools/test-build.sh x86_64-native-linux-gcc+next+shared
> /drivers/raw/ioat/ioat_rawdev.c: In function 'ioat_rawdev_create':
> drivers/raw/ioat/ioat_rawdev.c:163:20: error: initialization of 'int (*)(void)' from incompatible pointer type 'int (*)(uint16_t)' {aka 'int (*)(short unsigned int)'} [-Werror=incompatible-pointer-types]
>   163 |    .dev_selftest = ioat_rawdev_test,
> 

Apologies, I forgot to put in the cover letter that this depends upon the
rawdev patchset I previously upstreamed: 

http://patches.dpdk.org/project/dpdk/list/?series=5120
  

Patch

diff --git a/app/test/test_rawdev.c b/app/test/test_rawdev.c
index 4db762b4c..731e51717 100644
--- a/app/test/test_rawdev.c
+++ b/app/test/test_rawdev.c
@@ -36,7 +36,7 @@  test_rawdev_selftest_ioat(void)
 		struct rte_rawdev_info info = { .dev_private = NULL };
 		if (rte_rawdev_info_get(i, &info) == 0 &&
 				strstr(info.driver_name, "ioat") != NULL)
-			return 0;
+			return rte_rawdev_selftest(i);
 	}
 
 	printf("No IOAT rawdev found, skipping tests\n");
diff --git a/doc/guides/rawdevs/ioat_rawdev.rst b/doc/guides/rawdevs/ioat_rawdev.rst
index 0ce984490..9ab97e2aa 100644
--- a/doc/guides/rawdevs/ioat_rawdev.rst
+++ b/doc/guides/rawdevs/ioat_rawdev.rst
@@ -117,3 +117,35 @@  the ``dev_private`` field in the ``rte_rawdev_info`` struct should either
 be NULL, or else be set to point to a structure of type
 ``rte_ioat_rawdev_config``, in which case the size of the configured device
 input ring will be returned in that structure.
+
+Device Configuration
+~~~~~~~~~~~~~~~~~~~~~
+
+Configuring an IOAT rawdev device is done using the
+``rte_rawdev_configure()`` API, which takes the same structure parameters
+as the, previously referenced, ``rte_rawdev_info_get()`` API. The main
+difference is that, because the parameter is used as input rather than
+output, the ``dev_private`` structure element cannot be NULL, and must
+point to a valid ``rte_ioat_rawdev_config`` structure, containing the ring
+size to be used by the device. The ring size must be a power of two,
+between 64 and 4096.
+
+The following code shows how the device is configured in
+``test_ioat_rawdev.c``:
+
+.. code-block:: C
+
+   #define IOAT_TEST_RINGSIZE 512
+        struct rte_ioat_rawdev_config p = { .ring_size = -1 };
+        struct rte_rawdev_info info = { .dev_private = &p };
+
+        /* ... */
+
+        p.ring_size = IOAT_TEST_RINGSIZE;
+        if (rte_rawdev_configure(dev_id, &info) != 0) {
+                printf("Error with rte_rawdev_configure()\n");
+                return -1;
+        }
+
+Once configured, the device can then be made ready for use by calling the
+``rte_rawdev_start()`` API.
diff --git a/drivers/raw/ioat/Makefile b/drivers/raw/ioat/Makefile
index 1e10938f3..b1af9c666 100644
--- a/drivers/raw/ioat/Makefile
+++ b/drivers/raw/ioat/Makefile
@@ -21,6 +21,7 @@  EXPORT_MAP := rte_pmd_ioat_version.map
 
 # library source files
 SRCS-$(CONFIG_RTE_LIBRTE_PMD_IOAT_RAWDEV) += ioat_rawdev.c
+SRCS-$(CONFIG_RTE_LIBRTE_PMD_IOAT_RAWDEV) += ioat_rawdev_test.c
 
 # export include files
 SYMLINK-y-include += rte_ioat_rawdev.h
diff --git a/drivers/raw/ioat/ioat_rawdev.c b/drivers/raw/ioat/ioat_rawdev.c
index 08e7586c6..fd1653da2 100644
--- a/drivers/raw/ioat/ioat_rawdev.c
+++ b/drivers/raw/ioat/ioat_rawdev.c
@@ -24,6 +24,78 @@  static struct rte_pci_driver ioat_pmd_drv;
 #define IOAT_PMD_ERR(fmt, args...)    IOAT_PMD_LOG(ERR, fmt, ## args)
 #define IOAT_PMD_WARN(fmt, args...)   IOAT_PMD_LOG(WARNING, fmt, ## args)
 
+#define DESC_SZ sizeof(struct rte_ioat_generic_hw_desc)
+#define COMPLETION_SZ sizeof(__m128i)
+
+static int
+ioat_dev_configure(const struct rte_rawdev *dev, rte_rawdev_obj_t config)
+{
+	struct rte_ioat_rawdev_config *params = config;
+	struct rte_ioat_rawdev *ioat = dev->dev_private;
+	unsigned short i;
+
+	if (dev->started)
+		return -EBUSY;
+
+	if (params == NULL)
+		return -EINVAL;
+
+	if (params->ring_size > 4096 || params->ring_size < 64 ||
+			!rte_is_power_of_2(params->ring_size))
+		return -EINVAL;
+
+	ioat->ring_size = params->ring_size;
+	if (ioat->desc_ring != NULL) {
+		rte_free(ioat->desc_ring);
+		ioat->desc_ring = NULL;
+	}
+
+	/* allocate one block of memory for both descriptors
+	 * and completion handles.
+	 */
+	ioat->desc_ring = rte_zmalloc_socket(NULL,
+			(DESC_SZ + COMPLETION_SZ) * ioat->ring_size,
+			0, /* alignment, default to 64Byte */
+			dev->device->numa_node);
+	if (ioat->desc_ring == NULL)
+		return -ENOMEM;
+	ioat->hdls = (void *)&ioat->desc_ring[ioat->ring_size];
+
+	ioat->ring_addr = rte_malloc_virt2iova(ioat->desc_ring);
+
+	/* configure descriptor ring - each one points to next */
+	for (i = 0; i < ioat->ring_size; i++) {
+		ioat->desc_ring[i].next = ioat->ring_addr +
+				(((i + 1) % ioat->ring_size) * DESC_SZ);
+	}
+
+	return 0;
+}
+
+static int
+ioat_dev_start(struct rte_rawdev *dev)
+{
+	struct rte_ioat_rawdev *ioat = dev->dev_private;
+
+	if (ioat->ring_size == 0 || ioat->desc_ring == NULL)
+		return -EBUSY;
+
+	/* inform hardware of where the descriptor ring is */
+	ioat->regs->chainaddr = ioat->ring_addr;
+	/* inform hardware of where to write the status/completions */
+	ioat->regs->chancmp = ioat->status_addr;
+
+	/* prime the status register to be set to the last element */
+	ioat->status =  ioat->ring_addr + ((ioat->ring_size - 1) * DESC_SZ);
+	return 0;
+}
+
+static void
+ioat_dev_stop(struct rte_rawdev *dev)
+{
+	RTE_SET_USED(dev);
+}
+
 static void
 ioat_dev_info_get(struct rte_rawdev *dev, rte_rawdev_obj_t dev_info)
 {
@@ -34,11 +106,17 @@  ioat_dev_info_get(struct rte_rawdev *dev, rte_rawdev_obj_t dev_info)
 		cfg->ring_size = ioat->ring_size;
 }
 
+int ioat_rawdev_test(uint16_t dev_id);
+
 static int
 ioat_rawdev_create(const char *name, struct rte_pci_device *dev)
 {
 	static const struct rte_rawdev_ops ioat_rawdev_ops = {
+			.dev_configure = ioat_dev_configure,
+			.dev_start = ioat_dev_start,
+			.dev_stop = ioat_dev_stop,
 			.dev_info_get = ioat_dev_info_get,
+			.dev_selftest = ioat_rawdev_test,
 	};
 
 	struct rte_rawdev *rawdev = NULL;
diff --git a/drivers/raw/ioat/ioat_rawdev_test.c b/drivers/raw/ioat/ioat_rawdev_test.c
new file mode 100644
index 000000000..5375da26c
--- /dev/null
+++ b/drivers/raw/ioat/ioat_rawdev_test.c
@@ -0,0 +1,41 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2019 Intel Corporation
+ */
+
+#include "rte_rawdev.h"
+#include "rte_ioat_rawdev.h"
+
+int ioat_rawdev_test(uint16_t dev_id); /* pre-define to keep compiler happy */
+
+int
+ioat_rawdev_test(uint16_t dev_id)
+{
+#define IOAT_TEST_RINGSIZE 512
+	struct rte_ioat_rawdev_config p = { .ring_size = -1 };
+	struct rte_rawdev_info info = { .dev_private = &p };
+
+	rte_rawdev_info_get(dev_id, &info);
+	if (p.ring_size != 0) {
+		printf("Error, initial ring size is non-zero (%d)\n",
+				(int)p.ring_size);
+		return -1;
+	}
+
+	p.ring_size = IOAT_TEST_RINGSIZE;
+	if (rte_rawdev_configure(dev_id, &info) != 0) {
+		printf("Error with rte_rawdev_configure()\n");
+		return -1;
+	}
+	rte_rawdev_info_get(dev_id, &info);
+	if (p.ring_size != IOAT_TEST_RINGSIZE) {
+		printf("Error, ring size is not %d (%d)\n",
+				IOAT_TEST_RINGSIZE, (int)p.ring_size);
+		return -1;
+	}
+
+	if (rte_rawdev_start(dev_id) != 0) {
+		printf("Error with rte_rawdev_start()\n");
+		return -1;
+	}
+	return 0;
+}
diff --git a/drivers/raw/ioat/meson.build b/drivers/raw/ioat/meson.build
index ca23e23fc..40fff6654 100644
--- a/drivers/raw/ioat/meson.build
+++ b/drivers/raw/ioat/meson.build
@@ -2,7 +2,8 @@ 
 # Copyright 2019 Intel Corporation
 
 build = dpdk_conf.has('RTE_ARCH_X86')
-sources = files('ioat_rawdev.c')
+sources = files('ioat_rawdev.c',
+		'ioat_rawdev_test.c')
 deps += ['rawdev', 'bus_pci']
 
 install_headers('rte_ioat_rawdev.h',
diff --git a/drivers/raw/ioat/rte_ioat_rawdev.h b/drivers/raw/ioat/rte_ioat_rawdev.h
index 7e0d72ca3..c09fd0791 100644
--- a/drivers/raw/ioat/rte_ioat_rawdev.h
+++ b/drivers/raw/ioat/rte_ioat_rawdev.h
@@ -14,6 +14,7 @@ 
  * @b EXPERIMENTAL: these structures and APIs may change without prior notice
  */
 
+#include <x86intrin.h>
 #include <rte_memory.h>
 #include <rte_ioat_spec.h>
 
@@ -46,7 +47,8 @@  struct rte_ioat_rawdev {
 	phys_addr_t ring_addr;
 
 	unsigned short ring_size;
-	struct rte_ioat_desc *desc_ring;
+	struct rte_ioat_generic_hw_desc *desc_ring;
+	__m128i *hdls; /* completion handles for returning to user */
 
 	/* to report completions, the device will write status back here */
 	volatile uint64_t status __rte_cache_aligned;