Re: [PATCH v7 4/4] eal/linux: vfio: add pci ioport support

On Mon, Feb 8, 2016 at 2:21 PM, David Marchand [off-list ref] wrote:

On Sun, Feb 7, 2016 at 2:51 PM, Santosh Shukla [off-list ref] wrote:

quoted

@@ -999,37 +1000,56 @@ int
 pci_vfio_ioport_map(struct rte_pci_device *dev, int bar,
                    struct rte_pci_ioport *p)

p is passed as a value, not a reference ...

quoted

 {
-       RTE_SET_USED(dev);
-       RTE_SET_USED(bar);
-       RTE_SET_USED(p);
-       return -1;
+       if (bar < VFIO_PCI_BAR0_REGION_INDEX ||
+           bar > VFIO_PCI_BAR5_REGION_INDEX) {
+               RTE_LOG(ERR, EAL, "invalid bar (%d)!\n", bar);
+               return -1;
+       }
+
+       p = rte_zmalloc("VFIO_IOPORT", sizeof(*p), 0);

... so I don't think this allocation does what you expected.

Anyway, you don't need to allocate a rte_pci_ioport object with current api.
You already have a valid object passed by caller.
You only need to initialise it.

quoted

+       if (p == NULL) {
+               RTE_LOG(ERR, EAL, "cannot alloc vfio ioport mem\n");
+               return -1;
+       }
+
+       p->dev = dev;

Does not hurt to do this, but p->dev is already set by caller on ret
== 0 (rte_eal_pci_ioport_map).

so far I didn't found caller setting p->dev (looked at virtio pmd
driver), In-fact I tried to use w/o setting p->dev=dev and application
crashed, stating segfault :).

So I am keeping this one in next revision.

quoted

 void
 pci_vfio_ioport_read(struct rte_pci_ioport *p,
                     void *data, size_t len, off_t offset)
 {
-       RTE_SET_USED(p);
-       RTE_SET_USED(data);
-       RTE_SET_USED(len);
-       RTE_SET_USED(offset);
+       const struct rte_intr_handle *intr_handle = &p->dev->intr_handle;

Missing blank line between declaration and code.

checkpatch.sh didn't ntocied though, which all param you use in
checkpatch, I just do
./checkpatch.sh --no-tree *.patch

quoted

+       if (pread64(intr_handle->vfio_dev_fd, data,
+                   len, p->offset + offset) <= 0)
+               RTE_LOG(ERR, EAL,
+                       "Can't read from PCI bar (%" PRIu64 ") : offset (%x)\n",
+                       VFIO_GET_REGION_IDX(p->offset), (int)offset);
 }

 void
 pci_vfio_ioport_write(struct rte_pci_ioport *p,
                      const void *data, size_t len, off_t offset)
 {
-       RTE_SET_USED(p);
-       RTE_SET_USED(data);
-       RTE_SET_USED(len);
-       RTE_SET_USED(offset);
+       const struct rte_intr_handle *intr_handle = &p->dev->intr_handle;

Idem.

quoted

+       if (pwrite64(intr_handle->vfio_dev_fd, data,
+                    len, p->offset + offset) <= 0)
+               RTE_LOG(ERR, EAL,
+                       "Can't write to PCI bar (%" PRIu64 ") : offset (%x)\n",
+                       VFIO_GET_REGION_IDX(p->offset), (int)offset);
 }

 int
 pci_vfio_ioport_unmap(struct rte_pci_ioport *p)
 {
-       RTE_SET_USED(p);
-       return -1;
+       if (p == NULL)
+               return -1;
+       else {
+               rte_free(p);
+               return 0;
+       }
 }

Since you have nothing to allocate, nothing to free here ?

Removed, Thanks

--
David Marchand