Re: [dpdk-dev] [PATCH 6/6] virtio: arm/arm64: memory mapped IO support in pmd driver

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

Hi,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Santosh Shukla
> Sent: Friday, December 04, 2015 5:35 PM
> To: dev@dpdk.org
> Cc: Rizwan Ansari
> Subject: [dpdk-dev] [PATCH 6/6] virtio: arm/arm64: memory mapped IO support in pmd driver
> 
> This patch set add memory-mapped-IO support for arm/arm64 archs in virtio-pmd
> driver. Patch creates ioport-mem device name /dev/igb_ioport. virtio_ethdev_init
> function to open that device file for once, mappes 4K page_size memory such that
> all entry in cat /proc/ioport {all PCI_IOBAR entry} mapped for once. For that
> two ancessor api;s
> - virtio_map_ioport : Maps all cat /proc/ioport pci iobars.
> - virtio_set_ioport_addr : manages the each pci_iobar slot as an offset.
> 
> Tested for thunderX/arm64 platform, by creating maximum guest kernel supported
> virtio-net-pci device i.e.. 31 max, then attaching all 32 interface to uio,
> Verified with tespmd io_fwd application.
> 
> Signed-off-by: Santosh Shukla <sshukla@mvista.com>
> Signed-off-by: Rizwan Ansari <ransari@mvista.com>
> ---

Is it possible to rework patch a bit to minimise number of #ifdef ARM ... 
spread across the code? 
Group arch related code into separate file(s), use union for fields with sizes, etc?
Konstantin  

>  drivers/net/virtio/virtio_ethdev.c        |  138 ++++++++++++++++++++++++++++-
>  lib/librte_eal/linuxapp/igb_uio/igb_uio.c |   80 ++++++++++++++++-
>  2 files changed, 214 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index 74c00ee..93b8784 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -63,6 +63,30 @@
>  #include "virtqueue.h"
>  #include "virtio_rxtx.h"
> 
> +#ifdef RTE_EXEC_ENV_LINUXAPP
> +/* start address of first pci_iobar slot (user-space virtual-addres) */
> +void *ioport_map;
> +#if defined(RTE_ARCH_ARM) || defined(RTE_ARCH_ARM64)
> +
> +#include <sys/mman.h>
> +#define DEV_NAME		"/dev/igb_ioport"
> +
> +/* Keeping pci_ioport_size = 4k.
> + * So maximum mmaped pci_iobar supported =
> + * (ioport_size/pci_dev->mem_resource[0].len)
> + *
> + * note: kernel could allow maximum 32 virtio-net-pci interface, that mean
> + * maximum 32 PCI_IOBAR(s) where each PCI_IOBAR_LEN=0x20, so virtio_map_ioport()
> + * func by theory gonna support 4k/0x20 ==> 128 PCI_IOBAR(s), more than
> + * max-virtio-net-pci interface.
> + */
> +#define PAGE_SIZE		4096
> +#define PCI_IOPORT_SIZE		PAGE_SIZE
> +#define PCI_IOPORT_MAX		128 /* 4k / 0x20 */
> +
> +int ioport_map_cnt;
> +#endif /* ARM, ARM64 */
> +#endif /* RTE_EXEC_ENV_LINUXAPP */
> 
>  static int eth_virtio_dev_init(struct rte_eth_dev *eth_dev);
>  static int eth_virtio_dev_uninit(struct rte_eth_dev *eth_dev);
> @@ -497,6 +521,18 @@ virtio_dev_close(struct rte_eth_dev *dev)
>  	hw->started = 0;
>  	virtio_dev_free_mbufs(dev);
>  	virtio_free_queues(dev);
> +
> +#ifdef RTE_EXEC_ENV_LINUXAPP
> +#if defined(RTE_ARCH_ARM) || defined(RTE_ARCH_ARM64)
> +
> +	/* unmap ioport memory */
> +	ioport_map_cnt--;
> +	if (!ioport_map_cnt)
> +		munmap(ioport_map, PCI_IOPORT_SIZE);
> +
> +	PMD_INIT_LOG(DEBUG, "unmapping ioport_mem %d\n", ioport_map_cnt);
> +#endif
> +#endif
>  }
> 
>  static void
> @@ -1172,7 +1208,6 @@ static int virtio_resource_init_by_ioports(struct rte_pci_device *pci_dev)
> 
>  			sscanf(ptr, "%04hx-%04hx", &start, &end);
>  			size = end - start + 1;
> -
>  			break;
>  		}
>  	}
> @@ -1256,6 +1291,81 @@ rx_func_get(struct rte_eth_dev *eth_dev)
>  		eth_dev->rx_pkt_burst = &virtio_recv_pkts;
>  }
> 
> +#ifdef RTE_EXEC_ENV_LINUXAPP
> +#if defined(RTE_ARCH_ARM) || defined(RTE_ARCH_ARM64)
> +
> +static int
> +virtio_map_ioport(void **resource_addr)
> +{
> +	int fd;
> +	int ret = 0;
> +
> +	fd = open(DEV_NAME, O_RDWR);
> +	if (fd < 0) {
> +		PMD_INIT_LOG(ERR, "device file %s open error: %d\n",
> +			     DEV_NAME, fd);
> +		ret = -1;
> +		goto out;
> +	}
> +
> +	ioport_map = mmap(NULL, PCI_IOPORT_SIZE,
> +			PROT_EXEC | PROT_WRITE | PROT_READ, MAP_SHARED, fd, 0);
> +
> +	if (ioport_map == MAP_FAILED) {
> +		PMD_INIT_LOG(ERR, "mmap: failed to map bar Address=%p\n",
> +				*resource_addr);
> +		ret = -ENOMEM;
> +		goto out1;
> +	}
> +
> +	PMD_INIT_LOG(INFO, "First pci_iobar mapped at %p\n", ioport_map);
> +
> +out1:
> +	close(fd);
> +out:
> +	return ret;
> +}
> +
> +static int
> +virtio_set_ioport_addr(void **resource_addr, unsigned long offset)
> +{
> +	int ret = 0;
> +
> +	if (ioport_map_cnt >= PCI_IOPORT_MAX) {
> +		ret = -1;
> +		PMD_INIT_LOG(ERR,
> +			     "ioport_map_cnt(%d) greater than"
> +			     "PCI_IOPORT_MAX(%d)\n",
> +			     ioport_map_cnt, PCI_IOPORT_MAX);
> +		goto out;
> +	}
> +	*resource_addr = (void *)((char *)ioport_map + (ioport_map_cnt)*offset);
> +	ioport_map_cnt++;
> +
> +	PMD_INIT_LOG(DEBUG, "pci.resource_addr %p ioport_map_cnt %d\n",
> +			*resource_addr, ioport_map_cnt);
> +out:
> +	return ret;
> +}
> +#else /* !ARM, !ARM64 */
> +static int
> +virtio_map_ioport(void *resource_addr)
> +{
> +	(void)resource_addr;
> +	return 0;
> +}
> +
> +static int
> +virtio_set_ioport_addr(void *resource_addr, unsigned long offset)
> +{
> +	(void)resource_addr;
> +	(void)offset;
> +	return 0;
> +}
> +
> +#endif /* ARM, ARM64 */
> +#endif /* RTE_EXEC_ENV_LINUXAPP */
> +
>  /*
>   * This function is based on probe() function in virtio_pci.c
>   * It returns 0 on success.
> @@ -1294,8 +1404,31 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
>  	if (virtio_resource_init(pci_dev) < 0)
>  		return -1;
> 
> +#ifdef	RTE_EXEC_ENV_LINUXAPP
> +	int ret = 0;
> +
> +	/* Map the all IOBAR entry from /proc/ioport to 4k page_size only once.
> +	 * Later virtio_set_ioport_addr() func will update correct bar_addr
> +	 * eachk ioport (i.e..pci_dev->mem_resource[0].addr)
> +	 */
> +	if (!ioport_map) {
> +		ret = virtio_map_ioport(&pci_dev->mem_resource[0].addr);
> +		if (ret < 0)
> +			return -1;
> +	}
> +
> +	ret = virtio_set_ioport_addr(&pci_dev->mem_resource[0].addr,
> +					pci_dev->mem_resource[0].len);
> +	if (ret < 0)
> +		return -1;
> +
> +	PMD_INIT_LOG(INFO, "ioport_map %p resource_addr %p resource_len :%ld\n",
> +			ioport_map, pci_dev->mem_resource[0].addr,
> +			(unsigned long)pci_dev->mem_resource[0].len);
> +
> +#endif
>  	hw->use_msix = virtio_has_msix(&pci_dev->addr);
> -	hw->io_base = (uint32_t)(uintptr_t)pci_dev->mem_resource[0].addr;
> +	hw->io_base = (unsigned long)(uintptr_t)pci_dev->mem_resource[0].addr;
> 
>  	/* Reset the device although not necessary at startup */
>  	vtpci_reset(hw);
> @@ -1430,7 +1563,6 @@ eth_virtio_dev_uninit(struct rte_eth_dev *eth_dev)
>  		rte_intr_callback_unregister(&pci_dev->intr_handle,
>  						virtio_interrupt_handler,
>  						eth_dev);
> -
>  	PMD_INIT_LOG(DEBUG, "dev_uninit completed");
> 
>  	return 0;
> diff --git a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> index f5617d2..b154a44 100644
> --- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> +++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> @@ -324,6 +324,61 @@ igbuio_dom0_pci_mmap(struct uio_info *info, struct vm_area_struct *vma)
>  }
>  #endif
> 
> +#if defined(RTE_ARCH_ARM) || defined(RTE_ARCH_ARM64)
> +#ifdef CONFIG_HAS_IOPORT_MAP
> +/*
> + * mmap driver to map x86-style PCI_IOBAR (i.e..cat /proc/ioport pci-bar-memory)
> + * from kernel-space virtual address to user-space virtual address. This module
> + * required for non-x86 archs example arm/arm64, as those archs donot do
> + * IO_MAP_IO types access, Infact supports memory-mapped-IO. That is because
> + * arm/arm64 doesn't support direct IO instruction, so the design approach is to
> + * map `cat /proc/ioport` PCI_IOBAR's kernel-space virtual-addr to user-space
> + * virtual-addr. Therefore the need for mmap-driver.
> + */
> +#include <linux/fs.h>		/* file_operations */
> +#include <linux/miscdevice.h>
> +#include <linux/mm.h>		/* VM_IO */
> +#include <linux/uaccess.h>
> +#include <asm/page.h>
> +#include <linux/sched.h>
> +#include <linux/pid.h>
> +
> +void *__iomem mapped_io; /* ioport addr of `cat /proc/ioport` */
> +
> +static int igb_ioport_mmap(struct file *file, struct vm_area_struct *vma)
> +{
> +	struct page *npage;
> +	int ret = 0;
> +
> +	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> +	npage = vmalloc_to_page(mapped_io);
> +	ret = remap_pfn_range(vma, vma->vm_start,
> +				page_to_pfn(npage),
> +				vma->vm_end - vma->vm_start,
> +				vma->vm_page_prot);
> +	if (ret) {
> +		pr_info("Error: Failed to remap pfn=%lu error=%d\n",
> +			page_to_pfn(npage), ret);
> +	}
> +	return 0;
> +}
> +
> +static const struct file_operations igb_ioport_fops = {
> +	.mmap		= igb_ioport_mmap,
> +};
> +
> +static struct miscdevice igb_ioport_dev = {
> +	.minor	= MISC_DYNAMIC_MINOR,
> +	.name	= "igb_ioport",
> +	.fops	= &igb_ioport_fops
> +};
> +#else /* !CONFIG_HAS_IOPORT_MAP */
> +
> +#error "CONFIG_HAS_IOPORT_MAP not supported for $RTE_ARCH"
> +
> +#endif /* CONFIG_HAS_IOPORT_MAP */
> +#endif /* RTE_ARCH_ARM, RTE_ARCH_ARM64 */
> +
>  /* Remap pci resources described by bar #pci_bar in uio resource n. */
>  static int
>  igbuio_pci_setup_iomem(struct pci_dev *dev, struct uio_info *info,
> @@ -365,11 +420,22 @@ igbuio_pci_setup_ioport(struct pci_dev *dev, struct uio_info *info,
>  	if (addr == 0 || len == 0)
>  		return -EINVAL;
> 
> +#if defined(RTE_ARCH_ARM) || defined(RTE_ARCH_ARM64)
> +	/*
> +	 * TODO: KNI/procfs:: porttype entry need to be added for ARM/ARM64,
> +	 * for below mmap driver;
> +	 * */
> +	mapped_io = ioport_map(addr, 0);
> +	info->port[n].name = name;
> +	info->port[n].start = (unsigned long)(uintptr_t)mapped_io;
> +	info->port[n].size = len;
> +	info->port[n].porttype = UIO_PORT_X86;
> +#else
>  	info->port[n].name = name;
>  	info->port[n].start = addr;
>  	info->port[n].size = len;
>  	info->port[n].porttype = UIO_PORT_X86;
> -
> +#endif
>  	return 0;
>  }
> 
> @@ -615,6 +681,15 @@ igbuio_pci_init_module(void)
>  {
>  	int ret;
> 
> +#if defined(RTE_ARCH_ARM) || defined(RTE_ARCH_ARM64)
> +	ret = misc_register(&igb_ioport_dev);
> +	if (ret < 0) {
> +		pr_info("Error: failed to register ioport map driver (%d)\n",
> +				ret);
> +		return ret;
> +	}
> +#endif
> +
>  	ret = igbuio_config_intr_mode(intr_mode);
>  	if (ret < 0)
>  		return ret;
> @@ -625,6 +700,9 @@ igbuio_pci_init_module(void)
>  static void __exit
>  igbuio_pci_exit_module(void)
>  {
> +#if defined(RTE_ARCH_ARM) || defined(RTE_ARCH_ARM64)
> +	misc_deregister(&igb_ioport_dev);
> +#endif
>  	pci_unregister_driver(&igbuio_pci_driver);
>  }
> 
> --
> 1.7.9.5