[dpdk-dev,V2,3/5] Add Intel FPGA BUS Lib Code

Message ID 1521618694-140757-4-git-send-email-rosen.xu@intel.com (mailing list archive)
State Superseded, archived
Headers

Checks

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

Commit Message

Xu, Rosen March 21, 2018, 7:51 a.m. UTC
  Signed-off-by: Rosen Xu <rosen.xu@intel.com>
---
 drivers/bus/ifpga/Makefile                  |  64 ++++
 drivers/bus/ifpga/ifpga_bus.c               | 573 ++++++++++++++++++++++++++++
 drivers/bus/ifpga/ifpga_common.c            | 154 ++++++++
 drivers/bus/ifpga/ifpga_common.h            |  25 ++
 drivers/bus/ifpga/ifpga_logs.h              |  32 ++
 drivers/bus/ifpga/rte_bus_ifpga.h           | 141 +++++++
 drivers/bus/ifpga/rte_bus_ifpga_version.map |   8 +
 7 files changed, 997 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
  

Comments

Shreyansh Jain March 21, 2018, 9:28 a.m. UTC | #1
Hello Rosen,

inlined are some comments from a quick look...

On Wed, Mar 21, 2018 at 1:21 PM, Rosen Xu <rosen.xu@intel.com> wrote:
> Signed-off-by: Rosen Xu <rosen.xu@intel.com>
> ---
>  drivers/bus/ifpga/Makefile                  |  64 ++++
>  drivers/bus/ifpga/ifpga_bus.c               | 573 ++++++++++++++++++++++++++++
>  drivers/bus/ifpga/ifpga_common.c            | 154 ++++++++
>  drivers/bus/ifpga/ifpga_common.h            |  25 ++
>  drivers/bus/ifpga/ifpga_logs.h              |  32 ++
>  drivers/bus/ifpga/rte_bus_ifpga.h           | 141 +++++++
>  drivers/bus/ifpga/rte_bus_ifpga_version.map |   8 +
>  7 files changed, 997 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/ifpga/Makefile b/drivers/bus/ifpga/Makefile
> new file mode 100644
> index 0000000..c71f186
> --- /dev/null
> +++ b/drivers/bus/ifpga/Makefile
> @@ -0,0 +1,64 @@
> +#   BSD LICENSE
> +#
> +#   Copyright(c) 2010-2017 Intel Corporation. All rights reserved.
> +#   All rights reserved.
> +#
> +#   Redistribution and use in source and binary forms, with or without
> +#   modification, are permitted provided that the following conditions
> +#   are met:
> +#
> +#     * Redistributions of source code must retain the above copyright
> +#       notice, this list of conditions and the following disclaimer.
> +#     * Redistributions in binary form must reproduce the above copyright
> +#       notice, this list of conditions and the following disclaimer in
> +#       the documentation and/or other materials provided with the
> +#       distribution.
> +#     * Neither the name of Intel Corporation nor the names of its
> +#       contributors may be used to endorse or promote products derived
> +#       from this software without specific prior written permission.
> +#
> +#   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> +#   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> +#   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> +#   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> +#   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> +#   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> +#   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> +#   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> +#   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> +#   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> +#   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

Did you get a chance to go through the comment in RFC?
I think you should replace the boilerplate with SPDX tags.

> +
> +include $(RTE_SDK)/mk/rte.vars.mk
> +
> +#
> +# library name
> +#
> +LIB = librte_bus_ifpga.a
> +LIBABIVER := 1
> +EXPORT_MAP := rte_bus_ifpga_version.map
> +
> +ifeq ($(CONFIG_RTE_LIBRTE_DPAA2_DEBUG_INIT),y)

I think there was a similar comment on RFC - "...DPAA2..." macro is a
copy-paste error.

> +CFLAGS += -O0 -g
> +CFLAGS += "-Wno-error"
> +else
> +CFLAGS += -O3
> +CFLAGS += $(WERROR_FLAGS)
> +endif
> +
> +CFLAGS += -I$(RTE_SDK)/drivers/bus/ifpga
> +CFLAGS += -I$(RTE_SDK)/drivers/bus/pci
> +CFLAGS += -I$(RTE_SDK)/lib/librte_eal/linuxapp/eal
> +CFLAGS += -I$(RTE_SDK)/lib/librte_eal/common
> +#CFLAGS += -I$(RTE_SDK)/lib/librte_rawdev
> +#LDLIBS += -lrte_eal -lrte_mbuf -lrte_mempool -lrte_ring -lrte_rawdev

If you don't need these lines, don't keep them. That is ok until RFC,
but not in formal patch.

> +LDLIBS += -lrte_eal -lrte_mbuf -lrte_mempool -lrte_ring
> +#LDLIBS += -lrte_ethdev
> +
> +VPATH += $(SRCDIR)/base
> +
> +SRCS-y += \
> +        ifpga_bus.c \
> +        ifpga_common.c
> +
> +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..ff72b74
> --- /dev/null
> +++ b/drivers/bus/ifpga/ifpga_bus.c
> @@ -0,0 +1,573 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2010-2014 Intel Corporation.
> + * Copyright 2013-2014 6WIND S.A.

Are you sure of the above copyright? I think this is a new file. Maybe
your internal HW routines can have old copyrights.
Just a trivial comment, though.

> + */
> +
> +#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_pci.h>
> +#include <rte_bus_pci.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;
> +
> +/*register a ifpga bus based driver */

Comments have ' ' <space> after the '/*'.
Maybe you can refer [1] once.

[1] https://dpdk.org/doc/guides/contributing/coding_style.html#coding-style

> +void rte_ifpga_driver_register(struct rte_afu_driver *driver)
> +{
> +       RTE_VERIFY(driver);
> +
> +       TAILQ_INSERT_TAIL(&rte_ifpga_bus.driver_list, driver, next);
> +}
> +

[..snip..]

> diff --git a/drivers/bus/ifpga/ifpga_logs.h b/drivers/bus/ifpga/ifpga_logs.h
> new file mode 100644
> index 0000000..36b9b3f
> --- /dev/null
> +++ b/drivers/bus/ifpga/ifpga_logs.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2010-2015 Intel Corporation.
> + * Copyright 2013-2014 6WIND S.A.
> + */
> +
> +#ifndef _IFPGA_BUS_LOGS_H_
> +#define _IFPGA_BUS_LOGS_H_

Ideally this is name of the file, which is IFPFA_LOGS. But,
technically this is not an issue.

> +
> +#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)
> +

I noticed that at some places where you have used the above macros,
you have added '\n' in the call. It would lead to double '\n' as your
IFPGA_LOG and IFPGA_BUS_LOG already have one '\n'.

> +#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_ */

[..snip..]

> +#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..e2aa7da
> --- /dev/null
> +++ b/drivers/bus/ifpga/rte_bus_ifpga_version.map
> @@ -0,0 +1,8 @@
> +DPDK_17.11 {

Should be DPDK 18.05

> +       global:
> +
> +    rte_ifpga_driver_register;
> +    rte_ifpga_driver_unregister;

And indentation is incorrect.

> +
> +       local: *;
> +};
> --
> 1.8.3.1
>
  
Gaëtan Rivet March 21, 2018, 10:20 a.m. UTC | #2
Hi,

I have had issues compiling a few things here, have you checked
build status before submitting?

On Wed, Mar 21, 2018 at 03:51:32PM +0800, Rosen Xu wrote:
> Signed-off-by: Rosen Xu <rosen.xu@intel.com>
> ---
>  drivers/bus/ifpga/Makefile                  |  64 ++++
>  drivers/bus/ifpga/ifpga_bus.c               | 573 ++++++++++++++++++++++++++++
>  drivers/bus/ifpga/ifpga_common.c            | 154 ++++++++
>  drivers/bus/ifpga/ifpga_common.h            |  25 ++
>  drivers/bus/ifpga/ifpga_logs.h              |  32 ++
>  drivers/bus/ifpga/rte_bus_ifpga.h           | 141 +++++++
>  drivers/bus/ifpga/rte_bus_ifpga_version.map |   8 +
>  7 files changed, 997 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/ifpga/Makefile b/drivers/bus/ifpga/Makefile
> new file mode 100644
> index 0000000..c71f186
> --- /dev/null
> +++ b/drivers/bus/ifpga/Makefile
> @@ -0,0 +1,64 @@
> +#   BSD LICENSE
> +#
> +#   Copyright(c) 2010-2017 Intel Corporation. All rights reserved.
> +#   All rights reserved.
> +#
> +#   Redistribution and use in source and binary forms, with or without
> +#   modification, are permitted provided that the following conditions
> +#   are met:
> +#
> +#     * Redistributions of source code must retain the above copyright
> +#       notice, this list of conditions and the following disclaimer.
> +#     * Redistributions in binary form must reproduce the above copyright
> +#       notice, this list of conditions and the following disclaimer in
> +#       the documentation and/or other materials provided with the
> +#       distribution.
> +#     * Neither the name of Intel Corporation nor the names of its
> +#       contributors may be used to endorse or promote products derived
> +#       from this software without specific prior written permission.
> +#
> +#   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> +#   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> +#   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> +#   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> +#   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> +#   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> +#   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> +#   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> +#   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> +#   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> +#   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

Please use SPDX tags here instead.

> +
> +include $(RTE_SDK)/mk/rte.vars.mk
> +
> +#
> +# library name
> +#
> +LIB = librte_bus_ifpga.a
> +LIBABIVER := 1
> +EXPORT_MAP := rte_bus_ifpga_version.map
> +
> +ifeq ($(CONFIG_RTE_LIBRTE_DPAA2_DEBUG_INIT),y)

C/C issue, you probably do not want to rely on a DPAA option.

> +CFLAGS += -O0 -g
> +CFLAGS += "-Wno-error"
> +else
> +CFLAGS += -O3
> +CFLAGS += $(WERROR_FLAGS)
> +endif
> +
> +CFLAGS += -I$(RTE_SDK)/drivers/bus/ifpga
> +CFLAGS += -I$(RTE_SDK)/drivers/bus/pci

It is not okay to directly include PCI elements.
Either you use the public PCI API, or you make the elements you need
public (with proper versioning), or you come up with a different scheme.

But you cannot directly include the PCI bus.

> +CFLAGS += -I$(RTE_SDK)/lib/librte_eal/linuxapp/eal
> +CFLAGS += -I$(RTE_SDK)/lib/librte_eal/common
> +#CFLAGS += -I$(RTE_SDK)/lib/librte_rawdev
> +#LDLIBS += -lrte_eal -lrte_mbuf -lrte_mempool -lrte_ring -lrte_rawdev

Please clean-up those commented lines.

> +LDLIBS += -lrte_eal -lrte_mbuf -lrte_mempool -lrte_ring
> +#LDLIBS += -lrte_ethdev

Same here.

> +
> +VPATH += $(SRCDIR)/base
> +
> +SRCS-y += \
> +        ifpga_bus.c \
> +        ifpga_common.c
> +
> +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..ff72b74
> --- /dev/null
> +++ b/drivers/bus/ifpga/ifpga_bus.c
> @@ -0,0 +1,573 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2010-2014 Intel Corporation.
> + * Copyright 2013-2014 6WIND S.A.

Copy-paste error.

> + */
> +
> +#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_pci.h>
> +#include <rte_bus_pci.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;
> +
> +/*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 (!ifpga_pci_addr_cmp_1(&ifpga_dev->pci_addr, pci_addr))
> +			return ifpga_dev;
> +	}
> +	return NULL;
> +}
> +
> +static struct rte_afu_device *
> +ifpga_find_afu_dev(const struct rte_afu_id *afu_id)
> +{
> +	struct rte_afu_device *afu_dev = NULL;
> +
> +	TAILQ_FOREACH(afu_dev, &rte_ifpga_bus.afu_list, next) {
> +		if (!ifpga_afu_id_cmp(&afu_dev->id, afu_id))
> +			return afu_dev;
> +	}
> +	return NULL;
> +}
> +
> +static const char *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
> +};
> +
> +/*
> + * Scan the content of the FPGA bus, and the devices in the devices
> + * list
> + */

So you seem to scan your bus by reading parameters
given to the --ifpga EAL option.

Can you justify why you cannot use the PCI bus, have your FPGA be probed
by a PCI driver, that would take those parameters as driver parameters,
and spawn raw devices (one per bitstream) as needed as a result?

I see no reason this is not feasible. Unless you duly justify this
approach, it seems unacceptable to me. You are subverting generic EAL
code to bend things to your approach, without clear rationale.

Best regards,
  
Bruce Richardson March 21, 2018, 1:35 p.m. UTC | #3
On Wed, Mar 21, 2018 at 11:20:25AM +0100, Gaëtan Rivet wrote:
> Hi,
> 
> I have had issues compiling a few things here, have you checked
> build status before submitting?
> 
> On Wed, Mar 21, 2018 at 03:51:32PM +0800, Rosen Xu wrote:
> > Signed-off-by: Rosen Xu <rosen.xu@intel.com>
> > ---
<snip>
> +
> > +/*
> > + * Scan the content of the FPGA bus, and the devices in the devices
> > + * list
> > + */
> 
> So you seem to scan your bus by reading parameters
> given to the --ifpga EAL option.
> 
> Can you justify why you cannot use the PCI bus, have your FPGA be probed
> by a PCI driver, that would take those parameters as driver parameters,
> and spawn raw devices (one per bitstream) as needed as a result?
> 
> I see no reason this is not feasible. Unless you duly justify this
> approach, it seems unacceptable to me. You are subverting generic EAL
> code to bend things to your approach, without clear rationale.
> 

While I agree with the comments in other emails about avoiding
special-cases in the code that makes things not-scalable, I would take the
view that using a bus-type is the correct choice for this. While you could
have a single device that creates other devices, that is also true for all
other buses as well.  [Furthermore, I think it is incorrect assume that all
devices on the FPGA bus would be raw devices, it's entirely possible to
have cryptodevs, bbdevs or compress devs implemented in the AFUs].

Consider what a bus driver provides: it's a generic mechanism for scanning
for devices - which all use a common connection method - for DPDK use, and
mapping device drivers to those devices. For an FPGA device which presents
multiple AFUs, this seems to be exactly what is required - a device driver
to scan for devices and present them to DPDK. The FPGA bus driver will have
to check each AFU and match it against the set of registered AFU device
drivers to ensure that the crypto AFU gets the cryptodev driver, etc.

Logically, therefore, it is a bus - which just happens to be a sub-bus of
PCI, i.e. presented as a PCI device. Consider also that it may be possible
or even desirable, to use blacklisting and whitelisting for those AFU
devices so that some AFUs could be used by one app, while others by
another. If we just have a single PCI device, I think we'll find ourselves
duplicating a lot of bus-related functionality inside the driver in that
case.

Regards,
/Bruce
  
Shreyansh Jain March 21, 2018, 2:02 p.m. UTC | #4
On Wed, Mar 21, 2018 at 7:05 PM, Bruce Richardson
<bruce.richardson@intel.com> wrote:
> On Wed, Mar 21, 2018 at 11:20:25AM +0100, Gaėtan Rivet wrote:
>> Hi,
>>
>> I have had issues compiling a few things here, have you checked
>> build status before submitting?
>>
>> On Wed, Mar 21, 2018 at 03:51:32PM +0800, Rosen Xu wrote:
>> > Signed-off-by: Rosen Xu <rosen.xu@intel.com>
>> > ---
> <snip>
>> +
>> > +/*
>> > + * Scan the content of the FPGA bus, and the devices in the devices
>> > + * list
>> > + */
>>
>> So you seem to scan your bus by reading parameters
>> given to the --ifpga EAL option.
>>
>> Can you justify why you cannot use the PCI bus, have your FPGA be probed
>> by a PCI driver, that would take those parameters as driver parameters,
>> and spawn raw devices (one per bitstream) as needed as a result?
>>
>> I see no reason this is not feasible. Unless you duly justify this
>> approach, it seems unacceptable to me. You are subverting generic EAL
>> code to bend things to your approach, without clear rationale.
>>
>
> While I agree with the comments in other emails about avoiding
> special-cases in the code that makes things not-scalable, I would take the
> view that using a bus-type is the correct choice for this. While you could
> have a single device that creates other devices, that is also true for all
> other buses as well.  [Furthermore, I think it is incorrect assume that all
> devices on the FPGA bus would be raw devices, it's entirely possible to
> have cryptodevs, bbdevs or compress devs implemented in the AFUs].
>
> Consider what a bus driver provides: it's a generic mechanism for scanning
> for devices - which all use a common connection method - for DPDK use, and
> mapping device drivers to those devices. For an FPGA device which presents
> multiple AFUs, this seems to be exactly what is required - a device driver
> to scan for devices and present them to DPDK. The FPGA bus driver will have
> to check each AFU and match it against the set of registered AFU device
> drivers to ensure that the crypto AFU gets the cryptodev driver, etc.

In my opinion, its' not a problem that a heirarchichal bus model like
this is implemented in DPDK. That's being creative :)

But, here the issue is that same work can be done by a driver which
can create devices of multiple types (bbdev, cryptodev, ethdev etc)
and then use hotplugging (assuming that is supported by relevant
driver) over respective bus. I don't think we necessarily need a bus
for that. Is there some technical hurdle in following that model? Or,
is there some cases which cannot be handled by not having a dependency
bus?

Then, there is another problem of modifying the rte_bus code so that
this bus initialization can be delayed. That is something which is not
right in my opinion. That puts a fixed constraint on the EAL to look
for a bus as being special (like vdev). If that is a blocking problem,
it too needs to be solved - maybe separately.

Frankly, its not about 'can't-do-this' - but, I think this does
warrant a good thought before being mainlined.

>
> Logically, therefore, it is a bus - which just happens to be a sub-bus of
> PCI, i.e. presented as a PCI device. Consider also that it may be possible
> or even desirable, to use blacklisting and whitelisting for those AFU
> devices so that some AFUs could be used by one app, while others by
> another. If we just have a single PCI device, I think we'll find ourselves
> duplicating a lot of bus-related functionality inside the driver in that
> case.
>
> Regards,
> /Bruce
  
Xu, Rosen March 21, 2018, 2:06 p.m. UTC | #5
-----Original Message-----
From: Richardson, Bruce 
Sent: Wednesday, March 21, 2018 21:35
To: Gaëtan Rivet <gaetan.rivet@6wind.com>
Cc: Xu, Rosen <rosen.xu@intel.com>; dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>; shreyansh.jain@nxp.com; Zhang, Tianfei <tianfei.zhang@intel.com>; Wu, Hao <hao.wu@intel.com>
Subject: Re: [PATCH V2 3/5] Add Intel FPGA BUS Lib Code

On Wed, Mar 21, 2018 at 11:20:25AM +0100, Gaëtan Rivet wrote:
> > Hi,
> > 
> > I have had issues compiling a few things here, have you checked build 
> > status before submitting?
> > 
> > On Wed, Mar 21, 2018 at 03:51:32PM +0800, Rosen Xu wrote:
> > > Signed-off-by: Rosen Xu <rosen.xu@intel.com>
> > > ---
<snip>
> > +
> > > +/*
> > > + * Scan the content of the FPGA bus, and the devices in the devices
> > > + * list
> > > + */
> > 
> > So you seem to scan your bus by reading parameters given to the 
> > --ifpga EAL option.
> > 
> > Can you justify why you cannot use the PCI bus, have your FPGA be 
> > probed by a PCI driver, that would take those parameters as driver 
> > parameters, and spawn raw devices (one per bitstream) as needed as a result?
> > 
> > I see no reason this is not feasible. Unless you duly justify this 
> > approach, it seems unacceptable to me. You are subverting generic EAL 
> > code to bend things to your approach, without clear rationale.
> > 
> While I agree with the comments in other emails about avoiding special-cases in the code 
>  that makes things not-scalable, I would take the view that using a bus-type is the correct 
> choice for this. While you could have a single device that creates other devices, that is also
> true for all other buses as well.  [Furthermore, I think it is incorrect assume that all devices 
> on the FPGA bus would be raw devices, it's entirely possible to have cryptodevs, bbdevs or compress devs implemented in the AFUs].
>
> Consider what a bus driver provides: it's a generic mechanism for scanning for 
> devices - which all use a common connection method - for DPDK use, and 
> mapping device drivers to those devices. For an FPGA device which presents 
> multiple AFUs, this seems to be exactly what is required - a device driver to 
> scan for devices and present them to DPDK. The FPGA bus driver will have 
> to check each AFU and match it against the set of registered AFU device 
> drivers to ensure that the crypto AFU gets the cryptodev driver, etc.
>
> Logically, therefore, it is a bus - which just happens to be a sub-bus of 
> PCI, i.e. presented as a PCI device. Consider also that it may be possible 
> or even desirable, to use blacklisting and whitelisting for those AFU 
> devices so that some AFUs could be used by one app, while others by 
> another. If we just have a single PCI device, I think we'll find ourselves 
> duplicating a lot of bus-related functionality inside the driver in that case.

In our FPGA Usage Framework, each FPAG Bitstream is divided 2 parts, 
one part is only one Blue AFU, another part consist of many Green AFUs.

Blue AFU includes PCIe Interface and FPGA PR Unit, Blue AFU is fixed after
OS Initialization, because if we change Blue AFU, the PCIe Interface needed to be 
rescanned and the OS need to reboot.

Green AFUs can be dynamically PR by different Users.

The benefit of this FPGA Architecture is that we can dynamically change Green AFUs,
but OS don't need to rescan the FPGA PCIe Interface. For Cloud Scenario the FPGA 
Device can be viewed a common resource pools such as DDR Memory, and it can be easily
assigned to different users at some time. For TelCom/NFV Scenario, we can easily
upgrade the Acceleration AFU but the server don't need to reboot.

For Software Usage, there many FPGA Device in one system, some have the same
Green AFUs and some have different Green AFUs, same Green AFUs means same Acceleration, 
and it will use same driver. So we want to involve a new bus,
which can easily bind the same Green AFUs with its driver.


Regards,
/Bruce
  
Gaëtan Rivet March 21, 2018, 2:14 p.m. UTC | #6
Hi Bruce,

On Wed, Mar 21, 2018 at 01:35:09PM +0000, Bruce Richardson wrote:
> On Wed, Mar 21, 2018 at 11:20:25AM +0100, Gaëtan Rivet wrote:
> > Hi,
> > 
> > I have had issues compiling a few things here, have you checked
> > build status before submitting?
> > 
> > On Wed, Mar 21, 2018 at 03:51:32PM +0800, Rosen Xu wrote:
> > > Signed-off-by: Rosen Xu <rosen.xu@intel.com>
> > > ---
> <snip>
> > +
> > > +/*
> > > + * Scan the content of the FPGA bus, and the devices in the devices
> > > + * list
> > > + */
> > 
> > So you seem to scan your bus by reading parameters
> > given to the --ifpga EAL option.
> > 
> > Can you justify why you cannot use the PCI bus, have your FPGA be probed
> > by a PCI driver, that would take those parameters as driver parameters,
> > and spawn raw devices (one per bitstream) as needed as a result?
> > 
> > I see no reason this is not feasible. Unless you duly justify this
> > approach, it seems unacceptable to me. You are subverting generic EAL
> > code to bend things to your approach, without clear rationale.
> > 
> 
> While I agree with the comments in other emails about avoiding
> special-cases in the code that makes things not-scalable, I would take the
> view that using a bus-type is the correct choice for this. While you could
> have a single device that creates other devices, that is also true for all
> other buses as well.  [Furthermore, I think it is incorrect assume that all
> devices on the FPGA bus would be raw devices, it's entirely possible to
> have cryptodevs, bbdevs or compress devs implemented in the AFUs].
> 
> Consider what a bus driver provides: it's a generic mechanism for scanning
> for devices - which all use a common connection method - for DPDK use, and
> mapping device drivers to those devices. For an FPGA device which presents
> multiple AFUs, this seems to be exactly what is required - a device driver
> to scan for devices and present them to DPDK. The FPGA bus driver will have
> to check each AFU and match it against the set of registered AFU device
> drivers to ensure that the crypto AFU gets the cryptodev driver, etc.
> 
> Logically, therefore, it is a bus - which just happens to be a sub-bus of
> PCI, i.e. presented as a PCI device. Consider also that it may be possible
> or even desirable, to use blacklisting and whitelisting for those AFU
> devices so that some AFUs could be used by one app, while others by
> another. If we just have a single PCI device, I think we'll find ourselves
> duplicating a lot of bus-related functionality inside the driver in that
> case.
> 
> Regards,
> /Bruce

It makes sense indeed if you need to specialize several drivers specific
to AFU mappings.

So, taking the bus approach, it seems we almost all agree that the
current probe is not good enough.

I think you will run into issues if you registered your bus upon probing
a PCI driver. Instead, would it be possible to:

  * not add the EAL option --ifpga.
  * register the ifpga bus like any other.
  * have a PCI driver that hotplug an IFPGA device, triggering a
    scan and probe on the ifpga bus using the PCI device parameters.
    Essentially you would find there the parameters that could be
    given previously to the --ifpga option.

There should not be any EAL modification necessary.

The only downside I see would be that having both IFPGA device (for
triggering bus ops) and afterward using AFU devices on this bus could be
confusing. However, it seems safer and still cleaner overall.
  
Gaëtan Rivet March 21, 2018, 2:31 p.m. UTC | #7
On Wed, Mar 21, 2018 at 03:14:05PM +0100, Gaëtan Rivet wrote:
> Hi Bruce,
> 
> On Wed, Mar 21, 2018 at 01:35:09PM +0000, Bruce Richardson wrote:
> > On Wed, Mar 21, 2018 at 11:20:25AM +0100, Gaëtan Rivet wrote:
> > > Hi,
> > > 
> > > I have had issues compiling a few things here, have you checked
> > > build status before submitting?
> > > 
> > > On Wed, Mar 21, 2018 at 03:51:32PM +0800, Rosen Xu wrote:
> > > > Signed-off-by: Rosen Xu <rosen.xu@intel.com>
> > > > ---
> > <snip>
> > > +
> > > > +/*
> > > > + * Scan the content of the FPGA bus, and the devices in the devices
> > > > + * list
> > > > + */
> > > 
> > > So you seem to scan your bus by reading parameters
> > > given to the --ifpga EAL option.
> > > 
> > > Can you justify why you cannot use the PCI bus, have your FPGA be probed
> > > by a PCI driver, that would take those parameters as driver parameters,
> > > and spawn raw devices (one per bitstream) as needed as a result?
> > > 
> > > I see no reason this is not feasible. Unless you duly justify this
> > > approach, it seems unacceptable to me. You are subverting generic EAL
> > > code to bend things to your approach, without clear rationale.
> > > 
> > 
> > While I agree with the comments in other emails about avoiding
> > special-cases in the code that makes things not-scalable, I would take the
> > view that using a bus-type is the correct choice for this. While you could
> > have a single device that creates other devices, that is also true for all
> > other buses as well.  [Furthermore, I think it is incorrect assume that all
> > devices on the FPGA bus would be raw devices, it's entirely possible to
> > have cryptodevs, bbdevs or compress devs implemented in the AFUs].
> > 
> > Consider what a bus driver provides: it's a generic mechanism for scanning
> > for devices - which all use a common connection method - for DPDK use, and
> > mapping device drivers to those devices. For an FPGA device which presents
> > multiple AFUs, this seems to be exactly what is required - a device driver
> > to scan for devices and present them to DPDK. The FPGA bus driver will have
> > to check each AFU and match it against the set of registered AFU device
> > drivers to ensure that the crypto AFU gets the cryptodev driver, etc.
> > 
> > Logically, therefore, it is a bus - which just happens to be a sub-bus of
> > PCI, i.e. presented as a PCI device. Consider also that it may be possible
> > or even desirable, to use blacklisting and whitelisting for those AFU
> > devices so that some AFUs could be used by one app, while others by
> > another. If we just have a single PCI device, I think we'll find ourselves
> > duplicating a lot of bus-related functionality inside the driver in that
> > case.
> > 
> > Regards,
> > /Bruce
> 
> It makes sense indeed if you need to specialize several drivers specific
> to AFU mappings.
> 
> So, taking the bus approach, it seems we almost all agree that the
> current probe is not good enough.
> 
> I think you will run into issues if you registered your bus upon probing
> a PCI driver. Instead, would it be possible to:
> 
>   * not add the EAL option --ifpga.
>   * register the ifpga bus like any other.
>   * have a PCI driver that hotplug an IFPGA device, triggering a
>     scan and probe on the ifpga bus using the PCI device parameters.
>     Essentially you would find there the parameters that could be
>     given previously to the --ifpga option.
> 
> There should not be any EAL modification necessary.
> 
> The only downside I see would be that having both IFPGA device (for
> triggering bus ops) and afterward using AFU devices on this bus could be
> confusing. However, it seems safer and still cleaner overall.

Actually, just to build upon this remark, and thinking a little more
about it:

You would not need an additional "special" type of ifpga devices.
Upon attempting to hotplug an ifpga device, you would have a new
rte_devargs having the parameters within inserted, addressed to your
bus.

During scan, you would find this devargs, read the relevant parameters
and parse them (exactly like it is now implemented). You would then be
able to spawn as needed any number of rte_afu_device within your bus and
the scan would be complete.

The only trickery would be afterward, because you'd need at least one
device that would be detached and having the name given in the devargs.
So you would have to name your rte_afu_device after the devargs passed
as parameter to the plug, essentially having all of them using the same name.

Nothing prevents you from renaming your AFU devices after bus->plug, but
the EAL will rely on it for matching and knowing the scan went ok.
  
Bruce Richardson March 21, 2018, 3:41 p.m. UTC | #8
On Wed, Mar 21, 2018 at 03:31:31PM +0100, Gaëtan Rivet wrote:
> On Wed, Mar 21, 2018 at 03:14:05PM +0100, Gaëtan Rivet wrote:
> > Hi Bruce,
> > 
> > On Wed, Mar 21, 2018 at 01:35:09PM +0000, Bruce Richardson wrote:
> > > On Wed, Mar 21, 2018 at 11:20:25AM +0100, Gaëtan Rivet wrote:
> > > > Hi,
> > > > 
> > > > I have had issues compiling a few things here, have you checked
> > > > build status before submitting?
> > > > 
> > > > On Wed, Mar 21, 2018 at 03:51:32PM +0800, Rosen Xu wrote:
> > > > > Signed-off-by: Rosen Xu <rosen.xu@intel.com>
> > > > > ---
> > > <snip>
> > > > +
> > > > > +/*
> > > > > + * Scan the content of the FPGA bus, and the devices in the devices
> > > > > + * list
> > > > > + */
> > > > 
> > > > So you seem to scan your bus by reading parameters
> > > > given to the --ifpga EAL option.
> > > > 
> > > > Can you justify why you cannot use the PCI bus, have your FPGA be probed
> > > > by a PCI driver, that would take those parameters as driver parameters,
> > > > and spawn raw devices (one per bitstream) as needed as a result?
> > > > 
> > > > I see no reason this is not feasible. Unless you duly justify this
> > > > approach, it seems unacceptable to me. You are subverting generic EAL
> > > > code to bend things to your approach, without clear rationale.
> > > > 
> > > 
> > > While I agree with the comments in other emails about avoiding
> > > special-cases in the code that makes things not-scalable, I would take the
> > > view that using a bus-type is the correct choice for this. While you could
> > > have a single device that creates other devices, that is also true for all
> > > other buses as well.  [Furthermore, I think it is incorrect assume that all
> > > devices on the FPGA bus would be raw devices, it's entirely possible to
> > > have cryptodevs, bbdevs or compress devs implemented in the AFUs].
> > > 
> > > Consider what a bus driver provides: it's a generic mechanism for scanning
> > > for devices - which all use a common connection method - for DPDK use, and
> > > mapping device drivers to those devices. For an FPGA device which presents
> > > multiple AFUs, this seems to be exactly what is required - a device driver
> > > to scan for devices and present them to DPDK. The FPGA bus driver will have
> > > to check each AFU and match it against the set of registered AFU device
> > > drivers to ensure that the crypto AFU gets the cryptodev driver, etc.
> > > 
> > > Logically, therefore, it is a bus - which just happens to be a sub-bus of
> > > PCI, i.e. presented as a PCI device. Consider also that it may be possible
> > > or even desirable, to use blacklisting and whitelisting for those AFU
> > > devices so that some AFUs could be used by one app, while others by
> > > another. If we just have a single PCI device, I think we'll find ourselves
> > > duplicating a lot of bus-related functionality inside the driver in that
> > > case.
> > > 
> > > Regards,
> > > /Bruce
> > 
> > It makes sense indeed if you need to specialize several drivers specific
> > to AFU mappings.
> > 
> > So, taking the bus approach, it seems we almost all agree that the
> > current probe is not good enough.
> > 
> > I think you will run into issues if you registered your bus upon probing
> > a PCI driver. Instead, would it be possible to:
> > 
> >   * not add the EAL option --ifpga.
> >   * register the ifpga bus like any other.
> >   * have a PCI driver that hotplug an IFPGA device, triggering a
> >     scan and probe on the ifpga bus using the PCI device parameters.
> >     Essentially you would find there the parameters that could be
> >     given previously to the --ifpga option.
> > 
> > There should not be any EAL modification necessary.
> > 
> > The only downside I see would be that having both IFPGA device (for
> > triggering bus ops) and afterward using AFU devices on this bus could be
> > confusing. However, it seems safer and still cleaner overall.
> 
> Actually, just to build upon this remark, and thinking a little more
> about it:
> 
> You would not need an additional "special" type of ifpga devices.
> Upon attempting to hotplug an ifpga device, you would have a new
> rte_devargs having the parameters within inserted, addressed to your
> bus.
> 
> During scan, you would find this devargs, read the relevant parameters
> and parse them (exactly like it is now implemented). You would then be
> able to spawn as needed any number of rte_afu_device within your bus and
> the scan would be complete.
> 
> The only trickery would be afterward, because you'd need at least one
> device that would be detached and having the name given in the devargs.
> So you would have to name your rte_afu_device after the devargs passed
> as parameter to the plug, essentially having all of them using the same name.
> 
> Nothing prevents you from renaming your AFU devices after bus->plug, but
> the EAL will rely on it for matching and knowing the scan went ok.
> 
> -- 
Thanks, Gaëtan.

So, just to confirm I understand the suggestion correctly, you are
proposing that we create and use the FPGA bus as normal, except that the
scan will not report any devices. Then when an FPGA PCI device is
discovered, we use the hotplug infrastructure to trigger a real scan of the
FPGA for AFU devices, and do the whole driver mapping etc. process then.
Is that the basic idea you suggest?

/Bruce
  
Gaëtan Rivet March 21, 2018, 4:21 p.m. UTC | #9
On Wed, Mar 21, 2018 at 03:41:56PM +0000, Bruce Richardson wrote:
> On Wed, Mar 21, 2018 at 03:31:31PM +0100, Gaëtan Rivet wrote:
> > On Wed, Mar 21, 2018 at 03:14:05PM +0100, Gaëtan Rivet wrote:
> > > Hi Bruce,
> > > 
> > > On Wed, Mar 21, 2018 at 01:35:09PM +0000, Bruce Richardson wrote:
> > > > On Wed, Mar 21, 2018 at 11:20:25AM +0100, Gaëtan Rivet wrote:
> > > > > Hi,
> > > > > 
> > > > > I have had issues compiling a few things here, have you checked
> > > > > build status before submitting?
> > > > > 
> > > > > On Wed, Mar 21, 2018 at 03:51:32PM +0800, Rosen Xu wrote:
> > > > > > Signed-off-by: Rosen Xu <rosen.xu@intel.com>
> > > > > > ---
> > > > <snip>
> > > > > +
> > > > > > +/*
> > > > > > + * Scan the content of the FPGA bus, and the devices in the devices
> > > > > > + * list
> > > > > > + */
> > > > > 
> > > > > So you seem to scan your bus by reading parameters
> > > > > given to the --ifpga EAL option.
> > > > > 
> > > > > Can you justify why you cannot use the PCI bus, have your FPGA be probed
> > > > > by a PCI driver, that would take those parameters as driver parameters,
> > > > > and spawn raw devices (one per bitstream) as needed as a result?
> > > > > 
> > > > > I see no reason this is not feasible. Unless you duly justify this
> > > > > approach, it seems unacceptable to me. You are subverting generic EAL
> > > > > code to bend things to your approach, without clear rationale.
> > > > > 
> > > > 
> > > > While I agree with the comments in other emails about avoiding
> > > > special-cases in the code that makes things not-scalable, I would take the
> > > > view that using a bus-type is the correct choice for this. While you could
> > > > have a single device that creates other devices, that is also true for all
> > > > other buses as well.  [Furthermore, I think it is incorrect assume that all
> > > > devices on the FPGA bus would be raw devices, it's entirely possible to
> > > > have cryptodevs, bbdevs or compress devs implemented in the AFUs].
> > > > 
> > > > Consider what a bus driver provides: it's a generic mechanism for scanning
> > > > for devices - which all use a common connection method - for DPDK use, and
> > > > mapping device drivers to those devices. For an FPGA device which presents
> > > > multiple AFUs, this seems to be exactly what is required - a device driver
> > > > to scan for devices and present them to DPDK. The FPGA bus driver will have
> > > > to check each AFU and match it against the set of registered AFU device
> > > > drivers to ensure that the crypto AFU gets the cryptodev driver, etc.
> > > > 
> > > > Logically, therefore, it is a bus - which just happens to be a sub-bus of
> > > > PCI, i.e. presented as a PCI device. Consider also that it may be possible
> > > > or even desirable, to use blacklisting and whitelisting for those AFU
> > > > devices so that some AFUs could be used by one app, while others by
> > > > another. If we just have a single PCI device, I think we'll find ourselves
> > > > duplicating a lot of bus-related functionality inside the driver in that
> > > > case.
> > > > 
> > > > Regards,
> > > > /Bruce
> > > 
> > > It makes sense indeed if you need to specialize several drivers specific
> > > to AFU mappings.
> > > 
> > > So, taking the bus approach, it seems we almost all agree that the
> > > current probe is not good enough.
> > > 
> > > I think you will run into issues if you registered your bus upon probing
> > > a PCI driver. Instead, would it be possible to:
> > > 
> > >   * not add the EAL option --ifpga.
> > >   * register the ifpga bus like any other.
> > >   * have a PCI driver that hotplug an IFPGA device, triggering a
> > >     scan and probe on the ifpga bus using the PCI device parameters.
> > >     Essentially you would find there the parameters that could be
> > >     given previously to the --ifpga option.
> > > 
> > > There should not be any EAL modification necessary.
> > > 
> > > The only downside I see would be that having both IFPGA device (for
> > > triggering bus ops) and afterward using AFU devices on this bus could be
> > > confusing. However, it seems safer and still cleaner overall.
> > 
> > Actually, just to build upon this remark, and thinking a little more
> > about it:
> > 
> > You would not need an additional "special" type of ifpga devices.
> > Upon attempting to hotplug an ifpga device, you would have a new
> > rte_devargs having the parameters within inserted, addressed to your
> > bus.
> > 
> > During scan, you would find this devargs, read the relevant parameters
> > and parse them (exactly like it is now implemented). You would then be
> > able to spawn as needed any number of rte_afu_device within your bus and
> > the scan would be complete.
> > 
> > The only trickery would be afterward, because you'd need at least one
> > device that would be detached and having the name given in the devargs.
> > So you would have to name your rte_afu_device after the devargs passed
> > as parameter to the plug, essentially having all of them using the same name.
> > 
> > Nothing prevents you from renaming your AFU devices after bus->plug, but
> > the EAL will rely on it for matching and knowing the scan went ok.
> > 
> > -- 
> Thanks, Gaëtan.
> 
> So, just to confirm I understand the suggestion correctly, you are
> proposing that we create and use the FPGA bus as normal, except that the
> scan will not report any devices. Then when an FPGA PCI device is
> discovered, we use the hotplug infrastructure to trigger a real scan of the
> FPGA for AFU devices, and do the whole driver mapping etc. process then.
> Is that the basic idea you suggest?

Yes.
  

Patch

diff --git a/drivers/bus/ifpga/Makefile b/drivers/bus/ifpga/Makefile
new file mode 100644
index 0000000..c71f186
--- /dev/null
+++ b/drivers/bus/ifpga/Makefile
@@ -0,0 +1,64 @@ 
+#   BSD LICENSE
+#
+#   Copyright(c) 2010-2017 Intel Corporation. All rights reserved.
+#   All rights reserved.
+#
+#   Redistribution and use in source and binary forms, with or without
+#   modification, are permitted provided that the following conditions
+#   are met:
+#
+#     * Redistributions of source code must retain the above copyright
+#       notice, this list of conditions and the following disclaimer.
+#     * Redistributions in binary form must reproduce the above copyright
+#       notice, this list of conditions and the following disclaimer in
+#       the documentation and/or other materials provided with the
+#       distribution.
+#     * Neither the name of Intel Corporation nor the names of its
+#       contributors may be used to endorse or promote products derived
+#       from this software without specific prior written permission.
+#
+#   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+#   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+#   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+#   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+#   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+#   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+#   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+#   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+#   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+#   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+#   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+include $(RTE_SDK)/mk/rte.vars.mk
+
+#
+# library name
+#
+LIB = librte_bus_ifpga.a
+LIBABIVER := 1
+EXPORT_MAP := rte_bus_ifpga_version.map
+
+ifeq ($(CONFIG_RTE_LIBRTE_DPAA2_DEBUG_INIT),y)
+CFLAGS += -O0 -g
+CFLAGS += "-Wno-error"
+else
+CFLAGS += -O3
+CFLAGS += $(WERROR_FLAGS)
+endif
+
+CFLAGS += -I$(RTE_SDK)/drivers/bus/ifpga
+CFLAGS += -I$(RTE_SDK)/drivers/bus/pci
+CFLAGS += -I$(RTE_SDK)/lib/librte_eal/linuxapp/eal
+CFLAGS += -I$(RTE_SDK)/lib/librte_eal/common
+#CFLAGS += -I$(RTE_SDK)/lib/librte_rawdev
+#LDLIBS += -lrte_eal -lrte_mbuf -lrte_mempool -lrte_ring -lrte_rawdev
+LDLIBS += -lrte_eal -lrte_mbuf -lrte_mempool -lrte_ring
+#LDLIBS += -lrte_ethdev
+
+VPATH += $(SRCDIR)/base
+
+SRCS-y += \
+        ifpga_bus.c \
+        ifpga_common.c
+
+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..ff72b74
--- /dev/null
+++ b/drivers/bus/ifpga/ifpga_bus.c
@@ -0,0 +1,573 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2010-2014 Intel Corporation.
+ * Copyright 2013-2014 6WIND S.A.
+ */
+
+#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_pci.h>
+#include <rte_bus_pci.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;
+
+/*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 (!ifpga_pci_addr_cmp_1(&ifpga_dev->pci_addr, pci_addr))
+			return ifpga_dev;
+	}
+	return NULL;
+}
+
+static struct rte_afu_device *
+ifpga_find_afu_dev(const struct rte_afu_id *afu_id)
+{
+	struct rte_afu_device *afu_dev = NULL;
+
+	TAILQ_FOREACH(afu_dev, &rte_ifpga_bus.afu_list, next) {
+		if (!ifpga_afu_id_cmp(&afu_dev->id, afu_id))
+			return afu_dev;
+	}
+	return NULL;
+}
+
+static const char *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
+};
+
+/*
+ * 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_rawdev *rawdev)
+{
+	struct rte_kvargs *kvlist = NULL;
+	struct rte_bus *pci_bus = NULL;
+	struct rte_device *dev = 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);
+	}
+
+	if (ifpga_find_afu_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;
+	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;
+
+	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));
+		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;
+
+	/* 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;
+	}
+
+	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.bus      = pci_addr.bus;
+		ifpga_dev->pci_addr.devid    = pci_addr.devid;
+		ifpga_dev->pci_addr.function = pci_addr.function;
+
+		TAILQ_INSERT_TAIL(&rte_ifpga_bus.ifpga_list, ifpga_dev, next);
+	}
+
+	return 0;
+
+end:
+	if (kvlist)
+		rte_kvargs_free(kvlist);
+
+	return 0;
+}
+
+/*
+ * Scan the content of the PCI bus, and call the probe() function for
+ * all registered drivers that have a matching entry in its id_table
+ * for discovered devices.
+ */
+int
+rte_ifpga_scan_2nd(void *conf, void *id)
+{
+	struct rte_ifpga_device *ifpga_dev;
+	struct rte_rawdev *rdev;
+	struct rte_pci_addr *pci_addr;
+	struct rte_devargs *devargs;
+	struct rte_afu_device *afu_dev = NULL;
+
+	rdev     = conf;
+	pci_addr = id;
+
+	ifpga_dev = ifpga_find_ifpga_dev(pci_addr);
+	if (!ifpga_dev)
+		return 0;
+
+	ifpga_dev->rdev = rdev;
+
+    /* for FPGA devices we scan the devargs_list populated via cmdline */
+	TAILQ_FOREACH(devargs, &devargs_list, next) {
+	    afu_dev = rte_ifpga_scan_one(devargs, rdev);
+		if (afu_dev != NULL)
+		    TAILQ_INSERT_TAIL(&rte_ifpga_bus.afu_list, afu_dev, next);
+		else
+			return 0;
+	}
+
+	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) &&
+		(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
+ * all registered drivers that have a matching entry in its id_table
+ * for discovered devices.
+ */
+static int
+rte_ifpga_probe(void)
+{
+	struct rte_afu_device *afu_dev = NULL;
+	int ret = 0;
+
+	TAILQ_FOREACH(afu_dev, &rte_ifpga_bus.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);
+	return driver->remove(afu_dev);
+}
+
+static int
+rte_ifpga_unplug(struct rte_device *dev)
+{
+	struct rte_afu_device *afu_dev;
+	struct rte_devargs *devargs;
+	int ret;
+
+	if (dev == NULL)
+		return -EINVAL;
+
+	afu_dev = RTE_DEV_TO_AFU(dev);
+	if (!dev)
+		return -ENOENT;
+
+	devargs = dev->devargs;
+
+	ret = ifpga_remove_driver(afu_dev);
+	if (ret)
+		return ret;
+
+	TAILQ_REMOVE(&rte_ifpga_bus.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_afu_device *afu_dev;
+
+	TAILQ_FOREACH(afu_dev, &rte_ifpga_bus.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,
+	     .plug        = rte_ifpga_plug,
+	     .unplug      = rte_ifpga_unplug,
+	     .parse       = rte_ifpga_parse,
+	 },
+	.ifpga_list  = TAILQ_HEAD_INITIALIZER(rte_ifpga_bus.ifpga_list),
+	.afu_list    = TAILQ_HEAD_INITIALIZER(rte_ifpga_bus.afu_list),
+	.driver_list = TAILQ_HEAD_INITIALIZER(rte_ifpga_bus.driver_list),
+};
+
+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..8d98854
--- /dev/null
+++ b/drivers/bus/ifpga/ifpga_common.c
@@ -0,0 +1,154 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2010-2014 Intel Corporation.
+ * Copyright 2013-2014 6WIND S.A.
+ */
+
+#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_pci.h>
+#include <rte_bus_pci.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_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;
+}
+
+int ifpga_pci_addr_cmp_1(const struct rte_pci_addr *pci_addr0,
+	const struct rte_pci_addr *pci_addr1)
+{
+	if ((pci_addr0->bus == pci_addr1->bus) &&
+		(pci_addr0->devid == pci_addr1->devid) &&
+		(pci_addr0->function == pci_addr1->function)) {
+		return 0;
+	}
+
+	return 1;
+}
+
+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;
+}
+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);
+}
diff --git a/drivers/bus/ifpga/ifpga_common.h b/drivers/bus/ifpga/ifpga_common.h
new file mode 100644
index 0000000..23b68d4
--- /dev/null
+++ b/drivers/bus/ifpga/ifpga_common.h
@@ -0,0 +1,25 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2010-2015 Intel Corporation.
+ * Copyright 2013-2014 6WIND S.A.
+ */
+
+#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_pci_addr_cmp_1(const struct rte_pci_addr *pci_addr0,
+	const struct rte_pci_addr *pci_addr1);
+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..36b9b3f
--- /dev/null
+++ b/drivers/bus/ifpga/ifpga_logs.h
@@ -0,0 +1,32 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2010-2015 Intel Corporation.
+ * Copyright 2013-2014 6WIND S.A.
+ */
+
+#ifndef _IFPGA_BUS_LOGS_H_
+#define _IFPGA_BUS_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/rte_bus_ifpga.h b/drivers/bus/ifpga/rte_bus_ifpga.h
new file mode 100644
index 0000000..3ffa30a
--- /dev/null
+++ b/drivers/bus/ifpga/rte_bus_ifpga.h
@@ -0,0 +1,141 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2010-2015 Intel Corporation.
+ * Copyright 2013-2014 6WIND S.A.
+ */
+
+#ifndef _RTE_BUS_IFPGA_H_
+#define _RTE_BUS_IFPGA_H_
+
+/**
+ * @file
+ *
+ * RTE PCI 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_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
+
+/**
+ * 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];
+};
+
+#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;
+};
+
+/**
+ * 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;
+	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_device_list afu_list;  /**< List of AFU 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);
+int rte_ifpga_scan_2nd(void *conf, void *id);
+
+
+#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..e2aa7da
--- /dev/null
+++ b/drivers/bus/ifpga/rte_bus_ifpga_version.map
@@ -0,0 +1,8 @@ 
+DPDK_17.11 {
+	global:
+
+    rte_ifpga_driver_register;
+    rte_ifpga_driver_unregister;
+
+	local: *;
+};