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

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

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

[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

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

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 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]

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

[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