From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by dpdk.org (Postfix) with ESMTP id 4A7B21B120; Wed, 3 Oct 2018 14:43:22 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 03 Oct 2018 05:43:21 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,336,1534834800"; d="scan'208";a="97055663" Received: from aburakov-mobl1.ger.corp.intel.com (HELO [10.237.220.113]) ([10.237.220.113]) by orsmga002.jf.intel.com with ESMTP; 03 Oct 2018 05:43:18 -0700 To: Alejandro Lucero , dev@dpdk.org Cc: stable@dpdk.org References: <1535719857-19092-1-git-send-email-alejandro.lucero@netronome.com> <1535719857-19092-2-git-send-email-alejandro.lucero@netronome.com> From: "Burakov, Anatoly" Message-ID: <1ec4866d-24d4-1881-dfbe-ca2ff878e8c9@intel.com> Date: Wed, 3 Oct 2018 13:43:16 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <1535719857-19092-2-git-send-email-alejandro.lucero@netronome.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v2 1/5] mem: add function for checking memsegs IOVAs addresses X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 03 Oct 2018 12:43:23 -0000 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 > --- > 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? 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. > + /* 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. > + 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? 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. > + > + /* 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. -- Thanks, Anatoly