[dpdk-dev] [PATCH v6 4/4] kernel/linux/kni: add IOVA support in kni module

[Ferruh Yigit]

On 6/25/2019 4:57 AM, vattunuru at marvell.com wrote:
> From: Kiran Kumar K <kirankumark at marvell.com>
> 
> Patch adds support for kernel module to work in IOVA = VA mode,
> the idea is to get physical address from iova address using
> iommu_iova_to_phys API and later use phys_to_virt API to
> convert the physical address to kernel virtual address.
> 
> When compared with IOVA = PA mode, there is no performance
> drop with this approach.
> 
> This approach does not work with the kernel versions less
> than 4.4.0 because of API compatibility issues.
> 
> Signed-off-by: Kiran Kumar K <kirankumark at marvell.com>
> Signed-off-by: Vamsi Attunuru <vattunuru at marvell.com>

<...>

> @@ -351,15 +354,56 @@ kni_ioctl_create(struct net *net, uint32_t ioctl_num,
>  	strncpy(kni->name, dev_info.name, RTE_KNI_NAMESIZE);
>  
>  	/* Translate user space info into kernel space info */
> -	kni->tx_q = phys_to_virt(dev_info.tx_phys);
> -	kni->rx_q = phys_to_virt(dev_info.rx_phys);
> -	kni->alloc_q = phys_to_virt(dev_info.alloc_phys);
> -	kni->free_q = phys_to_virt(dev_info.free_phys);
> -
> -	kni->req_q = phys_to_virt(dev_info.req_phys);
> -	kni->resp_q = phys_to_virt(dev_info.resp_phys);
> -	kni->sync_va = dev_info.sync_va;
> -	kni->sync_kva = phys_to_virt(dev_info.sync_phys);
> +	if (dev_info.iova_mode) {
> +#if KERNEL_VERSION(4, 4, 0) > LINUX_VERSION_CODE

We have "kni/compat.h" to put the version checks, please use abstracted feature
checks only in the code.
>From experience this goes ugly quickly with the addition to distro kernels and
their specific versioning, so better to hide these all from the source code.

And this version requirement needs to be documented in kni doc.

> +		(void)pci;
> +		pr_err("Kernel version is not supported\n");

Can you please include 'iova_mode' condition into the message log, because this
kernel version is supported if user wants to use via 'iova_mode == 0' condition.

> +		return -EINVAL;
> +#else
> +		pci = pci_get_device(dev_info.vendor_id,
> +				     dev_info.device_id, NULL);
> +		while (pci) {
> +			if ((pci->bus->number == dev_info.bus) &&
> +			    (PCI_SLOT(pci->devfn) == dev_info.devid) &&
> +			    (PCI_FUNC(pci->devfn) == dev_info.function)) {
> +				domain = iommu_get_domain_for_dev(&pci->dev);
> +				break;
> +			}
> +			pci = pci_get_device(dev_info.vendor_id,
> +					     dev_info.device_id, pci);
> +		}

What if 'pci' is NULL here?
In kni it is not required to provide a device at all.

<...>

> @@ -186,7 +202,10 @@ kni_fifo_trans_pa2va(struct kni_dev *kni,
>  			return;
>  
>  		for (i = 0; i < num_rx; i++) {
> -			kva = pa2kva(kni->pa[i]);
> +			if (likely(kni->iova_mode == 1))
> +				kva = iova2kva(kni, kni->pa[i]);
> +			else
> +				kva = pa2kva(kni->pa[i]);

To reduce the churn, what about updating the 'pa2kva()' and put the
"(kni->iova_mode == 1)" check there? Does it help? (not only 'pa2kva()' but its
friends also, and if it makes more sense agree to rename the functions)

And btw, why 'likely' case is "kni->iova_mode == 1"?