Re: [PATCH 3/5] common/dpaax: add library for PA VA translation table

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

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