[dpdk-dev] [PATCH] eal/vfio: export internal vfio functions

[Hemant Agrawal]

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