[dpdk-dev] Fwd: [PATCH v2 1/5] mem: add function for checking memsegs IOVAs addresses

[Alejandro Lucero <alejandro.lucero@netronome.com>]

I sent this email only to Anatoly. Sending it again to mailing list.

On Wed, Oct 3, 2018 at 1:43 PM Burakov, Anatoly <anatoly.burakov@intel.com>
wrote:

> On 31-Aug-18 1:50 PM, Alejandro Lucero wrote:
> > A device can suffer addressing limitations. This functions checks
> > memsegs have iovas within the supported range based on dma mask.
> >
> > PMD should use this during initialization if supported devices
> > suffer addressing limitations, returning an error if this function
> > returns memsegs out of range.
> >
> > Another potential usage is for emulated IOMMU hardware with addressing
> > limitations.
> >
> > It is necessary to save the most restricted dma mask for checking
> > memory allocated dynamically after initialization.
> >
> > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> > ---
> >   lib/librte_eal/common/eal_common_memory.c         | 56
> +++++++++++++++++++++++
> >   lib/librte_eal/common/include/rte_eal_memconfig.h |  3 ++
> >   lib/librte_eal/common/include/rte_memory.h        |  3 ++
> >   lib/librte_eal/common/malloc_heap.c               | 12 +++++
> >   lib/librte_eal/linuxapp/eal/eal.c                 |  2 +
> >   lib/librte_eal/rte_eal_version.map                |  1 +
> >   6 files changed, 77 insertions(+)
> >
> > diff --git a/lib/librte_eal/common/eal_common_memory.c
> b/lib/librte_eal/common/eal_common_memory.c
> > index fbfb1b0..bdd8f44 100644
> > --- a/lib/librte_eal/common/eal_common_memory.c
> > +++ b/lib/librte_eal/common/eal_common_memory.c
> > @@ -383,6 +383,62 @@ struct virtiova {
> >       rte_memseg_walk(dump_memseg, f);
> >   }
> >
> > +static int
> > +check_iova(const struct rte_memseg_list *msl __rte_unused,
> > +             const struct rte_memseg *ms, void *arg)
> > +{
> > +     uint64_t *mask = arg;
> > +     rte_iova_t iova;
> > +
> > +     /* higher address within segment */
> > +     iova = (ms->iova + ms->len) - 1;
> > +     if (!(iova & *mask))
> > +             return 0;
> > +
> > +     RTE_LOG(INFO, EAL, "memseg iova %"PRIx64", len %zx, out of
> range\n",
> > +                        ms->iova, ms->len);
> > +
> > +     RTE_LOG(INFO, EAL, "\tusing dma mask %"PRIx64"\n", *mask);
>
> IMO putting these as INFO is overkill. I'd prefer not to spam the output
> unless it's really important. Can this go under DEBUG?
>
>
This checks comes from a device or from the alloc_pages_on_heap when
expanding memory. If the check discovers an address out of mask, a device
can not be used or the new memory can not be allocated. I think having this
info will help to understand why the device initialization or the memory
allocation are failing.


> Also, the message is misleading. You stop before you have a chance to
> check other masks, which may restrict them even further. You're
> outputting the message about using DMA mask XXX but this may not be the
> final DMA mask.
>

Well, this is the first triggering, and it is enough for reporting the
problem and avoiding the device or the new memory to be used.

Note that the mask is per device, and for the memory allocation case, it is
the most restrictive dma mask. So there are no other masks to try.



>
> > +     /* Stop the walk and change mask */
> > +     *mask = 0;
> > +     return 1;
> > +}
> > +
> > +#if defined(RTE_ARCH_64)
> > +#define MAX_DMA_MASK_BITS 63
> > +#else
> > +#define MAX_DMA_MASK_BITS 31
> > +#endif
> > +
> > +/* check memseg iovas are within the required range based on dma mask */
> > +int __rte_experimental
> > +rte_eal_check_dma_mask(uint8_t maskbits)
> > +{
> > +     struct rte_mem_config *mcfg =
> rte_eal_get_configuration()->mem_config;
> > +     uint64_t mask;
> > +
> > +     /* sanity check */
> > +     if (maskbits > MAX_DMA_MASK_BITS) {
> > +             RTE_LOG(INFO, EAL, "wrong dma mask size %u (Max: %u)\n",
> > +                                maskbits, MAX_DMA_MASK_BITS);
>
> Should be ERR, not INFO.
>
>
Right. I will change it.


> > +             return -1;
> > +     }
> > +
> > +     /* keep the more restricted maskbit */
> > +     if (!mcfg->dma_maskbits || maskbits < mcfg->dma_maskbits)
> > +             mcfg->dma_maskbits = maskbits;
>
> Do we need to modify mcfg->dma_maskbits before we know if we're going to
> fail? Suggest using a local variable maybe?
>
>
Yes, that's true. If the check fails, the device will not be used therefore
we do not need to keep that dma mask at all.
I will change the order here.
Thanks!


> Also, i think it's a good case for ternary:
>
> bits = mcfg->dma_maskbits == 0 ?
>         maskbits :
>         RTE_MIN(maskbits, mcfg->dma_maskbits);
>
> IMO the intention looks much clearer.
>
>
Agree.


> > +
> > +     /* create dma mask */
> > +     mask = ~((1ULL << maskbits) - 1);
> > +
> > +     rte_memseg_walk(check_iova, &mask);
> > +
> > +     if (!mask)
> > +             return -1;
> > +
> > +     return 0;
> > +}
> > +
> >   /* return the number of memory channels */
> >   unsigned rte_memory_get_nchannel(void)
> >   {
> > diff --git a/lib/librte_eal/common/include/rte_eal_memconfig.h
> b/lib/librte_eal/common/include/rte_eal_memconfig.h
> > index aff0688..aea44cb 100644
> > --- a/lib/librte_eal/common/include/rte_eal_memconfig.h
> > +++ b/lib/librte_eal/common/include/rte_eal_memconfig.h
> > @@ -77,6 +77,9 @@ struct rte_mem_config {
> >        * exact same address the primary process maps it.
> >        */
> >       uint64_t mem_cfg_addr;
> > +
> > +     /* keeps the more restricted dma mask */
> > +     uint8_t dma_maskbits;
>
> This needs to be documented as an ABI break in the 18.11 release notes.
>
>
Ok. I'll add that in the next version.
Thanks


>
> --
> Thanks,
> Anatoly
>