[dpdk-dev] [PATCH] bus/pci: set intr_handle type for secondary processes

[dpdk-dev] [PATCH] bus/pci: set intr_handle type for secondary processes

[Alejandro Lucero]

Invoking rte_pci_read/write_config functions requires device with
a intr_handle type for using VFIO or UIO driver related functions.

Secondary processes rely on primary processes for device initialization
so they do not usually require using these functions. However, some PMDs,
like NFP PMD, require using these functions even for secondary processes.

Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
---
 drivers/bus/pci/linux/pci_vfio.c | 2 ++
 drivers/bus/pci/pci_common_uio.c | 6 ++++++
 2 files changed, 8 insertions(+)

diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
index 686386d..19f5d8e 100644
--- a/drivers/bus/pci/linux/pci_vfio.c
+++ b/drivers/bus/pci/linux/pci_vfio.c
@@ -545,7 +545,9 @@
 
 	struct pci_map *maps;
 
+	/* This is required for using rte_pci_read/write_config */
 	dev->intr_handle.fd = -1;
+	dev->intr_handle.type = RTE_INTR_HANDLE_VFIO_MSIX;
 
 	/* store PCI address string */
 	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
diff --git a/drivers/bus/pci/pci_common_uio.c b/drivers/bus/pci/pci_common_uio.c
index 54bc20b..6dba4e1 100644
--- a/drivers/bus/pci/pci_common_uio.c
+++ b/drivers/bus/pci/pci_common_uio.c
@@ -31,6 +31,12 @@
 	struct mapped_pci_res_list *uio_res_list =
 			RTE_TAILQ_CAST(rte_uio_tailq.head, mapped_pci_res_list);
 
+	/* This is required for using rte_pci_read/write_config */
+	if (dev->kdrv == RTE_KDRV_IGB_UIO)
+		dev->intr_handle.type = RTE_INTR_HANDLE_UIO;
+	else
+		dev->intr_handle.type = RTE_INTR_HANDLE_UIO_INTX;
+
 	TAILQ_FOREACH(uio_res, uio_res_list, next) {
 
 		/* skip this element if it doesn't match our PCI address */
-- 
1.9.1

Re: [dpdk-dev] [PATCH] bus/pci: set intr_handle type for secondary processes

[Burakov, Anatoly]

On 27-Sep-18 1:30 PM, Alejandro Lucero wrote:
> Invoking rte_pci_read/write_config functions requires device with
> a intr_handle type for using VFIO or UIO driver related functions.
> 
> Secondary processes rely on primary processes for device initialization
> so they do not usually require using these functions. However, some PMDs,
> like NFP PMD, require using these functions even for secondary processes.
> 
> Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> ---

Hi Alejandro,

I’m curious of consequences of setting intr handle to a valid value when 
we don’t have an interrupt thread. Something may try to use it (although 
I couldn’t find any such usage).

PCI config read really uses intr handle type to discover userspace 
driver type – this seems ever so slightly wrong, and looks like 
something that should be part of rte_device somewhere, independent of 
interrupt types. Do we have any other alternative to do the same thing 
(i.e. know what userspace driver is used for a particular PCI device)?

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH] bus/pci: set intr_handle type for secondary processes

[Alejandro Lucero]

On Thu, Oct 18, 2018 at 5:26 PM Burakov, Anatoly <anatoly.burakov@intel.com>
wrote:

> On 27-Sep-18 1:30 PM, Alejandro Lucero wrote:
> > Invoking rte_pci_read/write_config functions requires device with
> > a intr_handle type for using VFIO or UIO driver related functions.
> >
> > Secondary processes rely on primary processes for device initialization
> > so they do not usually require using these functions. However, some PMDs,
> > like NFP PMD, require using these functions even for secondary processes.
> >
> > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> > ---
>
> Hi Alejandro,
>
> I’m curious of consequences of setting intr handle to a valid value when
> we don’t have an interrupt thread. Something may try to use it (although
> I couldn’t find any such usage).
>
>
The point is secondary processes do not deal with interrupts so I assume
setting the type does not change anything but it allows to use PCI
read/write functions by secondary processes.


> PCI config read really uses intr handle type to discover userspace
> driver type – this seems ever so slightly wrong, and looks like
> something that should be part of rte_device somewhere, independent of
> interrupt types. Do we have any other alternative to do the same thing
> (i.e. know what userspace driver is used for a particular PCI device)?
>
>
I agree current way not being specially good.

Your comment has reminded me there is another way: just using the kdrv
field from the rte_pci_device struct. I have code using that field for
doing a different thing in the NFP PMD depending on the driver in use, UIO
or VFIO. So I think a better patch would be just to modify those pci
functions for using kdrv field instead.

Adding Ferruh in the thread for commenting on this potential change.



> --
> Thanks,
> Anatoly
>

Re: [dpdk-dev] [PATCH] bus/pci: set intr_handle type for secondary processes

[Burakov, Anatoly]

On 18-Oct-18 5:41 PM, Alejandro Lucero wrote:
> 
> 
> On Thu, Oct 18, 2018 at 5:26 PM Burakov, Anatoly 
> <anatoly.burakov@intel.com <mailto:anatoly.burakov@intel.com>> wrote:
> 
>     On 27-Sep-18 1:30 PM, Alejandro Lucero wrote:
>      > Invoking rte_pci_read/write_config functions requires device with
>      > a intr_handle type for using VFIO or UIO driver related functions.
>      >
>      > Secondary processes rely on primary processes for device
>     initialization
>      > so they do not usually require using these functions. However,
>     some PMDs,
>      > like NFP PMD, require using these functions even for secondary
>     processes.
>      >
>      > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com
>     <mailto:alejandro.lucero@netronome.com>>
>      > ---
> 
>     Hi Alejandro,
> 
>     I’m curious of consequences of setting intr handle to a valid value
>     when
>     we don’t have an interrupt thread. Something may try to use it
>     (although
>     I couldn’t find any such usage).
> 
> 
> The point is secondary processes do not deal with interrupts so I assume 
> setting the type does not change anything but it allows to use PCI 
> read/write functions by secondary processes.
> 
>     PCI config read really uses intr handle type to discover userspace
>     driver type – this seems ever so slightly wrong, and looks like
>     something that should be part of rte_device somewhere, independent of
>     interrupt types. Do we have any other alternative to do the same thing
>     (i.e. know what userspace driver is used for a particular PCI device)?
> 
> 
> I agree current way not being specially good.
> 
> Your comment has reminded me there is another way: just using the kdrv 
> field from the rte_pci_device struct. I have code using that field for 
> doing a different thing in the NFP PMD depending on the driver in use, 
> UIO or VFIO. So I think a better patch would be just to modify those pci 
> functions for using kdrv field instead.
> 
> Adding Ferruh in the thread for commenting on this potential change.
> 

I definitely think the way you describe would be a better way to fix 
this (i.e. use kdrv in PCI config functions rather than intr handle type).

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH] bus/pci: set intr_handle type for secondary processes

[Alejandro Lucero]

On Fri, Oct 19, 2018 at 9:02 AM Burakov, Anatoly <anatoly.burakov@intel.com>
wrote:

> On 18-Oct-18 5:41 PM, Alejandro Lucero wrote:
> >
> >
> > On Thu, Oct 18, 2018 at 5:26 PM Burakov, Anatoly
> > <anatoly.burakov@intel.com <mailto:anatoly.burakov@intel.com>> wrote:
> >
> >     On 27-Sep-18 1:30 PM, Alejandro Lucero wrote:
> >      > Invoking rte_pci_read/write_config functions requires device with
> >      > a intr_handle type for using VFIO or UIO driver related functions.
> >      >
> >      > Secondary processes rely on primary processes for device
> >     initialization
> >      > so they do not usually require using these functions. However,
> >     some PMDs,
> >      > like NFP PMD, require using these functions even for secondary
> >     processes.
> >      >
> >      > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com
> >     <mailto:alejandro.lucero@netronome.com>>
> >      > ---
> >
> >     Hi Alejandro,
> >
> >     I’m curious of consequences of setting intr handle to a valid value
> >     when
> >     we don’t have an interrupt thread. Something may try to use it
> >     (although
> >     I couldn’t find any such usage).
> >
> >
> > The point is secondary processes do not deal with interrupts so I assume
> > setting the type does not change anything but it allows to use PCI
> > read/write functions by secondary processes.
> >
> >     PCI config read really uses intr handle type to discover userspace
> >     driver type – this seems ever so slightly wrong, and looks like
> >     something that should be part of rte_device somewhere, independent of
> >     interrupt types. Do we have any other alternative to do the same
> thing
> >     (i.e. know what userspace driver is used for a particular PCI
> device)?
> >
> >
> > I agree current way not being specially good.
> >
> > Your comment has reminded me there is another way: just using the kdrv
> > field from the rte_pci_device struct. I have code using that field for
> > doing a different thing in the NFP PMD depending on the driver in use,
> > UIO or VFIO. So I think a better patch would be just to modify those pci
> > functions for using kdrv field instead.
> >
> > Adding Ferruh in the thread for commenting on this potential change.
> >
>
> I definitely think the way you describe would be a better way to fix
> this (i.e. use kdrv in PCI config functions rather than intr handle type).
>
>
I will send another patch then.

Thanks!


> --
> Thanks,
> Anatoly
>