Re: [dpdk-dev] [PATCH v3 17/25] virtio: Use port IO to get PCI resource.

[Thomas Monjalon <thomas.monjalon@6wind.com>]

Hi Changchun,

2015-01-29 15:24, Ouyang Changchun:
> Make virtio not require UIO for some security reasons, this is to match 6Wind's virtio-net-pmd.

Thanks for your effort.
I think port IO is a really interesting option but it needs more EAL rework
to be correctly integrated. Then virtio-net-pmd (http://dpdk.org/browse/virtio-net-pmd/)
will be obsolete and moved in a deprecated area.

> --- a/config/common_linuxapp
> +++ b/config/common_linuxapp
> +# Only for VIRTIO PMD currently
> +CONFIG_RTE_EAL_PORT_IO=n

This is the first problem. We must stop adding new build-time options.
We should be able to choose between PCI mapping and port IO at runtime.

> +/** Device needs port IO(done with /proc/ioports) */
> +#ifdef RTE_EAL_PORT_IO
> +#define RTE_PCI_DRV_PORT_IO 0x0002
> +#endif

A flag should never be ifdef'ed.

> @@ -574,7 +574,10 @@ rte_eal_pci_probe_one_driver(struct rte_pci_driver *dr, struct rte_pci_device *d
>  			/* map resources for devices that use igb_uio */
>  			ret = pci_map_device(dev);
>  			if (ret != 0)
> -				return ret;
> +#ifdef RTE_EAL_PORT_IO
> +				if ((dr->drv_flags & RTE_PCI_DRV_PORT_IO) == 0)
> +#endif
> +					return ret;
>  		} else if (dr->drv_flags & RTE_PCI_DRV_FORCE_UNBIND &&
>  		           rte_eal_process_type() == RTE_PROC_PRIMARY) {
>  			/* unbind current driver */

Why do you need this ugly return?

> --- a/lib/librte_pmd_virtio/virtio_ethdev.c
> +++ b/lib/librte_pmd_virtio/virtio_ethdev.c
> @@ -961,6 +961,71 @@ static int virtio_resource_init(struct rte_pci_device *pci_dev)
>  		     start, size);
>  	return 0;
>  }
> +
> +#ifdef RTE_EAL_PORT_IO
> +/* Extract port I/O numbers from proc/ioports */
> +static int virtio_resource_init_by_portio(struct rte_pci_device *pci_dev)
> +{
> +	uint16_t start, end;
> +	int size;
> +	FILE *fp;
> +	char *line = NULL;
> +	char pci_id[16];
> +	int found = 0;
> +	size_t linesz;
> +
> +	snprintf(pci_id, sizeof(pci_id), PCI_PRI_FMT,
> +		 pci_dev->addr.domain,
> +		 pci_dev->addr.bus,
> +		 pci_dev->addr.devid,
> +		 pci_dev->addr.function);
> +
> +	fp = fopen("/proc/ioports", "r");
> +	if (fp == NULL) {
> +		PMD_INIT_LOG(ERR, "%s(): can't open ioports", __func__);
> +		return -1;
> +	}
> +
> +	while (getdelim(&line, &linesz, '\n', fp) > 0) {
> +		char *ptr = line;
> +		char *left;
> +		int n;
> +
> +		n = strcspn(ptr, ":");
> +		ptr[n] = 0;
> +		left = &ptr[n+1];
> +
> +		while (*left && isspace(*left))
> +			left++;
> +
> +		if (!strncmp(left, pci_id, strlen(pci_id))) {
> +			found = 1;
> +
> +			while (*ptr && isspace(*ptr))
> +				ptr++;
> +
> +			sscanf(ptr, "%04hx-%04hx", &start, &end);
> +			size = end - start + 1;
> +
> +			break;
> +		}
> +	}
> +
> +	free(line);
> +	fclose(fp);
> +
> +	if (!found)
> +		return -1;
> +
> +	pci_dev->mem_resource[0].addr = (void *)(uintptr_t)(uint32_t)start;
> +	pci_dev->mem_resource[0].len =  (uint64_t)size;
> +	PMD_INIT_LOG(DEBUG,
> +		     "PCI Port IO found start=0x%lx with size=0x%lx",
> +		     start, size);
> +	return 0;
> +}
> +#endif

This part should be a Linux EAL service.

> +#ifdef RTE_EAL_PORT_IO
> +static struct eth_driver rte_virtio_pmd = {
> +	{
> +		.name = "rte_virtio_pmd",
> +		.id_table = pci_id_virtio_map,
> +		.drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_PORT_IO |

Why does it need PCI mapping in port IO mode?

> +			RTE_PCI_DRV_INTR_LSC,
> +	},
> +	.eth_dev_init = eth_virtio_dev_init,
> +	.dev_private_size = sizeof(struct virtio_hw),
> +};
> +#else
>  static struct eth_driver rte_virtio_pmd = {
>  	{
>  		.name = "rte_virtio_pmd",

This is the biggest problem.
You are defining port IO as a different driver instead of providing a way to
choose the method for each virtio device.
I think that you should use devargs to configure the pci device.

Thanks
-- 
Thomas