DPDK-dev Archive on lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH v4] app/testpmd: supported offload capabilities query
From: Wu, Jingjing @ 2016-12-26  1:21 UTC (permalink / raw)
  To: Yang, Qiming, dev@dpdk.org
In-Reply-To: <1482485513-7087-1-git-send-email-qiming.yang@intel.com>



> -----Original Message-----
> From: Yang, Qiming
> Sent: Friday, December 23, 2016 5:32 PM
> To: dev@dpdk.org
> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Yang, Qiming
> <qiming.yang@intel.com>
> Subject: [PATCH v4] app/testpmd: supported offload capabilities query
> 
> Add two new commands "show port cap <port>" and "show port cap all"to
> diaplay what offload capabilities supported in ports. It will not only display all
> the capabilities of the port, but also the enabling condition for each capability
> in the running time.
> 
> Signed-off-by: Qiming Yang <qiming.yang@intel.com>

Acked-by: Jingjing Wu <jingjing.wu@intel.com>

^ permalink raw reply

* Re: [PATCH 01/18] net/ixgbe: store SYN filter
From: Zhao1, Wei @ 2016-12-26  1:47 UTC (permalink / raw)
  To: Yigit, Ferruh, dev@dpdk.org; +Cc: Lu, Wenzhuo
In-Reply-To: <f7eb54bf-f834-097e-cb84-6ee067bfae29@intel.com>

Hi, Ferruh

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Wednesday, December 21, 2016 12:56 AM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH 01/18] net/ixgbe: store SYN filter
> 
> On 12/2/2016 10:42 AM, Wei Zhao wrote:
> > From: wei zhao1 <wei.zhao1@intel.com>
> >
> > Add support for storing SYN filter in SW.
> 
> Do you think does it makes more clear to refer as TCP SYN filter? Or SYN filter
> is clear enough?
> 
> >
> > Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> > Signed-off-by: wei zhao1 <wei.zhao1@intel.com>
> 
> Can you please update sign-off to your actual name?
> 
> > ---
> >  drivers/net/ixgbe/ixgbe_ethdev.c | 12 ++++++++++--
> > drivers/net/ixgbe/ixgbe_ethdev.h |  2 ++
> >  2 files changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > b/drivers/net/ixgbe/ixgbe_ethdev.c
> > index edc9b22..7f10cca 100644
> > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > @@ -1287,6 +1287,8 @@ eth_ixgbe_dev_init(struct rte_eth_dev *eth_dev)
> >  	memset(filter_info->fivetuple_mask, 0,
> >  	       sizeof(uint32_t) * IXGBE_5TUPLE_ARRAY_SIZE);
> >
> > +	/* initialize SYN filter */
> > +	filter_info->syn_info = 0;
> 
> can it be an option to memset all filter_info? (and of course move list init
> after memset)
> 
> >  	return 0;
> >  }
> >
> > @@ -5509,15 +5511,19 @@ ixgbe_syn_filter_set(struct rte_eth_dev *dev,
> >  			bool add)
> >  {
> >  	struct ixgbe_hw *hw =
> > IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> > +	struct ixgbe_filter_info *filter_info =
> > +		IXGBE_DEV_PRIVATE_TO_FILTER_INFO(dev->data-
> >dev_private);
> > +	uint32_t syn_info;
> >  	uint32_t synqf;
> >
> >  	if (filter->queue >= IXGBE_MAX_RX_QUEUE_NUM)
> >  		return -EINVAL;
> >
> > +	syn_info = filter_info->syn_info;
> >  	synqf = IXGBE_READ_REG(hw, IXGBE_SYNQF);
> >
> >  	if (add) {
> > -		if (synqf & IXGBE_SYN_FILTER_ENABLE)
> > +		if (syn_info & IXGBE_SYN_FILTER_ENABLE)
> 
> If these checks will be done on syn_info, shouldn't syn_info be assigned to
> synqf before this. Specially for first usage, synqf may be different than hw
> register.
> 
> Or perhaps can keep continue to use synqf. Since synqf assigned to
> filter_info->syn_info after updated.
> 

ok, this code is alittle vague, in "add" branch synqf will be assigned a new value, so "synqf = IXGBE_READ_REG(hw, IXGBE_SYNQF)" is useless.
synqf read from hw only to be used in "else" branch.so I will make a little code change here.
Thank you for your suggestion.  

> >  			return -EINVAL;
> >  		synqf = (uint32_t)(((filter->queue <<
> IXGBE_SYN_FILTER_QUEUE_SHIFT) &
> >  			IXGBE_SYN_FILTER_QUEUE) |
> IXGBE_SYN_FILTER_ENABLE); @@ -5527,10
> > +5533,12 @@ ixgbe_syn_filter_set(struct rte_eth_dev *dev,
> >  		else
> >  			synqf &= ~IXGBE_SYN_FILTER_SYNQFP;
> >  	} else {
> > -		if (!(synqf & IXGBE_SYN_FILTER_ENABLE))
> > +		if (!(syn_info & IXGBE_SYN_FILTER_ENABLE))
> >  			return -ENOENT;
> >  		synqf &= ~(IXGBE_SYN_FILTER_QUEUE |
> IXGBE_SYN_FILTER_ENABLE);
> >  	}
> > +
> > +	filter_info->syn_info = synqf;
> >  	IXGBE_WRITE_REG(hw, IXGBE_SYNQF, synqf);
> >  	IXGBE_WRITE_FLUSH(hw);
> >  	return 0;
> <...>
> 

^ permalink raw reply

* Re: [PATCH 02/18] net/ixgbe: store flow director filter
From: Zhao1, Wei @ 2016-12-26  2:50 UTC (permalink / raw)
  To: Yigit, Ferruh, dev@dpdk.org; +Cc: Lu, Wenzhuo
In-Reply-To: <71288c3d-2abb-d99d-45a4-f26d399194c2@intel.com>

Hi,  Ferruh

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Wednesday, December 21, 2016 12:58 AM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH 02/18] net/ixgbe: store flow director filter
> 
> On 12/2/2016 10:42 AM, Wei Zhao wrote:
> > From: wei zhao1 <wei.zhao1@intel.com>
> >
> > Add support for storing flow director filter in SW.
> >
> > Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> > Signed-off-by: wei zhao1 <wei.zhao1@intel.com>
> > ---
> >  drivers/net/ixgbe/ixgbe_ethdev.c |  48 ++++++++++++++++++
> > drivers/net/ixgbe/ixgbe_ethdev.h |  19 ++++++-
> >  drivers/net/ixgbe/ixgbe_fdir.c   | 105
> ++++++++++++++++++++++++++++++++++++++-
> >  3 files changed, 169 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > b/drivers/net/ixgbe/ixgbe_ethdev.c
> > index 7f10cca..f8e5fe1 100644
> > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> <...>
> > @@ -1289,6 +1302,25 @@ eth_ixgbe_dev_init(struct rte_eth_dev
> *eth_dev)
> >
> >  	/* initialize SYN filter */
> >  	filter_info->syn_info = 0;
> > +	/* initialize flow director filter list & hash */
> > +	TAILQ_INIT(&fdir_info->fdir_list);
> > +	snprintf(fdir_hash_name, RTE_HASH_NAMESIZE,
> > +		 "fdir_%s", eth_dev->data->name);
> > +	fdir_info->hash_handle = rte_hash_create(&fdir_hash_params);
> 
> Do we really create a hash table in device init? Is there a way to do this if we
> know user will use it?

By now, we have no idea about whether user wiil use flow director or not.
So, our strategy is init all types of filter and give option to users.

> 
> > +	if (!fdir_info->hash_handle) {
> > +		PMD_INIT_LOG(ERR, "Failed to create fdir hash table!");
> > +		return -EINVAL;
> 
> And should we exit here? What if user will not use flow director at all?

Maybe, we can delete "return -EINVAL;" and only print error log as a warning.
But some uer maybe do not pay attention to warning error log sometimes, so we chose to 
exit if any init fail.

> 
> > +	}
> > +	fdir_info->hash_map = rte_zmalloc("ixgbe",
> > +					  sizeof(struct ixgbe_fdir_filter *) *
> > +					  IXGBE_MAX_FDIR_FILTER_NUM,
> > +					  0);
> > +	if (!fdir_info->hash_map) {
> > +		PMD_INIT_LOG(ERR,
> > +			     "Failed to allocate memory for fdir hash map!");
> > +		return -ENOMEM;
> > +	}
> > +
> 
> Can you please extract these into functions, to have more clear init/uninit
> functions?

ok, that seems like a good idea to creat two new functions to do initialize of flow director and l2 tunnel fliter.
That will make dev init function more clear. I wiil add two init function for this purpose in v2.  

> 
> >  	return 0;
> >  }
> >
> > @@ -1297,6 +1329,9 @@ eth_ixgbe_dev_uninit(struct rte_eth_dev
> > *eth_dev)  {
> >  	struct rte_pci_device *pci_dev;
> >  	struct ixgbe_hw *hw;
> > +	struct ixgbe_hw_fdir_info *fdir_info =
> > +		IXGBE_DEV_PRIVATE_TO_FDIR_INFO(eth_dev->data-
> >dev_private);
> > +	struct ixgbe_fdir_filter *fdir_filter;
> >
> >  	PMD_INIT_FUNC_TRACE();
> >
> > @@ -1330,6 +1365,19 @@ eth_ixgbe_dev_uninit(struct rte_eth_dev
> *eth_dev)
> >  	rte_free(eth_dev->data->hash_mac_addrs);
> >  	eth_dev->data->hash_mac_addrs = NULL;
> >
> > +	/* remove all the fdir filters & hash */
> > +	if (fdir_info->hash_map)
> > +		rte_free(fdir_info->hash_map);
> > +	if (fdir_info->hash_handle)
> > +		rte_hash_free(fdir_info->hash_handle);
> 
> All rte_hash_xxx() APIs gives build error for shared library build, [1] needs to
> be added into Makefile.
> 
> But, this makes hash library a dependency to the PMD, do you think is there
> a way to escape from this?
> 
> [1]
> +DEPDIRS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) += lib/librte_net
> +lib/librte_hash
> 

Ok, I will change as your suggestion in v2.

> > +
> > +	while ((fdir_filter = TAILQ_FIRST(&fdir_info->fdir_list))) {
> > +		TAILQ_REMOVE(&fdir_info->fdir_list,
> > +			     fdir_filter,
> > +			     entries);
> > +		rte_free(fdir_filter);
> > +	}
> > +
> >  	return 0;
> >  }
> >
> <...>

^ permalink raw reply

* Re: [PATCH 04/18] net/ixgbe: restore n-tuple filter
From: Zhao1, Wei @ 2016-12-26  3:32 UTC (permalink / raw)
  To: Yigit, Ferruh, dev@dpdk.org; +Cc: Lu, Wenzhuo
In-Reply-To: <c74df55b-d488-c9b3-89e9-37bbffc74d0b@intel.com>

Hi,  Ferruh

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Wednesday, December 21, 2016 12:59 AM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH 04/18] net/ixgbe: restore n-tuple filter
> 
> On 12/2/2016 10:43 AM, Wei Zhao wrote:
> > From: wei zhao1 <wei.zhao1@intel.com>
> >
> > Add support for restoring n-tuple filter in SW.
> >
> > Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> > Signed-off-by: wei zhao1 <wei.zhao1@intel.com>
> > ---
> >  drivers/net/ixgbe/ixgbe_ethdev.c | 131 +++++++++++++++++++++++++--
> ------------
> >  1 file changed, 83 insertions(+), 48 deletions(-)
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> b/drivers/net/ixgbe/ixgbe_ethdev.c
> <...>
> > @@ -2482,6 +2496,7 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
> >
> >  	/* resume enabled intr since hw reset */
> >  	ixgbe_enable_intr(dev);
> > +	ixgbe_filter_restore(dev);
> 
> Just to double check, when you stop the device does 5tuple_filter reset?
> If not reset, will adding same filters cause any problem?

yes, there is reset the NIC function of ixgbe_pf_reset_hw(hw) in dev stop process.

> 
> >
> >  	return 0;
> >
> <...>

^ permalink raw reply

* Re: DPDK Accelaration Enhancement
From: Jain, Deepak K @ 2016-12-26  6:02 UTC (permalink / raw)
  To: Ant loves honey, dev@dpdk.org
In-Reply-To: <2021242051.1516112.1482600358351@mail.yahoo.com>

HI Anthony,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ant loves honey
> Sent: Saturday, December 24, 2016 10:56 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] DPDK Accelaration Enhancement
> 
> Currently DPDK has the crypto PMD (.../drivers/crypto/qat/) and example
> code (.../examples/dpdk_qat).
> Intel QuickAssist Technology also supports compression along with
> crypto.  Last weekend, there is a proposed project to Intel Dev Mesh - "A
> VPP plugin utilizing Intel QucikAssist Technology to perform hardware
> assisted compression operations"
> 
> https://devmesh.intel.com/projects/a-vpp-plugin-utilizing-intel-quickassist-
> technology-to-perform-hardware-assisted-compression-operation
> 
> Do we need a new PMD driver for compression or should we modify the
> exiting crypto/qat driver to also support compression?

There is still NO compression support in DPDK which utilizes Intel(R) QuickAssist Technology.

> 
> The Intel QAT driver should be present in any Linux kernel greater than
> version 4.4
> 
> I am trying to put the pieces together and hitting a roadblock. I am also
> figuring how the PMD driver is interacting with the Intel QAT driver at the
> code level.
> Any pointer on how to move forward is greatly appreciated.
> Please also let me know if I should modify the proposed project.
> 

Have you gone through the Crypto Documentation on dpdk.org and release notes (qat.rst) for more information?

> Merry Christmas and Happy New Year,
> Anthony.

Regards,
Deepak

^ permalink raw reply

* Re: DPDK Accelaration Enhancement
From: Ant loves honey @ 2016-12-26  6:19 UTC (permalink / raw)
  To: Jain, Deepak K, dev@dpdk.org
In-Reply-To: <A09C9DDE180C7E429EC68E2BFB95C903395024EA@IRSMSX107.ger.corp.intel.com>

Deepak,
The .../doc/guides/cryptodevs/qat.rst is geared toward crypto and no mention of compression.
I am trying to figure out what it takes to have compression support on DPDK such as new PMD driver, additional defines and/or API or chipset initialization since the Intel QAT can support compression.
It seems to me IP payload can make good use of the hardware assisted compression and this will be a nice feature to have.
Thanks so much,
anthony.

      From: "Jain, Deepak K" <deepak.k.jain@intel.com>
 To: Ant loves honey <ant_love_honey@yahoo.com>; "dev@dpdk.org" <dev@dpdk.org> 
 Sent: Sunday, December 25, 2016 10:02 PM
 Subject: RE: [dpdk-dev] DPDK Accelaration Enhancement
   
HI Anthony,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ant loves honey
> Sent: Saturday, December 24, 2016 10:56 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] DPDK Accelaration Enhancement
> 
> Currently DPDK has the crypto PMD (.../drivers/crypto/qat/) and example
> code (.../examples/dpdk_qat).
> Intel QuickAssist Technology also supports compression along with
> crypto.  Last weekend, there is a proposed project to Intel Dev Mesh - "A
> VPP plugin utilizing Intel QucikAssist Technology to perform hardware
> assisted compression operations"
> 
> https://devmesh.intel.com/projects/a-vpp-plugin-utilizing-intel-quickassist-
> technology-to-perform-hardware-assisted-compression-operation
> 
> Do we need a new PMD driver for compression or should we modify the
> exiting crypto/qat driver to also support compression?

There is still NO compression support in DPDK which utilizes Intel(R) QuickAssist Technology.

> 
> The Intel QAT driver should be present in any Linux kernel greater than
> version 4.4
> 
> I am trying to put the pieces together and hitting a roadblock. I am also
> figuring how the PMD driver is interacting with the Intel QAT driver at the
> code level.
> Any pointer on how to move forward is greatly appreciated.
> Please also let me know if I should modify the proposed project.
> 

Have you gone through the Crypto Documentation on dpdk.org and release notes (qat.rst) for more information?

> Merry Christmas and Happy New Year,
> Anthony.

Regards,
Deepak


   

^ permalink raw reply

* Re: [PATCH v2 2/7] net/virtio_user: postpone DRIVER OK notification
From: Yuanhan Liu @ 2016-12-26  6:27 UTC (permalink / raw)
  To: Jianfeng Tan; +Cc: dev, ferruh.yigit, cunming.liang
In-Reply-To: <1482477266-39199-3-git-send-email-jianfeng.tan@intel.com>

On Fri, Dec 23, 2016 at 07:14:21AM +0000, Jianfeng Tan wrote:
> In driver probe phase, we obtain device information; and then virtio
> driver will initialize device and stores info, like negotiated
> features, in vhost_user layer; finally, vhost_user gets DRIVER_OK
> notification from virtio driver, and then sync with backend device.
> 
> Previously, DRIVER_OK could be sent twice: 1. when ether layer invokes
> eth_device_init to initialize device; 2. when user configures it with
> different configuration from that of previous.

Yes, but that's wrong. Now only 1) will be taken.

> Since we can only depend on DRIVER_OK notification to sync with backend
> device, we postpone it to virtio_dev_start when everything is settled.

I don't quite understand what you were going to fix here; you don't
state it in the commit log after all. Normally, when everything is
negotiated (when DRIVER_OK is set), we should not set it again, unless
a reset has been happened.

If you look at QEMU, qemu will simply ignore it once vhost has already
started.

	--yliu

^ permalink raw reply

* Re: [PATCH v2 3/7] net/virtio_user: move vhost user specific code
From: Yuanhan Liu @ 2016-12-26  6:28 UTC (permalink / raw)
  To: Jianfeng Tan; +Cc: dev, ferruh.yigit, cunming.liang
In-Reply-To: <1482477266-39199-4-git-send-email-jianfeng.tan@intel.com>

On Fri, Dec 23, 2016 at 07:14:22AM +0000, Jianfeng Tan wrote:
> To support vhost kernel as the backend of net_virtio_user in coming
> patches, we move vhost_user specific structs and macros into
> vhost_user.c, and only keep common definitions in vhost.h.

Good.

> Besides, remove VHOST_USER_MQ feature check, it will be added back
> in following multiqueue patch.

Why then?

	--yliu

^ permalink raw reply

* Re: [PATCH v2 4/7] net/virtio_user: abstract virtio user backend ops
From: Yuanhan Liu @ 2016-12-26  6:41 UTC (permalink / raw)
  To: Jianfeng Tan; +Cc: dev, ferruh.yigit, cunming.liang
In-Reply-To: <1482477266-39199-5-git-send-email-jianfeng.tan@intel.com>

On Fri, Dec 23, 2016 at 07:14:23AM +0000, Jianfeng Tan wrote:
> +typedef int (*vhost_setup_t)(struct virtio_user_dev *dev);
> +typedef int (*vhost_send_request_t)(struct virtio_user_dev *dev,
> +				    enum vhost_user_request req,
> +				    void *arg);
> +typedef int (*vhost_enable_qp_t)(struct virtio_user_dev *dev,
> +				 uint16_t pair_idx,
> +				 int enable);

I will not bother to define new types if they are just gonna be used once.
No strong perference here, just a suggestion.

...
> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h b/drivers/net/virtio/virtio_user/virtio_user_dev.h
> index 28fc788..503a496 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
> @@ -37,9 +37,15 @@
>  #include <limits.h>
>  #include "../virtio_pci.h"
>  #include "../virtio_ring.h"
> +#include "vhost.h"
>  
>  struct virtio_user_dev {
> +	/* for vhost_user backend */
>  	int		vhostfd;
> +
> +	/* for vhost_kernel backend */
> +
> +	/* for both vhost_user and vhost_kernel */

By far (till this patch), vhost_kernel is not introduced at all, that I
will not mention it here. That said, I will leave this change to the patch
that actually enables vhost-kernel.

	--yliu

^ permalink raw reply

* Re: [PATCH v2 2/7] net/virtio_user: postpone DRIVER OK notification
From: Tan, Jianfeng @ 2016-12-26  6:55 UTC (permalink / raw)
  To: Yuanhan Liu; +Cc: dev@dpdk.org, Yigit, Ferruh, Liang, Cunming
In-Reply-To: <20161226062719.GA19288@yliu-dev.sh.intel.com>



> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Monday, December 26, 2016 2:27 PM
> To: Tan, Jianfeng
> Cc: dev@dpdk.org; Yigit, Ferruh; Liang, Cunming
> Subject: Re: [PATCH v2 2/7] net/virtio_user: postpone DRIVER OK notification
> 
> On Fri, Dec 23, 2016 at 07:14:21AM +0000, Jianfeng Tan wrote:
> > In driver probe phase, we obtain device information; and then virtio
> > driver will initialize device and stores info, like negotiated
> > features, in vhost_user layer; finally, vhost_user gets DRIVER_OK
> > notification from virtio driver, and then sync with backend device.
> >
> > Previously, DRIVER_OK could be sent twice: 1. when ether layer invokes
> > eth_device_init to initialize device; 2. when user configures it with
> > different configuration from that of previous.
> 
> Yes, but that's wrong. Now only 1) will be taken.

>From code in virtio_dev_configure() as below, if hw_ip_checksum or enable_lro is configured, the device will be reset and re-initialized.
        if (rxmode->hw_ip_checksum)
                req_features |= (1ULL << VIRTIO_NET_F_GUEST_CSUM);
        if (rxmode->enable_lro)
                req_features |=
                        (1ULL << VIRTIO_NET_F_GUEST_TSO4) |
                        (1ULL << VIRTIO_NET_F_GUEST_TSO6);

        /* if request features changed, reinit the device */
        if (req_features != hw->req_guest_features) {
                ret = virtio_init_device(dev, req_features);
                if (ret < 0)
                        return ret;
        }

> 
> > Since we can only depend on DRIVER_OK notification to sync with backend
> > device, we postpone it to virtio_dev_start when everything is settled.
> 
> I don't quite understand what you were going to fix here; you don't
> state it in the commit log after all. Normally, when everything is
> negotiated (when DRIVER_OK is set), we should not set it again, unless
> a reset has been happened.

I agree that DRIVER_OK should be set only if everything is settled. Under the case of hw_ip_checksum or enable_lro, a reset will be sent and the device will be re-initialized.

So my point is that since the configuration might be changed in dev_configure(), why not just send DRIVER_OK after everything is done.

> 
> If you look at QEMU, qemu will simply ignore it once vhost has already
> started.

It does not apply here. The configuration is changed, and it cannot be ignored.

Thanks,
Jianfeng

> 
> 	--yliu

^ permalink raw reply

* Re: [PATCH v2 3/7] net/virtio_user: move vhost user specific code
From: Tan, Jianfeng @ 2016-12-26  6:58 UTC (permalink / raw)
  To: Yuanhan Liu; +Cc: dev@dpdk.org, Yigit, Ferruh, Liang, Cunming
In-Reply-To: <20161226062821.GB19288@yliu-dev.sh.intel.com>



> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Monday, December 26, 2016 2:28 PM
> To: Tan, Jianfeng
> Cc: dev@dpdk.org; Yigit, Ferruh; Liang, Cunming
> Subject: Re: [PATCH v2 3/7] net/virtio_user: move vhost user specific code
> 
> On Fri, Dec 23, 2016 at 07:14:22AM +0000, Jianfeng Tan wrote:
> > To support vhost kernel as the backend of net_virtio_user in coming
> > patches, we move vhost_user specific structs and macros into
> > vhost_user.c, and only keep common definitions in vhost.h.
> 
> Good.
> 
> > Besides, remove VHOST_USER_MQ feature check, it will be added back
> > in following multiqueue patch.
> 
> Why then?

Only vhost user recognizes this feature bit, and vhost kernel does not. My intension is to put those vhost user specific code inside vhost user. And in fact, I forget to add it back. I should add it back in this patch.

Thanks,
Jianfeng
> 
> 	--yliu

^ permalink raw reply

* Re: [PATCH v2 5/7] net/virtio_user: add vhost kernel support
From: Yuanhan Liu @ 2016-12-26  7:44 UTC (permalink / raw)
  To: Jianfeng Tan; +Cc: dev, ferruh.yigit, cunming.liang
In-Reply-To: <1482477266-39199-6-git-send-email-jianfeng.tan@intel.com>

On Fri, Dec 23, 2016 at 07:14:24AM +0000, Jianfeng Tan wrote:
> + *   Copyright(c) 2010-2016 Intel Corporation. All rights reserved.
                     ^^^^^^^^^

I think it should be 2016.


> +/* By default, vhost kernel module allows 64 regions, but DPDK allows
> + * 256 segments. As a relief, below function merges those virtually
> + * adjacent memsegs into one region.
> + */
> +static struct vhost_memory_kernel *
> +prepare_vhost_memory_kernel(void)
> +{
> +	uint32_t i, j, k = 0;
> +	struct rte_memseg *seg;
> +	struct vhost_memory_region *mr;
> +	struct vhost_memory_kernel *vm;
> +
> +	vm = malloc(sizeof(struct vhost_memory_kernel) +
> +		    VHOST_KERNEL_MAX_REGIONS *
> +		    sizeof(struct vhost_memory_region));
> +
> +	for (i = 0; i < RTE_MAX_MEMSEG; ++i) {
> +		seg = &rte_eal_get_configuration()->mem_config->memseg[i];
> +		if (!seg->addr)
> +			break;
> +
> +		int new_region = 1;
> +
> +		for (j = 0; j < k; ++j) {
> +			mr = &vm->regions[j];
> +
> +			if (mr->userspace_addr + mr->memory_size ==
> +			    (uint64_t)seg->addr) {
> +				mr->memory_size += seg->len;
> +				new_region = 0;
> +				break;
> +			}
> +
> +			if ((uint64_t)seg->addr + seg->len ==
> +			    mr->userspace_addr) {
> +				mr->guest_phys_addr = (uint64_t)seg->addr;

Be careful, such cast would break 32 bit OS build.

> +static int
> +vhost_kernel_ioctl(struct virtio_user_dev *dev,
> +		   enum vhost_user_request req,
> +		   void *arg)
> +{
> +	int i, ret = -1;
> +	uint64_t req_kernel;
> +	struct vhost_memory_kernel *vm = NULL;
> +
> +	req_kernel = vhost_req_user_to_kernel[req];
> +
> +	if (req_kernel == VHOST_SET_MEM_TABLE) {
> +		vm = prepare_vhost_memory_kernel();
> +		if (!vm)
> +			return -1;
> +		arg = (void *)vm;
> +	}
> +
> +	/* Does not work when VIRTIO_F_IOMMU_PLATFORM now, why? */

Because this feature need the vhost IOTLB support from the device
emulation. Patches for QEMU hasn't been merged yet, but it has been
there for a while.

Since we don't have the support yet, for sure it won't work. But
I'm wondering why you have to disable it explicitly?

> +	if (req_kernel == VHOST_SET_FEATURES)
> +		*(uint64_t *)arg &= ~(1ULL << VIRTIO_F_IOMMU_PLATFORM);
> +
> +	for (i = 0; i < VHOST_KERNEL_MAX_QUEUES; ++i) {
> +		if (dev->vhostfds[i] < 0)
> +			continue;
> +
> +		ret = ioctl(dev->vhostfds[i], req_kernel, arg);
> +		if (ret < 0)
> +			break;
> +	}
> +
> +	if (vm)
> +		free(vm);
> +
> +	return ret;
> +}
> +
> +/**
> + * Set up environment to talk with a vhost kernel backend.
> + *
> + * @return
> + *   - (-1) if fail to set up;
> + *   - (>=0) if successful.
> + */
> +static int
> +vhost_kernel_setup(struct virtio_user_dev *dev)
> +{
> +	int vhostfd;
> +	uint32_t q;

Why not simply use 'i'?

> +static int
> +vhost_kernel_enable_queue_pair(struct virtio_user_dev *dev,
> +			       uint16_t pair_idx,
> +			       int enable)
> +{
> +	unsigned int features;

Better to rename it to "tap_features" or something? When I first saw
such name, I thought it was about vhost features.

	--yliu

^ permalink raw reply

* Re: [PATCH v2 6/7] net/virtio_user: enable offloading
From: Yuanhan Liu @ 2016-12-26  7:53 UTC (permalink / raw)
  To: Jianfeng Tan; +Cc: dev, ferruh.yigit, cunming.liang
In-Reply-To: <1482477266-39199-7-git-send-email-jianfeng.tan@intel.com>

On Fri, Dec 23, 2016 at 07:14:25AM +0000, Jianfeng Tan wrote:
> When used with vhost kernel backend, we can offload at both directions.
>   - From vhost kernel to virtio_user, the offload is enabled so that
>     DPDK app can trust the flow is checksum-correct; and if DPDK app
>     sends it through another port, the checksum needs to be
>     recalculated or offloaded. It also applies to TSO.
>   - From virtio_user to vhost_kernel, the offload is enabled so that
>     kernel can trust the flow is L4-checksum-correct, no need to verify
>     it; if kernel will consume it, DPDK app should make sure the
>     l3-checksum is correctly set.
> 
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> ---
>  drivers/net/virtio/virtio_user/vhost_kernel.c | 61 ++++++++++++++++++++++++++-
>  1 file changed, 59 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_user/vhost_kernel.c b/drivers/net/virtio/virtio_user/vhost_kernel.c
> index 8984c5c..fb3c454 100644
> --- a/drivers/net/virtio/virtio_user/vhost_kernel.c
> +++ b/drivers/net/virtio/virtio_user/vhost_kernel.c
> @@ -91,6 +91,13 @@ struct vhost_memory_kernel {
>  #define IFF_ATTACH_QUEUE 0x0200
>  #define IFF_DETACH_QUEUE 0x0400
>  
> +/* Features for GSO (TUNSETOFFLOAD). */
> +#define TUN_F_CSUM	0x01	/* You can hand me unchecksummed packets. */
> +#define TUN_F_TSO4	0x02	/* I can handle TSO for IPv4 packets */
> +#define TUN_F_TSO6	0x04	/* I can handle TSO for IPv6 packets */
> +#define TUN_F_TSO_ECN	0x08	/* I can handle TSO with ECN bits. */
> +#define TUN_F_UFO	0x10	/* I can handle UFO packets */
> +
>  /* Constants */
>  #define TUN_DEF_SNDBUF	(1ull << 20)
>  #define PATH_NET_TUN	"/dev/net/tun"
> @@ -173,6 +180,28 @@ prepare_vhost_memory_kernel(void)
>  	return vm;
>  }
>  
> +/* with below features, vhost kernel does not need to do the checksum and TSO,
> + * these info will be passed to virtio_user through virtio net header.
> + */
> +static const uint64_t guest_offloads_mask =

The typical way is to define it as macro?

> +	(1ULL << VIRTIO_NET_F_GUEST_CSUM) |
> +	(1ULL << VIRTIO_NET_F_GUEST_TSO4) |
> +	(1ULL << VIRTIO_NET_F_GUEST_TSO6) |
> +	(1ULL << VIRTIO_NET_F_GUEST_ECN)  |
> +	(1ULL << VIRTIO_NET_F_GUEST_UFO);
> +
> @@ -271,6 +314,12 @@ vhost_kernel_enable_queue_pair(struct virtio_user_dev *dev,
>  	int hdr_size;
>  	int vhostfd;
>  	int tapfd;
> +	unsigned int offload =
> +			TUN_F_CSUM |
> +			TUN_F_TSO4 |
> +			TUN_F_TSO6 |
> +			TUN_F_TSO_ECN |
> +			TUN_F_UFO;

Same here?

	--yliu

^ permalink raw reply

* Re: [PATCH v2 3/7] net/virtio_user: move vhost user specific code
From: Yuanhan Liu @ 2016-12-26  7:57 UTC (permalink / raw)
  To: Tan, Jianfeng; +Cc: dev@dpdk.org, Yigit, Ferruh, Liang, Cunming
In-Reply-To: <ED26CBA2FAD1BF48A8719AEF02201E36510FC802@SHSMSX103.ccr.corp.intel.com>

On Mon, Dec 26, 2016 at 06:58:58AM +0000, Tan, Jianfeng wrote:
> 
> 
> > -----Original Message-----
> > From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> > Sent: Monday, December 26, 2016 2:28 PM
> > To: Tan, Jianfeng
> > Cc: dev@dpdk.org; Yigit, Ferruh; Liang, Cunming
> > Subject: Re: [PATCH v2 3/7] net/virtio_user: move vhost user specific code
> > 
> > On Fri, Dec 23, 2016 at 07:14:22AM +0000, Jianfeng Tan wrote:
> > > To support vhost kernel as the backend of net_virtio_user in coming
> > > patches, we move vhost_user specific structs and macros into
> > > vhost_user.c, and only keep common definitions in vhost.h.
> > 
> > Good.
> > 
> > > Besides, remove VHOST_USER_MQ feature check, it will be added back
> > > in following multiqueue patch.
> > 
> > Why then?
> 
> Only vhost user recognizes this feature bit, and vhost kernel does not. My intension is to put those vhost user specific code inside vhost user.

That's okay. But why you have to remove it and then add it back?

	--yliu

> And in fact, I forget to add it back. I should add it back in this patch.
> 
> Thanks,
> Jianfeng
> > 
> > 	--yliu

^ permalink raw reply

* Re: [PATCH v2 2/7] net/virtio_user: postpone DRIVER OK notification
From: Yuanhan Liu @ 2016-12-26  8:02 UTC (permalink / raw)
  To: Tan, Jianfeng; +Cc: dev@dpdk.org, Yigit, Ferruh, Liang, Cunming
In-Reply-To: <ED26CBA2FAD1BF48A8719AEF02201E36510FC7B6@SHSMSX103.ccr.corp.intel.com>

On Mon, Dec 26, 2016 at 06:55:16AM +0000, Tan, Jianfeng wrote:
> 
> 
> > -----Original Message-----
> > From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> > Sent: Monday, December 26, 2016 2:27 PM
> > To: Tan, Jianfeng
> > Cc: dev@dpdk.org; Yigit, Ferruh; Liang, Cunming
> > Subject: Re: [PATCH v2 2/7] net/virtio_user: postpone DRIVER OK notification
> > 
> > On Fri, Dec 23, 2016 at 07:14:21AM +0000, Jianfeng Tan wrote:
> > > In driver probe phase, we obtain device information; and then virtio
> > > driver will initialize device and stores info, like negotiated
> > > features, in vhost_user layer; finally, vhost_user gets DRIVER_OK
> > > notification from virtio driver, and then sync with backend device.
> > >
> > > Previously, DRIVER_OK could be sent twice: 1. when ether layer invokes
> > > eth_device_init to initialize device; 2. when user configures it with
> > > different configuration from that of previous.
> > 
> > Yes, but that's wrong. Now only 1) will be taken.
> 
> >From code in virtio_dev_configure() as below, if hw_ip_checksum or enable_lro is configured, the device will be reset and re-initialized.
>         if (rxmode->hw_ip_checksum)
>                 req_features |= (1ULL << VIRTIO_NET_F_GUEST_CSUM);
>         if (rxmode->enable_lro)
>                 req_features |=
>                         (1ULL << VIRTIO_NET_F_GUEST_TSO4) |
>                         (1ULL << VIRTIO_NET_F_GUEST_TSO6);
> 
>         /* if request features changed, reinit the device */
>         if (req_features != hw->req_guest_features) {
>                 ret = virtio_init_device(dev, req_features);
>                 if (ret < 0)
>                         return ret;
>         }

Yes, I know. But virtio_init_device() will call vtpci_reinit_complete()
at the end, doesn't it?

> > > Since we can only depend on DRIVER_OK notification to sync with backend
> > > device, we postpone it to virtio_dev_start when everything is settled.
> > 
> > I don't quite understand what you were going to fix here; you don't
> > state it in the commit log after all. Normally, when everything is
> > negotiated (when DRIVER_OK is set), we should not set it again, unless
> > a reset has been happened.
> 
> I agree that DRIVER_OK should be set only if everything is settled. Under the case of hw_ip_checksum or enable_lro, a reset will be sent and the device will be re-initialized.
> 
> So my point is that since the configuration might be changed in dev_configure(), why not just send DRIVER_OK after everything is done.
> 
> > 
> > If you look at QEMU, qemu will simply ignore it once vhost has already
> > started.
> 
> It does not apply here. The configuration is changed, and it cannot be ignored.

Why it doesn't apply here? What makes virtio_user special? If it works
to QEMU and it doesn't to virtio_user, it basically means the virtio_user
is broken ...

	--yliu

^ permalink raw reply

* Re: [PATCH v2 2/7] net/virtio_user: postpone DRIVER OK notification
From: Tan, Jianfeng @ 2016-12-26  8:26 UTC (permalink / raw)
  To: Yuanhan Liu; +Cc: dev@dpdk.org, Yigit, Ferruh, Liang, Cunming
In-Reply-To: <20161226080208.GG19288@yliu-dev.sh.intel.com>

As discussed offline, this will lead to many DRIVER OK among dev_stop()/dev_start(). I'll remove this patch.

Thanks,
Jianfeng

> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Monday, December 26, 2016 4:02 PM
> To: Tan, Jianfeng
> Cc: dev@dpdk.org; Yigit, Ferruh; Liang, Cunming
> Subject: Re: [PATCH v2 2/7] net/virtio_user: postpone DRIVER OK notification
> 
> On Mon, Dec 26, 2016 at 06:55:16AM +0000, Tan, Jianfeng wrote:
> >
> >
> > > -----Original Message-----
> > > From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> > > Sent: Monday, December 26, 2016 2:27 PM
> > > To: Tan, Jianfeng
> > > Cc: dev@dpdk.org; Yigit, Ferruh; Liang, Cunming
> > > Subject: Re: [PATCH v2 2/7] net/virtio_user: postpone DRIVER OK
> notification
> > >
> > > On Fri, Dec 23, 2016 at 07:14:21AM +0000, Jianfeng Tan wrote:
> > > > In driver probe phase, we obtain device information; and then virtio
> > > > driver will initialize device and stores info, like negotiated
> > > > features, in vhost_user layer; finally, vhost_user gets DRIVER_OK
> > > > notification from virtio driver, and then sync with backend device.
> > > >
> > > > Previously, DRIVER_OK could be sent twice: 1. when ether layer
> invokes
> > > > eth_device_init to initialize device; 2. when user configures it with
> > > > different configuration from that of previous.
> > >
> > > Yes, but that's wrong. Now only 1) will be taken.
> >
> > >From code in virtio_dev_configure() as below, if hw_ip_checksum or
> enable_lro is configured, the device will be reset and re-initialized.
> >         if (rxmode->hw_ip_checksum)
> >                 req_features |= (1ULL << VIRTIO_NET_F_GUEST_CSUM);
> >         if (rxmode->enable_lro)
> >                 req_features |=
> >                         (1ULL << VIRTIO_NET_F_GUEST_TSO4) |
> >                         (1ULL << VIRTIO_NET_F_GUEST_TSO6);
> >
> >         /* if request features changed, reinit the device */
> >         if (req_features != hw->req_guest_features) {
> >                 ret = virtio_init_device(dev, req_features);
> >                 if (ret < 0)
> >                         return ret;
> >         }
> 
> Yes, I know. But virtio_init_device() will call vtpci_reinit_complete()
> at the end, doesn't it?
> 
> > > > Since we can only depend on DRIVER_OK notification to sync with
> backend
> > > > device, we postpone it to virtio_dev_start when everything is settled.
> > >
> > > I don't quite understand what you were going to fix here; you don't
> > > state it in the commit log after all. Normally, when everything is
> > > negotiated (when DRIVER_OK is set), we should not set it again, unless
> > > a reset has been happened.
> >
> > I agree that DRIVER_OK should be set only if everything is settled. Under
> the case of hw_ip_checksum or enable_lro, a reset will be sent and the
> device will be re-initialized.
> >
> > So my point is that since the configuration might be changed in
> dev_configure(), why not just send DRIVER_OK after everything is done.
> >
> > >
> > > If you look at QEMU, qemu will simply ignore it once vhost has already
> > > started.
> >
> > It does not apply here. The configuration is changed, and it cannot be
> ignored.
> 
> Why it doesn't apply here? What makes virtio_user special? If it works
> to QEMU and it doesn't to virtio_user, it basically means the virtio_user
> is broken ...
> 
> 	--yliu

^ permalink raw reply

* Re: [PATCH v1] net/i40e: set no drop for traffic class
From: Wu, Jingjing @ 2016-12-26  8:45 UTC (permalink / raw)
  To: Sexton, Rory; +Cc: dev@dpdk.org, Marjanovic, Nemanja, Mcnamara, John
In-Reply-To: <EAC303C93824C94DB804849C22B752B30D2B20A0@IRSMSX104.ger.corp.intel.com>



> -----Original Message-----
> From: Sexton, Rory
> Sent: Friday, December 9, 2016 10:03 PM
> To: Wu, Jingjing <jingjing.wu@intel.com>
> Cc: dev@dpdk.org; Marjanovic, Nemanja <nemanja.marjanovic@intel.com>;
> Mcnamara, John <john.mcnamara@intel.com>
> Subject: RE: [PATCH v1] net/i40e: set no drop for traffic class
> 
> Hi Jingjing,
> 
> Yes PRTDCB_TC2PFC is used to control pfc for each TC however we have
> noticed other advantages of using the register.
> By setting the register explicitly by doing the "I40E_WRITE_REG(hw, 0x1c0980,
> 0xff);" it allows for packets to be temporarily stored on the NICs RX SRAM
> until there is space for them on SW descriptor ring versus dropping them
> when the SW ring becomes full. This also allows for larger burst handling. It
> also means SW doesn't have to be as quick to empty the DRAM based
> descriptor rings, allowing more processing on cores.
> 
> I have tested using the ETH_DCB_PFC_SUPPORT flag in
> rte_eth_conf.dcb_capability_en and rte_eth_dcb_rx_conf.nb_tcs.
> This results in the NIC's RX SRAM not being used and if there is no space on
> SW descriptor ring for packet it is dropped.

Besides ETH_DCB_PFC_SUPPORT, ETH_MQ_RX_DCB_FLAG is also required in
dev_conf.rxmode.mq_mode. After doing that, you will find register PRTDCB_TC2PFC
is also changed.
 
If you don't want to enable DCB, why not just implement that function "i40e_priority_flow_ctrl_set"?
You can change the register in this function without define a new API.


> The advantages of using the PRTDCB_TC2PFC explicitly is that there will be no
> packet loss and descriptor rings do not need to be modified (can be left at
> 128 for rx and 512 for tx as default settings for apps).  Enabling via this
> register allows Burst handling to be within the NIC Rx Buffer and SW rings
> combined.
> At the moment for tests the rx and tx descriptor rings have to be increased
> to 2048 to eliminate packet loss.
> 
> Ideally it would be an optional setting as using it may increase the max
> latency.
> 


Thanks for the clarification.

Thanks
Jingjing

^ permalink raw reply

* Re: [PATCH v2 12/12] drivers: update PMDs to use rte_driver probe and remove
From: Shreyansh Jain @ 2016-12-26  9:14 UTC (permalink / raw)
  To: Jan Blunck
  Cc: dev, David Marchand, Thomas Monjalon, Ferruh Yigit, jianbo.liu,
	Thomas Monjalon
In-Reply-To: <CALe+Z01W-GP0WiCBMvY60iv0sMd2jZMAuZRt2SvoWw+bJ7ZODw@mail.gmail.com>

On Friday 16 December 2016 03:06 AM, Jan Blunck wrote:
> On Wed, Dec 14, 2016 at 10:49 AM, Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
>> On Tuesday 13 December 2016 07:07 PM, Shreyansh Jain wrote:
>>>
>>> These callbacks now act as first layer of PCI interfaces from the Bus.
>>> Bus probe would enter the PMDs through the rte_driver->probe/remove
>>> callbacks, falling to rte_xxx_driver->probe/remove (Currently, all the
>>> drivers are rte_pci_driver).
>>>
>>> Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
>>> ---
>>>  drivers/net/bnx2x/bnx2x_ethdev.c        | 8 ++++++++
>>>  drivers/net/bnxt/bnxt_ethdev.c          | 4 ++++
>>>  drivers/net/cxgbe/cxgbe_ethdev.c        | 4 ++++
>>>  drivers/net/e1000/em_ethdev.c           | 4 ++++
>>>  drivers/net/e1000/igb_ethdev.c          | 8 ++++++++
>>>  drivers/net/ena/ena_ethdev.c            | 4 ++++
>>>  drivers/net/enic/enic_ethdev.c          | 4 ++++
>>>  drivers/net/fm10k/fm10k_ethdev.c        | 4 ++++
>>>  drivers/net/i40e/i40e_ethdev.c          | 4 ++++
>>>  drivers/net/i40e/i40e_ethdev_vf.c       | 4 ++++
>>>  drivers/net/ixgbe/ixgbe_ethdev.c        | 8 ++++++++
>>>  drivers/net/mlx4/mlx4.c                 | 4 +++-
>>>  drivers/net/mlx5/mlx5.c                 | 1 +
>>>  drivers/net/nfp/nfp_net.c               | 4 ++++
>>>  drivers/net/qede/qede_ethdev.c          | 8 ++++++++
>>>  drivers/net/szedata2/rte_eth_szedata2.c | 4 ++++
>>>  drivers/net/thunderx/nicvf_ethdev.c     | 4 ++++
>>>  drivers/net/virtio/virtio_ethdev.c      | 2 ++
>>>  drivers/net/vmxnet3/vmxnet3_ethdev.c    | 4 ++++
>>>  19 files changed, 86 insertions(+), 1 deletion(-)
>>>
>> <snip>
>>
>> drivers/crypto/qat/rte_qat_cryptodev.c should also be changed for this. It
>> seems to be only PCI registered PMD. All others are VDEV.
>>
>> I will send a v3 soon to fix this.
>>
>> @Jan, would you be looking in the VDEV part or should I start with that? [1]
>>
>
> Yes, I am doing that. Will send out early next week.

Do you think following model will help you for VDEV devices?

--->8--
Each bus has following methods:
  .probe(...)
  .remove(...)
  .attach(const char *name)
  .detach(const char *name)
--->8---

Each bus, in this case VDEV bus, would implement these (attach/detach
being optional).
  - For rte_eal_dev_attach, 'attach()' of each bus would be called and
    name of the devices passed to it.
  - Bus->attach() would search for the device name (in case of PCI, BDF)
    and call appropriate initialization routine.
  - Search over bus stops as soon as any bus confirms a successful attach

This is what I was thinking for VDEV:
1. Expose the attach/detach functions in eal_common_bus
2. Define VDEV bus
3. Rather than probe/remove, VDEV implements attach/detach.
    Rationale for this is that VDEV don't really have a bus and neither
    is a probe/remove natural for them. They are 'attached'/'detached'.

Let me know if this helps.
I can provide you base patches over bus series to enable this (I have it 
ready without much testing).

>
> Thx,
> Jan
>
>> [1] http://dpdk.org/ml/archives/dev/2016-November/050443.html
>>
>

-
Shreyansh

^ permalink raw reply

* Re: [PATCH v5 00/29] Support VFD and DPDK PF + kernel VF on i40e
From: Chen, Jing D @ 2016-12-26 11:59 UTC (permalink / raw)
  To: Vincent JARDIN
  Cc: Thomas Monjalon, dev@dpdk.org, Yigit, Ferruh, Wu, Jingjing,
	Zhang, Helin
In-Reply-To: <7b981a32-9837-5e5c-dc0d-3c4781341aeb@6wind.com>

Vincent,

Sorry, I missed this reply.

> 
> Le 22/12/2016 à 09:10, Chen, Jing D a écrit :
> > In the meanwhile, we have some test models ongoing to validate
> > combination of Linux and DPDK drivers for VF and PF. We'll fully support
> below 4 cases going forward.
> > 1. DPDK PF + DPDK VF
> > 2. DPDK PF + Linux VF
> 
> + DPDK PF + FreeBSD VF
> + DPDK PF + Windows VF
> + DPDK PF + OS xyz VF
> 

If all drivers follow same API spec, what's the problem here?
What extra DPDK PF effort you observed?

> > 3. Linux PF + DPDK VF
> > 4. Linux PF + Linux VF (it's not our scope)
> 
> So, you confirm the issue: having DPDK becoming a PF, even if SRIOV protocol
> includes version-ing, it doubles the combinatory cases.

If extended functions are needed, the answer is yes.
That's not a big problem, right? I have several workarounds/approaches to
support extended funcs while following original API spec. I can fix it in this
release. In order to have a mature solution, I left it here for further implementation.

> 
> >
> > After applied this patch, i've done below test without observing
> compatibility issue.
> > 1. DPDK PF + DPDK VF (middle of 16.11 and 17.02 code base). PF to support
> API 1.0 while VF
> >     to support API 1.1/1.0
> > 2. DPDK PF + Linux VF 1.5.14. PF to support 1.0, while Linux to
> > support 1.1/1.0
> >
> > Linux PF + DPDK VF has been tested with 1.0 API long time ago. There
> > is some test activities ongoing.
> >
> > Finally, please give strong reasons to support your NAC.
> 
> I feel bad because I do recognize the strong and hard work that you have done
> on this PF development, but I feel we need first to assess if DPDK should
> become a PF or not. I know ixgbe did open the path and that they are some
> historical DPDK PF supports in Intel NICs, but before we generalize it, we have
> to make sure we are not turning this DataPlane development Kit into a
> ControlPlane Driver Kit that we are scared to upstream into Linux kernel. Even
> if "DPDK is not Linux", it does not mean that Linux should be ignored. In case
> of DPDK on other OS, same, their PF could be extended too.
> 

Thanks for the recognition of our work on PF driver. :)

> So currently, yes, I do keep a nack't
> 
> Since DPDK PF features can be into Linux PF features too and since Linux (and
> other hypervisors) has already some tools to manage PF (see iproute2, etc.),
> why should we have an other management path with DPDK?
> DPDK is aimed to be a Dataplane Development kit, not a management/control
> plane driver kit.

Before we debated on Dataplane and ControPlane, can you answer me a question,
why we have generic filter API? Is it a API for dataplane?

I can't imagine that we'll have to say 'you need to use Linux PF' driver when users
want to deploy PF + VF cases. Why we can't provide an alternative option. they are not
exclusive and users can decide which combination is better for them. 
The reason why we developed DPDK PF host driver is we have requirements from
users. Our motivation is simple, there are requirements, we satisfy them.

Sorry, you NACK can't convince me.

> 
> Assuming you want to use DPDK PF for dataplane feature, that could be OK
> then, using:
>    - configure one VF on the hypervisor from Linux's PF, let's name if
> VF_forPFtraffic, see http://dpdk.org/doc/guides/howto/flow_bifurcation.html
>    - have no (or few IO)s to the PF's queue
>    - assign the traffic to all VF_forPFtraffic's queues of the hypervisor,
>    - run DPDK into the hypervisor's VF_forPFtraffic
> 
> Doing so, we get the same benefit of running DPDK over PF or running DPDK
> over VF_forPFtraffic, don't we? It is a benefit of:
>    http://dpdk.org/doc/guides/howto/flow_bifurcation.html
> 
> Thank you,
>    Vincent
> 

^ permalink raw reply

* Re: [PATCH 0/4] enhancement to i40e PF host driver
From: Chen, Jing D @ 2016-12-26 12:17 UTC (permalink / raw)
  To: Vincent JARDIN, dev@dpdk.org; +Cc: Yigit, Ferruh, Wu, Jingjing
In-Reply-To: <64ddfd77-e71e-298e-687a-a9991f8c64ee@6wind.com>

Hi, Vincent,

Since original patch set are split into 2 different patches, can
we discuss in this thread?
Attach original discussion below. Sorry for top-post.

> 
> Le 22/12/2016 à 09:10, Chen, Jing D a écrit :
> > In the meanwhile, we have some test models ongoing to validate 
> > combination of Linux and DPDK drivers for VF and PF. We'll fully 
> > support
> below 4 cases going forward.
> > 1. DPDK PF + DPDK VF
> > 2. DPDK PF + Linux VF
> 
> + DPDK PF + FreeBSD VF
> + DPDK PF + Windows VF
> + DPDK PF + OS xyz VF
> 

If all drivers follow same API spec, what's the problem here?
What extra DPDK PF effort you observed?

> > 3. Linux PF + DPDK VF
> > 4. Linux PF + Linux VF (it's not our scope)
> 
> So, you confirm the issue: having DPDK becoming a PF, even if SRIOV 
> protocol includes version-ing, it doubles the combinatory cases.

If extended functions are needed, the answer is yes.
That's not a big problem, right? I have several workarounds/approaches to support extended funcs while following original API spec. I can fix it in this release. In order to have a mature solution, I left it here for further implementation.

> 
> >
> > After applied this patch, i've done below test without observing
> compatibility issue.
> > 1. DPDK PF + DPDK VF (middle of 16.11 and 17.02 code base). PF to 
> > support
> API 1.0 while VF
> >     to support API 1.1/1.0
> > 2. DPDK PF + Linux VF 1.5.14. PF to support 1.0, while Linux to 
> > support 1.1/1.0
> >
> > Linux PF + DPDK VF has been tested with 1.0 API long time ago. There 
> > is some test activities ongoing.
> >
> > Finally, please give strong reasons to support your NAC.
> 
> I feel bad because I do recognize the strong and hard work that you 
> have done on this PF development, but I feel we need first to assess 
> if DPDK should become a PF or not. I know ixgbe did open the path and 
> that they are some historical DPDK PF supports in Intel NICs, but 
> before we generalize it, we have to make sure we are not turning this 
> DataPlane development Kit into a ControlPlane Driver Kit that we are 
> scared to upstream into Linux kernel. Even if "DPDK is not Linux", it 
> does not mean that Linux should be ignored. In case of DPDK on other OS, same, their PF could be extended too.
> 

Thanks for the recognition of our work on PF driver. :)

> So currently, yes, I do keep a nack't
> 
> Since DPDK PF features can be into Linux PF features too and since 
> Linux (and other hypervisors) has already some tools to manage PF (see 
> iproute2, etc.), why should we have an other management path with DPDK?
> DPDK is aimed to be a Dataplane Development kit, not a 
> management/control plane driver kit.

Before we debated on Dataplane and ControPlane, can you answer me a question, why we have generic filter API? Is it a API for dataplane?

I can't imagine that we'll have to say 'you need to use Linux PF' driver when users want to deploy PF + VF cases. Why we can't provide an alternative option. they are not exclusive and users can decide which combination is better for them. 
The reason why we developed DPDK PF host driver is we have requirements from users. Our motivation is simple, there are requirements, we satisfy them.

Sorry, you NACK can't convince me.

> 
> Assuming you want to use DPDK PF for dataplane feature, that could be 
> OK then, using:
>    - configure one VF on the hypervisor from Linux's PF, let's name if 
> VF_forPFtraffic, see http://dpdk.org/doc/guides/howto/flow_bifurcation.html
>    - have no (or few IO)s to the PF's queue
>    - assign the traffic to all VF_forPFtraffic's queues of the hypervisor,
>    - run DPDK into the hypervisor's VF_forPFtraffic
> 
> Doing so, we get the same benefit of running DPDK over PF or running 
> DPDK over VF_forPFtraffic, don't we? It is a benefit of:
>    http://dpdk.org/doc/guides/howto/flow_bifurcation.html
> 
> Thank you,
>    Vincent
>

> -----Original Message-----
> From: Vincent JARDIN [mailto:vincent.jardin@6wind.com]
> Sent: Friday, December 23, 2016 8:53 PM
> To: Chen, Jing D <jing.d.chen@intel.com>; dev@dpdk.org
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>
> Subject: Re: [PATCH 0/4] enhancement to i40e PF host driver
> 
> Thanks for this update.
> 
> Still, I would like to get good arguments why DPDK should be a PF because it
> creates fragmentations. Please, first, let's reply to:
>    http://dpdk.org/ml/archives/dev/2016-December/053107.html
> 
> Having both Linux and DPDK being PF, it means that we double the
> combinations so we double the issues.
> 
> The following should be used instead: assuming you want to use DPDK PF for
> dataplane feature, an alternative is,
>     - configure one VF on the hypervisor from Linux's PF, let's name if
> VF_forPFtraffic, see
>          http://dpdk.org/doc/guides/howto/flow_bifurcation.html
>     - have no (or few IOs) to the PF's queue
>     - assign the traffic to all VF_forPFtraffic's queues of the hypervisor,
>     - run DPDK into the hypervisor's VF_forPFtraffic
> 
> Thank you,
>    Vincent

^ permalink raw reply

* Re: [PATCH v3 3/4] net/mlx5: add rte_flow rule creation
From: Nélio Laranjeiro @ 2016-12-26 12:20 UTC (permalink / raw)
  To: Ferruh Yigit; +Cc: dev, Adrien Mazarguil
In-Reply-To: <1f1d914a-143b-94cd-9ade-f09e84c05df3@intel.com>

On Fri, Dec 23, 2016 at 12:21:10PM +0000, Ferruh Yigit wrote:
> On 12/21/2016 3:19 PM, Nelio Laranjeiro wrote:
> > Convert Ethernet, IPv4, IPv6, TCP, UDP layers into ibv_flow and create
> > those rules when after validation (i.e. NIC supports the rule).
> > 
> > VLAN is still not supported in this commit.
> > 
> > Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> 
> <...>
> 
> > +static struct rte_flow *
> > +priv_flow_create(struct priv *priv,
> > +		 const struct rte_flow_attr *attr,
> > +		 const struct rte_flow_item items[],
> > +		 const struct rte_flow_action actions[],
> > +		 struct rte_flow_error *error)
> > +{
> > +	struct rte_flow *rte_flow = NULL;
> 
> Unnecessary assignment.
> 
> <...>
> 
> > +	action = (struct action) {
> > +		.queue = -1,
> > +		.drop = 0,
> > +	};
> > +	for (; actions->type != RTE_FLOW_ACTION_TYPE_END; ++actions) {
> > +		if (actions->type == RTE_FLOW_ACTION_TYPE_VOID) {
> > +			continue;
> > +		} else if (actions->type == RTE_FLOW_ACTION_TYPE_QUEUE) {
> > +			action.queue = ((const struct rte_flow_action_queue *)
> > +					actions->conf)->index;
> > +		} else if (actions->type == RTE_FLOW_ACTION_TYPE_DROP) {
> > +			action.drop = 1;
> > +		} else {
> > +			rte_flow_error_set(error, ENOTSUP,
> > +					   RTE_FLOW_ERROR_TYPE_ACTION,
> > +					   actions, "unsupported action");
> > +			goto exit;
> > +		}
> > +	}
> > +	if (action.queue >= 0) {
> > +		queue = action.queue;
> > +	} else if (action.drop) {
> > +		queue = MLX5_FLOW_DROP_QUEUE;
> > +	} else {
> 
> Not really so important, but as a note, ACTION_TYPE_VOID hits here. It
> pass from validation, but gives error in creation.
> 
> > +		rte_flow_error_set(error, ENOTSUP,
> > +				   RTE_FLOW_ERROR_TYPE_ACTION,
> > +				   actions,
> > +				   "no possible action found");
> > +		goto exit;
> > +	}
> 
> <...>

Hi Ferruh,

I will send (very soon) a v4 to handle this situation.

Regards,

-- 
Nélio Laranjeiro
6WIND

^ permalink raw reply

* [PATCH v4 00/12] Introducing EAL Bus-Device-Driver Model
From: Shreyansh Jain @ 2016-12-26 12:50 UTC (permalink / raw)
  To: david.marchand; +Cc: dev, thomas.monjalon, Shreyansh Jain
In-Reply-To: <1481893853-31790-1-git-send-email-shreyansh.jain@nxp.com>

Link to v1: [10]
Link to v2: [11]
Link to v3: [13]

:: Introduction ::

DPDK has been inherently a PCI inclined framework. Because of this, the
design of device tree (or list) within DPDK is also PCI inclined. A
non-PCI device doesn't have a way of being expressed without using hooks
started from EAL to PMD.

(Check 'Version Changes' section for changes)

:: Overview of the Proposed Changes ::

Assuming the below graph for a computing node:

        device A1
          |
  +==.===='==============.============+ Bus A.
     |                    `--> driver A11     \
  device A2                `-> driver A12      \______
                                                |CPU |
                                                /`````
        device B1                              /
          |                                   /
  +==.===='==============.============+ Bus B`
     |                    `--> driver B11
  device B2                `-> driver B12


 - One or more buses are connected to a CPU (or core)
 - One or more devices are conneted to a Bus
 - Drivers are running instances which manage one or more devices
 - Bus is responsible for identifying devices (and interrupt propogation)
 - Driver is responsible for initializing the device

In context of DPDK EAL:
 - rte_bus, represents a Bus. An implementation of a physical bus would
   instantiate this class.
 - Buses are registered just like a PMD - RTE_REGISTER_BUS()
   `- Thus, implementation for PCI would instantiate a rte_bus, give it a
      name and provide scan/match hooks.
    - Currently, priority of RTE_REGISTER_BUS constructor has been set to
      101 to make sure bus is registered *before* drivers are.
 - Each registered bus is part of a doubly list.
   -- Each device refers to rte_bus on which it belongs
   -- Each driver refers to rte_bus with which it is associated
   -- Device and Drivers lists are part of rte_bus
   -- NO global device/driver list would exist
 - When a PMD wants to register itself, it would 'add' itself to an
   existing bus. Which essentially converts to adding the driver to
   a bus specific driver_list.
 - Bus would perform a scan and 'add' devices scanned to its list.
 - Bus would perform a probe and link devices and drivers on each bus and
   invoking a series of probes
   `-- There are some parallel work for combining scan/probe in EAL [5]
       and also for doing away with a independent scan function all
       together [6].


The view would be almost like:

                                  __ rte_bus_list
                                 /
                     +----------'---+
                     |rte_bus       |
                     | driver_list------> device_list for this bus
                     | device_list----    
                     | scan()       | `-> driver_list for this bus
                     | match()      |
                     | probe()      |
                     |              |
                     +--|------|----+
              _________/        \_________
    +--------/----+                     +-\---------------+
    |rte_device   |                     |rte_driver       |
    | *rte_bus    |                     | *rte_bus        |
    | rte_driver  |                     | probe()         |
    |             |                     | remove()        |
    |  devargs    |                     |                 |
    +---||--------+                     +---------|||-----+
        ||                                        '''      
        | \                                        \\\
        |  \_____________                           \\\
        |                \                          |||
 +------|---------+ +----|----------+               |||
 |rte_pci_device  | |rte_xxx_device |               |||
 | PCI specific   | | xxx device    |               |||
 | info (mem,)    | | specific fns  |              / | \
 +----------------+ +---------------+             /  |  \
                            _____________________/  /    \
                           /                    ___/      \
            +-------------'--+    +------------'---+    +--'------------+
            |rte_pci_driver  |    |rte_vdev_driver |    |rte_xxx_driver |
            | PCI id table,  |    | <probably,     |    | ....          |
            | other driver   |    |  nothing>      |    +---------------+
            | data           |    |  ...           |
            |  probe()       |    +----------------+
            |  remove()      |
            +----------------+

In continuation to the RFC posted on 17/Nov [9],
A series of patches is being posted which attempts to create:
 1. A basic bus model
    `- define rte_bus and associated methods/helpers
    `- test infrastructure to test the Bus infra
 2. Changes in EAL to support PCI as a bus
    `- a "pci" bus is registered
    `- existing scan/match/probe are modified to allow for bus integration
    `- PCI Device and Driver list, which were global entities, have been
       moved to rte_bus->[device/driver]_list

:: Brief about Patch Layout ::

0001~0002: Introducing the basic Bus model and associated test case
0003~0004: Add scan, match and insert support for devices on bus
0005:      Add probe and remove for rte_driver
0006:      Enable probing of PCI Bus (devices) from EAL
0007:      Split the existing PCI probe into match and probe
0008:      Make PCI probe/match work on rte_driver/device rather than
           rte_pci_device/rte_pci_driver
0009:      Patch from Ben [8], part of series [2]
0010:      Enable Scan/Match/probe on Bus from EAL and remove unused
           functions and lists. PMDs still don't work (in fact, PCI PMD
           don't work after this patch - but without any compilation
           issues). Also, fix PCI test framework to reflect the bus
           integration.
0011:      Change PMDs to integrate with PCI bus
0012:      Introduce helper macros for iteration over bus resources

:: Pending Changes/Caveats ::

1. One of the major changes pending, as against proposed in RFC, is the
   removal of eth_driver.
   Being a large change, and independent one, this would be done in a
   separate series of patches.
   - some patches to support this have already been merged into master

2. This patchset only moves the PCI into a bus. And, that movement is also
   currently part of the EAL (lib/librte_eal/linux)
   - there was an open question in RFC about where to place the PCI bus
     instance - whether in drivers/bus/... or in lib/librte_bus/... or
     lib/librte_eal/...; This patch uses the last option. But, movement
     only impacts placement of Makefiles.
   - It also impacts the point (5) about priority use in constructor

3. Though the implementation for bus is common for Linux and BSD, the PCI
   bus implementation has been done/tested only for Linux.

4. The overall layout for driver probing has changed a little.
   earlier, it was:
    rte_eal_init()
     `-> rte_eal_pci_probe() (and parallel for VDEV)
         `-> rte_pci_driver->probe()
             `-> eth_driver->eth_dev_init()

   now, it would be:
   rte_eal_init()
     `-> rte_eal_bus_probe() <- Iterator for PCI device/driver
         `-> rte_driver->probe() <- devargs handling
             |                      old rte_eal_pci_probe()
             `-> rte_xxx_driver->probe() <- eth_dev allocation
                 `-> eth_driver->eth_dev_init <- eth_dev init

   Open Questions:
       Also, rte_driver->probe() creating eth_dev certainly sounds a little
       wrong - but, I would like to get your opinion on how to lay this
       order of which layer ethernet device corresponds to.
       1) Which layer should allocate eth_dev?
          `-> My take: rte_driver->probe()
       2) which layer should fill the eth_dev?
          `-> My take: rte_xxx_driver->probe()
       3) Is init/uninit better name for rte_xxx_driver()->probe() if all
          they do is initialize the ethernet device?

 5. RTE_REGISTER_BUS has been declared with contructor priority of 101
    It is important that Bus is registered *before* drivers are registered.
    Only way I could find to assure that was via 
    __attribute(contructor(priority)) of GCC. I am not sure how it would
    behave on other compilers. Any suggestions?
    - One suggestion from David Marchand was to use global bus object
      handles, which I have not implemented for now. If that is common
      choice, I will change in v3.

:: ToDo list ::

 - Bump to librte_eal version
 - Documentation continues to have references to some _old_ PCI symbols
 - vdev changes
 - eth_device, eth_driver changes

:: References ::

[1] http://dpdk.org/ml/archives/dev/2016-November/050186.html
[2] http://dpdk.org/ml/archives/dev/2016-November/050622.html
[3] http://dpdk.org/ml/archives/dev/2016-November/050416.html
[4] http://dpdk.org/ml/archives/dev/2016-November/050567.html
[5] http://dpdk.org/ml/archives/dev/2016-November/050628.html
[6] http://dpdk.org/ml/archives/dev/2016-November/050415.html
[7] http://dpdk.org/ml/archives/dev/2016-November/050443.html
[8] http://dpdk.org/ml/archives/dev/2016-November/050624.html
[9] http://dpdk.org/ml/archives/dev/2016-November/050296.html
[10] http://dpdk.org/ml/archives/dev/2016-December/051349.html
[12] http://dpdk.org/ml/archives/dev/2016-December/052092.html
[13] http://dpdk.org/ml/archives/dev/2016-December/052381.html

:: Version Changes ::
v4:
 - rebase over master (eac901ce)
 - Fix a bug in test_bus while setup and cleanup
 - rename rte_eal_get_bus to rte_eal_bus_get
 - Add helper (iterator) macros for easy bus,device,driver traversal
 - removed 0001 patch as it is already merged in master
 - fix missing rte_eal_bus_insert_device symbol in map file

v3:
 - rebase over master (c431384c8f)
 - revert patch 0001 changes for checkpatch (container_of macro)
 - qat/rte_qat_cryptodev update for rte_driver->probe
 - test_pci update for using a test_pci_bus for verification
 - some bug fixes based on internal testing.
 -- rte_eal_dev_attach not handling devargs
 -- blacklisting not working

v2:
 - No more bus->probe()
   Now, rte_eal_bus_probe() calls rte_driver->probe based on match output
 - new functions, rte_eal_pci_probe and rte_eal_pci_remove have been
   added as glue code between PCI PMDs and PCI Bus
   `-> PMDs are updated to use these new functions as callbacks for
       rte_driver
 - 'default' keyword has been removed from match and scan
 - Fix for incorrect changes in mlx* and nicvf*
 - Checkpatch fixes
 - Some variable checks have been removed from internal functions;
   functions which are externally visible continue to have such checks
 - Some rearrangement of patches:
   -- changes to drivers have been separated from EAL changes (but this
      does make PCI PMDs non-working for a particular patch)

Ben Walker (1):
  pci: Pass rte_pci_addr to functions instead of separate args

Shreyansh Jain (11):
  eal/bus: introduce bus abstraction
  test: add basic bus infrastructure tests
  eal/bus: add scan, match and insert support
  eal: integrate bus scan and probe with EAL
  eal: add probe and remove support for rte_driver
  eal: enable probe from bus infrastructure
  pci: split match and probe function
  eal/pci: generalize args of PCI scan/match towards RTE device/driver
  eal: enable PCI bus and PCI test framework
  drivers: update PMDs to use rte_driver probe and remove
  eal/bus: add bus iteration macros

 app/test/Makefile                               |   2 +-
 app/test/test.h                                 |   2 +
 app/test/test_bus.c                             | 689 ++++++++++++++++++++++++
 app/test/test_pci.c                             | 154 ++++--
 drivers/crypto/qat/rte_qat_cryptodev.c          |   4 +
 drivers/net/bnx2x/bnx2x_ethdev.c                |   8 +
 drivers/net/bnxt/bnxt_ethdev.c                  |   4 +
 drivers/net/cxgbe/cxgbe_ethdev.c                |   4 +
 drivers/net/e1000/em_ethdev.c                   |   4 +
 drivers/net/e1000/igb_ethdev.c                  |   8 +
 drivers/net/ena/ena_ethdev.c                    |   4 +
 drivers/net/enic/enic_ethdev.c                  |   4 +
 drivers/net/fm10k/fm10k_ethdev.c                |   4 +
 drivers/net/i40e/i40e_ethdev.c                  |   4 +
 drivers/net/i40e/i40e_ethdev_vf.c               |   4 +
 drivers/net/ixgbe/ixgbe_ethdev.c                |   8 +
 drivers/net/mlx4/mlx4.c                         |   3 +-
 drivers/net/mlx5/mlx5.c                         |   3 +-
 drivers/net/nfp/nfp_net.c                       |   4 +
 drivers/net/qede/qede_ethdev.c                  |   8 +
 drivers/net/szedata2/rte_eth_szedata2.c         |   4 +
 drivers/net/thunderx/nicvf_ethdev.c             |   4 +
 drivers/net/virtio/virtio_ethdev.c              |   2 +
 drivers/net/vmxnet3/vmxnet3_ethdev.c            |   4 +
 lib/librte_eal/bsdapp/eal/Makefile              |   1 +
 lib/librte_eal/bsdapp/eal/eal.c                 |  13 +-
 lib/librte_eal/bsdapp/eal/eal_pci.c             |  52 +-
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  23 +-
 lib/librte_eal/common/Makefile                  |   2 +-
 lib/librte_eal/common/eal_common_bus.c          | 284 ++++++++++
 lib/librte_eal/common/eal_common_pci.c          | 322 ++++++-----
 lib/librte_eal/common/eal_private.h             |  14 +-
 lib/librte_eal/common/include/rte_bus.h         | 293 ++++++++++
 lib/librte_eal/common/include/rte_dev.h         |  14 +
 lib/librte_eal/common/include/rte_pci.h         |  58 +-
 lib/librte_eal/linuxapp/eal/Makefile            |   1 +
 lib/librte_eal/linuxapp/eal/eal.c               |  13 +-
 lib/librte_eal/linuxapp/eal/eal_pci.c           |  84 ++-
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  23 +-
 39 files changed, 1847 insertions(+), 289 deletions(-)
 create mode 100644 app/test/test_bus.c
 create mode 100644 lib/librte_eal/common/eal_common_bus.c
 create mode 100644 lib/librte_eal/common/include/rte_bus.h

-- 
2.7.4

^ permalink raw reply

* [PATCH v4 01/12] eal/bus: introduce bus abstraction
From: Shreyansh Jain @ 2016-12-26 12:50 UTC (permalink / raw)
  To: david.marchand; +Cc: dev, thomas.monjalon, Shreyansh Jain
In-Reply-To: <1482756644-13726-1-git-send-email-shreyansh.jain@nxp.com>

This patch introduces the rte_bus abstraction for devices and drivers in
EAL framework. The model is:
 - One or more buses are connected to a CPU (or core)
 - One or more devices are conneted to a Bus
 - Drivers are running instances which manage one or more devices
 - Bus is responsible for identifying devices (and interrupt propogation)
 - Driver is responsible for initializing the device

This patch adds a 'rte_bus' class which rte_driver and rte_device refer.
This way, each device (rte_xxx_device) would have reference to the bus
it is based on. As well as, each driver (rte_xxx_driver) would have link
to the bus and devices on it for servicing.

                                  __ rte_bus_list
                                 /
                     +----------'---+
                     |rte_bus       |
                     | driver_list------> List of rte_bus specific
                     | device_list----    devices
                     |              | `-> List of rte_bus associated
                     |              |     drivers
                     +--|------|----+
              _________/        \_________
    +--------/----+                     +-\---------------+
    |rte_device   |                     |rte_driver       |
    | rte_bus     |                     | rte_bus         |
    | rte_driver  |                     | ...             |
    | ...         |                     +---------...-----+
    |             |                               |||
    +---||--------+                               |||
        ||                                        |||
        | \                                        \\\
        |  \_____________                           \\\
        |                \                          |||
 +------|---------+ +----|----------+               |||
 |rte_pci_device  | |rte_xxx_device |               |||
 | ....           | | ....          |               |||
 +----------------+ +---------------+              / | \
                                                  /  |  \
                            _____________________/  /    \
                           /                    ___/      \
            +-------------'--+    +------------'---+    +--'------------+
            |rte_pci_driver  |    |rte_vdev_driver |    |rte_xxx_driver |
            | ....           |    | ....           |    | ....          |
            +----------------+    +----------------+    +---------------+

This patch only enables the bus references on rte_driver and rte_driver.
EAL wide global device and driver list continue to exist until an instance
of bus is added in subsequent patches.

This patch also introduces RTE_REGISTER_BUS macro on the lines of
RTE_PMD_REGISTER_XXX. Key difference is that the constructor priority has
been explicitly set to 101 so as to execute bus registration before PMD.

Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
---
 lib/librte_eal/bsdapp/eal/Makefile              |   1 +
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  16 ++
 lib/librte_eal/common/Makefile                  |   2 +-
 lib/librte_eal/common/eal_common_bus.c          | 190 ++++++++++++++++++++++++
 lib/librte_eal/common/include/rte_bus.h         | 177 ++++++++++++++++++++++
 lib/librte_eal/common/include/rte_dev.h         |   2 +
 lib/librte_eal/linuxapp/eal/Makefile            |   1 +
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  16 ++
 8 files changed, 404 insertions(+), 1 deletion(-)
 create mode 100644 lib/librte_eal/common/eal_common_bus.c
 create mode 100644 lib/librte_eal/common/include/rte_bus.h

diff --git a/lib/librte_eal/bsdapp/eal/Makefile b/lib/librte_eal/bsdapp/eal/Makefile
index a15b762..cce99f7 100644
--- a/lib/librte_eal/bsdapp/eal/Makefile
+++ b/lib/librte_eal/bsdapp/eal/Makefile
@@ -78,6 +78,7 @@ SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_common_cpuflags.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_common_string_fns.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_common_hexdump.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_common_devargs.c
+SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_common_bus.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_common_dev.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_common_options.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_common_thread.c
diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
index 2f81f7c..51115f4 100644
--- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
@@ -174,3 +174,19 @@ DPDK_16.11 {
 	rte_eal_vdrv_unregister;
 
 } DPDK_16.07;
+
+DPDK_17.02 {
+	global:
+
+	rte_bus_list;
+	rte_eal_bus_add_device;
+	rte_eal_bus_add_driver;
+	rte_eal_bus_get;
+	rte_eal_bus_dump;
+	rte_eal_bus_register;
+	rte_eal_bus_insert_device;
+	rte_eal_bus_remove_device;
+	rte_eal_bus_remove_driver;
+	rte_eal_bus_unregister;
+
+} DPDK_16.11;
diff --git a/lib/librte_eal/common/Makefile b/lib/librte_eal/common/Makefile
index a92c984..0c39414 100644
--- a/lib/librte_eal/common/Makefile
+++ b/lib/librte_eal/common/Makefile
@@ -38,7 +38,7 @@ INC += rte_per_lcore.h rte_random.h
 INC += rte_tailq.h rte_interrupts.h rte_alarm.h
 INC += rte_string_fns.h rte_version.h
 INC += rte_eal_memconfig.h rte_malloc_heap.h
-INC += rte_hexdump.h rte_devargs.h rte_dev.h rte_vdev.h
+INC += rte_hexdump.h rte_devargs.h rte_bus.h rte_dev.h rte_vdev.h
 INC += rte_pci_dev_feature_defs.h rte_pci_dev_features.h
 INC += rte_malloc.h rte_keepalive.h rte_time.h
 
diff --git a/lib/librte_eal/common/eal_common_bus.c b/lib/librte_eal/common/eal_common_bus.c
new file mode 100644
index 0000000..01730f8
--- /dev/null
+++ b/lib/librte_eal/common/eal_common_bus.c
@@ -0,0 +1,190 @@
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2016 NXP
+ *   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 NXP 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 <stdio.h>
+#include <string.h>
+#include <inttypes.h>
+#include <sys/queue.h>
+
+#include <rte_bus.h>
+#include <rte_devargs.h>
+#include <rte_debug.h>
+
+#include "eal_private.h"
+
+struct rte_bus_list rte_bus_list =
+	TAILQ_HEAD_INITIALIZER(rte_bus_list);
+
+/** @internal
+ * Add a device to a bus.
+ */
+void
+rte_eal_bus_add_device(struct rte_bus *bus, struct rte_device *dev)
+{
+	RTE_VERIFY(bus);
+	RTE_VERIFY(dev);
+
+	TAILQ_INSERT_TAIL(&bus->device_list, dev, next);
+	dev->bus = bus;
+}
+
+/** @internal
+ * Remove a device from its bus.
+ */
+void
+rte_eal_bus_remove_device(struct rte_device *dev)
+{
+	struct rte_bus *bus;
+
+	RTE_VERIFY(dev);
+	RTE_VERIFY(dev->bus);
+
+	bus = dev->bus;
+	TAILQ_REMOVE(&bus->device_list, dev, next);
+	dev->bus = NULL;
+}
+
+/** @internal
+ * Associate a driver with a bus.
+ */
+void
+rte_eal_bus_add_driver(struct rte_bus *bus, struct rte_driver *drv)
+{
+	RTE_VERIFY(bus);
+	RTE_VERIFY(drv);
+
+	TAILQ_INSERT_TAIL(&bus->driver_list, drv, next);
+	drv->bus = bus;
+}
+
+/** @internal
+ * Disassociate a driver from bus.
+ */
+void
+rte_eal_bus_remove_driver(struct rte_driver *drv)
+{
+	struct rte_bus *bus;
+
+	RTE_VERIFY(drv);
+	RTE_VERIFY(drv->bus);
+
+	bus = drv->bus;
+	TAILQ_REMOVE(&bus->driver_list, drv, next);
+	drv->bus = NULL;
+}
+
+/**
+ * Get the bus handle using its name
+ */
+struct rte_bus *
+rte_eal_bus_get(const char *bus_name)
+{
+	struct rte_bus *bus;
+
+	RTE_VERIFY(bus_name);
+
+	TAILQ_FOREACH(bus, &rte_bus_list, next) {
+		RTE_VERIFY(bus->name);
+
+		if (!strcmp(bus_name, bus->name))
+			return bus;
+	}
+
+	/* Unable to find bus requested */
+	return NULL;
+}
+
+/* register a bus */
+void
+rte_eal_bus_register(struct rte_bus *bus)
+{
+	RTE_VERIFY(bus);
+	RTE_VERIFY(bus->name && strlen(bus->name));
+
+	/* Initialize the driver and device list associated with the bus */
+	TAILQ_INIT(&(bus->driver_list));
+	TAILQ_INIT(&(bus->device_list));
+
+	TAILQ_INSERT_TAIL(&rte_bus_list, bus, next);
+	RTE_LOG(INFO, EAL, "Registered [%s] bus.\n", bus->name);
+}
+
+/* unregister a bus */
+void
+rte_eal_bus_unregister(struct rte_bus *bus)
+{
+	/* All devices and drivers associated with the bus should have been
+	 * 'device->uninit' and 'driver->remove()' already.
+	 */
+	RTE_VERIFY(TAILQ_EMPTY(&(bus->driver_list)));
+	RTE_VERIFY(TAILQ_EMPTY(&(bus->device_list)));
+
+	/* TODO: For each device, call its rte_device->driver->remove()
+	 * and rte_eal_bus_remove_driver()
+	 */
+
+	TAILQ_REMOVE(&rte_bus_list, bus, next);
+	RTE_LOG(INFO, EAL, "Unregistered [%s] bus.\n", bus->name);
+}
+
+/* dump one bus info */
+static int
+bus_dump_one(FILE *f, struct rte_bus *bus)
+{
+	int ret;
+
+	/* For now, dump only the bus name */
+	ret = fprintf(f, " %s\n", bus->name);
+
+	/* Error in case of inability in writing to stream */
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+void
+rte_eal_bus_dump(FILE *f)
+{
+	int ret;
+	struct rte_bus *bus;
+
+	TAILQ_FOREACH(bus, &rte_bus_list, next) {
+		ret = bus_dump_one(f, bus);
+		if (ret) {
+			RTE_LOG(ERR, EAL, "Unable to write to stream (%d)\n",
+				ret);
+			break;
+		}
+	}
+}
diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
new file mode 100644
index 0000000..ad2873c
--- /dev/null
+++ b/lib/librte_eal/common/include/rte_bus.h
@@ -0,0 +1,177 @@
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2016 NXP
+ *   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 NXP 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.
+ */
+
+#ifndef _RTE_BUS_H_
+#define _RTE_BUS_H_
+
+/**
+ * @file
+ *
+ * RTE PMD Bus Abstraction interfaces
+ *
+ * This file exposes APIs and Interfaces for Bus Abstraction over the devices
+ * drivers in EAL.
+ */
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include <stdio.h>
+#include <sys/queue.h>
+
+#include <rte_log.h>
+#include <rte_dev.h>
+
+/** Double linked list of buses */
+TAILQ_HEAD(rte_bus_list, rte_bus);
+
+/* Global Bus list */
+extern struct rte_bus_list rte_bus_list;
+
+/**
+ * A structure describing a generic bus.
+ */
+struct rte_bus {
+	TAILQ_ENTRY(rte_bus) next;   /**< Next bus object in linked list */
+	struct rte_driver_list driver_list;
+				     /**< List of all drivers on bus */
+	struct rte_device_list device_list;
+				     /**< List of all devices on bus */
+	const char *name;            /**< Name of the bus */
+};
+
+/** @internal
+ * Add a device to a bus.
+ *
+ * @param bus
+ *	Bus on which device is to be added
+ * @param dev
+ *	Device handle
+ * @return
+ *	None
+ */
+void
+rte_eal_bus_add_device(struct rte_bus *bus, struct rte_device *dev);
+
+/** @internal
+ * Remove a device from its bus.
+ *
+ * @param dev
+ *	Device handle to remove
+ * @return
+ *	None
+ */
+void
+rte_eal_bus_remove_device(struct rte_device *dev);
+
+/** @internal
+ * Associate a driver with a bus.
+ *
+ * @param bus
+ *	Bus on which driver is to be added
+ * @param dev
+ *	Driver handle
+ * @return
+ *	None
+ */
+void
+rte_eal_bus_add_driver(struct rte_bus *bus, struct rte_driver *drv);
+
+/** @internal
+ * Disassociate a driver from its bus.
+ *
+ * @param dev
+ *	Driver handle to remove
+ * @return
+ *	None
+ */
+void
+rte_eal_bus_remove_driver(struct rte_driver *drv);
+
+/**
+ * Register a Bus handler.
+ *
+ * @param bus
+ *   A pointer to a rte_bus structure describing the bus
+ *   to be registered.
+ */
+void rte_eal_bus_register(struct rte_bus *bus);
+
+/**
+ * Unregister a Bus handler.
+ *
+ * @param bus
+ *   A pointer to a rte_bus structure describing the bus
+ *   to be unregistered.
+ */
+void rte_eal_bus_unregister(struct rte_bus *bus);
+
+/**
+ * Obtain handle for bus given its name.
+ *
+ * @param bus_name
+ *	Name of the bus handle to search
+ * @return
+ *	Pointer to Bus object if name matches any registered bus object
+ *	NULL, if no matching bus found
+ */
+struct rte_bus *rte_eal_bus_get(const char *bus_name);
+
+/**
+ * Dump information of all the buses registered with EAL.
+ *
+ * @param f
+ *	A valid and open output stream handle
+ *
+ * @return
+ *	 0 in case of success
+ *	!0 in case there is error in opening the output stream
+ */
+void rte_eal_bus_dump(FILE *f);
+
+/** Helper for Bus registration. The constructor has higher priority than
+ * PMD constructors
+ */
+#define RTE_REGISTER_BUS(nm, bus) \
+static void __attribute__((constructor(101), used)) businitfn_ ##nm(void) \
+{\
+	(bus).name = RTE_STR(nm);\
+	rte_eal_bus_register(&bus); \
+}
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* _RTE_BUS_H */
diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
index b17791f..8ac09e0 100644
--- a/lib/librte_eal/common/include/rte_dev.h
+++ b/lib/librte_eal/common/include/rte_dev.h
@@ -122,6 +122,7 @@ struct rte_driver;
  */
 struct rte_device {
 	TAILQ_ENTRY(rte_device) next; /**< Next device */
+	struct rte_bus *bus;          /**< Device connected to this bus */
 	const struct rte_driver *driver;/**< Associated driver */
 	int numa_node;                /**< NUMA node connection */
 	struct rte_devargs *devargs;  /**< Device user arguments */
@@ -148,6 +149,7 @@ void rte_eal_device_remove(struct rte_device *dev);
  */
 struct rte_driver {
 	TAILQ_ENTRY(rte_driver) next;  /**< Next in list. */
+	struct rte_bus *bus;           /**< Bus serviced by this driver */
 	const char *name;                   /**< Driver name. */
 	const char *alias;              /**< Driver alias. */
 };
diff --git a/lib/librte_eal/linuxapp/eal/Makefile b/lib/librte_eal/linuxapp/eal/Makefile
index 4e206f0..aa874a5 100644
--- a/lib/librte_eal/linuxapp/eal/Makefile
+++ b/lib/librte_eal/linuxapp/eal/Makefile
@@ -87,6 +87,7 @@ SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_common_cpuflags.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_common_string_fns.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_common_hexdump.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_common_devargs.c
+SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_common_bus.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_common_dev.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_common_options.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_common_thread.c
diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
index 83721ba..abfe93e 100644
--- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
@@ -178,3 +178,19 @@ DPDK_16.11 {
 	rte_eal_vdrv_unregister;
 
 } DPDK_16.07;
+
+DPDK_17.02 {
+	global:
+
+	rte_bus_list;
+	rte_eal_bus_add_device;
+	rte_eal_bus_add_driver;
+	rte_eal_bus_get;
+	rte_eal_bus_dump;
+	rte_eal_bus_register;
+	rte_eal_bus_insert_device;
+	rte_eal_bus_remove_device;
+	rte_eal_bus_remove_driver;
+	rte_eal_bus_unregister;
+
+} DPDK_16.11;
-- 
2.7.4

^ permalink raw reply related

* [PATCH v4 02/12] test: add basic bus infrastructure tests
From: Shreyansh Jain @ 2016-12-26 12:50 UTC (permalink / raw)
  To: david.marchand; +Cc: dev, thomas.monjalon, Shreyansh Jain
In-Reply-To: <1482756644-13726-1-git-send-email-shreyansh.jain@nxp.com>

Verification of bus registration, driver registration on a bus.

Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
---
 app/test/Makefile   |   2 +-
 app/test/test.h     |   2 +
 app/test/test_bus.c | 424 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 427 insertions(+), 1 deletion(-)
 create mode 100644 app/test/test_bus.c

diff --git a/app/test/Makefile b/app/test/Makefile
index 5be023a..ca0f106 100644
--- a/app/test/Makefile
+++ b/app/test/Makefile
@@ -94,7 +94,7 @@ SRCS-y += test_cycles.c
 SRCS-y += test_spinlock.c
 SRCS-y += test_memory.c
 SRCS-y += test_memzone.c
-
+SRCS-y += test_bus.c
 SRCS-y += test_ring.c
 SRCS-y += test_ring_perf.c
 SRCS-y += test_pmd_perf.c
diff --git a/app/test/test.h b/app/test/test.h
index 82831f4..c8ec43f 100644
--- a/app/test/test.h
+++ b/app/test/test.h
@@ -236,6 +236,8 @@ int commands_init(void);
 int test_pci(void);
 int test_pci_run;
 
+int test_bus(void);
+
 int test_mp_secondary(void);
 
 int test_set_rxtx_conf(cmdline_fixed_string_t mode);
diff --git a/app/test/test_bus.c b/app/test/test_bus.c
new file mode 100644
index 0000000..60950d3
--- /dev/null
+++ b/app/test/test_bus.c
@@ -0,0 +1,424 @@
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2016 NXP.
+ *   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 NXP 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 <stdio.h>
+#include <string.h>
+#include <stdint.h>
+#include <sys/queue.h>
+
+#include <rte_bus.h>
+#include <rte_devargs.h>
+
+#include "test.h"
+#include "resource.h"
+
+/* Visualizing following bus-device-driver model for test
+ *
+ *  ===.===========.===========.=========,= busA
+ *     |           |           .         |
+ *   devA1       devA2         .         '____CPU_
+ *    `-----------`-------> driverA      |
+ *                                       '
+ *  ===.===========.===========.=========|= busB
+ *     |           |           .
+ *   devB1       devB2         .
+ *    `-----------`-------> driverB
+ *
+ */
+
+#define MAX_DEVICES_ON_BUS	10
+#define MAX_DRIVERS_ON_BUS	10
+
+/* A structure representing a ethernet/crypto device, embedding
+ * the rte_device.
+ */
+struct dummy_device {
+	const char *name;
+	struct rte_device dev;
+};
+
+/* Structure representing a Bus with devices attached to it, and drivers
+ * for those devices
+ */
+struct dummy_bus {
+	const char *name;
+	struct rte_bus *bus;
+	struct rte_driver *drivers[MAX_DRIVERS_ON_BUS];
+	struct dummy_device *devices[MAX_DEVICES_ON_BUS];
+};
+
+struct rte_bus_list orig_bus_list =
+	TAILQ_HEAD_INITIALIZER(orig_bus_list);
+
+struct rte_bus busA = {
+	.name = "busA", /* "busA" */
+};
+
+struct rte_bus busB = {
+	.name = "busB", /* "busB */
+};
+
+struct rte_driver driverA = {
+	.name = "driverA",
+};
+
+struct dummy_device devA1 = {
+	.name = "devA1",
+	.dev = {
+		.bus = NULL,
+		.driver = NULL,
+	}
+};
+
+struct dummy_device devA2 = {
+	.name = "devA2",
+	.dev = {
+		.bus = NULL,
+		.driver = NULL,
+	}
+};
+
+struct rte_driver driverB = {
+	.name = "driverB",
+};
+
+struct dummy_device devB1 = {
+	.name = "devB1",
+	.dev = {
+		.bus = NULL,
+		.driver = NULL,
+	}
+};
+
+struct dummy_device devB2 = {
+	.name = "devB2",
+	.dev = {
+		.bus = NULL,
+		.driver = NULL,
+	}
+};
+
+struct dummy_bus dummy_buses[] = {
+	{
+		.name = "busA",
+		.bus = &busA,
+		.drivers = {&driverA, NULL},
+		.devices = {&devA1, &devA2, NULL},
+	},
+	{
+		.name = "busB",
+		.bus = &busB,
+		.drivers = {&driverB, NULL},
+		.devices = {&devB1, &devB2, NULL},
+	},
+	{NULL, NULL, {NULL,}, {NULL,}, },
+};
+
+/* @internal
+ * Dump the device tree
+ */
+static void
+dump_device_tree(void)
+{
+	int i;
+	struct dummy_bus *db;
+	struct rte_bus *bus;
+	struct rte_driver *drv;
+	struct rte_device *dev;
+
+	printf("------>8-------\n");
+	printf("Device Tree:\n");
+	for (i = 0; dummy_buses[i].name; i++) {
+		db = &dummy_buses[i];
+
+		bus = rte_eal_bus_get(db->name);
+		if (!bus)
+			return;
+
+		printf(" Bus: %s\n", bus->name);
+
+		printf("  Drivers on bus:\n");
+		TAILQ_FOREACH(drv, &bus->driver_list, next) {
+			printf("    %s\n", drv->name);
+		}
+
+		printf("  Devices on bus:\n");
+		TAILQ_FOREACH(dev, &bus->device_list, next) {
+			printf("    Addr: %p\n", dev);
+			if (dev->driver)
+				printf("    Driver = %s\n", dev->driver->name);
+			else
+				printf("    Driver = None\n");
+		}
+	}
+	printf("------>8-------\n");
+}
+
+static int
+test_bus_setup(void)
+{
+	struct rte_bus *bus_p = NULL;
+
+	/* Preserve the original bus list before executing test */
+	while (!TAILQ_EMPTY(&rte_bus_list)) {
+		bus_p = TAILQ_FIRST(&rte_bus_list);
+		TAILQ_REMOVE(&rte_bus_list, bus_p, next);
+		TAILQ_INSERT_TAIL(&orig_bus_list, bus_p, next);
+	}
+
+	return 0;
+}
+
+static int
+test_bus_cleanup(void)
+{
+	struct rte_bus *bus_p = NULL;
+
+	/* Cleanup rte_bus_list before restoring entries */
+	while (!TAILQ_EMPTY(&rte_bus_list)) {
+		bus_p = TAILQ_FIRST(&rte_bus_list);
+		rte_eal_bus_unregister(bus_p);
+		TAILQ_REMOVE(&rte_bus_list, bus_p, next);
+	}
+
+	bus_p = NULL;
+	/* Restore original entries */
+	while (!TAILQ_EMPTY(&orig_bus_list)) {
+		bus_p = TAILQ_FIRST(&orig_bus_list);
+		TAILQ_REMOVE(&orig_bus_list, bus_p, next);
+		TAILQ_INSERT_TAIL(&rte_bus_list, bus_p, next);
+	}
+
+	dump_device_tree();
+	return 0;
+}
+
+
+static int
+test_bus_registration(void)
+{
+	int i;
+	int ret;
+	struct rte_bus *bus = NULL;
+
+	for (i = 0; dummy_buses[i].name != NULL; i++) {
+		bus = dummy_buses[i].bus;
+		rte_eal_bus_register(bus);
+		printf("Registered Bus %s\n", dummy_buses[i].name);
+	}
+
+	/* Verify that all buses have been successfully registered */
+	i = 0;
+	TAILQ_FOREACH(bus, &rte_bus_list, next) {
+		/* Or, if the name of the bus is NULL */
+		if (!bus->name) {
+			/* Incorrect entry in list */
+			printf("Incorrect bus registered.\n");
+			return -1;
+		}
+
+		/* Or, if the bus name doesn't match that of dummy_buses */
+		ret = strcmp(bus->name, dummy_buses[i].name);
+		if (ret) {
+			/* Bus name doesn't match */
+			printf("Unable to correctly register bus (%s).\n",
+			       dummy_buses[i].name);
+			return -1;
+		}
+		i++;
+	}
+
+	/* Current value of dummy_buses[i] should be the NULL entry */
+	if (dummy_buses[i].name != NULL) {
+		printf("Not all buses were registered. For e.g. (%s)\n",
+		       dummy_buses[i].name);
+		return -1;
+	}
+
+	printf("Buses registered are:\n");
+	rte_eal_bus_dump(stdout);
+
+	return 0;
+}
+
+static int
+test_bus_unregistration(void)
+{
+	int i;
+	struct rte_bus *bus = NULL;
+
+	for (i = 0; dummy_buses[i].name != NULL; i++) {
+		bus = rte_eal_bus_get(dummy_buses[i].name);
+		if (bus) {
+			printf("Unregistering bus: '%s'\n", bus->name);
+			rte_eal_bus_unregister(bus);
+		}
+	}
+
+	if (!TAILQ_EMPTY(&rte_bus_list)) {
+		/* Unable to unregister all dummy buses */
+		printf("Unable to unregister all buses\n");
+		return -1;
+	}
+
+	printf("All buses have been unregistered.\n");
+	dump_device_tree();
+	return 0;
+}
+
+/* Positive case: For each driver in dummy_buses, perform
+ * registration
+ */
+static int
+test_driver_registration_on_bus(void)
+{
+	int i, j;
+	struct rte_bus *bus = NULL;
+	struct rte_driver *drv, *drv2;
+
+	/* For each bus on the dummy_buses list:
+	 * 1. get the bus reference
+	 * 2. register all drivers from dummy_buses
+	 */
+	for (i = 0; dummy_buses[i].name; i++) {
+		bus = rte_eal_bus_get(dummy_buses[i].name);
+		if (!bus) {
+			printf("Unable to find bus (%s)\n",
+			       dummy_buses[i].name);
+			return -1;
+		}
+
+		/* For bus 'bus', register all drivers */
+		for (j = 0; dummy_buses[i].drivers[j]; j++) {
+			drv = dummy_buses[i].drivers[j];
+			rte_eal_bus_add_driver(bus, drv);
+		}
+	}
+
+	/* Drivers have been registered. Verify by parsing the list */
+	drv = NULL;
+	for (i = 0; dummy_buses[i].name; i++) {
+		bus = rte_eal_bus_get(dummy_buses[i].name);
+		if (!bus) {
+			printf("Unable to find bus (%s)\n",
+			       dummy_buses[i].name);
+			return -1;
+		}
+
+		j = 0;
+		TAILQ_FOREACH(drv, &bus->driver_list, next) {
+			drv2 = dummy_buses[i].drivers[j++];
+			if (strcmp(drv2->name, drv->name)) {
+				printf("Incorrectly registered drivers."
+				       " Expected: %s; Available: %s\n",
+				       drv2->name, drv->name);
+				return -1;
+			}
+		}
+	}
+
+	printf("Driver registration test successful.\n");
+	dump_device_tree();
+
+	return 0;
+}
+
+static int
+test_driver_unregistration_on_bus(void)
+{
+	int i;
+	struct rte_bus *bus = NULL;
+	struct rte_driver *drv;
+
+	for (i = 0; dummy_buses[i].name; i++) {
+		bus = rte_eal_bus_get(dummy_buses[i].name);
+		if (!bus) {
+			printf("Unable to find bus (%s)\n",
+			       dummy_buses[i].name);
+			return -1;
+		}
+
+		/* For bus 'bus', unregister all drivers */
+		TAILQ_FOREACH(drv, &bus->driver_list, next) {
+			rte_eal_bus_remove_driver(drv);
+		}
+	}
+
+	/* Verifying that all drivers have been removed */
+	for (i = 0; dummy_buses[i].name; i++) {
+		bus = rte_eal_bus_get(dummy_buses[i].name);
+
+		if (!TAILQ_EMPTY(&bus->driver_list)) {
+			printf("Unable to remove all drivers on bus (%s)\n",
+			       bus->name);
+			return -1;
+		}
+	}
+
+	printf("Unregistration of drivers on all buses is successful.\n");
+	/* All devices from all buses have been removed */
+	dump_device_tree();
+	return 0;
+
+}
+
+int
+test_bus(void)
+{
+	/* Make necessary arrangements before starting test */
+	if (test_bus_setup())
+		return -1;
+
+	if (test_bus_registration())
+		return -1;
+
+	/* Assuming that buses are already registered, register drivers
+	 * with them.
+	 */
+	if (test_driver_registration_on_bus())
+		return -1;
+
+	if (test_driver_unregistration_on_bus())
+		return -1;
+
+	if (test_bus_unregistration())
+		return -1;
+
+	/* Restore the original environment/settings */
+	if (test_bus_cleanup())
+		return -1;
+
+	return 0;
+}
+
+REGISTER_TEST_COMMAND(bus_autotest, test_bus);
-- 
2.7.4

^ permalink raw reply related

* [PATCH v4 03/12] eal/bus: add scan, match and insert support
From: Shreyansh Jain @ 2016-12-26 12:50 UTC (permalink / raw)
  To: david.marchand; +Cc: dev, thomas.monjalon, Shreyansh Jain
In-Reply-To: <1482756644-13726-1-git-send-email-shreyansh.jain@nxp.com>

When a PMD is registred, it will associate itself with a bus.

A bus is responsible for 'scan' of all the devices attached to it.
All the scanned devices are attached to bus specific device_list.
During the probe operation, 'match' of the drivers and devices would
be done.

Also, rather than adding a device to tail, a new device might be added to
the list (pivoted on bus) at a predefined position, for example, adding it
in order of addressing. Support for this is added as '*bus_insert'.

This patch also adds necessary test framework to test the scan and
match callbacks.

Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
---
 app/test/test_bus.c                     | 265 ++++++++++++++++++++++++++++++++
 lib/librte_eal/common/eal_common_bus.c  |  15 ++
 lib/librte_eal/common/include/rte_bus.h |  64 ++++++++
 3 files changed, 344 insertions(+)

diff --git a/app/test/test_bus.c b/app/test/test_bus.c
index 60950d3..98250c8 100644
--- a/app/test/test_bus.c
+++ b/app/test/test_bus.c
@@ -80,12 +80,32 @@ struct dummy_bus {
 struct rte_bus_list orig_bus_list =
 	TAILQ_HEAD_INITIALIZER(orig_bus_list);
 
+/* Forward declarations for callbacks from bus */
+
+/* Bus A
+ * Scan would register devA1 and devA2 to bus
+ */
+int scan_fn_for_busA(struct rte_bus *bus);
+
+/* Bus B
+ * Scan would register devB1 and devB2 to bus
+ */
+int scan_fn_for_busB(struct rte_bus *bus);
+
+/* generic implementations wrapped around by above declarations */
+static int generic_scan_fn(struct rte_bus *bus);
+static int generic_match_fn(struct rte_driver *drv, struct rte_device *dev);
+
 struct rte_bus busA = {
 	.name = "busA", /* "busA" */
+	.scan = scan_fn_for_busA,
+	.match = generic_match_fn,
 };
 
 struct rte_bus busB = {
 	.name = "busB", /* "busB */
+	.scan = scan_fn_for_busB,
+	.match = generic_match_fn,
 };
 
 struct rte_driver driverA = {
@@ -184,6 +204,92 @@ dump_device_tree(void)
 	printf("------>8-------\n");
 }
 
+/* @internal
+ * Move over the dummy_buses and find the entry matching the bus object
+ * passed as argument.
+ * For each device in that dummy_buses list, register.
+ *
+ * @param bus
+ *	bus to scan againt test entry
+ * @return
+ *	0 for successful scan, even if no devices are found
+ *	!0 for any error in scanning (like, invalid bus)
+ */
+static int
+generic_scan_fn(struct rte_bus *bus)
+{
+	int i = 0;
+	struct rte_device *dev = NULL;
+	struct dummy_bus *db = NULL;
+
+	if (!bus)
+		return -1;
+
+	/* Extract the device tree node using the bus passed */
+	for (i = 0; dummy_buses[i].name; i++) {
+		if (!strcmp(dummy_buses[i].name, bus->name)) {
+			db = &dummy_buses[i];
+			break;
+		}
+	}
+
+	if (!db)
+		return -1;
+
+	/* For all the devices in the device tree (dummy_buses), add device */
+	for (i = 0; db->devices[i]; i++) {
+		dev = &(db->devices[i]->dev);
+		rte_eal_bus_add_device(bus, dev);
+	}
+
+	return 0;
+}
+
+/* @internal
+ * Obtain bus from driver object. Match the address of rte_device object
+ * with all the devices associated with that bus.
+ *
+ * Being a test function, all this does is validate that device object
+ * provided is available on the same bus to which driver is registered.
+ *
+ * @param drv
+ *	driver to which matching is to be performed
+ * @param dev
+ *	device object to match with driver
+ * @return
+ *	0 for successful match
+ *	!0 for failed match
+ */
+static int
+generic_match_fn(struct rte_driver *drv, struct rte_device *dev)
+{
+	struct rte_bus *bus;
+	struct rte_device *dev_p = NULL;
+
+	/* Match is based entirely on address of 'dev' and 'dev_p' extracted
+	 * from bus->device_list.
+	 */
+
+	/* a driver is registered with the bus *before* the scan. */
+	bus = drv->bus;
+	TAILQ_FOREACH(dev_p, &bus->device_list, next) {
+		if (dev == dev_p)
+			return 0;
+	}
+
+	return 1;
+}
+
+int
+scan_fn_for_busA(struct rte_bus *bus) {
+	return generic_scan_fn(bus);
+}
+
+int
+scan_fn_for_busB(struct rte_bus *bus) {
+	return generic_scan_fn(bus);
+}
+
 static int
 test_bus_setup(void)
 {
@@ -392,6 +498,155 @@ test_driver_unregistration_on_bus(void)
 
 }
 
+static int
+test_device_unregistration_on_bus(void)
+{
+	int i;
+	struct rte_bus *bus = NULL;
+	struct rte_device *dev;
+
+	for (i = 0; dummy_buses[i].name; i++) {
+		bus = rte_eal_bus_get(dummy_buses[i].name);
+		if (!bus) {
+			printf("Unable to find bus (%s)\n",
+			       dummy_buses[i].name);
+			return -1;
+		}
+
+		/* For bus 'bus', unregister all devices */
+		TAILQ_FOREACH(dev, &bus->device_list, next) {
+			rte_eal_bus_remove_device(dev);
+		}
+	}
+
+	for (i = 0; dummy_buses[i].name; i++) {
+		bus = rte_eal_bus_get(dummy_buses[i].name);
+
+		if (!TAILQ_EMPTY(&bus->device_list)) {
+			printf("Unable to remove all devices on bus (%s)\n",
+			       bus->name);
+			return -1;
+		}
+	}
+
+	/* All devices from all buses have been removed */
+	printf("All devices on all buses unregistered.\n");
+	dump_device_tree();
+
+	return 0;
+}
+
+/* @internal
+ * For each bus registered, call the scan function to identify devices
+ * on the bus.
+ *
+ * @param void
+ * @return
+ *	0 for successful scan
+ *	!0 for unsuccessful scan
+ *
+ */
+static int
+test_bus_scan(void)
+{
+	int ret;
+	struct rte_bus *bus;
+
+	TAILQ_FOREACH(bus, &rte_bus_list, next) {
+		/* Call the scan function for each bus */
+		ret = bus->scan(bus);
+		if (ret) {
+			printf("Scan of buses failed.\n");
+			return -1;
+		}
+	}
+
+	printf("Scan of all buses completed.\n");
+	dump_device_tree();
+
+	return 0;
+}
+
+/* @internal
+ * Function to perform 'probe' and link devices and drivers on a bus.
+ * This would work over all the buses registered, and all devices and drivers
+ * registered with it - call match on each pair.
+ * Aim is to test the match_fn for each bus.
+ *
+ * @param void
+ * @return
+ *	0 for successful probe
+ *	!0 for failure in probe
+ *
+ */
+static int
+test_probe_on_bus(void)
+{
+	int ret = 0;
+	int i, j;
+	struct rte_bus *bus = NULL;
+	struct rte_device *dev = NULL;
+	struct rte_driver *drv = NULL;
+
+	/* In case of this test:
+	 * 1. for each bus in rte_bus_list
+	 * 2.  for each device in bus->device_list
+	 * 3.   for each driver in bus->driver_list
+	 * 4.    call match
+	 * 5.    link driver and device
+	 * 6. Verify the linkage.
+	 */
+	for (i = 0; dummy_buses[i].name; i++) {
+		/* get bus pointer from dummy_buses itself rather than
+		 * rte_eal_bus_get
+		 */
+		bus = dummy_buses[i].bus;
+
+		TAILQ_FOREACH(dev, &bus->device_list, next) {
+			TAILQ_FOREACH(drv, &bus->driver_list, next) {
+				if (!bus->match) {
+					printf("Incorrect bus without match "
+					       "fn: (%s).\n", bus->name);
+					return -1;
+				}
+
+				ret = bus->match(drv, dev);
+				if (ret) {
+					printf("Device and driver don't "
+					       "belong to same bus.\n");
+					return -1;
+				}
+				dev->driver = drv;
+
+				/* As match is generic, it always results in
+				 * dev->drv pointing to first driver entry in
+				 * dummy_buses[i]
+				 */
+			}
+		}
+	}
+
+	/* Verify the linkage. All devices belonging to a dummy_buses[i]
+	 * should have same driver (first driver entry of dummy_buses[i])
+	 */
+	for (i = 0; dummy_buses[i].name; i++) {
+		drv = dummy_buses[i].drivers[0];
+
+		for (j = 0; dummy_buses[i].devices[j]; j++) {
+			dev = &(dummy_buses[i].devices[j]->dev);
+			if (dev->driver != drv) {
+				printf("Incorrect driver<->device linkage.\n");
+				return -1;
+			}
+		}
+	}
+
+	printf("Probe on all buses successful.\n");
+	dump_device_tree();
+
+	return 0;
+}
+
 int
 test_bus(void)
 {
@@ -408,6 +663,16 @@ test_bus(void)
 	if (test_driver_registration_on_bus())
 		return -1;
 
+	if (test_bus_scan())
+		return -1;
+
+	/* Now that the devices and drivers are registered, perform probe */
+	if (test_probe_on_bus())
+		return -1;
+
+	if (test_device_unregistration_on_bus())
+		return -1;
+
 	if (test_driver_unregistration_on_bus())
 		return -1;
 
diff --git a/lib/librte_eal/common/eal_common_bus.c b/lib/librte_eal/common/eal_common_bus.c
index 01730f8..5a5ae75 100644
--- a/lib/librte_eal/common/eal_common_bus.c
+++ b/lib/librte_eal/common/eal_common_bus.c
@@ -58,6 +58,18 @@ rte_eal_bus_add_device(struct rte_bus *bus, struct rte_device *dev)
 	dev->bus = bus;
 }
 
+void
+rte_eal_bus_insert_device(struct rte_bus *bus, struct rte_device *old_dev,
+			  struct rte_device *new_dev)
+{
+	RTE_VERIFY(bus);
+	RTE_VERIFY(old_dev);
+	RTE_VERIFY(new_dev);
+
+	TAILQ_INSERT_BEFORE(old_dev, new_dev, next);
+	new_dev->bus = bus;
+}
+
 /** @internal
  * Remove a device from its bus.
  */
@@ -130,6 +142,9 @@ rte_eal_bus_register(struct rte_bus *bus)
 {
 	RTE_VERIFY(bus);
 	RTE_VERIFY(bus->name && strlen(bus->name));
+	/* A bus should mandatorily have the scan and match implemented */
+	RTE_VERIFY(bus->scan);
+	RTE_VERIFY(bus->match);
 
 	/* Initialize the driver and device list associated with the bus */
 	TAILQ_INIT(&(bus->driver_list));
diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
index ad2873c..da76db3 100644
--- a/lib/librte_eal/common/include/rte_bus.h
+++ b/lib/librte_eal/common/include/rte_bus.h
@@ -60,6 +60,49 @@ TAILQ_HEAD(rte_bus_list, rte_bus);
 extern struct rte_bus_list rte_bus_list;
 
 /**
+ * Bus specific scan for devices attached on the bus.
+ * For each bus object, the scan would be reponsible for finding devices and
+ * adding them to its private device list.
+ *
+ * Successful detection of a device results in rte_device object which is
+ * embedded within the respective device type (rte_pci_device, for example).
+ * Thereafter, PCI specific bus would need to perform
+ * container_of(rte_pci_device) to obtain PCI device object.
+ *
+ * Scan failure of a bus is not treated as exit criteria for application. Scan
+ * for all other buses would still continue.
+ *
+ * A bus should mandatorily implement this method.
+ *
+ * @param bus
+ *	Reference to the bus on which device is added
+ * @return
+ *	0 for successful scan
+ *	!0 (<0) for unsuccessful scan with error value
+ */
+typedef int (*bus_scan_t)(struct rte_bus *bus);
+
+/**
+ * Bus specific match for devices and drivers which can service them.
+ * For each scanned device, rte_driver->probe would be called for driver
+ * specific initialization of the device.
+ *
+ * It is the work of each bus handler to obtain the specific device object
+ * using container_of (or typecasting, as a less preferred way).
+ *
+ * A bus should mandatorily implement this method.
+ *
+ * @param drv
+ *	Driver object attached to the bus
+ * @param dev
+ *	Device object which is being probed.
+ * @return
+ *	0 for successful match
+ *	!0 for unsuccessful match
+ */
+typedef int (*bus_match_t)(struct rte_driver *drv, struct rte_device *dev);
+
+/**
  * A structure describing a generic bus.
  */
 struct rte_bus {
@@ -69,6 +112,9 @@ struct rte_bus {
 	struct rte_device_list device_list;
 				     /**< List of all devices on bus */
 	const char *name;            /**< Name of the bus */
+	bus_scan_t scan;            /**< Scan for devices attached to bus */
+	bus_match_t match;
+	/**< Match device with drivers associated with the bus */
 };
 
 /** @internal
@@ -85,6 +131,24 @@ void
 rte_eal_bus_add_device(struct rte_bus *bus, struct rte_device *dev);
 
 /** @internal
+ * Rather than adding a device to tail, insert at a predefined location.
+ * This is specifically useful for update device cases, or where addition
+ * of devices in the list needs to be ordered (addressing, for example).
+ *
+ * @param bus
+ *	Handle for bus on which device is to be added
+ * @param old_dev
+ *	Existing rte_device object before which new device needs to be added
+ * @param new_dev
+ *	Object for device to be added before old_dev
+ * @return
+ *	void
+ */
+void
+rte_eal_bus_insert_device(struct rte_bus *bus, struct rte_device *old_device,
+			  struct rte_device *new_device);
+
+/** @internal
  * Remove a device from its bus.
  *
  * @param dev
-- 
2.7.4

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox