Re: [dpdk-dev] [PATCH v2 5/7] eal/linux: mmap ioports on ppc64

[Olivier Matz <olivier.matz@6wind.com>]

Hi Yuanhan,

On 05/23/2016 03:07 PM, Yuanhan Liu wrote:
> On Tue, May 17, 2016 at 05:54:01PM +0200, David Marchand wrote:
>>> +pci_uio_ioport_map(struct rte_pci_device *dev, int bar,
>>> +                  struct rte_pci_ioport *p)
>>> +{
>>> +       FILE *f;
>>> +       char buf[BUFSIZ];
>>> +       char filename[PATH_MAX];
>>> +       uint64_t phys_addr, end_addr, flags;
>>> +       int fd, i;
>>> +       void *addr;
>>> +
>>> +       /* open and read addresses of the corresponding resource in sysfs */
>>> +       snprintf(filename, sizeof(filename), "%s/" PCI_PRI_FMT "/resource",
>>> +               SYSFS_PCI_DEVICES, dev->addr.domain, dev->addr.bus,
>>> +                dev->addr.devid, dev->addr.function);
>>> +       f = fopen(filename, "r");
>>> +       if (f == NULL) {
>>> +               RTE_LOG(ERR, EAL, "Cannot open sysfs resource: %s\n",
>>> +                       strerror(errno));
>>> +               return -1;
>>> +       }
>>> +       for (i = 0; i < bar + 1; i++) {
>>> +               if (fgets(buf, sizeof(buf), f) == NULL) {
>>> +                       RTE_LOG(ERR, EAL, "Cannot read sysfs resource\n");
>>> +                       goto error;
>>> +               }
>>> +       }
>>> +       if (pci_parse_one_sysfs_resource(buf, sizeof(buf), &phys_addr,
>>> +                       &end_addr, &flags) < 0)
>>> +               goto error;
>>> +       if ((flags & IORESOURCE_IO) == 0) {
>>> +               RTE_LOG(ERR, EAL, "BAR %d is not an IO resource\n", bar);
>>> +               goto error;
>>> +       }
>>> +       snprintf(filename, sizeof(filename), "%s/" PCI_PRI_FMT "/resource%d",
>>> +               SYSFS_PCI_DEVICES, dev->addr.domain, dev->addr.bus,
>>> +                dev->addr.devid, dev->addr.function, bar);
>>> +
>>> +       /* mmap the pci resource */
>>> +       fd = open(filename, O_RDWR);
>>> +       if (fd < 0) {
>>> +               RTE_LOG(ERR, EAL, "Cannot open %s: %s\n", filename,
>>> +                       strerror(errno));
>>> +               goto error;
>>> +       }
>>> +       addr = mmap(NULL, end_addr + 1, PROT_READ | PROT_WRITE,
>>> +               MAP_SHARED, fd, 0);
>>
>> Sorry, did not catch it in v1, but a close(fd) is missing here.
>> With this, I think the patchset looks good.
>>
>> Just missing some opinion from the virtio maintainers ?
> 
> Apologize for being late for review. Assuming you have done proper
> test, this patch set looks good to me. (well, I don't quite like
> the tons of "#ifdef ... #else ..#end" block though)
> 
> A side note is that I noticed an ABI breakage introduced in this
> patch, so, this release is not a good fit?

Thank you for the review.

For reference, here is the report of the ABI checker for EAL:

[−] struct rte_pci_ioport (2)

 1 Field len has been added to this type.
   1) This field will not be initialized by old clients.
   2) Size of the inclusive type has been changed.
      NOTE: this field should be accessed only from the new library
      functions, otherwise it may result in crash or incorrect behavior
      of applications.
 2 Size of this type has been changed from 16 bytes to 24 bytes. 	
   The fields or parameters of such data type may be incorrectly
   initialized or accessed by old client applications.

[−] affected symbols (4)
 rte_eal_pci_ioport_map ( struct rte_pci_device* dev, int bar,
    struct rte_pci_ioport* p ) @@ DPDK_16.04
 3rd parameter 'p' (pointer) has base type 'struct rte_pci_ioport'.
 rte_eal_pci_ioport_read ( struct rte_pci_ioport* p, void* data,
    size_t len, off_t offset ) @@ DPDK_16.04
 1st parameter 'p' (pointer) has base type 'struct rte_pci_ioport'.
 rte_eal_pci_ioport_unmap ( struct rte_pci_ioport* p ) @@ DPDK_16.04
 1st parameter 'p' (pointer) has base type 'struct rte_pci_ioport'.
 rte_eal_pci_ioport_write ( struct rte_pci_ioport* p, void const* data,
    size_t len, off_t offset ) @@ DPDK_16.04
 1st parameter 'p' (pointer) has base type 'struct rte_pci_ioport'.


My understanding of the comment for this structure is that it's
internal to EAL:

/**
 * A structure used to access io resources for a pci device.
 * rte_pci_ioport is arch, os, driver specific, and should not be used
outside
 * of pci ioport api.
 */
struct rte_pci_ioport {
 ...
}

So I'd say it's ok to have it integrated for 16.07.

Regards,
Olivier