From mboxrd@z Thu Jan  1 00:00:00 1970
From: David Marchand <david.marchand@6wind.com>
Subject: Re: [PATCH v7 4/4] eal/linux: vfio: add pci ioport
	support
Date: Mon, 8 Feb 2016 09:51:26 +0100
Message-ID: <CALwxeUvzym044mw-2hodb-BhzHwrtOy_axZi3fagVuXADec6Jw@mail.gmail.com>
References: <1454853068-14621-1-git-send-email-sshukla@mvista.com>
 <1454853068-14621-5-git-send-email-sshukla@mvista.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Cc: "dev@dpdk.org" <dev@dpdk.org>
To: Santosh Shukla <sshukla@mvista.com>
Return-path: <dev-bounces@dpdk.org>
Received: from mail-ob0-f169.google.com (mail-ob0-f169.google.com
 [209.85.214.169]) by dpdk.org (Postfix) with ESMTP id E8EFB2C72
 for <dev@dpdk.org>; Mon,  8 Feb 2016 09:51:46 +0100 (CET)
Received: by mail-ob0-f169.google.com with SMTP id wb13so145409793obb.1
 for <dev@dpdk.org>; Mon, 08 Feb 2016 00:51:46 -0800 (PST)
In-Reply-To: <1454853068-14621-5-git-send-email-sshukla@mvista.com>
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

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 ?


-- 
David Marchand