[Question] How pmd virtio works without UIO?

[Question] How pmd virtio works without UIO?

[Peter Xu]

Hi,

I got a question related to how virtio pmd driver work without
UIO layer.

I see that in virtio PMD driver, DPDK will first try to init the
device using UIO interfaces. If it fails, it will try to init by
manipulating IO ports directly (see virtio_resource_init()).

For the ioport case, is it okay to do it like this? E.g., in
eth_virtio_dev_init(), we are resetting the virtio device, however,
this device should still be owned by virtio-pci driver in the
kernel.

How is that working? Did I miss anything?

Thanks in advance.
Peter

Re: [Question] How pmd virtio works without UIO?

[Yuanhan Liu]

On Tue, Dec 22, 2015 at 11:50:41AM +0800, Peter Xu wrote:
> Hi,
> 
> I got a question related to how virtio pmd driver work without
> UIO layer.
> 
> I see that in virtio PMD driver, DPDK will first try to init the
> device using UIO interfaces. If it fails, it will try to init by
> manipulating IO ports directly (see virtio_resource_init()).
> 
> For the ioport case, is it okay to do it like this? E.g., in
> eth_virtio_dev_init(), we are resetting the virtio device, however,
> this device should still be owned by virtio-pci driver in the
> kernel.
> 
> How is that working? Did I miss anything?

That's for configuration part: as far as we can read/write the right
PCI port, virtio pmd configuration will work. Note that on this case,
uio just provides another way to tell you where the port is.

	--yliu

Re: [Question] How pmd virtio works without UIO?

[Peter Xu]

On Tue, Dec 22, 2015 at 03:00:29PM +0800, Yuanhan Liu wrote:
> On Tue, Dec 22, 2015 at 11:50:41AM +0800, Peter Xu wrote:
> > Hi,
> > 
> > I got a question related to how virtio pmd driver work without
> > UIO layer.
> > 
> > I see that in virtio PMD driver, DPDK will first try to init the
> > device using UIO interfaces. If it fails, it will try to init by
> > manipulating IO ports directly (see virtio_resource_init()).
> > 
> > For the ioport case, is it okay to do it like this? E.g., in
> > eth_virtio_dev_init(), we are resetting the virtio device, however,
> > this device should still be owned by virtio-pci driver in the
> > kernel.
> > 
> > How is that working? Did I miss anything?
> 
> That's for configuration part: as far as we can read/write the right
> PCI port, virtio pmd configuration will work. Note that on this case,
> uio just provides another way to tell you where the port is.

Will there be any conflict? For example, when we start testpmd in
the guest without UIO (now, guest virtio net device is using
virtio-pci driver), we directly do read/write to IO ports of the
virtio device, reset it, and configure it. During the time,
virtio-pci driver of the virtio device should be still working (we
could see it by doing lspci -k), never knowing that the device is
reset. So... It seems like that both DPDK and virtio-pci are
manipulating the device. After that, for example, when host trigger
interrupt for guest vring, who (DPDK or virtio-pci) will handle it?

I would understand if we unbind the virtio-pci driver before taking
over the device. But it seems not.

Peter

> 
> 	--yliu

Re: [Question] How pmd virtio works without UIO?

[Yuanhan Liu]

On Tue, Dec 22, 2015 at 04:23:38PM +0800, Peter Xu wrote:
> On Tue, Dec 22, 2015 at 03:00:29PM +0800, Yuanhan Liu wrote:
> > On Tue, Dec 22, 2015 at 11:50:41AM +0800, Peter Xu wrote:
> > > Hi,
> > > 
> > > I got a question related to how virtio pmd driver work without
> > > UIO layer.
> > > 
> > > I see that in virtio PMD driver, DPDK will first try to init the
> > > device using UIO interfaces. If it fails, it will try to init by
> > > manipulating IO ports directly (see virtio_resource_init()).
> > > 
> > > For the ioport case, is it okay to do it like this? E.g., in
> > > eth_virtio_dev_init(), we are resetting the virtio device, however,
> > > this device should still be owned by virtio-pci driver in the
> > > kernel.
> > > 
> > > How is that working? Did I miss anything?
> > 
> > That's for configuration part: as far as we can read/write the right
> > PCI port, virtio pmd configuration will work. Note that on this case,
> > uio just provides another way to tell you where the port is.
> 
> Will there be any conflict? For example, when we start testpmd in
> the guest without UIO (now, guest virtio net device is using
> virtio-pci driver), we directly do read/write to IO ports of the
> virtio device, reset it, and configure it. During the time,
> virtio-pci driver of the virtio device should be still working (we
> could see it by doing lspci -k), never knowing that the device is
> reset. So... It seems like that both DPDK and virtio-pci are
> manipulating the device. After that, for example, when host trigger
> interrupt for guest vring, who (DPDK or virtio-pci) will handle it?
> 
> I would understand if we unbind the virtio-pci driver before taking
> over the device. But it seems not.

Actually, you are right. I mentioned in the last email that this is
for configuration part. To answer your question in this email, you
will not be able to go that further (say initiating virtio pmd) if
you don't unbind the origin virtio-net driver, and bind it to igb_uio
(or something similar).

The start point is from rte_eal_pci_scan, where the sub-function
pci_san_one just initates a DPDK bond driver.

	--yliu

Re: [Question] How pmd virtio works without UIO?

[Peter Xu]

On Tue, Dec 22, 2015 at 04:32:46PM +0800, Yuanhan Liu wrote:
> Actually, you are right. I mentioned in the last email that this is
> for configuration part. To answer your question in this email, you
> will not be able to go that further (say initiating virtio pmd) if
> you don't unbind the origin virtio-net driver, and bind it to igb_uio
> (or something similar).
> 
> The start point is from rte_eal_pci_scan, where the sub-function
> pci_san_one just initates a DPDK bond driver.

I am not sure whether I do understand your meaning correctly
(regarding "you willl not be able to go that furture"): The problem
is that, we _can_ run testpmd without unbinding the ports and bind
to UIO or something. What we need to do is boot the guest, reserve
huge pages, and run testpmd (keeping its kernel driver as
"virtio-pci"). In pci_scan_one():

	if (!ret) {
		if (!strcmp(driver, "vfio-pci"))
			dev->kdrv = RTE_KDRV_VFIO;
		else if (!strcmp(driver, "igb_uio"))
			dev->kdrv = RTE_KDRV_IGB_UIO;
		else if (!strcmp(driver, "uio_pci_generic"))
			dev->kdrv = RTE_KDRV_UIO_GENERIC;
		else
			dev->kdrv = RTE_KDRV_UNKNOWN;
	} else
		dev->kdrv = RTE_KDRV_UNKNOWN;

I think it should be going to RTE_KDRV_UNKNOWN
(driver=="virtio-pci") here. I tried to run IO and it could work,
but I am not sure whether it is safe, and how.

Also, I am not sure whether I need to (at least) unbind the
virtio-pci driver, so that there should have no kernel driver
running for the virtio device before DPDK using it.

Thanks
Peter

> 
> 	--yliu

Re: [Question] How pmd virtio works without UIO?

[Xie, Huawei]

On 12/22/2015 5:57 PM, Peter Xu wrote:
> On Tue, Dec 22, 2015 at 04:32:46PM +0800, Yuanhan Liu wrote:
>> Actually, you are right. I mentioned in the last email that this is
>> for configuration part. To answer your question in this email, you
>> will not be able to go that further (say initiating virtio pmd) if
>> you don't unbind the origin virtio-net driver, and bind it to igb_uio
>> (or something similar).
>>
>> The start point is from rte_eal_pci_scan, where the sub-function
>> pci_san_one just initates a DPDK bond driver.
> I am not sure whether I do understand your meaning correctly
> (regarding "you willl not be able to go that furture"): The problem
> is that, we _can_ run testpmd without unbinding the ports and bind
> to UIO or something. What we need to do is boot the guest, reserve
> huge pages, and run testpmd (keeping its kernel driver as
> "virtio-pci"). In pci_scan_one():
>
> 	if (!ret) {
> 		if (!strcmp(driver, "vfio-pci"))
> 			dev->kdrv = RTE_KDRV_VFIO;
> 		else if (!strcmp(driver, "igb_uio"))
> 			dev->kdrv = RTE_KDRV_IGB_UIO;
> 		else if (!strcmp(driver, "uio_pci_generic"))
> 			dev->kdrv = RTE_KDRV_UIO_GENERIC;
> 		else
> 			dev->kdrv = RTE_KDRV_UNKNOWN;
> 	} else
> 		dev->kdrv = RTE_KDRV_UNKNOWN;
>
> I think it should be going to RTE_KDRV_UNKNOWN
> (driver=="virtio-pci") here. I tried to run IO and it could work,
> but I am not sure whether it is safe, and how.

Good catch, peter.
Actually recently customers complain that with this feature, DPDK always
tries to take over this virtio-pci device, which is unwanted behavior.
Using blacklist could workaround this issue.
However, the real serious problem is that kernel driver is still
managing this device.

Changchun, Thomas:
I think we should fix this, but firstly i wonder why using port IO to
get PCI resource is more secure.

>
> Also, I am not sure whether I need to (at least) unbind the
> virtio-pci driver, so that there should have no kernel driver
> running for the virtio device before DPDK using it.
If you unbind, you have no entry under /proc/ioports for virtio IO port.
>
> Thanks
> Peter
>
>> 	--yliu

Re: [Question] How pmd virtio works without UIO?

[Xie, Huawei]

On 12/22/2015 6:48 PM, Xie, Huawei wrote:
> On 12/22/2015 5:57 PM, Peter Xu wrote:
>> On Tue, Dec 22, 2015 at 04:32:46PM +0800, Yuanhan Liu wrote:
>>> Actually, you are right. I mentioned in the last email that this is
>>> for configuration part. To answer your question in this email, you
>>> will not be able to go that further (say initiating virtio pmd) if
>>> you don't unbind the origin virtio-net driver, and bind it to igb_uio
>>> (or something similar).
>>>
>>> The start point is from rte_eal_pci_scan, where the sub-function
>>> pci_san_one just initates a DPDK bond driver.
>> I am not sure whether I do understand your meaning correctly
>> (regarding "you willl not be able to go that furture"): The problem
>> is that, we _can_ run testpmd without unbinding the ports and bind
>> to UIO or something. What we need to do is boot the guest, reserve
>> huge pages, and run testpmd (keeping its kernel driver as
>> "virtio-pci"). In pci_scan_one():
>>
>> 	if (!ret) {
>> 		if (!strcmp(driver, "vfio-pci"))
>> 			dev->kdrv = RTE_KDRV_VFIO;
>> 		else if (!strcmp(driver, "igb_uio"))
>> 			dev->kdrv = RTE_KDRV_IGB_UIO;
>> 		else if (!strcmp(driver, "uio_pci_generic"))
>> 			dev->kdrv = RTE_KDRV_UIO_GENERIC;
>> 		else
>> 			dev->kdrv = RTE_KDRV_UNKNOWN;
>> 	} else
>> 		dev->kdrv = RTE_KDRV_UNKNOWN;
>>
>> I think it should be going to RTE_KDRV_UNKNOWN
>> (driver=="virtio-pci") here. I tried to run IO and it could work,
>> but I am not sure whether it is safe, and how.
> Good catch, peter.
> Actually recently customers complain that with this feature, DPDK always
> tries to take over this virtio-pci device, which is unwanted behavior.
> Using blacklist could workaround this issue.
> However, the real serious problem is that kernel driver is still
> managing this device.
>
> Changchun, Thomas:
> I think we should fix this, but firstly i wonder why using port IO to
> get PCI resource is more secure.
>
>> Also, I am not sure whether I need to (at least) unbind the
>> virtio-pci driver, so that there should have no kernel driver
>> running for the virtio device before DPDK using it.
> If you unbind, you have no entry under /proc/ioports for virtio IO port.
>> Thanks
>> Peter
>>
>>> 	--yliu
>

Re: [Question] How pmd virtio works without UIO?

[Peter Xu]

On Tue, Dec 22, 2015 at 10:47:21AM +0000, Xie, Huawei wrote:
> On 12/22/2015 5:57 PM, Peter Xu wrote:
> > On Tue, Dec 22, 2015 at 04:32:46PM +0800, Yuanhan Liu wrote:
> >> Actually, you are right. I mentioned in the last email that this is
> >> for configuration part. To answer your question in this email, you
> >> will not be able to go that further (say initiating virtio pmd) if
> >> you don't unbind the origin virtio-net driver, and bind it to igb_uio
> >> (or something similar).
> >>
> >> The start point is from rte_eal_pci_scan, where the sub-function
> >> pci_san_one just initates a DPDK bond driver.
> > I am not sure whether I do understand your meaning correctly
> > (regarding "you willl not be able to go that furture"): The problem
> > is that, we _can_ run testpmd without unbinding the ports and bind
> > to UIO or something. What we need to do is boot the guest, reserve
> > huge pages, and run testpmd (keeping its kernel driver as
> > "virtio-pci"). In pci_scan_one():
> >
> > 	if (!ret) {
> > 		if (!strcmp(driver, "vfio-pci"))
> > 			dev->kdrv = RTE_KDRV_VFIO;
> > 		else if (!strcmp(driver, "igb_uio"))
> > 			dev->kdrv = RTE_KDRV_IGB_UIO;
> > 		else if (!strcmp(driver, "uio_pci_generic"))
> > 			dev->kdrv = RTE_KDRV_UIO_GENERIC;
> > 		else
> > 			dev->kdrv = RTE_KDRV_UNKNOWN;
> > 	} else
> > 		dev->kdrv = RTE_KDRV_UNKNOWN;
> >
> > I think it should be going to RTE_KDRV_UNKNOWN
> > (driver=="virtio-pci") here. I tried to run IO and it could work,
> > but I am not sure whether it is safe, and how.
> 
> Good catch, peter.
> Actually recently customers complain that with this feature, DPDK always
> tries to take over this virtio-pci device, which is unwanted behavior.
> Using blacklist could workaround this issue.
> However, the real serious problem is that kernel driver is still
> managing this device.
> 
> Changchun, Thomas:
> I think we should fix this, but firstly i wonder why using port IO to
> get PCI resource is more secure.

I am not familiar with this, however, shouldn't port IO from
userspace the most dangerous way to access PCI resource?

> 
> >
> > Also, I am not sure whether I need to (at least) unbind the
> > virtio-pci driver, so that there should have no kernel driver
> > running for the virtio device before DPDK using it.
> If you unbind, you have no entry under /proc/ioports for virtio IO port.

I tried to unbind one of the virtio net device, I see the PCI entry
still there.

Before unbind:

[root@vm proc]# lspci -k -s 00:03.0
00:03.0 Ethernet controller: Red Hat, Inc Virtio network device
        Subsystem: Red Hat, Inc Device 0001
        Kernel driver in use: virtio-pci
[root@vm proc]# cat /proc/ioports | grep c060-c07f
  c060-c07f : 0000:00:03.0
    c060-c07f : virtio-pci

After unbind:

[root@vm proc]# lspci -k -s 00:03.0
00:03.0 Ethernet controller: Red Hat, Inc Virtio network device
        Subsystem: Red Hat, Inc Device 0001
[root@vm proc]# cat /proc/ioports | grep c060-c07f
  c060-c07f : 0000:00:03.0

So... does this means that it is an alternative to black list
solution?

Peter

> >
> > Thanks
> > Peter
> >
> >> 	--yliu
> 

Re: [Question] How pmd virtio works without UIO?

[Xie, Huawei]

On 12/22/2015 7:39 PM, Peter Xu wrote:
> On Tue, Dec 22, 2015 at 10:47:21AM +0000, Xie, Huawei wrote:
>> On 12/22/2015 5:57 PM, Peter Xu wrote:
>>> On Tue, Dec 22, 2015 at 04:32:46PM +0800, Yuanhan Liu wrote:
>>>> Actually, you are right. I mentioned in the last email that this is
>>>> for configuration part. To answer your question in this email, you
>>>> will not be able to go that further (say initiating virtio pmd) if
>>>> you don't unbind the origin virtio-net driver, and bind it to igb_uio
>>>> (or something similar).
>>>>
>>>> The start point is from rte_eal_pci_scan, where the sub-function
>>>> pci_san_one just initates a DPDK bond driver.
>>> I am not sure whether I do understand your meaning correctly
>>> (regarding "you willl not be able to go that furture"): The problem
>>> is that, we _can_ run testpmd without unbinding the ports and bind
>>> to UIO or something. What we need to do is boot the guest, reserve
>>> huge pages, and run testpmd (keeping its kernel driver as
>>> "virtio-pci"). In pci_scan_one():
>>>
>>> 	if (!ret) {
>>> 		if (!strcmp(driver, "vfio-pci"))
>>> 			dev->kdrv = RTE_KDRV_VFIO;
>>> 		else if (!strcmp(driver, "igb_uio"))
>>> 			dev->kdrv = RTE_KDRV_IGB_UIO;
>>> 		else if (!strcmp(driver, "uio_pci_generic"))
>>> 			dev->kdrv = RTE_KDRV_UIO_GENERIC;
>>> 		else
>>> 			dev->kdrv = RTE_KDRV_UNKNOWN;
>>> 	} else
>>> 		dev->kdrv = RTE_KDRV_UNKNOWN;
>>>
>>> I think it should be going to RTE_KDRV_UNKNOWN
>>> (driver=="virtio-pci") here. I tried to run IO and it could work,
>>> but I am not sure whether it is safe, and how.
>> Good catch, peter.
>> Actually recently customers complain that with this feature, DPDK always
>> tries to take over this virtio-pci device, which is unwanted behavior.
>> Using blacklist could workaround this issue.
>> However, the real serious problem is that kernel driver is still
>> managing this device.
>>
>> Changchun, Thomas:
>> I think we should fix this, but firstly i wonder why using port IO to
>> get PCI resource is more secure.
> I am not familiar with this, however, shouldn't port IO from
> userspace the most dangerous way to access PCI resource?
>
>>> Also, I am not sure whether I need to (at least) unbind the
>>> virtio-pci driver, so that there should have no kernel driver
>>> running for the virtio device before DPDK using it.
>> If you unbind, you have no entry under /proc/ioports for virtio IO port.
> I tried to unbind one of the virtio net device, I see the PCI entry
> still there.
>
> Before unbind:
>
> [root@vm proc]# lspci -k -s 00:03.0
> 00:03.0 Ethernet controller: Red Hat, Inc Virtio network device
>         Subsystem: Red Hat, Inc Device 0001
>         Kernel driver in use: virtio-pci
> [root@vm proc]# cat /proc/ioports | grep c060-c07f
>   c060-c07f : 0000:00:03.0
>     c060-c07f : virtio-pci
>
> After unbind:
>
> [root@vm proc]# lspci -k -s 00:03.0
> 00:03.0 Ethernet controller: Red Hat, Inc Virtio network device
>         Subsystem: Red Hat, Inc Device 0001
> [root@vm proc]# cat /proc/ioports | grep c060-c07f
>   c060-c07f : 0000:00:03.0
>
> So... does this means that it is an alternative to black list
> solution?
Users wants to use virtio-net device for normal communication, so they
still want virtio-net driver to manipulate this device. They don't want
DPDK to take over this device blindly.
>
> Peter
>
>>> Thanks
>>> Peter
>>>
>>>> 	--yliu

Re: [Question] How pmd virtio works without UIO?

[Xie, Huawei]

On 12/22/2015 7:39 PM, Peter Xu wrote:
> On Tue, Dec 22, 2015 at 10:47:21AM +0000, Xie, Huawei wrote:
>> On 12/22/2015 5:57 PM, Peter Xu wrote:
>>> On Tue, Dec 22, 2015 at 04:32:46PM +0800, Yuanhan Liu wrote:
>>>> Actually, you are right. I mentioned in the last email that this is
>>>> for configuration part. To answer your question in this email, you
>>>> will not be able to go that further (say initiating virtio pmd) if
>>>> you don't unbind the origin virtio-net driver, and bind it to igb_uio
>>>> (or something similar).
>>>>
>>>> The start point is from rte_eal_pci_scan, where the sub-function
>>>> pci_san_one just initates a DPDK bond driver.
>>> I am not sure whether I do understand your meaning correctly
>>> (regarding "you willl not be able to go that furture"): The problem
>>> is that, we _can_ run testpmd without unbinding the ports and bind
>>> to UIO or something. What we need to do is boot the guest, reserve
>>> huge pages, and run testpmd (keeping its kernel driver as
>>> "virtio-pci"). In pci_scan_one():
>>>
>>> 	if (!ret) {
>>> 		if (!strcmp(driver, "vfio-pci"))
>>> 			dev->kdrv = RTE_KDRV_VFIO;
>>> 		else if (!strcmp(driver, "igb_uio"))
>>> 			dev->kdrv = RTE_KDRV_IGB_UIO;
>>> 		else if (!strcmp(driver, "uio_pci_generic"))
>>> 			dev->kdrv = RTE_KDRV_UIO_GENERIC;
>>> 		else
>>> 			dev->kdrv = RTE_KDRV_UNKNOWN;
>>> 	} else
>>> 		dev->kdrv = RTE_KDRV_UNKNOWN;
>>>
>>> I think it should be going to RTE_KDRV_UNKNOWN
>>> (driver=="virtio-pci") here. I tried to run IO and it could work,
>>> but I am not sure whether it is safe, and how.
>> Good catch, peter.
>> Actually recently customers complain that with this feature, DPDK always
>> tries to take over this virtio-pci device, which is unwanted behavior.
>> Using blacklist could workaround this issue.
>> However, the real serious problem is that kernel driver is still
>> managing this device.
>>
>> Changchun, Thomas:
>> I think we should fix this, but firstly i wonder why using port IO to
>> get PCI resource is more secure.
> I am not familiar with this, however, shouldn't port IO from
> userspace the most dangerous way to access PCI resource?
>
>>> Also, I am not sure whether I need to (at least) unbind the
>>> virtio-pci driver, so that there should have no kernel driver
>>> running for the virtio device before DPDK using it.
>> If you unbind, you have no entry under /proc/ioports for virtio IO port.
> I tried to unbind one of the virtio net device, I see the PCI entry
> still there.
>
> Before unbind:
>
> [root@vm proc]# lspci -k -s 00:03.0
> 00:03.0 Ethernet controller: Red Hat, Inc Virtio network device
>         Subsystem: Red Hat, Inc Device 0001
>         Kernel driver in use: virtio-pci
> [root@vm proc]# cat /proc/ioports | grep c060-c07f
>   c060-c07f : 0000:00:03.0
>     c060-c07f : virtio-pci
>
> After unbind:
>
> [root@vm proc]# lspci -k -s 00:03.0
> 00:03.0 Ethernet controller: Red Hat, Inc Virtio network device
>         Subsystem: Red Hat, Inc Device 0001
> [root@vm proc]# cat /proc/ioports | grep c060-c07f
>   c060-c07f : 0000:00:03.0
>
> So... does this means that it is an alternative to black list
> solution?
Oh, we could firstly check if this port is manipulated by kernel driver
in virtio_resource_init/eth_virtio_dev_init, as long as it is not too late.
>
> Peter
>
>>> Thanks
>>> Peter
>>>
>>>> 	--yliu

Re: [Question] How pmd virtio works without UIO?

[Peter Xu]

On Tue, Dec 22, 2015 at 04:38:30PM +0000, Xie, Huawei wrote:
> On 12/22/2015 7:39 PM, Peter Xu wrote:
> > I tried to unbind one of the virtio net device, I see the PCI entry
> > still there.
> >
> > Before unbind:
> >
> > [root@vm proc]# lspci -k -s 00:03.0
> > 00:03.0 Ethernet controller: Red Hat, Inc Virtio network device
> >         Subsystem: Red Hat, Inc Device 0001
> >         Kernel driver in use: virtio-pci
> > [root@vm proc]# cat /proc/ioports | grep c060-c07f
> >   c060-c07f : 0000:00:03.0
> >     c060-c07f : virtio-pci
> >
> > After unbind:
> >
> > [root@vm proc]# lspci -k -s 00:03.0
> > 00:03.0 Ethernet controller: Red Hat, Inc Virtio network device
> >         Subsystem: Red Hat, Inc Device 0001
> > [root@vm proc]# cat /proc/ioports | grep c060-c07f
> >   c060-c07f : 0000:00:03.0
> >
> > So... does this means that it is an alternative to black list
> > solution?
> Oh, we could firstly check if this port is manipulated by kernel driver
> in virtio_resource_init/eth_virtio_dev_init, as long as it is not too late.

I guess there might be two problems? Which are:

1. How user avoid DPDK taking over virtio devices that they do not
   want for IO (chooses which device to use)

2. Driver conflict between virtio PMD in DPDK, and virtio-pci in
   kernel (happens on every virtio device that DPDK uses)

For the white/black list solution, I guess it's good enough to solve
(1) for customers. I am just curious about the 2nd.

Or say, even we black listed some virtio devices (or doing white
list), the virtio devices used by DPDK are still in danger if we
cannot make sure that virtio-pci will not touch the device any more
(even it will not touch it, it feels like errornous to not telling
virtio-pci to remove it before hand). E.g., if virtio-pci interrupt
is still working, when there are packets from outside to guest,
vp_interrupt() might be called? Then virtio-pci driver might do
read/write to vring as well? If so, that's problematic. Am I wrong?

Peter

Re: [Question] How pmd virtio works without UIO?

[Yuanhan Liu]

On Tue, Dec 22, 2015 at 05:56:41PM +0800, Peter Xu wrote:
> On Tue, Dec 22, 2015 at 04:32:46PM +0800, Yuanhan Liu wrote:
> > Actually, you are right. I mentioned in the last email that this is
> > for configuration part. To answer your question in this email, you
> > will not be able to go that further (say initiating virtio pmd) if
> > you don't unbind the origin virtio-net driver, and bind it to igb_uio
> > (or something similar).
> > 
> > The start point is from rte_eal_pci_scan, where the sub-function
> > pci_san_one just initates a DPDK bond driver.
> 
> I am not sure whether I do understand your meaning correctly
> (regarding "you willl not be able to go that furture"): The problem
> is that, we _can_ run testpmd without unbinding the ports and bind
> to UIO or something. What we need to do is boot the guest, reserve
> huge pages, and run testpmd (keeping its kernel driver as
> "virtio-pci"). In pci_scan_one():
> 
> 	if (!ret) {
> 		if (!strcmp(driver, "vfio-pci"))
> 			dev->kdrv = RTE_KDRV_VFIO;
> 		else if (!strcmp(driver, "igb_uio"))
> 			dev->kdrv = RTE_KDRV_IGB_UIO;
> 		else if (!strcmp(driver, "uio_pci_generic"))
> 			dev->kdrv = RTE_KDRV_UIO_GENERIC;
> 		else
> 			dev->kdrv = RTE_KDRV_UNKNOWN;
> 	} else
> 		dev->kdrv = RTE_KDRV_UNKNOWN;
> 
> I think it should be going to RTE_KDRV_UNKNOWN
> (driver=="virtio-pci") here.

Sorry, I simply overlook that. I was thinking it will quit here for
the RTE_KDRV_UNKNOWN case.

> I tried to run IO and it could work,
> but I am not sure whether it is safe, and how.

I also did a quick test then, however, with the virtio 1.0 patchset
I sent before, which sets the RTE_PCI_DRV_NEED_MAPPING, resulting to
pci_map_device() failure and virtio pmd is not initiated at all.

> 
> Also, I am not sure whether I need to (at least) unbind the
> virtio-pci driver, so that there should have no kernel driver
> running for the virtio device before DPDK using it.

Why not? That's what the DPDK document asked to do
(http://dpdk.org/doc/guides/linux_gsg/build_dpdk.html):

    3.6. Binding and Unbinding Network Ports to/from the Kernel Modules
    
    As of release 1.4, DPDK applications no longer automatically unbind
    all supported network ports from the kernel driver in use. Instead,
    all ports that are to be used by an DPDK application must be bound
    to the uio_pci_generic, igb_uio or vfio-pci module before the
    application is run. Any network ports under Linux* control will be
    ignored by the DPDK poll-mode drivers and cannot be used by the
    application.


	--yliu

Re: [Question] How pmd virtio works without UIO?

[Yuanhan Liu]

On Wed, Dec 23, 2015 at 09:55:54AM +0800, Peter Xu wrote:
> On Tue, Dec 22, 2015 at 04:38:30PM +0000, Xie, Huawei wrote:
> > On 12/22/2015 7:39 PM, Peter Xu wrote:
> > > I tried to unbind one of the virtio net device, I see the PCI entry
> > > still there.
> > >
> > > Before unbind:
> > >
> > > [root@vm proc]# lspci -k -s 00:03.0
> > > 00:03.0 Ethernet controller: Red Hat, Inc Virtio network device
> > >         Subsystem: Red Hat, Inc Device 0001
> > >         Kernel driver in use: virtio-pci
> > > [root@vm proc]# cat /proc/ioports | grep c060-c07f
> > >   c060-c07f : 0000:00:03.0
> > >     c060-c07f : virtio-pci
> > >
> > > After unbind:
> > >
> > > [root@vm proc]# lspci -k -s 00:03.0
> > > 00:03.0 Ethernet controller: Red Hat, Inc Virtio network device
> > >         Subsystem: Red Hat, Inc Device 0001
> > > [root@vm proc]# cat /proc/ioports | grep c060-c07f
> > >   c060-c07f : 0000:00:03.0
> > >
> > > So... does this means that it is an alternative to black list
> > > solution?
> > Oh, we could firstly check if this port is manipulated by kernel driver
> > in virtio_resource_init/eth_virtio_dev_init, as long as it is not too late.

Why can't we simply quit at pci_scan_one, once finding that it's not
bond to uio (or similar stuff)? That would be generic enough, that we
don't have to do similar checks for each new pmd driver.

Or, am I missing something?


> I guess there might be two problems? Which are:
> 
> 1. How user avoid DPDK taking over virtio devices that they do not
>    want for IO (chooses which device to use)

Isn't that what's the 'binding/unbinding' for?

> 2. Driver conflict between virtio PMD in DPDK, and virtio-pci in
>    kernel (happens on every virtio device that DPDK uses)

If you unbinded the kernel driver first, which is the suggested (or
must?) way to use DPDK, that will not happen.

	--yliu
> 
> For the white/black list solution, I guess it's good enough to solve
> (1) for customers. I am just curious about the 2nd.
> 
> Or say, even we black listed some virtio devices (or doing white
> list), the virtio devices used by DPDK are still in danger if we
> cannot make sure that virtio-pci will not touch the device any more
> (even it will not touch it, it feels like errornous to not telling
> virtio-pci to remove it before hand). E.g., if virtio-pci interrupt
> is still working, when there are packets from outside to guest,
> vp_interrupt() might be called? Then virtio-pci driver might do
> read/write to vring as well? If so, that's problematic. Am I wrong?
> 
> Peter

Re: [Question] How pmd virtio works without UIO?

[Peter Xu]

On Wed, Dec 23, 2015 at 10:09:49AM +0800, Yuanhan Liu wrote:
> Why can't we simply quit at pci_scan_one, once finding that it's not
> bond to uio (or similar stuff)? That would be generic enough, that we
> don't have to do similar checks for each new pmd driver.
> 
> Or, am I missing something?

It seems that ioport way to play with virtio devices do not require
any PCI wrapper layer like UIO/VFIO? Please check
virtio_resource_init().

> 
> 
> > I guess there might be two problems? Which are:
> > 
> > 1. How user avoid DPDK taking over virtio devices that they do not
> >    want for IO (chooses which device to use)
> 
> Isn't that what's the 'binding/unbinding' for?
> 
> > 2. Driver conflict between virtio PMD in DPDK, and virtio-pci in
> >    kernel (happens on every virtio device that DPDK uses)
> 
> If you unbinded the kernel driver first, which is the suggested (or
> must?) way to use DPDK, that will not happen.

Yes, maybe we should unbind it first. I am just not sure what will
happen if not.

Peter

Re: [Question] How pmd virtio works without UIO?

[Peter Xu]

On Wed, Dec 23, 2015 at 10:01:35AM +0800, Yuanhan Liu wrote:
> On Tue, Dec 22, 2015 at 05:56:41PM +0800, Peter Xu wrote:
> > On Tue, Dec 22, 2015 at 04:32:46PM +0800, Yuanhan Liu wrote:
> > > Actually, you are right. I mentioned in the last email that this is
> > > for configuration part. To answer your question in this email, you
> > > will not be able to go that further (say initiating virtio pmd) if
> > > you don't unbind the origin virtio-net driver, and bind it to igb_uio
> > > (or something similar).
> > > 
> > > The start point is from rte_eal_pci_scan, where the sub-function
> > > pci_san_one just initates a DPDK bond driver.
> > 
> > I am not sure whether I do understand your meaning correctly
> > (regarding "you willl not be able to go that furture"): The problem
> > is that, we _can_ run testpmd without unbinding the ports and bind
> > to UIO or something. What we need to do is boot the guest, reserve
> > huge pages, and run testpmd (keeping its kernel driver as
> > "virtio-pci"). In pci_scan_one():
> > 
> > 	if (!ret) {
> > 		if (!strcmp(driver, "vfio-pci"))
> > 			dev->kdrv = RTE_KDRV_VFIO;
> > 		else if (!strcmp(driver, "igb_uio"))
> > 			dev->kdrv = RTE_KDRV_IGB_UIO;
> > 		else if (!strcmp(driver, "uio_pci_generic"))
> > 			dev->kdrv = RTE_KDRV_UIO_GENERIC;
> > 		else
> > 			dev->kdrv = RTE_KDRV_UNKNOWN;
> > 	} else
> > 		dev->kdrv = RTE_KDRV_UNKNOWN;
> > 
> > I think it should be going to RTE_KDRV_UNKNOWN
> > (driver=="virtio-pci") here.
> 
> Sorry, I simply overlook that. I was thinking it will quit here for
> the RTE_KDRV_UNKNOWN case.
> 
> > I tried to run IO and it could work,
> > but I am not sure whether it is safe, and how.
> 
> I also did a quick test then, however, with the virtio 1.0 patchset
> I sent before, which sets the RTE_PCI_DRV_NEED_MAPPING, resulting to
> pci_map_device() failure and virtio pmd is not initiated at all.

Then, will the patch work with ioport way to access virtio devices?

> 
> > 
> > Also, I am not sure whether I need to (at least) unbind the
> > virtio-pci driver, so that there should have no kernel driver
> > running for the virtio device before DPDK using it.
> 
> Why not? That's what the DPDK document asked to do
> (http://dpdk.org/doc/guides/linux_gsg/build_dpdk.html):
> 
>     3.6. Binding and Unbinding Network Ports to/from the Kernel Modules
>     
>     As of release 1.4, DPDK applications no longer automatically unbind
>     all supported network ports from the kernel driver in use. Instead,
>     all ports that are to be used by an DPDK application must be bound
>     to the uio_pci_generic, igb_uio or vfio-pci module before the
>     application is run. Any network ports under Linux* control will be
>     ignored by the DPDK poll-mode drivers and cannot be used by the
>     application.

This seems obsolete? since it's not covering ioport.

Peter

> 
> 
> 	--yliu

Re: [Question] How pmd virtio works without UIO?

[Yuanhan Liu]

On Wed, Dec 23, 2015 at 10:41:57AM +0800, Peter Xu wrote:
> On Wed, Dec 23, 2015 at 10:01:35AM +0800, Yuanhan Liu wrote:
> > On Tue, Dec 22, 2015 at 05:56:41PM +0800, Peter Xu wrote:
> > > On Tue, Dec 22, 2015 at 04:32:46PM +0800, Yuanhan Liu wrote:
> > > > Actually, you are right. I mentioned in the last email that this is
> > > > for configuration part. To answer your question in this email, you
> > > > will not be able to go that further (say initiating virtio pmd) if
> > > > you don't unbind the origin virtio-net driver, and bind it to igb_uio
> > > > (or something similar).
> > > > 
> > > > The start point is from rte_eal_pci_scan, where the sub-function
> > > > pci_san_one just initates a DPDK bond driver.
> > > 
> > > I am not sure whether I do understand your meaning correctly
> > > (regarding "you willl not be able to go that furture"): The problem
> > > is that, we _can_ run testpmd without unbinding the ports and bind
> > > to UIO or something. What we need to do is boot the guest, reserve
> > > huge pages, and run testpmd (keeping its kernel driver as
> > > "virtio-pci"). In pci_scan_one():
> > > 
> > > 	if (!ret) {
> > > 		if (!strcmp(driver, "vfio-pci"))
> > > 			dev->kdrv = RTE_KDRV_VFIO;
> > > 		else if (!strcmp(driver, "igb_uio"))
> > > 			dev->kdrv = RTE_KDRV_IGB_UIO;
> > > 		else if (!strcmp(driver, "uio_pci_generic"))
> > > 			dev->kdrv = RTE_KDRV_UIO_GENERIC;
> > > 		else
> > > 			dev->kdrv = RTE_KDRV_UNKNOWN;
> > > 	} else
> > > 		dev->kdrv = RTE_KDRV_UNKNOWN;
> > > 
> > > I think it should be going to RTE_KDRV_UNKNOWN
> > > (driver=="virtio-pci") here.
> > 
> > Sorry, I simply overlook that. I was thinking it will quit here for
> > the RTE_KDRV_UNKNOWN case.
> > 
> > > I tried to run IO and it could work,
> > > but I am not sure whether it is safe, and how.
> > 
> > I also did a quick test then, however, with the virtio 1.0 patchset
> > I sent before, which sets the RTE_PCI_DRV_NEED_MAPPING, resulting to
> > pci_map_device() failure and virtio pmd is not initiated at all.
> 
> Then, will the patch work with ioport way to access virtio devices?

Yes.

> 
> > 
> > > 
> > > Also, I am not sure whether I need to (at least) unbind the
> > > virtio-pci driver, so that there should have no kernel driver
> > > running for the virtio device before DPDK using it.
> > 
> > Why not? That's what the DPDK document asked to do
> > (http://dpdk.org/doc/guides/linux_gsg/build_dpdk.html):
> > 
> >     3.6. Binding and Unbinding Network Ports to/from the Kernel Modules
> >     
> >     As of release 1.4, DPDK applications no longer automatically unbind
> >     all supported network ports from the kernel driver in use. Instead,
> >     all ports that are to be used by an DPDK application must be bound
> >     to the uio_pci_generic, igb_uio or vfio-pci module before the
> >     application is run. Any network ports under Linux* control will be
> >     ignored by the DPDK poll-mode drivers and cannot be used by the
> >     application.
> 
> This seems obsolete? since it's not covering ioport.

I don't think so. Above is for how to run DPDK applications. ioport
is just a (optional) way to access PCI resource in a specific PMD.

And, above speicification avoids your concerns, that two drivers try
to manipulate same device concurrently, doesn't it?

And, it is saying "any network ports under Linux* control will be
ignored by the DPDK poll-mode drivers and cannot be used by the
application", so that the case you were saying that virtio pmd
continues to work without the bind looks like a bug to me.

Can anyone confirm that?

	--yliu

Re: [Question] How pmd virtio works without UIO?

[Xie, Huawei]

On 12/23/2015 10:57 AM, Yuanhan Liu wrote:
> On Wed, Dec 23, 2015 at 10:41:57AM +0800, Peter Xu wrote:
>> On Wed, Dec 23, 2015 at 10:01:35AM +0800, Yuanhan Liu wrote:
>>> On Tue, Dec 22, 2015 at 05:56:41PM +0800, Peter Xu wrote:
>>>> On Tue, Dec 22, 2015 at 04:32:46PM +0800, Yuanhan Liu wrote:
>>>>> Actually, you are right. I mentioned in the last email that this is
>>>>> for configuration part. To answer your question in this email, you
>>>>> will not be able to go that further (say initiating virtio pmd) if
>>>>> you don't unbind the origin virtio-net driver, and bind it to igb_uio
>>>>> (or something similar).
>>>>>
>>>>> The start point is from rte_eal_pci_scan, where the sub-function
>>>>> pci_san_one just initates a DPDK bond driver.
>>>> I am not sure whether I do understand your meaning correctly
>>>> (regarding "you willl not be able to go that furture"): The problem
>>>> is that, we _can_ run testpmd without unbinding the ports and bind
>>>> to UIO or something. What we need to do is boot the guest, reserve
>>>> huge pages, and run testpmd (keeping its kernel driver as
>>>> "virtio-pci"). In pci_scan_one():
>>>>
>>>> 	if (!ret) {
>>>> 		if (!strcmp(driver, "vfio-pci"))
>>>> 			dev->kdrv = RTE_KDRV_VFIO;
>>>> 		else if (!strcmp(driver, "igb_uio"))
>>>> 			dev->kdrv = RTE_KDRV_IGB_UIO;
>>>> 		else if (!strcmp(driver, "uio_pci_generic"))
>>>> 			dev->kdrv = RTE_KDRV_UIO_GENERIC;
>>>> 		else
>>>> 			dev->kdrv = RTE_KDRV_UNKNOWN;
>>>> 	} else
>>>> 		dev->kdrv = RTE_KDRV_UNKNOWN;
>>>>
>>>> I think it should be going to RTE_KDRV_UNKNOWN
>>>> (driver=="virtio-pci") here.
>>> Sorry, I simply overlook that. I was thinking it will quit here for
>>> the RTE_KDRV_UNKNOWN case.
>>>
>>>> I tried to run IO and it could work,
>>>> but I am not sure whether it is safe, and how.
>>> I also did a quick test then, however, with the virtio 1.0 patchset
>>> I sent before, which sets the RTE_PCI_DRV_NEED_MAPPING, resulting to
>>> pci_map_device() failure and virtio pmd is not initiated at all.
>> Then, will the patch work with ioport way to access virtio devices?
> Yes.
>
>>>> Also, I am not sure whether I need to (at least) unbind the
>>>> virtio-pci driver, so that there should have no kernel driver
>>>> running for the virtio device before DPDK using it.
>>> Why not? That's what the DPDK document asked to do
>>> (http://dpdk.org/doc/guides/linux_gsg/build_dpdk.html):
>>>
>>>     3.6. Binding and Unbinding Network Ports to/from the Kernel Modules
>>>     
>>>     As of release 1.4, DPDK applications no longer automatically unbind
>>>     all supported network ports from the kernel driver in use. Instead,
>>>     all ports that are to be used by an DPDK application must be bound
>>>     to the uio_pci_generic, igb_uio or vfio-pci module before the
>>>     application is run. Any network ports under Linux* control will be
>>>     ignored by the DPDK poll-mode drivers and cannot be used by the
>>>     application.
>> This seems obsolete? since it's not covering ioport.
> I don't think so. Above is for how to run DPDK applications. ioport
> is just a (optional) way to access PCI resource in a specific PMD.
>
> And, above speicification avoids your concerns, that two drivers try
> to manipulate same device concurrently, doesn't it?
>
> And, it is saying "any network ports under Linux* control will be
> ignored by the DPDK poll-mode drivers and cannot be used by the
> application", so that the case you were saying that virtio pmd
> continues to work without the bind looks like a bug to me.
>
> Can anyone confirm that?

That document isn't accurate. virtio doesn't require binding to UIO
driver if it uses PORT IO. The PORT IO commit said it is because UIO
isn't secure, but avoid using uio doesn't bring more security as virtio
PMD still could ask device to DMA into any memory.
The thing we at least we might do is fail in virtio_resource_init if
kernel driver is still manipulating this device. This saves the effort
users use blacklist option and avoids the driver conflict.

>
> 	--yliu
>

Re: [Question] How pmd virtio works without UIO?

[Thomas Monjalon]

2015-12-23 05:13, Xie, Huawei:
> On 12/23/2015 10:57 AM, Yuanhan Liu wrote:
> > On Wed, Dec 23, 2015 at 10:41:57AM +0800, Peter Xu wrote:
> >> On Wed, Dec 23, 2015 at 10:01:35AM +0800, Yuanhan Liu wrote:
> >>> On Tue, Dec 22, 2015 at 05:56:41PM +0800, Peter Xu wrote:
> >>>> On Tue, Dec 22, 2015 at 04:32:46PM +0800, Yuanhan Liu wrote:
> >>>>> Actually, you are right. I mentioned in the last email that this is
> >>>>> for configuration part. To answer your question in this email, you
> >>>>> will not be able to go that further (say initiating virtio pmd) if
> >>>>> you don't unbind the origin virtio-net driver, and bind it to igb_uio
> >>>>> (or something similar).
> >>>>>
> >>>>> The start point is from rte_eal_pci_scan, where the sub-function
> >>>>> pci_san_one just initates a DPDK bond driver.
> >>>> I am not sure whether I do understand your meaning correctly
> >>>> (regarding "you willl not be able to go that furture"): The problem
> >>>> is that, we _can_ run testpmd without unbinding the ports and bind
> >>>> to UIO or something. What we need to do is boot the guest, reserve
> >>>> huge pages, and run testpmd (keeping its kernel driver as
> >>>> "virtio-pci"). In pci_scan_one():
> >>>>
> >>>> 	if (!ret) {
> >>>> 		if (!strcmp(driver, "vfio-pci"))
> >>>> 			dev->kdrv = RTE_KDRV_VFIO;
> >>>> 		else if (!strcmp(driver, "igb_uio"))
> >>>> 			dev->kdrv = RTE_KDRV_IGB_UIO;
> >>>> 		else if (!strcmp(driver, "uio_pci_generic"))
> >>>> 			dev->kdrv = RTE_KDRV_UIO_GENERIC;
> >>>> 		else
> >>>> 			dev->kdrv = RTE_KDRV_UNKNOWN;
> >>>> 	} else
> >>>> 		dev->kdrv = RTE_KDRV_UNKNOWN;
> >>>>
> >>>> I think it should be going to RTE_KDRV_UNKNOWN
> >>>> (driver=="virtio-pci") here.
> >>> Sorry, I simply overlook that. I was thinking it will quit here for
> >>> the RTE_KDRV_UNKNOWN case.
> >>>
> >>>> I tried to run IO and it could work,
> >>>> but I am not sure whether it is safe, and how.
> >>> I also did a quick test then, however, with the virtio 1.0 patchset
> >>> I sent before, which sets the RTE_PCI_DRV_NEED_MAPPING, resulting to
> >>> pci_map_device() failure and virtio pmd is not initiated at all.
> >> Then, will the patch work with ioport way to access virtio devices?
> > Yes.
> >
> >>>> Also, I am not sure whether I need to (at least) unbind the
> >>>> virtio-pci driver, so that there should have no kernel driver
> >>>> running for the virtio device before DPDK using it.
> >>> Why not? That's what the DPDK document asked to do
> >>> (http://dpdk.org/doc/guides/linux_gsg/build_dpdk.html):
> >>>
> >>>     3.6. Binding and Unbinding Network Ports to/from the Kernel Modules
> >>>     
> >>>     As of release 1.4, DPDK applications no longer automatically unbind
> >>>     all supported network ports from the kernel driver in use. Instead,
> >>>     all ports that are to be used by an DPDK application must be bound
> >>>     to the uio_pci_generic, igb_uio or vfio-pci module before the
> >>>     application is run. Any network ports under Linux* control will be
> >>>     ignored by the DPDK poll-mode drivers and cannot be used by the
> >>>     application.
> >> This seems obsolete? since it's not covering ioport.
> > I don't think so. Above is for how to run DPDK applications. ioport
> > is just a (optional) way to access PCI resource in a specific PMD.
> >
> > And, above speicification avoids your concerns, that two drivers try
> > to manipulate same device concurrently, doesn't it?
> >
> > And, it is saying "any network ports under Linux* control will be
> > ignored by the DPDK poll-mode drivers and cannot be used by the
> > application", so that the case you were saying that virtio pmd
> > continues to work without the bind looks like a bug to me.
> >
> > Can anyone confirm that?
> 
> That document isn't accurate. virtio doesn't require binding to UIO
> driver if it uses PORT IO. The PORT IO commit said it is because UIO
> isn't secure, but avoid using uio doesn't bring more security as virtio
> PMD still could ask device to DMA into any memory.
> The thing we at least we might do is fail in virtio_resource_init if
> kernel driver is still manipulating this device. This saves the effort
> users use blacklist option and avoids the driver conflict.

+1 for checking kernel driver in use

Re: [Question] How pmd virtio works without UIO?

[Thomas Monjalon]

2015-12-23 10:09, Yuanhan Liu:
> On Wed, Dec 23, 2015 at 09:55:54AM +0800, Peter Xu wrote:
> > On Tue, Dec 22, 2015 at 04:38:30PM +0000, Xie, Huawei wrote:
> > > On 12/22/2015 7:39 PM, Peter Xu wrote:
> > > > I tried to unbind one of the virtio net device, I see the PCI entry
> > > > still there.
> > > >
> > > > Before unbind:
> > > >
> > > > [root@vm proc]# lspci -k -s 00:03.0
> > > > 00:03.0 Ethernet controller: Red Hat, Inc Virtio network device
> > > >         Subsystem: Red Hat, Inc Device 0001
> > > >         Kernel driver in use: virtio-pci
> > > > [root@vm proc]# cat /proc/ioports | grep c060-c07f
> > > >   c060-c07f : 0000:00:03.0
> > > >     c060-c07f : virtio-pci
> > > >
> > > > After unbind:
> > > >
> > > > [root@vm proc]# lspci -k -s 00:03.0
> > > > 00:03.0 Ethernet controller: Red Hat, Inc Virtio network device
> > > >         Subsystem: Red Hat, Inc Device 0001
> > > > [root@vm proc]# cat /proc/ioports | grep c060-c07f
> > > >   c060-c07f : 0000:00:03.0
> > > >
> > > > So... does this means that it is an alternative to black list
> > > > solution?
> > > Oh, we could firstly check if this port is manipulated by kernel driver
> > > in virtio_resource_init/eth_virtio_dev_init, as long as it is not too late.
> 
> Why can't we simply quit at pci_scan_one, once finding that it's not
> bond to uio (or similar stuff)? That would be generic enough, that we
> don't have to do similar checks for each new pmd driver.
> 
> Or, am I missing something?

UIO is not needed to make virtio works (without interrupt support).
Sometimes it may be required to avoid using kernel modules.

> > I guess there might be two problems? Which are:
> > 
> > 1. How user avoid DPDK taking over virtio devices that they do not
> >    want for IO (chooses which device to use)
> 
> Isn't that what's the 'binding/unbinding' for?

Binding is, sometimes, required.
But does it mean DPDK should use every available ports?
That's the default and may be configured with blacklist/whitelist.

> > 2. Driver conflict between virtio PMD in DPDK, and virtio-pci in
> >    kernel (happens on every virtio device that DPDK uses)
> 
> If you unbinded the kernel driver first, which is the suggested (or
> must?) way to use DPDK, that will not happen.

Re: [Question] How pmd virtio works without UIO?

[Yuanhan Liu]

On Wed, Dec 23, 2015 at 11:26:17PM +0100, Thomas Monjalon wrote:
> 2015-12-23 10:09, Yuanhan Liu:
> > On Wed, Dec 23, 2015 at 09:55:54AM +0800, Peter Xu wrote:
> > > On Tue, Dec 22, 2015 at 04:38:30PM +0000, Xie, Huawei wrote:
> > > > On 12/22/2015 7:39 PM, Peter Xu wrote:
> > > > > I tried to unbind one of the virtio net device, I see the PCI entry
> > > > > still there.
> > > > >
> > > > > Before unbind:
> > > > >
> > > > > [root@vm proc]# lspci -k -s 00:03.0
> > > > > 00:03.0 Ethernet controller: Red Hat, Inc Virtio network device
> > > > >         Subsystem: Red Hat, Inc Device 0001
> > > > >         Kernel driver in use: virtio-pci
> > > > > [root@vm proc]# cat /proc/ioports | grep c060-c07f
> > > > >   c060-c07f : 0000:00:03.0
> > > > >     c060-c07f : virtio-pci
> > > > >
> > > > > After unbind:
> > > > >
> > > > > [root@vm proc]# lspci -k -s 00:03.0
> > > > > 00:03.0 Ethernet controller: Red Hat, Inc Virtio network device
> > > > >         Subsystem: Red Hat, Inc Device 0001
> > > > > [root@vm proc]# cat /proc/ioports | grep c060-c07f
> > > > >   c060-c07f : 0000:00:03.0
> > > > >
> > > > > So... does this means that it is an alternative to black list
> > > > > solution?
> > > > Oh, we could firstly check if this port is manipulated by kernel driver
> > > > in virtio_resource_init/eth_virtio_dev_init, as long as it is not too late.
> > 
> > Why can't we simply quit at pci_scan_one, once finding that it's not
> > bond to uio (or similar stuff)? That would be generic enough, that we
> > don't have to do similar checks for each new pmd driver.
> > 
> > Or, am I missing something?
> 
> UIO is not needed to make virtio works (without interrupt support).
> Sometimes it may be required to avoid using kernel modules.

While dig the git history, I found the commit:

    commit da978dfdc43b59e290a46d7ece5fd19ce79a1162
    Author: Ouyang Changchun <changchun.ouyang@intel.com>
    Date:   Mon Feb 9 09:14:06 2015 +0800
    
        virtio: use port IO to get PCI resource
    
        Make virtio not require UIO for some security reasons, this is to match
        6WIND's virtio-net-pmd.
    
        Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
        Acked-by: Huawei Xie <huawei.xie@intel.com>

The commit log is not well written, giving no explanation about the
"some security reasons".

Anyway, I see it now that it's kind of a design.


Note that my first patch set about enabling virtio 1.0 [0] sets the
RTE_PCI_DRV_NEED_MAPPING flag, which somehow implies that uio is a
must, otherwise, eal init would fail at pci_map_device().

So that my pathset breaks the un-documented rule, and I need fix it.
How about adding a wrapper, say rte_pci_map_device(), and exporting
it, so that virtio pmd driver could map resources when necessary?

[0]: http://dpdk.org/dev/patchwork/bundle/yliu/virtio-1.0-pmd/


> > > I guess there might be two problems? Which are:
> > > 
> > > 1. How user avoid DPDK taking over virtio devices that they do not
> > >    want for IO (chooses which device to use)
> > 
> > Isn't that what's the 'binding/unbinding' for?
> 
> Binding is, sometimes, required.

We may need fix the doc then. As the doc says it's a must:

    3.6. Binding and Unbinding Network Ports to/from the Kernel Modules

    Instead, all ports that are to be used by an DPDK application
==> must be bound to the uio_pci_generic, igb_uio or vfio-pci
    module before the application is run. Any network ports under
    Linux* control will be ignored by the DPDK poll-mode drivers
    and cannot be used by the application.


	--yliu

> But does it mean DPDK should use every available ports?
> That's the default and may be configured with blacklist/whitelist.
> 
> > > 2. Driver conflict between virtio PMD in DPDK, and virtio-pci in
> > >    kernel (happens on every virtio device that DPDK uses)
> > 
> > If you unbinded the kernel driver first, which is the suggested (or
> > must?) way to use DPDK, that will not happen.

Re: [Question] How pmd virtio works without UIO?

[Stephen Hemminger]

On Thu, 24 Dec 2015 11:30:27 +0800
Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote:

> On Wed, Dec 23, 2015 at 11:26:17PM +0100, Thomas Monjalon wrote:
> > 2015-12-23 10:09, Yuanhan Liu:
> > > On Wed, Dec 23, 2015 at 09:55:54AM +0800, Peter Xu wrote:
> > > > On Tue, Dec 22, 2015 at 04:38:30PM +0000, Xie, Huawei wrote:
> > > > > On 12/22/2015 7:39 PM, Peter Xu wrote:
> > > > > > I tried to unbind one of the virtio net device, I see the PCI entry
> > > > > > still there.
> > > > > >
> > > > > > Before unbind:
> > > > > >
> > > > > > [root@vm proc]# lspci -k -s 00:03.0
> > > > > > 00:03.0 Ethernet controller: Red Hat, Inc Virtio network device
> > > > > >         Subsystem: Red Hat, Inc Device 0001
> > > > > >         Kernel driver in use: virtio-pci
> > > > > > [root@vm proc]# cat /proc/ioports | grep c060-c07f
> > > > > >   c060-c07f : 0000:00:03.0
> > > > > >     c060-c07f : virtio-pci
> > > > > >
> > > > > > After unbind:
> > > > > >
> > > > > > [root@vm proc]# lspci -k -s 00:03.0
> > > > > > 00:03.0 Ethernet controller: Red Hat, Inc Virtio network device
> > > > > >         Subsystem: Red Hat, Inc Device 0001
> > > > > > [root@vm proc]# cat /proc/ioports | grep c060-c07f
> > > > > >   c060-c07f : 0000:00:03.0
> > > > > >
> > > > > > So... does this means that it is an alternative to black list
> > > > > > solution?
> > > > > Oh, we could firstly check if this port is manipulated by kernel driver
> > > > > in virtio_resource_init/eth_virtio_dev_init, as long as it is not too late.
> > > 
> > > Why can't we simply quit at pci_scan_one, once finding that it's not
> > > bond to uio (or similar stuff)? That would be generic enough, that we
> > > don't have to do similar checks for each new pmd driver.
> > > 
> > > Or, am I missing something?
> > 
> > UIO is not needed to make virtio works (without interrupt support).
> > Sometimes it may be required to avoid using kernel modules.
> 
> While dig the git history, I found the commit:
> 
>     commit da978dfdc43b59e290a46d7ece5fd19ce79a1162
>     Author: Ouyang Changchun <changchun.ouyang@intel.com>
>     Date:   Mon Feb 9 09:14:06 2015 +0800
>     
>         virtio: use port IO to get PCI resource
>     
>         Make virtio not require UIO for some security reasons, this is to match
>         6WIND's virtio-net-pmd.
>     
>         Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
>         Acked-by: Huawei Xie <huawei.xie@intel.com>
> 
> The commit log is not well written, giving no explanation about the
> "some security reasons".
> 
> Anyway, I see it now that it's kind of a design.
> 
> 
> Note that my first patch set about enabling virtio 1.0 [0] sets the
> RTE_PCI_DRV_NEED_MAPPING flag, which somehow implies that uio is a
> must, otherwise, eal init would fail at pci_map_device().
> 
> So that my pathset breaks the un-documented rule, and I need fix it.
> How about adding a wrapper, say rte_pci_map_device(), and exporting
> it, so that virtio pmd driver could map resources when necessary?
> 
> [0]: http://dpdk.org/dev/patchwork/bundle/yliu/virtio-1.0-pmd/
> 
> 
> > > > I guess there might be two problems? Which are:
> > > > 
> > > > 1. How user avoid DPDK taking over virtio devices that they do not
> > > >    want for IO (chooses which device to use)
> > > 
> > > Isn't that what's the 'binding/unbinding' for?
> > 
> > Binding is, sometimes, required.
> 
> We may need fix the doc then. As the doc says it's a must:
> 
>     3.6. Binding and Unbinding Network Ports to/from the Kernel Modules
> 
>     Instead, all ports that are to be used by an DPDK application
> ==> must be bound to the uio_pci_generic, igb_uio or vfio-pci
>     module before the application is run. Any network ports under
>     Linux* control will be ignored by the DPDK poll-mode drivers
>     and cannot be used by the application.
> 
> 
> 	--yliu
> 
> > But does it mean DPDK should use every available ports?
> > That's the default and may be configured with blacklist/whitelist.
> > 
> > > > 2. Driver conflict between virtio PMD in DPDK, and virtio-pci in
> > > >    kernel (happens on every virtio device that DPDK uses)
> > > 
> > > If you unbinded the kernel driver first, which is the suggested (or
> > > must?) way to use DPDK, that will not happen.


As far as I remember; there are some environments where DPDK but guests are not
allowed to load their own kernel modules. But since virtio only needs access to
I/O ports on x86, the driver can accomodate. I haven't run into these environments.

But the added code in virtio driver causes issues. Mostly because it causes
an unnecessary duplication of the initialization code, and is missing many of
the protections and interfaces that exist in the base code. For example,
there are lots of corner cases in interrupt support which are related to
this non-UIO mode of operation.

[PATCH 0/4] check if any kernel driver is manipulating the virtio device

[Huawei Xie]

virtio PMD doesn't set RTE_PCI_DRV_NEED_MAPPING in drv_flags of its
eth_driver. It will try igb_uio and PORT IO in turn to configure
virtio device. Even user in guest VM doesn't want to use virtio for
DPDK, virtio PMD will take over the device blindly.

The more serious problem is kernel driver is still manipulating the
device, which causes driver conflict.

This patch checks if there is any kernel driver manipulating the
virtio device before virtio PMD uses port IO to configure the device.

Huawei Xie (4):
  eal: make the comment more accurate
  eal: set kdrv to RTE_KDRV_NONE if kernel driver isn't manipulating the device.
  virtio: return 1 to tell the kernel we don't take over this device
  virtio: check if any kernel driver is manipulating the virtio device

 drivers/net/virtio/virtio_ethdev.c     | 15 +++++++++++++--
 lib/librte_eal/common/eal_common_pci.c |  8 ++++----
 lib/librte_eal/linuxapp/eal/eal_pci.c  |  2 +-
 3 files changed, 18 insertions(+), 7 deletions(-)

-- 
1.8.1.4

[PATCH 1/4] eal: make the comment more accurate

[Huawei Xie]

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
---
 lib/librte_eal/common/eal_common_pci.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
index dcfe947..bbcdb2b 100644
--- a/lib/librte_eal/common/eal_common_pci.c
+++ b/lib/librte_eal/common/eal_common_pci.c
@@ -204,7 +204,7 @@ rte_eal_pci_probe_one_driver(struct rte_pci_driver *dr, struct rte_pci_device *d
 		/* call the driver devinit() function */
 		return dr->devinit(dr, dev);
 	}
-	/* return positive value if driver is not found */
+	/* return positive value if driver doesn't support this device */
 	return 1;
 }
 
@@ -259,7 +259,7 @@ rte_eal_pci_detach_dev(struct rte_pci_driver *dr,
 		return 0;
 	}
 
-	/* return positive value if driver is not found */
+	/* return positive value if driver doesn't support this device */
 	return 1;
 }
 
@@ -283,7 +283,7 @@ pci_probe_all_drivers(struct rte_pci_device *dev)
 			/* negative value is an error */
 			return -1;
 		if (rc > 0)
-			/* positive value means driver not found */
+			/* positive value means driver doesn't support it */
 			continue;
 		return 0;
 	}
@@ -310,7 +310,7 @@ pci_detach_all_drivers(struct rte_pci_device *dev)
 			/* negative value is an error */
 			return -1;
 		if (rc > 0)
-			/* positive value means driver not found */
+			/* positive value means driver doesn't support it */
 			continue;
 		return 0;
 	}
-- 
1.8.1.4

[PATCH 2/4] eal: set kdrv to RTE_KDRV_NONE if kernel driver isn't manipulating the device.

[Huawei Xie]

Use RTE_KDRV_NONE to indicate that kernel driver isn't manipulating the
device.

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index bc5b5be..640b190 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -362,7 +362,7 @@ pci_scan_one(const char *dirname, uint16_t domain, uint8_t bus,
 		else
 			dev->kdrv = RTE_KDRV_UNKNOWN;
 	} else
-		dev->kdrv = RTE_KDRV_UNKNOWN;
+		dev->kdrv = RTE_KDRV_NONE;
 
 	/* device is valid, add in list (sorted) */
 	if (TAILQ_EMPTY(&pci_device_list)) {
-- 
1.8.1.4

[PATCH 3/4] virtio: return 1 to tell the upper layer we don't take over this device

[Huawei Xie]

if virtio_resource_init fails, cleanup the resource and return 1 to
tell the upper layer we don't take over this device.
return -1 means error and DPDK will exit.

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
---
 drivers/net/virtio/virtio_ethdev.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index d928339..00015ef 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1287,8 +1287,12 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
 
 	pci_dev = eth_dev->pci_dev;
 
-	if (virtio_resource_init(pci_dev) < 0)
-		return -1;
+	/* Return 1 to tell the upper layer we don't take over this device. */
+	if (virtio_resource_init(pci_dev) < 0) {
+		rte_free(eth_dev->data->mac_addrs);
+		eth_dev->data->mac_addrs = NULL;
+		return 1;
+	}
 
 	hw->use_msix = virtio_has_msix(&pci_dev->addr);
 	hw->io_base = (uint32_t)(uintptr_t)pci_dev->mem_resource[0].addr;
-- 
1.8.1.4

[PATCH 4/4] virtio: check if any kernel driver is manipulating the device

[Huawei Xie]

virtio PMD could use IO port to configure the virtio device without
using uio driver.

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

This patch checks if there is any kernel driver manipulating the virtio
device before virtio PMD uses IO port to configure the device.

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 | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 00015ef..504346a 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1138,6 +1138,13 @@ static int virtio_resource_init_by_ioports(struct rte_pci_device *pci_dev)
 	int found = 0;
 	size_t linesz;
 
+	if (pci_dev->kdrv != RTE_KDRV_NONE) {
+		PMD_INIT_LOG(ERR,
+			"%s(): kernel driver is manipulating this device." \
+			" Please unbind the kernel driver.", __func__);
+		return -1;
+	}
+
 	snprintf(pci_id, sizeof(pci_id), PCI_PRI_FMT,
 		 pci_dev->addr.domain,
 		 pci_dev->addr.bus,
-- 
1.8.1.4

Re: [PATCH 0/4] check if any kernel driver is manipulating the virtio device

[Peter Xu]

On Fri, Dec 25, 2015 at 02:38:08AM +0800, Huawei Xie wrote:
> virtio PMD doesn't set RTE_PCI_DRV_NEED_MAPPING in drv_flags of its
> eth_driver. It will try igb_uio and PORT IO in turn to configure
> virtio device. Even user in guest VM doesn't want to use virtio for
> DPDK, virtio PMD will take over the device blindly.
> 
> The more serious problem is kernel driver is still manipulating the
> device, which causes driver conflict.
> 
> This patch checks if there is any kernel driver manipulating the
> virtio device before virtio PMD uses port IO to configure the device.
> 
> Huawei Xie (4):
>   eal: make the comment more accurate
>   eal: set kdrv to RTE_KDRV_NONE if kernel driver isn't manipulating the device.
>   virtio: return 1 to tell the kernel we don't take over this device
>   virtio: check if any kernel driver is manipulating the virtio device

Thanks for the patch. Looks good to me.

Peter

> 
>  drivers/net/virtio/virtio_ethdev.c     | 15 +++++++++++++--
>  lib/librte_eal/common/eal_common_pci.c |  8 ++++----
>  lib/librte_eal/linuxapp/eal/eal_pci.c  |  2 +-
>  3 files changed, 18 insertions(+), 7 deletions(-)
> 
> -- 
> 1.8.1.4
> 

Re: [PATCH 3/4] virtio: return 1 to tell the upper layer we don't take over this device

[Yuanhan Liu]

On Fri, Dec 25, 2015 at 02:38:11AM +0800, Huawei Xie wrote:
> if virtio_resource_init fails, cleanup the resource and return 1 to
> tell the upper layer we don't take over this device.
> return -1 means error and DPDK will exit.
> 
> Signed-off-by: Huawei Xie <huawei.xie@intel.com>
> ---
>  drivers/net/virtio/virtio_ethdev.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index d928339..00015ef 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1287,8 +1287,12 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
>  
>  	pci_dev = eth_dev->pci_dev;
>  
> -	if (virtio_resource_init(pci_dev) < 0)
> -		return -1;
> +	/* Return 1 to tell the upper layer we don't take over this device. */
> +	if (virtio_resource_init(pci_dev) < 0) {
> +		rte_free(eth_dev->data->mac_addrs);
> +		eth_dev->data->mac_addrs = NULL;

This assignment looks unnecessary to me.


And, I think above comment is better to put here, right above the return
statement.

> +		return 1;
> +	}

	--yliu

Re: [PATCH 4/4] virtio: check if any kernel driver is manipulating the device

[Yuanhan Liu]

On Fri, Dec 25, 2015 at 02:38:12AM +0800, Huawei Xie wrote:
> virtio PMD could use IO port to configure the virtio device without
> using uio driver.
> 
> There are two issues with previous implementation:
> 1) virtio PMD will take over each virtio device blindly even if some
> are not intended for DPDK.
> 2) driver conflict between virtio PMD and virtio-net kernel driver.
> 
> This patch checks if there is any kernel driver manipulating the virtio
> device before virtio PMD uses IO port to configure the device.
> 
> 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 | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index 00015ef..504346a 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1138,6 +1138,13 @@ static int virtio_resource_init_by_ioports(struct rte_pci_device *pci_dev)
>  	int found = 0;
>  	size_t linesz;
>  
> +	if (pci_dev->kdrv != RTE_KDRV_NONE) {
> +		PMD_INIT_LOG(ERR,
> +			"%s(): kernel driver is manipulating this device." \
> +			" Please unbind the kernel driver.", __func__);

PMD_INIT_LOG already prints the function, no need to reference __func__
again here.

	--yliu
> +		return -1;
> +	}
> +
>  	snprintf(pci_id, sizeof(pci_id), PCI_PRI_FMT,
>  		 pci_dev->addr.domain,
>  		 pci_dev->addr.bus,
> -- 
> 1.8.1.4

Re: [PATCH 4/4] virtio: check if any kernel driver is manipulating the device

[Xie, Huawei]

> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Monday, December 28, 2015 1:27 PM
> To: Xie, Huawei
> Cc: dev@dpdk.org; Jayakumar, Muthurajan; Troitsky, Nikita;
> peterx@redhat.com; stephen@networkplumber.org;
> Changchun.ouyang@hotmail.com; thomas.monjalon@6wind.com
> Subject: Re: [PATCH 4/4] virtio: check if any kernel driver is
> manipulating the device
> 
> On Fri, Dec 25, 2015 at 02:38:12AM +0800, Huawei Xie wrote:
> > virtio PMD could use IO port to configure the virtio device without
> > using uio driver.
> >
> > There are two issues with previous implementation:
> > 1) virtio PMD will take over each virtio device blindly even if some
> > are not intended for DPDK.
> > 2) driver conflict between virtio PMD and virtio-net kernel driver.
> >
> > This patch checks if there is any kernel driver manipulating the
> virtio
> > device before virtio PMD uses IO port to configure the device.
> >
> > 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 | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/net/virtio/virtio_ethdev.c
> b/drivers/net/virtio/virtio_ethdev.c
> > index 00015ef..504346a 100644
> > --- a/drivers/net/virtio/virtio_ethdev.c
> > +++ b/drivers/net/virtio/virtio_ethdev.c
> > @@ -1138,6 +1138,13 @@ static int
> virtio_resource_init_by_ioports(struct rte_pci_device *pci_dev)
> >  	int found = 0;
> >  	size_t linesz;
> >
> > +	if (pci_dev->kdrv != RTE_KDRV_NONE) {
> > +		PMD_INIT_LOG(ERR,
> > +			"%s(): kernel driver is manipulating this device." \
> > +			" Please unbind the kernel driver.", __func__);
> 
> PMD_INIT_LOG already prints the function, no need to reference
> __func__
> again here.

Good. We have to fix the similar issue in virtio_resource_init_xx as well.

> 
> 	--yliu
> > +		return -1;
> > +	}
> > +
> >  	snprintf(pci_id, sizeof(pci_id), PCI_PRI_FMT,
> >  		 pci_dev->addr.domain,
> >  		 pci_dev->addr.bus,
> > --
> > 1.8.1.4

Re: [PATCH 3/4] virtio: return 1 to tell the upper layer we don't take over this device

[Xie, Huawei]

> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Monday, December 28, 2015 1:25 PM
> To: Xie, Huawei
> Cc: dev@dpdk.org; Jayakumar, Muthurajan; Troitsky, Nikita;
> peterx@redhat.com; stephen@networkplumber.org;
> Changchun.ouyang@hotmail.com; thomas.monjalon@6wind.com
> Subject: Re: [PATCH 3/4] virtio: return 1 to tell the upper layer we
> don't take over this device
> 
> On Fri, Dec 25, 2015 at 02:38:11AM +0800, Huawei Xie wrote:
> > if virtio_resource_init fails, cleanup the resource and return 1 to
> > tell the upper layer we don't take over this device.
> > return -1 means error and DPDK will exit.
> >
> > Signed-off-by: Huawei Xie <huawei.xie@intel.com>
> > ---
> >  drivers/net/virtio/virtio_ethdev.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/virtio/virtio_ethdev.c
> b/drivers/net/virtio/virtio_ethdev.c
> > index d928339..00015ef 100644
> > --- a/drivers/net/virtio/virtio_ethdev.c
> > +++ b/drivers/net/virtio/virtio_ethdev.c
> > @@ -1287,8 +1287,12 @@ eth_virtio_dev_init(struct rte_eth_dev
> *eth_dev)
> >
> >  	pci_dev = eth_dev->pci_dev;
> >
> > -	if (virtio_resource_init(pci_dev) < 0)
> > -		return -1;
> > +	/* Return 1 to tell the upper layer we don't take over this
> device. */
> > +	if (virtio_resource_init(pci_dev) < 0) {
> > +		rte_free(eth_dev->data->mac_addrs);
> > +		eth_dev->data->mac_addrs = NULL;
> 
> This assignment looks unnecessary to me.
> 

Noted this when write this code. It is ok not to reset as this layer is responsible for all the allocation/free.


> 
> And, I think above comment is better to put here, right above the
> return
> statement.
> 
> > +		return 1;
> > +	}
> 
> 	--yliu

Re: [PATCH 2/4] eal: set kdrv to RTE_KDRV_NONE if kernel driver isn't manipulating the device.

[David Marchand]

On Thu, Dec 24, 2015 at 7:38 PM, Huawei Xie <huawei.xie@intel.com> wrote:

> Use RTE_KDRV_NONE to indicate that kernel driver isn't manipulating the
> device.
>
> Signed-off-by: Huawei Xie <huawei.xie@intel.com>
> ---
>  lib/librte_eal/linuxapp/eal/eal_pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c
> b/lib/librte_eal/linuxapp/eal/eal_pci.c
> index bc5b5be..640b190 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_pci.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
> @@ -362,7 +362,7 @@ pci_scan_one(const char *dirname, uint16_t domain,
> uint8_t bus,
>                 else
>                         dev->kdrv = RTE_KDRV_UNKNOWN;
>         } else
> -               dev->kdrv = RTE_KDRV_UNKNOWN;
> +               dev->kdrv = RTE_KDRV_NONE;
>
>         /* device is valid, add in list (sorted) */
>         if (TAILQ_EMPTY(&pci_device_list)) {
> --
> 1.8.1.4
>
>
lgtm
Acked-by: David Marchand <david.marchand@6wind.com>


-- 
David Marchand

[PATCH v2 0/4] fix the issue that DPDK takes over virtio device blindly

[Huawei Xie]

v2 changes:
 Remove unnecessary assignment of NULL to dev->data->mac_addrs
 Ajust one comment's position
 change LOG level from ERR to INFO

virtio PMD doesn't set RTE_PCI_DRV_NEED_MAPPING in drv_flags of its
eth_driver. It will try igb_uio and PORT IO in turn to configure
virtio device. Even user in guest VM doesn't want to use virtio for
DPDK, virtio PMD will take over the device blindly.

The more serious problem is kernel driver is still manipulating the
device, which causes driver conflict.

This patch checks if there is any kernel driver manipulating the
virtio device before virtio PMD uses port IO to configure the device.

Huawei Xie (4):
  eal: make the comment more accurate
  eal: set kdrv to RTE_KDRV_NONE if kernel driver isn't manipulating the device.
  virtio: return 1 to tell the kernel we don't take over this device
  virtio: check if any kernel driver is manipulating the virtio device

 drivers/net/virtio/virtio_ethdev.c     | 16 ++++++++++++++--
 lib/librte_eal/common/eal_common_pci.c |  8 ++++----
 lib/librte_eal/linuxapp/eal/eal_pci.c  |  2 +-
 3 files changed, 19 insertions(+), 7 deletions(-)

-- 
1.8.1.4

[PATCH v2 1/4] eal: make the comment more accurate

[Huawei Xie]

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
---
 lib/librte_eal/common/eal_common_pci.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
index dcfe947..bbcdb2b 100644
--- a/lib/librte_eal/common/eal_common_pci.c
+++ b/lib/librte_eal/common/eal_common_pci.c
@@ -204,7 +204,7 @@ rte_eal_pci_probe_one_driver(struct rte_pci_driver *dr, struct rte_pci_device *d
 		/* call the driver devinit() function */
 		return dr->devinit(dr, dev);
 	}
-	/* return positive value if driver is not found */
+	/* return positive value if driver doesn't support this device */
 	return 1;
 }
 
@@ -259,7 +259,7 @@ rte_eal_pci_detach_dev(struct rte_pci_driver *dr,
 		return 0;
 	}
 
-	/* return positive value if driver is not found */
+	/* return positive value if driver doesn't support this device */
 	return 1;
 }
 
@@ -283,7 +283,7 @@ pci_probe_all_drivers(struct rte_pci_device *dev)
 			/* negative value is an error */
 			return -1;
 		if (rc > 0)
-			/* positive value means driver not found */
+			/* positive value means driver doesn't support it */
 			continue;
 		return 0;
 	}
@@ -310,7 +310,7 @@ pci_detach_all_drivers(struct rte_pci_device *dev)
 			/* negative value is an error */
 			return -1;
 		if (rc > 0)
-			/* positive value means driver not found */
+			/* positive value means driver doesn't support it */
 			continue;
 		return 0;
 	}
-- 
1.8.1.4

[PATCH v2 2/4] eal: set kdrv to RTE_KDRV_NONE if kernel driver isn't manipulating the device.

[Huawei Xie]

Use RTE_KDRV_NONE to indicate that kernel driver isn't manipulating
the device.

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
Acked-by: David Marchand <david.marchand@6wind.com>
---
 lib/librte_eal/linuxapp/eal/eal_pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index bc5b5be..640b190 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -362,7 +362,7 @@ pci_scan_one(const char *dirname, uint16_t domain, uint8_t bus,
 		else
 			dev->kdrv = RTE_KDRV_UNKNOWN;
 	} else
-		dev->kdrv = RTE_KDRV_UNKNOWN;
+		dev->kdrv = RTE_KDRV_NONE;
 
 	/* device is valid, add in list (sorted) */
 	if (TAILQ_EMPTY(&pci_device_list)) {
-- 
1.8.1.4

[PATCH v2 3/4] virtio: return 1 to tell the upper layer we don't take over this device

[Huawei Xie]

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

if virtio_resource_init fails, cleanup the resource and return 1 to
tell the upper layer we don't take over this device.
return -1 means error and DPDK will exit.

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
---
 drivers/net/virtio/virtio_ethdev.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index d928339..e815acd 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1287,8 +1287,13 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
 
 	pci_dev = eth_dev->pci_dev;
 
-	if (virtio_resource_init(pci_dev) < 0)
-		return -1;
+	if (virtio_resource_init(pci_dev) < 0) {
+		rte_free(eth_dev->data->mac_addrs);
+		/* Return 1 to tell the upper layer we don't take over
+		 * this device.
+		 */
+		return 1;
+	}
 
 	hw->use_msix = virtio_has_msix(&pci_dev->addr);
 	hw->io_base = (uint32_t)(uintptr_t)pci_dev->mem_resource[0].addr;
-- 
1.8.1.4

[PATCH v2 4/4] virtio: check if any kernel driver is manipulating the virtio device

[Huawei Xie]

v2 changes:
 change LOG level from ERR to INFO

virtio PMD could use IO port to configure the virtio device without
using uio driver.

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

This patch checks if there is any kernel driver manipulating the virtio
device before virtio PMD uses IO port to configure the device.

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 | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index e815acd..7a50dac 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1138,6 +1138,13 @@ static int virtio_resource_init_by_ioports(struct rte_pci_device *pci_dev)
 	int found = 0;
 	size_t linesz;
 
+	if (pci_dev->kdrv != RTE_KDRV_NONE) {
+		PMD_INIT_LOG(INFO,
+			"kernel driver is manipulating this device." \
+			" Please unbind the kernel driver.");
+		return -1;
+	}
+
 	snprintf(pci_id, sizeof(pci_id), PCI_PRI_FMT,
 		 pci_dev->addr.domain,
 		 pci_dev->addr.bus,
-- 
1.8.1.4

Re: [PATCH 4/4] virtio: check if any kernel driver is manipulating the device

[Xie, Huawei]

On 12/25/2015 6:33 PM, Xie, Huawei wrote:
> virtio PMD could use IO port to configure the virtio device without
> using uio driver.
>
> There are two issues with previous implementation:
> 1) virtio PMD will take over each virtio device blindly even if some
> are not intended for DPDK.
> 2) driver conflict between virtio PMD and virtio-net kernel driver.
>
> This patch checks if there is any kernel driver manipulating the virtio
> device before virtio PMD uses IO port to configure the device.
>
> 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 | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index 00015ef..504346a 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1138,6 +1138,13 @@ static int virtio_resource_init_by_ioports(struct rte_pci_device *pci_dev)
>  	int found = 0;
>  	size_t linesz;
>  
> +	if (pci_dev->kdrv != RTE_KDRV_NONE) {
> +		PMD_INIT_LOG(ERR,
Better change ERR to INFO and revise the message followed, since user
might not want to use this device for DPDK.
> +			"%s(): kernel driver is manipulating this device." \
> +			" Please unbind the kernel driver.", __func__);
> +		return -1;
> +	}
> +
>  	snprintf(pci_id, sizeof(pci_id), PCI_PRI_FMT,
>  		 pci_dev->addr.domain,
>  		 pci_dev->addr.bus,

Re: [PATCH v2 4/4] virtio: check if any kernel driver is manipulating the virtio device

[Stephen Hemminger]

On Mon,  4 Jan 2016 01:56:13 +0800
Huawei Xie <huawei.xie@intel.com> wrote:

> +	if (pci_dev->kdrv != RTE_KDRV_NONE) {
> +		PMD_INIT_LOG(INFO,
> +			"kernel driver is manipulating this device." \
> +			" Please unbind the kernel driver.");

Splitting strings in general is a bad idea since it makes it harder to find log messages.
Also the first clause is lower case and the second is captialized.

Lastly, the backslash continuation is unnecessary here and will cause checkpatch warning.

Re: [PATCH v2 0/4] fix the issue that DPDK takes over virtio device blindly

[Stephen Hemminger]

On Mon,  4 Jan 2016 01:56:09 +0800
Huawei Xie <huawei.xie@intel.com> wrote:

> v2 changes:
>  Remove unnecessary assignment of NULL to dev->data->mac_addrs
>  Ajust one comment's position
>  change LOG level from ERR to INFO
> 
> virtio PMD doesn't set RTE_PCI_DRV_NEED_MAPPING in drv_flags of its
> eth_driver. It will try igb_uio and PORT IO in turn to configure
> virtio device. Even user in guest VM doesn't want to use virtio for
> DPDK, virtio PMD will take over the device blindly.
> 
> The more serious problem is kernel driver is still manipulating the
> device, which causes driver conflict.
> 
> This patch checks if there is any kernel driver manipulating the
> virtio device before virtio PMD uses port IO to configure the device.
> 
> Huawei Xie (4):
>   eal: make the comment more accurate
>   eal: set kdrv to RTE_KDRV_NONE if kernel driver isn't manipulating the device.
>   virtio: return 1 to tell the kernel we don't take over this device
>   virtio: check if any kernel driver is manipulating the virtio device
> 
>  drivers/net/virtio/virtio_ethdev.c     | 16 ++++++++++++++--
>  lib/librte_eal/common/eal_common_pci.c |  8 ++++----
>  lib/librte_eal/linuxapp/eal/eal_pci.c  |  2 +-
>  3 files changed, 19 insertions(+), 7 deletions(-)
> 

Overall looks good, thanks for addressing this.

It would be good to note that VFIO no-IOMMU mode should work for this
as well.

Re: [PATCH 4/4] virtio: check if any kernel driver is manipulating the device

[Stephen Hemminger]

On Mon, 4 Jan 2016 09:02:53 +0000
"Xie, Huawei" <huawei.xie@intel.com> wrote:

> > +		PMD_INIT_LOG(ERR,  
> Better change ERR to INFO and revise the message followed, since user
> might not want to use this device for DPDK.
> > +			"%s(): kernel driver is manipulating this device." \
> > +			" Please unbind the kernel driver.", __func__);

The addition of __func__ here is redundant since this is already in PMD_INIT_LOG macro.

Re: [PATCH v2 4/4] virtio: check if any kernel driver is manipulating the virtio device

[Xie, Huawei]

On 1/5/2016 1:24 AM, Stephen Hemminger wrote:
> On Mon,  4 Jan 2016 01:56:13 +0800
> Huawei Xie <huawei.xie@intel.com> wrote:
>
>> +	if (pci_dev->kdrv != RTE_KDRV_NONE) {
>> +		PMD_INIT_LOG(INFO,
>> +			"kernel driver is manipulating this device." \
>> +			" Please unbind the kernel driver.");
> Splitting strings in general is a bad idea since it makes it harder to find log messages.
> Also the first clause is lower case and the second is captialized.
Got it. This is to avoid 80 char warning. Will put it in one line to
make it friendly for searching.
The first clause is lower is because it actually follows "%s():".
>
> Lastly, the backslash continuation is unnecessary here and will cause checkpatch warning.
>

Re: [PATCH v2 4/4] virtio: check if any kernel driver is manipulating the virtio device

[Yuanhan Liu]

On Mon, Jan 04, 2016 at 05:56:49PM +0000, Xie, Huawei wrote:
> On 1/5/2016 1:24 AM, Stephen Hemminger wrote:
> > On Mon,  4 Jan 2016 01:56:13 +0800
> > Huawei Xie <huawei.xie@intel.com> wrote:
> >
> >> +	if (pci_dev->kdrv != RTE_KDRV_NONE) {
> >> +		PMD_INIT_LOG(INFO,
> >> +			"kernel driver is manipulating this device." \
> >> +			" Please unbind the kernel driver.");
> > Splitting strings in general is a bad idea since it makes it harder to find log messages.
> > Also the first clause is lower case and the second is captialized.
> Got it. This is to avoid 80 char warning. Will put it in one line to
> make it friendly for searching.

I agree with Stephen that _in general_ it's a bad idea. But for this
case, I think it's okay, as it'd be enough to locate the code by
searching "manipulating this device", or "unbind the kernel driver",
or other combinations. I mean, nobody would try searching with:

  "kernel driver is manipulating this device. Please unbind the kernel driver."

Right?

	--yliu

> The first clause is lower is because it actually follows "%s():".
> >
> > Lastly, the backslash continuation is unnecessary here and will cause checkpatch warning.
> >
> 

Re: [PATCH 4/4] virtio: check if any kernel driver is manipulating the device

[Panu Matilainen]

On 01/04/2016 11:02 AM, Xie, Huawei wrote:
> On 12/25/2015 6:33 PM, Xie, Huawei wrote:
>> virtio PMD could use IO port to configure the virtio device without
>> using uio driver.
>>
>> There are two issues with previous implementation:
>> 1) virtio PMD will take over each virtio device blindly even if some
>> are not intended for DPDK.
>> 2) driver conflict between virtio PMD and virtio-net kernel driver.
>>
>> This patch checks if there is any kernel driver manipulating the virtio
>> device before virtio PMD uses IO port to configure the device.
>>
>> 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 | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
>> index 00015ef..504346a 100644
>> --- a/drivers/net/virtio/virtio_ethdev.c
>> +++ b/drivers/net/virtio/virtio_ethdev.c
>> @@ -1138,6 +1138,13 @@ static int virtio_resource_init_by_ioports(struct rte_pci_device *pci_dev)
>>   	int found = 0;
>>   	size_t linesz;
>>
>> +	if (pci_dev->kdrv != RTE_KDRV_NONE) {
>> +		PMD_INIT_LOG(ERR,
> Better change ERR to INFO and revise the message followed, since user
> might not want to use this device for DPDK.

Indeed. The whole point of this exercise is to have a clear way of 
telling DPDK which virtio devices it should (and should not) use, so it 
should just act accordingly and shut up.

>> +			"%s(): kernel driver is manipulating this device." \
>> +			" Please unbind the kernel driver.", __func__);

I'd suggest just dropping the whole message, DPDK doesn't log such 
messages for any other devices either. That, or make it a generic 
debug-level log in pci_scan_one().

	- Panu -

Re: [PATCH v2 4/4] virtio: check if any kernel driver is manipulating the virtio device

[Ferruh Yigit]

On Mon, Jan 04, 2016 at 05:56:49PM +0000, Xie, Huawei wrote:
> On 1/5/2016 1:24 AM, Stephen Hemminger wrote:
> > On Mon,  4 Jan 2016 01:56:13 +0800
> > Huawei Xie <huawei.xie@intel.com> wrote:
> >
> >> +	if (pci_dev->kdrv != RTE_KDRV_NONE) {
> >> +		PMD_INIT_LOG(INFO,
> >> +			"kernel driver is manipulating this device." \
> >> +			" Please unbind the kernel driver.");
> > Splitting strings in general is a bad idea since it makes it harder to find log messages.
> > Also the first clause is lower case and the second is captialized.
> Got it. This is to avoid 80 char warning. Will put it in one line to
> make it friendly for searching.
> The first clause is lower is because it actually follows "%s():".

Indeed checkpatch lets logging functions be longer than 80 chars, so does not forces to split message.
But checkpatch detects kernel logging functions only, not ours, that is why we are still getting over 80 chars warning.

Should we ignore LONG_LINE_STRING warnings in checkpatch?

Thanks,
ferruh 

Re: [PATCH v2 4/4] virtio: check if any kernel driver is manipulating the virtio device

[Panu Matilainen]

On 01/03/2016 07:56 PM, Huawei Xie wrote:
> v2 changes:
>   change LOG level from ERR to INFO
>
> virtio PMD could use IO port to configure the virtio device without
> using uio driver.
>
> There are two issues with previous implementation:
> 1) virtio PMD will take over each virtio device blindly even if some
> are not intended for DPDK.
> 2) driver conflict between virtio PMD and virtio-net kernel driver.
>
> This patch checks if there is any kernel driver manipulating the virtio
> device before virtio PMD uses IO port to configure the device.
>
> 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 | 7 +++++++
>   1 file changed, 7 insertions(+)
>
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index e815acd..7a50dac 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1138,6 +1138,13 @@ static int virtio_resource_init_by_ioports(struct rte_pci_device *pci_dev)
>   	int found = 0;
>   	size_t linesz;
>
> +	if (pci_dev->kdrv != RTE_KDRV_NONE) {
> +		PMD_INIT_LOG(INFO,
> +			"kernel driver is manipulating this device." \
> +			" Please unbind the kernel driver.");

At the very least this message needs to be changed.

Like said earlier, I think the message could just as well be dropped 
entirely, but at least it should be something to the tune of "ignoring 
kernel owned device" instead of asking the user to break their 
configuration.

	- Panu -

Re: [PATCH v2 0/4] fix the issue that DPDK takes over virtio device blindly

[Xie, Huawei]

On 1/5/2016 1:25 AM, Stephen Hemminger wrote:
> On Mon,  4 Jan 2016 01:56:09 +0800
> Huawei Xie <huawei.xie@intel.com> wrote:
>
>> v2 changes:
>>  Remove unnecessary assignment of NULL to dev->data->mac_addrs
>>  Ajust one comment's position
>>  change LOG level from ERR to INFO
>>
>> virtio PMD doesn't set RTE_PCI_DRV_NEED_MAPPING in drv_flags of its
>> eth_driver. It will try igb_uio and PORT IO in turn to configure
>> virtio device. Even user in guest VM doesn't want to use virtio for
>> DPDK, virtio PMD will take over the device blindly.
>>
>> The more serious problem is kernel driver is still manipulating the
>> device, which causes driver conflict.
>>
>> This patch checks if there is any kernel driver manipulating the
>> virtio device before virtio PMD uses port IO to configure the device.
>>
>> Huawei Xie (4):
>>   eal: make the comment more accurate
>>   eal: set kdrv to RTE_KDRV_NONE if kernel driver isn't manipulating the device.
>>   virtio: return 1 to tell the kernel we don't take over this device
>>   virtio: check if any kernel driver is manipulating the virtio device
>>
>>  drivers/net/virtio/virtio_ethdev.c     | 16 ++++++++++++++--
>>  lib/librte_eal/common/eal_common_pci.c |  8 ++++----
>>  lib/librte_eal/linuxapp/eal/eal_pci.c  |  2 +-
>>  3 files changed, 19 insertions(+), 7 deletions(-)
>>
> Overall looks good, thanks for addressing this.
>
> It would be good to note that VFIO no-IOMMU mode should work for this
> as well.

It isn't implemented yet in virtio PMD. I could add a note in the commit
message. Do you plan to implement this?

>

Re: [PATCH v2 0/4] fix the issue that DPDK takes over virtio device blindly

[Santosh Shukla]

On Tue, Jan 12, 2016 at 8:32 AM, Xie, Huawei <huawei.xie@intel.com> wrote:
>
> On 1/5/2016 1:25 AM, Stephen Hemminger wrote:
> > On Mon,  4 Jan 2016 01:56:09 +0800
> > Huawei Xie <huawei.xie@intel.com> wrote:
> >
> >> v2 changes:
> >>  Remove unnecessary assignment of NULL to dev->data->mac_addrs
> >>  Ajust one comment's position
> >>  change LOG level from ERR to INFO
> >>
> >> virtio PMD doesn't set RTE_PCI_DRV_NEED_MAPPING in drv_flags of its
> >> eth_driver. It will try igb_uio and PORT IO in turn to configure
> >> virtio device. Even user in guest VM doesn't want to use virtio for
> >> DPDK, virtio PMD will take over the device blindly.
> >>
> >> The more serious problem is kernel driver is still manipulating the
> >> device, which causes driver conflict.
> >>
> >> This patch checks if there is any kernel driver manipulating the
> >> virtio device before virtio PMD uses port IO to configure the device.
> >>
> >> Huawei Xie (4):
> >>   eal: make the comment more accurate
> >>   eal: set kdrv to RTE_KDRV_NONE if kernel driver isn't manipulating the device.
> >>   virtio: return 1 to tell the kernel we don't take over this device
> >>   virtio: check if any kernel driver is manipulating the virtio device
> >>
> >>  drivers/net/virtio/virtio_ethdev.c     | 16 ++++++++++++++--
> >>  lib/librte_eal/common/eal_common_pci.c |  8 ++++----
> >>  lib/librte_eal/linuxapp/eal/eal_pci.c  |  2 +-
> >>  3 files changed, 19 insertions(+), 7 deletions(-)
> >>
> > Overall looks good, thanks for addressing this.
> >
> > It would be good to note that VFIO no-IOMMU mode should work for this
> > as well.
>
> It isn't implemented yet in virtio PMD. I could add a note in the commit
> message. Do you plan to implement this?
>

I can send vfio-noiommu patches for this one, as I am looking at
vfio-noiommu for virtio  for my arm v4 patch series. Stephen, let me
know if you already started working on this?

Also for some reason I can't find [3/4] patch, could you point me to
patch link? Thanks.
>
> >
>

Re: [PATCH v2 0/4] fix the issue that DPDK takes over virtio device blindly

[Xie, Huawei]

On 1/12/2016 12:24 PM, Santosh Shukla wrote:
> On Tue, Jan 12, 2016 at 8:32 AM, Xie, Huawei <huawei.xie@intel.com> wrote:
>> On 1/5/2016 1:25 AM, Stephen Hemminger wrote:
>>> On Mon,  4 Jan 2016 01:56:09 +0800
>>> Huawei Xie <huawei.xie@intel.com> wrote:
>>>
>>>> v2 changes:
>>>>  Remove unnecessary assignment of NULL to dev->data->mac_addrs
>>>>  Ajust one comment's position
>>>>  change LOG level from ERR to INFO
>>>>
>>>> virtio PMD doesn't set RTE_PCI_DRV_NEED_MAPPING in drv_flags of its
>>>> eth_driver. It will try igb_uio and PORT IO in turn to configure
>>>> virtio device. Even user in guest VM doesn't want to use virtio for
>>>> DPDK, virtio PMD will take over the device blindly.
>>>>
>>>> The more serious problem is kernel driver is still manipulating the
>>>> device, which causes driver conflict.
>>>>
>>>> This patch checks if there is any kernel driver manipulating the
>>>> virtio device before virtio PMD uses port IO to configure the device.
>>>>
>>>> Huawei Xie (4):
>>>>   eal: make the comment more accurate
>>>>   eal: set kdrv to RTE_KDRV_NONE if kernel driver isn't manipulating the device.
>>>>   virtio: return 1 to tell the kernel we don't take over this device
>>>>   virtio: check if any kernel driver is manipulating the virtio device
>>>>
>>>>  drivers/net/virtio/virtio_ethdev.c     | 16 ++++++++++++++--
>>>>  lib/librte_eal/common/eal_common_pci.c |  8 ++++----
>>>>  lib/librte_eal/linuxapp/eal/eal_pci.c  |  2 +-
>>>>  3 files changed, 19 insertions(+), 7 deletions(-)
>>>>
>>> Overall looks good, thanks for addressing this.
>>>
>>> It would be good to note that VFIO no-IOMMU mode should work for this
>>> as well.
>> It isn't implemented yet in virtio PMD. I could add a note in the commit
>> message. Do you plan to implement this?
>>
> I can send vfio-noiommu patches for this one, as I am looking at
> vfio-noiommu for virtio  for my arm v4 patch series. Stephen, let me
> know if you already started working on this?
>
> Also for some reason I can't find [3/4] patch, could you point me to
> patch link? Thanks.
Thanks. Here is the patch: http://www.dpdk.org/dev/patchwork/patch/9720/

Re: [PATCH v2 0/4] fix the issue that DPDK takes over virtio device blindly

[Santosh Shukla]

On Tue, Jan 12, 2016 at 10:46 AM, Xie, Huawei <huawei.xie@intel.com> wrote:
> On 1/12/2016 12:24 PM, Santosh Shukla wrote:
>> On Tue, Jan 12, 2016 at 8:32 AM, Xie, Huawei <huawei.xie@intel.com> wrote:
>>> On 1/5/2016 1:25 AM, Stephen Hemminger wrote:
>>>> On Mon,  4 Jan 2016 01:56:09 +0800
>>>> Huawei Xie <huawei.xie@intel.com> wrote:
>>>>
>>>>> v2 changes:
>>>>>  Remove unnecessary assignment of NULL to dev->data->mac_addrs
>>>>>  Ajust one comment's position
>>>>>  change LOG level from ERR to INFO
>>>>>
>>>>> virtio PMD doesn't set RTE_PCI_DRV_NEED_MAPPING in drv_flags of its
>>>>> eth_driver. It will try igb_uio and PORT IO in turn to configure
>>>>> virtio device. Even user in guest VM doesn't want to use virtio for
>>>>> DPDK, virtio PMD will take over the device blindly.
>>>>>
>>>>> The more serious problem is kernel driver is still manipulating the
>>>>> device, which causes driver conflict.
>>>>>
>>>>> This patch checks if there is any kernel driver manipulating the
>>>>> virtio device before virtio PMD uses port IO to configure the device.
>>>>>
>>>>> Huawei Xie (4):
>>>>>   eal: make the comment more accurate
>>>>>   eal: set kdrv to RTE_KDRV_NONE if kernel driver isn't manipulating the device.
>>>>>   virtio: return 1 to tell the kernel we don't take over this device
>>>>>   virtio: check if any kernel driver is manipulating the virtio device
>>>>>
>>>>>  drivers/net/virtio/virtio_ethdev.c     | 16 ++++++++++++++--
>>>>>  lib/librte_eal/common/eal_common_pci.c |  8 ++++----
>>>>>  lib/librte_eal/linuxapp/eal/eal_pci.c  |  2 +-
>>>>>  3 files changed, 19 insertions(+), 7 deletions(-)
>>>>>
>>>> Overall looks good, thanks for addressing this.
>>>>
>>>> It would be good to note that VFIO no-IOMMU mode should work for this
>>>> as well.
>>> It isn't implemented yet in virtio PMD. I could add a note in the commit
>>> message. Do you plan to implement this?
>>>
>> I can send vfio-noiommu patches for this one, as I am looking at
>> vfio-noiommu for virtio  for my arm v4 patch series. Stephen, let me
>> know if you already started working on this?
>>
>> Also for some reason I can't find [3/4] patch, could you point me to
>> patch link? Thanks.
> Thanks. Here is the patch: http://www.dpdk.org/dev/patchwork/patch/9720/
>

Thanks, I am rebasing my v4 series on top of this patch, although I
couldn't see current series creating issue for vfio-noiommu case.
We'll post feedback in v4 cover-letter.

Re: [PATCH v2 4/4] virtio: check if any kernel driver is manipulating the virtio device

[Thomas Monjalon]

2016-01-07 16:17, Panu Matilainen:
> On 01/03/2016 07:56 PM, Huawei Xie wrote:
> > v2 changes:
> >   change LOG level from ERR to INFO
> >
> > virtio PMD could use IO port to configure the virtio device without
> > using uio driver.
> >
> > There are two issues with previous implementation:
> > 1) virtio PMD will take over each virtio device blindly even if some
> > are not intended for DPDK.
> > 2) driver conflict between virtio PMD and virtio-net kernel driver.
> >
> > This patch checks if there is any kernel driver manipulating the virtio
> > device before virtio PMD uses IO port to configure the device.
> >
> > 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 | 7 +++++++
> >   1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> > index e815acd..7a50dac 100644
> > --- a/drivers/net/virtio/virtio_ethdev.c
> > +++ b/drivers/net/virtio/virtio_ethdev.c
> > @@ -1138,6 +1138,13 @@ static int virtio_resource_init_by_ioports(struct rte_pci_device *pci_dev)
> >   	int found = 0;
> >   	size_t linesz;
> >
> > +	if (pci_dev->kdrv != RTE_KDRV_NONE) {
> > +		PMD_INIT_LOG(INFO,
> > +			"kernel driver is manipulating this device." \
> > +			" Please unbind the kernel driver.");
> 
> At the very least this message needs to be changed.
> 
> Like said earlier, I think the message could just as well be dropped 
> entirely, but at least it should be something to the tune of "ignoring 
> kernel owned device" instead of asking the user to break their 
> configuration.

Huawei, a v3 is required. Thanks

[PATCH v3 0/4] fix the issue that DPDK takes over virtio device blindly

[Huawei Xie]

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
 change LOG level from ERR to INFO

virtio PMD doesn't set RTE_PCI_DRV_NEED_MAPPING in drv_flags of its
eth_driver. It will try igb_uio and PORT IO in turn to configure
virtio device. Even user in guest VM doesn't want to use virtio for
DPDK, virtio PMD will take over the device blindly.

The more serious problem is kernel driver is still manipulating the
device, which causes driver conflict.

This patch checks if there is any kernel driver manipulating the
virtio device before virtio PMD uses port IO to configure the device.

Huawei Xie (4):
  eal: make the comment more accurate
  eal: set kdrv to RTE_KDRV_NONE if kernel driver isn't manipulating the device.
  virtio: return 1 to tell the kernel we don't take over this device
  virtio: check if kernel driver is manipulating the virtio device

 drivers/net/virtio/virtio_ethdev.c     | 14 ++++++++++++--
 lib/librte_eal/common/eal_common_pci.c |  8 ++++----
 lib/librte_eal/linuxapp/eal/eal_pci.c  |  2 +-
 3 files changed, 17 insertions(+), 7 deletions(-)

-- 
1.8.1.4

[PATCH v3 1/4] eal: make the comment more accurate

[Huawei Xie]

positive return of rte_eal_pci_probe_one_driver means the driver doesn't support the device.

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
---
 lib/librte_eal/common/eal_common_pci.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
index dcfe947..bbcdb2b 100644
--- a/lib/librte_eal/common/eal_common_pci.c
+++ b/lib/librte_eal/common/eal_common_pci.c
@@ -204,7 +204,7 @@ rte_eal_pci_probe_one_driver(struct rte_pci_driver *dr, struct rte_pci_device *d
 		/* call the driver devinit() function */
 		return dr->devinit(dr, dev);
 	}
-	/* return positive value if driver is not found */
+	/* return positive value if driver doesn't support this device */
 	return 1;
 }
 
@@ -259,7 +259,7 @@ rte_eal_pci_detach_dev(struct rte_pci_driver *dr,
 		return 0;
 	}
 
-	/* return positive value if driver is not found */
+	/* return positive value if driver doesn't support this device */
 	return 1;
 }
 
@@ -283,7 +283,7 @@ pci_probe_all_drivers(struct rte_pci_device *dev)
 			/* negative value is an error */
 			return -1;
 		if (rc > 0)
-			/* positive value means driver not found */
+			/* positive value means driver doesn't support it */
 			continue;
 		return 0;
 	}
@@ -310,7 +310,7 @@ pci_detach_all_drivers(struct rte_pci_device *dev)
 			/* negative value is an error */
 			return -1;
 		if (rc > 0)
-			/* positive value means driver not found */
+			/* positive value means driver doesn't support it */
 			continue;
 		return 0;
 	}
-- 
1.8.1.4

[PATCH v3 2/4] eal: set kdrv to RTE_KDRV_NONE if kernel driver isn't manipulating the device.

[Huawei Xie]

Use RTE_KDRV_NONE to indicate that kernel driver isn't manipulating
the device.

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
Acked-by: David Marchand <david.marchand@6wind.com>
---
 lib/librte_eal/linuxapp/eal/eal_pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index bc5b5be..640b190 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -362,7 +362,7 @@ pci_scan_one(const char *dirname, uint16_t domain, uint8_t bus,
 		else
 			dev->kdrv = RTE_KDRV_UNKNOWN;
 	} else
-		dev->kdrv = RTE_KDRV_UNKNOWN;
+		dev->kdrv = RTE_KDRV_NONE;
 
 	/* device is valid, add in list (sorted) */
 	if (TAILQ_EMPTY(&pci_device_list)) {
-- 
1.8.1.4

[PATCH v3 3/4] virtio: return 1 to tell the upper layer we don't take over this device

[Huawei Xie]

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

if virtio_resource_init fails, cleanup the resource and return 1 to
tell the upper layer we don't take over this device. -1 means error
which will cause DPDK to exit.

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
---
 drivers/net/virtio/virtio_ethdev.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index d928339..e815acd 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1287,8 +1287,13 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
 
 	pci_dev = eth_dev->pci_dev;
 
-	if (virtio_resource_init(pci_dev) < 0)
-		return -1;
+	if (virtio_resource_init(pci_dev) < 0) {
+		rte_free(eth_dev->data->mac_addrs);
+		/* Return 1 to tell the upper layer we don't take over
+		 * this device.
+		 */
+		return 1;
+	}
 
 	hw->use_msix = virtio_has_msix(&pci_dev->addr);
 	hw->io_base = (uint32_t)(uintptr_t)pci_dev->mem_resource[0].addr;
-- 
1.8.1.4

[PATCH v3 4/4] virtio: check if kernel driver is manipulating the virtio device

[Huawei Xie]

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:
 change LOG level from ERR to INFO

virtio PMD could use IO port to configure the virtio device without
using uio driver(vfio-noniommu mode should work as well).

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

This patch checks if there is any kernel driver manipulating the virtio
device before virtio PMD uses IO port to configure the device.

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 | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index e815acd..ea1874a 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1138,6 +1138,11 @@ static int virtio_resource_init_by_ioports(struct rte_pci_device *pci_dev)
 	int found = 0;
 	size_t linesz;
 
+	if (pci_dev->kdrv != RTE_KDRV_NONE) {
+		PMD_INIT_LOG(INFO, "skip kernel managed virtio device.");
+		return -1;
+	}
+
 	snprintf(pci_id, sizeof(pci_id), PCI_PRI_FMT,
 		 pci_dev->addr.domain,
 		 pci_dev->addr.bus,
-- 
1.8.1.4

Re: [PATCH v3 4/4] virtio: check if kernel driver is manipulating the virtio device

[Panu Matilainen]

On 01/27/2016 05:21 PM, Huawei Xie wrote:
> 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:
>   change LOG level from ERR to INFO
>
> virtio PMD could use IO port to configure the virtio device without
> using uio driver(vfio-noniommu mode should work as well).
>
> There are two issues with previous implementation:
> 1) virtio PMD will take over each virtio device blindly even if some
> are not intended for DPDK.
> 2) driver conflict between virtio PMD and virtio-net kernel driver.
>
> This patch checks if there is any kernel driver manipulating the virtio
> device before virtio PMD uses IO port to configure the device.
>
> 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 | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index e815acd..ea1874a 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1138,6 +1138,11 @@ static int virtio_resource_init_by_ioports(struct rte_pci_device *pci_dev)
>   	int found = 0;
>   	size_t linesz;
>
> +	if (pci_dev->kdrv != RTE_KDRV_NONE) {
> +		PMD_INIT_LOG(INFO, "skip kernel managed virtio device.");
> +		return -1;
> +	}
> +
>   	snprintf(pci_id, sizeof(pci_id), PCI_PRI_FMT,
>   		 pci_dev->addr.domain,
>   		 pci_dev->addr.bus,
>

"Manage" is a good term for this, much better than "manipulate" used in 
the subject of this patch and patch 2/4.

"Check if kernel is manipulating foo" sounds like something that is 
happening right now, as in "wait until kernel has stopped fiddling with 
it and then do our own stuff while its quiet", managed makes is clear 
its about the overall state instead.

Not asking you to submit v4 just because of that, but if the need arises 
for other reasons it'd be nice to fix it as well, otherwise perhaps 
Thomas can adjust it while committing?

	- Panu -

Re: [PATCH v3 0/4] fix the issue that DPDK takes over virtio device blindly

[Yuanhan Liu]

On Wed, Jan 27, 2016 at 11:21:18PM +0800, Huawei Xie wrote:
> 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
>  change LOG level from ERR to INFO
> 
> virtio PMD doesn't set RTE_PCI_DRV_NEED_MAPPING in drv_flags of its
> eth_driver. It will try igb_uio and PORT IO in turn to configure
> virtio device. Even user in guest VM doesn't want to use virtio for
> DPDK, virtio PMD will take over the device blindly.
> 
> The more serious problem is kernel driver is still manipulating the
> device, which causes driver conflict.
> 
> This patch checks if there is any kernel driver manipulating the
> virtio device before virtio PMD uses port IO to configure the device.

Series acked:

Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

	--yliu

Re: [PATCH v3 0/4] fix the issue that DPDK takes over virtio device blindly

[Thomas Monjalon]

> Huawei Xie (4):
>   eal: make the comment more accurate
>   eal: set kdrv to RTE_KDRV_NONE if kernel driver isn't manipulating the device.
>   virtio: return 1 to tell the kernel we don't take over this device
>   virtio: check if kernel driver is manipulating the virtio device

The virtio PCI code has been refactored.
Please Huawei, would it be possible to rebase on master?

[PATCH v4 0/4] fix the issue that DPDK takes over virtio device blindly

[Huawei Xie]

v4 changes:
 Rebase as IO port map is moved to EAL.
 Reword some commit messages.
 Don't fall back to PORT IO if VFIO/UIO fails.

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
 change LOG level from ERR to INFO

virtio PMD doesn't set RTE_PCI_DRV_NEED_MAPPING in drv_flags of its
eth_driver. It will try igb_uio and PORT IO in turn to configure
virtio device. Even user in guest VM doesn't want to use virtio for
DPDK, virtio PMD will take over the device blindly.

The more serious problem is kernel driver is still managing the device,
which causes driver conflict.

This patch checks if there is kernel driver(other than UIO/VFIO)
managing the virtio device before virtio PMD uses port IO to configure
the device.

Huawei Xie (4):
  eal: make the comment more accurate
  eal: set kdrv to RTE_KDRV_NONE if kernel driver isn't managing the device.
  eal: call pci_ioport_map when kernel driver isn't managing the device
  virtio: return 1 to tell the upper layer we don't take over this device

 drivers/net/virtio/virtio_ethdev.c     |  9 +++++++--
 drivers/net/virtio/virtio_pci.c        | 15 ++++++++++++++-
 lib/librte_eal/common/eal_common_pci.c |  8 ++++----
 lib/librte_eal/linuxapp/eal/eal_pci.c  | 16 +++++++---------
 4 files changed, 32 insertions(+), 16 deletions(-)

-- 
1.8.1.4

[PATCH v4 1/4] eal: make the comment more accurate

[Huawei Xie]

positive return of rte_eal_pci_probe_one_driver means the driver doesn't support
the device.

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 lib/librte_eal/common/eal_common_pci.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
index 96d5113..797e7e3 100644
--- a/lib/librte_eal/common/eal_common_pci.c
+++ b/lib/librte_eal/common/eal_common_pci.c
@@ -204,7 +204,7 @@ rte_eal_pci_probe_one_driver(struct rte_pci_driver *dr, struct rte_pci_device *d
 		/* call the driver devinit() function */
 		return dr->devinit(dr, dev);
 	}
-	/* return positive value if driver is not found */
+	/* return positive value if driver doesn't support this device */
 	return 1;
 }
 
@@ -259,7 +259,7 @@ rte_eal_pci_detach_dev(struct rte_pci_driver *dr,
 		return 0;
 	}
 
-	/* return positive value if driver is not found */
+	/* return positive value if driver doesn't support this device */
 	return 1;
 }
 
@@ -283,7 +283,7 @@ pci_probe_all_drivers(struct rte_pci_device *dev)
 			/* negative value is an error */
 			return -1;
 		if (rc > 0)
-			/* positive value means driver not found */
+			/* positive value means driver doesn't support it */
 			continue;
 		return 0;
 	}
@@ -310,7 +310,7 @@ pci_detach_all_drivers(struct rte_pci_device *dev)
 			/* negative value is an error */
 			return -1;
 		if (rc > 0)
-			/* positive value means driver not found */
+			/* positive value means driver doesn't support it */
 			continue;
 		return 0;
 	}
-- 
1.8.1.4

[PATCH v4 2/4] eal: set kdrv to RTE_KDRV_NONE if kernel driver isn't managing the device

[Huawei Xie]

v4 changes:
 reword the commit message. When we mention kernel driver, emphasizes
that it includes UIO/VFIO.

Use RTE_KDRV_NONE to indicate that kernel driver(including UIO/VFIO)
isn't manipulating the device.

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index 4346973..b44fa32 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -362,7 +362,7 @@ pci_scan_one(const char *dirname, uint16_t domain, uint8_t bus,
 		else
 			dev->kdrv = RTE_KDRV_UNKNOWN;
 	} else
-		dev->kdrv = RTE_KDRV_UNKNOWN;
+		dev->kdrv = RTE_KDRV_NONE;
 
 	/* device is valid, add in list (sorted) */
 	if (TAILQ_EMPTY(&pci_device_list)) {
-- 
1.8.1.4

[PATCH v4 3/4] eal: call pci_ioport_map when kernel driver isn't managing the device

[Huawei Xie]

Call rte_eal_pci_ioport_map only if driver type is RTE_KDRV_NONE, which
means kernel driver(including UIO/VFIO) isn't managing the device.

other minor changes:
 * use RTE_ARCH_X86 for pci ioport map
 * rework rte_eal_pci_ioport_map a bit

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_pci.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index b44fa32..112d540 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -621,7 +621,7 @@ int rte_eal_pci_write_config(const struct rte_pci_device *device,
 	}
 }
 
-#if defined(RTE_ARCH_X86_64) || defined(RTE_ARCH_I686)
+#if defined(RTE_ARCH_X86)
 static int
 pci_ioport_map(struct rte_pci_device *dev, int bar __rte_unused,
 	       struct rte_pci_ioport *p)
@@ -685,12 +685,11 @@ int
 rte_eal_pci_ioport_map(struct rte_pci_device *dev, int bar,
 		       struct rte_pci_ioport *p)
 {
-	int ret;
+	int ret = -1;
 
 	switch (dev->kdrv) {
 #ifdef VFIO_PRESENT
 	case RTE_KDRV_VFIO:
-		ret = -1;
 		if (pci_vfio_is_enabled())
 			ret = pci_vfio_ioport_map(dev, bar, p);
 		break;
@@ -699,14 +698,13 @@ rte_eal_pci_ioport_map(struct rte_pci_device *dev, int bar,
 	case RTE_KDRV_UIO_GENERIC:
 		ret = pci_uio_ioport_map(dev, bar, p);
 		break;
-	default:
-#if defined(RTE_ARCH_X86_64) || defined(RTE_ARCH_I686)
-		/* special case for x86 ... */
+	case RTE_KDRV_NONE:
+#if defined(RTE_ARCH_X86)
 		ret = pci_ioport_map(dev, bar, p);
-#else
-		ret = -1;
 #endif
 		break;
+	default:
+		break;
 	}
 
 	if (!ret)
-- 
1.8.1.4

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

[Huawei Xie]

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(-)

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);
-- 
1.8.1.4

Re: [PATCH v3 0/4] fix the issue that DPDK takes over virtio device blindly

[Xie, Huawei]

On 2/24/2016 8:45 PM, Thomas Monjalon wrote:
>> Huawei Xie (4):
>>   eal: make the comment more accurate
>>   eal: set kdrv to RTE_KDRV_NONE if kernel driver isn't manipulating the device.
>>   virtio: return 1 to tell the kernel we don't take over this device
>>   virtio: check if kernel driver is manipulating the virtio device
> The virtio PCI code has been refactored.
> Please Huawei, would it be possible to rebase on master?

OK. Since IO port map is moved to EAL layer, it is not straightforward
like before for virtio PMD to distinguish the reason why port map fails.
We have two choices. Return 1 to the upper layer to say that we don't
take over the device for all the map failures or we check the driver
type, return -1 for UIO/VFIO driver error, return 1 for kernel driver,
which is a bit overelaborate.

>

Re: [PATCH v3 0/4] fix the issue that DPDK takes over virtio device blindly

[David Marchand]

On Fri, Feb 26, 2016 at 7:09 AM, Xie, Huawei <huawei.xie@intel.com> wrote:
> On 2/24/2016 8:45 PM, Thomas Monjalon wrote:
>>> Huawei Xie (4):
>>>   eal: make the comment more accurate
>>>   eal: set kdrv to RTE_KDRV_NONE if kernel driver isn't manipulating the device.
>>>   virtio: return 1 to tell the kernel we don't take over this device
>>>   virtio: check if kernel driver is manipulating the virtio device
>> The virtio PCI code has been refactored.
>> Please Huawei, would it be possible to rebase on master?
>
> OK. Since IO port map is moved to EAL layer, it is not straightforward
> like before for virtio PMD to distinguish the reason why port map fails.
> We have two choices. Return 1 to the upper layer to say that we don't
> take over the device for all the map failures or we check the driver
> type, return -1 for UIO/VFIO driver error, return 1 for kernel driver,
> which is a bit overelaborate.

The important thing is to have eal report "none" driver first (your
2nd patch) , then in ioport_map, "none" driver will trigger the x86
special case (see other discussion [1]).
For "uio" drivers, code (when fixed for uio_pci_generic) already does
the right stuff.
"vfio" is handled.
Anything else should fail once we have the "none" driver correctly reported.


What did I miss ?


[1] http://dpdk.org/ml/archives/dev/2016-February/034035.html

-- 
David Marchand

Re: [PATCH v3 0/4] fix the issue that DPDK takes over virtio device blindly

[Xie, Huawei]

On 2/26/2016 4:41 PM, David Marchand wrote:
> On Fri, Feb 26, 2016 at 7:09 AM, Xie, Huawei <huawei.xie@intel.com> wrote:
>> On 2/24/2016 8:45 PM, Thomas Monjalon wrote:
>>>> Huawei Xie (4):
>>>>   eal: make the comment more accurate
>>>>   eal: set kdrv to RTE_KDRV_NONE if kernel driver isn't manipulating the device.
>>>>   virtio: return 1 to tell the kernel we don't take over this device
>>>>   virtio: check if kernel driver is manipulating the virtio device
>>> The virtio PCI code has been refactored.
>>> Please Huawei, would it be possible to rebase on master?
>> OK. Since IO port map is moved to EAL layer, it is not straightforward
>> like before for virtio PMD to distinguish the reason why port map fails.
>> We have two choices. Return 1 to the upper layer to say that we don't
>> take over the device for all the map failures or we check the driver
>> type, return -1 for UIO/VFIO driver error, return 1 for kernel driver,
>> which is a bit overelaborate.
> The important thing is to have eal report "none" driver first (your
> 2nd patch) , then in ioport_map, "none" driver will trigger the x86
> special case (see other discussion [1]).
> For "uio" drivers, code (when fixed for uio_pci_generic) already does
> the right stuff.
> "vfio" is handled.
> Anything else should fail once we have the "none" driver correctly reported.
>

in rte_eal_pci_map_device:
            case RTE_KDRV_NONE:
#if defined(RTE_ARCH_X86)
                ret = pci_ioport_map(dev, bar, p);
#endif
                break;
        }


in vtpci_init:
        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;
        }


in dev_init
    ret = vt_pci_init
    if (ret)
        return ret
> What did I miss ?
>
>
> [1] http://dpdk.org/ml/archives/dev/2016-February/034035.html
>

Re: [PATCH v4 2/4] eal: set kdrv to RTE_KDRV_NONE if kernel driver isn't managing the device

[Xie, Huawei]

在 2/27/2016 1:47 AM, Xie, Huawei 写道:
> Use RTE_KDRV_NONE to indicate that kernel driver(including UIO/VFIO)
> isn't manipulating the device.
Thomas, could you kindly help change manipulating->managing? I have
changed others per Panu's suggestion but missed this.

Re: [PATCH v4 2/4] eal: set kdrv to RTE_KDRV_NONE if kernel driver isn't managing the device

[David Marchand]

On Fri, Feb 26, 2016 at 2:53 AM, Huawei Xie <huawei.xie@intel.com> wrote:
> v4 changes:
>  reword the commit message. When we mention kernel driver, emphasizes
> that it includes UIO/VFIO.

Annotations should not be part of the commitlog itself.

> Use RTE_KDRV_NONE to indicate that kernel driver(including UIO/VFIO)
> isn't manipulating the device.

missing space before (

> Signed-off-by: Huawei Xie <huawei.xie@intel.com>
> Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

Thought I already acked this.
Anyway,
Acked-by: David Marchand <david.marchand@6wind.com>


-- 
David Marchand

Re: [PATCH v4 1/4] eal: make the comment more accurate

[David Marchand]

On Fri, Feb 26, 2016 at 2:53 AM, Huawei Xie <huawei.xie@intel.com> wrote:
> positive return of rte_eal_pci_probe_one_driver means the driver doesn't support
> the device.
>
> Signed-off-by: Huawei Xie <huawei.xie@intel.com>
> Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

Acked-by: David Marchand <david.marchand@6wind.com>

-- 
David Marchand

Re: [PATCH v4 2/4] eal: set kdrv to RTE_KDRV_NONE if kernel driver isn't managing the device

[Xie, Huawei]

On 2/29/2016 4:47 PM, David Marchand wrote:
> On Fri, Feb 26, 2016 at 2:53 AM, Huawei Xie <huawei.xie@intel.com> wrote:
>> v4 changes:
>>  reword the commit message. When we mention kernel driver, emphasizes
>> that it includes UIO/VFIO.
> Annotations should not be part of the commitlog itself.

Do you mean that "rewording the commit message" should not appear in the
commit message itself?  Those version changes will not appear in the
commit log when applied, right? So i added this so that reviewers know
that i have changed the commit message otherwise they don't need to
waste their time reviewing the commit message again. Is it that even if
i send a new patch version with only the changes to the commit message ,
i needn't mention this?
>
>> Use RTE_KDRV_NONE to indicate that kernel driver(including UIO/VFIO)
>> isn't manipulating the device.
> missing space before (

Thomas, could you help change this?

>
>> Signed-off-by: Huawei Xie <huawei.xie@intel.com>
>> Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> Thought I already acked this.
> Anyway,
> Acked-by: David Marchand <david.marchand@6wind.com>
>
>

Re: [PATCH v4 3/4] eal: call pci_ioport_map when kernel driver isn't managing the device

[David Marchand]

On Fri, Feb 26, 2016 at 2:53 AM, Huawei Xie <huawei.xie@intel.com> wrote:
> Call rte_eal_pci_ioport_map only if driver type is RTE_KDRV_NONE, which
> means kernel driver(including UIO/VFIO) isn't managing the device.

I suppose you meant 'Call pci_ioport_map when the pci device is not
bound to a kernel driver'.
If you keep on with your choice of words, at least put a space before the (.


> other minor changes:
>  * use RTE_ARCH_X86 for pci ioport map

This is a trivial change, but this should not be here.

>  * rework rte_eal_pci_ioport_map a bit

Well, not sure this comment helps the review, and anyway, why did you
need to change this ?
Your modification should be the smallest possible.


> Signed-off-by: Huawei Xie <huawei.xie@intel.com>

Let aside these nits.
Acked-by: David Marchand <david.marchand@6wind.com>


-- 
David Marchand

Re: [PATCH v4 2/4] eal: set kdrv to RTE_KDRV_NONE if kernel driver isn't managing the device

[David Marchand]

On Mon, Feb 29, 2016 at 10:00 AM, Xie, Huawei <huawei.xie@intel.com> wrote:
> On 2/29/2016 4:47 PM, David Marchand wrote:
>> On Fri, Feb 26, 2016 at 2:53 AM, Huawei Xie <huawei.xie@intel.com> wrote:
>>> v4 changes:
>>>  reword the commit message. When we mention kernel driver, emphasizes
>>> that it includes UIO/VFIO.
>> Annotations should not be part of the commitlog itself.
>
> Do you mean that "rewording the commit message" should not appear in the
> commit message itself?  Those version changes will not appear in the
> commit log when applied, right? So i added this so that reviewers know

Try to apply it.

http://dpdk.org/dev :

"Annotations take place after the 3 dashes and should explicit what
has changed since the previous version.".


-- 
David Marchand

Re: [PATCH v4 4/4] virtio: return 1 to tell the upper layer we don't take over this device

[Santosh Shukla]

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
>

Re: [PATCH v4 4/4] virtio: return 1 to tell the upper layer we don't take over this device

[Thomas Monjalon]

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.

Re: [PATCH v4 4/4] virtio: return 1 to tell the upper layer we don't take over this device

[Xie, Huawei]

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;
}
   

>
>

Re: [PATCH v4 4/4] virtio: return 1 to tell the upper layer we don't take over this device

[Thomas Monjalon]

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.

Re: [PATCH v4 4/4] virtio: return 1 to tell the upper layer we don't take over this device

[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?

Re: [PATCH v4 4/4] virtio: return 1 to tell the upper layer we don't take over this device

[Thomas Monjalon]

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.

Re: [PATCH v4 4/4] virtio: return 1 to tell the upper layer we don't take over this device

[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.
Even that, the upper layer should explicitly put the non-blacklisted
device into whitelist, i mean here we should only deal with whitelist.
>

[PATCH v5 0/6] fix the issue that DPDK takes over virtio device blindly

[Huawei Xie]

v5 changes:
 Split patches
 Remove free of mac addr when vtpci_init fails. Will send the fix in
a seperate patch.
 Fail if the virtio device is whitelisted but bound to kernel driver.

v4 changes:
 Rebase as IO port map is moved to EAL.
 Reword some commit messages.
 Don't fall back to PORT IO if VFIO/UIO fails.

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
 change LOG level from ERR to INFO

Huawei Xie (6):
  eal: make the comment more accurate
  eal: set kdrv to RTE_KDRV_NONE if kernel driver isn't managing the device.
  eal: use new RTE_ARCH_X86 macro for x86 arch
  eal: simple code rework
  eal: map IO port only when kernel driver isn't managing the device
  virtio: return 1 to tell the upper layer we don't take over this device

 drivers/net/virtio/virtio_ethdev.c     |  6 ++++--
 drivers/net/virtio/virtio_pci.c        | 16 +++++++++++++++-
 lib/librte_eal/common/eal_common_pci.c |  8 ++++----
 lib/librte_eal/linuxapp/eal/eal_pci.c  | 22 ++++++++++------------
 4 files changed, 33 insertions(+), 19 deletions(-)

-- 
1.8.1.4

[PATCH v5 1/6] eal: make the comment more accurate

[Huawei Xie]

positive return of devinit of pci driver means the driver doesn't support
this device.

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
Acked-by: David Marchand <david.marchand@6wind.com>
---
 lib/librte_eal/common/eal_common_pci.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
index 96d5113..797e7e3 100644
--- a/lib/librte_eal/common/eal_common_pci.c
+++ b/lib/librte_eal/common/eal_common_pci.c
@@ -204,7 +204,7 @@ rte_eal_pci_probe_one_driver(struct rte_pci_driver *dr, struct rte_pci_device *d
 		/* call the driver devinit() function */
 		return dr->devinit(dr, dev);
 	}
-	/* return positive value if driver is not found */
+	/* return positive value if driver doesn't support this device */
 	return 1;
 }
 
@@ -259,7 +259,7 @@ rte_eal_pci_detach_dev(struct rte_pci_driver *dr,
 		return 0;
 	}
 
-	/* return positive value if driver is not found */
+	/* return positive value if driver doesn't support this device */
 	return 1;
 }
 
@@ -283,7 +283,7 @@ pci_probe_all_drivers(struct rte_pci_device *dev)
 			/* negative value is an error */
 			return -1;
 		if (rc > 0)
-			/* positive value means driver not found */
+			/* positive value means driver doesn't support it */
 			continue;
 		return 0;
 	}
@@ -310,7 +310,7 @@ pci_detach_all_drivers(struct rte_pci_device *dev)
 			/* negative value is an error */
 			return -1;
 		if (rc > 0)
-			/* positive value means driver not found */
+			/* positive value means driver doesn't support it */
 			continue;
 		return 0;
 	}
-- 
1.8.1.4

[PATCH v5 2/6] eal: RTE_KDRV_NONE means kernel driver isn't managing the device

[Huawei Xie]

Use RTE_KDRV_NONE to indicate that kernel driver (other than VFIO/UIO) isn't
managing the device.

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
Acked-by: David Marchand <david.marchand@6wind.com>
---
 lib/librte_eal/linuxapp/eal/eal_pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index 4346973..b44fa32 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -362,7 +362,7 @@ pci_scan_one(const char *dirname, uint16_t domain, uint8_t bus,
 		else
 			dev->kdrv = RTE_KDRV_UNKNOWN;
 	} else
-		dev->kdrv = RTE_KDRV_UNKNOWN;
+		dev->kdrv = RTE_KDRV_NONE;
 
 	/* device is valid, add in list (sorted) */
 	if (TAILQ_EMPTY(&pci_device_list)) {
-- 
1.8.1.4

[PATCH v5 3/6] eal: use new RTE_ARCH_X86 for x86 arch

[Huawei Xie]

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
Acked-by: David Marchand <david.marchand@6wind.com>
---
 lib/librte_eal/linuxapp/eal/eal_pci.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index b44fa32..dc0aa37 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -621,7 +621,7 @@ int rte_eal_pci_write_config(const struct rte_pci_device *device,
 	}
 }
 
-#if defined(RTE_ARCH_X86_64) || defined(RTE_ARCH_I686)
+#if defined(RTE_ARCH_X86)
 static int
 pci_ioport_map(struct rte_pci_device *dev, int bar __rte_unused,
 	       struct rte_pci_ioport *p)
@@ -700,7 +700,7 @@ rte_eal_pci_ioport_map(struct rte_pci_device *dev, int bar,
 		ret = pci_uio_ioport_map(dev, bar, p);
 		break;
 	default:
-#if defined(RTE_ARCH_X86_64) || defined(RTE_ARCH_I686)
+#if defined(RTE_ARCH_X86)
 		/* special case for x86 ... */
 		ret = pci_ioport_map(dev, bar, p);
 #else
@@ -730,7 +730,7 @@ rte_eal_pci_ioport_read(struct rte_pci_ioport *p,
 		pci_uio_ioport_read(p, data, len, offset);
 		break;
 	default:
-#if defined(RTE_ARCH_X86_64) || defined(RTE_ARCH_I686)
+#if defined(RTE_ARCH_X86)
 		/* special case for x86 ... */
 		pci_uio_ioport_read(p, data, len, offset);
 #endif
@@ -753,7 +753,7 @@ rte_eal_pci_ioport_write(struct rte_pci_ioport *p,
 		pci_uio_ioport_write(p, data, len, offset);
 		break;
 	default:
-#if defined(RTE_ARCH_X86_64) || defined(RTE_ARCH_I686)
+#if defined(RTE_ARCH_X86)
 		/* special case for x86 ... */
 		pci_uio_ioport_write(p, data, len, offset);
 #endif
@@ -779,7 +779,7 @@ rte_eal_pci_ioport_unmap(struct rte_pci_ioport *p)
 		ret = pci_uio_ioport_unmap(p);
 		break;
 	default:
-#if defined(RTE_ARCH_X86_64) || defined(RTE_ARCH_I686)
+#if defined(RTE_ARCH_X86)
 		/* special case for x86 ... nothing to do */
 		ret = 0;
 #else
-- 
1.8.1.4

[PATCH v5 4/6] eal: simple code rework

[Huawei Xie]

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
Acked-by: David Marchand <david.marchand@6wind.com>
---
 lib/librte_eal/linuxapp/eal/eal_pci.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index dc0aa37..4ede4cb 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -685,12 +685,11 @@ int
 rte_eal_pci_ioport_map(struct rte_pci_device *dev, int bar,
 		       struct rte_pci_ioport *p)
 {
-	int ret;
+	int ret = -1;
 
 	switch (dev->kdrv) {
 #ifdef VFIO_PRESENT
 	case RTE_KDRV_VFIO:
-		ret = -1;
 		if (pci_vfio_is_enabled())
 			ret = pci_vfio_ioport_map(dev, bar, p);
 		break;
@@ -701,10 +700,7 @@ rte_eal_pci_ioport_map(struct rte_pci_device *dev, int bar,
 		break;
 	default:
 #if defined(RTE_ARCH_X86)
-		/* special case for x86 ... */
 		ret = pci_ioport_map(dev, bar, p);
-#else
-		ret = -1;
 #endif
 		break;
 	}
-- 
1.8.1.4

[PATCH v5 5/6] eal: map IO port when kernel driver isn't managing the device

[Huawei Xie]

call rte_eal_pci_ioport_map (on x86) only if the pci device is not bound
to a kernel driver.

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
Acked-by: David Marchand <david.marchand@6wind.com>
---
 lib/librte_eal/linuxapp/eal/eal_pci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index 4ede4cb..833529f 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -698,11 +698,13 @@ rte_eal_pci_ioport_map(struct rte_pci_device *dev, int bar,
 	case RTE_KDRV_UIO_GENERIC:
 		ret = pci_uio_ioport_map(dev, bar, p);
 		break;
-	default:
+	case RTE_KDRV_NONE:
 #if defined(RTE_ARCH_X86)
 		ret = pci_ioport_map(dev, bar, p);
 #endif
 		break;
+	default:
+		break;
 	}
 
 	if (!ret)
-- 
1.8.1.4

[PATCH v5 6/6] virtio: return 1 to tell the upper layer we don't take over this device

[Huawei Xie]

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

There are two issues with 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>
Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
Acked-by: David Marchand <david.marchand@6wind.com>
---
 drivers/net/virtio/virtio_ethdev.c |  6 ++++--
 drivers/net/virtio/virtio_pci.c    | 16 +++++++++++++++-
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index caa970c..06bddd7 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1015,6 +1015,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 +1038,9 @@ 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)
+		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..98fc370 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,15 @@ 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 &&
+		    dev->devargs->type != RTE_DEVTYPE_WHITELISTED_PCI) {
+			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);
-- 
1.8.1.4

Re: [PATCH v4 4/4] virtio: return 1 to tell the upper layer we don't take over this device

[Xie, Huawei]

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.

>

Re: [PATCH v4 4/4] virtio: return 1 to tell the upper layer we don't take over this device

[Thomas Monjalon]

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).

Re: [PATCH v5 3/6] eal: use new RTE_ARCH_X86 for x86 arch

[Thomas Monjalon]

>  lib/librte_eal/linuxapp/eal/eal_pci.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

There are other occurences to fix in EAL.

Re: [PATCH v5 6/6] virtio: return 1 to tell the upper layer we don't take over this device

[Thomas Monjalon]

2016-03-08 23:33, Huawei Xie:
>  	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 &&
> +		    dev->devargs->type != RTE_DEVTYPE_WHITELISTED_PCI) {

I still think you should add 
		  && dev->devargs->type != RTE_DEVTYPE_BLACKLISTED_PCI)
Not a big deal.

> +			PMD_INIT_LOG(INFO,
> +				"skip kernel managed virtio device.");
> +			return 1;
> +		}
>  		return -1;

Re: [PATCH v5 0/6] fix the issue that DPDK takes over virtio device blindly

[Thomas Monjalon]

> Huawei Xie (6):
>   eal: make the comment more accurate
>   eal: set kdrv to RTE_KDRV_NONE if kernel driver isn't managing the device.
>   eal: use new RTE_ARCH_X86 macro for x86 arch

I've completed this patch to use RTE_ARCH_X86 in EAL.

>   eal: simple code rework
>   eal: map IO port only when kernel driver isn't managing the device
>   virtio: return 1 to tell the upper layer we don't take over this device

Applied, thanks