From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yuanhan Liu Subject: Re: [PATCH v6 1/8] eal: pci: add api to rd/wr pci bar region Date: Wed, 3 Feb 2016 19:50:22 +0800 Message-ID: <20160203115022.GB16802@yliu-dev.sh.intel.com> References: <1454091717-32251-1-git-send-email-sshukla@mvista.com> <20160201134854.GE4257@yliu-dev.sh.intel.com> <20160202054345.GI4257@yliu-dev.sh.intel.com> <20160202084933.GJ4257@yliu-dev.sh.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: "dev@dpdk.org" To: Santosh Shukla Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id EB6482A5D for ; Wed, 3 Feb 2016 12:48:41 +0100 (CET) Content-Disposition: inline In-Reply-To: List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Wed, Feb 03, 2016 at 03:20:09PM +0530, Santosh Shukla wrote: > On Tue, Feb 2, 2016 at 9:48 PM, Santosh Shukla wrote: > > On Tue, Feb 2, 2016 at 9:21 PM, Santosh Shukla wrote: > >> On Tue, Feb 2, 2016 at 2:19 PM, Yuanhan Liu wrote: > >>> On Tue, Feb 02, 2016 at 06:50:18AM +0100, David Marchand wrote: > >>>> On Tue, Feb 2, 2016 at 6:43 AM, Yuanhan Liu wrote: > >>>> > On Tue, Feb 02, 2016 at 09:44:14AM +0530, Santosh Shukla wrote: > >>>> >> Current use-case is virtio: It is used as io_bar which is first > >>>> >> bar[1]. But implementation is generic, can be used to do rd/wr for > >>>> >> other bar index too. Also vfio facilitate user to do rd/wr to pci_bars > >>>> >> w/o mapping that bar, So apis will be useful for such cases in future. > >>>> >> > >>>> >> AFAIU: uio has read/write_config api only and Yes if bar region mapped > >>>> >> then no need to do rd/wr, user can directly access the pci_memory. But > >>>> >> use-case of this api entirely different: unmapped memory by > >>>> >> application context i.e.. vfio_rd/wr-way {pread/pwrite-way}. > >>>> >> > >>>> >> Is above explanation convincing? Pl. let me know. > >>>> > > >>>> > TBH, not really. So, as you stated, it should be generic APIs to > >>>> > read/write bar space, but limiting it to VFIO only and claiming > >>>> > that read/write bar space is not support by other drivers (such > >>>> > as UIO) while in fact it can (in some ways) doesn't seem right > >>>> > to me. > >>>> > > >>>> > Anyway, it's just some thoughts from me. David, comments? > >>>> > >>>> >From the very start, same opinion. > >>>> We should have a unique api to access those, and eal should hide > >>>> details like kernel drivers (uio, vfio, whatever) to the pmd. > >>>> > >>>> Now the thing is, how to do this in an elegant and efficient way. > >>> > >>> I was thinking that we may just make it be IO port specific read/ > >>> write functions: > >>> > >> > >> Ok, > >> > >>> rte_eal_pci_ioport_read(dev, bar, buf, size) > >>> { > >>> > >>> return if not an IO bar; > >>> > >>> if (has io) > >>> return inb/w/l(); > >>> > >> > >> In that case, It may be r / if (has io) / if (drv->kdrv == UIO) > >> > >>> if (vfio) > >>> return vfio_ioport_read(); > >>> > >>> else, claim aloud that io port read is not allowed > >>> } > >>> > >>> Let us not handle memory bar resource here: in such case, you should > >>> go with rte_eal_pci_map_device() and do it with memory mapped io. > >>> > >>> Does that make any sense? > >>> > >> I am not entirely sure. > >> Are you considering IGB_UIO, UIO_GENERIC and NIC_UIO: all the cases ? > >> > > > > Just came-up something below what Yuanhan has proposed, Does this look okay? > > > > int rte_eal_pci_ioport_read(const struct rte_pci_device *device, > > void *buf, size_t len, > > off_t offset, > > int bar_idx) > > { > > if (bar_idx != 0) { > > RTE_LOG(ERR, EAL, "not a ioport bar\n"); > > return -1; > > } > > > > switch (device->kdrv) { > > case RTE_KDRV_VFIO: > > return pci_vfio_ioport_read(device, buf, len, offset, bar_idx); > > case RTE_KDRV_IGB_UIO: > > case RTE_KDRV_UIO_GENERIC: > > case RTE_KDRV_NIC_UIO: > > { > > switch (size) > > case 1: return inb(buf /*ioport address*/); > > case 2: return inw(buf /* ioport address*/); > > case 4: return inl(buf /* ioport address*/); > > default: > > RTE_LOG(ERR, EAL, "invalid size\n"); > > } > > > > default: > > RTE_LOG(ERR, EAL, "read bar not supported by driver\n"); > > return -1; > > } > > } > > > > Ping? Please be a bit more patient. Everybody has work got to do; and today I was out for personal affairs the whole day. > > Also can someone please review rest of series. This patchset going > through multiple revision, Each revision get one / two comment, It > would help if I get review comment for each patch. The others looks good to me; if you have the EAL pci API resolved properly, I guess I could give my ACK. --yliu