From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 0FD844F91; Tue, 10 Jul 2018 13:33:47 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 10 Jul 2018 04:33:47 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,334,1526367600"; d="scan'208";a="71081485" Received: from aburakov-mobl.ger.corp.intel.com (HELO [10.237.220.102]) ([10.237.220.102]) by fmsmga001.fm.intel.com with ESMTP; 10 Jul 2018 04:33:45 -0700 To: Eelco Chaudron , Alejandro Lucero Cc: dev , stable@dpdk.org, Maxime Coquelin , Ferruh Yigit References: <1530708838-2682-1-git-send-email-alejandro.lucero@netronome.com> <1530708838-2682-2-git-send-email-alejandro.lucero@netronome.com> <24D4D56B-BE3A-43DC-AEEE-A063A11C73EC@redhat.com> From: "Burakov, Anatoly" Message-ID: <33c605a8-cf16-b2f2-ce1d-cdc5d85bb155@intel.com> Date: Tue, 10 Jul 2018 12:33:45 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.0 MIME-Version: 1.0 In-Reply-To: <24D4D56B-BE3A-43DC-AEEE-A063A11C73EC@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [dpdk-stable] [PATCH v3 1/6] 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: Tue, 10 Jul 2018 11:33:48 -0000 On 10-Jul-18 12:14 PM, Eelco Chaudron wrote: > > > On 10 Jul 2018, at 12:52, Alejandro Lucero wrote: > >> On Tue, Jul 10, 2018 at 11:06 AM, Eelco Chaudron >> wrote: >> >>> >>> >>> On 10 Jul 2018, at 11:34, Alejandro Lucero wrote: >>> >>> On Tue, Jul 10, 2018 at 9:56 AM, Eelco Chaudron >>>> wrote: >>>> >>>> >>>>> >>>>> On 4 Jul 2018, at 14:53, 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. >>>>>> >>>>>> Signed-off-by: Alejandro Lucero >>>>>> Acked-by: Anatoly Burakov >>>>>> --- >>>>>>  lib/librte_eal/common/eal_common_memory.c  | 33 >>>>>> ++++++++++++++++++++++++++++++ >>>>>>  lib/librte_eal/common/include/rte_memory.h |  3 +++ >>>>>>  lib/librte_eal/rte_eal_version.map         |  1 + >>>>>>  3 files changed, 37 insertions(+) >>>>>> >>>>>> diff --git a/lib/librte_eal/common/eal_common_memory.c >>>>>> b/lib/librte_eal/common/eal_common_memory.c >>>>>> index fc6c44d..f5efebe 100644 >>>>>> --- a/lib/librte_eal/common/eal_common_memory.c >>>>>> +++ b/lib/librte_eal/common/eal_common_memory.c >>>>>> @@ -109,6 +109,39 @@ >>>>>>         } >>>>>>  } >>>>>> >>>>>> +/* check memseg iovas are within the required range based on dma >>>>>> mask >>>>>> */ >>>>>> +int >>>>>> +rte_eal_check_dma_mask(uint8_t maskbits) >>>>>> +{ >>>>>> + >>>>>> +       const struct rte_mem_config *mcfg; >>>>>> +       uint64_t mask; >>>>>> +       int i; >>>>>> + >>>>>> >>>>>> >>>>> I think we should add some sanity check to the input maskbits, i.e. >>>>> [64,0) >>>>> or [64, 32]? What would be a reasonable lower bound. >>>>> >>>>> >>>>> This is not a user's API, so any invocation will be reviewed, but I >>>>> guess >>>> adding a sanity check here does not harm. >>>> >>>> Not sure about lower bound but upper should 64, although it does not >>>> make >>>> sense but it is safe. Lower bound is not so problematic. >>>> >>>> >>>> >>>>> +       /* create dma mask */ >>>>> >>>>>> +       mask = ~((1ULL << maskbits) - 1); >>>>>> + >>>>>> +       /* get pointer to global configuration */ >>>>>> +       mcfg = rte_eal_get_configuration()->mem_config; >>>>>> + >>>>>> +       for (i = 0; i < RTE_MAX_MEMSEG; i++) { >>>>>> +               if (mcfg->memseg[i].addr == NULL) >>>>>> +                       break; >>>>>> >>>>> >>> Looking at some other code, it looks like NULL entries might exists. So >>> should a continue; rather than a break; be used here? >>> >>> >> I do not think so. memsegs are allocated sequentially, so first with addr >> as NULL implies no more memsegs. > > I was referring to the mem walk functions, rte_memseg_list_walk(). Maybe > some having more experience with this area can review/comment. Pre-18.05, all memsegs are allocated continuously. Memseg lists and memseg list walk functions are 18.05+. Alejandro, perhaps it would be worth it to tag your patchset with "pre-18.05" to avoid similar confusion in the future? -- Thanks, Anatoly