Re: [dpdk-dev] [PATCH 1/8] eal: expose rte_eal_using_phys_addrs

[Olivier Matz <olivier.matz@6wind.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