Re: [dpdk-dev] [PATCH v2 3/3] bus/pci: only consider usable devices to select IOVA mode

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

On 14-Jun-19 10:39 AM, David Marchand wrote:
> From: Ben Walker <benjamin.walker@intel.com>
> 
> When selecting the preferred IOVA mode of the pci bus, the current
> heuristic ("are devices bound?", "are devices bound to UIO?", "are pmd
> drivers supporting IOVA as VA?" etc..) should honor the device
> white/blacklist so that an unwanted device does not impact the decision.
> 
> There is no reason to consider a device which has no driver available.
> 
> This applies to all OS, so implements this in common code then call a
> OS specific callback.
> 
> On Linux side:
> - the VFIO special considerations should be evaluated only if VFIO
>    support is built,
> - there is no strong requirement on using VA rather than PA if a driver
>    supports VA, so defaulting to DC in such a case.
> 
> Signed-off-by: Ben Walker <benjamin.walker@intel.com>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---

<snip>

> +		     const struct rte_pci_device *pdev)
>   {
> -	struct rte_pci_device *dev = NULL;
> -	struct rte_pci_driver *drv = NULL;
> +	enum rte_iova_mode iova_mode = RTE_IOVA_DC;
> +	static int iommu_no_va = -1;
>   
> -	FOREACH_DRIVER_ON_PCIBUS(drv) {
> -		FOREACH_DEVICE_ON_PCIBUS(dev) {
> -			if (!rte_pci_match(drv, dev))
> -				continue;
> -			/*
> -			 * just one PCI device needs to be checked out because
> -			 * the IOMMU hardware is the same for all of them.
> -			 */
> -			return pci_one_device_iommu_support_va(dev);
> +	switch (pdev->kdrv) {
> +	case RTE_KDRV_VFIO: {
> +#ifdef VFIO_PRESENT
> +		static int is_vfio_noiommu_enabled = -1;
> +
> +		if (is_vfio_noiommu_enabled == -1) {
> +			if (rte_vfio_noiommu_is_enabled() == 1)
> +				is_vfio_noiommu_enabled = 1;
> +			else
> +				is_vfio_noiommu_enabled = 0;
> +		}
> +		if ((pdrv->drv_flags & RTE_PCI_DRV_IOVA_AS_VA) == 0) {
> +			iova_mode = RTE_IOVA_PA;
> +		} else if (is_vfio_noiommu_enabled != 0) {
> +			RTE_LOG(DEBUG, EAL, "Forcing to 'PA', vfio-noiommu mode configured\n");
> +			iova_mode = RTE_IOVA_PA;
>   		}
> +#endif
> +		break;

I'm not too well-versed in bus code, so please excuse my ignorance of 
this codebase.

It seems that we would be ignoring drv_flags in case VFIO wasn't 
compiled - if the driver has no RTE_PCI_DRV_IOVA_AS_VA flag, i'm pretty 
sure we can set IOVA mode to PA without caring about VFIO at all. I 
think it would be better to have something like this:

if ((pdrv->drv_flags & RTE_PCI_DRV_IOVA_AS_VA) == 0) {
	iova_mode = RTE_IOVA_PA;
	break; // early exit
}
#ifdef VFIO_PRESENT
static int is_vfio_noiommu_enabled = -1;

if (is_vfio_noiommu_enabled == -1) {
	if (rte_vfio_noiommu_is_enabled() == 1)
		is_vfio_noiommu_enabled = 1;
	else
		is_vfio_noiommu_enabled = 0;
}
if (is_vfio_noiommu_enabled != 0) {
	iova_mode = RTE_IOVA_PA;
}
#endif
break;


In fact, could we not check if devices support both flags, and do an 
early exit in case they don't?

Something like this, for example:

if ((pdrv->drv_flags & RTE_PCI_DRV_IOVA_AS_VA) == 0) {
	return RTE_IOVA_PA; // early exit - device only wants PA
}
// device supports both PA and VA modes, so do more checks
switch (pdev->kdrv) {
...
}

Unless i'm missing something, this would look much simpler and easier to 
understand.

-- 
Thanks,
Anatoly