From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id C26A5AD89 for ; Wed, 4 Feb 2015 02:32:01 +0100 (CET) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga101.fm.intel.com with ESMTP; 03 Feb 2015 17:31:57 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.09,516,1418112000"; d="scan'208";a="646958770" Received: from pgsmsx105.gar.corp.intel.com ([10.221.44.96]) by orsmga001.jf.intel.com with ESMTP; 03 Feb 2015 17:31:47 -0800 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by PGSMSX105.gar.corp.intel.com (10.221.44.96) with Microsoft SMTP Server (TLS) id 14.3.195.1; Wed, 4 Feb 2015 09:31:45 +0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.124]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.91]) with mapi id 14.03.0195.001; Wed, 4 Feb 2015 09:31:45 +0800 From: "Ouyang, Changchun" To: Thomas Monjalon Thread-Topic: [dpdk-dev] [PATCH v3 17/25] virtio: Use port IO to get PCI resource. Thread-Index: AQHQO5Sz/rZe6qOJS0KxaEUdi2nqb5zXNWGAgAiBfhA= Date: Wed, 4 Feb 2015 01:31:43 +0000 Message-ID: References: <1422326164-13697-1-git-send-email-changchun.ouyang@intel.com> <1422516249-14596-1-git-send-email-changchun.ouyang@intel.com> <1422516249-14596-18-git-send-email-changchun.ouyang@intel.com> <2200098.eUytGrRrjF@xps13> In-Reply-To: <2200098.eUytGrRrjF@xps13> Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v3 17/25] virtio: Use port IO to get PCI resource. X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 04 Feb 2015 01:32:02 -0000 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. >=20 > Hi Changchun, >=20 > 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. >=20 > Thanks for your effort. > I think port IO is a really interesting option but it needs more EAL rewo= rk to > be correctly integrated. Then virtio-net-pmd (http://dpdk.org/browse/virt= io- > net-pmd/) > will be obsolete and moved in a deprecated area. >=20 > > --- a/config/common_linuxapp > > +++ b/config/common_linuxapp > > +# Only for VIRTIO PMD currently > > +CONFIG_RTE_EAL_PORT_IO=3Dn >=20 > 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 ea= sy 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 inte= rrupt, 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 determ= ine its method at compilation time. =20 =20 > > +/** Device needs port IO(done with /proc/ioports) */ #ifdef > > +RTE_EAL_PORT_IO #define RTE_PCI_DRV_PORT_IO 0x0002 #endif >=20 > A flag should never be ifdef'ed. I can remove the ifdef'ed. =20 > > @@ -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 =3D pci_map_device(dev); > > if (ret !=3D 0) > > - return ret; > > +#ifdef RTE_EAL_PORT_IO > > + if ((dr->drv_flags & RTE_PCI_DRV_PORT_IO) > =3D=3D 0) #endif > > + return ret; > > } else if (dr->drv_flags & RTE_PCI_DRV_FORCE_UNBIND && > > rte_eal_process_type() =3D=3D RTE_PROC_PRIMARY) { > > /* unbind current driver */ >=20 > Why do you need this ugly return? Without it, port-io method will return error when probe one driver the vri= tio 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_devic= e. >=20 > > --- 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_dev= ice > *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 =3D NULL; > > + char pci_id[16]; > > + int found =3D 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 =3D fopen("/proc/ioports", "r"); > > + if (fp =3D=3D NULL) { > > + PMD_INIT_LOG(ERR, "%s(): can't open ioports", __func__); > > + return -1; > > + } > > + > > + while (getdelim(&line, &linesz, '\n', fp) > 0) { > > + char *ptr =3D line; > > + char *left; > > + int n; > > + > > + n =3D strcspn(ptr, ":"); > > + ptr[n] =3D 0; > > + left =3D &ptr[n+1]; > > + > > + while (*left && isspace(*left)) > > + left++; > > + > > + if (!strncmp(left, pci_id, strlen(pci_id))) { > > + found =3D 1; > > + > > + while (*ptr && isspace(*ptr)) > > + ptr++; > > + > > + sscanf(ptr, "%04hx-%04hx", &start, &end); > > + size =3D end - start + 1; > > + > > + break; > > + } > > + } > > + > > + free(line); > > + fclose(fp); > > + > > + if (!found) > > + return -1; > > + > > + pci_dev->mem_resource[0].addr =3D (void *)(uintptr_t)(uint32_t)start; > > + pci_dev->mem_resource[0].len =3D (uint64_t)size; > > + PMD_INIT_LOG(DEBUG, > > + "PCI Port IO found start=3D0x%lx with size=3D0x%lx", > > + start, size); > > + return 0; > > +} > > +#endif >=20 > 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=20 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.=20 >=20 > > +#ifdef RTE_EAL_PORT_IO > > +static struct eth_driver rte_virtio_pmd =3D { > > + { > > + .name =3D "rte_virtio_pmd", > > + .id_table =3D pci_id_virtio_map, > > + .drv_flags =3D RTE_PCI_DRV_NEED_MAPPING | > RTE_PCI_DRV_PORT_IO | >=20 > Why does it need PCI mapping in port IO mode? Good catch, I need remove the pci mapping here in next patch. >=20 > > + RTE_PCI_DRV_INTR_LSC, > > + }, > > + .eth_dev_init =3D eth_virtio_dev_init, > > + .dev_private_size =3D sizeof(struct virtio_hw), }; #else > > static struct eth_driver rte_virtio_pmd =3D { > > { > > .name =3D "rte_virtio_pmd", >=20 > 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 me= thod to get the address. I don't see any big issue here for the logic. Thanks Changchun