> -----Original Message-----
> From: Tan, Jianfeng
> Sent: Thursday, May 3, 2018 11:58 AM
> To: Xu, Rosen <rosen...@intel.com>; dev@dpdk.org
> Cc: Doherty, Declan <declan.dohe...@intel.com>; Richardson, Bruce
> <bruce.richard...@intel.com>; shreyansh.j...@nxp.com; Yigit, Ferruh
> <ferruh.yi...@intel.com>; Ananyev, Konstantin
> <konstantin.anan...@intel.com>; Zhang, Tianfei <tianfei.zh...@intel.com>;
> Wu, Hao <hao...@intel.com>; gaetan.ri...@6wind.com
> Subject: Re: [dpdk-dev] [PATCH v6 1/5] iFPGA: Add Intel FPGA BUS Library
>
> For title prefix, use "bus/ifpga" instead.
Will fix in next version.
>
>
> On 4/26/2018 5:43 PM, Xu, Rosen wrote:
> > From: Rosen Xu <rosen...@intel.com>
> >
> > Defined FPGA-BUS for Acceleration Drivers of AFUs 1. FPGA PCI Scan
> > (1st Scan) follows DPDK UIO/VFIO PCI Scan Process, probe Intel FPGA
> > Rawdev Driver.
>
> As I understand, this part is not covered in this patch. You might want to add
> a note that "it will be covered in following patches".
Yes, this scan in Intel FPGA raw device driver. Will fix this comment in next
version.
>
> > 2. AFU Scan(2nd Scan) bind DPDK driver to FPGA Partial-Bitstream.
> > This scan is trigged by hotplug of IFPGA Rawdev probe, in this scan
> > the AFUs will be created and their drivers are also probed.
>
> This patch is not only responsible for scan, but also other bus ops. So in
> your
> commit log, you might want to clarify this, like we introduce rte_afu_device.
>
> I have a question, hope you can clarify it a little bit. As I understand, this
> FPGA bus are used for AFU device enumeration, and each device on this bus
> needs a AFU driver to drive. But now we register AFU drivers, but enumerate
> rte_ifpga_device. Why? The reason I can think of, we need to maintain the
> relationship of fpga devices and afu devices.
> But I think similar problem would exist in dpaa and fslmc bus too. It is
> interesting to know the best practice on this.
Suppose we will have multiple FPGA chips in one system, and each FPGA chip will
has multiple AFUs,
So the rte_ifpga_device will describe the FPGA chip.
>
> Besides, I see you are using a devargs for scanning, would you like to provide
> an example here?
This is an example run ethernet functionality on AFU 0 of FPGA (pci addr =
5e:00.0)
#./x86_64-native-linuxapp-gcc/app/testpmd -c 0x3 -n 4 --socket-mem 1024,0
--huge-dir=/mnt/huge --vdev
'net_ifpga_cfg0,ifpga=5e:00.0,port=0,afu_bts=./ether.gbs' -- -i --no-numa
>
>
> >
> > Signed-off-by: Rosen Xu <rosen...@intel.com>
> > Signed-off-by: Figo zhang <tianfei.zh...@intel.com>
> > ---
> > config/common_base | 5 +
> > drivers/bus/Makefile | 1 +
> > drivers/bus/ifpga/Makefile | 32 ++
> > drivers/bus/ifpga/ifpga_bus.c | 503
> ++++++++++++++++++++++++++++
> > drivers/bus/ifpga/ifpga_common.c | 88 +++++
> > drivers/bus/ifpga/ifpga_common.h | 18 +
> > drivers/bus/ifpga/ifpga_logs.h | 31 ++
> > drivers/bus/ifpga/meson.build | 6 +
> > drivers/bus/ifpga/rte_bus_ifpga.h | 168 ++++++++++
> > drivers/bus/ifpga/rte_bus_ifpga_version.map | 10 +
> > drivers/bus/meson.build | 2 +-
> > mk/rte.app.mk | 2 +
> > 12 files changed, 865 insertions(+), 1 deletion(-)
> > 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/meson.build
> > 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 7e45412..b59f1de 100644
> > --- a/config/common_base
> > +++ b/config/common_base
> > @@ -148,6 +148,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 c251b65..cff3567 100644
> > --- a/drivers/bus/Makefile
> > +++ b/drivers/bus/Makefile
> > @@ -9,5 +9,6 @@ DIRS-$(CONFIG_RTE_LIBRTE_FSLMC_BUS) += fslmc
> > endif
> > DIRS-$(CONFIG_RTE_LIBRTE_PCI_BUS) += pci
> > DIRS-$(CONFIG_RTE_LIBRTE_VDEV_BUS) += vdev
> > +DIRS-$(CONFIG_RTE_LIBRTE_IFPGA_BUS) += ifpga
> >
> > 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..3ff3bdb
> > --- /dev/null
> > +++ b/drivers/bus/ifpga/Makefile
> > @@ -0,0 +1,32 @@
> > +# 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 += -DALLOW_EXPERIMENTAL_API
> > +CFLAGS += -O3
> > +CFLAGS += $(WERROR_FLAGS)
> > +LDLIBS += -lrte_eal
> > +LDLIBS += -lrte_rawdev
> > +LDLIBS += -lrte_kvargs
> > +
> > +# versioning export map
> > +EXPORT_MAP := rte_bus_ifpga_version.map
> > +
> > +# library version
> > +LIBABIVER := 1
> > +
> > +SRCS-$(CONFIG_RTE_LIBRTE_IFPGA_BUS) += ifpga_bus.c
> > +SRCS-$(CONFIG_RTE_LIBRTE_IFPGA_BUS) += ifpga_common.c
> > +
> > +#
> > +# Export include files
> > +#
> > +SYMLINK-$(CONFIG_RTE_LIBRTE_IFPGA_BUS)-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..67c24ae
> > --- /dev/null
> > +++ b/drivers/bus/ifpga/ifpga_bus.c
> > @@ -0,0 +1,503 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2010-2018 Intel Corporation
> > + */
> > +
> > +#include <string.h>
> > +#include <inttypes.h>
> > +#include <stdint.h>
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +#include <sys/queue.h>
> > +#include <sys/mman.h>
> > +#include <sys/types.h>
> > +#include <unistd.h>
> > +#include <fcntl.h>
> > +
> > +#include <rte_errno.h>
> > +#include <rte_bus.h>
> > +#include <rte_per_lcore.h>
> > +#include <rte_memory.h>
> > +#include <rte_memzone.h>
> > +#include <rte_eal.h>
> > +#include <rte_common.h>
> > +
> > +#include <rte_devargs.h>
> > +#include <rte_kvargs.h>
> > +#include <rte_alarm.h>
> > +
> > +#include "rte_rawdev.h"
> > +#include "rte_rawdev_pmd.h"
> > +#include "rte_bus_ifpga.h"
> > +#include "ifpga_logs.h"
> > +#include "ifpga_common.h"
> > +
> > +int ifpga_bus_logtype;
> > +
> > +/* Forward declare to access Intel FPGA bus name */
>
> s/declare/declaration, actually, does "Intel FPGA bus on which iFPGA
> devices are connected" sound better?
Yes, sounds good.
>
> > +static struct rte_bus rte_ifpga_bus;
> > +
> > +/** Double linked list of IFPGA device. */
> > +TAILQ_HEAD(ifpga_device_list, rte_ifpga_device);
> > +
> > +static struct ifpga_device_list ifpga_device_list =
> > + TAILQ_HEAD_INITIALIZER(ifpga_device_list);
> > +struct afu_driver_list afu_driver_list =
>
> Why not keep this list as static?
Will fix it in next version.
>
> > + TAILQ_HEAD_INITIALIZER(afu_driver_list);
> > +
> > +
> > +/* register a ifpga bus based driver */
> > +void rte_ifpga_driver_register(struct rte_afu_driver *driver)
> > +{
> > + RTE_VERIFY(driver);
> > +
> > + TAILQ_INSERT_TAIL(&afu_driver_list, driver, next);
> > +}
> > +
> > +/* un-register a fpga bus based driver */
> > +void rte_ifpga_driver_unregister(struct rte_afu_driver *driver)
> > +{
> > + TAILQ_REMOVE(&afu_driver_list, driver, next);
> > +}
> > +
> > +static struct rte_ifpga_device *
> > +ifpga_find_ifpga_dev(const struct rte_rawdev *rdev)
> > +{
> > + struct rte_ifpga_device *ifpga_dev = NULL;
> > +
> > + TAILQ_FOREACH(ifpga_dev, &ifpga_device_list, next) {
> > + if (rdev &&
> > + ifpga_dev->rdev &&
> > + ifpga_dev->rdev == rdev)
> > + return ifpga_dev;
> > + }
> > + return NULL;
> > +}
> > +
> > +static struct rte_afu_device *
> > +ifpga_find_afu_dev(const struct rte_ifpga_device *ifpga_dev,
> > + const struct rte_afu_id *afu_id)
> > +{
> > + struct rte_afu_device *afu_dev = NULL;
> > +
> > + TAILQ_FOREACH(afu_dev, &ifpga_dev->afu_list, next) {
> > + if (!ifpga_afu_id_cmp(&afu_dev->id, afu_id))
> > + return afu_dev;
> > + }
> > + return NULL;
> > +}
> > +
> > +static const char * const valid_args[] = {
> > +#define IFPGA_ARG_NAME "ifpga"
> > + IFPGA_ARG_NAME,
> > +#define IFPGA_ARG_PORT "port"
> > + IFPGA_ARG_PORT,
> > +#define IFPGA_AFU_BTS "afu_bts"
> > + IFPGA_AFU_BTS,
> > + NULL
> > +};
> > +
> > +/*
> > + * Scan the content of the FPGA bus, and the devices in the devices
> > + * list
> > + */
> > +static struct rte_afu_device *
> > +rte_ifpga_scan_one(struct rte_devargs *devargs,
>
> For internal functions, we'd better not use the prefix "rte".
Will fix in next version.
>
> > + struct rte_ifpga_device *ifpga_dev)
> > +{
> > + struct rte_kvargs *kvlist = NULL;
> > + struct rte_rawdev *rawdev = NULL;
> > + struct rte_afu_device *afu_dev = NULL;
> > + struct rte_afu_pr_conf afu_pr_conf;
> > + int ret = 0;
> > + char *path = NULL;
> > +
> > + memset((char *)(&afu_pr_conf), 0, sizeof(struct rte_afu_pr_conf));
>
> For "void *", no need to add explicit type cast.
Will fix in next version.
>
> > +
> > + kvlist = rte_kvargs_parse(devargs->args, valid_args);
> > + if (!kvlist) {
> > + IFPGA_BUS_ERR("error when parsing param");
> > + goto end;
> > + }
> > +
> > + if (rte_kvargs_count(kvlist, IFPGA_ARG_PORT) == 1) {
> > + if (rte_kvargs_process(kvlist, IFPGA_ARG_PORT,
> > + &ifpga_get_integer32_arg, &afu_pr_conf.afu_id.port) < 0) {
> > + IFPGA_BUS_ERR("error to parse %s",
> > + IFPGA_ARG_PORT);
> > + goto end;
> > + }
> > + } else {
> > + IFPGA_BUS_ERR("arg %s is mandatory for ifpga bus",
> > + IFPGA_ARG_PORT);
> > + goto end;
> > + }
> > +
> > + if (rte_kvargs_count(kvlist, IFPGA_AFU_BTS) == 1) {
> > + if (rte_kvargs_process(kvlist, IFPGA_AFU_BTS,
> > + &ifpga_get_string_arg, &path) < 0) {
> > + IFPGA_BUS_ERR("error to parse %s",
>
> s/error/failed
I think error and failed is ok.
>
> > + IFPGA_AFU_BTS);
> > + goto end;
> > + }
> > + } else {
> > + IFPGA_BUS_ERR("arg %s is mandatory for ifpga bus",
> > + IFPGA_AFU_BTS);
> > + goto end;
> > + }
> > +
> > + afu_pr_conf.afu_id.uuid_low = 0;
> > + afu_pr_conf.afu_id.uuid_high = 0;
> > + afu_pr_conf.pr_enable = path?1:0;
> > +
> > + rawdev = ifpga_dev->rdev;
> > + if (ifpga_find_afu_dev(ifpga_dev, &afu_pr_conf.afu_id))
> > + goto end;
> > +
> > + afu_dev = calloc(1, sizeof(*afu_dev));
> > + if (!afu_dev)
> > + goto end;
> > +
> > + afu_dev->device.devargs = devargs;
> > + afu_dev->device.numa_node = SOCKET_ID_ANY;
> > + afu_dev->device.name = devargs->name;
> > + afu_dev->rawdev = rawdev;
> > + afu_dev->id.uuid_low = 0;
> > + afu_dev->id.uuid_high = 0;
> > + afu_dev->id.port = afu_pr_conf.afu_id.port;
> > + afu_dev->ifpga_dev = ifpga_dev;
> > +
> > + if (rawdev->dev_ops && rawdev->dev_ops->dev_info_get)
> > + rawdev->dev_ops->dev_info_get(rawdev, afu_dev);
> > +
> > + if (rawdev->dev_ops &&
> > + rawdev->dev_ops->dev_start &&
> > + rawdev->dev_ops->dev_start(rawdev))
> > + goto free_dev;
> > +
> > + strncpy(afu_pr_conf.bs_path, path, sizeof(afu_pr_conf.bs_path));
> > + if (rawdev->dev_ops->firmware_load &&
> > + rawdev->dev_ops->firmware_load(rawdev,
> > + &afu_pr_conf)){
> > + IFPGA_BUS_ERR("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)
>
> You can remove above if statement for simplicity.
>
> > + rte_kvargs_free(kvlist);
> > + if (path)
>
> Ditto.
>
> > + free(path);
> > +
> > + return NULL;
> > +}
> > +
> > +/*
> > + * Scan the content of the FPGA bus, and the devices in the devices
> > + * list
> > + */
> > +static int
> > +rte_ifpga_scan(void)
> > +{
> > + struct rte_ifpga_device *ifpga_dev;
> > + struct rte_devargs *devargs;
> > + struct rte_kvargs *kvlist = NULL;
> > + struct rte_rawdev *rawdev = NULL;
> > + char *name = NULL;
> > + char name1[RTE_RAWDEV_NAME_MAX_LEN];
> > + struct rte_afu_device *afu_dev = NULL;
> > +
> > + /* for FPGA devices we scan the devargs_list populated via cmdline */
> > + RTE_EAL_DEVARGS_FOREACH("ifpga", devargs) {
> > + if (devargs->bus != &rte_ifpga_bus)
> > + continue;
> > +
> > + kvlist = rte_kvargs_parse(devargs->args, valid_args);
> > + if (!kvlist) {
> > + IFPGA_BUS_ERR("error when parsing param");
> > + goto end;
> > + }
> > +
> > + if (rte_kvargs_count(kvlist, IFPGA_ARG_NAME) == 1) {
> > + if (rte_kvargs_process(kvlist, IFPGA_ARG_NAME,
> > + &ifpga_get_string_arg, &name) < 0) {
> > + IFPGA_BUS_ERR("error to parse %s",
> > + IFPGA_ARG_NAME);
> > + goto end;
> > + }
> > + } else {
> > + IFPGA_BUS_ERR("arg %s is mandatory for ifpga bus",
> > + IFPGA_ARG_NAME);
> > + goto end;
> > + }
> > +
> > + memset(name1, 0, sizeof(name1));
> > + snprintf(name1, RTE_RAWDEV_NAME_MAX_LEN, "IFPGA:%s",
> name);
>
> Change to use strlcpy instead.
Got it, I agree, will fix in next version.
>
> > +
> > + rawdev = rte_rawdev_pmd_get_named_dev(name1);
> > + if (!rawdev)
> > + goto end;
> > +
> > + if (ifpga_find_ifpga_dev(rawdev))
> > + continue;
> > +
> > + ifpga_dev = calloc(1, sizeof(*ifpga_dev));
> > + if (!ifpga_dev)
> > + goto end;
> > +
> > + ifpga_dev->rdev = rawdev;
> > + TAILQ_INIT(&ifpga_dev->afu_list);
> > +
> > + TAILQ_INSERT_TAIL(&ifpga_device_list, ifpga_dev, next);
> > + afu_dev = rte_ifpga_scan_one(devargs, ifpga_dev);
> > + if (afu_dev != NULL)
> > + TAILQ_INSERT_TAIL(&ifpga_dev->afu_list, afu_dev, next);
> > + }
> > +
> > +end:
> > + if (kvlist)
>
> Ditto.
>
> > + rte_kvargs_free(kvlist);
> > + if (name)
>
> Ditto.
>
> > + free(name);
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * Match the AFU Driver and AFU Device using the ID Table
> > + */
> > +static int
> > +rte_afu_match(const struct rte_afu_driver *afu_drv,
> > + const struct rte_afu_device *afu_dev)
> > +{
> > + const struct rte_afu_uuid *id_table;
> > +
> > + for (id_table = afu_drv->id_table;
> > + ((id_table->uuid_low != 0) && (id_table->uuid_high != 0));
> > + id_table++) {
> > + /* check if device's identifiers match the driver's ones */
> > + if ((id_table->uuid_low != afu_dev->id.uuid_low) ||
> > + (id_table->uuid_high != afu_dev->id.uuid_high))
> > + continue;
> > +
> > + return 1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +ifpga_probe_one_driver(struct rte_afu_driver *drv,
> > + struct rte_afu_device *afu_dev)
> > +{
> > + int ret;
> > +
> > + if (!rte_afu_match(drv, afu_dev))
> > + /* Match of device and driver failed */
> > + return 1;
> > +
> > + /* reference driver structure */
> > + afu_dev->driver = drv;
> > + afu_dev->device.driver = &drv->driver;
> > +
> > + /* call the driver probe() function */
> > + ret = drv->probe(afu_dev);
> > + if (ret) {
> > + afu_dev->driver = NULL;
> > + afu_dev->device.driver = NULL;
> > + }
> > +
> > + /* return positive value if driver doesn't support this device */
>
> This comment seems wrong; you already exclude the case that this driver
> does not support this device (>0); here, we only have the cases that,
> probe succeeds (=0), and probe fails (<0).
>
> > + return 0;
>
> return ret instead of 0?
I agree, this return ret is better.
>
> > +}
> > +
> > +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, &afu_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 Intel FPGA bus, and call the probe() function
> > for
> > + * 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, &ifpga_device_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;
> > +}
> > +
> > +static int
> > +rte_ifpga_plug(struct rte_device *dev)
> > +{
> > + return ifpga_probe_all_drivers(RTE_DEV_TO_AFU(dev));
> > +}
> > +
> > +static int ifpga_remove_driver(struct rte_afu_device *afu_dev)
> > +{
> > + const char *name;
> > + const struct rte_afu_driver *driver;
> > +
> > + name = rte_ifpga_device_name(afu_dev);
> > + if (!afu_dev->device.driver) {
> > + IFPGA_BUS_DEBUG("no driver attach to device %s\n", name);
> > + return 1;
> > + }
> > +
> > + driver = RTE_DRV_TO_AFU_CONST(afu_dev->device.driver);
> > + return driver->remove(afu_dev);
> > +}
> > +
> > +static int
> > +rte_ifpga_unplug(struct rte_device *dev)
> > +{
> > + struct rte_ifpga_device *ifpga_dev = NULL;
> > + struct rte_afu_device *afu_dev = NULL;
> > + struct rte_devargs *devargs = NULL;
> > + int ret;
> > +
> > + if (dev == NULL)
> > + return -EINVAL;
> > +
> > + afu_dev = RTE_DEV_TO_AFU(dev);
> > + if (!dev)
> > + return -ENOENT;
> > +
> > + ifpga_dev = afu_dev->ifpga_dev;
> > + devargs = dev->devargs;
> > +
> > + ret = ifpga_remove_driver(afu_dev);
> > + if (ret)
> > + return ret;
> > +
> > + TAILQ_REMOVE(&ifpga_dev->afu_list, afu_dev, next);
> > +
> > + rte_devargs_remove(devargs->bus->name, devargs->name);
> > + free(afu_dev);
> > + return 0;
> > +
> > +}
> > +
> > +static struct rte_device *
> > +rte_ifpga_find_device(const struct rte_device *start,
> > + rte_dev_cmp_t cmp, const void *data)
> > +{
> > + struct rte_ifpga_device *ifpga_dev;
> > + struct rte_afu_device *afu_dev;
> > +
> > + TAILQ_FOREACH(ifpga_dev, &ifpga_device_list, next) {
> > + TAILQ_FOREACH(afu_dev, &ifpga_dev->afu_list, next) {
> > + if (start && &afu_dev->device == start) {
> > + start = NULL;
> > + continue;
> > + }
> > + if (cmp(&afu_dev->device, data) == 0)
> > + return &afu_dev->device;
> > + }
> > + }
> > + return NULL;
> > +}
> > +static int
> > +rte_ifpga_parse(const char *name, void *addr)
> > +{
> > + int *out = addr;
> > + struct rte_rawdev *rawdev = NULL;
> > + char rawdev_name[RTE_RAWDEV_NAME_MAX_LEN];
> > + char *c1 = NULL, *c2 = NULL;
> > + int port = IFPGA_BUS_DEV_PORT_MAX;
>
> Duplicated spaces after "int".
Will fix it in next version.
>
> > + char str_port[8];
> > + int str_port_len = 0;
> > + int ret;
> > +
> > + memset(str_port, 0, 8);
> > + c1 = strchr(name, '|');
> > + if (c1 != NULL) {
> > + str_port_len = c1-name;
> > + c2 = c1+1;
> > + }
> > +
> > + if (str_port_len < 8 &&
> > + str_port_len > 0) {
> > + memcpy(str_port, name, str_port_len);
> > + ret = sscanf(str_port, "%d", &port);
> > + if (ret == -1)
> > + return 0;
> > + }
> > +
> > + memset(rawdev_name, 0, sizeof(rawdev_name));
> > + snprintf(rawdev_name, RTE_RAWDEV_NAME_MAX_LEN, "IFPGA:%s",
> c2);
> > + rawdev = rte_rawdev_pmd_get_named_dev(rawdev_name);
> > +
> > + if ((port < IFPGA_BUS_DEV_PORT_MAX) &&
> > + rawdev &&
> > + (addr != NULL))
> > + *out = port;
> > +
> > + if ((port < IFPGA_BUS_DEV_PORT_MAX) &&
> > + rawdev)
> > + return 0;
> > + else
> > + return 1;
> > +}
> > +
> > +static struct rte_bus rte_ifpga_bus = {
> > + .scan = rte_ifpga_scan,
> > + .probe = rte_ifpga_probe,
> > + .find_device = rte_ifpga_find_device,
> > + .plug = rte_ifpga_plug,
> > + .unplug = rte_ifpga_unplug,
> > + .parse = rte_ifpga_parse,
> > +};
> > +
> > +RTE_REGISTER_BUS(IFPGA_BUS_NAME, rte_ifpga_bus);
> > +
> > +RTE_INIT(ifpga_init_log)
> > +{
> > + ifpga_bus_logtype = rte_log_register("bus.ifpga");
> > + if (ifpga_bus_logtype >= 0)
> > + rte_log_set_level(ifpga_bus_logtype, RTE_LOG_NOTICE);
> > +}
> > diff --git a/drivers/bus/ifpga/ifpga_common.c
> b/drivers/bus/ifpga/ifpga_common.c
> > new file mode 100644
> > index 0000000..26fee27
> > --- /dev/null
> > +++ b/drivers/bus/ifpga/ifpga_common.c
> > @@ -0,0 +1,88 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2010-2018 Intel Corporation
> > + */
> > +
> > +#include <string.h>
> > +#include <inttypes.h>
> > +#include <stdint.h>
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +#include <sys/queue.h>
> > +#include <sys/mman.h>
> > +#include <sys/types.h>
> > +#include <unistd.h>
> > +#include <fcntl.h>
> > +
> > +#include <rte_errno.h>
> > +#include <rte_bus.h>
> > +#include <rte_per_lcore.h>
> > +#include <rte_memory.h>
> > +#include <rte_memzone.h>
> > +#include <rte_eal.h>
> > +#include <rte_common.h>
> > +
> > +#include <rte_devargs.h>
> > +#include <rte_kvargs.h>
> > +#include <rte_alarm.h>
> > +
> > +#include "rte_bus_ifpga.h"
> > +#include "ifpga_logs.h"
> > +#include "ifpga_common.h"
> > +
> > +int ifpga_get_string_arg(const char *key __rte_unused,
> > + const char *value, void *extra_args)
> > +{
> > + if (!value || !extra_args)
> > + return -EINVAL;
> > +
> > + *(char **)extra_args = strdup(value);
> > +
> > + if (!*(char **)extra_args)
> > + return -ENOMEM;
> > +
> > + return 0;
> > +}
> > +int ifpga_get_integer32_arg(const char *key __rte_unused,
> > + const char *value, void *extra_args)
> > +{
> > + if (!value || !extra_args)
> > + return -EINVAL;
> > +
> > + *(int *)extra_args = strtoull(value, NULL, 0);
> > +
> > + return 0;
> > +}
> > +int ifpga_get_integer64_arg(const char *key __rte_unused,
> > + const char *value, void *extra_args)
> > +{
> > + if (!value || !extra_args)
> > + return -EINVAL;
> > +
> > + *(uint64_t *)extra_args = strtoull(value, NULL, 0);
> > +
> > + return 0;
> > +}
> > +int ifpga_get_unsigned_long(const char *str, int base)
> > +{
> > + unsigned long num;
> > + char *end = NULL;
> > +
> > + errno = 0;
> > +
> > + num = strtoul(str, &end, base);
> > + if ((str[0] == '\0') || (end == NULL) || (*end != '\0') || (errno != 0))
> > + return -1;
> > +
> > + return num;
> > +}
> > +
> > +int ifpga_afu_id_cmp(const struct rte_afu_id *afu_id0,
> > + const struct rte_afu_id *afu_id1)
> > +{
> > + if ((afu_id0->uuid_low == afu_id1->uuid_low) &&
> > + (afu_id0->uuid_high == afu_id1->uuid_high) &&
> > + (afu_id0->port == afu_id1->port)) {
> > + return 0;
> > + } else
> > + return 1;
> > +}
> > diff --git a/drivers/bus/ifpga/ifpga_common.h
> b/drivers/bus/ifpga/ifpga_common.h
> > new file mode 100644
> > index 0000000..b6662c8
> > --- /dev/null
> > +++ b/drivers/bus/ifpga/ifpga_common.h
> > @@ -0,0 +1,18 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2010-2018 Intel Corporation
> > + */
> > +
> > +#ifndef _IFPGA_COMMON_H_
> > +#define _IFPGA_COMMON_H_
> > +
> > +int ifpga_get_string_arg(const char *key __rte_unused,
> > + const char *value, void *extra_args);
> > +int ifpga_get_integer32_arg(const char *key __rte_unused,
> > + const char *value, void *extra_args);
> > +int ifpga_get_integer64_arg(const char *key __rte_unused,
> > + const char *value, void *extra_args);
> > +int ifpga_get_unsigned_long(const char *str, int base);
> > +int ifpga_afu_id_cmp(const struct rte_afu_id *afu_id0,
> > + const struct rte_afu_id *afu_id1);
> > +
> > +#endif /* _IFPGA_COMMON_H_ */
> > 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)
> > +
> > +#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/meson.build b/drivers/bus/ifpga/meson.build
> > new file mode 100644
> > index 0000000..4ea31f1
> > --- /dev/null
> > +++ b/drivers/bus/ifpga/meson.build
> > @@ -0,0 +1,6 @@
> > +# SPDX-License-Identifier: BSD-3-Clause
> > +# Copyright(c) 2010-2018 Intel Corporation
> > +
> > +deps += ['pci', 'kvargs', 'rawdev']
> > +install_headers('rte_bus_ifpga.h')
> > +sources = files('ifpga_common.c', 'ifpga_bus.c')
> > diff --git a/drivers/bus/ifpga/rte_bus_ifpga.h
> b/drivers/bus/ifpga/rte_bus_ifpga.h
> > new file mode 100644
> > index 0000000..53e7183
> > --- /dev/null
> > +++ b/drivers/bus/ifpga/rte_bus_ifpga.h
> > @@ -0,0 +1,168 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2010-2018 Intel Corporation
> > + */
> > +
> > +#ifndef _RTE_BUS_IFPGA_H_
> > +#define _RTE_BUS_IFPGA_H_
> > +
> > +/**
> > + * @file
> > + *
> > + * RTE Intel FPGA Bus Interface
> > + */
> > +
> > +#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_afu_device;
> > +
> > +/** List of Intel AFU devices */
> > +TAILQ_HEAD(afu_device_list, rte_afu_device);
> > +/** Double linked list of AFU device drivers. */
> > +TAILQ_HEAD(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
>
> It seems that this struct is for describing the AFU device, instead of
> AFU driver, no?
This struct is describing the AFU device's attribute like UUID which is match
AFU driver for AFU device,
and port number.
>
> > + * table of these IDs for each device that it supports.
> > + */
> > +struct rte_afu_id {
> > + uint64_t uuid_low;
> > + uint64_t uuid_high;
>
> Why not reuse struct rte_afu_uuid for above two fields?
Yes, we can reuse the struct rte_afu_uuid.
>
> > + int port;
>
> Can you add a note on what *port* means?
This is port number, suppose that one FPGA chip may has
multiple AFUs, one AFU has one dedicate port. It is like bridge function.
>
> > +} __attribute__ ((packed));
> > +
> > +/**
> > + * A structure pr configuration AFU driver.
>
> What does pr mean? Mind to add a full name for it?
PR means partial reconfiguration.
>
> > + */
> > +
> > +struct rte_afu_pr_conf {
> > + struct rte_afu_id afu_id;
> > + int pr_enable;
> > + char bs_path[IFPGA_BUS_BITSTREAM_PATH_MAX_LEN];
> > +};
> > +
> > +#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_rawdev *rdev;
> > + struct 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];
> > + /**< AFU 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)
> > +
> > +#define RTE_DRV_TO_AFU_CONST(ptr) \
> > + container_of(ptr, const struct rte_afu_driver, driver)
> > +
> > +/**
> > + * Initialisation function for the driver called during FPGA BUS probing.
> > + */
> > +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 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 */
> > +};
>
> Where is this struct used?
This struct no need, will remove in next version.
>
> > +
> > +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;
> > +}
> > +
> > +/**
> > + * 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)
>
> Do we really need alias for new bus? Originally, alias is a way for
> compatibility, but it seems that it's not necessary for new bus.
>
> > +
> > +#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..1304caf
> > --- /dev/null
> > +++ b/drivers/bus/ifpga/rte_bus_ifpga_version.map
> > @@ -0,0 +1,10 @@
> > +DPDK_18.05 {
> > + global:
> > +
> > + ifpga_get_integer32_arg;
> > + ifpga_get_string_arg;
>
> Above two functions shall be not exposed as APIs.
Those APIs used in ifpga raw device driver.
>
> > + rte_ifpga_driver_register;
> > + rte_ifpga_driver_unregister;
> > +
> > + local: *;
> > +};
> > diff --git a/drivers/bus/meson.build b/drivers/bus/meson.build
> > index 58dfbe2..170df25 100644
> > --- a/drivers/bus/meson.build
> > +++ b/drivers/bus/meson.build
> > @@ -1,7 +1,7 @@
> > # SPDX-License-Identifier: BSD-3-Clause
> > # Copyright(c) 2017 Intel Corporation
> >
> > -drivers = ['dpaa', 'fslmc', 'pci', 'vdev']
> > +drivers = ['dpaa', 'fslmc', 'pci', 'vdev', 'ifpga']
> > std_deps = ['eal']
> > config_flag_fmt = 'RTE_LIBRTE_@0@_BUS'
> > driver_name_fmt = 'rte_bus_@0@'
> > diff --git a/mk/rte.app.mk b/mk/rte.app.mk
> > index a145791..9de2edb 100644
> > --- a/mk/rte.app.mk
> > +++ b/mk/rte.app.mk
> > @@ -107,6 +107,8 @@ _LDLIBS-$(CONFIG_RTE_LIBRTE_CMDLINE)
> += -lrte_cmdline
> > _LDLIBS-$(CONFIG_RTE_LIBRTE_REORDER) += -lrte_reorder
> > _LDLIBS-$(CONFIG_RTE_LIBRTE_SCHED) += -lrte_sched
> >
> > +_LDLIBS-$(CONFIG_RTE_LIBRTE_IFPGA_BUS) += -lrte_bus_ifpga
> > +
> > ifeq ($(CONFIG_RTE_EXEC_ENV_LINUXAPP),y)
> > _LDLIBS-$(CONFIG_RTE_LIBRTE_KNI) += -lrte_kni
> > endif
