[dpdk-dev] eal/vfio: export internal vfio functions
Checks
Commit Message
This patch moves some of the internal vfio functions from
eal_vfio.h to rte_vfio.h for common uses with "rte_" prefix.
This patch also change the FSLMC bus usages from the internal
VFIO functions to external ones with "rte_" prefix
Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
This patch has some minor conflicts with "eal/vfio: add support for multiple container"
https://dpdk.org/dev/patchwork/patch/35862/
however they can be resolved easily.
drivers/bus/fslmc/Makefile | 1 -
drivers/bus/fslmc/fslmc_vfio.c | 6 ++--
drivers/bus/fslmc/fslmc_vfio.h | 2 --
drivers/bus/fslmc/meson.build | 1 -
lib/librte_eal/bsdapp/eal/eal.c | 20 +++++++++++
lib/librte_eal/common/include/rte_vfio.h | 47 ++++++++++++++++++++++++++
lib/librte_eal/linuxapp/eal/eal_vfio.c | 18 +++++-----
lib/librte_eal/linuxapp/eal/eal_vfio.h | 21 ------------
lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c | 4 +--
lib/librte_eal/rte_eal_version.map | 3 ++
10 files changed, 84 insertions(+), 39 deletions(-)
Comments
On 14-Mar-18 8:00 AM, Hemant Agrawal wrote:
> This patch moves some of the internal vfio functions from
> eal_vfio.h to rte_vfio.h for common uses with "rte_" prefix.
>
> This patch also change the FSLMC bus usages from the internal
> VFIO functions to external ones with "rte_" prefix
>
> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> ---
VFIO parts seem OK to me.
Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
14/03/2018 09:00, Hemant Agrawal:
> This patch moves some of the internal vfio functions from
> eal_vfio.h to rte_vfio.h for common uses with "rte_" prefix.
>
> This patch also change the FSLMC bus usages from the internal
> VFIO functions to external ones with "rte_" prefix
>
> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> ---
> --- a/lib/librte_eal/common/include/rte_vfio.h
> +++ b/lib/librte_eal/common/include/rte_vfio.h
> @@ -28,6 +28,12 @@
> +#if LINUX_VERSION_CODE < KERNEL_VERSION(4, 5, 0)
> +#define RTE_VFIO_NOIOMMU 8
> +#else
> +#define RTE_VFIO_NOIOMMU VFIO_NOIOMMU_IOMMU
> +#endif
I know this is just a move of an existing code,
but do you know why this check is against a version number (4.5.0),
instead of #ifdef VFIO_NOIOMMU_IOMMU which would be backport-safe?
> +/**
> + * Parse IOMMU group number for a device
> + *
> + * This function is only relevant to linux and will return
> + * an error on BSD.
> + *
> + * @return
> + * 1 on success
> + * 0 for non-existent group
> + * <0 for errors
> + */
> +int __rte_experimental
> +rte_vfio_get_group_no(const char *sysfs_base,
> + const char *dev_addr, int *iommu_group_no);
> +
> +/**
> + * Open VFIO container fd or get an existing one
> + *
> + * This function is only relevant to linux and will return
> + * an error on BSD.
> + *
> + * @return
> + * > 0 container fd
> + * < 0 for errors
> + */
> +int __rte_experimental
> +rte_vfio_get_container_fd(void);
> +
> +/**
> + * Open VFIO group fd or get an existing one
> + *
> + * This function is only relevant to linux and will return
> + * an error on BSD.
> + *
> + * @return
> + * > 0 group fd
> + * < 0 for errors
> + */
> +int __rte_experimental
> +rte_vfio_get_group_fd(int iommu_group_no);
All these new functions should have some @param documentation.
This file is not included in doxygen, probably because @file is missing.
About the naming, are you sure about "group_no" instead of "group_num"?
> --- a/lib/librte_eal/rte_eal_version.map
> +++ b/lib/librte_eal/rte_eal_version.map
> @@ -254,5 +254,8 @@ EXPERIMENTAL {
> rte_service_set_runstate_mapped_check;
> rte_service_set_stats_enable;
> rte_service_start_with_defaults;
> + rte_vfio_get_group_no;
> + rte_vfio_get_container_fd;
> + rte_vfio_get_group_fd;
Please indent with tabs.
Hi Thomas,
On 3/27/2018 9:23 PM, Thomas Monjalon wrote:
> 14/03/2018 09:00, Hemant Agrawal:
>> This patch moves some of the internal vfio functions from
>> eal_vfio.h to rte_vfio.h for common uses with "rte_" prefix.
>>
>> This patch also change the FSLMC bus usages from the internal
>> VFIO functions to external ones with "rte_" prefix
>>
>> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
>> ---
>> --- a/lib/librte_eal/common/include/rte_vfio.h
>> +++ b/lib/librte_eal/common/include/rte_vfio.h
>> @@ -28,6 +28,12 @@
>> +#if LINUX_VERSION_CODE < KERNEL_VERSION(4, 5, 0)
>> +#define RTE_VFIO_NOIOMMU 8
>> +#else
>> +#define RTE_VFIO_NOIOMMU VFIO_NOIOMMU_IOMMU
>> +#endif
> I know this is just a move of an existing code,
> but do you know why this check is against a version number (4.5.0),
> instead of #ifdef VFIO_NOIOMMU_IOMMU which would be backport-safe?
Agreed. please check it in v3.
>> +/**
>> + * Parse IOMMU group number for a device
>> + *
>> + * This function is only relevant to linux and will return
>> + * an error on BSD.
>> + *
>> + * @return
>> + * 1 on success
>> + * 0 for non-existent group
>> + * <0 for errors
>> + */
>> +int __rte_experimental
>> +rte_vfio_get_group_no(const char *sysfs_base,
>> + const char *dev_addr, int *iommu_group_no);
>> +
>> +/**
>> + * Open VFIO container fd or get an existing one
>> + *
>> + * This function is only relevant to linux and will return
>> + * an error on BSD.
>> + *
>> + * @return
>> + * > 0 container fd
>> + * < 0 for errors
>> + */
>> +int __rte_experimental
>> +rte_vfio_get_container_fd(void);
>> +
>> +/**
>> + * Open VFIO group fd or get an existing one
>> + *
>> + * This function is only relevant to linux and will return
>> + * an error on BSD.
>> + *
>> + * @return
>> + * > 0 group fd
>> + * < 0 for errors
>> + */
>> +int __rte_experimental
>> +rte_vfio_get_group_fd(int iommu_group_no);
> All these new functions should have some @param documentation.
added the @param
> This file is not included in doxygen, probably because @file is missing.
most of these functions are internal functions. do you think we should
add it in doxygen as well?
>
> About the naming, are you sure about "group_no" instead of "group_num"?
Agree, but this is already in many places. I feel this change will be
unnecessary.
>
>> --- a/lib/librte_eal/rte_eal_version.map
>> +++ b/lib/librte_eal/rte_eal_version.map
>> @@ -254,5 +254,8 @@ EXPERIMENTAL {
>> rte_service_set_runstate_mapped_check;
>> rte_service_set_stats_enable;
>> rte_service_start_with_defaults;
>> + rte_vfio_get_group_no;
>> + rte_vfio_get_container_fd;
>> + rte_vfio_get_group_fd;
> Please indent with tabs.
>
>
done in v3
03/04/2018 08:27, Hemant Agrawal:
> On 3/27/2018 9:23 PM, Thomas Monjalon wrote:
> > 14/03/2018 09:00, Hemant Agrawal:
> >> This patch moves some of the internal vfio functions from
> >> eal_vfio.h to rte_vfio.h for common uses with "rte_" prefix.
> >>
> >> This patch also change the FSLMC bus usages from the internal
> >> VFIO functions to external ones with "rte_" prefix
> >>
> >> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> >> ---
> >> --- a/lib/librte_eal/common/include/rte_vfio.h
> >> +++ b/lib/librte_eal/common/include/rte_vfio.h
> >> @@ -28,6 +28,12 @@
> >> +#if LINUX_VERSION_CODE < KERNEL_VERSION(4, 5, 0)
> >> +#define RTE_VFIO_NOIOMMU 8
> >> +#else
> >> +#define RTE_VFIO_NOIOMMU VFIO_NOIOMMU_IOMMU
> >> +#endif
> > I know this is just a move of an existing code,
> > but do you know why this check is against a version number (4.5.0),
> > instead of #ifdef VFIO_NOIOMMU_IOMMU which would be backport-safe?
> Agreed. please check it in v3.
Yes, in v2.
> >> +/**
> >> + * Parse IOMMU group number for a device
> >> + *
> >> + * This function is only relevant to linux and will return
> >> + * an error on BSD.
> >> + *
> >> + * @return
> >> + * 1 on success
> >> + * 0 for non-existent group
> >> + * <0 for errors
> >> + */
> >> +int __rte_experimental
> >> +rte_vfio_get_group_no(const char *sysfs_base,
> >> + const char *dev_addr, int *iommu_group_no);
> >> +
> >> +/**
> >> + * Open VFIO container fd or get an existing one
> >> + *
> >> + * This function is only relevant to linux and will return
> >> + * an error on BSD.
> >> + *
> >> + * @return
> >> + * > 0 container fd
> >> + * < 0 for errors
> >> + */
> >> +int __rte_experimental
> >> +rte_vfio_get_container_fd(void);
> >> +
> >> +/**
> >> + * Open VFIO group fd or get an existing one
> >> + *
> >> + * This function is only relevant to linux and will return
> >> + * an error on BSD.
> >> + *
> >> + * @return
> >> + * > 0 group fd
> >> + * < 0 for errors
> >> + */
> >> +int __rte_experimental
> >> +rte_vfio_get_group_fd(int iommu_group_no);
> >
> > All these new functions should have some @param documentation.
>
> added the @param
>
> > This file is not included in doxygen, probably because @file is missing.
>
> most of these functions are internal functions. do you think we should
> add it in doxygen as well?
I think yes. It is an exported header of EAL.
The @file is missing to make it visible in doxygen.
> > About the naming, are you sure about "group_no" instead of "group_num"?
>
> Agree, but this is already in many places. I feel this change will be
> unnecessary.
I don't see any other function using "_no".
What about naming the function "rte_vfio_get_group_no"
as "rte_vfio_get_group_num"?
On 4/3/2018 1:04 PM, Thomas Monjalon wrote:
>>>> +/**
>>>> + * Parse IOMMU group number for a device
>>>> + *
>>>> + * This function is only relevant to linux and will return
>>>> + * an error on BSD.
>>>> + *
>>>> + * @return
>>>> + * 1 on success
>>>> + * 0 for non-existent group
>>>> + * <0 for errors
>>>> + */
>>>> +int __rte_experimental
>>>> +rte_vfio_get_group_no(const char *sysfs_base,
>>>> + const char *dev_addr, int *iommu_group_no);
>>>> +
>>>> +/**
>>>> + * Open VFIO container fd or get an existing one
>>>> + *
>>>> + * This function is only relevant to linux and will return
>>>> + * an error on BSD.
>>>> + *
>>>> + * @return
>>>> + * > 0 container fd
>>>> + * < 0 for errors
>>>> + */
>>>> +int __rte_experimental
>>>> +rte_vfio_get_container_fd(void);
>>>> +
>>>> +/**
>>>> + * Open VFIO group fd or get an existing one
>>>> + *
>>>> + * This function is only relevant to linux and will return
>>>> + * an error on BSD.
>>>> + *
>>>> + * @return
>>>> + * > 0 group fd
>>>> + * < 0 for errors
>>>> + */
>>>> +int __rte_experimental
>>>> +rte_vfio_get_group_fd(int iommu_group_no);
>>>
>>> All these new functions should have some @param documentation.
>>
>> added the @param
>>
>>> This file is not included in doxygen, probably because @file is missing.
>>
>> most of these functions are internal functions. do you think we should
>> add it in doxygen as well?
>
> I think yes. It is an exported header of EAL.
> The @file is missing to make it visible in doxygen.
done.
>
>>> About the naming, are you sure about "group_no" instead of "group_num"?
>>
>> Agree, but this is already in many places. I feel this change will be
>> unnecessary.
>
> I don't see any other function using "_no".
> What about naming the function "rte_vfio_get_group_no"
> as "rte_vfio_get_group_num"?
>
>
done
>
@@ -21,7 +21,6 @@ endif
CFLAGS += -I$(RTE_SDK)/drivers/bus/fslmc
CFLAGS += -I$(RTE_SDK)/drivers/bus/fslmc/mc
CFLAGS += -I$(RTE_SDK)/drivers/bus/fslmc/qbman/include
-CFLAGS += -I$(RTE_SDK)/lib/librte_eal/linuxapp/eal
CFLAGS += -I$(RTE_SDK)/lib/librte_eal/common
LDLIBS += -lrte_eal -lrte_mbuf -lrte_mempool -lrte_ring
LDLIBS += -lrte_ethdev
@@ -94,7 +94,7 @@ fslmc_get_container_group(int *groupid)
}
/* get group number */
- ret = vfio_get_group_no(SYSFS_FSL_MC_DEVICES, g_container, groupid);
+ ret = rte_vfio_get_group_no(SYSFS_FSL_MC_DEVICES, g_container, groupid);
if (ret <= 0) {
FSLMC_VFIO_LOG(ERR, "Unable to find %s IOMMU group",
g_container);
@@ -128,7 +128,7 @@ vfio_connect_container(void)
}
/* Opens main vfio file descriptor which represents the "container" */
- fd = vfio_get_container_fd();
+ fd = rte_vfio_get_container_fd();
if (fd < 0) {
FSLMC_VFIO_LOG(ERR, "Failed to open VFIO container");
return -errno;
@@ -627,7 +627,7 @@ fslmc_vfio_setup_group(void)
}
/* Get the actual group fd */
- ret = vfio_get_group_fd(groupid);
+ ret = rte_vfio_get_group_fd(groupid);
if (ret < 0)
return ret;
vfio_group.fd = ret;
@@ -10,8 +10,6 @@
#include <rte_vfio.h>
-#include "eal_vfio.h"
-
#define DPAA2_MC_DPNI_DEVID 7
#define DPAA2_MC_DPSECI_DEVID 3
#define DPAA2_MC_DPCON_DEVID 5
@@ -22,6 +22,5 @@ sources = files('fslmc_bus.c',
allow_experimental_apis = true
-includes += include_directories('../../../lib/librte_eal/linuxapp/eal')
includes += include_directories('mc', 'qbman/include', 'portal')
cflags += ['-D_GNU_SOURCE']
@@ -781,3 +781,23 @@ int rte_vfio_clear_group(__rte_unused int vfio_group_fd)
{
return 0;
}
+
+int __rte_experimental
+rte_vfio_get_group_no(__rte_unused const char *sysfs_base,
+ __rte_unused const char *dev_addr,
+ __rte_unused int *iommu_group_no)
+{
+ return -1;
+}
+
+int __rte_experimental
+rte_vfio_get_container_fd(void)
+{
+ return -1;
+}
+
+int __rte_experimental
+rte_vfio_get_group_fd(__rte_unused int iommu_group_no)
+{
+ return -1;
+}
@@ -28,6 +28,12 @@
#define VFIO_NOIOMMU_MODE \
"/sys/module/vfio/parameters/enable_unsafe_noiommu_mode"
+#if LINUX_VERSION_CODE < KERNEL_VERSION(4, 5, 0)
+#define RTE_VFIO_NOIOMMU 8
+#else
+#define RTE_VFIO_NOIOMMU VFIO_NOIOMMU_IOMMU
+#endif
+
/**
* Setup vfio_cfg for the device identified by its address.
* It discovers the configured I/O MMU groups or sets a new one for the device.
@@ -123,6 +129,47 @@ int rte_vfio_noiommu_is_enabled(void);
int
rte_vfio_clear_group(int vfio_group_fd);
+/**
+ * Parse IOMMU group number for a device
+ *
+ * This function is only relevant to linux and will return
+ * an error on BSD.
+ *
+ * @return
+ * 1 on success
+ * 0 for non-existent group
+ * <0 for errors
+ */
+int __rte_experimental
+rte_vfio_get_group_no(const char *sysfs_base,
+ const char *dev_addr, int *iommu_group_no);
+
+/**
+ * Open VFIO container fd or get an existing one
+ *
+ * This function is only relevant to linux and will return
+ * an error on BSD.
+ *
+ * @return
+ * > 0 container fd
+ * < 0 for errors
+ */
+int __rte_experimental
+rte_vfio_get_container_fd(void);
+
+/**
+ * Open VFIO group fd or get an existing one
+ *
+ * This function is only relevant to linux and will return
+ * an error on BSD.
+ *
+ * @return
+ * > 0 group fd
+ * < 0 for errors
+ */
+int __rte_experimental
+rte_vfio_get_group_fd(int iommu_group_no);
+
#endif /* VFIO_PRESENT */
#endif /* _RTE_VFIO_H_ */
@@ -36,7 +36,7 @@ static const struct vfio_iommu_type iommu_types[] = {
};
int
-vfio_get_group_fd(int iommu_group_no)
+rte_vfio_get_group_fd(int iommu_group_no)
{
int i;
int vfio_group_fd;
@@ -266,7 +266,7 @@ rte_vfio_setup_device(const char *sysfs_base, const char *dev_addr,
int ret;
/* get group number */
- ret = vfio_get_group_no(sysfs_base, dev_addr, &iommu_group_no);
+ ret = rte_vfio_get_group_no(sysfs_base, dev_addr, &iommu_group_no);
if (ret == 0) {
RTE_LOG(WARNING, EAL, " %s not managed by VFIO driver, skipping\n",
dev_addr);
@@ -278,7 +278,7 @@ rte_vfio_setup_device(const char *sysfs_base, const char *dev_addr,
return -1;
/* get the actual group fd */
- vfio_group_fd = vfio_get_group_fd(iommu_group_no);
+ vfio_group_fd = rte_vfio_get_group_fd(iommu_group_no);
if (vfio_group_fd < 0)
return -1;
@@ -398,7 +398,7 @@ rte_vfio_release_device(const char *sysfs_base, const char *dev_addr,
int ret;
/* get group number */
- ret = vfio_get_group_no(sysfs_base, dev_addr, &iommu_group_no);
+ ret = rte_vfio_get_group_no(sysfs_base, dev_addr, &iommu_group_no);
if (ret <= 0) {
RTE_LOG(WARNING, EAL, " %s not managed by VFIO driver\n",
dev_addr);
@@ -407,9 +407,9 @@ rte_vfio_release_device(const char *sysfs_base, const char *dev_addr,
}
/* get the actual group fd */
- vfio_group_fd = vfio_get_group_fd(iommu_group_no);
+ vfio_group_fd = rte_vfio_get_group_fd(iommu_group_no);
if (vfio_group_fd <= 0) {
- RTE_LOG(INFO, EAL, "vfio_get_group_fd failed for %s\n",
+ RTE_LOG(INFO, EAL, "rte_vfio_get_group_fd failed for %s\n",
dev_addr);
return -1;
}
@@ -480,7 +480,7 @@ rte_vfio_enable(const char *modname)
return 0;
}
- vfio_cfg.vfio_container_fd = vfio_get_container_fd();
+ vfio_cfg.vfio_container_fd = rte_vfio_get_container_fd();
/* check if we have VFIO driver enabled */
if (vfio_cfg.vfio_container_fd != -1) {
@@ -558,7 +558,7 @@ vfio_has_supported_extensions(int vfio_container_fd)
}
int
-vfio_get_container_fd(void)
+rte_vfio_get_container_fd(void)
{
int ret, vfio_container_fd;
@@ -622,7 +622,7 @@ vfio_get_container_fd(void)
}
int
-vfio_get_group_no(const char *sysfs_base,
+rte_vfio_get_group_no(const char *sysfs_base,
const char *dev_addr, int *iommu_group_no)
{
char linkname[PATH_MAX];
@@ -79,12 +79,6 @@ struct vfio_iommu_spapr_tce_info {
#define RTE_VFIO_SPAPR VFIO_SPAPR_TCE_v2_IOMMU
#endif
-#if LINUX_VERSION_CODE < KERNEL_VERSION(4, 5, 0)
-#define RTE_VFIO_NOIOMMU 8
-#else
-#define RTE_VFIO_NOIOMMU VFIO_NOIOMMU_IOMMU
-#endif
-
#define VFIO_MAX_GROUPS RTE_MAX_VFIO_GROUPS
/*
@@ -133,21 +127,6 @@ vfio_set_iommu_type(int vfio_container_fd);
int
vfio_has_supported_extensions(int vfio_container_fd);
-/* open container fd or get an existing one */
-int
-vfio_get_container_fd(void);
-
-/* parse IOMMU group number for a device
- * returns 1 on success, -1 for errors, 0 for non-existent group
- */
-int
-vfio_get_group_no(const char *sysfs_base,
- const char *dev_addr, int *iommu_group_no);
-
-/* open group fd or get an existing one */
-int
-vfio_get_group_fd(int iommu_group_no);
-
int vfio_mp_sync_setup(void);
#define SOCKET_REQ_CONTAINER 0x100
@@ -267,7 +267,7 @@ vfio_mp_sync_thread(void __rte_unused * arg)
switch (ret) {
case SOCKET_REQ_CONTAINER:
- fd = vfio_get_container_fd();
+ fd = rte_vfio_get_container_fd();
if (fd < 0)
vfio_mp_sync_send_request(conn_sock, SOCKET_ERR);
else
@@ -283,7 +283,7 @@ vfio_mp_sync_thread(void __rte_unused * arg)
continue;
}
- fd = vfio_get_group_fd(vfio_data);
+ fd = rte_vfio_get_group_fd(vfio_data);
if (fd < 0)
vfio_mp_sync_send_request(conn_sock, SOCKET_ERR);
@@ -254,5 +254,8 @@ EXPERIMENTAL {
rte_service_set_runstate_mapped_check;
rte_service_set_stats_enable;
rte_service_start_with_defaults;
+ rte_vfio_get_group_no;
+ rte_vfio_get_container_fd;
+ rte_vfio_get_group_fd;
} DPDK_18.02;