[dpdk-dev] virtio PMD is not working with master version

  Mauricio, thanks for the testing and report.

On Thu, Feb 25, 2016 at 02:30:18PM +0100, David Marchand wrote:
> On Thu, Feb 25, 2016 at 12:30 PM, Mauricio Vásquez
> <mauricio.vasquezbernal@studenti.polito.it> wrote:
> > ...
> > EAL: PCI device 0000:00:04.0 on NUMA socket -1
> > EAL:   probe driver: 1af4:1000 rte_virtio_pmd
> > EAL:   PCI memory mapped at 0x7f892dc00000
> > PMD: virtio_read_caps(): [40] skipping non VNDR cap id: 11
> > PMD: virtio_read_caps(): no modern virtio pci device found.
> > PMD: vtpci_init(): trying with legacy virtio pci.
> > EAL: eal_parse_sysfs_value(): cannot open sysfs value
> > /sys/bus/pci/devices/0000:00:04.0/uio/uio0/portio/port0/start
> > EAL: pci_uio_ioport_map(): cannot parse portio start
> > EAL: Error - exiting with code: 1
> >   Cause: Requested device 0000:00:04.0 cannot be used
> > ...
> 
> [snip]
> 
> > ...
> > PMD: parse_sysfs_value(): parse_sysfs_value(): cannot open sysfs value
> > /sys/bus/pci/devices/0000:00:04.0/uio/uio0/portio/port0/size
> > PMD: virtio_resource_init_by_uio(): virtio_resource_init_by_uio(): cannot
> > parse size
> > PMD: virtio_resource_init_by_ioports(): PCI Port IO found start=0xc100 with
> > size=0x20
> 
> [snip]
> 
> >
> > According to git bisect it appears to be that it does not work anymore after
> > the b8f04520ad71 ("virtio: use PCI ioport API") commit.
> 
> >From the logs, I would say I broke uio_pci_generic since we are in
> "uio" case, but uio portio sysfs does not exist.
> virtio pmd fell back to ioports discovery before my change.

Maybe we can do same?

---
-------------------------------------------------------


If that looks okay to you, I could send a formal patch.

	--yliu

> Problem can be workaround for now by unbinding your device from
> uio_pci_generic or on the contrary bind to igb_uio.
> 
> I've been sick at home, all this week.
> Will see next week for a fix unless someone sends one.
> 
> 
> -- 
> David Marchand
  

On Fri, Feb 26, 2016 at 3:23 AM, Yuanhan Liu
<yuanhan.liu@linux.intel.com> wrote:
> Mauricio, thanks for the testing and report.
>
> On Thu, Feb 25, 2016 at 02:30:18PM +0100, David Marchand wrote:
>> >From the logs, I would say I broke uio_pci_generic since we are in
>> "uio" case, but uio portio sysfs does not exist.
>> virtio pmd fell back to ioports discovery before my change.
>
> Maybe we can do same?

I suppose, but see below.

>
> ---
> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
> index 4346973..579731c 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;
> @@ -700,14 +699,14 @@ rte_eal_pci_ioport_map(struct rte_pci_device *dev, int bar,
>                 ret = pci_uio_ioport_map(dev, bar, p);
>                 break;
>         default:
> +               break;
> +       }
> +
>  #if defined(RTE_ARCH_X86_64) || defined(RTE_ARCH_I686)
> -               /* special case for x86 ... */
> +       /* special case for x86 ... */
> +       if (ret)
>                 ret = pci_ioport_map(dev, bar, p);
> -#else
> -               ret = -1;
>  #endif
> -               break;
> -       }

What if we are supposed to do vfio here, but for some reason init failed ?
Next thing, we will call ioport_read in vfio context, but init went
through the ioports parsing => boom ?

Another issue is that when device is bound to a kernel driver (let's
say virtio-pci here), then init will succeed and pmd will kick in the
device registers.

This special case should really be narrowed down to "uio" and "none"
driver cases.
  

On 2/26/2016 4:29 PM, David Marchand wrote:
> On Fri, Feb 26, 2016 at 3:23 AM, Yuanhan Liu
> <yuanhan.liu@linux.intel.com> wrote:
>> Mauricio, thanks for the testing and report.
>>
>> On Thu, Feb 25, 2016 at 02:30:18PM +0100, David Marchand wrote:
>>> >From the logs, I would say I broke uio_pci_generic since we are in
>>> "uio" case, but uio portio sysfs does not exist.
>>> virtio pmd fell back to ioports discovery before my change.
>> Maybe we can do same?
We shouldn't, :). I am now rebasing the patch to fix the issue that
virtio driver takes the virtio device blindly.
With the patch:
 if driver is VFIO/UIO, and errors happens, returns without falling back
to IO port.
 if no any kernel driver is managing the device, try IO port; otherwise
returns 1 to tell the layer we don't take over this device.

> I suppose, but see below.
>
>> ---
>> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
>> index 4346973..579731c 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;
>> @@ -700,14 +699,14 @@ rte_eal_pci_ioport_map(struct rte_pci_device *dev, int bar,
>>                 ret = pci_uio_ioport_map(dev, bar, p);
>>                 break;
>>         default:
>> +               break;
>> +       }
>> +
>>  #if defined(RTE_ARCH_X86_64) || defined(RTE_ARCH_I686)
>> -               /* special case for x86 ... */
>> +       /* special case for x86 ... */
>> +       if (ret)
>>                 ret = pci_ioport_map(dev, bar, p);
>> -#else
>> -               ret = -1;
>>  #endif
>> -               break;
>> -       }
> What if we are supposed to do vfio here, but for some reason init failed ?
> Next thing, we will call ioport_read in vfio context, but init went
> through the ioports parsing => boom ?
>
> Another issue is that when device is bound to a kernel driver (let's
> say virtio-pci here), then init will succeed and pmd will kick in the
> device registers.
>
> This special case should really be narrowed down to "uio" and "none"
> driver cases.
>
>
  

On Fri, Feb 26, 2016 at 2:14 PM, Xie, Huawei <huawei.xie@intel.com> wrote:
> On 2/26/2016 4:29 PM, David Marchand wrote:
>> On Fri, Feb 26, 2016 at 3:23 AM, Yuanhan Liu
>> <yuanhan.liu@linux.intel.com> wrote:
>>> Mauricio, thanks for the testing and report.
>>>
>>> On Thu, Feb 25, 2016 at 02:30:18PM +0100, David Marchand wrote:
>>>> >From the logs, I would say I broke uio_pci_generic since we are in
>>>> "uio" case, but uio portio sysfs does not exist.
>>>> virtio pmd fell back to ioports discovery before my change.
>>> Maybe we can do same?
> We shouldn't, :). I am now rebasing the patch to fix the issue that
> virtio driver takes the virtio device blindly.
> With the patch:
>  if driver is VFIO/UIO, and errors happens, returns without falling back
> to IO port.

Nice, This will be useful for non-x86 arch case, IO port is NA for
non-x86 arch so falling back to IO port would always fail. so
defaulting to IO port case is incorrect. IMO, not arch agnostic.

>  if no any kernel driver is managing the device, try IO port; otherwise
> returns 1 to tell the layer we don't take over this device.
>


>> I suppose, but see below.
>>
>>> ---
>>> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
>>> index 4346973..579731c 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;
>>> @@ -700,14 +699,14 @@ rte_eal_pci_ioport_map(struct rte_pci_device *dev, int bar,
>>>                 ret = pci_uio_ioport_map(dev, bar, p);
>>>                 break;
>>>         default:
>>> +               break;
>>> +       }
>>> +
>>>  #if defined(RTE_ARCH_X86_64) || defined(RTE_ARCH_I686)
>>> -               /* special case for x86 ... */
>>> +       /* special case for x86 ... */
>>> +       if (ret)
>>>                 ret = pci_ioport_map(dev, bar, p);
>>> -#else
>>> -               ret = -1;
>>>  #endif
>>> -               break;
>>> -       }
>> What if we are supposed to do vfio here, but for some reason init failed ?
>> Next thing, we will call ioport_read in vfio context, but init went
>> through the ioports parsing => boom ?
>>
>> Another issue is that when device is bound to a kernel driver (let's
>> say virtio-pci here), then init will succeed and pmd will kick in the
>> device registers.
>>
>> This special case should really be narrowed down to "uio" and "none"
>> driver cases.
>>
>>
>