Re: [dpdk-dev] [PATCH] vfio: support iommu group zero

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen
> Hemminger
> Sent: Wednesday, December 9, 2015 5:56 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH] vfio: support iommu group zero
> 
> The current implementation of VFIO will not with the new no-IOMMU mode
> in 4.4 kernel. The original code assumed that IOMMU group zero would
> never be used. Group numbers are assigned starting at zero, and up until
> now the group numbers came from the hardware which is likely to use group
> 0 for system devices that are not used with DPDK.
> 
> The fix is to allow 0 as a valid group and rearrange code to split the return
> value from the group value.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> Why was this ignored? It was originally sent on 26 Oct 15 back when IOMMU
> discussion was lively.
> 
>  lib/librte_eal/linuxapp/eal/eal_pci_vfio.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> index 0e6c48a..74f91ba 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> @@ -499,14 +499,15 @@ pci_vfio_get_group_fd(int iommu_group_no)  }
> 
>  /* parse IOMMU group number for a PCI device
> - * returns -1 for errors, 0 for non-existent group */
> + * returns 1 on success, -1 for errors, 0 for non-existent group */
>  static int
> -pci_vfio_get_group_no(const char *pci_addr)
> +pci_vfio_get_group_no(const char *pci_addr, int *iommu_group_no)
>  {
>  	char linkname[PATH_MAX];
>  	char filename[PATH_MAX];
>  	char *tok[16], *group_tok, *end;
> -	int ret, iommu_group_no;
> +	int ret;
> 
>  	memset(linkname, 0, sizeof(linkname));
>  	memset(filename, 0, sizeof(filename)); @@ -533,13 +534,13 @@
> pci_vfio_get_group_no(const char *pci_addr)
>  	errno = 0;
>  	group_tok = tok[ret - 1];
>  	end = group_tok;
> -	iommu_group_no = strtol(group_tok, &end, 10);
> +	*iommu_group_no = strtol(group_tok, &end, 10);
>  	if ((end != group_tok && *end != '\0') || errno != 0) {
>  		RTE_LOG(ERR, EAL, "  %s error parsing IOMMU number!\n",
> pci_addr);
>  		return -1;
>  	}
> 
> -	return iommu_group_no;
> +	return 1;
>  }
> 
>  static void
> @@ -581,16 +582,15 @@ pci_vfio_map_resource(struct rte_pci_device
> *dev)
>  			loc->domain, loc->bus, loc->devid, loc->function);
> 
>  	/* get group number */
> -	iommu_group_no = pci_vfio_get_group_no(pci_addr);
> -
> -	/* if 0, group doesn't exist */
> -	if (iommu_group_no == 0) {
> +	ret = pci_vfio_get_group_no(pci_addr, &iommu_group_no);
> +	if (ret == 0) {
>  		RTE_LOG(WARNING, EAL, "  %s not managed by VFIO driver,
> skipping\n",
> -				pci_addr);
> +			pci_addr);
>  		return 1;
>  	}
> +
>  	/* if negative, something failed */
> -	else if (iommu_group_no < 0)
> +	if (ret < 0)
>  		return -1;
> 
>  	/* get the actual group fd */
> --
> 2.1.4

Sorry, slipped through the cracks somehow. I see no obvious issues with the patch, however I've never encountered a group 0 while testing VFIO. As per Alex, that would be more used in the no-iommu scenario, and this break doesn't appear to affect anything else even without no-iommu functionality, so...

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>