Re: [dpdk-dev] [PATCH v2 1/5] mem: add function for checking memsegs IOVAs addresses

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

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 <alejandro.lucero@netronome.com>
> ---
>   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