[dpdk-dev] [PATCH] net/thunderx: manage PCI device mapping for SQS VFs

[dpdk-dev] [PATCH] net/thunderx: manage PCI device mapping for SQS VFs

[Jerin Jacob]

Since the commit e84ad157b7bc ("pci: unmap resources if probe fails"),
EAL unmaps the PCI device if ethdev probe returns positive or
negative value.

nicvf thunderx PMD needs special treatment for Secondary queue set(SQS)
PCIe VF devices, where, it expects to not unmap or free the memory
without registering the ethdev subsystem.

To keep the same behavior, moved the PCI map function inside
the driver without using the EAL services.

Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
Signed-off-by: Angela Czubak <aczubak@caviumnetworks.com>
---
 drivers/net/thunderx/nicvf_ethdev.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/thunderx/nicvf_ethdev.c b/drivers/net/thunderx/nicvf_ethdev.c
index 796701b0f..6ec2f9266 100644
--- a/drivers/net/thunderx/nicvf_ethdev.c
+++ b/drivers/net/thunderx/nicvf_ethdev.c
@@ -2025,6 +2025,13 @@ nicvf_eth_dev_init(struct rte_eth_dev *eth_dev)
 	}
 
 	pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);
+
+	ret = rte_pci_map_device(pci_dev);
+	if (ret) {
+		PMD_INIT_LOG(ERR, "Failed to map pci device");
+		goto fail;
+	}
+
 	rte_eth_copy_pci_info(eth_dev, pci_dev);
 
 	nic->device_id = pci_dev->id.device_id;
@@ -2171,7 +2178,7 @@ static int nicvf_eth_pci_remove(struct rte_pci_device *pci_dev)
 
 static struct rte_pci_driver rte_nicvf_pmd = {
 	.id_table = pci_id_nicvf_map,
-	.drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC,
+	.drv_flags = RTE_PCI_DRV_INTR_LSC,
 	.probe = nicvf_eth_pci_probe,
 	.remove = nicvf_eth_pci_remove,
 };
-- 
2.13.0

Re: [dpdk-dev] [PATCH] net/thunderx: manage PCI device mapping for SQS VFs

[Ferruh Yigit]

On 6/1/2017 2:05 PM, Jerin Jacob wrote:
> Since the commit e84ad157b7bc ("pci: unmap resources if probe fails"),
> EAL unmaps the PCI device if ethdev probe returns positive or
> negative value.
> 
> nicvf thunderx PMD needs special treatment for Secondary queue set(SQS)
> PCIe VF devices, where, it expects to not unmap or free the memory
> without registering the ethdev subsystem.
> 
> To keep the same behavior, moved the PCI map function inside
> the driver without using the EAL services.

What do you think adding a flag something like
RTE_PCI_DRV_FIXED_MAPPING? Does mapping but not unmap on error.
This would be more generic solution.

I am concerned about calling eal level API from PMD.

> 
> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> Signed-off-by: Angela Czubak <aczubak@caviumnetworks.com>
> ---
>  drivers/net/thunderx/nicvf_ethdev.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/thunderx/nicvf_ethdev.c b/drivers/net/thunderx/nicvf_ethdev.c
> index 796701b0f..6ec2f9266 100644
> --- a/drivers/net/thunderx/nicvf_ethdev.c
> +++ b/drivers/net/thunderx/nicvf_ethdev.c
> @@ -2025,6 +2025,13 @@ nicvf_eth_dev_init(struct rte_eth_dev *eth_dev)
>  	}
>  
>  	pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);
> +
> +	ret = rte_pci_map_device(pci_dev);
> +	if (ret) {
> +		PMD_INIT_LOG(ERR, "Failed to map pci device");
> +		goto fail;
> +	}
> +
>  	rte_eth_copy_pci_info(eth_dev, pci_dev);
>  
>  	nic->device_id = pci_dev->id.device_id;
> @@ -2171,7 +2178,7 @@ static int nicvf_eth_pci_remove(struct rte_pci_device *pci_dev)
>  
>  static struct rte_pci_driver rte_nicvf_pmd = {
>  	.id_table = pci_id_nicvf_map,
> -	.drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC,
> +	.drv_flags = RTE_PCI_DRV_INTR_LSC,
>  	.probe = nicvf_eth_pci_probe,
>  	.remove = nicvf_eth_pci_remove,
>  };
> 

Re: [dpdk-dev] [PATCH] net/thunderx: manage PCI device mapping for SQS VFs

[Jerin Jacob]

-----Original Message-----
> Date: Tue, 6 Jun 2017 14:36:09 +0100
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, dev@dpdk.org
> CC: Angela Czubak <aczubak@caviumnetworks.com>, Thomas Monjalon
>  <thomas@monjalon.net>
> Subject: Re: [dpdk-dev] [PATCH] net/thunderx: manage PCI device mapping for
>  SQS VFs
> User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
>  Thunderbird/52.1.1
> 
> On 6/1/2017 2:05 PM, Jerin Jacob wrote:
> > Since the commit e84ad157b7bc ("pci: unmap resources if probe fails"),
> > EAL unmaps the PCI device if ethdev probe returns positive or
> > negative value.
> > 
> > nicvf thunderx PMD needs special treatment for Secondary queue set(SQS)
> > PCIe VF devices, where, it expects to not unmap or free the memory
> > without registering the ethdev subsystem.
> > 
> > To keep the same behavior, moved the PCI map function inside
> > the driver without using the EAL services.
> 
> What do you think adding a flag something like
> RTE_PCI_DRV_FIXED_MAPPING? Does mapping but not unmap on error.
> This would be more generic solution.
> 
> I am concerned about calling eal level API from PMD.

Understood.

Another option is to unmap only on ERROR(ie, when probe return <0 value)

	ret = dr->probe(dr, dev);
        if (ret) { // change to if (ret < 0)
                dev->driver = NULL;
                if (dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING)
                        rte_pci_unmap_device(dev);
        }

I am fine with either way. Let me know, what you prefer. I will
change accordingly.

>
> > 
> > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > Signed-off-by: Angela Czubak <aczubak@caviumnetworks.com>
> > ---
> >  drivers/net/thunderx/nicvf_ethdev.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/thunderx/nicvf_ethdev.c b/drivers/net/thunderx/nicvf_ethdev.c
> > index 796701b0f..6ec2f9266 100644
> > --- a/drivers/net/thunderx/nicvf_ethdev.c
> > +++ b/drivers/net/thunderx/nicvf_ethdev.c
> > @@ -2025,6 +2025,13 @@ nicvf_eth_dev_init(struct rte_eth_dev *eth_dev)
> >  	}
> >  
> >  	pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);
> > +
> > +	ret = rte_pci_map_device(pci_dev);
> > +	if (ret) {
> > +		PMD_INIT_LOG(ERR, "Failed to map pci device");
> > +		goto fail;
> > +	}
> > +
> >  	rte_eth_copy_pci_info(eth_dev, pci_dev);
> >  
> >  	nic->device_id = pci_dev->id.device_id;
> > @@ -2171,7 +2178,7 @@ static int nicvf_eth_pci_remove(struct rte_pci_device *pci_dev)
> >  
> >  static struct rte_pci_driver rte_nicvf_pmd = {
> >  	.id_table = pci_id_nicvf_map,
> > -	.drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC,
> > +	.drv_flags = RTE_PCI_DRV_INTR_LSC,
> >  	.probe = nicvf_eth_pci_probe,
> >  	.remove = nicvf_eth_pci_remove,
> >  };
> > 
> 

Re: [dpdk-dev] [PATCH] net/thunderx: manage PCI device mapping for SQS VFs

[Ferruh Yigit]

On 6/6/2017 3:05 PM, Jerin Jacob wrote:
> -----Original Message-----
>> Date: Tue, 6 Jun 2017 14:36:09 +0100
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, dev@dpdk.org
>> CC: Angela Czubak <aczubak@caviumnetworks.com>, Thomas Monjalon
>>  <thomas@monjalon.net>
>> Subject: Re: [dpdk-dev] [PATCH] net/thunderx: manage PCI device mapping for
>>  SQS VFs
>> User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
>>  Thunderbird/52.1.1
>>
>> On 6/1/2017 2:05 PM, Jerin Jacob wrote:
>>> Since the commit e84ad157b7bc ("pci: unmap resources if probe fails"),
>>> EAL unmaps the PCI device if ethdev probe returns positive or
>>> negative value.
>>>
>>> nicvf thunderx PMD needs special treatment for Secondary queue set(SQS)
>>> PCIe VF devices, where, it expects to not unmap or free the memory
>>> without registering the ethdev subsystem.
>>>
>>> To keep the same behavior, moved the PCI map function inside
>>> the driver without using the EAL services.
>>
>> What do you think adding a flag something like
>> RTE_PCI_DRV_FIXED_MAPPING? Does mapping but not unmap on error.
>> This would be more generic solution.
>>
>> I am concerned about calling eal level API from PMD.
> 
> Understood.
> 
> Another option is to unmap only on ERROR(ie, when probe return <0 value)
> 
> 	ret = dr->probe(dr, dev);
>         if (ret) { // change to if (ret < 0)
>                 dev->driver = NULL;
>                 if (dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING)
>                         rte_pci_unmap_device(dev);
>         }
> 
> I am fine with either way. Let me know, what you prefer. I will
> change accordingly.

"unmap only on ERROR" looks simpler, but it needs to be documented -in
the code, otherwise easy to miss in the future:

probe() return:
0   : success
< 0 : error, unmap resources
> 0 : error, no unmap

And requires to check that all existing drivers return negative error on
probe()


Adding new flag is more explicit, and no need to concern about what
other PMDs return, but can be overkill.


I would go with second option, but I guess both are OK, -as long as
behavior change in first one is commented in the code. Please pick one.

Thanks,
ferruh

> 
>>
>>>
>>> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>>> Signed-off-by: Angela Czubak <aczubak@caviumnetworks.com>
>>> ---
>>>  drivers/net/thunderx/nicvf_ethdev.c | 9 ++++++++-
>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/thunderx/nicvf_ethdev.c b/drivers/net/thunderx/nicvf_ethdev.c
>>> index 796701b0f..6ec2f9266 100644
>>> --- a/drivers/net/thunderx/nicvf_ethdev.c
>>> +++ b/drivers/net/thunderx/nicvf_ethdev.c
>>> @@ -2025,6 +2025,13 @@ nicvf_eth_dev_init(struct rte_eth_dev *eth_dev)
>>>  	}
>>>  
>>>  	pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);
>>> +
>>> +	ret = rte_pci_map_device(pci_dev);
>>> +	if (ret) {
>>> +		PMD_INIT_LOG(ERR, "Failed to map pci device");
>>> +		goto fail;
>>> +	}
>>> +
>>>  	rte_eth_copy_pci_info(eth_dev, pci_dev);
>>>  
>>>  	nic->device_id = pci_dev->id.device_id;
>>> @@ -2171,7 +2178,7 @@ static int nicvf_eth_pci_remove(struct rte_pci_device *pci_dev)
>>>  
>>>  static struct rte_pci_driver rte_nicvf_pmd = {
>>>  	.id_table = pci_id_nicvf_map,
>>> -	.drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC,
>>> +	.drv_flags = RTE_PCI_DRV_INTR_LSC,
>>>  	.probe = nicvf_eth_pci_probe,
>>>  	.remove = nicvf_eth_pci_remove,
>>>  };
>>>
>>

[dpdk-dev] [PATCH v2 1/2] eal/pci: introduce a PCI driver flag

[Jerin Jacob]

Some ethdev devices like nicvf thunderx PMD need special treatment for
Secondary queue set(SQS) PCIe VF devices, where, it expects to not unmap
or free the memory without registering the ethdev subsystem.

Introducing a new RTE_PCI_DRV_KEEP_MAPPED_RES
PCI driver flag to request PCI subsystem to not unmap the mapped PCI
resources(PCI BAR address) if unsupported device detected.

Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
---
v2:
Introduced RTE_PCI_DRV_KEEP_MAPPED_RES flag scheme(Ferruh),
Based on the discussion in the following thread.
http://dpdk.org/ml/archives/dev/2017-June/067091.htmlRTE_PCI_DRV_KEEP_MAPPED_RES
---
 lib/librte_eal/common/eal_common_pci.c  | 19 +++++++++++++++----
 lib/librte_eal/common/include/rte_pci.h |  2 ++
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
index 5ae520186..0d4e4a9f1 100644
--- a/lib/librte_eal/common/eal_common_pci.c
+++ b/lib/librte_eal/common/eal_common_pci.c
@@ -221,7 +221,12 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr,
 	ret = dr->probe(dr, dev);
 	if (ret) {
 		dev->driver = NULL;
-		if (dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING)
+		if ((dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING) &&
+			/* Don't unmap if device is unsupported and
+			 * driver needs mapped resources.
+			 */
+			!(ret > 0 &&
+				(dr->drv_flags & RTE_PCI_DRV_KEEP_MAPPED_RES)))
 			rte_pci_unmap_device(dev);
 	}
 
@@ -235,6 +240,7 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr,
 static int
 rte_pci_detach_dev(struct rte_pci_device *dev)
 {
+	int ret = 0;
 	struct rte_pci_addr *loc;
 	struct rte_pci_driver *dr;
 
@@ -251,13 +257,18 @@ rte_pci_detach_dev(struct rte_pci_device *dev)
 	RTE_LOG(DEBUG, EAL, "  remove driver: %x:%x %s\n", dev->id.vendor_id,
 			dev->id.device_id, dr->driver.name);
 
-	if (dr->remove && (dr->remove(dev) < 0))
-		return -1;	/* negative value is an error */
+	if (dr->remove) {
+		ret = dr->remove(dev);
+		if (ret < 0)
+			return -1; /* negative value is an error */
+	}
 
 	/* clear driver structure */
 	dev->driver = NULL;
 
-	if (dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING)
+	if ((dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING) &&
+	/* Don't unmap if dev is unsupported and it needs mapped resources */
+		!(ret > 0 && (dr->drv_flags & RTE_PCI_DRV_KEEP_MAPPED_RES)))
 		/* unmap resources for devices that use igb_uio */
 		rte_pci_unmap_device(dev);
 
diff --git a/lib/librte_eal/common/include/rte_pci.h b/lib/librte_eal/common/include/rte_pci.h
index b82ab9e79..0284a6208 100644
--- a/lib/librte_eal/common/include/rte_pci.h
+++ b/lib/librte_eal/common/include/rte_pci.h
@@ -212,6 +212,8 @@ struct rte_pci_bus {
 #define RTE_PCI_DRV_INTR_LSC	0x0008
 /** Device driver supports device removal interrupt */
 #define RTE_PCI_DRV_INTR_RMV 0x0010
+/** Device driver needs to keep mapped resources if unsupported dev detected */
+#define RTE_PCI_DRV_KEEP_MAPPED_RES 0x0020
 
 /**
  * A structure describing a PCI mapping.
-- 
2.13.1

[dpdk-dev] [PATCH v2 2/2] net/thunderx: manage PCI device mapping for SQS VFs

[Jerin Jacob]

Since the commit e84ad157b7bc ("pci: unmap resources if probe fails"),
EAL unmaps the PCI device if ethdev probe returns positive or
negative value.

nicvf thunderx PMD needs special treatment for Secondary queue set(SQS)
PCIe VF devices, where, it expects to not unmap or free the memory
without registering the ethdev subsystem.

Enable the same behavior by using RTE_PCI_DRV_KEEP_MAPPED_RES
PCI driver flag.

Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
---
 drivers/net/thunderx/nicvf_ethdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/thunderx/nicvf_ethdev.c b/drivers/net/thunderx/nicvf_ethdev.c
index 520ccc631..76f8101ea 100644
--- a/drivers/net/thunderx/nicvf_ethdev.c
+++ b/drivers/net/thunderx/nicvf_ethdev.c
@@ -2171,7 +2171,8 @@ static int nicvf_eth_pci_remove(struct rte_pci_device *pci_dev)
 
 static struct rte_pci_driver rte_nicvf_pmd = {
 	.id_table = pci_id_nicvf_map,
-	.drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC,
+	.drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_KEEP_MAPPED_RES |
+			RTE_PCI_DRV_INTR_LSC,
 	.probe = nicvf_eth_pci_probe,
 	.remove = nicvf_eth_pci_remove,
 };
-- 
2.13.1

Re: [dpdk-dev] [PATCH v2 1/2] eal/pci: introduce a PCI driver flag

[Ferruh Yigit]

On 6/8/2017 12:44 PM, Jerin Jacob wrote:
> Some ethdev devices like nicvf thunderx PMD need special treatment for
> Secondary queue set(SQS) PCIe VF devices, where, it expects to not unmap
> or free the memory without registering the ethdev subsystem.
> 
> Introducing a new RTE_PCI_DRV_KEEP_MAPPED_RES
> PCI driver flag to request PCI subsystem to not unmap the mapped PCI
> resources(PCI BAR address) if unsupported device detected.
> 
> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>

<...>

> @@ -235,6 +240,7 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr,
>  static int
>  rte_pci_detach_dev(struct rte_pci_device *dev)
>  {
> +	int ret = 0;
>  	struct rte_pci_addr *loc;
>  	struct rte_pci_driver *dr;
>  
> @@ -251,13 +257,18 @@ rte_pci_detach_dev(struct rte_pci_device *dev)
>  	RTE_LOG(DEBUG, EAL, "  remove driver: %x:%x %s\n", dev->id.vendor_id,
>  			dev->id.device_id, dr->driver.name);
>  
> -	if (dr->remove && (dr->remove(dev) < 0))
> -		return -1;	/* negative value is an error */
> +	if (dr->remove) {
> +		ret = dr->remove(dev);
> +		if (ret < 0)
> +			return -1; /* negative value is an error */
> +	}
>  
>  	/* clear driver structure */
>  	dev->driver = NULL;
>  
> -	if (dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING)
> +	if ((dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING) &&
> +	/* Don't unmap if dev is unsupported and it needs mapped resources */
> +		!(ret > 0 && (dr->drv_flags & RTE_PCI_DRV_KEEP_MAPPED_RES)))

Why it is required to keep mapping during detach?

>  		/* unmap resources for devices that use igb_uio */
>  		rte_pci_unmap_device(dev);
>  

<...>

Re: [dpdk-dev] [PATCH v2 1/2] eal/pci: introduce a PCI driver flag

[Jerin Jacob]

-----Original Message-----
> Date: Thu, 8 Jun 2017 15:40:33 +0100
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, dev@dpdk.org
> CC: thomas@monjalon.net
> Subject: Re: [dpdk-dev] [PATCH v2 1/2] eal/pci: introduce a PCI driver flag
> User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
>  Thunderbird/52.1.1
> 
> On 6/8/2017 12:44 PM, Jerin Jacob wrote:
> > Some ethdev devices like nicvf thunderx PMD need special treatment for
> > Secondary queue set(SQS) PCIe VF devices, where, it expects to not unmap
> > or free the memory without registering the ethdev subsystem.
> > 
> > Introducing a new RTE_PCI_DRV_KEEP_MAPPED_RES
> > PCI driver flag to request PCI subsystem to not unmap the mapped PCI
> > resources(PCI BAR address) if unsupported device detected.
> > 
> > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> 
> <...>
> 
> > @@ -235,6 +240,7 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr,
> >  static int
> >  rte_pci_detach_dev(struct rte_pci_device *dev)
> >  {
> > +	int ret = 0;
> >  	struct rte_pci_addr *loc;
> >  	struct rte_pci_driver *dr;
> >  
> > @@ -251,13 +257,18 @@ rte_pci_detach_dev(struct rte_pci_device *dev)
> >  	RTE_LOG(DEBUG, EAL, "  remove driver: %x:%x %s\n", dev->id.vendor_id,
> >  			dev->id.device_id, dr->driver.name);
> >  
> > -	if (dr->remove && (dr->remove(dev) < 0))
> > -		return -1;	/* negative value is an error */
> > +	if (dr->remove) {
> > +		ret = dr->remove(dev);
> > +		if (ret < 0)
> > +			return -1; /* negative value is an error */
> > +	}
> >  
> >  	/* clear driver structure */
> >  	dev->driver = NULL;
> >  
> > -	if (dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING)
> > +	if ((dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING) &&
> > +	/* Don't unmap if dev is unsupported and it needs mapped resources */
> > +		!(ret > 0 && (dr->drv_flags & RTE_PCI_DRV_KEEP_MAPPED_RES)))
> 
> Why it is required to keep mapping during detach?

To keep symmetrical with other(on probe) unmap change. This will
activated only when PMD returns the positive number on remove() so PMD
has control over it. The existing use case, We cannot just detach a single
VF(one SQS VF is _not_ one ethdev port i.e one ethdev port consists of
multiple VFs) so we need control on when to unmap those BARs.

> 
> >  		/* unmap resources for devices that use igb_uio */
> >  		rte_pci_unmap_device(dev);
> >  
> 
> <...>
> 

Re: [dpdk-dev] [PATCH v2 1/2] eal/pci: introduce a PCI driver flag

[Ferruh Yigit]

On 6/8/2017 6:15 PM, Jerin Jacob wrote:
> -----Original Message-----
>> Date: Thu, 8 Jun 2017 15:40:33 +0100
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, dev@dpdk.org
>> CC: thomas@monjalon.net
>> Subject: Re: [dpdk-dev] [PATCH v2 1/2] eal/pci: introduce a PCI driver flag
>> User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
>>  Thunderbird/52.1.1
>>
>> On 6/8/2017 12:44 PM, Jerin Jacob wrote:
>>> Some ethdev devices like nicvf thunderx PMD need special treatment for
>>> Secondary queue set(SQS) PCIe VF devices, where, it expects to not unmap
>>> or free the memory without registering the ethdev subsystem.
>>>
>>> Introducing a new RTE_PCI_DRV_KEEP_MAPPED_RES
>>> PCI driver flag to request PCI subsystem to not unmap the mapped PCI
>>> resources(PCI BAR address) if unsupported device detected.
>>>
>>> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>>
>> <...>
>>
>>> @@ -235,6 +240,7 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr,
>>>  static int
>>>  rte_pci_detach_dev(struct rte_pci_device *dev)
>>>  {
>>> +	int ret = 0;
>>>  	struct rte_pci_addr *loc;
>>>  	struct rte_pci_driver *dr;
>>>  
>>> @@ -251,13 +257,18 @@ rte_pci_detach_dev(struct rte_pci_device *dev)
>>>  	RTE_LOG(DEBUG, EAL, "  remove driver: %x:%x %s\n", dev->id.vendor_id,
>>>  			dev->id.device_id, dr->driver.name);
>>>  
>>> -	if (dr->remove && (dr->remove(dev) < 0))
>>> -		return -1;	/* negative value is an error */
>>> +	if (dr->remove) {
>>> +		ret = dr->remove(dev);
>>> +		if (ret < 0)
>>> +			return -1; /* negative value is an error */
>>> +	}
>>>  
>>>  	/* clear driver structure */
>>>  	dev->driver = NULL;
>>>  
>>> -	if (dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING)
>>> +	if ((dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING) &&
>>> +	/* Don't unmap if dev is unsupported and it needs mapped resources */
>>> +		!(ret > 0 && (dr->drv_flags & RTE_PCI_DRV_KEEP_MAPPED_RES)))
>>
>> Why it is required to keep mapping during detach?
> 
> To keep symmetrical with other(on probe) unmap change. This will
> activated only when PMD returns the positive number on remove() so PMD
> has control over it. The existing use case, We cannot just detach a single
> VF(one SQS VF is _not_ one ethdev port i.e one ethdev port consists of
> multiple VFs) so we need control on when to unmap those BARs.

For generic eal, there is an explicit request to detach the device, I am
not sure about returning success but not releasing the resources based
on PMD flag. How this will work with hotplug?


And specific to your case, -thanks for clarification, since no eth_dev
created for SQS VF, rte_eth_dev_pci_generic_remove() won't be useful but
assuming you have implemented your remove(), can it be possible to
detect SQS VF and act accordingly, or just return error perhaps if you
cannot detach that VF?

> 
>>
>>>  		/* unmap resources for devices that use igb_uio */
>>>  		rte_pci_unmap_device(dev);
>>>  
>>
>> <...>
>>

Re: [dpdk-dev] [PATCH v2 1/2] eal/pci: introduce a PCI driver flag

[Jerin Jacob]

-----Original Message-----
> Date: Thu, 8 Jun 2017 20:44:17 +0100
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> CC: dev@dpdk.org, thomas@monjalon.net
> Subject: Re: [dpdk-dev] [PATCH v2 1/2] eal/pci: introduce a PCI driver flag
> User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
>  Thunderbird/52.1.1
> 
> On 6/8/2017 6:15 PM, Jerin Jacob wrote:
> > -----Original Message-----
> >> Date: Thu, 8 Jun 2017 15:40:33 +0100
> >> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, dev@dpdk.org
> >> CC: thomas@monjalon.net
> >> Subject: Re: [dpdk-dev] [PATCH v2 1/2] eal/pci: introduce a PCI driver flag
> >> User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
> >>  Thunderbird/52.1.1
> >>
> >> On 6/8/2017 12:44 PM, Jerin Jacob wrote:
> >>> Some ethdev devices like nicvf thunderx PMD need special treatment for
> >>> Secondary queue set(SQS) PCIe VF devices, where, it expects to not unmap
> >>> or free the memory without registering the ethdev subsystem.
> >>>
> >>> Introducing a new RTE_PCI_DRV_KEEP_MAPPED_RES
> >>> PCI driver flag to request PCI subsystem to not unmap the mapped PCI
> >>> resources(PCI BAR address) if unsupported device detected.
> >>>
> >>> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> >>
> >> <...>
> >>
> >>> @@ -235,6 +240,7 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr,
> >>>  static int
> >>>  rte_pci_detach_dev(struct rte_pci_device *dev)
> >>>  {
> >>> +	int ret = 0;
> >>>  	struct rte_pci_addr *loc;
> >>>  	struct rte_pci_driver *dr;
> >>>  
> >>> @@ -251,13 +257,18 @@ rte_pci_detach_dev(struct rte_pci_device *dev)
> >>>  	RTE_LOG(DEBUG, EAL, "  remove driver: %x:%x %s\n", dev->id.vendor_id,
> >>>  			dev->id.device_id, dr->driver.name);
> >>>  
> >>> -	if (dr->remove && (dr->remove(dev) < 0))
> >>> -		return -1;	/* negative value is an error */
> >>> +	if (dr->remove) {
> >>> +		ret = dr->remove(dev);
> >>> +		if (ret < 0)
> >>> +			return -1; /* negative value is an error */
> >>> +	}
> >>>  
> >>>  	/* clear driver structure */
> >>>  	dev->driver = NULL;
> >>>  
> >>> -	if (dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING)
> >>> +	if ((dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING) &&
> >>> +	/* Don't unmap if dev is unsupported and it needs mapped resources */
> >>> +		!(ret > 0 && (dr->drv_flags & RTE_PCI_DRV_KEEP_MAPPED_RES)))
> >>
> >> Why it is required to keep mapping during detach?
> > 
> > To keep symmetrical with other(on probe) unmap change. This will
> > activated only when PMD returns the positive number on remove() so PMD
> > has control over it. The existing use case, We cannot just detach a single
> > VF(one SQS VF is _not_ one ethdev port i.e one ethdev port consists of
> > multiple VFs) so we need control on when to unmap those BARs.
> 
> For generic eal, there is an explicit request to detach the device, I am
> not sure about returning success but not releasing the resources based
> on PMD flag. How this will work with hotplug?

Again it is in the control of PMD. If PMD remove() returns 0 or <0 or
!RTE_PCI_DRV_KEEP_MAPPED_RES flag it will release the memory. If PMD is
keeping the resources it can free on primary(!SQS VF) VF detach.

> 
> 
> And specific to your case, -thanks for clarification, since no eth_dev
> created for SQS VF, rte_eth_dev_pci_generic_remove() won't be useful but
> assuming you have implemented your remove(), can it be possible to
> detect SQS VF and act accordingly, or just return error perhaps if you
> cannot detach that VF?

nicvf PMD is not advertising RTE_ETH_DEV_DETACHABLE capable and it is in
integrated internal bus so PCI hot-plug may not be a use case for this
PMD.

if you still think, RTE_PCI_DRV_KEEP_MAPPED_RES check needs to removed from
rte_pci_detach_dev(), I can do that send a new version.

> 
> > 
> >>
> >>>  		/* unmap resources for devices that use igb_uio */
> >>>  		rte_pci_unmap_device(dev);
> >>>  
> >>
> >> <...>
> >>
> 

Re: [dpdk-dev] [PATCH v2 1/2] eal/pci: introduce a PCI driver flag

[Ferruh Yigit]

On 6/9/2017 5:35 AM, Jerin Jacob wrote:
> -----Original Message-----
>> Date: Thu, 8 Jun 2017 20:44:17 +0100
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>> CC: dev@dpdk.org, thomas@monjalon.net
>> Subject: Re: [dpdk-dev] [PATCH v2 1/2] eal/pci: introduce a PCI driver flag
>> User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
>>  Thunderbird/52.1.1
>>
>> On 6/8/2017 6:15 PM, Jerin Jacob wrote:
>>> -----Original Message-----
>>>> Date: Thu, 8 Jun 2017 15:40:33 +0100
>>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, dev@dpdk.org
>>>> CC: thomas@monjalon.net
>>>> Subject: Re: [dpdk-dev] [PATCH v2 1/2] eal/pci: introduce a PCI driver flag
>>>> User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
>>>>  Thunderbird/52.1.1
>>>>
>>>> On 6/8/2017 12:44 PM, Jerin Jacob wrote:
>>>>> Some ethdev devices like nicvf thunderx PMD need special treatment for
>>>>> Secondary queue set(SQS) PCIe VF devices, where, it expects to not unmap
>>>>> or free the memory without registering the ethdev subsystem.
>>>>>
>>>>> Introducing a new RTE_PCI_DRV_KEEP_MAPPED_RES
>>>>> PCI driver flag to request PCI subsystem to not unmap the mapped PCI
>>>>> resources(PCI BAR address) if unsupported device detected.
>>>>>
>>>>> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>>>>
>>>> <...>
>>>>
>>>>> @@ -235,6 +240,7 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr,
>>>>>  static int
>>>>>  rte_pci_detach_dev(struct rte_pci_device *dev)
>>>>>  {
>>>>> +	int ret = 0;
>>>>>  	struct rte_pci_addr *loc;
>>>>>  	struct rte_pci_driver *dr;
>>>>>  
>>>>> @@ -251,13 +257,18 @@ rte_pci_detach_dev(struct rte_pci_device *dev)
>>>>>  	RTE_LOG(DEBUG, EAL, "  remove driver: %x:%x %s\n", dev->id.vendor_id,
>>>>>  			dev->id.device_id, dr->driver.name);
>>>>>  
>>>>> -	if (dr->remove && (dr->remove(dev) < 0))
>>>>> -		return -1;	/* negative value is an error */
>>>>> +	if (dr->remove) {
>>>>> +		ret = dr->remove(dev);
>>>>> +		if (ret < 0)
>>>>> +			return -1; /* negative value is an error */
>>>>> +	}
>>>>>  
>>>>>  	/* clear driver structure */
>>>>>  	dev->driver = NULL;
>>>>>  
>>>>> -	if (dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING)
>>>>> +	if ((dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING) &&
>>>>> +	/* Don't unmap if dev is unsupported and it needs mapped resources */
>>>>> +		!(ret > 0 && (dr->drv_flags & RTE_PCI_DRV_KEEP_MAPPED_RES)))
>>>>
>>>> Why it is required to keep mapping during detach?
>>>
>>> To keep symmetrical with other(on probe) unmap change. This will
>>> activated only when PMD returns the positive number on remove() so PMD
>>> has control over it. The existing use case, We cannot just detach a single
>>> VF(one SQS VF is _not_ one ethdev port i.e one ethdev port consists of
>>> multiple VFs) so we need control on when to unmap those BARs.
>>
>> For generic eal, there is an explicit request to detach the device, I am
>> not sure about returning success but not releasing the resources based
>> on PMD flag. How this will work with hotplug?
> 
> Again it is in the control of PMD. If PMD remove() returns 0 or <0 or
> !RTE_PCI_DRV_KEEP_MAPPED_RES flag it will release the memory. If PMD is
> keeping the resources it can free on primary(!SQS VF) VF detach.
> 
>>
>>
>> And specific to your case, -thanks for clarification, since no eth_dev
>> created for SQS VF, rte_eth_dev_pci_generic_remove() won't be useful but
>> assuming you have implemented your remove(), can it be possible to
>> detect SQS VF and act accordingly, or just return error perhaps if you
>> cannot detach that VF?
> 
> nicvf PMD is not advertising RTE_ETH_DEV_DETACHABLE capable and it is in
> integrated internal bus so PCI hot-plug may not be a use case for this
> PMD.
> 
> if you still think, RTE_PCI_DRV_KEEP_MAPPED_RES check needs to removed from
> rte_pci_detach_dev(), I can do that send a new version.

I see it is control of PMD but I think it shouldn't. If it is required
to unplug a device it should or return an error, not releasing them but
returning success doesn't sound right.

For your case it doesn't matter to have this. And the flag is there,
this can be added later if a special use case emerges. Right now I am
for removing check from detach.

Thanks,
ferruh

> 
>>
>>>
>>>>
>>>>>  		/* unmap resources for devices that use igb_uio */
>>>>>  		rte_pci_unmap_device(dev);
>>>>>  
>>>>
>>>> <...>
>>>>
>>

Re: [dpdk-dev] [PATCH v2 1/2] eal/pci: introduce a PCI driver flag

[Jerin Jacob]

-----Original Message-----
> Date: Fri, 9 Jun 2017 10:13:56 +0100
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> CC: dev@dpdk.org, thomas@monjalon.net
> Subject: Re: [dpdk-dev] [PATCH v2 1/2] eal/pci: introduce a PCI driver flag
> User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
>  Thunderbird/52.1.1
> 
> On 6/9/2017 5:35 AM, Jerin Jacob wrote:
> > -----Original Message-----
> >> Date: Thu, 8 Jun 2017 20:44:17 +0100
> >> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> >> CC: dev@dpdk.org, thomas@monjalon.net
> >> Subject: Re: [dpdk-dev] [PATCH v2 1/2] eal/pci: introduce a PCI driver flag
> >> User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
> >>  Thunderbird/52.1.1
> >>
> >> On 6/8/2017 6:15 PM, Jerin Jacob wrote:
> >>> -----Original Message-----
> >>>> Date: Thu, 8 Jun 2017 15:40:33 +0100
> >>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >>>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, dev@dpdk.org
> >>>> CC: thomas@monjalon.net
> >>>> Subject: Re: [dpdk-dev] [PATCH v2 1/2] eal/pci: introduce a PCI driver flag
> >>>> User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
> >>>>  Thunderbird/52.1.1
> >>>>
> >>>> On 6/8/2017 12:44 PM, Jerin Jacob wrote:
> >>>>> Some ethdev devices like nicvf thunderx PMD need special treatment for
> >>>>> Secondary queue set(SQS) PCIe VF devices, where, it expects to not unmap
> >>>>> or free the memory without registering the ethdev subsystem.
> >>>>>
> >>>>> Introducing a new RTE_PCI_DRV_KEEP_MAPPED_RES
> >>>>> PCI driver flag to request PCI subsystem to not unmap the mapped PCI
> >>>>> resources(PCI BAR address) if unsupported device detected.
> >>>>>
> >>>>> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> >>>>
> >>>> <...>
> >>>>
> >>>>> @@ -235,6 +240,7 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr,
> >>>>>  static int
> >>>>>  rte_pci_detach_dev(struct rte_pci_device *dev)
> >>>>>  {
> >>>>> +	int ret = 0;
> >>>>>  	struct rte_pci_addr *loc;
> >>>>>  	struct rte_pci_driver *dr;
> >>>>>  
> >>>>> @@ -251,13 +257,18 @@ rte_pci_detach_dev(struct rte_pci_device *dev)
> >>>>>  	RTE_LOG(DEBUG, EAL, "  remove driver: %x:%x %s\n", dev->id.vendor_id,
> >>>>>  			dev->id.device_id, dr->driver.name);
> >>>>>  
> >>>>> -	if (dr->remove && (dr->remove(dev) < 0))
> >>>>> -		return -1;	/* negative value is an error */
> >>>>> +	if (dr->remove) {
> >>>>> +		ret = dr->remove(dev);
> >>>>> +		if (ret < 0)
> >>>>> +			return -1; /* negative value is an error */
> >>>>> +	}
> >>>>>  
> >>>>>  	/* clear driver structure */
> >>>>>  	dev->driver = NULL;
> >>>>>  
> >>>>> -	if (dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING)
> >>>>> +	if ((dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING) &&
> >>>>> +	/* Don't unmap if dev is unsupported and it needs mapped resources */
> >>>>> +		!(ret > 0 && (dr->drv_flags & RTE_PCI_DRV_KEEP_MAPPED_RES)))
> >>>>
> >>>> Why it is required to keep mapping during detach?
> >>>
> >>> To keep symmetrical with other(on probe) unmap change. This will
> >>> activated only when PMD returns the positive number on remove() so PMD
> >>> has control over it. The existing use case, We cannot just detach a single
> >>> VF(one SQS VF is _not_ one ethdev port i.e one ethdev port consists of
> >>> multiple VFs) so we need control on when to unmap those BARs.
> >>
> >> For generic eal, there is an explicit request to detach the device, I am
> >> not sure about returning success but not releasing the resources based
> >> on PMD flag. How this will work with hotplug?
> > 
> > Again it is in the control of PMD. If PMD remove() returns 0 or <0 or
> > !RTE_PCI_DRV_KEEP_MAPPED_RES flag it will release the memory. If PMD is
> > keeping the resources it can free on primary(!SQS VF) VF detach.
> > 
> >>
> >>
> >> And specific to your case, -thanks for clarification, since no eth_dev
> >> created for SQS VF, rte_eth_dev_pci_generic_remove() won't be useful but
> >> assuming you have implemented your remove(), can it be possible to
> >> detect SQS VF and act accordingly, or just return error perhaps if you
> >> cannot detach that VF?
> > 
> > nicvf PMD is not advertising RTE_ETH_DEV_DETACHABLE capable and it is in
> > integrated internal bus so PCI hot-plug may not be a use case for this
> > PMD.
> > 
> > if you still think, RTE_PCI_DRV_KEEP_MAPPED_RES check needs to removed from
> > rte_pci_detach_dev(), I can do that send a new version.
> 
> I see it is control of PMD but I think it shouldn't. If it is required
> to unplug a device it should or return an error, not releasing them but
> returning success doesn't sound right.
> 
> For your case it doesn't matter to have this. And the flag is there,
> this can be added later if a special use case emerges. Right now I am
> for removing check from detach.

OK. Yes, As far the current use case concerned, We don't use that flag in
detach as detach is not supported. We will revisit this later.
I will send the next version with the flag removal in detach().

[dpdk-dev] [PATCH v3 1/2] eal/pci: introduce a PCI driver flag

[Jerin Jacob]

Some ethdev devices like nicvf thunderx PMD need special treatment for
Secondary queue set(SQS) PCIe VF devices, where, it expects to not unmap
or free the memory without registering the ethdev subsystem.

Introducing a new RTE_PCI_DRV_KEEP_MAPPED_RES
PCI driver flag to request PCI subsystem to not unmap the mapped PCI
resources(PCI BAR address) if unsupported device detected.

Suggested-by: Ferruh Yigit <ferruh.yigit@intel.com>
Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
---
v3:

Remove the use of RTE_PCI_DRV_KEEP_MAPPED_RES in detach()(Ferruh)
http://dpdk.org/ml/archives/dev/2017-June/067691.html

v2:
Introduced RTE_PCI_DRV_KEEP_MAPPED_RES flag scheme(Ferruh),
Based on the discussion in the following thread.
http://dpdk.org/ml/archives/dev/2017-June/067091.html
---
 lib/librte_eal/common/eal_common_pci.c  | 7 ++++++-
 lib/librte_eal/common/include/rte_pci.h | 2 ++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
index 5ae520186..78b097e61 100644
--- a/lib/librte_eal/common/eal_common_pci.c
+++ b/lib/librte_eal/common/eal_common_pci.c
@@ -221,7 +221,12 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr,
 	ret = dr->probe(dr, dev);
 	if (ret) {
 		dev->driver = NULL;
-		if (dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING)
+		if ((dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING) &&
+			/* Don't unmap if device is unsupported and
+			 * driver needs mapped resources.
+			 */
+			!(ret > 0 &&
+				(dr->drv_flags & RTE_PCI_DRV_KEEP_MAPPED_RES)))
 			rte_pci_unmap_device(dev);
 	}
 
diff --git a/lib/librte_eal/common/include/rte_pci.h b/lib/librte_eal/common/include/rte_pci.h
index b82ab9e79..0284a6208 100644
--- a/lib/librte_eal/common/include/rte_pci.h
+++ b/lib/librte_eal/common/include/rte_pci.h
@@ -212,6 +212,8 @@ struct rte_pci_bus {
 #define RTE_PCI_DRV_INTR_LSC	0x0008
 /** Device driver supports device removal interrupt */
 #define RTE_PCI_DRV_INTR_RMV 0x0010
+/** Device driver needs to keep mapped resources if unsupported dev detected */
+#define RTE_PCI_DRV_KEEP_MAPPED_RES 0x0020
 
 /**
  * A structure describing a PCI mapping.
-- 
2.13.1

[dpdk-dev] [PATCH v3 2/2] net/thunderx: manage PCI device mapping for SQS VFs

[Jerin Jacob]

Since the commit e84ad157b7bc ("pci: unmap resources if probe fails"),
EAL unmaps the PCI device if ethdev probe returns positive or
negative value.

nicvf thunderx PMD needs special treatment for Secondary queue set(SQS)
PCIe VF devices, where, it expects to not unmap or free the memory
without registering the ethdev subsystem.

Enable the same behavior by using RTE_PCI_DRV_KEEP_MAPPED_RES
PCI driver flag.

Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
---
 drivers/net/thunderx/nicvf_ethdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/thunderx/nicvf_ethdev.c b/drivers/net/thunderx/nicvf_ethdev.c
index 2152029b5..9d9f2c76e 100644
--- a/drivers/net/thunderx/nicvf_ethdev.c
+++ b/drivers/net/thunderx/nicvf_ethdev.c
@@ -2164,7 +2164,8 @@ static int nicvf_eth_pci_remove(struct rte_pci_device *pci_dev)
 
 static struct rte_pci_driver rte_nicvf_pmd = {
 	.id_table = pci_id_nicvf_map,
-	.drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC,
+	.drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_KEEP_MAPPED_RES |
+			RTE_PCI_DRV_INTR_LSC,
 	.probe = nicvf_eth_pci_probe,
 	.remove = nicvf_eth_pci_remove,
 };
-- 
2.13.1

Re: [dpdk-dev] [PATCH v3 1/2] eal/pci: introduce a PCI driver flag

[Ferruh Yigit]

On 6/9/2017 11:27 AM, Jerin Jacob wrote:
> Some ethdev devices like nicvf thunderx PMD need special treatment for
> Secondary queue set(SQS) PCIe VF devices, where, it expects to not unmap
> or free the memory without registering the ethdev subsystem.
> 
> Introducing a new RTE_PCI_DRV_KEEP_MAPPED_RES
> PCI driver flag to request PCI subsystem to not unmap the mapped PCI
> resources(PCI BAR address) if unsupported device detected.
> 
> Suggested-by: Ferruh Yigit <ferruh.yigit@intel.com>
> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>

Series Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Re: [dpdk-dev] [PATCH v3 1/2] eal/pci: introduce a PCI driver flag

[Ferruh Yigit]

On 6/9/2017 11:46 AM, Ferruh Yigit wrote:
> On 6/9/2017 11:27 AM, Jerin Jacob wrote:
>> Some ethdev devices like nicvf thunderx PMD need special treatment for
>> Secondary queue set(SQS) PCIe VF devices, where, it expects to not unmap
>> or free the memory without registering the ethdev subsystem.
>>
>> Introducing a new RTE_PCI_DRV_KEEP_MAPPED_RES
>> PCI driver flag to request PCI subsystem to not unmap the mapped PCI
>> resources(PCI BAR address) if unsupported device detected.
>>
>> Suggested-by: Ferruh Yigit <ferruh.yigit@intel.com>
>> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> 
> Series Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Series applied to dpdk-next-net/master, thanks.

Re: [dpdk-dev] [PATCH v3 1/2] eal/pci: introduce a PCI driver flag

[Thomas Monjalon]

09/06/2017 12:27, Jerin Jacob:
> Some ethdev devices like nicvf thunderx PMD need special treatment for
> Secondary queue set(SQS) PCIe VF devices, where, it expects to not unmap
> or free the memory without registering the ethdev subsystem.
> 
> Introducing a new RTE_PCI_DRV_KEEP_MAPPED_RES
> PCI driver flag to request PCI subsystem to not unmap the mapped PCI
> resources(PCI BAR address) if unsupported device detected.
> 
> Suggested-by: Ferruh Yigit <ferruh.yigit@intel.com>
> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
[...]
> --- a/lib/librte_eal/common/eal_common_pci.c
> +++ b/lib/librte_eal/common/eal_common_pci.c
> @@ -221,7 +221,12 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr,
>  	ret = dr->probe(dr, dev);
>  	if (ret) {
>  		dev->driver = NULL;
> -		if (dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING)
> +		if ((dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING) &&
> +			/* Don't unmap if device is unsupported and
> +			 * driver needs mapped resources.
> +			 */
> +			!(ret > 0 &&
> +				(dr->drv_flags & RTE_PCI_DRV_KEEP_MAPPED_RES)))
>  			rte_pci_unmap_device(dev);
>  	}
>  
> --- a/lib/librte_eal/common/include/rte_pci.h
> +++ b/lib/librte_eal/common/include/rte_pci.h
> +/** Device driver needs to keep mapped resources if unsupported dev detected */
> +#define RTE_PCI_DRV_KEEP_MAPPED_RES 0x0020

If I understand well, you want to map resources but not probe it?
Shouldn't it be less hacky to probe it as a (new) null class?

Re: [dpdk-dev] [PATCH v3 1/2] eal/pci: introduce a PCI driver flag

[Jerin Jacob]

-----Original Message-----
> Date: Mon, 12 Jun 2017 18:21:33 +0200
> From: Thomas Monjalon <thomas@monjalon.net>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> Cc: dev@dpdk.org, ferruh.yigit@intel.com
> Subject: Re: [dpdk-dev]  [PATCH v3 1/2] eal/pci: introduce a PCI driver flag
> 
> 09/06/2017 12:27, Jerin Jacob:
> > Some ethdev devices like nicvf thunderx PMD need special treatment for
> > Secondary queue set(SQS) PCIe VF devices, where, it expects to not unmap
> > or free the memory without registering the ethdev subsystem.
> > 
> > Introducing a new RTE_PCI_DRV_KEEP_MAPPED_RES
> > PCI driver flag to request PCI subsystem to not unmap the mapped PCI
> > resources(PCI BAR address) if unsupported device detected.
> > 
> > Suggested-by: Ferruh Yigit <ferruh.yigit@intel.com>
> > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> [...]
> > --- a/lib/librte_eal/common/eal_common_pci.c
> > +++ b/lib/librte_eal/common/eal_common_pci.c
> > @@ -221,7 +221,12 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr,
> >  	ret = dr->probe(dr, dev);
> >  	if (ret) {
> >  		dev->driver = NULL;
> > -		if (dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING)
> > +		if ((dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING) &&
> > +			/* Don't unmap if device is unsupported and
> > +			 * driver needs mapped resources.
> > +			 */
> > +			!(ret > 0 &&
> > +				(dr->drv_flags & RTE_PCI_DRV_KEEP_MAPPED_RES)))
> >  			rte_pci_unmap_device(dev);
> >  	}
> >  
> > --- a/lib/librte_eal/common/include/rte_pci.h
> > +++ b/lib/librte_eal/common/include/rte_pci.h
> > +/** Device driver needs to keep mapped resources if unsupported dev detected */
> > +#define RTE_PCI_DRV_KEEP_MAPPED_RES 0x0020
> 
> If I understand well, you want to map resources but not probe it?

Yes.

> Shouldn't it be less hacky to probe it as a (new) null class?

The Vendor and Class ID is same for those device too so we need to map
the PCI bar and have access to know the class of device. If you are concerned about
if it an common code change, My first version was without common code change.
http://dpdk.org/dev/patchwork/patch/24983/

Ferruh would like to have flag scheme, I think it make sense for
PMD maintenance perspective.

Re: [dpdk-dev] [PATCH v3 1/2] eal/pci: introduce a PCI driver flag

[Thomas Monjalon]

13/06/2017 06:43, Jerin Jacob:
> -----Original Message-----
> > Date: Mon, 12 Jun 2017 18:21:33 +0200
> > From: Thomas Monjalon <thomas@monjalon.net>
> > To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > Cc: dev@dpdk.org, ferruh.yigit@intel.com
> > Subject: Re: [dpdk-dev]  [PATCH v3 1/2] eal/pci: introduce a PCI driver flag
> > 
> > 09/06/2017 12:27, Jerin Jacob:
> > > Some ethdev devices like nicvf thunderx PMD need special treatment for
> > > Secondary queue set(SQS) PCIe VF devices, where, it expects to not unmap
> > > or free the memory without registering the ethdev subsystem.
> > > 
> > > Introducing a new RTE_PCI_DRV_KEEP_MAPPED_RES
> > > PCI driver flag to request PCI subsystem to not unmap the mapped PCI
> > > resources(PCI BAR address) if unsupported device detected.
> > > 
> > > Suggested-by: Ferruh Yigit <ferruh.yigit@intel.com>
> > > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > [...]
> > > --- a/lib/librte_eal/common/eal_common_pci.c
> > > +++ b/lib/librte_eal/common/eal_common_pci.c
> > > @@ -221,7 +221,12 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr,
> > >  	ret = dr->probe(dr, dev);
> > >  	if (ret) {
> > >  		dev->driver = NULL;
> > > -		if (dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING)
> > > +		if ((dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING) &&
> > > +			/* Don't unmap if device is unsupported and
> > > +			 * driver needs mapped resources.
> > > +			 */
> > > +			!(ret > 0 &&
> > > +				(dr->drv_flags & RTE_PCI_DRV_KEEP_MAPPED_RES)))
> > >  			rte_pci_unmap_device(dev);
> > >  	}
> > >  
> > > --- a/lib/librte_eal/common/include/rte_pci.h
> > > +++ b/lib/librte_eal/common/include/rte_pci.h
> > > +/** Device driver needs to keep mapped resources if unsupported dev detected */
> > > +#define RTE_PCI_DRV_KEEP_MAPPED_RES 0x0020
> > 
> > If I understand well, you want to map resources but not probe it?
> 
> Yes.
> 
> > Shouldn't it be less hacky to probe it as a (new) null class?
> 
> The Vendor and Class ID is same for those device too so we need to map
> the PCI bar and have access to know the class of device. If you are concerned about
> if it an common code change, My first version was without common code change.
> http://dpdk.org/dev/patchwork/patch/24983/
> 
> Ferruh would like to have flag scheme, I think it make sense for
> PMD maintenance perspective.

Yes

My idea was to have a new class of device interface to reserve those
resources, so the probe function would succeed.
Do you think it would be a good idea?

Re: [dpdk-dev] [PATCH v3 1/2] eal/pci: introduce a PCI driver flag

[Jerin Jacob]

-----Original Message-----
> Date: Tue, 13 Jun 2017 09:03:56 +0200
> From: Thomas Monjalon <thomas@monjalon.net>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> Cc: dev@dpdk.org, ferruh.yigit@intel.com
> Subject: Re: [dpdk-dev]  [PATCH v3 1/2] eal/pci: introduce a PCI driver flag
> 
> 13/06/2017 06:43, Jerin Jacob:
> > -----Original Message-----
> > > Date: Mon, 12 Jun 2017 18:21:33 +0200
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > Cc: dev@dpdk.org, ferruh.yigit@intel.com
> > > Subject: Re: [dpdk-dev]  [PATCH v3 1/2] eal/pci: introduce a PCI driver flag
> > > 
> > > 09/06/2017 12:27, Jerin Jacob:
> > > > Some ethdev devices like nicvf thunderx PMD need special treatment for
> > > > Secondary queue set(SQS) PCIe VF devices, where, it expects to not unmap
> > > > or free the memory without registering the ethdev subsystem.
> > > > 
> > > > Introducing a new RTE_PCI_DRV_KEEP_MAPPED_RES
> > > > PCI driver flag to request PCI subsystem to not unmap the mapped PCI
> > > > resources(PCI BAR address) if unsupported device detected.
> > > > 
> > > > Suggested-by: Ferruh Yigit <ferruh.yigit@intel.com>
> > > > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > [...]
> > > > --- a/lib/librte_eal/common/eal_common_pci.c
> > > > +++ b/lib/librte_eal/common/eal_common_pci.c
> > > > @@ -221,7 +221,12 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr,
> > > >  	ret = dr->probe(dr, dev);
> > > >  	if (ret) {
> > > >  		dev->driver = NULL;
> > > > -		if (dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING)
> > > > +		if ((dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING) &&
> > > > +			/* Don't unmap if device is unsupported and
> > > > +			 * driver needs mapped resources.
> > > > +			 */
> > > > +			!(ret > 0 &&
> > > > +				(dr->drv_flags & RTE_PCI_DRV_KEEP_MAPPED_RES)))
> > > >  			rte_pci_unmap_device(dev);
> > > >  	}
> > > >  
> > > > --- a/lib/librte_eal/common/include/rte_pci.h
> > > > +++ b/lib/librte_eal/common/include/rte_pci.h
> > > > +/** Device driver needs to keep mapped resources if unsupported dev detected */
> > > > +#define RTE_PCI_DRV_KEEP_MAPPED_RES 0x0020
> > > 
> > > If I understand well, you want to map resources but not probe it?
> > 
> > Yes.
> > 
> > > Shouldn't it be less hacky to probe it as a (new) null class?
> > 
> > The Vendor and Class ID is same for those device too so we need to map
> > the PCI bar and have access to know the class of device. If you are concerned about
> > if it an common code change, My first version was without common code change.
> > http://dpdk.org/dev/patchwork/patch/24983/
> > 
> > Ferruh would like to have flag scheme, I think it make sense for
> > PMD maintenance perspective.
> 
> Yes
> 
> My idea was to have a new class of device interface to reserve those
> resources, so the probe function would succeed.
> Do you think it would be a good idea?

Currently Kernel PF code creates 12 SRIOV VF devices per port(one VF device has
8 queues === 96 queues(12VFs) for 96 cores(thunderx max cores)), out of that 1 VF
device is _primary_ which mapped to dpdk ethdev port. If probe succeeds for
another 11 VFs then too may ethdev NULL ports show up. We can support
up to 12 ports(12*12 VF = 144 ports). I think, it is not good from
end user perceptive. We already have unsupported device concept in eal device
framework(when probe returns > 0). IMHO, it OK to keep as it for
simplicity.

Re: [dpdk-dev] [PATCH v3 1/2] eal/pci: introduce a PCI driver flag

[Thomas Monjalon]

13/06/2017 09:24, Jerin Jacob:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 13/06/2017 06:43, Jerin Jacob:
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > 09/06/2017 12:27, Jerin Jacob:
> > > > > Some ethdev devices like nicvf thunderx PMD need special treatment for
> > > > > Secondary queue set(SQS) PCIe VF devices, where, it expects to not unmap
> > > > > or free the memory without registering the ethdev subsystem.
> > > > > 
> > > > > Introducing a new RTE_PCI_DRV_KEEP_MAPPED_RES
> > > > > PCI driver flag to request PCI subsystem to not unmap the mapped PCI
> > > > > resources(PCI BAR address) if unsupported device detected.
> > > > > 
> > > > > Suggested-by: Ferruh Yigit <ferruh.yigit@intel.com>
> > > > > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > > [...]
> > > > > --- a/lib/librte_eal/common/eal_common_pci.c
> > > > > +++ b/lib/librte_eal/common/eal_common_pci.c
> > > > > @@ -221,7 +221,12 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr,
> > > > >  	ret = dr->probe(dr, dev);
> > > > >  	if (ret) {
> > > > >  		dev->driver = NULL;
> > > > > -		if (dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING)
> > > > > +		if ((dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING) &&
> > > > > +			/* Don't unmap if device is unsupported and
> > > > > +			 * driver needs mapped resources.
> > > > > +			 */
> > > > > +			!(ret > 0 &&
> > > > > +				(dr->drv_flags & RTE_PCI_DRV_KEEP_MAPPED_RES)))
> > > > >  			rte_pci_unmap_device(dev);
> > > > >  	}
> > > > >  
> > > > > --- a/lib/librte_eal/common/include/rte_pci.h
> > > > > +++ b/lib/librte_eal/common/include/rte_pci.h
> > > > > +/** Device driver needs to keep mapped resources if unsupported dev detected */
> > > > > +#define RTE_PCI_DRV_KEEP_MAPPED_RES 0x0020
> > > > 
> > > > If I understand well, you want to map resources but not probe it?
> > > 
> > > Yes.
> > > 
> > > > Shouldn't it be less hacky to probe it as a (new) null class?
> > > 
> > > The Vendor and Class ID is same for those device too so we need to map
> > > the PCI bar and have access to know the class of device. If you are concerned about
> > > if it an common code change, My first version was without common code change.
> > > http://dpdk.org/dev/patchwork/patch/24983/
> > > 
> > > Ferruh would like to have flag scheme, I think it make sense for
> > > PMD maintenance perspective.
> > 
> > Yes
> > 
> > My idea was to have a new class of device interface to reserve those
> > resources, so the probe function would succeed.
> > Do you think it would be a good idea?
> 
> Currently Kernel PF code creates 12 SRIOV VF devices per port(one VF device has
> 8 queues === 96 queues(12VFs) for 96 cores(thunderx max cores)), out of that 1 VF
> device is _primary_ which mapped to dpdk ethdev port. If probe succeeds for
> another 11 VFs then too may ethdev NULL ports show up. We can support
> up to 12 ports(12*12 VF = 144 ports). I think, it is not good from
> end user perceptive. We already have unsupported device concept in eal device
> framework(when probe returns > 0). IMHO, it OK to keep as it for
> simplicity.

Thanks for the explanation.
So we have a flag RTE_PCI_DRV_KEEP_MAPPED_RES for this kind of device.
Maybe someone else will think to another usage of a null interface,
so I would like to keep this idea floating in the air, just in case :)