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

On Tue, Feb 2, 2016 at 9:48 PM, Santosh Shukla [off-list ref] wrote:

On Tue, Feb 2, 2016 at 9:21 PM, Santosh Shukla [off-list ref] wrote:

quoted

On Tue, Feb 2, 2016 at 2:19 PM, Yuanhan Liu [off-list ref] wrote:

quoted

On Tue, Feb 02, 2016 at 06:50:18AM +0100, David Marchand wrote:

quoted

On Tue, Feb 2, 2016 at 6:43 AM, Yuanhan Liu [off-list ref] wrote:

quoted

On Tue, Feb 02, 2016 at 09:44:14AM +0530, Santosh Shukla wrote:

quoted

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?

quoted

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,

quoted

        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)

quoted

                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?

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.

quoted

quoted

        --yliu