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

["Ouyang, Changchun" <changchun.ouyang@intel.com>]

Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Friday, January 30, 2015 7:14 AM
> To: Ouyang, Changchun
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 17/25] virtio: Use port IO to get PCI
> resource.
> 
> 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.
>
But I don't think choosing between PCI mapping and port IO at runtime is easy way,
That  means virtio-pmd need support both method, as we discussed before,
port IO can't support lsc interrupt, while pci mapping can support lsc interrupt,
they are contradict, e.g. the driver flag has issue to set its value.

So I think using build flag should be a better way to let virtio-pmd determine its method at compilation time.  
 
> > +/** 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.

I can remove the 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?

Without it,  port-io method will return error when probe one driver the vritio device,
As it don't use uio, so it can't map bar to virtual address.
Here, just let port-io method don't check the return value of pci_map_device.

> 
> > --- 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.

As the port-io method is not used by other pmd code but only for virtio pmd,  so we can say it is virtio specific codes, so 
Putting them here is good way.

out of similar reason, the function get_uio_dev for uio method also is put here.
So just keep the consistence between both uio and port-io methods. 

> 
> > +#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?

Good catch, I need remove the pci mapping here in next patch.

> 
> > +			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.

Do you mean I need new rte_devtype to handle it?
Currently the implement check if the virtio dev is bind to igb_uio or not,
If it bind to igb_uio, then it use uio/mapping method to get the address,
If it doesn't bind to igb_uio, and it is in white list, then use port-io method to get the address.
I don't see any big issue here for the logic.

Thanks
Changchun