DPDK patches and discussions
 help / color / mirror / Atom feed
From: Olivier Matz <olivier.matz@6wind.com>
To: Jan Blunck <jblunck@infradead.org>
Cc: Gaetan Rivet <gaetan.rivet@6wind.com>, dev <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH 1/8] eal: expose rte_eal_using_phys_addrs
Date: Mon, 3 Jul 2017 13:49:13 +0200	[thread overview]
Message-ID: <20170703134913.2953592d@platinum> (raw)
In-Reply-To: <CALe+Z01cQwRfy7DgNqRhbeMZv63u+C7T9RyzzMeHPRrZJW1WPA@mail.gmail.com>

On Mon, 3 Jul 2017 12:04:37 +0200, Jan Blunck <jblunck@infradead.org> wrote:
> On Mon, Jul 3, 2017 at 10:25 AM, Olivier Matz <olivier.matz@6wind.com> wrote:
> > Hi,
> >
> > On Fri, 30 Jun 2017 21:05:47 +0200, Jan Blunck <jblunck@infradead.org> wrote:  
> >> On Thu, Jun 1, 2017 at 12:14 PM, Gaetan Rivet <gaetan.rivet@6wind.com> wrote:  
> >> > This function was previously private to the EAL layer.
> >> > Other subsystems requires it, such as the PCI bus.
> >> >
> >> > This function is only exposed for linuxapps.
> >> >
> >> > In order not to force other components to include stdbool, which is
> >> > incompatible with several NIC drivers, the return type has
> >> > been changed from bool to int.
> >> >
> >> > Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> >> > ---
> >> >  lib/librte_eal/common/eal_private.h             | 11 -----
> >> >  lib/librte_eal/linuxapp/eal/Makefile            |  2 +
> >> >  lib/librte_eal/linuxapp/eal/eal_memory.c        |  3 +-
> >> >  lib/librte_eal/linuxapp/eal/eal_pci.c           |  1 +
> >> >  lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
> >> >  lib/librte_eal/linuxapp/eal/rte_memory_linux.h  | 64 +++++++++++++++++++++++++
> >> >  6 files changed, 70 insertions(+), 12 deletions(-)
> >> >  create mode 100644 lib/librte_eal/linuxapp/eal/rte_memory_linux.h
> >> >
> >> > diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
> >> > index 6d2206a..b7e4cc6 100644
> >> > --- a/lib/librte_eal/common/eal_private.h
> >> > +++ b/lib/librte_eal/common/eal_private.h
> >> > @@ -327,17 +327,6 @@ int rte_eal_hugepage_init(void);
> >> >   */
> >> >  int rte_eal_hugepage_attach(void);
> >> >
> >> > -/**
> >> > - * Returns true if the system is able to obtain
> >> > - * physical addresses. Return false if using DMA
> >> > - * addresses through an IOMMU.
> >> > - *
> >> > - * Drivers based on uio will not load unless physical
> >> > - * addresses are obtainable. It is only possible to get
> >> > - * physical addresses when running as a privileged user.
> >> > - */
> >> > -bool rte_eal_using_phys_addrs(void);
> >> > -
> >> >  /*
> >> >   * Validate a bus name.
> >> >   *
> >> > diff --git a/lib/librte_eal/linuxapp/eal/Makefile b/lib/librte_eal/linuxapp/eal/Makefile
> >> > index 640afd0..530e286 100644
> >> > --- a/lib/librte_eal/linuxapp/eal/Makefile
> >> > +++ b/lib/librte_eal/linuxapp/eal/Makefile
> >> > @@ -131,4 +131,6 @@ INC := rte_interrupts.h rte_kni_common.h rte_dom0_common.h
> >> >  SYMLINK-$(CONFIG_RTE_EXEC_ENV_LINUXAPP)-include/exec-env := \
> >> >         $(addprefix include/exec-env/,$(INC))
> >> >
> >> > +SYMLINK-$(CONFIG_RTE_EXEC_ENV_LINUXAPP)-include += rte_memory_linux.h
> >> > +
> >> >  include $(RTE_SDK)/mk/rte.lib.mk
> >> > diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c
> >> > index ebe0683..072bfe4 100644
> >> > --- a/lib/librte_eal/linuxapp/eal/eal_memory.c
> >> > +++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
> >> > @@ -99,6 +99,7 @@
> >> >  #include "eal_internal_cfg.h"
> >> >  #include "eal_filesystem.h"
> >> >  #include "eal_hugepages.h"
> >> > +#include "rte_memory_linux.h"
> >> >
> >> >  #define PFN_MASK_SIZE  8
> >> >
> >> > @@ -1472,7 +1473,7 @@ rte_eal_hugepage_attach(void)
> >> >         return -1;
> >> >  }
> >> >
> >> > -bool
> >> > +int
> >> >  rte_eal_using_phys_addrs(void)
> >> >  {
> >> >         return phys_addrs_available;
> >> > diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
> >> > index 595622b..9d5b051 100644
> >> > --- a/lib/librte_eal/linuxapp/eal/eal_pci.c
> >> > +++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
> >> > @@ -45,6 +45,7 @@
> >> >  #include "eal_filesystem.h"
> >> >  #include "eal_private.h"
> >> >  #include "eal_pci_init.h"
> >> > +#include "rte_memory_linux.h"
> >> >
> >> >  /**
> >> >   * @file
> >> > diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> >> > index a5127d6..8916520 100644
> >> > --- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> >> > +++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> >> > @@ -207,5 +207,6 @@ DPDK_17.08 {
> >> >
> >> >         rte_bus_from_name;
> >> >         rte_bus_from_dev;
> >> > +       rte_eal_using_phys_addrs;  
> >>
> >> Is this only for the UIO mapping? Would it be better to let UIO skip
> >> (and warn?) over RTE_BAD_PHYS_ADDR mappings? That way we don't need
> >> this export and its probably more resilient to bad mappings.
> >>
> >> Thoughts? Olivier?  
> >
> > From what I see, rte_eal_using_phys_addrs() is used by:
> >
> >         rte_eal_pci_map_device(struct rte_pci_device *dev)
> >         {
> >                 ...
> >                 case RTE_KDRV_IGB_UIO:
> >                 case RTE_KDRV_UIO_GENERIC:
> >                 if (rte_eal_using_phys_addrs()) {
> >                         /* map resources for devices that use uio */
> >                         ret = pci_uio_map_resource(dev);
> >                 }
> >
> >
> > Looking at pci_uio_map_resource() and its callees, I'm not sure it's
> > easy to replace it something else. The functions that would return
> > RTE_BAD_PHYS_ADDR (virt2phy-like) are not called there.
> >  
> 
> Right, so why is it there in the first place? From what I can see the
> code should work just fine even in cases we don't have physical
> addresses available. Shouldn't the code actually requiring physical
> addresses (e.g. RX queue setup, ...) check for this instead?


My understanding of the commit cdc242f260e7 ("eal/linux: support
running as unprivileged user") is:
if dpdk is started with hugepages but without the privilege to get the
physical address, the memsegs are filled with dummy physical addresses
starting at 0. The mapping of these dummy physical addresses as IOVA is
done in eal_vfio.c. 

So, back to our question, I think I was wrong in my previous answer,
and virt2phy-like functions would not return RTE_BAD_PHYS_ADDR in that
case, they will return the dummy physical address instead.

Maybe the function rte_eal_using_phys_addrs() could have a better name,
but I think having this guard here is correct.


Olivier

  reply	other threads:[~2017-07-03 11:49 UTC|newest]

Thread overview: 63+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-06-01 10:14 [dpdk-dev] [PATCH 0/8] bus/pci: remove PCI bus from EAL Gaetan Rivet
2017-06-01 10:14 ` [dpdk-dev] [PATCH 1/8] eal: expose rte_eal_using_phys_addrs Gaetan Rivet
2017-06-30 19:05   ` Jan Blunck
2017-07-03  8:25     ` Olivier Matz
2017-07-03 10:04       ` Jan Blunck
2017-07-03 11:49         ` Olivier Matz [this message]
2017-06-01 10:14 ` [dpdk-dev] [PATCH 2/8] ethdev: remove useless PCI dependency Gaetan Rivet
2017-06-01 10:14 ` [dpdk-dev] [PATCH 3/8] pmdinfogen: move to drivers subdirectory Gaetan Rivet
2017-06-01 10:14 ` [dpdk-dev] [PATCH 4/8] cryptodev: disabled by default Gaetan Rivet
2017-06-01 10:14 ` [dpdk-dev] [PATCH 5/8] eventdev: " Gaetan Rivet
2017-06-01 10:14 ` [dpdk-dev] [PATCH 6/8] pdump: " Gaetan Rivet
2017-06-01 10:14 ` [dpdk-dev] [PATCH 7/8] kni: " Gaetan Rivet
2017-06-01 10:14 ` [dpdk-dev] [PATCH 8/8] bus/pci: introduce pci bus Gaetan Rivet
2017-06-07 23:58 ` [dpdk-dev] [PATCH 0/8] bus/pci: remove PCI bus from EAL Gaetan Rivet
2017-06-07 23:58   ` [dpdk-dev] [PATCH v2 01/12] eal: expose rte_eal_using_phys_addrs Gaetan Rivet
2017-06-07 23:58   ` [dpdk-dev] [PATCH v2 02/12] ethdev: remove useless PCI dependency Gaetan Rivet
2017-06-07 23:58   ` [dpdk-dev] [PATCH v2 03/12] bus: properly include rte_debug Gaetan Rivet
2017-06-07 23:59   ` [dpdk-dev] [PATCH v2 04/12] eal: remove references to PCI Gaetan Rivet
2017-06-07 23:59   ` [dpdk-dev] [PATCH v2 05/12] pmdinfogen: move to drivers subdirectory Gaetan Rivet
2017-06-07 23:59   ` [dpdk-dev] [PATCH v2 06/12] cryptodev: disabled by default Gaetan Rivet
2017-06-09 15:03     ` De Lara Guarch, Pablo
2017-06-14  8:00       ` Gaëtan Rivet
2017-06-07 23:59   ` [dpdk-dev] [PATCH v2 07/12] pdump: " Gaetan Rivet
2017-06-09 14:24     ` Pattan, Reshma
2017-06-11 19:42       ` Gaëtan Rivet
2017-06-13 17:15         ` Ferruh Yigit
2017-06-14 23:01           ` Gaëtan Rivet
2017-06-15 13:07             ` Ferruh Yigit
2017-06-14  9:09         ` Pattan, Reshma
2017-06-14  9:20           ` Gaëtan Rivet
2017-06-07 23:59   ` [dpdk-dev] [PATCH v2 08/12] kni: " Gaetan Rivet
2017-06-09  8:56     ` Ferruh Yigit
2017-06-09  9:06       ` Gaëtan Rivet
2017-06-15 13:09         ` Ferruh Yigit
2017-06-15 14:48           ` Gaëtan Rivet
2017-06-07 23:59   ` [dpdk-dev] [PATCH v2 09/12] bus/pci: introduce pci bus Gaetan Rivet
2017-06-07 23:59   ` [dpdk-dev] [PATCH v2 10/12] bus/pci: follow checkpatch Gaetan Rivet
2017-06-07 23:59   ` [dpdk-dev] [PATCH v2 11/12] drivers: update eventdev dependencies Gaetan Rivet
2017-06-07 23:59   ` [dpdk-dev] [PATCH v2 12/12] drivers: update cryptodev dependencies Gaetan Rivet
2017-06-20 23:36   ` [dpdk-dev] [PATCH v3 0/9] bus/pci: remove PCI bus from EAL Gaetan Rivet
2017-06-20 23:36     ` [dpdk-dev] [PATCH v3 1/9] kni: disabled by default Gaetan Rivet
2017-06-20 23:36     ` [dpdk-dev] [PATCH v3 2/9] eal: expose rte_eal_using_phys_addrs Gaetan Rivet
2017-06-21  7:44       ` Thomas Monjalon
2017-06-21  9:21         ` Gaëtan Rivet
2017-06-20 23:36     ` [dpdk-dev] [PATCH v3 3/9] ethdev: remove useless PCI dependency Gaetan Rivet
2017-06-20 23:36     ` [dpdk-dev] [PATCH v3 4/9] bus: properly include rte_debug Gaetan Rivet
2017-06-21  7:45       ` Thomas Monjalon
2017-06-21  9:26         ` Gaëtan Rivet
2017-06-20 23:36     ` [dpdk-dev] [PATCH v3 5/9] pmdinfogen: move to drivers subdirectory Gaetan Rivet
2017-06-21  7:57       ` Thomas Monjalon
2017-06-21  9:40         ` Gaëtan Rivet
2017-06-21 10:00           ` Thomas Monjalon
2017-06-21 11:39             ` Gaëtan Rivet
2017-06-21 12:14               ` Thomas Monjalon
2017-06-20 23:36     ` [dpdk-dev] [PATCH v3 6/9] bus/pci: introduce pci bus Gaetan Rivet
2017-06-20 23:36     ` [dpdk-dev] [PATCH v3 7/9] bus/pci: follow checkpatch Gaetan Rivet
2017-06-20 23:36     ` [dpdk-dev] [PATCH v3 8/9] drivers: update eventdev dependencies Gaetan Rivet
2017-06-20 23:36     ` [dpdk-dev] [PATCH v3 9/9] drivers: update cryptodev dependencies Gaetan Rivet
2017-06-23  3:29     ` [dpdk-dev] [PATCH v3 0/9] bus/pci: remove PCI bus from EAL Tan, Jianfeng
2017-06-23  8:19       ` Gaëtan Rivet
2017-06-23 12:48         ` Thomas Monjalon
2017-06-23 14:35           ` Tan, Jianfeng
2017-06-23 14:45             ` Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170703134913.2953592d@platinum \
    --to=olivier.matz@6wind.com \
    --cc=dev@dpdk.org \
    --cc=gaetan.rivet@6wind.com \
    --cc=jblunck@infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).