[dpdk-dev,v7,4/4] eal/linux: vfio: add pci ioport support
Commit Message
Include vfio map/unmap/rd/wr support for pci ioport.
Signed-off-by: Santosh Shukla <sshukla@mvista.com>
---
v7:
- This is enhancement patch for vfio map/rd/wr, rebased on top of David(s) -
"Rework ioport for virtio" patchset. For more information about api, refer
patch [1].
[1] http://dpdk.org/dev/patchwork/patch/10426/
lib/librte_eal/linuxapp/eal/eal_pci_vfio.c | 48 ++++++++++++++++++++--------
1 file changed, 34 insertions(+), 14 deletions(-)
Comments
On Sun, Feb 7, 2016 at 2:51 PM, Santosh Shukla <sshukla@mvista.com> wrote:
> @@ -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 ...
> {
> - 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.
> + 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).
>
> 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.
> + 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.
> + 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 ?
On Mon, Feb 8, 2016 at 2:21 PM, David Marchand <david.marchand@6wind.com> wrote:
> On Sun, Feb 7, 2016 at 2:51 PM, Santosh Shukla <sshukla@mvista.com> wrote:
>> @@ -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 ...
>
>> {
>> - 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.
>
>
>> + 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.
>>
>> 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
>
>> + 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.
>
>> + 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
@@ -74,6 +74,7 @@ EAL_REGISTER_TAILQ(rte_vfio_tailq)
#define VFIO_GROUP_FMT "/dev/vfio/%u"
#define VFIO_NOIOMMU_GROUP_FMT "/dev/vfio/noiommu-%u"
#define VFIO_GET_REGION_ADDR(x) ((uint64_t) x << 40ULL)
+#define VFIO_GET_REGION_IDX(x) (x >> 40)
/* per-process VFIO config */
static struct vfio_config vfio_cfg;
@@ -999,37 +1000,56 @@ int
pci_vfio_ioport_map(struct rte_pci_device *dev, int bar,
struct rte_pci_ioport *p)
{
- 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);
+ if (p == NULL) {
+ RTE_LOG(ERR, EAL, "cannot alloc vfio ioport mem\n");
+ return -1;
+ }
+
+ p->dev = dev;
+ p->offset = VFIO_GET_REGION_ADDR(bar);
+ return 0;
}
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;
+ 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;
+ 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;
+ }
}
int