Re: [dpdk-dev] [PATCH v6 1/8] eal: pci: add api to rd/wr pci bar region

[Yuanhan Liu <yuanhan.liu@linux.intel.com>]

On Wed, Feb 03, 2016 at 03:20:09PM +0530, Santosh Shukla wrote:
> On Tue, Feb 2, 2016 at 9:48 PM, Santosh Shukla <sshukla@mvista.com> wrote:
> > On Tue, Feb 2, 2016 at 9:21 PM, Santosh Shukla <sshukla@mvista.com> wrote:
> >> On Tue, Feb 2, 2016 at 2:19 PM, Yuanhan Liu <yuanhan.liu@linux.intel.com> 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 <yuanhan.liu@linux.intel.com> 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