[dpdk-dev,v2] 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>
---
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(-)
Comments
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
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
>
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
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
>
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
@@ -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_ */
@@ -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 */