From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 22249A0540; Mon, 13 Jul 2020 10:56:49 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 782431D544; Mon, 13 Jul 2020 10:56:48 +0200 (CEST) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id 91D5B1D543 for ; Mon, 13 Jul 2020 10:56:46 +0200 (CEST) IronPort-SDR: PUyArKs1tAka7GOH92UHm+JnmCRaO92G7s5AeuqsM5r/fhohEHQKoBfO4S8eOm+rT+RdNxx1B/ Od9N8wEQrGEA== X-IronPort-AV: E=McAfee;i="6000,8403,9680"; a="210115227" X-IronPort-AV: E=Sophos;i="5.75,347,1589266800"; d="scan'208";a="210115227" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 13 Jul 2020 01:56:45 -0700 IronPort-SDR: naVB/0nddM1L/Eq7L78pMvb3MyI0GewFuZBcULvQoQ9f9sEvuAFkSkCQykLsGcnZQ4gIE6wgV8 QJqO4++hyrBQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.75,347,1589266800"; d="scan'208";a="485403951" Received: from cpeters1-mobl.ger.corp.intel.com (HELO [10.213.225.87]) ([10.213.225.87]) by fmsmga005.fm.intel.com with ESMTP; 13 Jul 2020 01:56:42 -0700 To: Thomas Monjalon Cc: dev@dpdk.org, david.marchand@redhat.com, ferruh.yigit@intel.com, grive@u256.net, alvinx.zhang@intel.com, beilei.xing@intel.com, jia.guo@intel.com, bruce.richardson@intel.com, dmitry.kozliuk@gmail.com, navasile@linux.microsoft.com, dmitrym@microsoft.com, pallavi.kadam@intel.com, talshn@mellanox.com References: <20200710115324.3902559-1-thomas@monjalon.net> <3151e427-77af-80c0-e53b-4e107bb1a40c@intel.com> <2414408.smBOq31esu@thomas> From: "Burakov, Anatoly" Message-ID: <63cf0ddf-3d12-06ab-1fd0-a27a827bea74@intel.com> Date: Mon, 13 Jul 2020 09:56:41 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.9.0 MIME-Version: 1.0 In-Reply-To: <2414408.smBOq31esu@thomas> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] pci: keep API compatibility with mmap values 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 10-Jul-20 5:17 PM, Thomas Monjalon wrote: > 10/07/2020 17:39, Burakov, Anatoly: >> On 10-Jul-20 12:53 PM, Thomas Monjalon wrote: >>> The function pci_map_resource() returns MAP_FAILED in case of error. >>> When replacing the call to mmap() by rte_mem_map(), >>> the error code became NULL, breaking the API. >>> This function is probably not used outside of DPDK, >>> but it is still a problem for two reasons: >>> - the deprecation process was not followed >>> - the Linux function pci_vfio_mmap_bar() is broken for i40e >>> >>> The error code is reverted to the Unix value MAP_FAILED. >>> Windows needs to define this special value (-1 as in Unix). >>> After proper deprecation process, the API could be changed again >>> if really needed. >>> >>> Because of the switch from mmap() to rte_mem_map(), >>> another part of the API was changed: "int additional_flags" >>> are defined as "additional flags for the mapping range" >>> without mentioning it was directly used in mmap(). >>> Currently it is directly used in rte_mem_map(), >>> that's why the values rte_map_flags must be mapped (sic) on the mmap ones >>> in case of Unix OS. >>> >>> These are side effects of a badly defined API using Unix values. > [...] >>> /** Additional flags for memory mapping. */ >>> enum rte_map_flags { >>> +#ifdef RTE_EXEC_ENV_WINDOWS >>> /** Changes to the mapped memory are visible to other processes. */ >>> RTE_MAP_SHARED = 1 << 0, >>> /** Mapping is not backed by a regular file. */ >>> @@ -35,6 +37,12 @@ enum rte_map_flags { >>> * it is not required to do so, thus mapping with this flag may fail. >>> */ >>> RTE_MAP_FORCE_ADDRESS = 1 << 3 >>> +#else /* map mmap flags because they are exposed in pci_map_resource() API */ >>> + RTE_MAP_SHARED = MAP_SHARED, >>> + RTE_MAP_ANONYMOUS = MAP_ANONYMOUS, >>> + RTE_MAP_PRIVATE = MAP_PRIVATE, >>> + RTE_MAP_FORCE_ADDRESS = MAP_FIXED, >>> +#endif >> >> I'm probably missing something, but why is this needed? Doesn't > > Yes you missed reading the commit log :) > Or maybe it is not written clearly enough. Will try to rephrase. > >> rte_mem_map() automatically translate these flags into proper ones? >> pci_map_resource() will call rte_mem_map(), and that will translate >> these flags into their Unix equivalents. > > The problem is that we have an API which is taking mmap flags as input. > "int additional_flags" is a parameter of the function, > and are supposed to be mmap flags. But it is not stated clearly. > When Windows will use this function, it won't use mmap flags > but RTE_MAP_*. So we must accept both. > That's why the best is to make values the same. > > In 20.11, we could change the API, > make clear that only RTE_MAP_* is accepted, > and remove this workaround. > Or even better, remove pci_map_resource from the PCI lib, > and implement it in the PCI bus driver. > > pci_map_resource() function is a bad designed API > > Right, this makes it clearer :) Thanks! -- Thanks, Anatoly