Re: [dpdk-dev] [PATCH] bus/pci: support iova=va on PowerNV systems

[David Marchand <david.marchand@redhat.com>]

On Wed, Feb 26, 2020 at 8:50 PM David Christensen
<drc@linux.vnet.ibm.com> wrote:
>
> Bare metal PowerNV systems include a DPDK supported IOMMU that allows
> IOVA=VA support. Test for the platform type and report virtual address
> support if running on a PowerNV system.

I can't test it, so I'll trust you on this :-).
Comments below.


>
> Signed-off-by: David Christensen <drc@linux.vnet.ibm.com>
> ---
>  drivers/bus/pci/linux/pci.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>
> diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c
> index 740a2cdad..696dc177e 100644
> --- a/drivers/bus/pci/linux/pci.c
> +++ b/drivers/bus/pci/linux/pci.c
> @@ -549,6 +549,35 @@ pci_device_iommu_support_va(const struct rte_pci_device *dev)
>  bool
>  pci_device_iommu_support_va(__rte_unused const struct rte_pci_device *dev)
>  {
> +       /*
> +        * IOMMU is always present on a PowerNV host (IOMMUv2).
> +        * IOMMU is also present in a KVM/QEMU VM (IOMMUv1) but is not
> +        * currently supported by DPDK. Test for our current environment
> +        * and report VA support as appropriate.
> +        */
> +
> +       char *line = 0;

Nit: NULL

> +       size_t len = 0;
> +       char filename[PATH_MAX] = "/proc/cpuinfo";
> +       FILE *fp = fopen(filename, "r");
> +
> +       if (fp == NULL) {
> +               RTE_LOG(ERR, EAL, "%s(): can't open %s: %s\n",
> +                       __func__, filename, strerror(errno));
> +               return false;
> +       }
> +
> +       /* Check for a PowerNV platform */
> +       while (getline(&line, &len, fp) != -1) {

From here, you leak line.

man getline

       ssize_t getline(char **lineptr, size_t *n, FILE *stream);

       If  *lineptr is NULL, then getline() will allocate a buffer for storing
       the line, which should be freed by the user program.   (In  this  case,
       the value in *n is ignored.)


> +               if (strstr(line, "platform") != NULL)

Nit, to avoid multiple level of indent, how about invert the check:

if (strstr(line, "platform") == NULL)
    continue;

> +                       if (strstr(line, "PowerNV") != NULL) {
> +                               RTE_LOG(DEBUG, EAL, "Running on a PowerNV system\n");

Not really helpful, it does not indicate that this system iommu supports VA.


> +                               fclose(fp);
> +                               return true;
> +                       }
> +       }
> +       fclose(fp);
> +
>         return false;
>  }
>  #else
> --
> 2.18.1
>


-- 
David Marchand