From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f51.google.com (mail-wm0-f51.google.com [74.125.82.51]) by dpdk.org (Postfix) with ESMTP id 901BC2A66 for ; Wed, 28 Mar 2018 15:52:33 +0200 (CEST) Received: by mail-wm0-f51.google.com with SMTP id l201so5131156wmg.0 for ; Wed, 28 Mar 2018 06:52:33 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=3hZJ6NMs3ubMUUt5a6sb3L6EScNLVPBbBRKqq7Q6m9M=; b=POV9gqaGbmRveFu3kNdY6zyuHu/LIUXl/QbvUXJFWlAHRAsHo3a3aFLcWKAK9bpVJO wyR5NIQF8qo+txUFZdHrkxdzX2TZscy7+aDeXx0D1oex3/Xsc1bNd1FgVoI4EI+aTtH9 LuS19/0KNuCp6NMBr01ugpxhv6tfIC8NHyGDP+q6nHnfG1zO0K3DADUCM+GGnoDhUoHm xqw5iEWnuQF0Y7ql+JCGeqfIRgJmpzFQwgK4hiAomT0es5m1pqjXbBuCeAZ0r6JM86jX aUQZqX2PkYLYzTEpR19td25xaHJN1SDytsSny71hLx47izLINKjXbSqUjvEc96BvH3eU bmkg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=3hZJ6NMs3ubMUUt5a6sb3L6EScNLVPBbBRKqq7Q6m9M=; b=G40zsizir/a5cZ1qbu4Yb1JtUikelKa836GHR4LGM3F5J2vFqjICp262rrklahguhZ tlYII4Bu86NR+qCGeLcUQTHbKQRihPLMlAb3iwNmHBFptKBwnQyyS4/AaIg8O8k/+eNg 256i6QqvyjKYqn0+LEwu74NqsT/Xo52QFN4zCoTaJZEktgHxkgGnn7E7szZ/5Yv4Uby4 I6MkwRpe0Y6+jm9uJYyKv/S2c3Z5G0gRU5qZ23u7eWdYxfAymI8R85Yn1PxlDmMC+H3e 8U0LTPR1ixVyBV0VJYlqjrHbwtSCYglpxf+cW6wb6tIPBusqc2DCCXCZWkhwZTE0Cd2r aLCw== X-Gm-Message-State: AElRT7Gis72rmkVp2JMd03+rkuE+9fvQS4ctm37jEm78IlkFalCem1NI 5Hlkl4pBdZccv4ZyHjALlMF4PQ== X-Google-Smtp-Source: AIpwx4+y9USHdW9V3pTIGT0YCzHonRAH4EDbgf9A2dUWspC8qGWrpAGHxL87n/sT1T9kmsrUfmvBkA== X-Received: by 10.80.179.230 with SMTP id t35mr3541763edd.173.1522245152837; Wed, 28 Mar 2018 06:52:32 -0700 (PDT) Received: from bidouze.vm.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id n30sm2597189edd.45.2018.03.28.06.52.31 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 28 Mar 2018 06:52:31 -0700 (PDT) Date: Wed, 28 Mar 2018 15:52:18 +0200 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Rosen Xu Cc: dev@dpdk.org, declan.doherty@intel.com, bruce.richardson@intel.com, shreyansh.jain@nxp.com, tianfei.zhang@intel.com, hao.wu@intel.com Message-ID: <20180328135218.b3umvqqmfjnto5nl@bidouze.vm.6wind.com> References: <1521553556-62982-1-git-send-email-rosen.xu@intel.com> <1522229396-17898-1-git-send-email-rosen.xu@intel.com> <1522229396-17898-5-git-send-email-rosen.xu@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1522229396-17898-5-git-send-email-rosen.xu@intel.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH v3 4/6] drivers/bus: Add Intel FPGA Bus Lib Code X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 28 Mar 2018 13:52:33 -0000 On Wed, Mar 28, 2018 at 05:29:54PM +0800, Rosen Xu wrote: > Signed-off-by: Rosen Xu > --- > drivers/bus/Makefile | 1 + > drivers/bus/ifpga/Makefile | 33 ++ > drivers/bus/ifpga/ifpga_bus.c | 562 ++++++++++++++++++++++++++++ > 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 | 140 +++++++ > drivers/bus/ifpga/rte_bus_ifpga_version.map | 8 + > 8 files changed, 938 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/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 > > 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..e611950 > --- /dev/null > +++ b/drivers/bus/ifpga/Makefile > @@ -0,0 +1,33 @@ > +# SPDX-License-Identifier: BSD-3-Clause > +# Copyright(c) 2017 Intel Corporation Copyrigth 2018 (same comment for most SPDX tags, I think I saw 2017 or a variation of it everywhere I think). > + > +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 you should probably load librte_pci. You use afterward functions (at least address comparison). I haven't tested the SHARED build, but my guess is that it would break. On that note, you haven't acted on my previous remarks to use the common PCI utilities provided. > + > +# > +# 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..092be65 > --- /dev/null > +++ b/drivers/bus/ifpga/ifpga_bus.c > @@ -0,0 +1,562 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright(c) 2010-2017 Intel Corporation > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > +#include > + > +#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; > + > +/* register a ifpga bus based driver */ > +void rte_ifpga_driver_register(struct rte_afu_driver *driver) > +{ > + RTE_VERIFY(driver); > + > + TAILQ_INSERT_TAIL(&rte_ifpga_bus.driver_list, driver, next); > +} > + > +/* un-register a fpga bus based driver */ > +void rte_ifpga_driver_unregister(struct rte_afu_driver *driver) > +{ > + TAILQ_REMOVE(&rte_ifpga_bus.driver_list, driver, next); > +} > + > +static struct rte_ifpga_device * > +ifpga_find_ifpga_dev(const struct rte_pci_addr *pci_addr) > +{ > + struct rte_ifpga_device *ifpga_dev = NULL; > + > + TAILQ_FOREACH(ifpga_dev, &rte_ifpga_bus.ifpga_list, next) { > + if (!rte_pci_addr_cmp(&ifpga_dev->pci_addr, pci_addr)) > + 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_BDF "bdf" > + IFPGA_ARG_BDF, > +#define IFPGA_ARG_PORT "port" > + IFPGA_ARG_PORT, > +#define IFPGA_ARG_PATH "path" > + IFPGA_ARG_PATH, > +#define IFPGA_ARG_UUID_HIGH "uuid_high" > + IFPGA_ARG_UUID_HIGH, > +#define IFPGA_ARG_UUID_LOW "uuid_low" > + IFPGA_ARG_UUID_LOW, > +#define IFPGA_ARG_PR_ENABLE "pr_enable" > + IFPGA_ARG_PR_ENABLE, > +#define IFPGA_ARG_DEBUG "debug" > + IFPGA_ARG_DEBUG, > + NULL > +}; So these parameters are parsed during scan afterward, taken from the PCI device that was probed. Given the kerfuffle of the VIRTUAL devtype in the EAL option, I'm not sure how the PCI bus would react with your IFPGA PCI device being probed by its blacklist operation: the devargs would be null. Do you segfault during your scan? Have you tested this? Why are those parameters parsed again here on this note? Shouldn't they be parsed by the PCI device being probed with a custome PCI driver that you would have added (and I think you provide it afterward), that would spawn the rawdev with the relevant metadata? Then you could scan this rawdev here and have all these parameters already parsed? Is there a reason that you did all this in this order? (specific initialization step for example?) > + > +/* > + * 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, > + struct rte_ifpga_device *ifpga_dev) > +{ > + struct rte_kvargs *kvlist = NULL; > + struct rte_bus *pci_bus = NULL; > + struct rte_device *dev = 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; > + int pr_enable = 1; > + int debug = 0; > + > + memset((char *)(&afu_pr_conf), 0, sizeof(struct rte_afu_pr_conf)); > + > + 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_BDF) == 1) { > + if (rte_kvargs_process(kvlist, IFPGA_ARG_BDF, > + &ifpga_get_bdf_arg, &afu_pr_conf.afu_id.pci_addr) < 0) { > + IFPGA_BUS_ERR("error to parse %s", IFPGA_ARG_BDF); > + goto end; > + } > + } else { > + IFPGA_BUS_ERR("arg %s is mandatory for ifpga bus", > + IFPGA_ARG_PATH); > + 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_PATH); > + goto end; > + } > + > + if (rte_kvargs_count(kvlist, IFPGA_ARG_PATH) == 1) { > + if (rte_kvargs_process(kvlist, IFPGA_ARG_PATH, > + &ifpga_get_string_arg, &path) < 0) { > + IFPGA_BUS_ERR("error to parse %s", > + IFPGA_ARG_PATH); > + goto end; > + } > + } else { > + IFPGA_BUS_ERR("arg %s is mandatory for ifpga bus", > + IFPGA_ARG_PATH); > + goto end; > + } > + > + if (rte_kvargs_count(kvlist, IFPGA_ARG_UUID_HIGH) == 1) { > + if (rte_kvargs_process(kvlist, IFPGA_ARG_UUID_HIGH, > + &ifpga_get_integer64_arg, &afu_pr_conf.afu_id.uuid_high) < 0) { > + IFPGA_BUS_ERR("error to parse %s", > + IFPGA_ARG_UUID_HIGH); > + goto end; > + } > + } else { > + IFPGA_BUS_ERR("arg %s is mandatory for ifpga bus", > + IFPGA_ARG_PATH); > + goto end; > + } > + > + if (rte_kvargs_count(kvlist, IFPGA_ARG_UUID_LOW) == 1) { > + if (rte_kvargs_process(kvlist, IFPGA_ARG_UUID_LOW, > + &ifpga_get_integer64_arg, &afu_pr_conf.afu_id.uuid_low) < 0) { > + IFPGA_BUS_ERR("error to parse %s", > + IFPGA_ARG_UUID_LOW); > + goto end; > + } > + } else { > + IFPGA_BUS_ERR("arg %s is mandatory for ifpga bus", > + IFPGA_ARG_PATH); > + goto end; > + } > + > + if (rte_kvargs_count(kvlist, IFPGA_ARG_PR_ENABLE) == 1) { > + if (rte_kvargs_process(kvlist, IFPGA_ARG_PR_ENABLE, > + &ifpga_get_integer32_arg, &pr_enable) < 0) { > + IFPGA_BUS_ERR("error to parse %s", > + IFPGA_ARG_UUID_HIGH); > + goto end; > + } > + } > + > + if (rte_kvargs_count(kvlist, IFPGA_ARG_DEBUG) == 1) { > + if (rte_kvargs_process(kvlist, IFPGA_ARG_DEBUG, > + &ifpga_get_integer32_arg, &debug) < 0) { > + IFPGA_BUS_ERR("error to parse %s", > + IFPGA_ARG_UUID_HIGH); > + goto end; > + } > + } > + > + if (!debug) { > + pci_bus = rte_bus_find_by_name("pci"); > + if (pci_bus == NULL) { > + IFPGA_BUS_ERR("unable to find PCI bus\n"); > + goto end; > + } > + > + dev = pci_bus->find_device(NULL, > + ifpga_pci_addr_cmp, > + &afu_pr_conf.afu_id.pci_addr); > + if (dev == NULL) { > + IFPGA_BUS_ERR("unable to find PCI device\n"); > + goto end; > + } > + } else { > + IFPGA_BUS_DEBUG("pci_addr domain : %x\n", > + afu_pr_conf.afu_id.pci_addr.domain); > + IFPGA_BUS_DEBUG("pci_addr bus : %x\n", > + afu_pr_conf.afu_id.pci_addr.bus); > + IFPGA_BUS_DEBUG("pci_addr devid : %x\n", > + afu_pr_conf.afu_id.pci_addr.devid); > + IFPGA_BUS_DEBUG("pci_addr function : %x\n", > + afu_pr_conf.afu_id.pci_addr.function); > + IFPGA_BUS_DEBUG("uuid_low : %lx\n", > + afu_pr_conf.afu_id.uuid_low); > + IFPGA_BUS_DEBUG("uuid_high : %lx\n", > + afu_pr_conf.afu_id.uuid_high); > + IFPGA_BUS_DEBUG("afu port : %x\n", > + afu_pr_conf.afu_id.port); > + } > + > + 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.pci_addr.domain = afu_pr_conf.afu_id.pci_addr.domain; > + afu_dev->id.pci_addr.bus = afu_pr_conf.afu_id.pci_addr.bus; > + afu_dev->id.pci_addr.devid = afu_pr_conf.afu_id.pci_addr.devid; > + afu_dev->id.pci_addr.function = afu_pr_conf.afu_id.pci_addr.function; Can you add a function called rte_pci_addr_cpy in librte_pci and use it instead of doing this by hand here? Others would benefit from this. > + afu_dev->id.uuid_low = afu_pr_conf.afu_id.uuid_low; > + afu_dev->id.uuid_high = afu_pr_conf.afu_id.uuid_high; > + 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; > + if (pr_enable) { > + strncpy(afu_pr_conf.bs_path, path, strlen(path)); strncpy is dangerous here, please use strlcpy or snprintf. (And your use of strncpy is dangerous, you are limitting the write to the length of the source, not to the size of the destination.) > + 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; > + } > + } > + > + 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) > +{ > + struct rte_ifpga_device *ifpga_dev; > + struct rte_devargs *devargs; > + struct rte_kvargs *kvlist = NULL; > + struct rte_pci_addr pci_addr; > + struct rte_rawdev *rawdev = NULL; > + char name[RTE_RAWDEV_NAME_MAX_LEN]; > + struct rte_afu_device *afu_dev = NULL; > + > + /* for FPGA devices we scan the devargs_list populated via cmdline */ > + TAILQ_FOREACH(devargs, &devargs_list, next) { > + if (devargs->bus != &rte_ifpga_bus.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_BDF) == 1) { > + if (rte_kvargs_process(kvlist, IFPGA_ARG_BDF, > + &ifpga_get_bdf_arg, &pci_addr) < 0) { > + IFPGA_BUS_ERR("error to parse %s", > + IFPGA_ARG_BDF); > + goto end; > + } > + } else { > + IFPGA_BUS_ERR("arg %s is mandatory for ifpga bus", > + IFPGA_ARG_PATH); > + goto end; > + } > + > + memset(name, 0, sizeof(name)); > + snprintf(name, RTE_RAWDEV_NAME_MAX_LEN, "IFPGA:%x:%x:%x", > + pci_addr.bus, pci_addr.devid, pci_addr.function); > + > + rawdev = rte_rawdev_pmd_get_named_dev(name); > + if (!rawdev) > + goto end; > + > + if (ifpga_find_ifpga_dev(&pci_addr)) > + goto end; > + > + ifpga_dev = calloc(1, sizeof(*ifpga_dev)); > + if (!ifpga_dev) > + goto end; > + > + ifpga_dev->pci_addr.domain = pci_addr.domain; > + ifpga_dev->pci_addr.bus = pci_addr.bus; > + ifpga_dev->pci_addr.devid = pci_addr.devid; > + ifpga_dev->pci_addr.function = pci_addr.function; Again, PCI copy would be nice instead of doing it by hand. > + ifpga_dev->rdev = rawdev; > + TAILQ_INIT(&ifpga_dev->afu_list); > + > + TAILQ_INSERT_TAIL(&rte_ifpga_bus.ifpga_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) > + rte_kvargs_free(kvlist); > + > + return 0; > +} > + > +static int > +ifpga_probe_one_driver(struct rte_afu_driver *drv, > + struct rte_afu_device *afu_dev) > +{ > + int ret; > + > + if ((drv->id.pci_addr.bus == afu_dev->id.pci_addr.bus) && > + (drv->id.pci_addr.devid == afu_dev->id.pci_addr.devid) && > + (drv->id.pci_addr.function == afu_dev->id.pci_addr.function) && There is the rte_pci_addr_cmp function that should be used here instead. Additionally, you haven't responded to my comment about ignoring the Domain part of the address. Why? Is there a reason for this? Is the domain irrelevant? If so, it should be documented. If not, please use the relevant function instead. > + (drv->id.uuid_low == afu_dev->id.uuid_low) && > + (drv->id.uuid_high == afu_dev->id.uuid_high) && > + (drv->id.port == afu_dev->id.port)) { > + > + afu_dev->driver = drv; > + > + /* call the driver probe() function */ > + ret = drv->probe(afu_dev); > + if (ret) > + afu_dev->driver = NULL; > + return ret; > + } > + > + /* return positive value if driver doesn't support this device */ > + return 1; > +} > + > +static int > +ifpga_probe_all_drivers(struct rte_afu_device *afu_dev) > +{ > + const char *name; > + 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; > + > + name = rte_ifpga_device_name(afu_dev); > + IFPGA_BUS_DEBUG("Search driver %s to probe device %s\n", name, > + rte_ifpga_device_name(afu_dev)); > + > + 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 One should read IFPGA bus here I guess. > + * 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; > +} > + > +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 = container_of(afu_dev->device.driver, > + const struct rte_afu_driver, > + driver); No RTE_DRV_TO_AFU_CONST? It would be easier to read. > + 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); > + > + TAILQ_REMOVE(&devargs_list, devargs, next); > + > + free(devargs->args); > + free(devargs); > + 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, &rte_ifpga_bus.ifpga_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) > +{ > + struct rte_afu_driver **out = addr; > + struct rte_afu_driver *driver = NULL; > + > + TAILQ_FOREACH(driver, &rte_ifpga_bus.driver_list, next) { > + if (strncmp(driver->driver.name, name, > + strlen(driver->driver.name)) == 0) > + break; > + if (driver->driver.alias && > + strncmp(driver->driver.alias, name, > + strlen(driver->driver.alias)) == 0) > + break; > + } > + if (driver != NULL && > + addr != NULL) > + *out = driver; > + return driver == NULL; > +} > + > +struct rte_ifpga_bus rte_ifpga_bus = { > + .bus = { > + .scan = rte_ifpga_scan, > + .probe = rte_ifpga_probe, > + .find_device = rte_ifpga_find_device, Wrong indentation (2 tabs instead of one). > + .plug = rte_ifpga_plug, > + .unplug = rte_ifpga_unplug, > + .parse = rte_ifpga_parse, > + }, > + .ifpga_list = TAILQ_HEAD_INITIALIZER(rte_ifpga_bus.ifpga_list), > + .driver_list = TAILQ_HEAD_INITIALIZER(rte_ifpga_bus.driver_list), Why are those lists part of the bus definition? Can you do the same as the vdev bus and have them statically declared in your source file only? Is there a reason you want them part of the bus, do you need to access it somewhere? > +}; > + > +RTE_REGISTER_BUS(IFPGA_BUS_NAME, rte_ifpga_bus.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..03a8d48 > --- /dev/null > +++ b/drivers/bus/ifpga/ifpga_common.c > @@ -0,0 +1,141 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright(c) 2010-2017 Intel Corporation > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > +#include > + > +#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; > + > +} Missing newline here. > +int ifpga_get_bdf_arg(const char *key __rte_unused, > + const char *value, void *extra_args) > +{ > +#define MAX_PATH_LEN 1024 > + struct rte_pci_addr *addr; > + int num[4]; > + char str[MAX_PATH_LEN]; > + int i, j; > + > + if (!value || !extra_args) > + return -EINVAL; > + > + addr = (struct rte_pci_addr *)extra_args; > + strcpy(str, value); > + memset(num, 0, 4 * sizeof(num[0])); > + i = strlen(str) - 1; > + j = 3; > + while (i > 0 && j >= 0) { > + while ((str[i - 1] != ':' && str[i - 1] != '.') && i > 0) > + i--; > + num[j--] = ifpga_get_unsigned_long(&str[i], 16); > + i--; > + if (i >= 0) > + str[i] = '\0'; > + } > + addr->domain = num[0]; > + addr->bus = num[1]; > + addr->devid = num[2]; > + addr->function = num[3]; > + printf("[%s]: bdf %04d:%02d:%02d.%02d\n", > + __func__, > + addr->domain, > + addr->bus, > + addr->devid, > + addr->function); > + > + return 0; > +} Use rte_pci_addr_parse instead of rewriting it please. > + > +int ifpga_afu_id_cmp(const struct rte_afu_id *afu_id0, > + const struct rte_afu_id *afu_id1) > +{ > + if ((afu_id0->pci_addr.bus == afu_id1->pci_addr.bus) && > + (afu_id0->pci_addr.devid == afu_id1->pci_addr.devid) && > + (afu_id0->pci_addr.function == afu_id1->pci_addr.function) && > + (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; > +} You used the exact same check in the ifpga_bus.c file, why not call this function instead? > +int ifpga_pci_addr_cmp(const struct rte_device *dev, > + const void *_pci_addr) > +{ > + struct rte_pci_device *pdev; > + const struct rte_pci_addr *paddr = _pci_addr; > + > + pdev = RTE_DEV_TO_PCI(*(struct rte_device **)(void *)&dev); > + return rte_eal_compare_pci_addr(&pdev->addr, paddr); This function is deprecated (I should mark is as so to forbid its use). Use rte_pci_addr_cmp() instead. > +} > diff --git a/drivers/bus/ifpga/ifpga_common.h b/drivers/bus/ifpga/ifpga_common.h > new file mode 100644 > index 0000000..48a5012 > --- /dev/null > +++ b/drivers/bus/ifpga/ifpga_common.h > @@ -0,0 +1,22 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright(c) 2010-2017 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_get_bdf_arg(const char *key __rte_unused, > + const char *value, void *extra_args); > +int ifpga_afu_id_cmp(const struct rte_afu_id *afu_id0, > + const struct rte_afu_id *afu_id1); > +int ifpga_pci_addr_cmp(const struct rte_device *dev, > + const void *_pci_addr); > + > +#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..e3f4849 > --- /dev/null > +++ b/drivers/bus/ifpga/ifpga_logs.h > @@ -0,0 +1,31 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright(c) 2010-2017 Intel Corporation > + */ > + > +#ifndef _IFPGA_LOGS_H_ > +#define _IFPGA_LOGS_H_ > + > +#include > + > +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/rte_bus_ifpga.h b/drivers/bus/ifpga/rte_bus_ifpga.h > new file mode 100644 > index 0000000..7290149 > --- /dev/null > +++ b/drivers/bus/ifpga/rte_bus_ifpga.h > @@ -0,0 +1,140 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright(c) 2010-2017 Intel Corporation > + */ > + > +#ifndef _RTE_BUS_IFPGA_H_ > +#define _RTE_BUS_IFPGA_H_ > + > +/** > + * @file > + * > + * RTE PCI Bus Interface > + */ > + > +#ifdef __cplusplus > +extern "C" { > +#endif > + > +#include > +#include > + > +/** 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); Is there a reason for you to expose your lists publicly? These symbols should be made private to your bus unless necessary. > + > +#define IFPGA_BUS_BITSTREAM_PATH_MAX_LEN 256 > + > +/** > + * 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; You use a symbol from librte_pci, without linking it. > + 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]; > +}; > + > +#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. > + */ > +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. > + */ > +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. */ > + struct rte_afu_id id; /**< AFU id 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; > + > +void rte_ifpga_driver_register(struct rte_afu_driver *driver); > +void rte_ifpga_driver_unregister(struct rte_afu_driver *driver); > + > + > +#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; Wrong indentation. It should be one tabulation, not 4 spaces. > + > + local: *; > +}; > -- > 1.8.3.1 > -- Gaëtan Rivet 6WIND