[dpdk-dev,v4] 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>
---
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(-)
  

> 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
  

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
>
  

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

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
>