diff mbox series

[v4,2/2] bus/auxiliary: introduce auxiliary bus

Message ID 20210613125846.19852-2-xuemingl@nvidia.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series None | expand

Checks

Context Check Description
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/iol-mellanox-Functional fail Functional Testing issues
ci/iol-intel-Functional fail Functional Testing issues
ci/iol-abi-testing success Testing PASS
ci/checkpatch warning coding style issues

Commit Message

Xueming(Steven) Li June 13, 2021, 12:58 p.m. UTC
Auxiliary bus [1] provides a way to split function into child-devices
representing sub-domains of functionality. Each auxiliary device
represents a part of its parent functionality.

Auxiliary device is identified by unique device name, sysfs path:
  /sys/bus/auxiliary/devices/<name>

Devargs syntax of auxiliary device:
  -a auxiliary:<name>[,args...]

[1] kernel auxiliary bus document:
https://www.kernel.org/doc/html/latest/driver-api/auxiliary_bus.html

Signed-off-by: Xueming Li <xuemingl@nvidia.com>
Cc: Wang Haiyue <haiyue.wang@intel.com>
Cc: Thomas Monjalon <thomas@monjalon.net>
Cc: Kinsella Ray <mdr@ashroe.eu>
---
 MAINTAINERS                               |   5 +
 doc/guides/rel_notes/release_21_08.rst    |   7 +
 drivers/bus/auxiliary/auxiliary_common.c  | 419 ++++++++++++++++++++++
 drivers/bus/auxiliary/auxiliary_params.c  |  58 +++
 drivers/bus/auxiliary/linux/auxiliary.c   | 142 ++++++++
 drivers/bus/auxiliary/meson.build         |  11 +
 drivers/bus/auxiliary/private.h           | 112 ++++++
 drivers/bus/auxiliary/rte_bus_auxiliary.h | 201 +++++++++++
 drivers/bus/auxiliary/version.map         |   7 +
 drivers/bus/meson.build                   |   1 +
 10 files changed, 963 insertions(+)
 create mode 100644 drivers/bus/auxiliary/auxiliary_common.c
 create mode 100644 drivers/bus/auxiliary/auxiliary_params.c
 create mode 100644 drivers/bus/auxiliary/linux/auxiliary.c
 create mode 100644 drivers/bus/auxiliary/meson.build
 create mode 100644 drivers/bus/auxiliary/private.h
 create mode 100644 drivers/bus/auxiliary/rte_bus_auxiliary.h
 create mode 100644 drivers/bus/auxiliary/version.map

Comments

Thomas Monjalon June 21, 2021, 4:11 p.m. UTC | #1
13/06/2021 14:58, Xueming Li:
> Auxiliary bus [1] provides a way to split function into child-devices
> representing sub-domains of functionality. Each auxiliary device
> represents a part of its parent functionality.
> 
> Auxiliary device is identified by unique device name, sysfs path:
>   /sys/bus/auxiliary/devices/<name>
> 
> Devargs syntax of auxiliary device:
>   -a auxiliary:<name>[,args...]

What about suggesting the new generic syntax?

> [1] kernel auxiliary bus document:
> https://www.kernel.org/doc/html/latest/driver-api/auxiliary_bus.html
> 
> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
[...]
> --- a/doc/guides/rel_notes/release_21_08.rst
> +++ b/doc/guides/rel_notes/release_21_08.rst
> @@ -55,6 +55,13 @@ New Features
>       Also, make sure to start the actual text at the margin.
>       =======================================================
>  
> +* **Added auxiliary bus support.**
> +
> +  * Auxiliary bus provides a way to split function into child-devices
> +    representing sub-domains of functionality. Each auxiliary device
> +    represents a part of its parent functionality.
> +  * Devargs syntax of auxiliary device: -a auxiliary:<name>[,args...]

I am not sure the release notes are the right place to provide
a guide of the syntax, and this syntax is not the new generice one
with "bus=" that we want to promote.
I would just remove this last line from the release notes.

> --- /dev/null
> +++ b/drivers/bus/auxiliary/auxiliary_common.c
> @@ -0,0 +1,419 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright 2021 Mellanox Technologies, Ltd

I think we should use the NVIDIA copyright now.

> +static struct rte_devargs *
> +auxiliary_devargs_lookup(const char *name)
> +{
> +	struct rte_devargs *devargs;
> +
> +	RTE_EAL_DEVARGS_FOREACH(RTE_BUS_AXILIARY_NAME, devargs) {

Missing an "U" in RTE_BUS_AXILIARY_NAME

[...]
> +/*
> + * Scan the content of the auxiliary bus, and the devices in the devices
> + * list

Simpler: Scan the devices in the auxiliary bus.

[...]
> +/**
> + * Update a device being scanned.

Not clear what is updated.
It seems to be just the devargs part?

> + *
> + * @param aux_dev
> + *	AUXILIARY device.
> + */

Should not be a doxygen comment.

> +void
> +auxiliary_on_scan(struct rte_auxiliary_device *aux_dev)
> +{
> +	aux_dev->device.devargs = auxiliary_devargs_lookup(aux_dev->name);
> +}

[...]
> +static int
> +rte_auxiliary_probe_one_driver(struct rte_auxiliary_driver *dr,
> +			       struct rte_auxiliary_device *dev)
> +{
> +	enum rte_iova_mode iova_mode;
> +	int ret;
> +
> +	if ((dr == NULL) || (dev == NULL))
> +		return -EINVAL;
> +
> +	/* The device is not blocked; Check if driver supports it. */

I don't understand why the comment about "not blocked" here.
The policy check is below.

> +	if (!auxiliary_match(dr, dev))
> +		/* Match of device and driver failed */
> +		return 1;
> +
> +	AUXILIARY_LOG(DEBUG, "Auxiliary device %s on NUMA socket %i\n",
> +		      dev->name, dev->device.numa_node);
> +
> +	/* No initialization when marked as blocked, return without error. */
> +	if (dev->device.devargs != NULL &&
> +	    dev->device.devargs->policy == RTE_DEV_BLOCKED) {
> +		AUXILIARY_LOG(INFO, "  Device is blocked, not initializing\n");

Please no indent inside logs.
And no \n as it is already in the macro.

> +		return -1;
> +	}

[...]
> +static int
> +rte_auxiliary_driver_remove_dev(struct rte_auxiliary_device *dev)
> +{
> +	struct rte_auxiliary_driver *dr;

Not sure this variable is needed.
If you keep it, please "drv" is better.

> +	int ret = 0;
> +
> +	if (dev == NULL)
> +		return -EINVAL;
> +
> +	dr = dev->driver;
> +
> +	AUXILIARY_LOG(DEBUG, "Auxiliary device %s on NUMA socket %i\n",
> +		      dev->name, dev->device.numa_node);
> +
> +	AUXILIARY_LOG(DEBUG, "  remove driver: %s %s\n",
> +		      dev->name, dr->driver.name);
> +
> +	if (dr->remove) {
> +		ret = dr->remove(dev);
> +		if (ret < 0)
> +			return ret;
> +	}

[...]
> +/*
> + * Scan the content of the auxiliary bus, and call the probe() function for
> + *
> + * all registered drivers that have a matching entry in its id_table
> + * for discovered devices.

Please elaborate what is the id_table.

[...]
> +static int
> +auxiliary_dma_map(struct rte_device *dev, void *addr, uint64_t iova, size_t len)
> +{
> +	struct rte_auxiliary_device *aux_dev = RTE_DEV_TO_AUXILIARY(dev);
> +
> +	if (dev == NULL || !aux_dev->driver) {

For all pointers, please compare with NULL, they are not booleans.

> +		rte_errno = EINVAL;
> +		return -1;
> +	}
> +	if (aux_dev->driver->dma_map)
> +		return aux_dev->driver->dma_map(aux_dev, addr, iova, len);
> +	rte_errno = ENOTSUP;
> +	return -1;

I would prever the reverse logic: error first
and callback return at last.

[...]
Some code is not reviewed here to not make this mail too long.
[...]

> --- /dev/null
> +++ b/drivers/bus/auxiliary/meson.build
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright 2021 Mellanox Technologies, Ltd
> +
> +headers = files('rte_bus_auxiliary.h')
> +sources = files('auxiliary_common.c',
> +    'auxiliary_params.c')

I think it should with a comma and the parenthesis on next line.
Please check style of other meson files which were re-styled recently.

> +if is_linux
> +    sources += files('linux/auxiliary.c')
> +endif
> +deps += ['kvargs']
> +

Empty line at EOF

> --- /dev/null
> +++ b/drivers/bus/auxiliary/private.h
> @@ -0,0 +1,112 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright 2021 Mellanox Technologies, Ltd
> + */
> +
> +#ifndef _AUXILIARY_PRIVATE_H_
> +#define _AUXILIARY_PRIVATE_H_
> +
> +#include <stdbool.h>
> +#include <stdio.h>

An empty line is missing here.

> +#include "rte_bus_auxiliary.h"
> +
> +extern struct rte_auxiliary_bus auxiliary_bus;
> +extern int auxiliary_bus_logtype;
> +
> +#define AUXILIARY_LOG(level, fmt, args...) \
> +	rte_log(RTE_LOG_ ## level, auxiliary_bus_logtype, "%s(): " fmt "\n", \
> +		__func__, ##args)

I suggest this better (pedantic-compliant) format:
#define AUXILIARY_LOG(level, ...) \
    rte_log(RTE_LOG_ ## level, auxiliary_bus_logtype, RTE_FMT("auxiliary bus: " \
        RTE_FMT_HEAD(__VA_ARGS__,) "\n", RTE_FMT_TAIL(__VA_ARGS__,)))

I think the __func__ should not be needed if log is well written.

> +
> +/* Auxiliary bus iterators */
> +#define FOREACH_DEVICE_ON_AUXILIARYBUS(p) \
> +		TAILQ_FOREACH(p, &(auxiliary_bus.device_list), next)
> +
> +#define FOREACH_DRIVER_ON_AUXILIARYBUS(p) \
> +		TAILQ_FOREACH(p, &(auxiliary_bus.driver_list), next)

An underscore is missing between AUXILIARY and BUS.

> +
> +bool auxiliary_dev_exists(const char *name);
> +
> +/**
> + * Scan the content of the auxiliary bus, and the devices in the devices
> + * list
> + *
> + * @return
> + *  0 on success, negative on error
> + */

You can make the comments shorter as it is private (no doxygen).

> +int auxiliary_scan(void);

[...]
> + * @return void

Especially this comment is useless :)

[...]
> --- /dev/null
> +++ b/drivers/bus/auxiliary/rte_bus_auxiliary.h
[...]
> +typedef bool(rte_auxiliary_match_t) (const char *name);

I think checkpatch will complain about the space between parens.

[...]
> +struct rte_auxiliary_device {
> +	TAILQ_ENTRY(rte_auxiliary_device) next;   /**< Next probed device. */
> +	char name[RTE_DEV_NAME_MAX_LEN + 1];      /**< ASCII device name */
> +	struct rte_device device;                 /**< Inherit core device */

core device should be before the name.

> +	struct rte_intr_handle intr_handle;       /**< Interrupt handle */
> +	struct rte_auxiliary_driver *driver;      /**< driver used in probing */

Why in probing?
I suggest "Device driver"

> +};
> +
> +/** List of auxiliary devices */
> +TAILQ_HEAD(rte_auxiliary_device_list, rte_auxiliary_device);
> +/** List of auxiliary drivers */
> +TAILQ_HEAD(rte_auxiliary_driver_list, rte_auxiliary_driver);
> +
> +/**
> + * Structure describing the auxiliary bus
> + */
> +struct rte_auxiliary_bus {
> +	struct rte_bus bus;                  /**< Inherit the generic class */
> +	struct rte_auxiliary_device_list device_list;  /**< List of devices */
> +	struct rte_auxiliary_driver_list driver_list;  /**< List of drivers */
> +};
> +
> +/**
> + * A structure describing an auxiliary driver.
> + */
> +struct rte_auxiliary_driver {
> +	TAILQ_ENTRY(rte_auxiliary_driver) next; /**< Next in list. */
> +	struct rte_driver driver;            /**< Inherit core driver. */
> +	struct rte_auxiliary_bus *bus;       /**< Auxiliary bus reference. */
> +	rte_auxiliary_match_t *match;         /**< Device match function. */
> +	rte_auxiliary_probe_t *probe;         /**< Device Probe function. */
> +	rte_auxiliary_remove_t *remove;       /**< Device Remove function. */
> +	rte_auxiliary_dma_map_t *dma_map;     /**< Device dma map function. */
> +	rte_auxiliary_dma_unmap_t *dma_unmap; /**< Device dma unmap function. */
> +	uint32_t drv_flags;                  /**< Flags RTE_auxiliary_DRV_*. */

Wrong search/replace missing capital letters.

[...]
> --- /dev/null
> +++ b/drivers/bus/auxiliary/version.map
> @@ -0,0 +1,7 @@
> +EXPERIMENTAL {
> +	global:
> +
> +	# added in 21.08
> +	rte_auxiliary_register;
> +	rte_auxiliary_unregister;
> +};

After more thoughts, shouldn't it be an internal symbol?
It is used only by DPDK drivers.
Xueming(Steven) Li June 22, 2021, 11:50 p.m. UTC | #2
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday, June 22, 2021 12:11 AM
> To: Parav Pandit <parav@nvidia.com>; Xueming(Steven) Li <xuemingl@nvidia.com>
> Cc: dev@dpdk.org; Wang Haiyue <haiyue.wang@intel.com>; Kinsella Ray <mdr@ashroe.eu>; david.marchand@redhat.com;
> ferruh.yigit@intel.com
> Subject: Re: [dpdk-dev] [PATCH v4 2/2] bus/auxiliary: introduce auxiliary bus
> 
> 13/06/2021 14:58, Xueming Li:
> > Auxiliary bus [1] provides a way to split function into child-devices
> > representing sub-domains of functionality. Each auxiliary device
> > represents a part of its parent functionality.
> >
> > Auxiliary device is identified by unique device name, sysfs path:
> >   /sys/bus/auxiliary/devices/<name>
> >
> > Devargs syntax of auxiliary device:
> >   -a auxiliary:<name>[,args...]
> 
> What about suggesting the new generic syntax?

I'll list both.

> 
> > [1] kernel auxiliary bus document:
> > https://www.kernel.org/doc/html/latest/driver-api/auxiliary_bus.html
> >
> > Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> [...]
> > --- a/doc/guides/rel_notes/release_21_08.rst
> > +++ b/doc/guides/rel_notes/release_21_08.rst
> > @@ -55,6 +55,13 @@ New Features
> >       Also, make sure to start the actual text at the margin.
> >       =======================================================
> >
> > +* **Added auxiliary bus support.**
> > +
> > +  * Auxiliary bus provides a way to split function into child-devices
> > +    representing sub-domains of functionality. Each auxiliary device
> > +    represents a part of its parent functionality.
> > +  * Devargs syntax of auxiliary device: -a auxiliary:<name>[,args...]
> 
> I am not sure the release notes are the right place to provide a guide of the syntax, and this syntax is not the new generice one with
> "bus=" that we want to promote.
> I would just remove this last line from the release notes
> 
> > --- /dev/null
> > +++ b/drivers/bus/auxiliary/auxiliary_common.c
> > @@ -0,0 +1,419 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright 2021 Mellanox Technologies, Ltd
> 
> I think we should use the NVIDIA copyright now.

Good catch!

> 
> > +static struct rte_devargs *
> > +auxiliary_devargs_lookup(const char *name) {
> > +	struct rte_devargs *devargs;
> > +
> > +	RTE_EAL_DEVARGS_FOREACH(RTE_BUS_AXILIARY_NAME, devargs) {
> 
> Missing an "U" in RTE_BUS_AXILIARY_NAME
> 
> [...]
> > +/*
> > + * Scan the content of the auxiliary bus, and the devices in the
> > +devices
> > + * list
> 
> Simpler: Scan the devices in the auxiliary bus.
> 
> [...]
> > +/**
> > + * Update a device being scanned.
> 
> Not clear what is updated.
> It seems to be just the devargs part?
> 
> > + *
> > + * @param aux_dev
> > + *	AUXILIARY device.
> > + */
> 
> Should not be a doxygen comment.
> 
> > +void
> > +auxiliary_on_scan(struct rte_auxiliary_device *aux_dev) {
> > +	aux_dev->device.devargs = auxiliary_devargs_lookup(aux_dev->name);
> > +}
> 
> [...]
> > +static int
> > +rte_auxiliary_probe_one_driver(struct rte_auxiliary_driver *dr,
> > +			       struct rte_auxiliary_device *dev) {
> > +	enum rte_iova_mode iova_mode;
> > +	int ret;
> > +
> > +	if ((dr == NULL) || (dev == NULL))
> > +		return -EINVAL;
> > +
> > +	/* The device is not blocked; Check if driver supports it. */
> 
> I don't understand why the comment about "not blocked" here.
> The policy check is below.
> 
> > +	if (!auxiliary_match(dr, dev))
> > +		/* Match of device and driver failed */
> > +		return 1;
> > +
> > +	AUXILIARY_LOG(DEBUG, "Auxiliary device %s on NUMA socket %i\n",
> > +		      dev->name, dev->device.numa_node);
> > +
> > +	/* No initialization when marked as blocked, return without error. */
> > +	if (dev->device.devargs != NULL &&
> > +	    dev->device.devargs->policy == RTE_DEV_BLOCKED) {
> > +		AUXILIARY_LOG(INFO, "  Device is blocked, not initializing\n");
> 
> Please no indent inside logs.
> And no \n as it is already in the macro.
> 
> > +		return -1;
> > +	}
> 
> [...]
> > +static int
> > +rte_auxiliary_driver_remove_dev(struct rte_auxiliary_device *dev) {
> > +	struct rte_auxiliary_driver *dr;
> 
> Not sure this variable is needed.
> If you keep it, please "drv" is better.
> 
> > +	int ret = 0;
> > +
> > +	if (dev == NULL)
> > +		return -EINVAL;
> > +
> > +	dr = dev->driver;
> > +
> > +	AUXILIARY_LOG(DEBUG, "Auxiliary device %s on NUMA socket %i\n",
> > +		      dev->name, dev->device.numa_node);
> > +
> > +	AUXILIARY_LOG(DEBUG, "  remove driver: %s %s\n",
> > +		      dev->name, dr->driver.name);
> > +
> > +	if (dr->remove) {
> > +		ret = dr->remove(dev);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> 
> [...]
> > +/*
> > + * Scan the content of the auxiliary bus, and call the probe()
> > +function for
> > + *
> > + * all registered drivers that have a matching entry in its id_table
> > + * for discovered devices.
> 
> Please elaborate what is the id_table.

Hmm, legacy code form pci bus, remove it.

> 
> [...]
> > +static int
> > +auxiliary_dma_map(struct rte_device *dev, void *addr, uint64_t iova,
> > +size_t len) {
> > +	struct rte_auxiliary_device *aux_dev = RTE_DEV_TO_AUXILIARY(dev);
> > +
> > +	if (dev == NULL || !aux_dev->driver) {
> 
> For all pointers, please compare with NULL, they are not booleans.
> 
> > +		rte_errno = EINVAL;
> > +		return -1;
> > +	}
> > +	if (aux_dev->driver->dma_map)
> > +		return aux_dev->driver->dma_map(aux_dev, addr, iova, len);
> > +	rte_errno = ENOTSUP;
> > +	return -1;
> 
> I would prever the reverse logic: error first and callback return at last.
> 
> [...]
> Some code is not reviewed here to not make this mail too long.
> [...]
> 
> > --- /dev/null
> > +++ b/drivers/bus/auxiliary/meson.build
> > @@ -0,0 +1,11 @@
> > +# SPDX-License-Identifier: BSD-3-Clause # Copyright 2021 Mellanox
> > +Technologies, Ltd
> > +
> > +headers = files('rte_bus_auxiliary.h') sources =
> > +files('auxiliary_common.c',
> > +    'auxiliary_params.c')
> 
> I think it should with a comma and the parenthesis on next line.
> Please check style of other meson files which were re-styled recently.
> 
> > +if is_linux
> > +    sources += files('linux/auxiliary.c') endif deps += ['kvargs']
> > +
> 
> Empty line at EOF
> 
> > --- /dev/null
> > +++ b/drivers/bus/auxiliary/private.h
> > @@ -0,0 +1,112 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright 2021 Mellanox Technologies, Ltd  */
> > +
> > +#ifndef _AUXILIARY_PRIVATE_H_
> > +#define _AUXILIARY_PRIVATE_H_
> > +
> > +#include <stdbool.h>
> > +#include <stdio.h>
> 
> An empty line is missing here.
> 
> > +#include "rte_bus_auxiliary.h"
> > +
> > +extern struct rte_auxiliary_bus auxiliary_bus; extern int
> > +auxiliary_bus_logtype;
> > +
> > +#define AUXILIARY_LOG(level, fmt, args...) \
> > +	rte_log(RTE_LOG_ ## level, auxiliary_bus_logtype, "%s(): " fmt "\n", \
> > +		__func__, ##args)
> 
> I suggest this better (pedantic-compliant) format:
> #define AUXILIARY_LOG(level, ...) \
>     rte_log(RTE_LOG_ ## level, auxiliary_bus_logtype, RTE_FMT("auxiliary bus: " \
>         RTE_FMT_HEAD(__VA_ARGS__,) "\n", RTE_FMT_TAIL(__VA_ARGS__,)))
> 
> I think the __func__ should not be needed if log is well written.

Thanks!

> 
> > +
> > +/* Auxiliary bus iterators */
> > +#define FOREACH_DEVICE_ON_AUXILIARYBUS(p) \
> > +		TAILQ_FOREACH(p, &(auxiliary_bus.device_list), next)
> > +
> > +#define FOREACH_DRIVER_ON_AUXILIARYBUS(p) \
> > +		TAILQ_FOREACH(p, &(auxiliary_bus.driver_list), next)
> 
> An underscore is missing between AUXILIARY and BUS.
> 
> > +
> > +bool auxiliary_dev_exists(const char *name);
> > +
> > +/**
> > + * Scan the content of the auxiliary bus, and the devices in the
> > +devices
> > + * list
> > + *
> > + * @return
> > + *  0 on success, negative on error
> > + */
> 
> You can make the comments shorter as it is private (no doxygen).
> 
> > +int auxiliary_scan(void);
> 
> [...]
> > + * @return void
> 
> Especially this comment is useless :)
> 
> [...]
> > --- /dev/null
> > +++ b/drivers/bus/auxiliary/rte_bus_auxiliary.h
> [...]
> > +typedef bool(rte_auxiliary_match_t) (const char *name);
> 
> I think checkpatch will complain about the space between parens.
> 
> [...]
> > +struct rte_auxiliary_device {
> > +	TAILQ_ENTRY(rte_auxiliary_device) next;   /**< Next probed device. */
> > +	char name[RTE_DEV_NAME_MAX_LEN + 1];      /**< ASCII device name */
> > +	struct rte_device device;                 /**< Inherit core device */
> 
> core device should be before the name.
> 
> > +	struct rte_intr_handle intr_handle;       /**< Interrupt handle */
> > +	struct rte_auxiliary_driver *driver;      /**< driver used in probing */
> 
> Why in probing?
> I suggest "Device driver"

A SF device could be probed by a class driver t then another class driver, the driver field will be overridden by later probe.
Will change to "last device driver"

> 
> > +};
> > +
> > +/** List of auxiliary devices */
> > +TAILQ_HEAD(rte_auxiliary_device_list, rte_auxiliary_device);
> > +/** List of auxiliary drivers */
> > +TAILQ_HEAD(rte_auxiliary_driver_list, rte_auxiliary_driver);
> > +
> > +/**
> > + * Structure describing the auxiliary bus  */ struct
> > +rte_auxiliary_bus {
> > +	struct rte_bus bus;                  /**< Inherit the generic class */
> > +	struct rte_auxiliary_device_list device_list;  /**< List of devices */
> > +	struct rte_auxiliary_driver_list driver_list;  /**< List of drivers
> > +*/ };
> > +
> > +/**
> > + * A structure describing an auxiliary driver.
> > + */
> > +struct rte_auxiliary_driver {
> > +	TAILQ_ENTRY(rte_auxiliary_driver) next; /**< Next in list. */
> > +	struct rte_driver driver;            /**< Inherit core driver. */
> > +	struct rte_auxiliary_bus *bus;       /**< Auxiliary bus reference. */
> > +	rte_auxiliary_match_t *match;         /**< Device match function. */
> > +	rte_auxiliary_probe_t *probe;         /**< Device Probe function. */
> > +	rte_auxiliary_remove_t *remove;       /**< Device Remove function. */
> > +	rte_auxiliary_dma_map_t *dma_map;     /**< Device dma map function. */
> > +	rte_auxiliary_dma_unmap_t *dma_unmap; /**< Device dma unmap function. */
> > +	uint32_t drv_flags;                  /**< Flags RTE_auxiliary_DRV_*. */
> 
> Wrong search/replace missing capital letters.
> 
> [...]
> > --- /dev/null
> > +++ b/drivers/bus/auxiliary/version.map
> > @@ -0,0 +1,7 @@
> > +EXPERIMENTAL {
> > +	global:
> > +
> > +	# added in 21.08
> > +	rte_auxiliary_register;
> > +	rte_auxiliary_unregister;
> > +};
> 
> After more thoughts, shouldn't it be an internal symbol?
> It is used only by DPDK drivers.
> 

So users will not be able to compose their own driver and register with auxiliary bus?z

Agree with all other great comments, thanks!
Thomas Monjalon June 23, 2021, 8:15 a.m. UTC | #3
23/06/2021 01:50, Xueming(Steven) Li:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 13/06/2021 14:58, Xueming Li:
> > > --- /dev/null
> > > +++ b/drivers/bus/auxiliary/version.map
> > > @@ -0,0 +1,7 @@
> > > +EXPERIMENTAL {
> > > +	global:
> > > +
> > > +	# added in 21.08
> > > +	rte_auxiliary_register;
> > > +	rte_auxiliary_unregister;
> > > +};
> > 
> > After more thoughts, shouldn't it be an internal symbol?
> > It is used only by DPDK drivers.
> 
> So users will not be able to compose their own driver and register with auxiliary bus?z

Yes, that's an interesting question actually.
We can continue with experimental/stable status of driver ABI,
but we should invent a new ABI flag like DRIVER,
so there is no stability policy on such symbol.
Thomas Monjalon June 23, 2021, 8:21 a.m. UTC | #4
23/06/2021 01:50, Xueming(Steven) Li:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 13/06/2021 14:58, Xueming Li:
> > > +	struct rte_auxiliary_driver *driver;      /**< driver used in probing */
> > 
> > Why in probing?
> > I suggest "Device driver"
> 
> A SF device could be probed by a class driver t then another class driver, the driver field will be overridden by later probe.
> Will change to "last device driver"

"Last" is a bit confusing.
Any field can be potentially overwritten, i.e. change its value,
and we don't comment "last value" ;)
Xueming(Steven) Li June 23, 2021, 1:54 p.m. UTC | #5
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, June 23, 2021 4:22 PM
> To: Parav Pandit <parav@nvidia.com>; Xueming(Steven) Li <xuemingl@nvidia.com>
> Cc: dev@dpdk.org; Wang Haiyue <haiyue.wang@intel.com>; Kinsella Ray <mdr@ashroe.eu>; david.marchand@redhat.com;
> ferruh.yigit@intel.com
> Subject: Re: [dpdk-dev] [PATCH v4 2/2] bus/auxiliary: introduce auxiliary bus
> 
> 23/06/2021 01:50, Xueming(Steven) Li:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 13/06/2021 14:58, Xueming Li:
> > > > +	struct rte_auxiliary_driver *driver;      /**< driver used in probing */
> > >
> > > Why in probing?
> > > I suggest "Device driver"
> >
> > A SF device could be probed by a class driver t then another class driver, the driver field will be overridden by later probe.
> > Will change to "last device driver"
> 
> "Last" is a bit confusing.
> Any field can be potentially overwritten, i.e. change its value, and we don't comment "last value" ;)

OK, will reflect in v6.
Xueming(Steven) Li June 23, 2021, 2:52 p.m. UTC | #6
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, June 23, 2021 4:15 PM
> To: Xueming(Steven) Li <xuemingl@nvidia.com>
> Cc: Parav Pandit <parav@nvidia.com>; dev@dpdk.org; Wang Haiyue <haiyue.wang@intel.com>; Kinsella Ray <mdr@ashroe.eu>;
> david.marchand@redhat.com; ferruh.yigit@intel.com
> Subject: Re: [dpdk-dev] [PATCH v4 2/2] bus/auxiliary: introduce auxiliary bus
> 
> 23/06/2021 01:50, Xueming(Steven) Li:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 13/06/2021 14:58, Xueming Li:
> > > > --- /dev/null
> > > > +++ b/drivers/bus/auxiliary/version.map
> > > > @@ -0,0 +1,7 @@
> > > > +EXPERIMENTAL {
> > > > +	global:
> > > > +
> > > > +	# added in 21.08
> > > > +	rte_auxiliary_register;
> > > > +	rte_auxiliary_unregister;
> > > > +};
> > >
> > > After more thoughts, shouldn't it be an internal symbol?
> > > It is used only by DPDK drivers.
> >
> > So users will not be able to compose their own driver and register
> > with auxiliary bus?z
> 
> Yes, that's an interesting question actually.
> We can continue with experimental/stable status of driver ABI, but we should invent a new ABI flag like DRIVER, so there is no stability
> policy on such symbol.

Not quite understand here, why we want to export the function but no ABI guarantee? the api shouldn't change frequently IMHO.
Thomas Monjalon June 24, 2021, 6:37 a.m. UTC | #7
23/06/2021 16:52, Xueming(Steven) Li:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 23/06/2021 01:50, Xueming(Steven) Li:
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > 13/06/2021 14:58, Xueming Li:
> > > > > --- /dev/null
> > > > > +++ b/drivers/bus/auxiliary/version.map
> > > > > @@ -0,0 +1,7 @@
> > > > > +EXPERIMENTAL {
> > > > > +	global:
> > > > > +
> > > > > +	# added in 21.08
> > > > > +	rte_auxiliary_register;
> > > > > +	rte_auxiliary_unregister;
> > > > > +};
> > > >
> > > > After more thoughts, shouldn't it be an internal symbol?
> > > > It is used only by DPDK drivers.
> > >
> > > So users will not be able to compose their own driver and register
> > > with auxiliary bus?z
> > 
> > Yes, that's an interesting question actually.
> > We can continue with experimental/stable status of driver ABI, but we should invent a new ABI flag like DRIVER, so there is no stability
> > policy on such symbol.
> 
> Not quite understand here, why we want to export the function but no ABI guarantee? the api shouldn't change frequently IMHO.

Sorry my message was not clear.
I am OK to keep "EXPERIMENTAL" in this patch.
But in future, we don't want to make driver interface as part
of the stable ABI because it makes evolution harder for no good reason:
nobody is asking for a stable interface with drivers.
Xueming(Steven) Li June 24, 2021, 8:42 a.m. UTC | #8
Thanks for clarification, will update in next version.
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 5877a16971..eaf691ca6a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -525,6 +525,11 @@  F: doc/guides/mempool/octeontx2.rst
 Bus Drivers
 -----------
 
+Auxiliary bus driver
+M: Parav Pandit <parav@nvidia.com>
+M: Xueming Li <xuemingl@nvidia.com>
+F: drivers/bus/auxiliary/
+
 Intel FPGA bus
 M: Rosen Xu <rosen.xu@intel.com>
 F: drivers/bus/ifpga/
diff --git a/doc/guides/rel_notes/release_21_08.rst b/doc/guides/rel_notes/release_21_08.rst
index a6ecfdf3ce..b335064963 100644
--- a/doc/guides/rel_notes/release_21_08.rst
+++ b/doc/guides/rel_notes/release_21_08.rst
@@ -55,6 +55,13 @@  New Features
      Also, make sure to start the actual text at the margin.
      =======================================================
 
+* **Added auxiliary bus support.**
+
+  * Auxiliary bus provides a way to split function into child-devices
+    representing sub-domains of functionality. Each auxiliary device
+    represents a part of its parent functionality.
+  * Devargs syntax of auxiliary device: -a auxiliary:<name>[,args...]
+
 
 Removed Items
 -------------
diff --git a/drivers/bus/auxiliary/auxiliary_common.c b/drivers/bus/auxiliary/auxiliary_common.c
new file mode 100644
index 0000000000..91008bdc80
--- /dev/null
+++ b/drivers/bus/auxiliary/auxiliary_common.c
@@ -0,0 +1,419 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright 2021 Mellanox Technologies, Ltd
+ */
+
+#include <string.h>
+#include <inttypes.h>
+#include <stdint.h>
+#include <stdbool.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <sys/queue.h>
+#include <rte_errno.h>
+#include <rte_interrupts.h>
+#include <rte_log.h>
+#include <rte_bus.h>
+#include <rte_per_lcore.h>
+#include <rte_memory.h>
+#include <rte_eal.h>
+#include <rte_eal_paging.h>
+#include <rte_string_fns.h>
+#include <rte_common.h>
+#include <rte_devargs.h>
+
+#include "private.h"
+#include "rte_bus_auxiliary.h"
+
+static struct rte_devargs *
+auxiliary_devargs_lookup(const char *name)
+{
+	struct rte_devargs *devargs;
+
+	RTE_EAL_DEVARGS_FOREACH(RTE_BUS_AXILIARY_NAME, devargs) {
+		if (strcmp(devargs->name, name) == 0)
+			return devargs;
+	}
+	return NULL;
+}
+
+/*
+ * Test whether the auxiliary device exist
+ *
+ * Stub for OS not supporting auxiliary bus.
+ */
+__rte_weak bool
+auxiliary_dev_exists(const char *name)
+{
+	RTE_SET_USED(name);
+	return false;
+}
+
+/*
+ * Scan the content of the auxiliary bus, and the devices in the devices
+ * list
+ *
+ * Stub for OS not supporting auxiliary bus.
+ */
+__rte_weak int
+auxiliary_scan(void)
+{
+	return 0;
+}
+
+/**
+ * Update a device being scanned.
+ *
+ * @param aux_dev
+ *	AUXILIARY device.
+ */
+void
+auxiliary_on_scan(struct rte_auxiliary_device *aux_dev)
+{
+	aux_dev->device.devargs = auxiliary_devargs_lookup(aux_dev->name);
+}
+
+/*
+ * Match the auxiliary driver and device using driver function.
+ */
+bool
+auxiliary_match(const struct rte_auxiliary_driver *aux_drv,
+		const struct rte_auxiliary_device *aux_dev)
+{
+	if (aux_drv->match == NULL)
+		return false;
+	return aux_drv->match(aux_dev->name);
+}
+
+/*
+ * Call the probe() function of the driver.
+ */
+static int
+rte_auxiliary_probe_one_driver(struct rte_auxiliary_driver *dr,
+			       struct rte_auxiliary_device *dev)
+{
+	enum rte_iova_mode iova_mode;
+	int ret;
+
+	if ((dr == NULL) || (dev == NULL))
+		return -EINVAL;
+
+	/* The device is not blocked; Check if driver supports it. */
+	if (!auxiliary_match(dr, dev))
+		/* Match of device and driver failed */
+		return 1;
+
+	AUXILIARY_LOG(DEBUG, "Auxiliary device %s on NUMA socket %i\n",
+		      dev->name, dev->device.numa_node);
+
+	/* No initialization when marked as blocked, return without error. */
+	if (dev->device.devargs != NULL &&
+	    dev->device.devargs->policy == RTE_DEV_BLOCKED) {
+		AUXILIARY_LOG(INFO, "  Device is blocked, not initializing\n");
+		return -1;
+	}
+
+	if (dev->device.numa_node < 0) {
+		AUXILIARY_LOG(WARNING, "  Invalid NUMA socket, default to 0\n");
+		dev->device.numa_node = 0;
+	}
+
+	AUXILIARY_LOG(DEBUG, "  Probe driver: %s\n", dr->driver.name);
+
+	iova_mode = rte_eal_iova_mode();
+	if ((dr->drv_flags & RTE_AUXILIARY_DRV_NEED_IOVA_AS_VA) &&
+	    iova_mode != RTE_IOVA_VA) {
+		AUXILIARY_LOG(ERR, "  Expecting VA IOVA mode but current mode is PA, not initializing\n");
+		return -EINVAL;
+	}
+
+	dev->driver = dr;
+
+	AUXILIARY_LOG(INFO, "Probe auxiliary driver: %s device: %s (socket %i)\n",
+		      dr->driver.name, dev->name, dev->device.numa_node);
+	ret = dr->probe(dr, dev);
+	if (ret)
+		dev->driver = NULL;
+	else
+		dev->device.driver = &dr->driver;
+
+	return ret;
+}
+
+/*
+ * Call the remove() function of the driver.
+ */
+static int
+rte_auxiliary_driver_remove_dev(struct rte_auxiliary_device *dev)
+{
+	struct rte_auxiliary_driver *dr;
+	int ret = 0;
+
+	if (dev == NULL)
+		return -EINVAL;
+
+	dr = dev->driver;
+
+	AUXILIARY_LOG(DEBUG, "Auxiliary device %s on NUMA socket %i\n",
+		      dev->name, dev->device.numa_node);
+
+	AUXILIARY_LOG(DEBUG, "  remove driver: %s %s\n",
+		      dev->name, dr->driver.name);
+
+	if (dr->remove) {
+		ret = dr->remove(dev);
+		if (ret < 0)
+			return ret;
+	}
+
+	/* clear driver structure */
+	dev->driver = NULL;
+	dev->device.driver = NULL;
+
+	return 0;
+}
+
+/*
+ * Call the probe() function of all registered driver for the given device.
+ * Return < 0 if initialization failed.
+ * Return 1 if no driver is found for this device.
+ */
+static int
+auxiliary_probe_all_drivers(struct rte_auxiliary_device *dev)
+{
+	struct rte_auxiliary_driver *dr;
+	int rc;
+
+	if (dev == NULL)
+		return -EINVAL;
+
+	FOREACH_DRIVER_ON_AUXILIARYBUS(dr) {
+		if (!dr->match(dev->name))
+			continue;
+
+		rc = rte_auxiliary_probe_one_driver(dr, dev);
+		if (rc < 0)
+			/* negative value is an error */
+			return rc;
+		if (rc > 0)
+			/* positive value means driver doesn't support it */
+			continue;
+		return 0;
+	}
+	return 1;
+}
+
+/*
+ * Scan the content of the auxiliary bus, and call the probe() function for
+ *
+ * all registered drivers that have a matching entry in its id_table
+ * for discovered devices.
+ */
+static int
+auxiliary_probe(void)
+{
+	struct rte_auxiliary_device *dev = NULL;
+	size_t probed = 0, failed = 0;
+	int ret = 0;
+
+	FOREACH_DEVICE_ON_AUXILIARYBUS(dev) {
+		probed++;
+
+		ret = auxiliary_probe_all_drivers(dev);
+		if (ret < 0) {
+			if (ret != -EEXIST) {
+				AUXILIARY_LOG(ERR, "Requested device %s cannot be used\n",
+					      dev->name);
+				rte_errno = errno;
+				failed++;
+			}
+			ret = 0;
+		}
+	}
+
+	return (probed && probed == failed) ? -1 : 0;
+}
+
+static int
+auxiliary_parse(const char *name, void *addr)
+{
+	struct rte_auxiliary_driver *dr = NULL;
+	const char **out = addr;
+
+	/* Allow dummy name to prevent bus scan. */
+	if (strlen(name) == 0)
+		return 0;
+
+	FOREACH_DRIVER_ON_AUXILIARYBUS(dr) {
+		if (dr->match(name))
+			break;
+	}
+	if (dr != NULL && addr != NULL)
+		*out = name;
+	return dr != NULL ? 0 : -1;
+}
+
+/* register a driver */
+void
+rte_auxiliary_register(struct rte_auxiliary_driver *driver)
+{
+	TAILQ_INSERT_TAIL(&auxiliary_bus.driver_list, driver, next);
+	driver->bus = &auxiliary_bus;
+}
+
+/* unregister a driver */
+void
+rte_auxiliary_unregister(struct rte_auxiliary_driver *driver)
+{
+	TAILQ_REMOVE(&auxiliary_bus.driver_list, driver, next);
+	driver->bus = NULL;
+}
+
+/* Add a device to auxiliary bus */
+void
+auxiliary_add_device(struct rte_auxiliary_device *aux_dev)
+{
+	TAILQ_INSERT_TAIL(&auxiliary_bus.device_list, aux_dev, next);
+}
+
+/* Insert a device into a predefined position in auxiliary bus */
+void
+auxiliary_insert_device(struct rte_auxiliary_device *exist_aux_dev,
+			struct rte_auxiliary_device *new_aux_dev)
+{
+	TAILQ_INSERT_BEFORE(exist_aux_dev, new_aux_dev, next);
+}
+
+/* Remove a device from auxiliary bus */
+static void
+rte_auxiliary_remove_device(struct rte_auxiliary_device *auxiliary_dev)
+{
+	TAILQ_REMOVE(&auxiliary_bus.device_list, auxiliary_dev, next);
+}
+
+static struct rte_device *
+auxiliary_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
+		      const void *data)
+{
+	const struct rte_auxiliary_device *pstart;
+	struct rte_auxiliary_device *adev;
+
+	if (start != NULL) {
+		pstart = RTE_DEV_TO_AUXILIARY_CONST(start);
+		adev = TAILQ_NEXT(pstart, next);
+	} else {
+		adev = TAILQ_FIRST(&auxiliary_bus.device_list);
+	}
+	while (adev != NULL) {
+		if (cmp(&adev->device, data) == 0)
+			return &adev->device;
+		adev = TAILQ_NEXT(adev, next);
+	}
+	return NULL;
+}
+
+static int
+auxiliary_plug(struct rte_device *dev)
+{
+	if (!auxiliary_dev_exists(dev->name))
+		return -ENOENT;
+	return auxiliary_probe_all_drivers(RTE_DEV_TO_AUXILIARY(dev));
+}
+
+static int
+auxiliary_unplug(struct rte_device *dev)
+{
+	struct rte_auxiliary_device *adev;
+	int ret;
+
+	adev = RTE_DEV_TO_AUXILIARY(dev);
+	ret = rte_auxiliary_driver_remove_dev(adev);
+	if (ret == 0) {
+		rte_auxiliary_remove_device(adev);
+		rte_devargs_remove(dev->devargs);
+		free(adev);
+	}
+	return ret;
+}
+
+static int
+auxiliary_dma_map(struct rte_device *dev, void *addr, uint64_t iova, size_t len)
+{
+	struct rte_auxiliary_device *aux_dev = RTE_DEV_TO_AUXILIARY(dev);
+
+	if (dev == NULL || !aux_dev->driver) {
+		rte_errno = EINVAL;
+		return -1;
+	}
+	if (aux_dev->driver->dma_map)
+		return aux_dev->driver->dma_map(aux_dev, addr, iova, len);
+	rte_errno = ENOTSUP;
+	return -1;
+}
+
+static int
+auxiliary_dma_unmap(struct rte_device *dev, void *addr, uint64_t iova,
+		    size_t len)
+{
+	struct rte_auxiliary_device *aux_dev = RTE_DEV_TO_AUXILIARY(dev);
+
+	if (dev == NULL || !aux_dev->driver) {
+		rte_errno = EINVAL;
+		return -1;
+	}
+	if (aux_dev->driver->dma_unmap)
+		return aux_dev->driver->dma_unmap(aux_dev, addr, iova, len);
+	rte_errno = ENOTSUP;
+	return -1;
+}
+
+bool
+auxiliary_ignore_device(const char *name)
+{
+	struct rte_devargs *devargs = auxiliary_devargs_lookup(name);
+
+	switch (auxiliary_bus.bus.conf.scan_mode) {
+	case RTE_BUS_SCAN_ALLOWLIST:
+		if (devargs && devargs->policy == RTE_DEV_ALLOWED)
+			return false;
+		break;
+	case RTE_BUS_SCAN_UNDEFINED:
+	case RTE_BUS_SCAN_BLOCKLIST:
+		if (devargs == NULL || devargs->policy != RTE_DEV_BLOCKED)
+			return false;
+		break;
+	}
+	return true;
+}
+
+static enum rte_iova_mode
+auxiliary_get_iommu_class(void)
+{
+	const struct rte_auxiliary_driver *drv;
+
+	FOREACH_DRIVER_ON_AUXILIARYBUS(drv) {
+		if (drv->drv_flags & RTE_AUXILIARY_DRV_NEED_IOVA_AS_VA)
+			return RTE_IOVA_VA;
+	}
+
+	return RTE_IOVA_DC;
+}
+
+struct rte_auxiliary_bus auxiliary_bus = {
+	.bus = {
+		.scan = auxiliary_scan,
+		.probe = auxiliary_probe,
+		.find_device = auxiliary_find_device,
+		.plug = auxiliary_plug,
+		.unplug = auxiliary_unplug,
+		.parse = auxiliary_parse,
+		.dma_map = auxiliary_dma_map,
+		.dma_unmap = auxiliary_dma_unmap,
+		.get_iommu_class = auxiliary_get_iommu_class,
+		.dev_iterate = auxiliary_dev_iterate,
+	},
+	.device_list = TAILQ_HEAD_INITIALIZER(auxiliary_bus.device_list),
+	.driver_list = TAILQ_HEAD_INITIALIZER(auxiliary_bus.driver_list),
+};
+
+RTE_REGISTER_BUS(auxiliary, auxiliary_bus.bus);
+RTE_LOG_REGISTER_DEFAULT(auxiliary_bus_logtype, NOTICE);
diff --git a/drivers/bus/auxiliary/auxiliary_params.c b/drivers/bus/auxiliary/auxiliary_params.c
new file mode 100644
index 0000000000..5a1b029839
--- /dev/null
+++ b/drivers/bus/auxiliary/auxiliary_params.c
@@ -0,0 +1,58 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright 2021 Mellanox Technologies, Ltd
+ */
+
+#include <string.h>
+
+#include <rte_bus.h>
+#include <rte_dev.h>
+#include <rte_errno.h>
+#include <rte_kvargs.h>
+
+#include "private.h"
+#include "rte_bus_auxiliary.h"
+
+enum auxiliary_params {
+	RTE_AUXILIARY_PARAM_NAME,
+};
+
+static const char * const auxiliary_params_keys[] = {
+	[RTE_AUXILIARY_PARAM_NAME] = "name",
+};
+
+static int
+auxiliary_dev_match(const struct rte_device *dev,
+	      const void *_kvlist)
+{
+	const struct rte_kvargs *kvlist = _kvlist;
+	int ret;
+
+	ret = rte_kvargs_process(kvlist,
+			auxiliary_params_keys[RTE_AUXILIARY_PARAM_NAME],
+			rte_kvargs_strcmp, (void *)(uintptr_t)dev->name);
+
+	return ret != 0 ? -1 : 0;
+}
+
+void *
+auxiliary_dev_iterate(const void *start,
+		    const char *str,
+		    const struct rte_dev_iterator *it __rte_unused)
+{
+	rte_bus_find_device_t find_device;
+	struct rte_kvargs *kvargs = NULL;
+	struct rte_device *dev;
+
+	if (str != NULL) {
+		kvargs = rte_kvargs_parse(str, auxiliary_params_keys);
+		if (kvargs == NULL) {
+			RTE_LOG(ERR, EAL, "cannot parse argument list\n");
+			rte_errno = EINVAL;
+			return NULL;
+		}
+	}
+	find_device = auxiliary_bus.bus.find_device;
+	dev = find_device(start, auxiliary_dev_match, kvargs);
+	rte_kvargs_free(kvargs);
+	return dev;
+}
diff --git a/drivers/bus/auxiliary/linux/auxiliary.c b/drivers/bus/auxiliary/linux/auxiliary.c
new file mode 100644
index 0000000000..22e0e20493
--- /dev/null
+++ b/drivers/bus/auxiliary/linux/auxiliary.c
@@ -0,0 +1,142 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright 2021 Mellanox Technologies, Ltd
+ */
+
+#include <string.h>
+#include <dirent.h>
+
+#include <rte_log.h>
+#include <rte_bus.h>
+#include <rte_malloc.h>
+#include <rte_devargs.h>
+#include <rte_memcpy.h>
+#include <eal_filesystem.h>
+
+#include "../rte_bus_auxiliary.h"
+#include "../private.h"
+
+#define AUXILIARY_SYSFS_PATH "/sys/bus/auxiliary/devices"
+
+/* Scan one auxiliary sysfs entry, and fill the devices list from it. */
+static int
+auxiliary_scan_one(const char *dirname, const char *name)
+{
+	struct rte_auxiliary_device *dev;
+	struct rte_auxiliary_device *dev2;
+	char filename[PATH_MAX];
+	unsigned long tmp;
+	int ret;
+
+	dev = malloc(sizeof(*dev));
+	if (dev == NULL)
+		return -1;
+
+	memset(dev, 0, sizeof(*dev));
+	if (rte_strscpy(dev->name, name, sizeof(dev->name)) < 0) {
+		free(dev);
+		return -1;
+	}
+	dev->device.name = dev->name;
+	dev->device.bus = &auxiliary_bus.bus;
+
+	/* Get numa node, default to 0 if not present */
+	snprintf(filename, sizeof(filename), "%s/%s/numa_node",
+		 dirname, name);
+	if (access(filename, F_OK) != -1) {
+		if (eal_parse_sysfs_value(filename, &tmp) == 0)
+			dev->device.numa_node = tmp;
+		else
+			dev->device.numa_node = -1;
+	} else {
+		dev->device.numa_node = 0;
+	}
+
+	auxiliary_on_scan(dev);
+
+	/* Device is valid, add in list (sorted) */
+	TAILQ_FOREACH(dev2, &auxiliary_bus.device_list, next) {
+		ret = strcmp(dev->name, dev2->name);
+		if (ret > 0)
+			continue;
+		if (ret < 0) {
+			auxiliary_insert_device(dev2, dev);
+		} else { /* already registered */
+			if (rte_dev_is_probed(&dev2->device) &&
+			    dev2->device.devargs != dev->device.devargs) {
+				/* To probe device with new devargs. */
+				rte_devargs_remove(dev2->device.devargs);
+				auxiliary_on_scan(dev2);
+			}
+			free(dev);
+		}
+		return 0;
+	}
+	auxiliary_add_device(dev);
+	return 0;
+}
+
+/*
+ * Test whether the auxiliary device exist
+ */
+bool
+auxiliary_dev_exists(const char *name)
+{
+	DIR *dir;
+	char dirname[PATH_MAX];
+
+	snprintf(dirname, sizeof(dirname), "%s/%s",
+		 AUXILIARY_SYSFS_PATH, name);
+	dir = opendir(dirname);
+	if (dir == NULL)
+		return false;
+	closedir(dir);
+	return true;
+}
+
+/*
+ * Scan the content of the auxiliary bus, and the devices in the devices
+ * list
+ */
+int
+auxiliary_scan(void)
+{
+	struct dirent *e;
+	DIR *dir;
+	char dirname[PATH_MAX];
+	struct rte_auxiliary_driver *drv;
+
+	dir = opendir(AUXILIARY_SYSFS_PATH);
+	if (dir == NULL) {
+		AUXILIARY_LOG(INFO, "%s not found, is auxiliary module loaded?\n",
+			      AUXILIARY_SYSFS_PATH);
+		return 0;
+	}
+
+	while ((e = readdir(dir)) != NULL) {
+		if (e->d_name[0] == '.')
+			continue;
+
+		if (auxiliary_ignore_device(e->d_name))
+			continue;
+
+		snprintf(dirname, sizeof(dirname), "%s/%s",
+			 AUXILIARY_SYSFS_PATH, e->d_name);
+
+		/* Ignore if no driver can handle. */
+		FOREACH_DRIVER_ON_AUXILIARYBUS(drv) {
+			if (drv->match(e->d_name))
+				break;
+		}
+		if (drv == NULL)
+			continue;
+
+		if (auxiliary_scan_one(dirname, e->d_name) < 0)
+			goto error;
+	}
+	closedir(dir);
+	return 0;
+
+error:
+	closedir(dir);
+	return -1;
+}
diff --git a/drivers/bus/auxiliary/meson.build b/drivers/bus/auxiliary/meson.build
new file mode 100644
index 0000000000..3d5c7b0d4a
--- /dev/null
+++ b/drivers/bus/auxiliary/meson.build
@@ -0,0 +1,11 @@ 
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright 2021 Mellanox Technologies, Ltd
+
+headers = files('rte_bus_auxiliary.h')
+sources = files('auxiliary_common.c',
+    'auxiliary_params.c')
+if is_linux
+    sources += files('linux/auxiliary.c')
+endif
+deps += ['kvargs']
+
diff --git a/drivers/bus/auxiliary/private.h b/drivers/bus/auxiliary/private.h
new file mode 100644
index 0000000000..3146fcd5d3
--- /dev/null
+++ b/drivers/bus/auxiliary/private.h
@@ -0,0 +1,112 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright 2021 Mellanox Technologies, Ltd
+ */
+
+#ifndef _AUXILIARY_PRIVATE_H_
+#define _AUXILIARY_PRIVATE_H_
+
+#include <stdbool.h>
+#include <stdio.h>
+#include "rte_bus_auxiliary.h"
+
+extern struct rte_auxiliary_bus auxiliary_bus;
+extern int auxiliary_bus_logtype;
+
+#define AUXILIARY_LOG(level, fmt, args...) \
+	rte_log(RTE_LOG_ ## level, auxiliary_bus_logtype, "%s(): " fmt "\n", \
+		__func__, ##args)
+
+/* Auxiliary bus iterators */
+#define FOREACH_DEVICE_ON_AUXILIARYBUS(p) \
+		TAILQ_FOREACH(p, &(auxiliary_bus.device_list), next)
+
+#define FOREACH_DRIVER_ON_AUXILIARYBUS(p) \
+		TAILQ_FOREACH(p, &(auxiliary_bus.driver_list), next)
+
+bool auxiliary_dev_exists(const char *name);
+
+/**
+ * Scan the content of the auxiliary bus, and the devices in the devices
+ * list
+ *
+ * @return
+ *  0 on success, negative on error
+ */
+int auxiliary_scan(void);
+
+/**
+ * Update a device being scanned.
+ *
+ * @param aux_dev
+ *	AUXILIARY device.
+ */
+void auxiliary_on_scan(struct rte_auxiliary_device *aux_dev);
+
+/**
+ * Validate whether a device with given auxiliary device should be ignored
+ * or not.
+ *
+ * @param name
+ *	Auxiliary name of device to be validated
+ * @return
+ *	true: if device is to be ignored,
+ *	false: if device is to be scanned,
+ */
+bool auxiliary_ignore_device(const char *name);
+
+/**
+ * Add an auxiliary device to the auxiliary bus (append to auxiliary Device
+ * list). This function also updates the bus references of the auxiliary
+ * Device (and the generic device object embedded within.
+ *
+ * @param aux_dev
+ *	AUXILIARY device to add
+ * @return void
+ */
+void auxiliary_add_device(struct rte_auxiliary_device *aux_dev);
+
+/**
+ * Insert an auxiliary device in the auxiliary bus at a particular location
+ * in the device list. It also updates the auxiliary bus reference of the
+ * new devices to be inserted.
+ *
+ * @param exist_aux_dev
+ *	Existing auxiliary device in auxiliary bus
+ * @param new_aux_dev
+ *	AUXILIARY device to be added before exist_aux_dev
+ * @return void
+ */
+void auxiliary_insert_device(struct rte_auxiliary_device *exist_aux_dev,
+			     struct rte_auxiliary_device *new_aux_dev);
+
+/**
+ * Match the auxiliary Driver and Device by driver function
+ *
+ * @param aux_drv
+ *      auxiliary driver
+ * @param aux_dev
+ *      auxiliary device to match against the driver
+ * @return
+ *      the driver can handle the device
+ */
+bool auxiliary_match(const struct rte_auxiliary_driver *aux_drv,
+		     const struct rte_auxiliary_device *aux_dev);
+
+/**
+ * Iterate over internal devices, matching any device against the provided
+ * string.
+ *
+ * @param start
+ *   Iteration starting point.
+ * @param str
+ *   Device string to match against.
+ * @param it
+ *   (unused) iterator structure.
+ * @return
+ *   A pointer to the next matching device if any.
+ *   NULL otherwise.
+ */
+void *auxiliary_dev_iterate(const void *start, const char *str,
+			    const struct rte_dev_iterator *it);
+
+#endif /* _AUXILIARY_PRIVATE_H_ */
diff --git a/drivers/bus/auxiliary/rte_bus_auxiliary.h b/drivers/bus/auxiliary/rte_bus_auxiliary.h
new file mode 100644
index 0000000000..94f0790ba0
--- /dev/null
+++ b/drivers/bus/auxiliary/rte_bus_auxiliary.h
@@ -0,0 +1,201 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright 2021 Mellanox Technologies, Ltd
+ */
+
+#ifndef RTE_BUS_AUXILIARY_H
+#define RTE_BUS_AUXILIARY_H
+
+/**
+ * @file
+ *
+ * Auxiliary Bus Interface.
+ */
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <limits.h>
+#include <errno.h>
+#include <sys/queue.h>
+#include <stdint.h>
+#include <inttypes.h>
+
+#include <rte_debug.h>
+#include <rte_interrupts.h>
+#include <rte_dev.h>
+#include <rte_bus.h>
+#include <rte_kvargs.h>
+
+#define RTE_BUS_AXILIARY_NAME "auxiliary"
+
+/* Forward declarations */
+struct rte_auxiliary_driver;
+struct rte_auxiliary_bus;
+struct rte_auxiliary_device;
+
+/**
+ * Match function for the driver to decide if device can be handled.
+ *
+ * @param name
+ *   Pointer to the auxiliary device name.
+ * @return
+ *   Whether the driver can handle the auxiliary device.
+ */
+typedef bool(rte_auxiliary_match_t) (const char *name);
+
+/**
+ * Initialization function for the driver called during auxiliary probing.
+ *
+ * @param drv
+ *   Pointer to the auxiliary driver.
+ * @param dev
+ *   Pointer to the auxiliary device.
+ * @return
+ *   - 0 On success.
+ *   - Negative value and rte_errno is set otherwise.
+ */
+typedef int(rte_auxiliary_probe_t) (struct rte_auxiliary_driver *drv,
+				    struct rte_auxiliary_device *dev);
+
+/**
+ * Uninitialization function for the driver called during hotplugging.
+ *
+ * @param dev
+ *   Pointer to the auxiliary device.
+ * @return
+ *   - 0 On success.
+ *   - Negative value and rte_errno is set otherwise.
+ */
+typedef int (rte_auxiliary_remove_t)(struct rte_auxiliary_device *dev);
+
+/**
+ * Driver-specific DMA mapping. After a successful call the device
+ * will be able to read/write from/to this segment.
+ *
+ * @param dev
+ *   Pointer to the auxiliary device.
+ * @param addr
+ *   Starting virtual address of memory to be mapped.
+ * @param iova
+ *   Starting IOVA address of memory to be mapped.
+ * @param len
+ *   Length of memory segment being mapped.
+ * @return
+ *   - 0 On success.
+ *   - Negative value and rte_errno is set otherwise.
+ */
+typedef int (rte_auxiliary_dma_map_t)(struct rte_auxiliary_device *dev,
+				       void *addr, uint64_t iova, size_t len);
+
+/**
+ * Driver-specific DMA un-mapping. After a successful call the device
+ * will not be able to read/write from/to this segment.
+ *
+ * @param dev
+ *   Pointer to the auxiliary device.
+ * @param addr
+ *   Starting virtual address of memory to be unmapped.
+ * @param iova
+ *   Starting IOVA address of memory to be unmapped.
+ * @param len
+ *   Length of memory segment being unmapped.
+ * @return
+ *   - 0 On success.
+ *   - Negative value and rte_errno is set otherwise.
+ */
+typedef int (rte_auxiliary_dma_unmap_t)(struct rte_auxiliary_device *dev,
+					 void *addr, uint64_t iova, size_t len);
+
+/**
+ * A structure describing an auxiliary device.
+ */
+struct rte_auxiliary_device {
+	TAILQ_ENTRY(rte_auxiliary_device) next;   /**< Next probed device. */
+	char name[RTE_DEV_NAME_MAX_LEN + 1];      /**< ASCII device name */
+	struct rte_device device;                 /**< Inherit core device */
+	struct rte_intr_handle intr_handle;       /**< Interrupt handle */
+	struct rte_auxiliary_driver *driver;      /**< driver used in probing */
+};
+
+/** List of auxiliary devices */
+TAILQ_HEAD(rte_auxiliary_device_list, rte_auxiliary_device);
+/** List of auxiliary drivers */
+TAILQ_HEAD(rte_auxiliary_driver_list, rte_auxiliary_driver);
+
+/**
+ * Structure describing the auxiliary bus
+ */
+struct rte_auxiliary_bus {
+	struct rte_bus bus;                  /**< Inherit the generic class */
+	struct rte_auxiliary_device_list device_list;  /**< List of devices */
+	struct rte_auxiliary_driver_list driver_list;  /**< List of drivers */
+};
+
+/**
+ * A structure describing an auxiliary driver.
+ */
+struct rte_auxiliary_driver {
+	TAILQ_ENTRY(rte_auxiliary_driver) next; /**< Next in list. */
+	struct rte_driver driver;            /**< Inherit core driver. */
+	struct rte_auxiliary_bus *bus;       /**< Auxiliary bus reference. */
+	rte_auxiliary_match_t *match;         /**< Device match function. */
+	rte_auxiliary_probe_t *probe;         /**< Device Probe function. */
+	rte_auxiliary_remove_t *remove;       /**< Device Remove function. */
+	rte_auxiliary_dma_map_t *dma_map;     /**< Device dma map function. */
+	rte_auxiliary_dma_unmap_t *dma_unmap; /**< Device dma unmap function. */
+	uint32_t drv_flags;                  /**< Flags RTE_auxiliary_DRV_*. */
+};
+
+/**
+ * @internal
+ * Helper macro for drivers that need to convert to struct rte_auxiliary_device.
+ */
+#define RTE_DEV_TO_AUXILIARY(ptr) \
+	container_of(ptr, struct rte_auxiliary_device, device)
+
+#define RTE_DEV_TO_AUXILIARY_CONST(ptr) \
+	container_of(ptr, const struct rte_auxiliary_device, device)
+
+#define RTE_ETH_DEV_TO_AUXILIARY(eth_dev) \
+	RTE_DEV_TO_AUXILIARY((eth_dev)->device)
+
+/** Device driver needs IOVA as VA and cannot work with IOVA as PA */
+#define RTE_AUXILIARY_DRV_NEED_IOVA_AS_VA 0x002
+
+/**
+ * Register an auxiliary driver.
+ *
+ * @param driver
+ *   A pointer to a rte_auxiliary_driver structure describing the driver
+ *   to be registered.
+ */
+__rte_experimental
+void rte_auxiliary_register(struct rte_auxiliary_driver *driver);
+
+/** Helper for auxiliary device registration from driver instance */
+#define RTE_PMD_REGISTER_AUXILIARY(nm, auxiliary_drv)		\
+	RTE_INIT(auxiliaryinitfn_ ##nm)				\
+	{							\
+		(auxiliary_drv).driver.name = RTE_STR(nm);	\
+		rte_auxiliary_register(&(auxiliary_drv));	\
+	}							\
+	RTE_PMD_EXPORT_NAME(nm, __COUNTER__)
+
+/**
+ * Unregister an auxiliary driver.
+ *
+ * @param driver
+ *   A pointer to a rte_auxiliary_driver structure describing the driver
+ *   to be unregistered.
+ */
+__rte_experimental
+void rte_auxiliary_unregister(struct rte_auxiliary_driver *driver);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* RTE_BUS_AUXILIARY_H */
diff --git a/drivers/bus/auxiliary/version.map b/drivers/bus/auxiliary/version.map
new file mode 100644
index 0000000000..a52260657c
--- /dev/null
+++ b/drivers/bus/auxiliary/version.map
@@ -0,0 +1,7 @@ 
+EXPERIMENTAL {
+	global:
+
+	# added in 21.08
+	rte_auxiliary_register;
+	rte_auxiliary_unregister;
+};
diff --git a/drivers/bus/meson.build b/drivers/bus/meson.build
index 410058de3a..45eab5233d 100644
--- a/drivers/bus/meson.build
+++ b/drivers/bus/meson.build
@@ -2,6 +2,7 @@ 
 # Copyright(c) 2017 Intel Corporation
 
 drivers = [
+        'auxiliary',
         'dpaa',
         'fslmc',
         'ifpga',