| Message ID | 1521014434-3399-1-git-send-email-hemant.agrawal@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | eal/vfio: export internal vfio functions | expand |
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> -- Thanks, Anatoly
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 >
diff --git a/drivers/bus/fslmc/Makefile b/drivers/bus/fslmc/Makefile index 952b4c0..04814db 100644 --- a/drivers/bus/fslmc/Makefile +++ b/drivers/bus/fslmc/Makefile @@ -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 diff --git a/drivers/bus/fslmc/fslmc_vfio.c b/drivers/bus/fslmc/fslmc_vfio.c index e840ad6..4e1187e 100644 --- a/drivers/bus/fslmc/fslmc_vfio.c +++ b/drivers/bus/fslmc/fslmc_vfio.c @@ -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; diff --git a/drivers/bus/fslmc/fslmc_vfio.h b/drivers/bus/fslmc/fslmc_vfio.h index e8fb344..9e2c4fe 100644 --- a/drivers/bus/fslmc/fslmc_vfio.h +++ b/drivers/bus/fslmc/fslmc_vfio.h @@ -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 diff --git a/drivers/bus/fslmc/meson.build b/drivers/bus/fslmc/meson.build index e94340e..78f9d92 100644 --- a/drivers/bus/fslmc/meson.build +++ b/drivers/bus/fslmc/meson.build @@ -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'] diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c index 4eafcb5..29cac28 100644 --- a/lib/librte_eal/bsdapp/eal/eal.c +++ b/lib/librte_eal/bsdapp/eal/eal.c @@ -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; +} diff --git a/lib/librte_eal/common/include/rte_vfio.h b/lib/librte_eal/common/include/rte_vfio.h index e981a62..a9665c6 100644 --- a/lib/librte_eal/common/include/rte_vfio.h +++ b/lib/librte_eal/common/include/rte_vfio.h @@ -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_ */ diff --git a/lib/librte_eal/linuxapp/eal/eal_vfio.c b/lib/librte_eal/linuxapp/eal/eal_vfio.c index e44ae4d..3dc2c18 100644 --- a/lib/librte_eal/linuxapp/eal/eal_vfio.c +++ b/lib/librte_eal/linuxapp/eal/eal_vfio.c @@ -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]; diff --git a/lib/librte_eal/linuxapp/eal/eal_vfio.h b/lib/librte_eal/linuxapp/eal/eal_vfio.h index 8059577..c72b76c 100644 --- a/lib/librte_eal/linuxapp/eal/eal_vfio.h +++ b/lib/librte_eal/linuxapp/eal/eal_vfio.h @@ -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 diff --git a/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c b/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c index 7cc3c15..e19b571 100644 --- a/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c +++ b/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c @@ -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); diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map index d123602..7522b2b 100644 --- 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; } DPDK_18.02;
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(-) -- 2.7.4