[dpdk-dev,v4,4/4] virtio: return 1 to tell the upper layer we don't take over this device

  v4 changes:
 Rebase as io port map is moved to eal.
 Only fall back to PORT IO when there isn't any kernel driver (including
VFIO/UIO) managing the device. Before v4, we fall back to PORT IO even if
VFIO/UIO fails.
 Reword the commit message.

v3 changes:
 Change log message to tell user that the virtio device is skipped
due to it is managed by kernel driver, instead of asking user to
unbind it from kernel driver.

v2 changes:
 Remove unnecessary assignment of NULL to dev->data->mac_addrs.
 Ajust one comment's position.

virtio PMD could use IO port to configure the virtio device without
using UIO/VFIO driver in legacy mode.

There are two issues with the previous implementation:
1) virtio PMD will take over the virtio device(s) blindly even if not
intended for DPDK.
2) driver conflict between virtio PMD and virtio-net kernel driver.

This patch checks if there is kernel driver other than UIO/VFIO managing
the virtio device before using port IO.

If legacy_virtio_resource_init fails and kernel driver other than
VFIO/UIO is managing the device, return 1 to tell the upper layer we
don't take over this device.
For all other IO port mapping errors, return -1.

Note than if VFIO/UIO fails, now we don't fall back to port IO.

Fixes: da978dfdc43b ("virtio: use port IO to get PCI resource")

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
---
 drivers/net/virtio/virtio_ethdev.c |  9 +++++++--
 drivers/net/virtio/virtio_pci.c    | 15 ++++++++++++++-
 2 files changed, 21 insertions(+), 3 deletions(-)
  

On Fri, Feb 26, 2016 at 7:23 AM, Huawei Xie <huawei.xie@intel.com> wrote:
> v4 changes:
>  Rebase as io port map is moved to eal.
>  Only fall back to PORT IO when there isn't any kernel driver (including

Pl. mention that fallback behaviour applicable to x86 arch only..

However this patch fixes one problem in non-x86 arch issue, Example:
VM has 8 virtio interface and 2 i/f attached out of 8, so in default
case - after 2nd interface, ioport try to program 3..8 ports, result
to failure, lead to exit dpdk application. Patch fixes this problem
for non-x86 arch, test on arm64 platform.

> VFIO/UIO) managing the device. Before v4, we fall back to PORT IO even if
> VFIO/UIO fails.
>  Reword the commit message.
>
> v3 changes:
>  Change log message to tell user that the virtio device is skipped
> due to it is managed by kernel driver, instead of asking user to
> unbind it from kernel driver.
>
> v2 changes:
>  Remove unnecessary assignment of NULL to dev->data->mac_addrs.
>  Ajust one comment's position.
>
> virtio PMD could use IO port to configure the virtio device without
> using UIO/VFIO driver in legacy mode.
>
> There are two issues with the previous implementation:
> 1) virtio PMD will take over the virtio device(s) blindly even if not
> intended for DPDK.
> 2) driver conflict between virtio PMD and virtio-net kernel driver.
>
> This patch checks if there is kernel driver other than UIO/VFIO managing
> the virtio device before using port IO.
>
> If legacy_virtio_resource_init fails and kernel driver other than
> VFIO/UIO is managing the device, return 1 to tell the upper layer we
> don't take over this device.
> For all other IO port mapping errors, return -1.
>
> Note than if VFIO/UIO fails, now we don't fall back to port IO.
>
> Fixes: da978dfdc43b ("virtio: use port IO to get PCI resource")
>
> Signed-off-by: Huawei Xie <huawei.xie@intel.com>
> ---
>  drivers/net/virtio/virtio_ethdev.c |  9 +++++++--
>  drivers/net/virtio/virtio_pci.c    | 15 ++++++++++++++-
>  2 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index caa970c..8601080 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1,4 +1,5 @@
>  /*-
> +
>   *   BSD LICENSE
>   *
>   *   Copyright(c) 2010-2015 Intel Corporation. All rights reserved.
> @@ -1015,6 +1016,7 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
>         struct virtio_net_config *config;
>         struct virtio_net_config local_config;
>         struct rte_pci_device *pci_dev;
> +       int ret;
>
>         RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM < sizeof(struct virtio_net_hdr));
>
> @@ -1037,8 +1039,11 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
>
>         pci_dev = eth_dev->pci_dev;
>
> -       if (vtpci_init(pci_dev, hw) < 0)
> -               return -1;
> +       ret = vtpci_init(pci_dev, hw);
> +       if (ret) {
> +               rte_free(eth_dev->data->mac_addrs);
> +               return ret;
> +       }
>
>         /* Reset the device although not necessary at startup */
>         vtpci_reset(hw);
> diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
> index 85fbe88..f159b2a 100644
> --- a/drivers/net/virtio/virtio_pci.c
> +++ b/drivers/net/virtio/virtio_pci.c
> @@ -622,6 +622,13 @@ next:
>         return 0;
>  }
>
> +/*
> + * Return -1:
> + *   if there is error mapping with VFIO/UIO.
> + *   if port map error when driver type is KDRV_NONE.
> + * Return 1 if kernel driver is managing the device.
> + * Return 0 on success.
> + */
>  int
>  vtpci_init(struct rte_pci_device *dev, struct virtio_hw *hw)
>  {
> @@ -641,8 +648,14 @@ vtpci_init(struct rte_pci_device *dev, struct virtio_hw *hw)
>         }
>
>         PMD_INIT_LOG(INFO, "trying with legacy virtio pci.");
> -       if (legacy_virtio_resource_init(dev, hw) < 0)
> +       if (legacy_virtio_resource_init(dev, hw) < 0) {
> +               if (dev->kdrv == RTE_KDRV_UNKNOWN) {
> +                       PMD_INIT_LOG(INFO,
> +                               "skip kernel managed virtio device.");
> +                       return 1;
> +               }
>                 return -1;
> +       }
>
>         hw->vtpci_ops = &legacy_ops;
>         hw->use_msix = legacy_virtio_has_msix(&dev->addr);

Tested-by: Santosh Shukla <sshukla@mvista.com>
Acked-by: Santosh Shukla <sshukla@mvista.com>

> --
> 1.8.1.4
>
  

Hi Huawei,

2016-02-26 09:53, Huawei Xie:
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1,4 +1,5 @@
>  /*-
> +

This new line seems useless :)

>   *   BSD LICENSE
>   *
[...]
> @@ -1037,8 +1039,11 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
>  
>  	pci_dev = eth_dev->pci_dev;
>  
> -	if (vtpci_init(pci_dev, hw) < 0)
> -		return -1;
> +	ret = vtpci_init(pci_dev, hw);
> +	if (ret) {
> +		rte_free(eth_dev->data->mac_addrs);

The freeing seems not related to this patch.

> +		return ret;
> +	}
[...]
>  	PMD_INIT_LOG(INFO, "trying with legacy virtio pci.");
> -	if (legacy_virtio_resource_init(dev, hw) < 0)
> +	if (legacy_virtio_resource_init(dev, hw) < 0) {
> +		if (dev->kdrv == RTE_KDRV_UNKNOWN) {
> +			PMD_INIT_LOG(INFO,
> +				"skip kernel managed virtio device.");
> +			return 1;
> +		}
>  		return -1;
> +	}

You cannot skip a device if it was whitelisted.
I think you should check RTE_DEVTYPE_WHITELISTED_PCI and throw an error
in this case.
  

On 3/1/2016 3:18 PM, Thomas Monjalon wrote:
> Hi Huawei,
>
> 2016-02-26 09:53, Huawei Xie:
>> --- a/drivers/net/virtio/virtio_ethdev.c
>> +++ b/drivers/net/virtio/virtio_ethdev.c
>> @@ -1,4 +1,5 @@
>>  /*-
>> +
> This new line seems useless :)

Sorry, would fix.

>
>>   *   BSD LICENSE
>>   *
> [...]
>> @@ -1037,8 +1039,11 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
>>  
>>  	pci_dev = eth_dev->pci_dev;
>>  
>> -	if (vtpci_init(pci_dev, hw) < 0)
>> -		return -1;
>> +	ret = vtpci_init(pci_dev, hw);
>> +	if (ret) {
>> +		rte_free(eth_dev->data->mac_addrs);
> The freeing seems not related to this patch.

I can send a separate patch, ok within this patchset?

>
>> +		return ret;
>> +	}
> [...]
>>  	PMD_INIT_LOG(INFO, "trying with legacy virtio pci.");
>> -	if (legacy_virtio_resource_init(dev, hw) < 0)
>> +	if (legacy_virtio_resource_init(dev, hw) < 0) {
>> +		if (dev->kdrv == RTE_KDRV_UNKNOWN) {
>> +			PMD_INIT_LOG(INFO,
>> +				"skip kernel managed virtio device.");
>> +			return 1;
>> +		}
>>  		return -1;
>> +	}
> You cannot skip a device if it was whitelisted.
> I think you should check RTE_DEVTYPE_WHITELISTED_PCI and throw an error
> in this case.

I feel there is a subtle difference on the understanding of -w args. To
me, without it, probe all devices; with it, only probe whiltelisted API.
That is all.
Do you mean that -w implies that devices whitelisted must be probed
successfully otherwise we throw an error? If i get it right, then what
about the devices whitelisted but without PMD driver?

I will fix, :).
if (dev->kdrv == RTE_KDRV_UNKNOWN && dev->devargs->type !=
RTE_DEVTYPE_WHITELISTED_PCI) {
        ....
        return 1;
}
   

>
>
  

2016-03-01 07:53, Xie, Huawei:
> On 3/1/2016 3:18 PM, Thomas Monjalon wrote:
> > 2016-02-26 09:53, Huawei Xie:
> >> @@ -1037,8 +1039,11 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> >>  
> >>  	pci_dev = eth_dev->pci_dev;
> >>  
> >> -	if (vtpci_init(pci_dev, hw) < 0)
> >> -		return -1;
> >> +	ret = vtpci_init(pci_dev, hw);
> >> +	if (ret) {
> >> +		rte_free(eth_dev->data->mac_addrs);
> > The freeing seems not related to this patch.
> 
> I can send a separate patch, ok within this patchset?

Yes

> > [...]
> >>  	PMD_INIT_LOG(INFO, "trying with legacy virtio pci.");
> >> -	if (legacy_virtio_resource_init(dev, hw) < 0)
> >> +	if (legacy_virtio_resource_init(dev, hw) < 0) {
> >> +		if (dev->kdrv == RTE_KDRV_UNKNOWN) {
> >> +			PMD_INIT_LOG(INFO,
> >> +				"skip kernel managed virtio device.");
> >> +			return 1;
> >> +		}
> >>  		return -1;
> >> +	}
> > You cannot skip a device if it was whitelisted.
> > I think you should check RTE_DEVTYPE_WHITELISTED_PCI and throw an error
> > in this case.
> 
> I feel there is a subtle difference on the understanding of -w args. To
> me, without it, probe all devices; with it, only probe whiltelisted API.
> That is all.

I don't know if it is clearly documented indeed.

> Do you mean that -w implies that devices whitelisted must be probed
> successfully otherwise we throw an error? If i get it right, then what
> about the devices whitelisted but without PMD driver?

Yes we should probably consider the whitelist as a "forced" init.
Later, we could introduce some device flags for probing/discovery:
PROBE_AUTO, PROBE_FORCE, PROBE_IGNORE. It would make white/black list
more precise.

> I will fix, :).
> if (dev->kdrv == RTE_KDRV_UNKNOWN && dev->devargs->type !=
> RTE_DEVTYPE_WHITELISTED_PCI) {
>         ....
>         return 1;
> }

You should also consider the blacklist case: if there is a blacklist,
the not blacklisted devices must be initialised or throw an error.
  

On 3/1/2016 4:24 PM, Thomas Monjalon wrote:
> 2016-03-01 07:53, Xie, Huawei:
>> On 3/1/2016 3:18 PM, Thomas Monjalon wrote:
>>> 2016-02-26 09:53, Huawei Xie:
>>>> @@ -1037,8 +1039,11 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
>>>>  
>>>>  	pci_dev = eth_dev->pci_dev;
>>>>  
>>>> -	if (vtpci_init(pci_dev, hw) < 0)
>>>> -		return -1;
>>>> +	ret = vtpci_init(pci_dev, hw);
>>>> +	if (ret) {
>>>> +		rte_free(eth_dev->data->mac_addrs);
>>> The freeing seems not related to this patch.
>> I can send a separate patch, ok within this patchset?
> Yes
>
>>> [...]
>>>>  	PMD_INIT_LOG(INFO, "trying with legacy virtio pci.");
>>>> -	if (legacy_virtio_resource_init(dev, hw) < 0)
>>>> +	if (legacy_virtio_resource_init(dev, hw) < 0) {
>>>> +		if (dev->kdrv == RTE_KDRV_UNKNOWN) {
>>>> +			PMD_INIT_LOG(INFO,
>>>> +				"skip kernel managed virtio device.");
>>>> +			return 1;
>>>> +		}
>>>>  		return -1;
>>>> +	}
>>> You cannot skip a device if it was whitelisted.
>>> I think you should check RTE_DEVTYPE_WHITELISTED_PCI and throw an error
>>> in this case.
>> I feel there is a subtle difference on the understanding of -w args. To
>> me, without it, probe all devices; with it, only probe whiltelisted API.
>> That is all.
> I don't know if it is clearly documented indeed.
>
>> Do you mean that -w implies that devices whitelisted must be probed
>> successfully otherwise we throw an error? If i get it right, then what
>> about the devices whitelisted but without PMD driver?
> Yes we should probably consider the whitelist as a "forced" init.
> Later, we could introduce some device flags for probing/discovery:
> PROBE_AUTO, PROBE_FORCE, PROBE_IGNORE. It would make white/black list
> more precise.
>
>> I will fix, :).
>> if (dev->kdrv == RTE_KDRV_UNKNOWN && dev->devargs->type !=
>> RTE_DEVTYPE_WHITELISTED_PCI) {
>>         ....
>>         return 1;
>> }
> You should also consider the blacklist case: if there is a blacklist,
> the not blacklisted devices must be initialised or throw an error.
>
Don't we already skip probing the blacklisted device in
rte_eal_pci_probe_one_driver?
  

2016-03-01 08:39, Xie, Huawei:
> On 3/1/2016 4:24 PM, Thomas Monjalon wrote:
> > 2016-03-01 07:53, Xie, Huawei:
> >> On 3/1/2016 3:18 PM, Thomas Monjalon wrote:
> >>> 2016-02-26 09:53, Huawei Xie:
> >>>> @@ -1037,8 +1039,11 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> >>>>  
> >>>>  	pci_dev = eth_dev->pci_dev;
> >>>>  
> >>>> -	if (vtpci_init(pci_dev, hw) < 0)
> >>>> -		return -1;
> >>>> +	ret = vtpci_init(pci_dev, hw);
> >>>> +	if (ret) {
> >>>> +		rte_free(eth_dev->data->mac_addrs);
> >>> The freeing seems not related to this patch.
> >> I can send a separate patch, ok within this patchset?
> > Yes
> >
> >>> [...]
> >>>>  	PMD_INIT_LOG(INFO, "trying with legacy virtio pci.");
> >>>> -	if (legacy_virtio_resource_init(dev, hw) < 0)
> >>>> +	if (legacy_virtio_resource_init(dev, hw) < 0) {
> >>>> +		if (dev->kdrv == RTE_KDRV_UNKNOWN) {
> >>>> +			PMD_INIT_LOG(INFO,
> >>>> +				"skip kernel managed virtio device.");
> >>>> +			return 1;
> >>>> +		}
> >>>>  		return -1;
> >>>> +	}
> >>> You cannot skip a device if it was whitelisted.
> >>> I think you should check RTE_DEVTYPE_WHITELISTED_PCI and throw an error
> >>> in this case.
> >> I feel there is a subtle difference on the understanding of -w args. To
> >> me, without it, probe all devices; with it, only probe whiltelisted API.
> >> That is all.
> > I don't know if it is clearly documented indeed.
> >
> >> Do you mean that -w implies that devices whitelisted must be probed
> >> successfully otherwise we throw an error? If i get it right, then what
> >> about the devices whitelisted but without PMD driver?
> > Yes we should probably consider the whitelist as a "forced" init.
> > Later, we could introduce some device flags for probing/discovery:
> > PROBE_AUTO, PROBE_FORCE, PROBE_IGNORE. It would make white/black list
> > more precise.
> >
> >> I will fix, :).
> >> if (dev->kdrv == RTE_KDRV_UNKNOWN && dev->devargs->type !=
> >> RTE_DEVTYPE_WHITELISTED_PCI) {
> >>         ....
> >>         return 1;
> >> }
> > You should also consider the blacklist case: if there is a blacklist,
> > the not blacklisted devices must be initialised or throw an error.
> >
> Don't we already skip probing the blacklisted device in
> rte_eal_pci_probe_one_driver?

Yes, I'm talking about the devices which are not blacklisted.
Having some blacklisted devices imply that others are implicitly whitelisted.
  

On 3/1/2016 5:57 PM, Thomas Monjalon wrote:
> 2016-03-01 08:39, Xie, Huawei:
>> On 3/1/2016 4:24 PM, Thomas Monjalon wrote:
>>> 2016-03-01 07:53, Xie, Huawei:
>>>> On 3/1/2016 3:18 PM, Thomas Monjalon wrote:
>>>>> 2016-02-26 09:53, Huawei Xie:
>>>>>> @@ -1037,8 +1039,11 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
>>>>>>  
>>>>>>  	pci_dev = eth_dev->pci_dev;
>>>>>>  
>>>>>> -	if (vtpci_init(pci_dev, hw) < 0)
>>>>>> -		return -1;
>>>>>> +	ret = vtpci_init(pci_dev, hw);
>>>>>> +	if (ret) {
>>>>>> +		rte_free(eth_dev->data->mac_addrs);
>>>>> The freeing seems not related to this patch.
>>>> I can send a separate patch, ok within this patchset?
>>> Yes
>>>
>>>>> [...]
>>>>>>  	PMD_INIT_LOG(INFO, "trying with legacy virtio pci.");
>>>>>> -	if (legacy_virtio_resource_init(dev, hw) < 0)
>>>>>> +	if (legacy_virtio_resource_init(dev, hw) < 0) {
>>>>>> +		if (dev->kdrv == RTE_KDRV_UNKNOWN) {
>>>>>> +			PMD_INIT_LOG(INFO,
>>>>>> +				"skip kernel managed virtio device.");
>>>>>> +			return 1;
>>>>>> +		}
>>>>>>  		return -1;
>>>>>> +	}
>>>>> You cannot skip a device if it was whitelisted.
>>>>> I think you should check RTE_DEVTYPE_WHITELISTED_PCI and throw an error
>>>>> in this case.
>>>> I feel there is a subtle difference on the understanding of -w args. To
>>>> me, without it, probe all devices; with it, only probe whiltelisted API.
>>>> That is all.
>>> I don't know if it is clearly documented indeed.
>>>
>>>> Do you mean that -w implies that devices whitelisted must be probed
>>>> successfully otherwise we throw an error? If i get it right, then what
>>>> about the devices whitelisted but without PMD driver?
>>> Yes we should probably consider the whitelist as a "forced" init.
>>> Later, we could introduce some device flags for probing/discovery:
>>> PROBE_AUTO, PROBE_FORCE, PROBE_IGNORE. It would make white/black list
>>> more precise.
>>>
>>>> I will fix, :).
>>>> if (dev->kdrv == RTE_KDRV_UNKNOWN && dev->devargs->type !=
>>>> RTE_DEVTYPE_WHITELISTED_PCI) {
>>>>         ....
>>>>         return 1;
>>>> }
>>> You should also consider the blacklist case: if there is a blacklist,
>>> the not blacklisted devices must be initialised or throw an error.
>>>
>> Don't we already skip probing the blacklisted device in
>> rte_eal_pci_probe_one_driver?
> Yes, I'm talking about the devices which are not blacklisted.
> Having some blacklisted devices imply that others are implicitly whitelisted.

For blacklist, it only means the blacklisted device should be excluded
from being probed. It doesn't mean all other devices should be probed
either successfully or otherwise throw an error which cause DPDK exit.
Even that, the upper layer should explicitly put the non-blacklisted
device into whitelist, i mean here we should only deal with whitelist.
>
  

On 3/1/2016 6:09 PM, Xie, Huawei wrote:
> On 3/1/2016 5:57 PM, Thomas Monjalon wrote:
>> 2016-03-01 08:39, Xie, Huawei:
>>> On 3/1/2016 4:24 PM, Thomas Monjalon wrote:
>>>> 2016-03-01 07:53, Xie, Huawei:
>>>>> On 3/1/2016 3:18 PM, Thomas Monjalon wrote:
>>>>>> 2016-02-26 09:53, Huawei Xie:
>>>>>>> @@ -1037,8 +1039,11 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
>>>>>>>  
>>>>>>>  	pci_dev = eth_dev->pci_dev;
>>>>>>>  
>>>>>>> -	if (vtpci_init(pci_dev, hw) < 0)
>>>>>>> -		return -1;
>>>>>>> +	ret = vtpci_init(pci_dev, hw);
>>>>>>> +	if (ret) {
>>>>>>> +		rte_free(eth_dev->data->mac_addrs);
>>>>>> The freeing seems not related to this patch.
>>>>> I can send a separate patch, ok within this patchset?
>>>> Yes
>>>>
>>>>>> [...]
>>>>>>>  	PMD_INIT_LOG(INFO, "trying with legacy virtio pci.");
>>>>>>> -	if (legacy_virtio_resource_init(dev, hw) < 0)
>>>>>>> +	if (legacy_virtio_resource_init(dev, hw) < 0) {
>>>>>>> +		if (dev->kdrv == RTE_KDRV_UNKNOWN) {
>>>>>>> +			PMD_INIT_LOG(INFO,
>>>>>>> +				"skip kernel managed virtio device.");
>>>>>>> +			return 1;
>>>>>>> +		}
>>>>>>>  		return -1;
>>>>>>> +	}
>>>>>> You cannot skip a device if it was whitelisted.
>>>>>> I think you should check RTE_DEVTYPE_WHITELISTED_PCI and throw an error
>>>>>> in this case.
>>>>> I feel there is a subtle difference on the understanding of -w args. To
>>>>> me, without it, probe all devices; with it, only probe whiltelisted API.
>>>>> That is all.
>>>> I don't know if it is clearly documented indeed.
>>>>
>>>>> Do you mean that -w implies that devices whitelisted must be probed
>>>>> successfully otherwise we throw an error? If i get it right, then what
>>>>> about the devices whitelisted but without PMD driver?
>>>> Yes we should probably consider the whitelist as a "forced" init.
>>>> Later, we could introduce some device flags for probing/discovery:
>>>> PROBE_AUTO, PROBE_FORCE, PROBE_IGNORE. It would make white/black list
>>>> more precise.
>>>>
>>>>> I will fix, :).
>>>>> if (dev->kdrv == RTE_KDRV_UNKNOWN && dev->devargs->type !=
>>>>> RTE_DEVTYPE_WHITELISTED_PCI) {
>>>>>         ....
>>>>>         return 1;
>>>>> }
>>>> You should also consider the blacklist case: if there is a blacklist,
>>>> the not blacklisted devices must be initialised or throw an error.
>>>>
>>> Don't we already skip probing the blacklisted device in
>>> rte_eal_pci_probe_one_driver?
>> Yes, I'm talking about the devices which are not blacklisted.
>> Having some blacklisted devices imply that others are implicitly whitelisted.
> For blacklist, it only means the blacklisted device should be excluded
> from being probed. It doesn't mean all other devices should be probed
> either successfully or otherwise throw an error which cause DPDK exit.
> Even that, the upper layer should explicitly put the non-blacklisted
> device into whitelist, i mean here we should only deal with whitelist.

Thomas, comments?
Btw, i have reworked the patch, but git am always complains patch format
detection failed. Investigating. Will send it later.

>
  

2016-03-01 10:08, Xie, Huawei:
> On 3/1/2016 5:57 PM, Thomas Monjalon wrote:
> > 2016-03-01 08:39, Xie, Huawei:
> >> On 3/1/2016 4:24 PM, Thomas Monjalon wrote:
> >>> 2016-03-01 07:53, Xie, Huawei:
> >>>> On 3/1/2016 3:18 PM, Thomas Monjalon wrote:
> >>>>> 2016-02-26 09:53, Huawei Xie:
> >>>>>> @@ -1037,8 +1039,11 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> >>>>>>  
> >>>>>>  	pci_dev = eth_dev->pci_dev;
> >>>>>>  
> >>>>>> -	if (vtpci_init(pci_dev, hw) < 0)
> >>>>>> -		return -1;
> >>>>>> +	ret = vtpci_init(pci_dev, hw);
> >>>>>> +	if (ret) {
> >>>>>> +		rte_free(eth_dev->data->mac_addrs);
> >>>>> The freeing seems not related to this patch.
> >>>> I can send a separate patch, ok within this patchset?
> >>> Yes
> >>>
> >>>>> [...]
> >>>>>>  	PMD_INIT_LOG(INFO, "trying with legacy virtio pci.");
> >>>>>> -	if (legacy_virtio_resource_init(dev, hw) < 0)
> >>>>>> +	if (legacy_virtio_resource_init(dev, hw) < 0) {
> >>>>>> +		if (dev->kdrv == RTE_KDRV_UNKNOWN) {
> >>>>>> +			PMD_INIT_LOG(INFO,
> >>>>>> +				"skip kernel managed virtio device.");
> >>>>>> +			return 1;
> >>>>>> +		}
> >>>>>>  		return -1;
> >>>>>> +	}
> >>>>> You cannot skip a device if it was whitelisted.
> >>>>> I think you should check RTE_DEVTYPE_WHITELISTED_PCI and throw an error
> >>>>> in this case.
> >>>> I feel there is a subtle difference on the understanding of -w args. To
> >>>> me, without it, probe all devices; with it, only probe whiltelisted API.
> >>>> That is all.
> >>> I don't know if it is clearly documented indeed.
> >>>
> >>>> Do you mean that -w implies that devices whitelisted must be probed
> >>>> successfully otherwise we throw an error? If i get it right, then what
> >>>> about the devices whitelisted but without PMD driver?
> >>> Yes we should probably consider the whitelist as a "forced" init.
> >>> Later, we could introduce some device flags for probing/discovery:
> >>> PROBE_AUTO, PROBE_FORCE, PROBE_IGNORE. It would make white/black list
> >>> more precise.
> >>>
> >>>> I will fix, :).
> >>>> if (dev->kdrv == RTE_KDRV_UNKNOWN && dev->devargs->type !=
> >>>> RTE_DEVTYPE_WHITELISTED_PCI) {
> >>>>         ....
> >>>>         return 1;
> >>>> }
> >>> You should also consider the blacklist case: if there is a blacklist,
> >>> the not blacklisted devices must be initialised or throw an error.
> >>>
> >> Don't we already skip probing the blacklisted device in
> >> rte_eal_pci_probe_one_driver?
> > Yes, I'm talking about the devices which are not blacklisted.
> > Having some blacklisted devices imply that others are implicitly whitelisted.
> 
> For blacklist, it only means the blacklisted device should be excluded
> from being probed. It doesn't mean all other devices should be probed
> either successfully or otherwise throw an error which cause DPDK exit.

Yes it is.
Currently we have 3 cases:
- probe auto (best effort)
- whitelist (implicitly blacklist other devices)
- blacklist (implicitly whitelist other devices)

If you want to mix blacklist and best effort (auto probing), we must
set these requirements as per device flags instead of the current lists.

> Even that, the upper layer should explicitly put the non-blacklisted
> device into whitelist, i mean here we should only deal with whitelist.

It is not the way it is done currently. But some per-device flags could
be introduced later to make it explicit and deal only with the whitelist
flag (let's call it PROBE_FORCE).