Re: [dpdk-dev] [PATCH v3 6/8] raw/ioat: add configure, start and stop functions

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

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).

-- 
Thanks,
Anatoly