From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Burakov, Anatoly" <anatoly.burakov@intel.com>
Subject: Re: [PATCH 3/5] common/dpaax: add library for PA VA
	translation table
Date: Tue, 25 Sep 2018 14:28:20 +0100
Message-ID: <afdf0eb5-433f-0e65-1772-3564a129bf44@intel.com>
References: <20180925125423.7505-1-shreyansh.jain@nxp.com>
 <20180925125423.7505-4-shreyansh.jain@nxp.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Cc: dev@dpdk.org
To: Shreyansh Jain <shreyansh.jain@nxp.com>, ferruh.yigit@intel.com
Return-path: <dev-bounces@dpdk.org>
Received: from mga04.intel.com (mga04.intel.com [192.55.52.120])
 by dpdk.org (Postfix) with ESMTP id CD1491B12B
 for <dev@dpdk.org>; Tue, 25 Sep 2018 15:28:30 +0200 (CEST)
In-Reply-To: <20180925125423.7505-4-shreyansh.jain@nxp.com>
Content-Language: en-US
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

On 25-Sep-18 1:54 PM, Shreyansh Jain wrote:
> A common library, valid for dpaaX drivers, which is used to maintain
> a local copy of PA->VA translations.
> 
> In case of physical addressing mode (one of the option for FSLMC, and
> only option for DPAA bus), the addresses of descriptors Rx'd are
> physical. These need to be converted into equivalent VA for rte_mbuf
> and other similar calls.
> 
> Using the rte_mem_virt2iova or rte_mem_virt2phy is expensive. This
> library is an attempt to reduce the overall cost associated with
> this translation.
> 
> A small table is maintained, containing continuous entries
> representing a continguous physical range. Each of these entries
> stores the equivalent VA, which is fed during mempool creation, or
> memory allocation/deallocation callbacks.
> 
> Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
> ---

Hi Shreyansh,

So, basically, you're reimplementing old DPDK's memory view (storing 
VA's in a PA-centric way). Makes sense :)

I should caution you that right now, external memory allocator 
implementation does *not* trigger any callbacks for newly added memory. 
So, anything coming from external memory will not be reflected in your 
table, unless it happens to be already there before 
dpaax_iova_table_populate() gets called. This patchset makes a good 
argument for why perhaps it should trigger callbacks. Thoughts?

Also, a couple of nitpicks below.

>   config/common_base                            |   5 +
>   config/common_linuxapp                        |   5 +
>   drivers/common/Makefile                       |   4 +
>   drivers/common/dpaax/Makefile                 |  31 ++
>   drivers/common/dpaax/dpaax_iova_table.c       | 509 ++++++++++++++++++
>   drivers/common/dpaax/dpaax_iova_table.h       | 104 ++++
>   drivers/common/dpaax/dpaax_logs.h             |  39 ++
>   drivers/common/dpaax/meson.build              |  12 +

<snip>

> +	DPAAX_DEBUG("Add: Found slot at (%"PRIu64")[(%zu)] for vaddr:(%p),"
> +		    " phy(%"PRIu64"), len(%zu)", entry[i].start, e_offset,
> +		    vaddr, paddr, length);
> +	return 0;
> +}
> +
> +int
> +dpaax_iova_table_del(phys_addr_t paddr, size_t len __rte_unused)

len is not unused.

> +{
> +	int found = 0;
> +	unsigned int i;
> +	size_t e_offset;
> +	struct dpaax_iovat_element *entry;
> +	phys_addr_t align_paddr;
> +
> +	align_paddr = paddr & DPAAX_MEM_SPLIT_MASK;
> +
> +	/* Check if paddr is available in table */

<snip>

> +static inline void *
> +dpaax_iova_table_get_va(phys_addr_t paddr) {
> +	unsigned int i = 0, index;
> +	void *vaddr = 0;
> +	phys_addr_t paddr_align = paddr & DPAAX_MEM_SPLIT_MASK;
> +	size_t offset = paddr & DPAAX_MEM_SPLIT_MASK_OFF;
> +	struct dpaax_iovat_element *entry;
> +
> +	entry = dpaax_iova_table_p->entries;
> +
> +	do {
> +		if (unlikely(i > dpaax_iova_table_p->count))
> +			break;
> +
> +		if (paddr_align < entry[i].start) {
> +			/* Incorrect paddr; Not in memory range */
> +			return 0;

NULL?

-- 
Thanks,
Anatoly