[dpdk-dev] [PATCH v4 1/3] Add Intel FPGA BUS Lib Code

[Shreyansh Jain]

Hello Rosen,

On Saturday 31 March 2018 09:33 PM, Rosen Xu wrote:
> Signed-off-by: Rosen Xu <rosen.xu at intel.com>
> ---
>   config/common_base                          |   5 +
>   drivers/bus/Makefile                        |   1 +
>   drivers/bus/ifpga/Makefile                  |  33 ++
>   drivers/bus/ifpga/ifpga_bus.c               | 517 ++++++++++++++++++++++++++++
>   drivers/bus/ifpga/ifpga_common.c            | 141 ++++++++
>   drivers/bus/ifpga/ifpga_common.h            |  22 ++
>   drivers/bus/ifpga/ifpga_logs.h              |  31 ++
>   drivers/bus/ifpga/rte_bus_ifpga.h           | 175 ++++++++++
>   drivers/bus/ifpga/rte_bus_ifpga_version.map |   8 +
>   mk/rte.app.mk                               |   2 +
>   10 files changed, 935 insertions(+)
>   create mode 100644 drivers/bus/ifpga/Makefile
>   create mode 100644 drivers/bus/ifpga/ifpga_bus.c
>   create mode 100644 drivers/bus/ifpga/ifpga_common.c
>   create mode 100644 drivers/bus/ifpga/ifpga_common.h
>   create mode 100644 drivers/bus/ifpga/ifpga_logs.h
>   create mode 100644 drivers/bus/ifpga/rte_bus_ifpga.h
>   create mode 100644 drivers/bus/ifpga/rte_bus_ifpga_version.map
> 
> diff --git a/config/common_base b/config/common_base
> index ad03cf4..49f6b09 100644
> --- a/config/common_base
> +++ b/config/common_base
> @@ -134,6 +134,11 @@ CONFIG_RTE_LIBRTE_PCI_BUS=y
>   CONFIG_RTE_LIBRTE_VDEV_BUS=y
>   
>   #
> +# Compile the Intel FPGA bus
> +#
> +CONFIG_RTE_LIBRTE_IFPGA_BUS=y
> +
> +#
>   # Compile ARK PMD
>   #
>   CONFIG_RTE_LIBRTE_ARK_PMD=y
> diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
> index 7ef2593..55d2dfe 100644
> --- a/drivers/bus/Makefile
> +++ b/drivers/bus/Makefile
> @@ -7,5 +7,6 @@ DIRS-$(CONFIG_RTE_LIBRTE_DPAA_BUS) += dpaa
>   DIRS-$(CONFIG_RTE_LIBRTE_FSLMC_BUS) += fslmc
>   DIRS-$(CONFIG_RTE_LIBRTE_PCI_BUS) += pci
>   DIRS-$(CONFIG_RTE_LIBRTE_VDEV_BUS) += vdev
> +DIRS-$(CONFIG_RTE_LIBRTE_IFPGA_BUS) += ifpga

When I attempted to compile the above using SHARED_LIB=y, this is what I 
got:

--->8---
   LD librte_bus_ifpga.so.1.1
ifpga_bus.o: In function `rte_ifpga_parse':
ifpga_bus.c:(.text+0x2eb): undefined reference to `rte_rawdevs'
ifpga_bus.o: In function `rte_ifpga_scan':
ifpga_bus.c:(.text+0x53e): undefined reference to `rte_kvargs_parse'
ifpga_bus.c:(.text+0x55e): undefined reference to `rte_kvargs_count'
ifpga_bus.c:(.text+0x580): undefined reference to `rte_kvargs_process'
ifpga_bus.c:(.text+0x5e0): undefined reference to `rte_rawdevs'
ifpga_bus.c:(.text+0x746): undefined reference to `rte_kvargs_free'
ifpga_bus.c:(.text+0x7b8): undefined reference to `rte_pci_addr_cmp'
ifpga_bus.c:(.text+0x858): undefined reference to `rte_kvargs_parse'
ifpga_bus.c:(.text+0x878): undefined reference to `rte_kvargs_count'
ifpga_bus.c:(.text+0x89c): undefined reference to `rte_kvargs_process'
ifpga_bus.c:(.text+0x8b8): undefined reference to `rte_kvargs_count'
ifpga_bus.c:(.text+0x8dc): undefined reference to `rte_kvargs_process'
ifpga_bus.c:(.text+0x8f8): undefined reference to `rte_kvargs_count'
ifpga_bus.c:(.text+0x91c): undefined reference to `rte_kvargs_process'
ifpga_bus.c:(.text+0x981): undefined reference to `rte_kvargs_free'
ifpga_common.o: In function `ifpga_pci_addr_cmp':
ifpga_common.c:(.text+0x2c5): undefined reference to 
`rte_eal_compare_pci_addr'
collect2: error: ld returned 1 exit status
/home/shreyansh/build/DPDK/00_dpdk/mk/rte.lib.mk:98: recipe for target 
'librte_bus_ifpga.so.1.1' failed
make[6]: *** [librte_bus_ifpga.so.1.1] Error 1
/home/shreyansh/build/DPDK/00_dpdk/mk/rte.subdir.mk:35: recipe for 
target 'ifpga' failed
make[5]: *** [ifpga] Error 2
--->8---

Essentially, you are dependent on librte_kvargs but you have not 
declared that in your Makefile. Same for lib_rawdev.

>   
>   include $(RTE_SDK)/mk/rte.subdir.mk
> diff --git a/drivers/bus/ifpga/Makefile b/drivers/bus/ifpga/Makefile
> new file mode 100644
> index 0000000..1b569af
> --- /dev/null
> +++ b/drivers/bus/ifpga/Makefile
> @@ -0,0 +1,33 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2018 Intel Corporation
> +
> +include $(RTE_SDK)/mk/rte.vars.mk
> +
> +#
> +# library name
> +#
> +LIB = librte_bus_ifpga.a
> +
> +CFLAGS += -O3
> +CFLAGS += $(WERROR_FLAGS)
> +
> +# versioning export map
> +EXPORT_MAP := rte_bus_ifpga_version.map
> +
> +# library version
> +LIBABIVER := 1
> +
> +VPATH += $(SRCDIR)/base
> +
> +SRCS-y += \
> +        ifpga_bus.c \
> +        ifpga_common.c
> +
> +LDLIBS += -lrte_eal
> +
> +#
> +# Export include files
> +#
> +SYMLINK-y-include += rte_bus_ifpga.h
> +
> +include $(RTE_SDK)/mk/rte.lib.mk
> diff --git a/drivers/bus/ifpga/ifpga_bus.c b/drivers/bus/ifpga/ifpga_bus.c
> new file mode 100644
> index 0000000..eba2615
> --- /dev/null
> +++ b/drivers/bus/ifpga/ifpga_bus.c
> @@ -0,0 +1,517 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2010-2018 Intel Corporation
> + */
> +

[...]

> +
> +	if (rawdev->dev_ops &&
> +		rawdev->dev_ops->dev_start &&
> +		rawdev->dev_ops->dev_start(rawdev))
> +		goto free_dev;
> +	if (path) {
> +		strncpy(afu_pr_conf.bs_path, path, strlen(path));

strncpy with demarcation based on source is not right. strlen(path) can 
be larger than afu_pr_conf.bs_path

I saw some comment from Gaetan in previous version, if I recall correctly.

> +		if (rawdev->dev_ops->firmware_load &&
> +			rawdev->dev_ops->firmware_load(rawdev,
> +					&afu_pr_conf)){
> +			printf("firmware load error %d\n", ret);
> +			goto free_dev;
> +		}
> +		afu_dev->id.uuid_low  = afu_pr_conf.afu_id.uuid_low;
> +		afu_dev->id.uuid_high = afu_pr_conf.afu_id.uuid_high;
> +	}
> +
> +	return afu_dev;
> +
> +free_dev:
> +	free(afu_dev);
> +end:
> +	if (kvlist)
> +		rte_kvargs_free(kvlist);
> +	if (path)
> +		free(path);
> +
> +	return NULL;
> +}
> +
> +/*
> + * Scan the content of the FPGA bus, and the devices in the devices
> + * list
> + */
> +static int
> +rte_ifpga_scan(void)
> +{

[...]

> +
> +static int
> +ifpga_probe_all_drivers(struct rte_afu_device *afu_dev)
> +{
> +	struct rte_afu_driver *drv = NULL;
> +	int rc;
> +
> +	if (afu_dev == NULL)
> +		return -1;
> +
> +	/* Check if a driver is already loaded */
> +	if (afu_dev->driver != NULL)
> +		return 0;
> +
> +	TAILQ_FOREACH(drv, &rte_ifpga_bus.driver_list, next) {
> +		rc = ifpga_probe_one_driver(drv, afu_dev);
> +		if (rc < 0)
> +			/* negative value is an error */
> +			return -1;
> +		if (rc > 0)
> +			/* positive value means driver doesn't support it */
> +			continue;
> +		return 0;
> +	}
> +	return 1;
> +}
> +
> +/*
> + * Scan the content of the PCI bus, and call the probe() function for

I think you are not moving on PCI bus elements. You are looping on 
rte_ifpga_bus.

> + * all registered drivers that have a matching entry in its id_table
> + * for discovered devices.
> + */
> +static int
> +rte_ifpga_probe(void)
> +{
> +	struct rte_ifpga_device *ifpga_dev;
> +	struct rte_afu_device *afu_dev = NULL;
> +	int ret = 0;
> +
> +	TAILQ_FOREACH(ifpga_dev, &rte_ifpga_bus.ifpga_list, next) {
> +		TAILQ_FOREACH(afu_dev, &ifpga_dev->afu_list, next) {
> +
> +			if (afu_dev->device.driver)
> +				continue;
> +
> +			ret = ifpga_probe_all_drivers(afu_dev);
> +			if (ret < 0)
> +				IFPGA_BUS_ERR("failed to initialize %s device\n",
> +					rte_ifpga_device_name(afu_dev));
> +		}
> +	}
> +
> +	return 0;
> +}
> +

[...]

> diff --git a/drivers/bus/ifpga/ifpga_common.c b/drivers/bus/ifpga/ifpga_common.c
> new file mode 100644
> index 0000000..124ffd2
> --- /dev/null
> +++ b/drivers/bus/ifpga/ifpga_common.c
> @@ -0,0 +1,141 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2010-2018 Intel Corporation
> + */
> +

[...]

> +
> +}
> +int ifpga_get_bdf_arg(const char *key __rte_unused,
> +	const char *value, void *extra_args)
> +{
> +#define MAX_PATH_LEN 1024

Is this max len of a file path or a max len of the value string (BDF).
Can you rename this?

Just a trivial comment, though.

[...]

> diff --git a/drivers/bus/ifpga/ifpga_logs.h b/drivers/bus/ifpga/ifpga_logs.h
> new file mode 100644
> index 0000000..873e0a4
> --- /dev/null
> +++ b/drivers/bus/ifpga/ifpga_logs.h
> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2010-2018 Intel Corporation
> + */
> +
> +#ifndef _IFPGA_LOGS_H_
> +#define _IFPGA_LOGS_H_
> +
> +#include <rte_log.h>
> +
> +extern int ifpga_bus_logtype;
> +
> +#define IFPGA_LOG(level, fmt, args...) \
> +	rte_log(RTE_LOG_ ## level, ifpga_bus_logtype, "%s(): " fmt "\n", \
> +		__func__, ##args)

I don't see macro above being used. Would you be using this in later 
patches? (But, i think they might have their own logging definitions).

> +
> +#define IFPGA_BUS_LOG(level, fmt, args...) \
> +	rte_log(RTE_LOG_ ## level, ifpga_bus_logtype, "%s(): " fmt "\n", \
> +		__func__, ##args)
> +
> +#define IFPGA_BUS_FUNC_TRACE() IFPGA_BUS_LOG(DEBUG, ">>")
> +
> +#define IFPGA_BUS_DEBUG(fmt, args...) \
> +	IFPGA_BUS_LOG(DEBUG, fmt, ## args)
> +#define IFPGA_BUS_INFO(fmt, args...) \
> +	IFPGA_BUS_LOG(INFO, fmt, ## args)
> +#define IFPGA_BUS_ERR(fmt, args...) \
> +	IFPGA_BUS_LOG(ERR, fmt, ## args)
> +#define IFPGA_BUS_WARN(fmt, args...) \
> +	IFPGA_BUS_LOG(WARNING, fmt, ## args)
> +
> +#endif /* _IFPGA_BUS_LOGS_H_ */
> diff --git a/drivers/bus/ifpga/rte_bus_ifpga.h b/drivers/bus/ifpga/rte_bus_ifpga.h
> new file mode 100644
> index 0000000..e22ab4e
> --- /dev/null
> +++ b/drivers/bus/ifpga/rte_bus_ifpga.h
> @@ -0,0 +1,175 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2010-2018 Intel Corporation
> + */
> +
> +#ifndef _RTE_BUS_IFPGA_H_
> +#define _RTE_BUS_IFPGA_H_
> +
> +/**
> + * @file
> + *
> + * RTE PCI Bus Interface

This is not a "RTE PCI Bus Interface" - It should be AFU/IFPA Bus 
interface file

There are some comments which were given in early versions. Like this 
one. Please do respond to those individually if you are not fixing them.
(Also, it is nice to get acknowledgment of those which are being fixed).

> + */
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include <rte_bus.h>
> +#include <rte_pci.h>
> +
> +/** Name of Intel FPGA Bus */
> +#define IFPGA_BUS_NAME ifpga
> +
> +/* Forward declarations */
> +struct rte_ifpga_device;
> +struct rte_afu_device;
> +struct rte_afu_driver;
> +
> +/** List of Intel FPGA devices */
> +TAILQ_HEAD(rte_ifpga_device_list, rte_ifpga_device);
> +/** List of Intel AFU devices */
> +TAILQ_HEAD(rte_afu_device_list, rte_afu_device);
> +/** List of AFU drivers */
> +TAILQ_HEAD(rte_afu_driver_list, rte_afu_driver);
> +
> +#define IFPGA_BUS_BITSTREAM_PATH_MAX_LEN 256
> +
> +struct rte_afu_uuid {
> +	uint64_t uuid_low;
> +	uint64_t uuid_high;
> +} __attribute__ ((packed));
> +
> +#define IFPGA_BUS_DEV_PORT_MAX 4
> +
> +/**
> + * A structure describing an ID for a AFU driver. Each driver provides a
> + * table of these IDs for each device that it supports.
> + */
> +struct rte_afu_id {
> +	struct rte_pci_addr pci_addr;
> +	uint64_t uuid_low;
> +	uint64_t uuid_high;
> +	int      port;
> +} __attribute__ ((packed));
> +
> +/**
> + * A structure pr configuration AFU driver.
> + */
> +
> +struct rte_afu_pr_conf {
> +	struct rte_afu_id afu_id;
> +	int pr_enable;
> +	char		bs_path[IFPGA_BUS_BITSTREAM_PATH_MAX_LEN];
             ^^^^^^^^^^^
Some stray indentation issue, it seems

> +};
> +
> +#define AFU_PRI_STR_SIZE (PCI_PRI_STR_SIZE + 8)
> +
> +/**
> + * A structure describing a fpga device.
> + */
> +struct rte_ifpga_device {
> +	TAILQ_ENTRY(rte_ifpga_device) next;       /**< Next in device list. */
> +	struct rte_pci_addr pci_addr;
> +	struct rte_rawdev *rdev;
> +	struct rte_afu_device_list afu_list;  /**< List of AFU devices */
> +};
> +
> +/**
> + * A structure describing a AFU device.
> + */
> +struct rte_afu_device {
> +	TAILQ_ENTRY(rte_afu_device) next;       /**< Next in device list. */
> +	struct rte_device device;               /**< Inherit core device */
> +	struct rte_rawdev *rawdev;    /**< Point Rawdev */
> +	struct rte_ifpga_device *ifpga_dev;    /**< Point ifpga device */
> +	struct rte_afu_id id;                   /**< AFU id within FPGA. */
> +	uint32_t num_region;   /**< number of regions found */
> +	struct rte_mem_resource mem_resource[PCI_MAX_RESOURCE];
> +						/**< PCI Memory Resource */
> +	struct rte_intr_handle intr_handle;     /**< Interrupt handle */
> +	struct rte_afu_driver *driver;          /**< Associated driver */
> +	char path[IFPGA_BUS_BITSTREAM_PATH_MAX_LEN];
> +} __attribute__ ((packed));
> +
> +/**
> + * @internal
> + * Helper macro for drivers that need to convert to struct rte_afu_device.
> + */
> +#define RTE_DEV_TO_AFU(ptr) \
> +	container_of(ptr, struct rte_afu_device, device)
> +
> +/**
> + * Initialisation function for the driver called during PCI probing.

PCI Probing would have already been done through the PCI bus. I think 
this is probing of the AFU devices (based on the PCI already probed).

> + */
> +typedef int (afu_probe_t)(struct rte_afu_device *);
> +
> +/**
> + * Uninitialisation function for the driver called during hotplugging.
> + */
> +typedef int (afu_remove_t)(struct rte_afu_device *);
> +
> +/**
> + * A structure describing a PCI device.
                               ^^^^
Structure describing an AFU device.

> + */
> +struct rte_afu_driver {
> +	TAILQ_ENTRY(rte_afu_driver) next;       /**< Next afu driver. */
> +	struct rte_driver driver;               /**< Inherit core driver. */
> +	afu_probe_t *probe;                     /**< Device Probe function. */
> +	afu_remove_t *remove;                   /**< Device Remove function. */
> +	const struct rte_afu_uuid *id_table;    /**< AFU uuid within FPGA. */
> +	uint32_t drv_flags;         /**< Flags contolling handling of device. */
> +};
> +
> +/**
> + * Structure describing the Intel FPGA bus
> + */
> +struct rte_ifpga_bus {
> +	struct rte_bus bus;               /**< Inherit the generic class */
> +	struct rte_ifpga_device_list ifpga_list;  /**< List of FPGA devices */
> +	struct rte_afu_driver_list driver_list;  /**< List of FPGA drivers */
> +};
> +
> +static inline const char *
> +rte_ifpga_device_name(const struct rte_afu_device *afu)
> +{
> +	if (afu && afu->device.name)
> +		return afu->device.name;
> +	return NULL;
> +}
> +
> +extern struct rte_ifpga_bus rte_ifpga_bus;
> +
> +/**
> + * Register a ifpga afu device driver.
> + *
> + * @param driver
> + *   A pointer to a rte_afu_driver structure describing the driver
> + *   to be registered.
> + */
> +void rte_ifpga_driver_register(struct rte_afu_driver *driver);
> +
> +/**
> + * Unregister a ifpga afu device driver.
> + *
> + * @param driver
> + *   A pointer to a rte_afu_driver structure describing the driver
> + *   to be unregistered.
> + */
> +void rte_ifpga_driver_unregister(struct rte_afu_driver *driver);
> +
> +#define RTE_PMD_REGISTER_AFU(nm, afudrv)\
> +RTE_INIT(afudrvinitfn_ ##afudrv);\
> +static const char *afudrvinit_ ## nm ## _alias;\
> +static void afudrvinitfn_ ##afudrv(void)\
> +{\
> +	(afudrv).driver.name = RTE_STR(nm);\
> +	(afudrv).driver.alias = afudrvinit_ ## nm ## _alias;\
> +	rte_ifpga_driver_register(&afudrv);\
> +} \
> +RTE_PMD_EXPORT_NAME(nm, __COUNTER__)
> +
> +#define RTE_PMD_REGISTER_AFU_ALIAS(nm, alias)\
> +static const char *afudrvinit_ ## nm ## _alias = RTE_STR(alias)
> +
> +#endif /* _RTE_BUS_IFPGA_H_ */
> diff --git a/drivers/bus/ifpga/rte_bus_ifpga_version.map b/drivers/bus/ifpga/rte_bus_ifpga_version.map
> new file mode 100644
> index 0000000..4edc9c0
> --- /dev/null
> +++ b/drivers/bus/ifpga/rte_bus_ifpga_version.map
> @@ -0,0 +1,8 @@
> +DPDK_18.05 {
> +	global:
> +
> +    rte_ifpga_driver_register;
> +    rte_ifpga_driver_unregister;

Should be tab indented

> +
> +	local: *;
> +};

[...]

One suggestion:

I think a lot of comments were provided by Gaetan in the previous 
version. I see some that some of them are still not fixed in this version.

I suggest you individually reply to his comments if you are not going to 
fix, with reason. He put quite an effort to go through your patches.

That would help you track comments as well as not discount a reviewers 
effort.

-
Shreyansh