Re: [dpdk-dev] [PATCH v2 10/16] Added support for selecting VFIO interrupt type from EAL command-line

Re: [dpdk-dev] [PATCH v2 10/16] Added support for selecting VFIO interrupt type from EAL command-line

[Xu, HuilongX]

Anatoly Burakov anatoly.burakov at intel.com
Mon May 19 17:51:43 CEST 2014
Previous message: [dpdk-dev] [PATCH v2 09/16] Enable VFIO device binding
Next message: [dpdk-dev] [PATCH v2 10/16] Added support for selecting VFIO interrupt type from EAL command-line
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
Unlike igb_uio, VFIO interrupt type is not set by kernel module
parameters but is set up via ioctl() calls at runtime. This warrants
a new EAL command-line parameter. It will have no effect if VFIO is
not compiled, but will set VFIO interrupt type to either "legacy" or
"msix" if VFIO support is compiled. Note that VFIO initialization will
fail if the interrupt type selected is not supported by the system.

Signed-off-by: Anatoly Burakov <anatoly.burakov at intel.com>

Test-by: HuilongX Xu huilongx.xu@intel.com<mailto:huilongx.xu@intel.com>

Compile pass

     >>Compile OS: FC20 x86_64

     >>Kernel version: 3.13.6-200

     >>GCC version: 4.8.2

     >>Server: Crownpass
---
lib/librte_eal/linuxapp/eal/eal.c |   32 ++++++++++++++++++++++++++++++++
1 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index 01bfd6c..bae1078 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -99,6 +99,7 @@
#define OPT_BASE_VIRTADDR   "base-virtaddr"
#define OPT_XEN_DOM0    "xen-dom0"
#define OPT_CREATE_UIO_DEV "create-uio-dev"
+#define OPT_VFIO_INTR    "vfio-intr"
 #define RTE_EAL_BLACKLIST_SIZE          0x100
@@ -361,6 +362,7 @@ eal_usage(const char *prgname)
                      "  --"OPT_VMWARE_TSC_MAP": use VMware TSC map instead of "
                                                  "native RDTSC\n"
                      "  --"OPT_BASE_VIRTADDR": specify base virtual address\n"
+                    "  --"OPT_VFIO_INTR": specify desired interrupt mode for VFIO (intx|msix)\n"
                      "  --"OPT_CREATE_UIO_DEV": create /dev/uioX (usually done by hotplug)\n"
                      "\nEAL options for DEBUG use only:\n"
                      "  --"OPT_NO_HUGE"  : use malloc instead of hugetlbfs\n"
@@ -579,6 +581,27 @@ eal_parse_base_virtaddr(const char *arg)
               return 0;
}
+static int
+eal_parse_vfio_intr(const char *mode)
+{
+             unsigned i;
+             static struct {
+                             const char *name;
+                             enum rte_intr_mode value;
+             } map[] = {
+                             { "legacy", RTE_INTR_MODE_LEGACY },
+                             { "msix", RTE_INTR_MODE_MSIX },
+             };
+
+             for (i = 0; i < RTE_DIM(map); i++) {
+                             if (!strcmp(mode, map[i].name)) {
+                                             internal_config.vfio_intr_mode = map[i].value;
+                                             return 0;
+                             }
+             }
+             return -1;
+}
+
static inline size_t
eal_get_hugepage_mem_size(void)
{
@@ -633,6 +656,7 @@ eal_parse_args(int argc, char **argv)
                               {OPT_PCI_BLACKLIST, 1, 0, 0},
                               {OPT_VDEV, 1, 0, 0},
                               {OPT_SYSLOG, 1, NULL, 0},
+                             {OPT_VFIO_INTR, 1, NULL, 0},
                               {OPT_BASE_VIRTADDR, 1, 0, 0},
                               {OPT_XEN_DOM0, 0, 0, 0},
                               {OPT_CREATE_UIO_DEV, 1, NULL, 0},
@@ -828,6 +852,14 @@ eal_parse_args(int argc, char **argv)
                                                                               return -1;
                                                               }
                                               }
+                                             else if (!strcmp(lgopts[option_index].name, OPT_VFIO_INTR)) {
+                                                             if (eal_parse_vfio_intr(optarg) < 0) {
+                                                                             RTE_LOG(ERR, EAL, "invalid parameters for --"
+                                                                                                             OPT_VFIO_INTR "\n");
+                                                                             eal_usage(prgname);
+                                                                             return -1;
+                                                             }
+                                             }
                                               else if (!strcmp(lgopts[option_index].name, OPT_CREATE_UIO_DEV)) {
                                                               internal_config.create_uio_dev = 1;
                                               }
--
1.7.0.7

[dpdk-dev] [PATCH 00/16] [RFC] [VFIO] Add VFIO support to DPDK

[Burakov, Anatoly]

This patchset adds support for using VFIO instead of IGB_UIO to
map the device BARs.

VFIO is a kernel 3.6+ driver allowing secure DMA from userspace
by means of using IOMMU instead of working directly with physical
memory like igb_uio does.

Short summary:
* Adding support for VFIO in EAL PCI code
* Adding new command-line parameter for VFIO interrupt type
* Adding support for VFIO in setup.sh
* Renaming igb_uio_bind to dpdk_nic_bind and adding support for
  VFIO there
* Removing PCI ID list from igb_uio, effectively making it another
  generic PCI driver similar to pci_stub, vfio-pci et al
* Adding autotest for VFIO interrupt types
* Making igb_uio and VFIO compilation optional

I'm submitting this as an RFC because this patch is based off
current dpdk.org branch with David Marchand's RTE_EAL_UNBIND_PORTS
patchset. IOW, this will *not* apply to the dpdk.org tree *unless* you
also apply David's patches beforehand.

Signed-off by: Anatoly Burakov <anatoly.burakov@intel.com>

Anatoly Burakov (16):
  Separate igb_uio mapping into a separate file
  Distinguish between legitimate failures and non-fatal errors
  Rename RTE_PCI_DRV_NEED_IGB_UIO to RTE_PCI_DRV_NEED_MAPPING
  Make igb_uio compilation optional
  Moved interrupt type out of igb_uio
  Add support for VFIO in Linuxapp targets
  Add support for VFIO interrupts, add VFIO header
  Add support for mapping devices through VFIO.
  Enable VFIO device binding
  Added support for selecting VFIO interrupt type from EAL command-line
  Make --no-huge use mmap instead of malloc.
  Adding unit tests for VFIO EAL command-line parameter
  Removed PCI ID table from igb_uio
  Renamed igb_uio_bind to dpdk_nic_bind
  Added support for VFIO drivers in dpdk_nic_bind.py
  Adding support for VFIO to setup.sh

 app/test/test_eal_flags.c                          |  24 +
 app/test/test_pci.c                                |   4 +-
 config/defconfig_i686-default-linuxapp-gcc         |   2 +
 config/defconfig_i686-default-linuxapp-icc         |   2 +
 config/defconfig_x86_64-default-linuxapp-gcc       |   2 +
 config/defconfig_x86_64-default-linuxapp-icc       |   2 +
 lib/librte_eal/bsdapp/eal/eal_pci.c                |   2 +-
 lib/librte_eal/common/Makefile                     |   1 +
 lib/librte_eal/common/eal_common_pci.c             |  17 +-
 lib/librte_eal/common/include/rte_pci.h            |   7 +-
 .../common/include/rte_pci_dev_feature_defs.h      |  46 ++
 .../common/include/rte_pci_dev_features.h          |  42 ++
 lib/librte_eal/linuxapp/Makefile                   |   2 +
 lib/librte_eal/linuxapp/eal/Makefile               |   6 +-
 lib/librte_eal/linuxapp/eal/eal.c                  |  35 +
 lib/librte_eal/linuxapp/eal/eal_interrupts.c       | 203 +++++-
 lib/librte_eal/linuxapp/eal/eal_memory.c           |   8 +-
 lib/librte_eal/linuxapp/eal/eal_pci.c              | 480 ++------------
 lib/librte_eal/linuxapp/eal/eal_pci_uio.c          | 416 ++++++++++++
 lib/librte_eal/linuxapp/eal/eal_pci_vfio.c         | 709 +++++++++++++++++++++
 lib/librte_eal/linuxapp/eal/eal_pci_vfio_socket.c  | 367 +++++++++++
 .../linuxapp/eal/include/eal_internal_cfg.h        |   3 +
 lib/librte_eal/linuxapp/eal/include/eal_pci_init.h | 120 ++++
 lib/librte_eal/linuxapp/eal/include/eal_vfio.h     |  55 ++
 .../linuxapp/eal/include/exec-env/rte_interrupts.h |   7 +-
 lib/librte_eal/linuxapp/igb_uio/igb_uio.c          |  70 +-
 lib/librte_pmd_e1000/em_ethdev.c                   |   2 +-
 lib/librte_pmd_e1000/igb_ethdev.c                  |   4 +-
 lib/librte_pmd_ixgbe/ixgbe_ethdev.c                |   4 +-
 lib/librte_pmd_vmxnet3/vmxnet3_ethdev.c            |   2 +-
 tools/dpdk_nic_bind.py                             | 500 +++++++++++++++
 tools/igb_uio_bind.py                              | 485 --------------
 tools/setup.sh                                     | 168 ++++-
 33 files changed, 2797 insertions(+), 1000 deletions(-)
 create mode 100644 lib/librte_eal/common/include/rte_pci_dev_feature_defs.h
 create mode 100644 lib/librte_eal/common/include/rte_pci_dev_features.h
 create mode 100644 lib/librte_eal/linuxapp/eal/eal_pci_uio.c
 create mode 100644 lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
 create mode 100644 lib/librte_eal/linuxapp/eal/eal_pci_vfio_socket.c
 create mode 100644 lib/librte_eal/linuxapp/eal/include/eal_pci_init.h
 create mode 100644 lib/librte_eal/linuxapp/eal/include/eal_vfio.h
 create mode 100755 tools/dpdk_nic_bind.py
 delete mode 100755 tools/igb_uio_bind.py

-- 
1.8.1.4

[dpdk-dev] [PATCH v2 10/16] Added support for selecting VFIO interrupt type from EAL command-line

[Anatoly Burakov]

Unlike igb_uio, VFIO interrupt type is not set by kernel module
parameters but is set up via ioctl() calls at runtime. This warrants
a new EAL command-line parameter. It will have no effect if VFIO is
not compiled, but will set VFIO interrupt type to either "legacy" or
"msix" if VFIO support is compiled. Note that VFIO initialization will
fail if the interrupt type selected is not supported by the system.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal.c |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index 01bfd6c..bae1078 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -99,6 +99,7 @@
 #define OPT_BASE_VIRTADDR   "base-virtaddr"
 #define OPT_XEN_DOM0    "xen-dom0"
 #define OPT_CREATE_UIO_DEV "create-uio-dev"
+#define OPT_VFIO_INTR    "vfio-intr"
 
 #define RTE_EAL_BLACKLIST_SIZE	0x100
 
@@ -361,6 +362,7 @@ eal_usage(const char *prgname)
 	       "  --"OPT_VMWARE_TSC_MAP": use VMware TSC map instead of "
 	    		   "native RDTSC\n"
 	       "  --"OPT_BASE_VIRTADDR": specify base virtual address\n"
+	       "  --"OPT_VFIO_INTR": specify desired interrupt mode for VFIO (intx|msix)\n"
 	       "  --"OPT_CREATE_UIO_DEV": create /dev/uioX (usually done by hotplug)\n"
 	       "\nEAL options for DEBUG use only:\n"
 	       "  --"OPT_NO_HUGE"  : use malloc instead of hugetlbfs\n"
@@ -579,6 +581,27 @@ eal_parse_base_virtaddr(const char *arg)
 	return 0;
 }
 
+static int
+eal_parse_vfio_intr(const char *mode)
+{
+	unsigned i;
+	static struct {
+		const char *name;
+		enum rte_intr_mode value;
+	} map[] = {
+		{ "legacy", RTE_INTR_MODE_LEGACY },
+		{ "msix", RTE_INTR_MODE_MSIX },
+	};
+
+	for (i = 0; i < RTE_DIM(map); i++) {
+		if (!strcmp(mode, map[i].name)) {
+			internal_config.vfio_intr_mode = map[i].value;
+			return 0;
+		}
+	}
+	return -1;
+}
+
 static inline size_t
 eal_get_hugepage_mem_size(void)
 {
@@ -633,6 +656,7 @@ eal_parse_args(int argc, char **argv)
 		{OPT_PCI_BLACKLIST, 1, 0, 0},
 		{OPT_VDEV, 1, 0, 0},
 		{OPT_SYSLOG, 1, NULL, 0},
+		{OPT_VFIO_INTR, 1, NULL, 0},
 		{OPT_BASE_VIRTADDR, 1, 0, 0},
 		{OPT_XEN_DOM0, 0, 0, 0},
 		{OPT_CREATE_UIO_DEV, 1, NULL, 0},
@@ -828,6 +852,14 @@ eal_parse_args(int argc, char **argv)
 					return -1;
 				}
 			}
+			else if (!strcmp(lgopts[option_index].name, OPT_VFIO_INTR)) {
+				if (eal_parse_vfio_intr(optarg) < 0) {
+					RTE_LOG(ERR, EAL, "invalid parameters for --"
+							OPT_VFIO_INTR "\n");
+					eal_usage(prgname);
+					return -1;
+				}
+			}
 			else if (!strcmp(lgopts[option_index].name, OPT_CREATE_UIO_DEV)) {
 				internal_config.create_uio_dev = 1;
 			}
-- 
1.7.0.7

Re: [dpdk-dev] [PATCH v2 10/16] Added support for selecting VFIO interrupt type from EAL command-line

[Stephen Hemminger]

I really wish the code did automatic fall back based on PCI config. It is
possible to know the right mode, and do the right thing.
Rather than punting the problem out to command line which is totally
unusable in hot plug and generic application.


On Mon, May 19, 2014 at 8:51 AM, Anatoly Burakov
<anatoly.burakov@intel.com>wrote:

> Unlike igb_uio, VFIO interrupt type is not set by kernel module
> parameters but is set up via ioctl() calls at runtime. This warrants
> a new EAL command-line parameter. It will have no effect if VFIO is
> not compiled, but will set VFIO interrupt type to either "legacy" or
> "msix" if VFIO support is compiled. Note that VFIO initialization will
> fail if the interrupt type selected is not supported by the system.
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>  lib/librte_eal/linuxapp/eal/eal.c |   32 ++++++++++++++++++++++++++++++++
>  1 files changed, 32 insertions(+), 0 deletions(-)
>
> diff --git a/lib/librte_eal/linuxapp/eal/eal.c
> b/lib/librte_eal/linuxapp/eal/eal.c
> index 01bfd6c..bae1078 100644
> --- a/lib/librte_eal/linuxapp/eal/eal.c
> +++ b/lib/librte_eal/linuxapp/eal/eal.c
> @@ -99,6 +99,7 @@
>  #define OPT_BASE_VIRTADDR   "base-virtaddr"
>  #define OPT_XEN_DOM0    "xen-dom0"
>  #define OPT_CREATE_UIO_DEV "create-uio-dev"
> +#define OPT_VFIO_INTR    "vfio-intr"
>
>  #define RTE_EAL_BLACKLIST_SIZE 0x100
>
> @@ -361,6 +362,7 @@ eal_usage(const char *prgname)
>                "  --"OPT_VMWARE_TSC_MAP": use VMware TSC map instead of "
>                            "native RDTSC\n"
>                "  --"OPT_BASE_VIRTADDR": specify base virtual address\n"
> +              "  --"OPT_VFIO_INTR": specify desired interrupt mode for
> VFIO (intx|msix)\n"
>                "  --"OPT_CREATE_UIO_DEV": create /dev/uioX (usually done
> by hotplug)\n"
>                "\nEAL options for DEBUG use only:\n"
>                "  --"OPT_NO_HUGE"  : use malloc instead of hugetlbfs\n"
> @@ -579,6 +581,27 @@ eal_parse_base_virtaddr(const char *arg)
>         return 0;
>  }
>
> +static int
> +eal_parse_vfio_intr(const char *mode)
> +{
> +       unsigned i;
> +       static struct {
> +               const char *name;
> +               enum rte_intr_mode value;
> +       } map[] = {
> +               { "legacy", RTE_INTR_MODE_LEGACY },
> +               { "msix", RTE_INTR_MODE_MSIX },
> +       };
> +
> +       for (i = 0; i < RTE_DIM(map); i++) {
> +               if (!strcmp(mode, map[i].name)) {
> +                       internal_config.vfio_intr_mode = map[i].value;
> +                       return 0;
> +               }
> +       }
> +       return -1;
> +}
> +
>  static inline size_t
>  eal_get_hugepage_mem_size(void)
>  {
> @@ -633,6 +656,7 @@ eal_parse_args(int argc, char **argv)
>                 {OPT_PCI_BLACKLIST, 1, 0, 0},
>                 {OPT_VDEV, 1, 0, 0},
>                 {OPT_SYSLOG, 1, NULL, 0},
> +               {OPT_VFIO_INTR, 1, NULL, 0},
>                 {OPT_BASE_VIRTADDR, 1, 0, 0},
>                 {OPT_XEN_DOM0, 0, 0, 0},
>                 {OPT_CREATE_UIO_DEV, 1, NULL, 0},
> @@ -828,6 +852,14 @@ eal_parse_args(int argc, char **argv)
>                                         return -1;
>                                 }
>                         }
> +                       else if (!strcmp(lgopts[option_index].name,
> OPT_VFIO_INTR)) {
> +                               if (eal_parse_vfio_intr(optarg) < 0) {
> +                                       RTE_LOG(ERR, EAL, "invalid
> parameters for --"
> +                                                       OPT_VFIO_INTR
> "\n");
> +                                       eal_usage(prgname);
> +                                       return -1;
> +                               }
> +                       }
>                         else if (!strcmp(lgopts[option_index].name,
> OPT_CREATE_UIO_DEV)) {
>                                 internal_config.create_uio_dev = 1;
>                         }
> --
> 1.7.0.7
>
>

Re: [dpdk-dev] [PATCH v2 10/16] Added support for selecting VFIO interrupt type from EAL command-line

[Burakov, Anatoly]

Hi Stephen,

> I really wish the code did automatic fall back based on PCI config. It is possible to know the right mode, and do the right thing.
> Rather than punting the problem out to command line which is totally unusable in hot plug and generic application.

You mean we should use whatever is available rather than default to MSI-X if nothing was explicitly specified? That could be done, I guess.

Best regards,
Anatoly Burakov
DPDK SW Engineer

Re: [dpdk-dev] [PATCH v2 10/16] Added support for selecting VFIO interrupt type from EAL command-line

[Stephen Hemminger]

On Tue, 20 May 2014 08:33:43 +0000
"Burakov, Anatoly" <anatoly.burakov@intel.com> wrote:

> Hi Stephen,
> 
> > I really wish the code did automatic fall back based on PCI config. It is possible to know the right mode, and do the right thing.
> > Rather than punting the problem out to command line which is totally unusable in hot plug and generic application.
> 
> You mean we should use whatever is available rather than default to MSI-X if nothing was explicitly specified? That could be done, I guess.
> 
> Best regards,
> Anatoly Burakov
> DPDK SW Engineer
> 
> 

I am not sure that MSI-X has any advantage with only one IRQ, so MSI would do.
Then have the code look at the PCI capability of device and fallback to INTX
if needed. It should also check if INTX works, see kernel for details, since
some PCI emulation like VMware is broken.

Re: [dpdk-dev] [PATCH v2 10/16] Added support for selecting VFIO interrupt type from EAL command-line

[Burakov, Anatoly]

Hi Stephen,

> I am not sure that MSI-X has any advantage with only one IRQ, so MSI would
> do.

Igb_uio doesn't support MSI, so I never included MSI support. It can be added though, but I don't see much point.

> Then have the code look at the PCI capability of device and fallback to INTX if
> needed. It should also check if INTX works, see kernel for details, since some
> PCI emulation like VMware is broken.

I believe VFIO itself does that already when it sets up interrupts.

Best regards,
Anatoly Burakov
DPDK SW Engineer

Re: [dpdk-dev] [PATCH v2 10/16] Added support for selecting VFIO interrupt type from EAL command-line

[Stephen Hemminger]

Originally igb_uio had code for MSI, but it was broken.
See my patches which fixed that and several other bugs.


On Tue, May 20, 2014 at 8:26 PM, Burakov, Anatoly <anatoly.burakov@intel.com
> wrote:

> Hi Stephen,
>
> > I am not sure that MSI-X has any advantage with only one IRQ, so MSI
> would
> > do.
>
> Igb_uio doesn't support MSI, so I never included MSI support. It can be
> added though, but I don't see much point.
>
> > Then have the code look at the PCI capability of device and fallback to
> INTX if
> > needed. It should also check if INTX works, see kernel for details,
> since some
> > PCI emulation like VMware is broken.
>
> I believe VFIO itself does that already when it sets up interrupts.
>
> Best regards,
> Anatoly Burakov
> DPDK SW Engineer
>
>
>
>

Re: [dpdk-dev] [PATCH v2 10/16] Added support for selecting VFIO interrupt type from EAL command-line

[Thomas Monjalon]

2014-05-19 16:51, Anatoly Burakov:
> Unlike igb_uio, VFIO interrupt type is not set by kernel module
> parameters but is set up via ioctl() calls at runtime. This warrants
> a new EAL command-line parameter. It will have no effect if VFIO is
> not compiled, but will set VFIO interrupt type to either "legacy" or
> "msix" if VFIO support is compiled. Note that VFIO initialization will
> fail if the interrupt type selected is not supported by the system.
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>

> +			}
>  			else if (!strcmp(lgopts[option_index].name, OPT_CREATE_UIO_DEV)) 

another code style issue reported by checkpatch.pl ;)

But it should be fixed by removing this code as Stephen suggests.

-- 
Thomas

Re: [dpdk-dev] [PATCH v2 10/16] Added support for selecting VFIO interrupt type from EAL command-line

[Burakov, Anatoly]

Hi Thomas,

> > +			}
> >  			else if (!strcmp(lgopts[option_index].name,
> OPT_CREATE_UIO_DEV))
> 
> another code style issue reported by checkpatch.pl ;)
> 
> But it should be fixed by removing this code as Stephen suggests.

I'm not sure this could should be removed. Igb_uio allows to pick interrupt mode, so why not VFIO? I've modified my code to try all interrupt modes if nothing was explicitly specified, but why should that preclude the user from selecting a specific interrupt type if he so desires?

As for the style error - the whole chunk of code uses the same style there, so either we fix all of that (in a separate patch?), or leave it as it is.

Best regards,
Anatoly Burakov
DPDK SW Engineer

Re: [dpdk-dev] [PATCH v2 10/16] Added support for selecting VFIO interrupt type from EAL command-line

[Thomas Monjalon]

2014-05-28 10:35, Burakov, Anatoly:
> Hi Thomas,
> 
> > > +			}
> > > 
> > >  			else if (!strcmp(lgopts[option_index].name,
> > 
> > OPT_CREATE_UIO_DEV))
> > 
> > another code style issue reported by checkpatch.pl ;)
> > 
> > But it should be fixed by removing this code as Stephen suggests.
> 
> I'm not sure this could should be removed. Igb_uio allows to pick interrupt
> mode, so why not VFIO? I've modified my code to try all interrupt modes if
> nothing was explicitly specified, but why should that preclude the user
> from selecting a specific interrupt type if he so desires?
> 
> As for the style error - the whole chunk of code uses the same style there,
> so either we fix all of that (in a separate patch?), or leave it as it is.

OK to leave it as is.

But please, let's try to keep a clean code style.
About existing code style issues, separated patches for cleaning should be 
well accepted.

Thanks
-- 
Thomas