[dpdk-dev,v5] VFIO: Avoid to enable vfio while the module not loaded
Commit Message
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(-)
Comments
>
> 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
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
>
@@ -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_ */
@@ -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;
+}
@@ -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 */