[dpdk-dev] [PATCH] VFIO: Avoid to enable vfio while the module not loaded

[dpdk-dev] [PATCH] VFIO: Avoid to enable vfio while the module not loaded

[Michael Qiu]

When vfio module is not loaded when kernel support vfio feature,
the routine still try to open the container to get file
description.

This action is not safe, and of cause got error messages:

EAL: Detected 40 lcore(s)
EAL:   unsupported IOMMU type!
EAL: VFIO support could not be initialized
EAL: Setting up memory...

This may make user confuse, this patch make it reasonable
and much more soomth to user.

Signed-off-by: Michael Qiu <michael.qiu@intel.com>
---
 lib/librte_eal/common/include/rte_common.h | 37 ++++++++++++++++++++++++++++++
 lib/librte_eal/linuxapp/eal/eal_pci_vfio.c | 23 +++++++++++++------
 2 files changed, 53 insertions(+), 7 deletions(-)

diff --git a/lib/librte_eal/common/include/rte_common.h b/lib/librte_eal/common/include/rte_common.h
index 921b91f..333aa6b 100644
--- a/lib/librte_eal/common/include/rte_common.h
+++ b/lib/librte_eal/common/include/rte_common.h
@@ -50,6 +50,8 @@ extern "C" {
 #include <ctype.h>
 #include <errno.h>
 #include <limits.h>
+#include <string.h>
+#include <stdio.h>
 
 /*********** Macros to eliminate unused variable warnings ********/
 
@@ -382,6 +384,41 @@ rte_exit(int exit_code, const char *format, ...)
 	__attribute__((noreturn))
 	__attribute__((format(printf, 2, 3)));
 
+/**
+ * Function is to check if the kernel module(like, vfio, vfio_iommu_type1,
+ * etc.) loaded.
+ *
+ * @param module_name
+ *	The module's name which need to be checked
+ *
+ * @return
+ * 	-1 means some error happens(NULL pointer or open failure)
+ * 	0  means the module not loaded
+ * 	1  means the module loaded
+ */
+static inline int
+check_module(const char *module_name)
+{
+	char mod_name[30]; /* Any module names can be longer than 30 bytes? */
+	int ret = 0;
+
+	if (NULL == module_name)
+		return -1;
+	FILE * fd = fopen("/proc/modules", "r");
+	if( fd == NULL)
+		return -1;
+	while(!feof(fd)) {
+		fscanf(fd, "%s %*[^\n]", mod_name);
+		if(!strcmp(mod_name, module_name)) {
+			ret = 1;
+			break;
+		}
+	}
+	fclose(fd);
+
+	return ret;
+}
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
index c1246e8..a11cc4b 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
@@ -44,6 +44,7 @@
 #include <rte_tailq.h>
 #include <rte_eal_memconfig.h>
 #include <rte_malloc.h>
+#include <rte_common.h>
 
 #include "eal_filesystem.h"
 #include "eal_pci_init.h"
@@ -342,7 +343,8 @@ pci_vfio_get_container_fd(void)
 				RTE_LOG(ERR, EAL, "  could not get IOMMU type, "
 						"error %i (%s)\n", errno, strerror(errno));
 			else
-				RTE_LOG(ERR, EAL, "  unsupported IOMMU type!\n");
+				RTE_LOG(ERR, EAL, "  unsupported IOMMU type! "
+					"expect: 1, actual: %d\n", ret);
 			close(vfio_container_fd);
 			return -1;
 		}
@@ -788,13 +790,20 @@ pci_vfio_enable(void)
 		vfio_cfg.vfio_groups[i].fd = -1;
 		vfio_cfg.vfio_groups[i].group_no = -1;
 	}
-	vfio_cfg.vfio_container_fd = pci_vfio_get_container_fd();
 
-	/* check if we have VFIO driver enabled */
-	if (vfio_cfg.vfio_container_fd != -1)
-		vfio_cfg.vfio_enabled = 1;
-	else
-		RTE_LOG(INFO, EAL, "VFIO support could not be initialized\n");
+	if (check_module("vfio") == 1 &&
+	    check_module("vfio_iommu_type1") == 1) {
+		vfio_cfg.vfio_container_fd = pci_vfio_get_container_fd();
+
+		/* check if we have VFIO driver enabled */
+		if (vfio_cfg.vfio_container_fd != -1)
+			vfio_cfg.vfio_enabled = 1;
+		else
+			RTE_LOG(INFO, EAL, "VFIO support could not be"
+				" initialized\n");
+	} else
+		RTE_LOG(INFO, EAL, "VFIO modules are not all loaded,"
+			" skip VFIO support ...\n");
 
 	return 0;
 }
-- 
1.9.3

[dpdk-dev] [PATCH v2] VFIO: Avoid to enable vfio while the module not loaded

[Michael Qiu]

When vfio module is not loaded when kernel support vfio feature,
the routine still try to open the container to get file
description.

This action is not safe, and of cause got error messages:

EAL: Detected 40 lcore(s)
EAL:   unsupported IOMMU type!
EAL: VFIO support could not be initialized
EAL: Setting up memory...

This may make user confuse, this patch make it reasonable
and much more soomth to user.

Signed-off-by: Michael Qiu <michael.qiu@intel.com>

---
 v2 --> v1:
	1. Move check_module() from rte_common.h to eal_private.h
	   and rename to rte_eal_check_module().
	   To make it linuxapp only.
	2. Some code clean up.

Tested in host and guest, the below is results in VM:

1. VM | without patch | os: 3.11.10-301.fc20.x86_64 | VFIO not loaded

EAL: Support maximum 64 logical core(s) by configuration.
EAL: Detected 2 lcore(s)
EAL:   cannot open VFIO container, error 2 (No such file or directory)
EAL: VFIO support could not be initialized
EAL: Setting up memory...

2. VM | without patch | os: 3.11.10-301.fc20.x86_64 | VFIO loaded vfio_iommu_type1 not loaded
modprobe vfio

lsmod | grep "vfio"
vfio                   19626  0

modprobe vfio_iommu_type1
modprobe: ERROR: could not insert 'vfio_iommu_type1': No such device

ls /dev/vfio/vfio
/dev/vfio/vfio

EAL: Support maximum 64 logical core(s) by configuration.
EAL: Detected 2 lcore(s)
EAL:   unsupported IOMMU type!
EAL: VFIO support could not be initialized
EAL: Setting up memory...

3. VM | with patch | os: 3.11.10-301.fc20.x86_64 | VFIO(vfio_iommu_type1 never can be loaded inside VM)

EAL: Detected 2 lcore(s)
EAL: VFIO modules not all loaded, skip VFIO support ...
EAL: Setting up memory...

 lib/librte_eal/common/eal_private.h        | 36 ++++++++++++++++++++++++++++++
 lib/librte_eal/linuxapp/eal/eal_pci_vfio.c | 24 +++++++++++++++++---
 2 files changed, 57 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
index 232fcec..e741bdb 100644
--- a/lib/librte_eal/common/eal_private.h
+++ b/lib/librte_eal/common/eal_private.h
@@ -35,6 +35,7 @@
 #define _EAL_PRIVATE_H_
 
 #include <stdio.h>
+#include <string.h>
 
 /**
  * Initialize the memzone subsystem (private to eal).
@@ -203,4 +204,39 @@ int rte_eal_alarm_init(void);
  */
 int rte_eal_dev_init(void);
 
+/**
+ * Function is to check if the kernel module(like, vfio, vfio_iommu_type1,
+ * etc.) loaded.
+ *
+ * @param module_name
+ *	The module's name which need to be checked
+ *
+ * @return
+ * 	-1 means error happens(NULL pointer or open failure)
+ * 	 0 means the module not loaded
+ * 	 1 means the module loaded
+ */
+static inline int
+rte_eal_check_module(const char *module_name)
+{
+	char mod_name[30]; /* Any module names can be longer than 30 bytes? */
+	int ret = 0;
+
+	if (NULL == module_name)
+		return -1;
+	FILE * fd = fopen("/proc/modules", "r");
+	if (fd == NULL)
+		return -1;
+	while(!feof(fd)) {
+		fscanf(fd, "%s %*[^\n]", mod_name);
+		if(!strcmp(mod_name, module_name)) {
+			ret = 1;
+			break;
+		}
+	}
+	fclose(fd);
+
+	return ret;
+}
+
 #endif /* _EAL_PRIVATE_H_ */
diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
index c1246e8..52ab2d0 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
@@ -44,6 +44,7 @@
 #include <rte_tailq.h>
 #include <rte_eal_memconfig.h>
 #include <rte_malloc.h>
+#include <eal_private.h>
 
 #include "eal_filesystem.h"
 #include "eal_pci_init.h"
@@ -339,10 +340,13 @@ pci_vfio_get_container_fd(void)
 		ret = ioctl(vfio_container_fd, VFIO_CHECK_EXTENSION, VFIO_TYPE1_IOMMU);
 		if (ret != 1) {
 			if (ret < 0)
-				RTE_LOG(ERR, EAL, "  could not get IOMMU type, "
-						"error %i (%s)\n", errno, strerror(errno));
+				RTE_LOG(ERR, EAL, "  could not get IOMMU type,"
+					" error %i (%s)\n", errno,
+					strerror(errno));
 			else
-				RTE_LOG(ERR, EAL, "  unsupported IOMMU type!\n");
+				RTE_LOG(ERR, EAL, "  unsupported IOMMU type! "
+					"expect: VFIO_TYPE1_IOMMU, "
+					"actual: %d\n", ret);
 			close(vfio_container_fd);
 			return -1;
 		}
@@ -788,6 +792,20 @@ pci_vfio_enable(void)
 		vfio_cfg.vfio_groups[i].fd = -1;
 		vfio_cfg.vfio_groups[i].group_no = -1;
 	}
+
+	/* return error directly */
+	if (rte_eal_check_module("vfio") == -1 ||
+	    rte_eal_check_module("vfio_iommu_type1") == -1)
+		return -1;
+
+	/* return 0 if not all VFIO modules loaded */
+	if (rte_eal_check_module("vfio") == 0 ||
+	    rte_eal_check_module("vfio_iommu_type1") == 0) {
+		RTE_LOG(INFO, EAL, "VFIO modules not all loaded,"
+			" skip VFIO support ...\n");
+		return 0;
+	}
+
 	vfio_cfg.vfio_container_fd = pci_vfio_get_container_fd();
 
 	/* check if we have VFIO driver enabled */
-- 
1.9.3

Re: [dpdk-dev] [PATCH v2] VFIO: Avoid to enable vfio while the module not loaded

[Burakov, Anatoly]

Hi Michael,

Few nitpicks :-) (wording of the log message I guess is up to Thomas, I won't comment on that)

>  lib/librte_eal/common/eal_private.h        | 36
> ++++++++++++++++++++++++++++++
>  lib/librte_eal/linuxapp/eal/eal_pci_vfio.c | 24 +++++++++++++++++---
>  2 files changed, 57 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_private.h
> b/lib/librte_eal/common/eal_private.h
> index 232fcec..e741bdb 100644
> --- a/lib/librte_eal/common/eal_private.h
> +++ b/lib/librte_eal/common/eal_private.h
> @@ -35,6 +35,7 @@
>  #define _EAL_PRIVATE_H_
> 
>  #include <stdio.h>
> +#include <string.h>
> 
>  /**
>   * Initialize the memzone subsystem (private to eal).
> @@ -203,4 +204,39 @@ int rte_eal_alarm_init(void);
>   */
>  int rte_eal_dev_init(void);
> 
> +/**
> + * Function is to check if the kernel module(like, vfio,
> +vfio_iommu_type1,
> + * etc.) loaded.
> + *
> + * @param module_name
> + *	The module's name which need to be checked
> + *
> + * @return
> + * 	-1 means error happens(NULL pointer or open failure)
> + * 	 0 means the module not loaded
> + * 	 1 means the module loaded
> + */
> +static inline int
> +rte_eal_check_module(const char *module_name) {
> +	char mod_name[30]; /* Any module names can be longer than 30
> bytes? */
> +	int ret = 0;
> +
> +	if (NULL == module_name)
> +		return -1;
> +	FILE * fd = fopen("/proc/modules", "r");
> +	if (fd == NULL)
> +		return -1;

Can we add RTE_LOG statement here, with an strerror(errno) like in other places? Fopen failed, we should at least know why :-)

> +	while(!feof(fd)) {
> +		fscanf(fd, "%s %*[^\n]", mod_name);
> +		if(!strcmp(mod_name, module_name)) {

Probably should use strncmp instead of strcmp.

> +			ret = 1;
> +			break;
> +		}
> +	}
> +	fclose(fd);
> +
> +	return ret;
> +}
> +
>  #endif /* _EAL_PRIVATE_H_ */
> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> index c1246e8..52ab2d0 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> @@ -44,6 +44,7 @@
>  #include <rte_tailq.h>
>  #include <rte_eal_memconfig.h>
>  #include <rte_malloc.h>
> +#include <eal_private.h>
> 
>  #include "eal_filesystem.h"
>  #include "eal_pci_init.h"
> @@ -339,10 +340,13 @@ pci_vfio_get_container_fd(void)
>  		ret = ioctl(vfio_container_fd, VFIO_CHECK_EXTENSION,
> VFIO_TYPE1_IOMMU);
>  		if (ret != 1) {
>  			if (ret < 0)
> -				RTE_LOG(ERR, EAL, "  could not get IOMMU
> type, "
> -						"error %i (%s)\n", errno,
> strerror(errno));
> +				RTE_LOG(ERR, EAL, "  could not get IOMMU
> type,"
> +					" error %i (%s)\n", errno,
> +					strerror(errno));
>  			else
> -				RTE_LOG(ERR, EAL, "  unsupported IOMMU
> type!\n");
> +				RTE_LOG(ERR, EAL, "  unsupported IOMMU
> type! "
> +					"expect: VFIO_TYPE1_IOMMU, "
> +					"actual: %d\n", ret);

I'm not even sure we need this "expected" bit at all. We don't get back the IOMMU type VFIO currently supports; rather, this code checks if VFIO's support for VFIO_TYPE1_IOMMU is enabled or not. So I would change the error message to something more descriptive, such as " required IOMMU type support not present in VFIO!\n", and get rid of the "expected".

>  			close(vfio_container_fd);
>  			return -1;
>  		}
> @@ -788,6 +792,20 @@ pci_vfio_enable(void)
>  		vfio_cfg.vfio_groups[i].fd = -1;
>  		vfio_cfg.vfio_groups[i].group_no = -1;
>  	}
> +
> +	/* return error directly */
> +	if (rte_eal_check_module("vfio") == -1 ||
> +	    rte_eal_check_module("vfio_iommu_type1") == -1)
> +		return -1;
> +
> +	/* return 0 if not all VFIO modules loaded */
> +	if (rte_eal_check_module("vfio") == 0 ||
> +	    rte_eal_check_module("vfio_iommu_type1") == 0) {
> +		RTE_LOG(INFO, EAL, "VFIO modules not all loaded,"
> +			" skip VFIO support ...\n");
> +		return 0;
> +	}

Can we perhaps make one call per module instead of two? i.e. something like:

int vfio_ret, vfio_ type1_ret;
vfio_ret = rte_eal_check_module("vfio");
vfio_type1_ret = rte_eal_check_module("vfio_iommu_type1");

if (vfio_ret == -1 || vfio_type1_ret == -1)
    return -1;
else if (vfio_ret == 0 || vfio_type1_ret == 0) {
     ....
    return 0;
}

> +
>  	vfio_cfg.vfio_container_fd = pci_vfio_get_container_fd();
> 
>  	/* check if we have VFIO driver enabled */
> --
> 1.9.3

Re: [dpdk-dev] [PATCH v2] VFIO: Avoid to enable vfio while the module not loaded

[Qiu, Michael]

On 12/5/2014 6:00 PM, Burakov, Anatoly wrote:
> Hi Michael,
>
> Few nitpicks :-) (wording of the log message I guess is up to Thomas, I won't comment on that)
>
>>  lib/librte_eal/common/eal_private.h        | 36
>> ++++++++++++++++++++++++++++++
>>  lib/librte_eal/linuxapp/eal/eal_pci_vfio.c | 24 +++++++++++++++++---
>>  2 files changed, 57 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/librte_eal/common/eal_private.h
>> b/lib/librte_eal/common/eal_private.h
>> index 232fcec..e741bdb 100644
>> --- a/lib/librte_eal/common/eal_private.h
>> +++ b/lib/librte_eal/common/eal_private.h
>> @@ -35,6 +35,7 @@
>>  #define _EAL_PRIVATE_H_
>>
>>  #include <stdio.h>
>> +#include <string.h>
>>
>>  /**
>>   * Initialize the memzone subsystem (private to eal).
>> @@ -203,4 +204,39 @@ int rte_eal_alarm_init(void);
>>   */
>>  int rte_eal_dev_init(void);
>>
>> +/**
>> + * Function is to check if the kernel module(like, vfio,
>> +vfio_iommu_type1,
>> + * etc.) loaded.
>> + *
>> + * @param module_name
>> + *	The module's name which need to be checked
>> + *
>> + * @return
>> + * 	-1 means error happens(NULL pointer or open failure)
>> + * 	 0 means the module not loaded
>> + * 	 1 means the module loaded
>> + */
>> +static inline int
>> +rte_eal_check_module(const char *module_name) {
>> +	char mod_name[30]; /* Any module names can be longer than 30
>> bytes? */
>> +	int ret = 0;
>> +
>> +	if (NULL == module_name)
>> +		return -1;
>> +	FILE * fd = fopen("/proc/modules", "r");
>> +	if (fd == NULL)
>> +		return -1;
> Can we add RTE_LOG statement here, with an strerror(errno) like in other places? Fopen failed, we should at least know why :-)
>
>> +	while(!feof(fd)) {
>> +		fscanf(fd, "%s %*[^\n]", mod_name);
>> +		if(!strcmp(mod_name, module_name)) {
> Probably should use strncmp instead of strcmp.

I don't think so, if we check module "vfio", but if given  module name
is "vfio_xx", it will also correct if use strncmp.

Thanks,
Michael
>
>> +			ret = 1;
>> +			break;
>> +		}
>> +	}
>> +	fclose(fd);
>> +
>> +	return ret;
>> +}
>> +
>>  #endif /* _EAL_PRIVATE_H_ */
>> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
>> b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
>> index c1246e8..52ab2d0 100644
>> --- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
>> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
>> @@ -44,6 +44,7 @@
>>  #include <rte_tailq.h>
>>  #include <rte_eal_memconfig.h>
>>  #include <rte_malloc.h>
>> +#include <eal_private.h>
>>
>>  #include "eal_filesystem.h"
>>  #include "eal_pci_init.h"
>> @@ -339,10 +340,13 @@ pci_vfio_get_container_fd(void)
>>  		ret = ioctl(vfio_container_fd, VFIO_CHECK_EXTENSION,
>> VFIO_TYPE1_IOMMU);
>>  		if (ret != 1) {
>>  			if (ret < 0)
>> -				RTE_LOG(ERR, EAL, "  could not get IOMMU
>> type, "
>> -						"error %i (%s)\n", errno,
>> strerror(errno));
>> +				RTE_LOG(ERR, EAL, "  could not get IOMMU
>> type,"
>> +					" error %i (%s)\n", errno,
>> +					strerror(errno));
>>  			else
>> -				RTE_LOG(ERR, EAL, "  unsupported IOMMU
>> type!\n");
>> +				RTE_LOG(ERR, EAL, "  unsupported IOMMU
>> type! "
>> +					"expect: VFIO_TYPE1_IOMMU, "
>> +					"actual: %d\n", ret);
> I'm not even sure we need this "expected" bit at all. We don't get back the IOMMU type VFIO currently supports; rather, this code checks if VFIO's support for VFIO_TYPE1_IOMMU is enabled or not. So I would change the error message to something more descriptive, such as " required IOMMU type support not present in VFIO!\n", and get rid of the "expected".
>
>>  			close(vfio_container_fd);
>>  			return -1;
>>  		}
>> @@ -788,6 +792,20 @@ pci_vfio_enable(void)
>>  		vfio_cfg.vfio_groups[i].fd = -1;
>>  		vfio_cfg.vfio_groups[i].group_no = -1;
>>  	}
>> +
>> +	/* return error directly */
>> +	if (rte_eal_check_module("vfio") == -1 ||
>> +	    rte_eal_check_module("vfio_iommu_type1") == -1)
>> +		return -1;
>> +
>> +	/* return 0 if not all VFIO modules loaded */
>> +	if (rte_eal_check_module("vfio") == 0 ||
>> +	    rte_eal_check_module("vfio_iommu_type1") == 0) {
>> +		RTE_LOG(INFO, EAL, "VFIO modules not all loaded,"
>> +			" skip VFIO support ...\n");
>> +		return 0;
>> +	}
> Can we perhaps make one call per module instead of two? i.e. something like:
>
> int vfio_ret, vfio_ type1_ret;
> vfio_ret = rte_eal_check_module("vfio");
> vfio_type1_ret = rte_eal_check_module("vfio_iommu_type1");
>
> if (vfio_ret == -1 || vfio_type1_ret == -1)
>     return -1;
> else if (vfio_ret == 0 || vfio_type1_ret == 0) {
>      ....
>     return 0;
> }
>
>> +
>>  	vfio_cfg.vfio_container_fd = pci_vfio_get_container_fd();
>>
>>  	/* check if we have VFIO driver enabled */
>> --
>> 1.9.3
>

Re: [dpdk-dev] [PATCH v2] VFIO: Avoid to enable vfio while the module not loaded

[Burakov, Anatoly]

Hi Michael

> I don't think so, if we check module "vfio", but if given  module name is
> "vfio_xx", it will also correct if use strncmp.

Sorry I missed this the last time. I don't think that is the case. If you do strncmp on sizeof(buffer), strncmp will always check 30 bytes. That way if you check vfio against vfio_xx, you'll get a mismatch. Of course, replacing fscanf with fgets would be better too, to make sure we never go over our buffer size when dealing with /proc/modules. 

Thanks,
Anatoly 

Re: [dpdk-dev] [PATCH v2] VFIO: Avoid to enable vfio while the module not loaded

[Qiu, Michael]

On 12/9/2014 2:47 AM, Burakov, Anatoly wrote:
> Hi Michael
>
>> I don't think so, if we check module "vfio", but if given  module name is
>> "vfio_xx", it will also correct if use strncmp.
> Sorry I missed this the last time. I don't think that is the case. If you do strncmp on sizeof(buffer), strncmp will always check 30 bytes. That way if you check vfio against vfio_xx, you'll get a mismatch. Of course, replacing 

Yes, you are right, strncmp() will check 30 bytes if use sizeof(buffer).

But any issue of strcmp() ? This rountin cares about exactly match. I
think no need to convert to strncmp() if it does have other issue.

> fscanf with fgets would be better too, to make sure we never go over our buffer size when dealing with /proc/modules. 

If we use fgets, we need additional efforts to get the modname,  for
potential overflow issue, we can limit counts of fscanf(). like below:

fscanf(fd, "%30s %*[^\n]", mod_name);

Thanks,
Michael

>
> Thanks,
> Anatoly 
>

Re: [dpdk-dev] [PATCH v2] VFIO: Avoid to enable vfio while the module not loaded

[Burakov, Anatoly]

Hi Michael,

> 
> Yes, you are right, strncmp() will check 30 bytes if use sizeof(buffer).
> 
> But any issue of strcmp() ? This rountin cares about exactly match. I think no
> need to convert to strncmp() if it does have other issue.
> 
> > fscanf with fgets would be better too, to make sure we never go over our
> buffer size when dealing with /proc/modules.
> 
> If we use fgets, we need additional efforts to get the modname,  for
> potential overflow issue, we can limit counts of fscanf(). like below:
> 
> fscanf(fd, "%30s %*[^\n]", mod_name);
> 

As  long as it doesn't cause easy buffer overruns, I'm fine with it :-)

Thanks,
Anatoly

[dpdk-dev] [PATCH v3] VFIO: Avoid to enable vfio while the module not loaded

[Michael Qiu]

When vfio module is not loaded when kernel support vfio feature,
the routine still try to open the container to get file
description.

This action is not safe, and of cause got error messages:

EAL: Detected 40 lcore(s)
EAL:   unsupported IOMMU type!
EAL: VFIO support could not be initialized
EAL: Setting up memory...

This may make user confuse, this patch make it reasonable
and much more soomth to user.

Signed-off-by: Michael Qiu <michael.qiu@intel.com>
---
v3 --> v2:
	1. Add error log in rte_eal_check_module()
	2. Some code clean up.

v2 --> v1:
	1. Move check_module() from rte_common.h to eal_private.h
	   and rename to rte_eal_check_module().
	   To make it linuxapp only.
	2. Some code clean up.

 lib/librte_eal/common/eal_private.h        | 43 ++++++++++++++++++++++++++++++
 lib/librte_eal/linuxapp/eal/eal_pci_vfio.c | 30 ++++++++++++++++++---
 2 files changed, 70 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
index 232fcec..d1d8126 100644
--- a/lib/librte_eal/common/eal_private.h
+++ b/lib/librte_eal/common/eal_private.h
@@ -35,6 +35,9 @@
 #define _EAL_PRIVATE_H_
 
 #include <stdio.h>
+#include <string.h>
+#include <rte_log.h>
+#include <errno.h>
 
 /**
  * Initialize the memzone subsystem (private to eal).
@@ -203,4 +206,44 @@ int rte_eal_alarm_init(void);
  */
 int rte_eal_dev_init(void);
 
+/**
+ * Function is to check if the kernel module(like, vfio, vfio_iommu_type1,
+ * etc.) loaded.
+ *
+ * @param module_name
+ *	The module's name which need to be checked
+ *
+ * @return
+ * 	-1 means some error happens(NULL pointer or open failure)
+ * 	0  means the module not loaded
+ * 	1  means the module loaded
+ */
+static inline int
+rte_eal_check_module(const char *module_name)
+{
+	char mod_name[30]; /* Any module names can be longer than 30 bytes? */
+	int ret = 0;
+
+	if (NULL == module_name) {
+		RTE_LOG(ERR, EAL, "The module name is NULL\n");
+		return -1;
+	}
+	FILE * fd = fopen("/proc/modules", "r");
+	if (NULL == fd) {
+		RTE_LOG(ERR, EAL, "Open /proc/modules failed!"
+			" error %i (%s)\n", errno, strerror(errno));
+		return -1;
+	}
+	while(!feof(fd)) {
+		fscanf(fd, "%s %*[^\n]", mod_name);
+		if(!strcmp(mod_name, module_name)) {
+			ret = 1;
+			break;
+		}
+	}
+	fclose(fd);
+
+	return ret;
+}
+
 #endif /* _EAL_PRIVATE_H_ */
diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
index c1246e8..b34b3f5 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
@@ -44,6 +44,7 @@
 #include <rte_tailq.h>
 #include <rte_eal_memconfig.h>
 #include <rte_malloc.h>
+#include <eal_private.h>
 
 #include "eal_filesystem.h"
 #include "eal_pci_init.h"
@@ -339,10 +340,15 @@ pci_vfio_get_container_fd(void)
 		ret = ioctl(vfio_container_fd, VFIO_CHECK_EXTENSION, VFIO_TYPE1_IOMMU);
 		if (ret != 1) {
 			if (ret < 0)
-				RTE_LOG(ERR, EAL, "  could not get IOMMU type, "
-						"error %i (%s)\n", errno, strerror(errno));
+				RTE_LOG(ERR, EAL, "  could not get IOMMU type,"
+					" error %i (%s)\n", errno,
+					strerror(errno));
 			else
-				RTE_LOG(ERR, EAL, "  unsupported IOMMU type!\n");
+				/* Better to show the IOMMU type return from
+				 * kernel for easy debug
+				 */
+				RTE_LOG(ERR, EAL, "  unsupported IOMMU type"
+					" detected: %d in VFIO\n", ret);
 			close(vfio_container_fd);
 			return -1;
 		}
@@ -783,11 +789,29 @@ pci_vfio_enable(void)
 {
 	/* initialize group list */
 	int i;
+	int module_vfio, module_vfio_type1;
 
 	for (i = 0; i < VFIO_MAX_GROUPS; i++) {
 		vfio_cfg.vfio_groups[i].fd = -1;
 		vfio_cfg.vfio_groups[i].group_no = -1;
 	}
+
+	module_vfio = rte_eal_check_module("vfio");
+	module_vfio_type1 = rte_eal_check_module("vfio_iommu_type1");
+
+	/* return error directly */
+	if (module_vfio == -1 || module_vfio_type1 == -1) {
+		RTE_LOG(INFO, EAL, "Could not get loaded module details!\n");
+		return -1;
+	}
+
+	/* return 0 if not all VFIO modules loaded */
+	if (module_vfio == 0 || module_vfio_type1 == 0) {
+		RTE_LOG(INFO, EAL, "VFIO modules not all loaded,"
+			" skip VFIO support ...\n");
+		return 0;
+	}
+
 	vfio_cfg.vfio_container_fd = pci_vfio_get_container_fd();
 
 	/* check if we have VFIO driver enabled */
-- 
1.9.3

Re: [dpdk-dev] [PATCH v3] VFIO: Avoid to enable vfio while the module not loaded

[Burakov, Anatoly]

> When vfio module is not loaded when kernel support vfio feature, the
> routine still try to open the container to get file description.
> 
> This action is not safe, and of cause got error messages:
> 
> EAL: Detected 40 lcore(s)
> EAL:   unsupported IOMMU type!
> EAL: VFIO support could not be initialized
> EAL: Setting up memory...
> 
> This may make user confuse, this patch make it reasonable and much more
> soomth to user.
> 
> Signed-off-by: Michael Qiu <michael.qiu@intel.com>
> ---
> v3 --> v2:
> 	1. Add error log in rte_eal_check_module()
> 	2. Some code clean up.
> 
> v2 --> v1:
> 	1. Move check_module() from rte_common.h to eal_private.h
> 	   and rename to rte_eal_check_module().
> 	   To make it linuxapp only.
> 	2. Some code clean up.
> 
>  lib/librte_eal/common/eal_private.h        | 43
> ++++++++++++++++++++++++++++++
>  lib/librte_eal/linuxapp/eal/eal_pci_vfio.c | 30 ++++++++++++++++++---
>  2 files changed, 70 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_private.h
> b/lib/librte_eal/common/eal_private.h
> index 232fcec..d1d8126 100644
> --- a/lib/librte_eal/common/eal_private.h
> +++ b/lib/librte_eal/common/eal_private.h
> @@ -35,6 +35,9 @@
>  #define _EAL_PRIVATE_H_
> 
>  #include <stdio.h>
> +#include <string.h>
> +#include <rte_log.h>
> +#include <errno.h>
> 
>  /**
>   * Initialize the memzone subsystem (private to eal).
> @@ -203,4 +206,44 @@ int rte_eal_alarm_init(void);
>   */
>  int rte_eal_dev_init(void);
> 
> +/**
> + * Function is to check if the kernel module(like, vfio,
> +vfio_iommu_type1,
> + * etc.) loaded.
> + *
> + * @param module_name
> + *	The module's name which need to be checked
> + *
> + * @return
> + * 	-1 means some error happens(NULL pointer or open failure)
> + * 	0  means the module not loaded
> + * 	1  means the module loaded
> + */
> +static inline int
> +rte_eal_check_module(const char *module_name) {
> +	char mod_name[30]; /* Any module names can be longer than 30
> bytes? */
> +	int ret = 0;
> +
> +	if (NULL == module_name) {
> +		RTE_LOG(ERR, EAL, "The module name is NULL\n");

I don't think this RTE_LOG is necessary - we never RTE_LOG invalid parameters.

> +		return -1;
> +	}
> +	FILE * fd = fopen("/proc/modules", "r");
> +	if (NULL == fd) {
> +		RTE_LOG(ERR, EAL, "Open /proc/modules failed!"
> +			" error %i (%s)\n", errno, strerror(errno));
> +		return -1;
> +	}
> +	while(!feof(fd)) {
> +		fscanf(fd, "%s %*[^\n]", mod_name);
> +		if(!strcmp(mod_name, module_name)) {
> +			ret = 1;
> +			break;
> +		}
> +	}
> +	fclose(fd);
> +
> +	return ret;
> +}
> +
>  #endif /* _EAL_PRIVATE_H_ */
> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> index c1246e8..b34b3f5 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> @@ -44,6 +44,7 @@
>  #include <rte_tailq.h>
>  #include <rte_eal_memconfig.h>
>  #include <rte_malloc.h>
> +#include <eal_private.h>
> 
>  #include "eal_filesystem.h"
>  #include "eal_pci_init.h"
> @@ -339,10 +340,15 @@ pci_vfio_get_container_fd(void)
>  		ret = ioctl(vfio_container_fd, VFIO_CHECK_EXTENSION,
> VFIO_TYPE1_IOMMU);
>  		if (ret != 1) {
>  			if (ret < 0)
> -				RTE_LOG(ERR, EAL, "  could not get IOMMU
> type, "
> -						"error %i (%s)\n", errno,
> strerror(errno));
> +				RTE_LOG(ERR, EAL, "  could not get IOMMU
> type,"
> +					" error %i (%s)\n", errno,
> +					strerror(errno));
>  			else
> -				RTE_LOG(ERR, EAL, "  unsupported IOMMU
> type!\n");
> +				/* Better to show the IOMMU type return
> from
> +				 * kernel for easy debug
> +				 */
> +				RTE_LOG(ERR, EAL, "  unsupported IOMMU
> type"
> +					" detected: %d in VFIO\n", ret);
>  			close(vfio_container_fd);
>  			return -1;
>  		}
> @@ -783,11 +789,29 @@ pci_vfio_enable(void)  {
>  	/* initialize group list */
>  	int i;
> +	int module_vfio, module_vfio_type1;
> 
>  	for (i = 0; i < VFIO_MAX_GROUPS; i++) {
>  		vfio_cfg.vfio_groups[i].fd = -1;
>  		vfio_cfg.vfio_groups[i].group_no = -1;
>  	}
> +
> +	module_vfio = rte_eal_check_module("vfio");
> +	module_vfio_type1 = rte_eal_check_module("vfio_iommu_type1");

We can actually get away with checking just vfio_iommu_type1 (having it loaded implies that vfio is loaded as well).

> +
> +	/* return error directly */
> +	if (module_vfio == -1 || module_vfio_type1 == -1) {
> +		RTE_LOG(INFO, EAL, "Could not get loaded module
> details!\n");
> +		return -1;
> +	}
> +
> +	/* return 0 if not all VFIO modules loaded */
> +	if (module_vfio == 0 || module_vfio_type1 == 0) {
> +		RTE_LOG(INFO, EAL, "VFIO modules not all loaded,"
> +			" skip VFIO support ...\n");
> +		return 0;
> +	}
> +
>  	vfio_cfg.vfio_container_fd = pci_vfio_get_container_fd();
> 
>  	/* check if we have VFIO driver enabled */
> --
> 1.9.3

Re: [dpdk-dev] [PATCH v3] VFIO: Avoid to enable vfio while the module not loaded

[Qiu, Michael]

On 12/8/2014 5:54 PM, Burakov, Anatoly wrote:
>> When vfio module is not loaded when kernel support vfio feature, the
>> routine still try to open the container to get file description.
>>
>> This action is not safe, and of cause got error messages:
>>
>> EAL: Detected 40 lcore(s)
>> EAL:   unsupported IOMMU type!
>> EAL: VFIO support could not be initialized
>> EAL: Setting up memory...
>>
>> This may make user confuse, this patch make it reasonable and much more
>> soomth to user.
>>
>> Signed-off-by: Michael Qiu <michael.qiu@intel.com>
>> ---
>> v3 --> v2:
>> 	1. Add error log in rte_eal_check_module()
>> 	2. Some code clean up.
>>
>> v2 --> v1:
>> 	1. Move check_module() from rte_common.h to eal_private.h
>> 	   and rename to rte_eal_check_module().
>> 	   To make it linuxapp only.
>> 	2. Some code clean up.
>>
>>  lib/librte_eal/common/eal_private.h        | 43
>> ++++++++++++++++++++++++++++++
>>  lib/librte_eal/linuxapp/eal/eal_pci_vfio.c | 30 ++++++++++++++++++---
>>  2 files changed, 70 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/librte_eal/common/eal_private.h
>> b/lib/librte_eal/common/eal_private.h
>> index 232fcec..d1d8126 100644
>> --- a/lib/librte_eal/common/eal_private.h
>> +++ b/lib/librte_eal/common/eal_private.h
>> @@ -35,6 +35,9 @@
>>  #define _EAL_PRIVATE_H_
>>
>>  #include <stdio.h>
>> +#include <string.h>
>> +#include <rte_log.h>
>> +#include <errno.h>
>>
>>  /**
>>   * Initialize the memzone subsystem (private to eal).
>> @@ -203,4 +206,44 @@ int rte_eal_alarm_init(void);
>>   */
>>  int rte_eal_dev_init(void);
>>
>> +/**
>> + * Function is to check if the kernel module(like, vfio,
>> +vfio_iommu_type1,
>> + * etc.) loaded.
>> + *
>> + * @param module_name
>> + *	The module's name which need to be checked
>> + *
>> + * @return
>> + * 	-1 means some error happens(NULL pointer or open failure)
>> + * 	0  means the module not loaded
>> + * 	1  means the module loaded
>> + */
>> +static inline int
>> +rte_eal_check_module(const char *module_name) {
>> +	char mod_name[30]; /* Any module names can be longer than 30
>> bytes? */
>> +	int ret = 0;
>> +
>> +	if (NULL == module_name) {
>> +		RTE_LOG(ERR, EAL, "The module name is NULL\n");
> I don't think this RTE_LOG is necessary - we never RTE_LOG invalid parameters.

OK, I will remove this.

>
>> +		return -1;
>> +	}
>> +	FILE * fd = fopen("/proc/modules", "r");
>> +	if (NULL == fd) {
>> +		RTE_LOG(ERR, EAL, "Open /proc/modules failed!"
>> +			" error %i (%s)\n", errno, strerror(errno));
>> +		return -1;
>> +	}
>> +	while(!feof(fd)) {
>> +		fscanf(fd, "%s %*[^\n]", mod_name);
>> +		if(!strcmp(mod_name, module_name)) {
>> +			ret = 1;
>> +			break;
>> +		}
>> +	}
>> +	fclose(fd);
>> +
>> +	return ret;
>> +}
>> +
>>  #endif /* _EAL_PRIVATE_H_ */
>> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
>> b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
>> index c1246e8..b34b3f5 100644
>> --- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
>> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
>> @@ -44,6 +44,7 @@
>>  #include <rte_tailq.h>
>>  #include <rte_eal_memconfig.h>
>>  #include <rte_malloc.h>
>> +#include <eal_private.h>
>>
>>  #include "eal_filesystem.h"
>>  #include "eal_pci_init.h"
>> @@ -339,10 +340,15 @@ pci_vfio_get_container_fd(void)
>>  		ret = ioctl(vfio_container_fd, VFIO_CHECK_EXTENSION,
>> VFIO_TYPE1_IOMMU);
>>  		if (ret != 1) {
>>  			if (ret < 0)
>> -				RTE_LOG(ERR, EAL, "  could not get IOMMU
>> type, "
>> -						"error %i (%s)\n", errno,
>> strerror(errno));
>> +				RTE_LOG(ERR, EAL, "  could not get IOMMU
>> type,"
>> +					" error %i (%s)\n", errno,
>> +					strerror(errno));
>>  			else
>> -				RTE_LOG(ERR, EAL, "  unsupported IOMMU
>> type!\n");
>> +				/* Better to show the IOMMU type return
>> from
>> +				 * kernel for easy debug
>> +				 */
>> +				RTE_LOG(ERR, EAL, "  unsupported IOMMU
>> type"
>> +					" detected: %d in VFIO\n", ret);
>>  			close(vfio_container_fd);
>>  			return -1;
>>  		}
>> @@ -783,11 +789,29 @@ pci_vfio_enable(void)  {
>>  	/* initialize group list */
>>  	int i;
>> +	int module_vfio, module_vfio_type1;
>>
>>  	for (i = 0; i < VFIO_MAX_GROUPS; i++) {
>>  		vfio_cfg.vfio_groups[i].fd = -1;
>>  		vfio_cfg.vfio_groups[i].group_no = -1;
>>  	}
>> +
>> +	module_vfio = rte_eal_check_module("vfio");
>> +	module_vfio_type1 = rte_eal_check_module("vfio_iommu_type1");
> We can actually get away with checking just vfio_iommu_type1 (having it loaded implies that vfio is loaded as well).

Yes, you are right. That's the point, I will rework this.

Thanks,
Michael
>
>> +
>> +	/* return error directly */
>> +	if (module_vfio == -1 || module_vfio_type1 == -1) {
>> +		RTE_LOG(INFO, EAL, "Could not get loaded module
>> details!\n");
>> +		return -1;
>> +	}
>> +
>> +	/* return 0 if not all VFIO modules loaded */
>> +	if (module_vfio == 0 || module_vfio_type1 == 0) {
>> +		RTE_LOG(INFO, EAL, "VFIO modules not all loaded,"
>> +			" skip VFIO support ...\n");
>> +		return 0;
>> +	}
>> +
>>  	vfio_cfg.vfio_container_fd = pci_vfio_get_container_fd();
>>
>>  	/* check if we have VFIO driver enabled */
>> --
>> 1.9.3
>

[dpdk-dev] [PATCH v4] VFIO: Avoid to enable vfio while the module not loaded

[Michael Qiu]

When vfio module is not loaded when kernel support vfio feature,
the routine still try to open the container to get file
description.

This action is not safe, and of cause got error messages:

EAL: Detected 40 lcore(s)
EAL:   unsupported IOMMU type!
EAL: VFIO support could not be initialized
EAL: Setting up memory...

This may make user confuse, this patch make it reasonable
and much more soomth to user.

Signed-off-by: Michael Qiu <michael.qiu@intel.com>
---
v4 --> v3:
	1. Remove RTE_LOG for params check
	2. Remove "vfio" module check as "vfio_iommu_type1"
	   loaded indecated "vfio" loaded

v3 --> v2:
        1. Add error log in rte_eal_check_module()
        2. Some code clean up.

v2 --> v1:
        1. Move check_module() from rte_common.h to eal_private.h
           and rename to rte_eal_check_module().
           To make it linuxapp only.
        2. Some code clean up.

 lib/librte_eal/common/eal_private.h        | 42 ++++++++++++++++++++++++++++++
 lib/librte_eal/linuxapp/eal/eal_pci_vfio.c | 29 ++++++++++++++++++---
 2 files changed, 68 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
index 232fcec..e877a25 100644
--- a/lib/librte_eal/common/eal_private.h
+++ b/lib/librte_eal/common/eal_private.h
@@ -35,6 +35,9 @@
 #define _EAL_PRIVATE_H_
 
 #include <stdio.h>
+#include <string.h>
+#include <rte_log.h>
+#include <errno.h>
 
 /**
  * Initialize the memzone subsystem (private to eal).
@@ -203,4 +206,43 @@ int rte_eal_alarm_init(void);
  */
 int rte_eal_dev_init(void);
 
+/**
+ * Function is to check if the kernel module(like, vfio, vfio_iommu_type1,
+ * etc.) loaded.
+ *
+ * @param module_name
+ *	The module's name which need to be checked
+ *
+ * @return
+ * 	-1 means some error happens(NULL pointer or open failure)
+ * 	0  means the module not loaded
+ * 	1  means the module loaded
+ */
+static inline int
+rte_eal_check_module(const char *module_name)
+{
+	char mod_name[30]; /* Any module names can be longer than 30 bytes? */
+	int ret = 0;
+
+	if (NULL == module_name)
+		return -1;
+
+	FILE * fd = fopen("/proc/modules", "r");
+	if (NULL == fd) {
+		RTE_LOG(ERR, EAL, "Open /proc/modules failed!"
+			" error %i (%s)\n", errno, strerror(errno));
+		return -1;
+	}
+	while(!feof(fd)) {
+		fscanf(fd, "%s %*[^\n]", mod_name);
+		if(!strcmp(mod_name, module_name)) {
+			ret = 1;
+			break;
+		}
+	}
+	fclose(fd);
+
+	return ret;
+}
+
 #endif /* _EAL_PRIVATE_H_ */
diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
index c1246e8..8c54d2a 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
@@ -44,6 +44,7 @@
 #include <rte_tailq.h>
 #include <rte_eal_memconfig.h>
 #include <rte_malloc.h>
+#include <eal_private.h>
 
 #include "eal_filesystem.h"
 #include "eal_pci_init.h"
@@ -339,10 +340,15 @@ pci_vfio_get_container_fd(void)
 		ret = ioctl(vfio_container_fd, VFIO_CHECK_EXTENSION, VFIO_TYPE1_IOMMU);
 		if (ret != 1) {
 			if (ret < 0)
-				RTE_LOG(ERR, EAL, "  could not get IOMMU type, "
-						"error %i (%s)\n", errno, strerror(errno));
+				RTE_LOG(ERR, EAL, "  could not get IOMMU type,"
+					" error %i (%s)\n", errno,
+					strerror(errno));
 			else
-				RTE_LOG(ERR, EAL, "  unsupported IOMMU type!\n");
+				/* Better to show the IOMMU type return from
+				 * kernel for easy debug
+				 */
+				RTE_LOG(ERR, EAL, "  unsupported IOMMU type"
+					" detected: %d in VFIO\n", ret);
 			close(vfio_container_fd);
 			return -1;
 		}
@@ -783,11 +789,28 @@ pci_vfio_enable(void)
 {
 	/* initialize group list */
 	int i;
+	int module_vfio_type1;
 
 	for (i = 0; i < VFIO_MAX_GROUPS; i++) {
 		vfio_cfg.vfio_groups[i].fd = -1;
 		vfio_cfg.vfio_groups[i].group_no = -1;
 	}
+
+	module_vfio_type1 = rte_eal_check_module("vfio_iommu_type1");
+
+	/* return error directly */
+	if (module_vfio_type1 == -1) {
+		RTE_LOG(INFO, EAL, "Could not get loaded module details!\n");
+		return -1;
+	}
+
+	/* return 0 if VFIO modules not loaded */
+	if (module_vfio_type1 == 0) {
+		RTE_LOG(INFO, EAL, "VFIO modules not all loaded,"
+			" skip VFIO support ...\n");
+		return 0;
+	}
+
 	vfio_cfg.vfio_container_fd = pci_vfio_get_container_fd();
 
 	/* check if we have VFIO driver enabled */
-- 
1.9.3

Re: [dpdk-dev] [PATCH v4] VFIO: Avoid to enable vfio while the module not loaded

[Burakov, Anatoly]

> When vfio module is not loaded when kernel support vfio feature, the
> routine still try to open the container to get file description.
> 
> This action is not safe, and of cause got error messages:
> 
> EAL: Detected 40 lcore(s)
> EAL:   unsupported IOMMU type!
> EAL: VFIO support could not be initialized
> EAL: Setting up memory...
> 
> This may make user confuse, this patch make it reasonable and much more
> soomth to user.
> 
> Signed-off-by: Michael Qiu <michael.qiu@intel.com>
> ---
> v4 --> v3:
> 	1. Remove RTE_LOG for params check
> 	2. Remove "vfio" module check as "vfio_iommu_type1"
> 	   loaded indecated "vfio" loaded
> 
> v3 --> v2:
>         1. Add error log in rte_eal_check_module()
>         2. Some code clean up.
> 
> v2 --> v1:
>         1. Move check_module() from rte_common.h to eal_private.h
>            and rename to rte_eal_check_module().
>            To make it linuxapp only.
>         2. Some code clean up.
> 
>  lib/librte_eal/common/eal_private.h        | 42
> ++++++++++++++++++++++++++++++
>  lib/librte_eal/linuxapp/eal/eal_pci_vfio.c | 29 ++++++++++++++++++---
>  2 files changed, 68 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_private.h
> b/lib/librte_eal/common/eal_private.h
> index 232fcec..e877a25 100644
> --- a/lib/librte_eal/common/eal_private.h
> +++ b/lib/librte_eal/common/eal_private.h
> @@ -35,6 +35,9 @@
>  #define _EAL_PRIVATE_H_
> 
>  #include <stdio.h>
> +#include <string.h>
> +#include <rte_log.h>
> +#include <errno.h>
> 
>  /**
>   * Initialize the memzone subsystem (private to eal).
> @@ -203,4 +206,43 @@ int rte_eal_alarm_init(void);
>   */
>  int rte_eal_dev_init(void);
> 
> +/**
> + * Function is to check if the kernel module(like, vfio,
> +vfio_iommu_type1,
> + * etc.) loaded.
> + *
> + * @param module_name
> + *	The module's name which need to be checked
> + *
> + * @return
> + * 	-1 means some error happens(NULL pointer or open failure)
> + * 	0  means the module not loaded
> + * 	1  means the module loaded
> + */
> +static inline int
> +rte_eal_check_module(const char *module_name) {
> +	char mod_name[30]; /* Any module names can be longer than 30
> bytes? */
> +	int ret = 0;
> +
> +	if (NULL == module_name)
> +		return -1;
> +
> +	FILE * fd = fopen("/proc/modules", "r");
> +	if (NULL == fd) {
> +		RTE_LOG(ERR, EAL, "Open /proc/modules failed!"
> +			" error %i (%s)\n", errno, strerror(errno));
> +		return -1;
> +	}
> +	while(!feof(fd)) {
> +		fscanf(fd, "%s %*[^\n]", mod_name);
> +		if(!strcmp(mod_name, module_name)) {
> +			ret = 1;
> +			break;
> +		}
> +	}
> +	fclose(fd);
> +
> +	return ret;
> +}
> +

Apologies for not bringing this up before, but do we really want the rte_eal_check_module inline in the header? I think it would be better to declare it in eal_private but move the definition into eal.c.

>  #endif /* _EAL_PRIVATE_H_ */
> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> index c1246e8..8c54d2a 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> @@ -44,6 +44,7 @@
>  #include <rte_tailq.h>
>  #include <rte_eal_memconfig.h>
>  #include <rte_malloc.h>
> +#include <eal_private.h>
> 
>  #include "eal_filesystem.h"
>  #include "eal_pci_init.h"
> @@ -339,10 +340,15 @@ pci_vfio_get_container_fd(void)
>  		ret = ioctl(vfio_container_fd, VFIO_CHECK_EXTENSION,
> VFIO_TYPE1_IOMMU);
>  		if (ret != 1) {
>  			if (ret < 0)
> -				RTE_LOG(ERR, EAL, "  could not get IOMMU
> type, "
> -						"error %i (%s)\n", errno,
> strerror(errno));
> +				RTE_LOG(ERR, EAL, "  could not get IOMMU
> type,"
> +					" error %i (%s)\n", errno,
> +					strerror(errno));
>  			else
> -				RTE_LOG(ERR, EAL, "  unsupported IOMMU
> type!\n");
> +				/* Better to show the IOMMU type return
> from
> +				 * kernel for easy debug
> +				 */
> +				RTE_LOG(ERR, EAL, "  unsupported IOMMU
> type"
> +					" detected: %d in VFIO\n", ret);

I'm not sure this message is meaningful. That ioctl call can either -1, 0 or 1. We already handle 1 separately; -1 means an error; 0 means IOMMU type 1 is not supported. The return value will *not* indicate which IOMMU types *are* currently supported - it will only indicate that the IOMMU type you requested is not supported. So there's really no point in indicating the return value in case of ret 0 - it is best to just mention that requested IOMMU type support is not enabled in VFIO.

>  			close(vfio_container_fd);
>  			return -1;
>  		}
> @@ -783,11 +789,28 @@ pci_vfio_enable(void)  {
>  	/* initialize group list */
>  	int i;
> +	int module_vfio_type1;
> 
>  	for (i = 0; i < VFIO_MAX_GROUPS; i++) {
>  		vfio_cfg.vfio_groups[i].fd = -1;
>  		vfio_cfg.vfio_groups[i].group_no = -1;
>  	}
> +
> +	module_vfio_type1 = rte_eal_check_module("vfio_iommu_type1");
> +
> +	/* return error directly */
> +	if (module_vfio_type1 == -1) {
> +		RTE_LOG(INFO, EAL, "Could not get loaded module
> details!\n");
> +		return -1;
> +	}
> +
> +	/* return 0 if VFIO modules not loaded */
> +	if (module_vfio_type1 == 0) {
> +		RTE_LOG(INFO, EAL, "VFIO modules not all loaded,"
> +			" skip VFIO support ...\n");
> +		return 0;
> +	}
> +
>  	vfio_cfg.vfio_container_fd = pci_vfio_get_container_fd();
> 
>  	/* check if we have VFIO driver enabled */
> --
> 1.9.3

Re: [dpdk-dev] [PATCH v4] VFIO: Avoid to enable vfio while the module not loaded

[Qiu, Michael]

On 2014/12/8 20:19, Burakov, Anatoly wrote:
>> When vfio module is not loaded when kernel support vfio feature, the
>> routine still try to open the container to get file description.
>>
>> This action is not safe, and of cause got error messages:
>>
>> EAL: Detected 40 lcore(s)
>> EAL:   unsupported IOMMU type!
>> EAL: VFIO support could not be initialized
>> EAL: Setting up memory...
>>
>> This may make user confuse, this patch make it reasonable and much more
>> soomth to user.
>>
>> Signed-off-by: Michael Qiu <michael.qiu@intel.com>
>> ---
>> v4 --> v3:
>> 	1. Remove RTE_LOG for params check
>> 	2. Remove "vfio" module check as "vfio_iommu_type1"
>> 	   loaded indecated "vfio" loaded
>>
>> v3 --> v2:
>>         1. Add error log in rte_eal_check_module()
>>         2. Some code clean up.
>>
>> v2 --> v1:
>>         1. Move check_module() from rte_common.h to eal_private.h
>>            and rename to rte_eal_check_module().
>>            To make it linuxapp only.
>>         2. Some code clean up.
>>
>>  lib/librte_eal/common/eal_private.h        | 42
>> ++++++++++++++++++++++++++++++
>>  lib/librte_eal/linuxapp/eal/eal_pci_vfio.c | 29 ++++++++++++++++++---
>>  2 files changed, 68 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/librte_eal/common/eal_private.h
>> b/lib/librte_eal/common/eal_private.h
>> index 232fcec..e877a25 100644
>> --- a/lib/librte_eal/common/eal_private.h
>> +++ b/lib/librte_eal/common/eal_private.h
>> @@ -35,6 +35,9 @@
>>  #define _EAL_PRIVATE_H_
>>
>>  #include <stdio.h>
>> +#include <string.h>
>> +#include <rte_log.h>
>> +#include <errno.h>
>>
>>  /**
>>   * Initialize the memzone subsystem (private to eal).
>> @@ -203,4 +206,43 @@ int rte_eal_alarm_init(void);
>>   */
>>  int rte_eal_dev_init(void);
>>
>> +/**
>> + * Function is to check if the kernel module(like, vfio,
>> +vfio_iommu_type1,
>> + * etc.) loaded.
>> + *
>> + * @param module_name
>> + *	The module's name which need to be checked
>> + *
>> + * @return
>> + * 	-1 means some error happens(NULL pointer or open failure)
>> + * 	0  means the module not loaded
>> + * 	1  means the module loaded
>> + */
>> +static inline int
>> +rte_eal_check_module(const char *module_name) {
>> +	char mod_name[30]; /* Any module names can be longer than 30
>> bytes? */
>> +	int ret = 0;
>> +
>> +	if (NULL == module_name)
>> +		return -1;
>> +
>> +	FILE * fd = fopen("/proc/modules", "r");
>> +	if (NULL == fd) {
>> +		RTE_LOG(ERR, EAL, "Open /proc/modules failed!"
>> +			" error %i (%s)\n", errno, strerror(errno));
>> +		return -1;
>> +	}
>> +	while(!feof(fd)) {
>> +		fscanf(fd, "%s %*[^\n]", mod_name);
>> +		if(!strcmp(mod_name, module_name)) {
>> +			ret = 1;
>> +			break;
>> +		}
>> +	}
>> +	fclose(fd);
>> +
>> +	return ret;
>> +}
>> +
> Apologies for not bringing this up before, but do we really want the rte_eal_check_module inline in the header? I think it would be better to declare it in eal_private but move the definition into eal.c.

No need, actually, I'm very appreciate that you can spend your time to
review my patch again and again. I really want to say thank you to you.

For rte_eal_check_module inline in the header, it really no need stay in
header, so ugly. I will make new version of it, and re-post.

 
>>  #endif /* _EAL_PRIVATE_H_ */
>> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
>> b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
>> index c1246e8..8c54d2a 100644
>> --- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
>> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
>> @@ -44,6 +44,7 @@
>>  #include <rte_tailq.h>
>>  #include <rte_eal_memconfig.h>
>>  #include <rte_malloc.h>
>> +#include <eal_private.h>
>>
>>  #include "eal_filesystem.h"
>>  #include "eal_pci_init.h"
>> @@ -339,10 +340,15 @@ pci_vfio_get_container_fd(void)
>>  		ret = ioctl(vfio_container_fd, VFIO_CHECK_EXTENSION,
>> VFIO_TYPE1_IOMMU);
>>  		if (ret != 1) {
>>  			if (ret < 0)
>> -				RTE_LOG(ERR, EAL, "  could not get IOMMU
>> type, "
>> -						"error %i (%s)\n", errno,
>> strerror(errno));
>> +				RTE_LOG(ERR, EAL, "  could not get IOMMU
>> type,"
>> +					" error %i (%s)\n", errno,
>> +					strerror(errno));
>>  			else
>> -				RTE_LOG(ERR, EAL, "  unsupported IOMMU
>> type!\n");
>> +				/* Better to show the IOMMU type return
>> from
>> +				 * kernel for easy debug
>> +				 */
>> +				RTE_LOG(ERR, EAL, "  unsupported IOMMU
>> type"
>> +					" detected: %d in VFIO\n", ret);
> I'm not sure this message is meaningful. That ioctl call can either -1, 0 or 1. We already handle 1 separately; -1 means an error; 0 means IOMMU type 1 is not supported. The return value will *not* indicate which IOMMU types *are* currently supported - it will only indicate that the IOMMU type you requested is not supported. So there's really no point in indicating the return value in case of ret 0 - it is best to just mention that requested IOMMU type support is not enabled in VFIO.

Yes, you are right, I make a mistake.

>
>>  			close(vfio_container_fd);
>>  			return -1;
>>  		}
>> @@ -783,11 +789,28 @@ pci_vfio_enable(void)  {
>>  	/* initialize group list */
>>  	int i;
>> +	int module_vfio_type1;
>>
>>  	for (i = 0; i < VFIO_MAX_GROUPS; i++) {
>>  		vfio_cfg.vfio_groups[i].fd = -1;
>>  		vfio_cfg.vfio_groups[i].group_no = -1;
>>  	}
>> +
>> +	module_vfio_type1 = rte_eal_check_module("vfio_iommu_type1");
>> +
>> +	/* return error directly */
>> +	if (module_vfio_type1 == -1) {
>> +		RTE_LOG(INFO, EAL, "Could not get loaded module
>> details!\n");
>> +		return -1;
>> +	}
>> +
>> +	/* return 0 if VFIO modules not loaded */
>> +	if (module_vfio_type1 == 0) {
>> +		RTE_LOG(INFO, EAL, "VFIO modules not all loaded,"
>> +			" skip VFIO support ...\n");
>> +		return 0;
>> +	}
>> +
>>  	vfio_cfg.vfio_container_fd = pci_vfio_get_container_fd();
>>
>>  	/* check if we have VFIO driver enabled */
>> --
>> 1.9.3
>

Re: [dpdk-dev] [PATCH v4] VFIO: Avoid to enable vfio while the module not loaded

[Xie, Huawei]

Hi Michael:

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Qiu, Michael
> Sent: Monday, December 08, 2014 8:28 AM
> To: Burakov, Anatoly; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v4] VFIO: Avoid to enable vfio while the module
> not loaded
> 
> On 2014/12/8 20:19, Burakov, Anatoly wrote:
> >> When vfio module is not loaded when kernel support vfio feature, the
> >> routine still try to open the container to get file description.
> >>
> >> This action is not safe, and of cause got error messages:
> >>
> >> EAL: Detected 40 lcore(s)
> >> EAL:   unsupported IOMMU type!
> >> EAL: VFIO support could not be initialized
> >> EAL: Setting up memory...
> >>
> >> This may make user confuse, this patch make it reasonable and much more
> >> soomth to user.
> >>
> >> Signed-off-by: Michael Qiu <michael.qiu@intel.com>
> >> ---
> >> v4 --> v3:
> >> 	1. Remove RTE_LOG for params check
> >> 	2. Remove "vfio" module check as "vfio_iommu_type1"
> >> 	   loaded indecated "vfio" loaded
> >>
> >> v3 --> v2:
> >>         1. Add error log in rte_eal_check_module()
> >>         2. Some code clean up.
> >>
> >> v2 --> v1:
> >>         1. Move check_module() from rte_common.h to eal_private.h
> >>            and rename to rte_eal_check_module().
> >>            To make it linuxapp only.
> >>         2. Some code clean up.
> >>
> >>  lib/librte_eal/common/eal_private.h        | 42
> >> ++++++++++++++++++++++++++++++
> >>  lib/librte_eal/linuxapp/eal/eal_pci_vfio.c | 29 ++++++++++++++++++---
> >>  2 files changed, 68 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/lib/librte_eal/common/eal_private.h
> >> b/lib/librte_eal/common/eal_private.h
> >> index 232fcec..e877a25 100644
> >> --- a/lib/librte_eal/common/eal_private.h
> >> +++ b/lib/librte_eal/common/eal_private.h
> >> @@ -35,6 +35,9 @@
> >>  #define _EAL_PRIVATE_H_
> >>
> >>  #include <stdio.h>
> >> +#include <string.h>
> >> +#include <rte_log.h>
> >> +#include <errno.h>
> >>
> >>  /**
> >>   * Initialize the memzone subsystem (private to eal).
> >> @@ -203,4 +206,43 @@ int rte_eal_alarm_init(void);
> >>   */
> >>  int rte_eal_dev_init(void);
> >>
> >> +/**
> >> + * Function is to check if the kernel module(like, vfio,
> >> +vfio_iommu_type1,
> >> + * etc.) loaded.
> >> + *
> >> + * @param module_name
> >> + *	The module's name which need to be checked
> >> + *
> >> + * @return
> >> + * 	-1 means some error happens(NULL pointer or open failure)
> >> + * 	0  means the module not loaded
> >> + * 	1  means the module loaded
> >> + */
> >> +static inline int
> >> +rte_eal_check_module(const char *module_name) {
> >> +	char mod_name[30]; /* Any module names can be longer than 30
> >> bytes? */
> >> +	int ret = 0;
> >> +
> >> +	if (NULL == module_name)
> >> +		return -1;
> >> +
> >> +	FILE * fd = fopen("/proc/modules", "r");
> >> +	if (NULL == fd) {
> >> +		RTE_LOG(ERR, EAL, "Open /proc/modules failed!"
> >> +			" error %i (%s)\n", errno, strerror(errno));
> >> +		return -1;
> >> +	}
> >> +	while(!feof(fd)) {
> >> +		fscanf(fd, "%s %*[^\n]", mod_name);
I see it is already discussed in the other mail that we could limit the count to avoid overflow.
I don't understand why you don't apply it here.
There are already several existing modules that has 20+ length. 
> >> +		if(!strcmp(mod_name, module_name)) {
> >> +			ret = 1;
> >> +			break;
> >> +		}
> >> +	}
> >> +	fclose(fd);
> >> +
> >> +	return ret;
> >> +}
> >> +
> > Apologies for not bringing this up before, but do we really want the
> rte_eal_check_module inline in the header? I think it would be better to declare
> it in eal_private but move the definition into eal.c.
> 
> No need, actually, I'm very appreciate that you can spend your time to
> review my patch again and again. I really want to say thank you to you.
> 
> For rte_eal_check_module inline in the header, it really no need stay in
> header, so ugly. I will make new version of it, and re-post.
> 
> 
> >>  #endif /* _EAL_PRIVATE_H_ */
> >> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> >> b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> >> index c1246e8..8c54d2a 100644
> >> --- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> >> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> >> @@ -44,6 +44,7 @@
> >>  #include <rte_tailq.h>
> >>  #include <rte_eal_memconfig.h>
> >>  #include <rte_malloc.h>
> >> +#include <eal_private.h>
> >>
> >>  #include "eal_filesystem.h"
> >>  #include "eal_pci_init.h"
> >> @@ -339,10 +340,15 @@ pci_vfio_get_container_fd(void)
> >>  		ret = ioctl(vfio_container_fd, VFIO_CHECK_EXTENSION,
> >> VFIO_TYPE1_IOMMU);
> >>  		if (ret != 1) {
> >>  			if (ret < 0)
> >> -				RTE_LOG(ERR, EAL, "  could not get IOMMU
> >> type, "
> >> -						"error %i (%s)\n", errno,
> >> strerror(errno));
> >> +				RTE_LOG(ERR, EAL, "  could not get IOMMU
> >> type,"
> >> +					" error %i (%s)\n", errno,
> >> +					strerror(errno));
> >>  			else
> >> -				RTE_LOG(ERR, EAL, "  unsupported IOMMU
> >> type!\n");
> >> +				/* Better to show the IOMMU type return
> >> from
> >> +				 * kernel for easy debug
> >> +				 */
> >> +				RTE_LOG(ERR, EAL, "  unsupported IOMMU
> >> type"
> >> +					" detected: %d in VFIO\n", ret);
> > I'm not sure this message is meaningful. That ioctl call can either -1, 0 or 1. We
> already handle 1 separately; -1 means an error; 0 means IOMMU type 1 is not
> supported. The return value will *not* indicate which IOMMU types *are*
> currently supported - it will only indicate that the IOMMU type you requested is
> not supported. So there's really no point in indicating the return value in case of
> ret 0 - it is best to just mention that requested IOMMU type support is not
> enabled in VFIO.
> 
> Yes, you are right, I make a mistake.
> 
> >
> >>  			close(vfio_container_fd);
> >>  			return -1;
> >>  		}
> >> @@ -783,11 +789,28 @@ pci_vfio_enable(void)  {
> >>  	/* initialize group list */
> >>  	int i;
> >> +	int module_vfio_type1;
> >>
> >>  	for (i = 0; i < VFIO_MAX_GROUPS; i++) {
> >>  		vfio_cfg.vfio_groups[i].fd = -1;
> >>  		vfio_cfg.vfio_groups[i].group_no = -1;
> >>  	}
> >> +
> >> +	module_vfio_type1 = rte_eal_check_module("vfio_iommu_type1");
> >> +
> >> +	/* return error directly */
> >> +	if (module_vfio_type1 == -1) {
> >> +		RTE_LOG(INFO, EAL, "Could not get loaded module
> >> details!\n");
> >> +		return -1;
> >> +	}
> >> +
> >> +	/* return 0 if VFIO modules not loaded */
> >> +	if (module_vfio_type1 == 0) {
> >> +		RTE_LOG(INFO, EAL, "VFIO modules not all loaded,"
> >> +			" skip VFIO support ...\n");
> >> +		return 0;
> >> +	}
> >> +
> >>  	vfio_cfg.vfio_container_fd = pci_vfio_get_container_fd();
> >>
> >>  	/* check if we have VFIO driver enabled */
> >> --
> >> 1.9.3
> >

Re: [dpdk-dev] [PATCH v4] VFIO: Avoid to enable vfio while the module not loaded

[Qiu, Michael]

On 12/10/2014 8:17 AM, Xie, Huawei wrote:
> Hi Michael:
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Qiu, Michael
>> Sent: Monday, December 08, 2014 8:28 AM
>> To: Burakov, Anatoly; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v4] VFIO: Avoid to enable vfio while the module
>> not loaded
>>
>> On 2014/12/8 20:19, Burakov, Anatoly wrote:
>>>> When vfio module is not loaded when kernel support vfio feature, the
>>>> routine still try to open the container to get file description.
>>>>
>>>> This action is not safe, and of cause got error messages:
>>>>
>>>> EAL: Detected 40 lcore(s)
>>>> EAL:   unsupported IOMMU type!
>>>> EAL: VFIO support could not be initialized
>>>> EAL: Setting up memory...
>>>>
>>>> This may make user confuse, this patch make it reasonable and much more
>>>> soomth to user.
>>>>
>>>> Signed-off-by: Michael Qiu <michael.qiu@intel.com>
>>>> ---
>>>> v4 --> v3:
>>>> 	1. Remove RTE_LOG for params check
>>>> 	2. Remove "vfio" module check as "vfio_iommu_type1"
>>>> 	   loaded indecated "vfio" loaded
>>>>
>>>> v3 --> v2:
>>>>         1. Add error log in rte_eal_check_module()
>>>>         2. Some code clean up.
>>>>
>>>> v2 --> v1:
>>>>         1. Move check_module() from rte_common.h to eal_private.h
>>>>            and rename to rte_eal_check_module().
>>>>            To make it linuxapp only.
>>>>         2. Some code clean up.
>>>>
>>>>  lib/librte_eal/common/eal_private.h        | 42
>>>> ++++++++++++++++++++++++++++++
>>>>  lib/librte_eal/linuxapp/eal/eal_pci_vfio.c | 29 ++++++++++++++++++---
>>>>  2 files changed, 68 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/lib/librte_eal/common/eal_private.h
>>>> b/lib/librte_eal/common/eal_private.h
>>>> index 232fcec..e877a25 100644
>>>> --- a/lib/librte_eal/common/eal_private.h
>>>> +++ b/lib/librte_eal/common/eal_private.h
>>>> @@ -35,6 +35,9 @@
>>>>  #define _EAL_PRIVATE_H_
>>>>
>>>>  #include <stdio.h>
>>>> +#include <string.h>
>>>> +#include <rte_log.h>
>>>> +#include <errno.h>
>>>>
>>>>  /**
>>>>   * Initialize the memzone subsystem (private to eal).
>>>> @@ -203,4 +206,43 @@ int rte_eal_alarm_init(void);
>>>>   */
>>>>  int rte_eal_dev_init(void);
>>>>
>>>> +/**
>>>> + * Function is to check if the kernel module(like, vfio,
>>>> +vfio_iommu_type1,
>>>> + * etc.) loaded.
>>>> + *
>>>> + * @param module_name
>>>> + *	The module's name which need to be checked
>>>> + *
>>>> + * @return
>>>> + * 	-1 means some error happens(NULL pointer or open failure)
>>>> + * 	0  means the module not loaded
>>>> + * 	1  means the module loaded
>>>> + */
>>>> +static inline int
>>>> +rte_eal_check_module(const char *module_name) {
>>>> +	char mod_name[30]; /* Any module names can be longer than 30
>>>> bytes? */
>>>> +	int ret = 0;
>>>> +
>>>> +	if (NULL == module_name)
>>>> +		return -1;
>>>> +
>>>> +	FILE * fd = fopen("/proc/modules", "r");
>>>> +	if (NULL == fd) {
>>>> +		RTE_LOG(ERR, EAL, "Open /proc/modules failed!"
>>>> +			" error %i (%s)\n", errno, strerror(errno));
>>>> +		return -1;
>>>> +	}
>>>> +	while(!feof(fd)) {
>>>> +		fscanf(fd, "%s %*[^\n]", mod_name);
> I see it is already discussed in the other mail that we could limit the count to avoid overflow.
> I don't understand why you don't apply it here.


Yes, I have reply in this thread with below:
  fscanf(fd, "%30s %*[^\n]", mod_name);

> There are already several existing modules that has 20+ length. 

Yes, would you think 30 length is enough? Otherwise I need to increase
the length

Thanks,
Michael
>>>> +		if(!strcmp(mod_name, module_name)) {
>>>> +			ret = 1;
>>>> +			break;
>>>> +		}
>>>> +	}
>>>> +	fclose(fd);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>> Apologies for not bringing this up before, but do we really want the
>> rte_eal_check_module inline in the header? I think it would be better to declare
>> it in eal_private but move the definition into eal.c.
>>
>> No need, actually, I'm very appreciate that you can spend your time to
>> review my patch again and again. I really want to say thank you to you.
>>
>> For rte_eal_check_module inline in the header, it really no need stay in
>> header, so ugly. I will make new version of it, and re-post.
>>
>>
>>>>  #endif /* _EAL_PRIVATE_H_ */
>>>> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
>>>> b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
>>>> index c1246e8..8c54d2a 100644
>>>> --- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
>>>> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
>>>> @@ -44,6 +44,7 @@
>>>>  #include <rte_tailq.h>
>>>>  #include <rte_eal_memconfig.h>
>>>>  #include <rte_malloc.h>
>>>> +#include <eal_private.h>
>>>>
>>>>  #include "eal_filesystem.h"
>>>>  #include "eal_pci_init.h"
>>>> @@ -339,10 +340,15 @@ pci_vfio_get_container_fd(void)
>>>>  		ret = ioctl(vfio_container_fd, VFIO_CHECK_EXTENSION,
>>>> VFIO_TYPE1_IOMMU);
>>>>  		if (ret != 1) {
>>>>  			if (ret < 0)
>>>> -				RTE_LOG(ERR, EAL, "  could not get IOMMU
>>>> type, "
>>>> -						"error %i (%s)\n", errno,
>>>> strerror(errno));
>>>> +				RTE_LOG(ERR, EAL, "  could not get IOMMU
>>>> type,"
>>>> +					" error %i (%s)\n", errno,
>>>> +					strerror(errno));
>>>>  			else
>>>> -				RTE_LOG(ERR, EAL, "  unsupported IOMMU
>>>> type!\n");
>>>> +				/* Better to show the IOMMU type return
>>>> from
>>>> +				 * kernel for easy debug
>>>> +				 */
>>>> +				RTE_LOG(ERR, EAL, "  unsupported IOMMU
>>>> type"
>>>> +					" detected: %d in VFIO\n", ret);
>>> I'm not sure this message is meaningful. That ioctl call can either -1, 0 or 1. We
>> already handle 1 separately; -1 means an error; 0 means IOMMU type 1 is not
>> supported. The return value will *not* indicate which IOMMU types *are*
>> currently supported - it will only indicate that the IOMMU type you requested is
>> not supported. So there's really no point in indicating the return value in case of
>> ret 0 - it is best to just mention that requested IOMMU type support is not
>> enabled in VFIO.
>>
>> Yes, you are right, I make a mistake.
>>
>>>>  			close(vfio_container_fd);
>>>>  			return -1;
>>>>  		}
>>>> @@ -783,11 +789,28 @@ pci_vfio_enable(void)  {
>>>>  	/* initialize group list */
>>>>  	int i;
>>>> +	int module_vfio_type1;
>>>>
>>>>  	for (i = 0; i < VFIO_MAX_GROUPS; i++) {
>>>>  		vfio_cfg.vfio_groups[i].fd = -1;
>>>>  		vfio_cfg.vfio_groups[i].group_no = -1;
>>>>  	}
>>>> +
>>>> +	module_vfio_type1 = rte_eal_check_module("vfio_iommu_type1");
>>>> +
>>>> +	/* return error directly */
>>>> +	if (module_vfio_type1 == -1) {
>>>> +		RTE_LOG(INFO, EAL, "Could not get loaded module
>>>> details!\n");
>>>> +		return -1;
>>>> +	}
>>>> +
>>>> +	/* return 0 if VFIO modules not loaded */
>>>> +	if (module_vfio_type1 == 0) {
>>>> +		RTE_LOG(INFO, EAL, "VFIO modules not all loaded,"
>>>> +			" skip VFIO support ...\n");
>>>> +		return 0;
>>>> +	}
>>>> +
>>>>  	vfio_cfg.vfio_container_fd = pci_vfio_get_container_fd();
>>>>
>>>>  	/* check if we have VFIO driver enabled */
>>>> --
>>>> 1.9.3
>

[dpdk-dev] [PATCH v5] VFIO: Avoid to enable vfio while the module not loaded

[Michael Qiu]

When vfio module is not loaded when kernel support vfio feature,
the routine still try to open the container to get file
description.

This action is not safe, and of cause got error messages:

EAL: Detected 40 lcore(s)
EAL:   unsupported IOMMU type!
EAL: VFIO support could not be initialized
EAL: Setting up memory...

This may make user confuse, this patch make it reasonable
and much more soomth to user.

Signed-off-by: Michael Qiu <michael.qiu@intel.com>
---
 v5 --> v4
	1. Move rte_eal_check_module() body to eal.c
	2. Clean up "unsupported IOMMU type" log

v4 --> v3:
	1. Remove RTE_LOG for params check
	2. Remove "vfio" module check as "vfio_iommu_type1"
	   loaded indecated "vfio" loaded

v3 --> v2:
        1. Add error log in rte_eal_check_module()
        2. Some code clean up.

v2 --> v1:
        1. Move check_module() from rte_common.h to eal_private.h
           and rename to rte_eal_check_module().
           To make it linuxapp only.
        2. Some code clean up.

 lib/librte_eal/common/eal_private.h        | 15 +++++++++++++++
 lib/librte_eal/linuxapp/eal/eal.c          | 27 +++++++++++++++++++++++++++
 lib/librte_eal/linuxapp/eal/eal_pci_vfio.c | 26 +++++++++++++++++++++++---
 3 files changed, 65 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
index 232fcec..4183b54 100644
--- a/lib/librte_eal/common/eal_private.h
+++ b/lib/librte_eal/common/eal_private.h
@@ -203,4 +203,19 @@ int rte_eal_alarm_init(void);
  */
 int rte_eal_dev_init(void);
 
+/**
+ * Function is to check if the kernel module(like, vfio, vfio_iommu_type1,
+ * etc.) loaded.
+ *
+ * @param module_name
+ *	The module's name which need to be checked
+ *
+ * @return
+ * 	-1 means some error happens(NULL pointer or open failure)
+ * 	0  means the module not loaded
+ * 	1  means the module loaded
+ */
+inline int
+rte_eal_check_module(const char *module_name);
+
 #endif /* _EAL_PRIVATE_H_ */
diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index 89f3b5e..40b462e 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -859,3 +859,30 @@ int rte_eal_has_hugepages(void)
 {
 	return ! internal_config.no_hugetlbfs;
 }
+
+inline int
+rte_eal_check_module(const char *module_name)
+{
+	char mod_name[30]; /* Any module names can be longer than 30 bytes? */
+	int ret = 0;
+
+	if (NULL == module_name)
+		return -1;
+
+	FILE * fd = fopen("/proc/modules", "r");
+	if (NULL == fd) {
+		RTE_LOG(ERR, EAL, "Open /proc/modules failed!"
+			" error %i (%s)\n", errno, strerror(errno));
+		return -1;
+	}
+	while(!feof(fd)) {
+		fscanf(fd, "%30s %*[^\n]", mod_name);
+		if(!strcmp(mod_name, module_name)) {
+			ret = 1;
+			break;
+		}
+	}
+	fclose(fd);
+
+	return ret;
+}
diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
index c1246e8..16fe10f 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
@@ -44,6 +44,7 @@
 #include <rte_tailq.h>
 #include <rte_eal_memconfig.h>
 #include <rte_malloc.h>
+#include <eal_private.h>
 
 #include "eal_filesystem.h"
 #include "eal_pci_init.h"
@@ -339,10 +340,12 @@ pci_vfio_get_container_fd(void)
 		ret = ioctl(vfio_container_fd, VFIO_CHECK_EXTENSION, VFIO_TYPE1_IOMMU);
 		if (ret != 1) {
 			if (ret < 0)
-				RTE_LOG(ERR, EAL, "  could not get IOMMU type, "
-						"error %i (%s)\n", errno, strerror(errno));
+				RTE_LOG(ERR, EAL, "  could not get IOMMU type,"
+					" error %i (%s)\n", errno,
+					strerror(errno));
 			else
-				RTE_LOG(ERR, EAL, "  unsupported IOMMU type!\n");
+				RTE_LOG(ERR, EAL, "  unsupported IOMMU type"
+					" detected in VFIO\n");
 			close(vfio_container_fd);
 			return -1;
 		}
@@ -783,11 +786,28 @@ pci_vfio_enable(void)
 {
 	/* initialize group list */
 	int i;
+	int module_vfio_type1;
 
 	for (i = 0; i < VFIO_MAX_GROUPS; i++) {
 		vfio_cfg.vfio_groups[i].fd = -1;
 		vfio_cfg.vfio_groups[i].group_no = -1;
 	}
+
+	module_vfio_type1 = rte_eal_check_module("vfio_iommu_type1");
+
+	/* return error directly */
+	if (module_vfio_type1 == -1) {
+		RTE_LOG(INFO, EAL, "Could not get loaded module details!\n");
+		return -1;
+	}
+
+	/* return 0 if VFIO modules not loaded */
+	if (module_vfio_type1 == 0) {
+		RTE_LOG(INFO, EAL, "VFIO modules not all loaded,"
+			" skip VFIO support ...\n");
+		return 0;
+	}
+
 	vfio_cfg.vfio_container_fd = pci_vfio_get_container_fd();
 
 	/* check if we have VFIO driver enabled */
-- 
1.9.3

Re: [dpdk-dev] [PATCH v5] VFIO: Avoid to enable vfio while the module not loaded

[Burakov, Anatoly]

> 
> When vfio module is not loaded when kernel support vfio feature, the
> routine still try to open the container to get file description.
> 
> This action is not safe, and of cause got error messages:
> 
> EAL: Detected 40 lcore(s)
> EAL:   unsupported IOMMU type!
> EAL: VFIO support could not be initialized
> EAL: Setting up memory...
> 
> This may make user confuse, this patch make it reasonable and much more
> soomth to user.
> 
> Signed-off-by: Michael Qiu <michael.qiu@intel.com>
> ---
>  v5 --> v4
> 	1. Move rte_eal_check_module() body to eal.c
> 	2. Clean up "unsupported IOMMU type" log
> 
> v4 --> v3:
> 	1. Remove RTE_LOG for params check
> 	2. Remove "vfio" module check as "vfio_iommu_type1"
> 	   loaded indecated "vfio" loaded
> 
> v3 --> v2:
>         1. Add error log in rte_eal_check_module()
>         2. Some code clean up.
> 
> v2 --> v1:
>         1. Move check_module() from rte_common.h to eal_private.h
>            and rename to rte_eal_check_module().
>            To make it linuxapp only.
>         2. Some code clean up.
> 
>  lib/librte_eal/common/eal_private.h        | 15 +++++++++++++++
>  lib/librte_eal/linuxapp/eal/eal.c          | 27 +++++++++++++++++++++++++++
>  lib/librte_eal/linuxapp/eal/eal_pci_vfio.c | 26
> +++++++++++++++++++++++---
>  3 files changed, 65 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_private.h
> b/lib/librte_eal/common/eal_private.h
> index 232fcec..4183b54 100644
> --- a/lib/librte_eal/common/eal_private.h
> +++ b/lib/librte_eal/common/eal_private.h
> @@ -203,4 +203,19 @@ int rte_eal_alarm_init(void);
>   */
>  int rte_eal_dev_init(void);
> 
> +/**
> + * Function is to check if the kernel module(like, vfio,
> +vfio_iommu_type1,
> + * etc.) loaded.
> + *
> + * @param module_name
> + *	The module's name which need to be checked
> + *
> + * @return
> + * 	-1 means some error happens(NULL pointer or open failure)
> + * 	0  means the module not loaded
> + * 	1  means the module loaded
> + */
> +inline int
> +rte_eal_check_module(const char *module_name);

Just curious - why inline?

> +
>  #endif /* _EAL_PRIVATE_H_ */
> diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
> index 89f3b5e..40b462e 100644
> --- a/lib/librte_eal/linuxapp/eal/eal.c
> +++ b/lib/librte_eal/linuxapp/eal/eal.c
> @@ -859,3 +859,30 @@ int rte_eal_has_hugepages(void)  {
>  	return ! internal_config.no_hugetlbfs;  }
> +
> +inline int
> +rte_eal_check_module(const char *module_name) {
> +	char mod_name[30]; /* Any module names can be longer than 30
> bytes? */
> +	int ret = 0;
> +
> +	if (NULL == module_name)
> +		return -1;
> +
> +	FILE * fd = fopen("/proc/modules", "r");
> +	if (NULL == fd) {
> +		RTE_LOG(ERR, EAL, "Open /proc/modules failed!"
> +			" error %i (%s)\n", errno, strerror(errno));
> +		return -1;
> +	}
> +	while(!feof(fd)) {
> +		fscanf(fd, "%30s %*[^\n]", mod_name);

As far as I know, in fscanf terms, %30s will result in at most 30 character string, i.e. 31 bytes (30 characters + null terminator). So it probably should be %29s.

> +		if(!strcmp(mod_name, module_name)) {
> +			ret = 1;
> +			break;
> +		}
> +	}
> +	fclose(fd);
> +
> +	return ret;
> +}
> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> index c1246e8..16fe10f 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> @@ -44,6 +44,7 @@
>  #include <rte_tailq.h>
>  #include <rte_eal_memconfig.h>
>  #include <rte_malloc.h>
> +#include <eal_private.h>
> 
>  #include "eal_filesystem.h"
>  #include "eal_pci_init.h"
> @@ -339,10 +340,12 @@ pci_vfio_get_container_fd(void)
>  		ret = ioctl(vfio_container_fd, VFIO_CHECK_EXTENSION,
> VFIO_TYPE1_IOMMU);
>  		if (ret != 1) {
>  			if (ret < 0)
> -				RTE_LOG(ERR, EAL, "  could not get IOMMU
> type, "
> -						"error %i (%s)\n", errno,
> strerror(errno));
> +				RTE_LOG(ERR, EAL, "  could not get IOMMU
> type,"
> +					" error %i (%s)\n", errno,
> +					strerror(errno));
>  			else
> -				RTE_LOG(ERR, EAL, "  unsupported IOMMU
> type!\n");
> +				RTE_LOG(ERR, EAL, "  unsupported IOMMU
> type"
> +					" detected in VFIO\n");
>  			close(vfio_container_fd);
>  			return -1;
>  		}
> @@ -783,11 +786,28 @@ pci_vfio_enable(void)  {
>  	/* initialize group list */
>  	int i;
> +	int module_vfio_type1;
> 
>  	for (i = 0; i < VFIO_MAX_GROUPS; i++) {
>  		vfio_cfg.vfio_groups[i].fd = -1;
>  		vfio_cfg.vfio_groups[i].group_no = -1;
>  	}
> +
> +	module_vfio_type1 = rte_eal_check_module("vfio_iommu_type1");
> +
> +	/* return error directly */
> +	if (module_vfio_type1 == -1) {
> +		RTE_LOG(INFO, EAL, "Could not get loaded module
> details!\n");
> +		return -1;
> +	}
> +
> +	/* return 0 if VFIO modules not loaded */
> +	if (module_vfio_type1 == 0) {
> +		RTE_LOG(INFO, EAL, "VFIO modules not all loaded,"
> +			" skip VFIO support ...\n");
> +		return 0;
> +	}
> +
>  	vfio_cfg.vfio_container_fd = pci_vfio_get_container_fd();
> 
>  	/* check if we have VFIO driver enabled */
> --
> 1.9.3

Re: [dpdk-dev] [PATCH v5] VFIO: Avoid to enable vfio while the module not loaded

[Qiu, Michael]

On 12/10/2014 5:21 PM, Burakov, Anatoly wrote:
>> When vfio module is not loaded when kernel support vfio feature, the
>> routine still try to open the container to get file description.
>>
>> This action is not safe, and of cause got error messages:
>>
>> EAL: Detected 40 lcore(s)
>> EAL:   unsupported IOMMU type!
>> EAL: VFIO support could not be initialized
>> EAL: Setting up memory...
>>
>> This may make user confuse, this patch make it reasonable and much more
>> soomth to user.
>>
>> Signed-off-by: Michael Qiu <michael.qiu@intel.com>
>> ---
>>  v5 --> v4
>> 	1. Move rte_eal_check_module() body to eal.c
>> 	2. Clean up "unsupported IOMMU type" log
>>
>> v4 --> v3:
>> 	1. Remove RTE_LOG for params check
>> 	2. Remove "vfio" module check as "vfio_iommu_type1"
>> 	   loaded indecated "vfio" loaded
>>
>> v3 --> v2:
>>         1. Add error log in rte_eal_check_module()
>>         2. Some code clean up.
>>
>> v2 --> v1:
>>         1. Move check_module() from rte_common.h to eal_private.h
>>            and rename to rte_eal_check_module().
>>            To make it linuxapp only.
>>         2. Some code clean up.
>>
>>  lib/librte_eal/common/eal_private.h        | 15 +++++++++++++++
>>  lib/librte_eal/linuxapp/eal/eal.c          | 27 +++++++++++++++++++++++++++
>>  lib/librte_eal/linuxapp/eal/eal_pci_vfio.c | 26
>> +++++++++++++++++++++++---
>>  3 files changed, 65 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/librte_eal/common/eal_private.h
>> b/lib/librte_eal/common/eal_private.h
>> index 232fcec..4183b54 100644
>> --- a/lib/librte_eal/common/eal_private.h
>> +++ b/lib/librte_eal/common/eal_private.h
>> @@ -203,4 +203,19 @@ int rte_eal_alarm_init(void);
>>   */
>>  int rte_eal_dev_init(void);
>>
>> +/**
>> + * Function is to check if the kernel module(like, vfio,
>> +vfio_iommu_type1,
>> + * etc.) loaded.
>> + *
>> + * @param module_name
>> + *	The module's name which need to be checked
>> + *
>> + * @return
>> + * 	-1 means some error happens(NULL pointer or open failure)
>> + * 	0  means the module not loaded
>> + * 	1  means the module loaded
>> + */
>> +inline int
>> +rte_eal_check_module(const char *module_name);
> Just curious - why inline?

Just want to make it inline, no strong reason. If you do not agree I
will remove.
>
>> +
>>  #endif /* _EAL_PRIVATE_H_ */
>> diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
>> index 89f3b5e..40b462e 100644
>> --- a/lib/librte_eal/linuxapp/eal/eal.c
>> +++ b/lib/librte_eal/linuxapp/eal/eal.c
>> @@ -859,3 +859,30 @@ int rte_eal_has_hugepages(void)  {
>>  	return ! internal_config.no_hugetlbfs;  }
>> +
>> +inline int
>> +rte_eal_check_module(const char *module_name) {
>> +	char mod_name[30]; /* Any module names can be longer than 30
>> bytes? */
>> +	int ret = 0;
>> +
>> +	if (NULL == module_name)
>> +		return -1;
>> +
>> +	FILE * fd = fopen("/proc/modules", "r");
>> +	if (NULL == fd) {
>> +		RTE_LOG(ERR, EAL, "Open /proc/modules failed!"
>> +			" error %i (%s)\n", errno, strerror(errno));
>> +		return -1;
>> +	}
>> +	while(!feof(fd)) {
>> +		fscanf(fd, "%30s %*[^\n]", mod_name);
> As far as I know, in fscanf terms, %30s will result in at most 30 character string, i.e. 31 bytes (30 characters + null terminator). So it probably should be %29s.

Good catch, you are right.

Thanks,
Michael
>
>> +		if(!strcmp(mod_name, module_name)) {
>> +			ret = 1;
>> +			break;
>> +		}
>> +	}
>> +	fclose(fd);
>> +
>> +	return ret;
>> +}
>> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
>> b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
>> index c1246e8..16fe10f 100644
>> --- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
>> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
>> @@ -44,6 +44,7 @@
>>  #include <rte_tailq.h>
>>  #include <rte_eal_memconfig.h>
>>  #include <rte_malloc.h>
>> +#include <eal_private.h>
>>
>>  #include "eal_filesystem.h"
>>  #include "eal_pci_init.h"
>> @@ -339,10 +340,12 @@ pci_vfio_get_container_fd(void)
>>  		ret = ioctl(vfio_container_fd, VFIO_CHECK_EXTENSION,
>> VFIO_TYPE1_IOMMU);
>>  		if (ret != 1) {
>>  			if (ret < 0)
>> -				RTE_LOG(ERR, EAL, "  could not get IOMMU
>> type, "
>> -						"error %i (%s)\n", errno,
>> strerror(errno));
>> +				RTE_LOG(ERR, EAL, "  could not get IOMMU
>> type,"
>> +					" error %i (%s)\n", errno,
>> +					strerror(errno));
>>  			else
>> -				RTE_LOG(ERR, EAL, "  unsupported IOMMU
>> type!\n");
>> +				RTE_LOG(ERR, EAL, "  unsupported IOMMU
>> type"
>> +					" detected in VFIO\n");
>>  			close(vfio_container_fd);
>>  			return -1;
>>  		}
>> @@ -783,11 +786,28 @@ pci_vfio_enable(void)  {
>>  	/* initialize group list */
>>  	int i;
>> +	int module_vfio_type1;
>>
>>  	for (i = 0; i < VFIO_MAX_GROUPS; i++) {
>>  		vfio_cfg.vfio_groups[i].fd = -1;
>>  		vfio_cfg.vfio_groups[i].group_no = -1;
>>  	}
>> +
>> +	module_vfio_type1 = rte_eal_check_module("vfio_iommu_type1");
>> +
>> +	/* return error directly */
>> +	if (module_vfio_type1 == -1) {
>> +		RTE_LOG(INFO, EAL, "Could not get loaded module
>> details!\n");
>> +		return -1;
>> +	}
>> +
>> +	/* return 0 if VFIO modules not loaded */
>> +	if (module_vfio_type1 == 0) {
>> +		RTE_LOG(INFO, EAL, "VFIO modules not all loaded,"
>> +			" skip VFIO support ...\n");
>> +		return 0;
>> +	}
>> +
>>  	vfio_cfg.vfio_container_fd = pci_vfio_get_container_fd();
>>
>>  	/* check if we have VFIO driver enabled */
>> --
>> 1.9.3
>

[dpdk-dev] [PATCH v6] VFIO: Avoid to enable vfio while the module not loaded

[Michael Qiu]

When vfio module is not loaded when kernel support vfio feature,
the routine still try to open the container to get file
description.

This action is not safe, and of cause got error messages:

EAL: Detected 40 lcore(s)
EAL:   unsupported IOMMU type!
EAL: VFIO support could not be initialized
EAL: Setting up memory...

This may make user confuse, this patch make it reasonable
and much more soomth to user.

Signed-off-by: Michael Qiu <michael.qiu@intel.com>
---
 v6 --> v5
	1. Change rte_eal_check_module() to normal
	   function instead of inline
	2. limit fscanf to get 29 charactors not include '\0'

 lib/librte_eal/common/eal_private.h        | 14 ++++++++++++++
 lib/librte_eal/linuxapp/eal/eal.c          | 27 +++++++++++++++++++++++++++
 lib/librte_eal/linuxapp/eal/eal_pci_vfio.c | 26 +++++++++++++++++++++++---
 3 files changed, 64 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
index 232fcec..2c751c6 100644
--- a/lib/librte_eal/common/eal_private.h
+++ b/lib/librte_eal/common/eal_private.h
@@ -203,4 +203,18 @@ int rte_eal_alarm_init(void);
  */
 int rte_eal_dev_init(void);
 
+/**
+ * Function is to check if the kernel module(like, vfio, vfio_iommu_type1,
+ * etc.) loaded.
+ *
+ * @param module_name
+ *	The module's name which need to be checked
+ *
+ * @return
+ * 	-1 means some error happens(NULL pointer or open failure)
+ * 	0  means the module not loaded
+ * 	1  means the module loaded
+ */
+int rte_eal_check_module(const char *module_name);
+
 #endif /* _EAL_PRIVATE_H_ */
diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index 89f3b5e..9c1a1cc 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -859,3 +859,30 @@ int rte_eal_has_hugepages(void)
 {
 	return ! internal_config.no_hugetlbfs;
 }
+
+int
+rte_eal_check_module(const char *module_name)
+{
+	char mod_name[30]; /* Any module names can be longer than 30 bytes? */
+	int ret = 0;
+
+	if (NULL == module_name)
+		return -1;
+
+	FILE * fd = fopen("/proc/modules", "r");
+	if (NULL == fd) {
+		RTE_LOG(ERR, EAL, "Open /proc/modules failed!"
+			" error %i (%s)\n", errno, strerror(errno));
+		return -1;
+	}
+	while(!feof(fd)) {
+		fscanf(fd, "%29s %*[^\n]", mod_name);
+		if(!strcmp(mod_name, module_name)) {
+			ret = 1;
+			break;
+		}
+	}
+	fclose(fd);
+
+	return ret;
+}
diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
index c1246e8..16fe10f 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
@@ -44,6 +44,7 @@
 #include <rte_tailq.h>
 #include <rte_eal_memconfig.h>
 #include <rte_malloc.h>
+#include <eal_private.h>
 
 #include "eal_filesystem.h"
 #include "eal_pci_init.h"
@@ -339,10 +340,12 @@ pci_vfio_get_container_fd(void)
 		ret = ioctl(vfio_container_fd, VFIO_CHECK_EXTENSION, VFIO_TYPE1_IOMMU);
 		if (ret != 1) {
 			if (ret < 0)
-				RTE_LOG(ERR, EAL, "  could not get IOMMU type, "
-						"error %i (%s)\n", errno, strerror(errno));
+				RTE_LOG(ERR, EAL, "  could not get IOMMU type,"
+					" error %i (%s)\n", errno,
+					strerror(errno));
 			else
-				RTE_LOG(ERR, EAL, "  unsupported IOMMU type!\n");
+				RTE_LOG(ERR, EAL, "  unsupported IOMMU type"
+					" detected in VFIO\n");
 			close(vfio_container_fd);
 			return -1;
 		}
@@ -783,11 +786,28 @@ pci_vfio_enable(void)
 {
 	/* initialize group list */
 	int i;
+	int module_vfio_type1;
 
 	for (i = 0; i < VFIO_MAX_GROUPS; i++) {
 		vfio_cfg.vfio_groups[i].fd = -1;
 		vfio_cfg.vfio_groups[i].group_no = -1;
 	}
+
+	module_vfio_type1 = rte_eal_check_module("vfio_iommu_type1");
+
+	/* return error directly */
+	if (module_vfio_type1 == -1) {
+		RTE_LOG(INFO, EAL, "Could not get loaded module details!\n");
+		return -1;
+	}
+
+	/* return 0 if VFIO modules not loaded */
+	if (module_vfio_type1 == 0) {
+		RTE_LOG(INFO, EAL, "VFIO modules not all loaded,"
+			" skip VFIO support ...\n");
+		return 0;
+	}
+
 	vfio_cfg.vfio_container_fd = pci_vfio_get_container_fd();
 
 	/* check if we have VFIO driver enabled */
-- 
1.9.3

Re: [dpdk-dev] [PATCH v6] VFIO: Avoid to enable vfio while the module not loaded

[Burakov, Anatoly]

> When vfio module is not loaded when kernel support vfio feature, the
> routine still try to open the container to get file description.
> 
> This action is not safe, and of cause got error messages:
> 
> EAL: Detected 40 lcore(s)
> EAL:   unsupported IOMMU type!
> EAL: VFIO support could not be initialized
> EAL: Setting up memory...
> 
> This may make user confuse, this patch make it reasonable and much more
> soomth to user.
> 
> Signed-off-by: Michael Qiu <michael.qiu@intel.com>
> ---
>  v6 --> v5
> 	1. Change rte_eal_check_module() to normal
> 	   function instead of inline
> 	2. limit fscanf to get 29 charactors not include '\0'
> 
>  lib/librte_eal/common/eal_private.h        | 14 ++++++++++++++
>  lib/librte_eal/linuxapp/eal/eal.c          | 27 +++++++++++++++++++++++++++
>  lib/librte_eal/linuxapp/eal/eal_pci_vfio.c | 26
> +++++++++++++++++++++++---
>  3 files changed, 64 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_private.h
> b/lib/librte_eal/common/eal_private.h
> index 232fcec..2c751c6 100644
> --- a/lib/librte_eal/common/eal_private.h
> +++ b/lib/librte_eal/common/eal_private.h
> @@ -203,4 +203,18 @@ int rte_eal_alarm_init(void);
>   */
>  int rte_eal_dev_init(void);
> 
> +/**
> + * Function is to check if the kernel module(like, vfio,
> +vfio_iommu_type1,
> + * etc.) loaded.
> + *
> + * @param module_name
> + *	The module's name which need to be checked
> + *
> + * @return
> + * 	-1 means some error happens(NULL pointer or open failure)
> + * 	0  means the module not loaded
> + * 	1  means the module loaded
> + */
> +int rte_eal_check_module(const char *module_name);
> +
>  #endif /* _EAL_PRIVATE_H_ */
> diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
> index 89f3b5e..9c1a1cc 100644
> --- a/lib/librte_eal/linuxapp/eal/eal.c
> +++ b/lib/librte_eal/linuxapp/eal/eal.c
> @@ -859,3 +859,30 @@ int rte_eal_has_hugepages(void)  {
>  	return ! internal_config.no_hugetlbfs;  }
> +
> +int
> +rte_eal_check_module(const char *module_name) {
> +	char mod_name[30]; /* Any module names can be longer than 30
> bytes? */
> +	int ret = 0;
> +
> +	if (NULL == module_name)
> +		return -1;
> +
> +	FILE * fd = fopen("/proc/modules", "r");
> +	if (NULL == fd) {
> +		RTE_LOG(ERR, EAL, "Open /proc/modules failed!"
> +			" error %i (%s)\n", errno, strerror(errno));
> +		return -1;
> +	}
> +	while(!feof(fd)) {
> +		fscanf(fd, "%29s %*[^\n]", mod_name);
> +		if(!strcmp(mod_name, module_name)) {
> +			ret = 1;
> +			break;
> +		}
> +	}
> +	fclose(fd);
> +
> +	return ret;
> +}
> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> index c1246e8..16fe10f 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> @@ -44,6 +44,7 @@
>  #include <rte_tailq.h>
>  #include <rte_eal_memconfig.h>
>  #include <rte_malloc.h>
> +#include <eal_private.h>
> 
>  #include "eal_filesystem.h"
>  #include "eal_pci_init.h"
> @@ -339,10 +340,12 @@ pci_vfio_get_container_fd(void)
>  		ret = ioctl(vfio_container_fd, VFIO_CHECK_EXTENSION,
> VFIO_TYPE1_IOMMU);
>  		if (ret != 1) {
>  			if (ret < 0)
> -				RTE_LOG(ERR, EAL, "  could not get IOMMU
> type, "
> -						"error %i (%s)\n", errno,
> strerror(errno));
> +				RTE_LOG(ERR, EAL, "  could not get IOMMU
> type,"
> +					" error %i (%s)\n", errno,
> +					strerror(errno));
>  			else
> -				RTE_LOG(ERR, EAL, "  unsupported IOMMU
> type!\n");
> +				RTE_LOG(ERR, EAL, "  unsupported IOMMU
> type"
> +					" detected in VFIO\n");
>  			close(vfio_container_fd);
>  			return -1;
>  		}
> @@ -783,11 +786,28 @@ pci_vfio_enable(void)  {
>  	/* initialize group list */
>  	int i;
> +	int module_vfio_type1;
> 
>  	for (i = 0; i < VFIO_MAX_GROUPS; i++) {
>  		vfio_cfg.vfio_groups[i].fd = -1;
>  		vfio_cfg.vfio_groups[i].group_no = -1;
>  	}
> +
> +	module_vfio_type1 = rte_eal_check_module("vfio_iommu_type1");
> +
> +	/* return error directly */
> +	if (module_vfio_type1 == -1) {
> +		RTE_LOG(INFO, EAL, "Could not get loaded module
> details!\n");
> +		return -1;
> +	}
> +
> +	/* return 0 if VFIO modules not loaded */
> +	if (module_vfio_type1 == 0) {
> +		RTE_LOG(INFO, EAL, "VFIO modules not all loaded,"
> +			" skip VFIO support ...\n");
> +		return 0;
> +	}
> +
>  	vfio_cfg.vfio_container_fd = pci_vfio_get_container_fd();
> 
>  	/* check if we have VFIO driver enabled */
> --
> 1.9.3

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

Re: [dpdk-dev] [PATCH v6] VFIO: Avoid to enable vfio while the module not loaded

[Qiu, Michael]

Hi Thomas,

Will you plan to merge this patch? If yes,   you plan to merge it into
1.8 or 2.0?

Thanks,
Michael
On 12/10/2014 7:48 PM, Burakov, Anatoly wrote:
>> When vfio module is not loaded when kernel support vfio feature, the
>> routine still try to open the container to get file description.
>>
>> This action is not safe, and of cause got error messages:
>>
>> EAL: Detected 40 lcore(s)
>> EAL:   unsupported IOMMU type!
>> EAL: VFIO support could not be initialized
>> EAL: Setting up memory...
>>
>> This may make user confuse, this patch make it reasonable and much more
>> soomth to user.
>>
>> Signed-off-by: Michael Qiu <michael.qiu@intel.com>
>> ---
>>  v6 --> v5
>> 	1. Change rte_eal_check_module() to normal
>> 	   function instead of inline
>> 	2. limit fscanf to get 29 charactors not include '\0'
>>
>>  lib/librte_eal/common/eal_private.h        | 14 ++++++++++++++
>>  lib/librte_eal/linuxapp/eal/eal.c          | 27 +++++++++++++++++++++++++++
>>  lib/librte_eal/linuxapp/eal/eal_pci_vfio.c | 26
>> +++++++++++++++++++++++---
>>  3 files changed, 64 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/librte_eal/common/eal_private.h
>> b/lib/librte_eal/common/eal_private.h
>> index 232fcec..2c751c6 100644
>> --- a/lib/librte_eal/common/eal_private.h
>> +++ b/lib/librte_eal/common/eal_private.h
>> @@ -203,4 +203,18 @@ int rte_eal_alarm_init(void);
>>   */
>>  int rte_eal_dev_init(void);
>>
>> +/**
>> + * Function is to check if the kernel module(like, vfio,
>> +vfio_iommu_type1,
>> + * etc.) loaded.
>> + *
>> + * @param module_name
>> + *	The module's name which need to be checked
>> + *
>> + * @return
>> + * 	-1 means some error happens(NULL pointer or open failure)
>> + * 	0  means the module not loaded
>> + * 	1  means the module loaded
>> + */
>> +int rte_eal_check_module(const char *module_name);
>> +
>>  #endif /* _EAL_PRIVATE_H_ */
>> diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
>> index 89f3b5e..9c1a1cc 100644
>> --- a/lib/librte_eal/linuxapp/eal/eal.c
>> +++ b/lib/librte_eal/linuxapp/eal/eal.c
>> @@ -859,3 +859,30 @@ int rte_eal_has_hugepages(void)  {
>>  	return ! internal_config.no_hugetlbfs;  }
>> +
>> +int
>> +rte_eal_check_module(const char *module_name) {
>> +	char mod_name[30]; /* Any module names can be longer than 30
>> bytes? */
>> +	int ret = 0;
>> +
>> +	if (NULL == module_name)
>> +		return -1;
>> +
>> +	FILE * fd = fopen("/proc/modules", "r");
>> +	if (NULL == fd) {
>> +		RTE_LOG(ERR, EAL, "Open /proc/modules failed!"
>> +			" error %i (%s)\n", errno, strerror(errno));
>> +		return -1;
>> +	}
>> +	while(!feof(fd)) {
>> +		fscanf(fd, "%29s %*[^\n]", mod_name);
>> +		if(!strcmp(mod_name, module_name)) {
>> +			ret = 1;
>> +			break;
>> +		}
>> +	}
>> +	fclose(fd);
>> +
>> +	return ret;
>> +}
>> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
>> b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
>> index c1246e8..16fe10f 100644
>> --- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
>> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
>> @@ -44,6 +44,7 @@
>>  #include <rte_tailq.h>
>>  #include <rte_eal_memconfig.h>
>>  #include <rte_malloc.h>
>> +#include <eal_private.h>
>>
>>  #include "eal_filesystem.h"
>>  #include "eal_pci_init.h"
>> @@ -339,10 +340,12 @@ pci_vfio_get_container_fd(void)
>>  		ret = ioctl(vfio_container_fd, VFIO_CHECK_EXTENSION,
>> VFIO_TYPE1_IOMMU);
>>  		if (ret != 1) {
>>  			if (ret < 0)
>> -				RTE_LOG(ERR, EAL, "  could not get IOMMU
>> type, "
>> -						"error %i (%s)\n", errno,
>> strerror(errno));
>> +				RTE_LOG(ERR, EAL, "  could not get IOMMU
>> type,"
>> +					" error %i (%s)\n", errno,
>> +					strerror(errno));
>>  			else
>> -				RTE_LOG(ERR, EAL, "  unsupported IOMMU
>> type!\n");
>> +				RTE_LOG(ERR, EAL, "  unsupported IOMMU
>> type"
>> +					" detected in VFIO\n");
>>  			close(vfio_container_fd);
>>  			return -1;
>>  		}
>> @@ -783,11 +786,28 @@ pci_vfio_enable(void)  {
>>  	/* initialize group list */
>>  	int i;
>> +	int module_vfio_type1;
>>
>>  	for (i = 0; i < VFIO_MAX_GROUPS; i++) {
>>  		vfio_cfg.vfio_groups[i].fd = -1;
>>  		vfio_cfg.vfio_groups[i].group_no = -1;
>>  	}
>> +
>> +	module_vfio_type1 = rte_eal_check_module("vfio_iommu_type1");
>> +
>> +	/* return error directly */
>> +	if (module_vfio_type1 == -1) {
>> +		RTE_LOG(INFO, EAL, "Could not get loaded module
>> details!\n");
>> +		return -1;
>> +	}
>> +
>> +	/* return 0 if VFIO modules not loaded */
>> +	if (module_vfio_type1 == 0) {
>> +		RTE_LOG(INFO, EAL, "VFIO modules not all loaded,"
>> +			" skip VFIO support ...\n");
>> +		return 0;
>> +	}
>> +
>>  	vfio_cfg.vfio_container_fd = pci_vfio_get_container_fd();
>>
>>  	/* check if we have VFIO driver enabled */
>> --
>> 1.9.3
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
>
>

Re: [dpdk-dev] [PATCH v6] VFIO: Avoid to enable vfio while the module not loaded

[Thomas Monjalon]

2014-12-19 07:09, Qiu, Michael:
> Hi Thomas,
> 
> Will you plan to merge this patch? If yes,   you plan to merge it into
> 1.8 or 2.0?

It won't be in 1.8 for 2 reasons:
- there is  a risk to break something in some environments, so need more tests
- you add a function protoype in eal common without bsd implementation

-- 
Thomas

Re: [dpdk-dev] [PATCH v6] VFIO: Avoid to enable vfio while the module not loaded

[Qiu, Michael]

On 12/19/2014 4:24 PM, Thomas Monjalon wrote:
> 2014-12-19 07:09, Qiu, Michael:
>> Hi Thomas,
>>
>> Will you plan to merge this patch? If yes,   you plan to merge it into
>> 1.8 or 2.0?
> It won't be in 1.8 for 2 reasons:
> - there is  a risk to break something in some environments, so need more tests
> - you add a function protoype in eal common without bsd implementation

OK, fair enough, I check with you just as a reminder to be ensure that
you do not miss this thread.

Re: [dpdk-dev] [PATCH v6] VFIO: Avoid to enable vfio while the module not loaded

[Thomas Monjalon]

> > When vfio module is not loaded when kernel support vfio feature, the
> > routine still try to open the container to get file description.
> > 
> > This action is not safe, and of cause got error messages:
> > 
> > EAL: Detected 40 lcore(s)
> > EAL:   unsupported IOMMU type!
> > EAL: VFIO support could not be initialized
> > EAL: Setting up memory...
> > 
> > This may make user confuse, this patch make it reasonable and much more
> > soomth to user.
> > 
> > Signed-off-by: Michael Qiu <michael.qiu@intel.com>
> 
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

Note that rte_eal_check_module has no bsd counterpart.
It could be needed later.

Applied

Thanks
-- 
Thomas

Re: [dpdk-dev] [PATCH v6] VFIO: Avoid to enable vfio while the module not loaded

[Burakov, Anatoly]

Yep, apologies, it's my fault as it was my suggestion. I knew there was a linuxapp-only EAL header, for some reason I thought it's eal_private. Any suggestions on where to put this function? I don't think BSD needs this function. 

Thanks,
Anatoly

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Thursday, January 15, 2015 1:38 PM
> To: Qiu, Michael
> Cc: Burakov, Anatoly; dev@dpdk.org; Xie, Huawei
> Subject: Re: [PATCH v6] VFIO: Avoid to enable vfio while the module not
> loaded
> 
> > > When vfio module is not loaded when kernel support vfio feature, the
> > > routine still try to open the container to get file description.
> > >
> > > This action is not safe, and of cause got error messages:
> > >
> > > EAL: Detected 40 lcore(s)
> > > EAL:   unsupported IOMMU type!
> > > EAL: VFIO support could not be initialized
> > > EAL: Setting up memory...
> > >
> > > This may make user confuse, this patch make it reasonable and much
> > > more soomth to user.
> > >
> > > Signed-off-by: Michael Qiu <michael.qiu@intel.com>
> >
> > Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
> 
> Note that rte_eal_check_module has no bsd counterpart.
> It could be needed later.
> 
> Applied
> 
> Thanks
> --
> Thomas

Re: [dpdk-dev] [PATCH v6] VFIO: Avoid to enable vfio while the module not loaded

[Thomas Monjalon]

2015-01-15 13:42, Burakov, Anatoly:
> Yep, apologies, it's my fault as it was my suggestion.
> I knew there was a linuxapp-only EAL header, for some reason I thought it's eal_private.
> Any suggestions on where to put this function? I don't think BSD needs this function. 

No, it's OK. I think it could be needed in bsd if a PMD
depends on a kernel driver or try to unload one.

PS: please don't top post
-- 
Thomas


> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > Sent: Thursday, January 15, 2015 1:38 PM
> > To: Qiu, Michael
> > Cc: Burakov, Anatoly; dev@dpdk.org; Xie, Huawei
> > Subject: Re: [PATCH v6] VFIO: Avoid to enable vfio while the module not
> > loaded
> > 
> > > > When vfio module is not loaded when kernel support vfio feature, the
> > > > routine still try to open the container to get file description.
> > > >
> > > > This action is not safe, and of cause got error messages:
> > > >
> > > > EAL: Detected 40 lcore(s)
> > > > EAL:   unsupported IOMMU type!
> > > > EAL: VFIO support could not be initialized
> > > > EAL: Setting up memory...
> > > >
> > > > This may make user confuse, this patch make it reasonable and much
> > > > more soomth to user.
> > > >
> > > > Signed-off-by: Michael Qiu <michael.qiu@intel.com>
> > >
> > > Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
> > 
> > Note that rte_eal_check_module has no bsd counterpart.
> > It could be needed later.
> > 
> > Applied
> > 
> > Thanks
> > --
> > Thomas

Re: [dpdk-dev] [PATCH] VFIO: Avoid to enable vfio while the module not loaded

[Burakov, Anatoly]

Hi Michael

> When vfio module is not loaded when kernel support vfio feature, the
> routine still try to open the container to get file description.
> 
> This action is not safe, and of cause got error messages:
> 
> EAL: Detected 40 lcore(s)
> EAL:   unsupported IOMMU type!
> EAL: VFIO support could not be initialized
> EAL: Setting up memory...
> 
> This may make user confuse, this patch make it reasonable and much more
> soomth to user.

 Not sure I agree with the premise of this patch.

First of all, if VFIO driver is not enabled, the container file would not be present and you would get a different error (namely, "cannot open VFIO container", in pci_vfio_get_container_fd()). If you have a container file,  that means VFIO driver is loaded, so I'm not sure why you get the "unsupported IOMMU type" error. I suppose it could happen when vfio is loaded but vfio_iommu_type1 isn't?

And even then, this error is harmless and doesn't do anything, so I'm not sure what this patch is supposed to fix. The error messages tells the user exactly what happens.
 
Thanks,
Anatoly 

Re: [dpdk-dev] [PATCH] VFIO: Avoid to enable vfio while the module not loaded

[Qiu, Michael]

On 12/4/2014 9:12 PM, Burakov, Anatoly wrote:
> Hi Michael
>
>> When vfio module is not loaded when kernel support vfio feature, the
>> routine still try to open the container to get file description.
>>
>> This action is not safe, and of cause got error messages:
>>
>> EAL: Detected 40 lcore(s)
>> EAL:   unsupported IOMMU type!
>> EAL: VFIO support could not be initialized
>> EAL: Setting up memory...
>>
>> This may make user confuse, this patch make it reasonable and much more
>> soomth to user.
>  Not sure I agree with the premise of this patch.
>
> First of all, if VFIO driver is not enabled, the container file would not be present and you would get a different error (namely, "cannot open VFIO container", in pci_vfio_get_container_fd()). If you have a container file,  that means VFIO driver is loaded, so I'm not sure why you get the "unsupported IOMMU type" error. I suppose it could happen when vfio is loaded but vfio_iommu_type1 isn't?

But indeed, when try to unload both vfio and vfio_iommu_type1,
/dev/vfio/vfio still there, I'm also surprise.

My ENV is fedora20, kernel version 3.6.7-200 X86_64.

Believe or not, you can have a try, it seems a kernel issue.

When you unload both two modules, then open /dev/vfio/vfio, you will
find it can be opened with no errors(but this time both two modules
loaded automatically, strange enough)

Also you can use ioctl to get API Version. But when you try to get the
iommu type, it will return a "0" not expect value of  '1'.

Then you can shutdown DPDK, reopen like test-pmd, all works fine :)

I will take a deep look at in the kernel side, to find out why this happens.

Thanks,
Michael
> And even then, this error is harmless and doesn't do anything, so I'm not sure what this patch is supposed to fix. The error messages tells the user exactly what happens.
>  
> Thanks,
> Anatoly 
>

Re: [dpdk-dev] [PATCH] VFIO: Avoid to enable vfio while the module not loaded

[Burakov, Anatoly]

Hi Michael

> But indeed, when try to unload both vfio and vfio_iommu_type1,
> /dev/vfio/vfio still there, I'm also surprise.
> 
> My ENV is fedora20, kernel version 3.6.7-200 X86_64.
> 
> Believe or not, you can have a try, it seems a kernel issue.
> 
> When you unload both two modules, then open /dev/vfio/vfio, you will find
> it can be opened with no errors(but this time both two modules loaded
> automatically, strange enough)
> 

Thanks to Sergio, we found a most likely cause for this. This patch to Linux kernel by Alex Williamson of Red Hat:

https://lkml.org/lkml/2013/12/12/421

it seems, however, that it has been merged into 3.14. Your kernel, by your own admission, is 3.6. Are you sure this is the right kernel version? Because my own machine has Fedora 18 with a 3.11 kernel, and it (correctly) does not display this behavior. So unless Fedora 20 backported those changes to kernel 3.6, this shouldn't happen on your set up. (but it doesn't really matter, just FYI - the patch still should be fixed and resubmitted, just as we discussed)

Thanks,
Anatoly

Re: [dpdk-dev] [PATCH] VFIO: Avoid to enable vfio while the module not loaded

[Qiu, Michael]

On 12/5/2014 12:31 AM, Burakov, Anatoly wrote:
> Hi Michael
>
>> But indeed, when try to unload both vfio and vfio_iommu_type1,
>> /dev/vfio/vfio still there, I'm also surprise.
>>
>> My ENV is fedora20, kernel version 3.6.7-200 X86_64.
>>
>> Believe or not, you can have a try, it seems a kernel issue.
>>
>> When you unload both two modules, then open /dev/vfio/vfio, you will find
>> it can be opened with no errors(but this time both two modules loaded
>> automatically, strange enough)
>>
> Thanks to Sergio, we found a most likely cause for this. This patch to Linux kernel by Alex Williamson of Red Hat:
>
> https://lkml.org/lkml/2013/12/12/421
>
> it seems, however, that it has been merged into 3.14. Your kernel, by your own admission, is 3.6. Are you sure this is the right kernel version? Because my own machine has Fedora 18 with a 3.11 kernel, and it (correctly) does not 

Sorry, the kernel version is 3.16 :), just make a mistake :)

Thanks,
Michael
> display this behavior. So unless Fedora 20 backported those changes to kernel 3.6, this shouldn't happen on your set up. (but it doesn't really matter, just FYI - the patch still should be fixed and resubmitted, just as we discussed)
>
> Thanks,
> Anatoly
>

Re: [dpdk-dev] [PATCH] VFIO: Avoid to enable vfio while the module not loaded

[Burakov, Anatoly]

Hi Michael

> When vfio module is not loaded when kernel support vfio feature, the
> routine still try to open the container to get file description.
> 
> This action is not safe, and of cause got error messages:
> 
> EAL: Detected 40 lcore(s)
> EAL:   unsupported IOMMU type!
> EAL: VFIO support could not be initialized
> EAL: Setting up memory...
> 
> This may make user confuse, this patch make it reasonable and much more
> soomth to user.

 Not sure I agree with the premise of this patch.

First of all, if VFIO driver is not enabled, the container file would not be present and you would get a different error (namely, "cannot open VFIO container", in pci_vfio_get_container_fd()). If you have a container file,  that means VFIO driver is loaded, so I'm not sure why you get the "unsupported IOMMU type" error. I suppose it could happen when vfio is loaded but vfio_iommu_type1 isn't?

And even then, this error is harmless and doesn't do anything, so I'm not sure what this patch is supposed to fix. The error messages tells the user exactly what happens.
 
Thanks,
Anatoly