|SUCCESS| [2/2] eal/freebsd: implement virtual to IOVA translation fn

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/164388

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-25

Reviewing patch series for DPDK coding standards and correctness.

Commit: patch 1/2 (eal/freebsd: fix memseg addresses in EAL VA mode)
Author: Bruce Richardson <bruce.richardson at intel.com>
Subject: [PATCH 1/2] eal/freebsd: fix memseg addresses in EAL VA mode

Summary: This patch fixes the IOVA assignment for memory segments on FreeBSD
by conditionally setting the IOVA to either the virtual address (in VA mode)
or the physical address (in PA mode), rather than unconditionally using the
physical address.

> diff --git a/lib/eal/freebsd/eal_memory.c b/lib/eal/freebsd/eal_memory.c
> index cd608db9f9..e56d149a22 100644
> --- a/lib/eal/freebsd/eal_memory.c
> +++ b/lib/eal/freebsd/eal_memory.c
> @@ -214,7 +214,10 @@ rte_eal_hugepage_init(void)
>  			}
>  
>  			seg->addr = addr;
> -			seg->iova = physaddr;
> +			if (rte_eal_iova_mode() == RTE_IOVA_VA)
> +				seg->iova = (uintptr_t)addr;
> +			else
> +				seg->iova = physaddr;

The coding style guide requires braces around all conditional blocks, even
single statement blocks. From the style guide: "Do not use braces where a
single statement will do... The closing brace is empty on a line of its own,
except in the cases where it is followed by a continuation of the same
statement". However, this contradicts the requirement that braces must be
used for all if/else chains. Can you confirm the intended style for this
if-else block?

>  			seg->hugepage_sz = page_sz;
>  			seg->len = page_sz;
>  			seg->nchannel = mcfg->nchannel;


Commit: patch 2/2 (eal/freebsd: implement virtual to IOVA translation fn)
Author: Bruce Richardson <bruce.richardson at intel.com>
Subject: [PATCH 2/2] eal/freebsd: implement virtual to IOVA translation fn

Summary: This patch implements rte_mem_virt2iova() for FreeBSD and updates
rte_mem_virt2phy() to use it. It also adds unit tests to verify the
implementation works correctly with rte_malloc allocations.

> diff --git a/app/test/test_memory.c b/app/test/test_memory.c
> index 628714c0b8..260d5ed8f8 100644
> --- a/app/test/test_memory.c
> +++ b/app/test/test_memory.c
> @@ -8,6 +8,7 @@
>  #include <rte_eal.h>
>  #include <rte_errno.h>
>  #include <rte_memory.h>
> +#include <rte_malloc.h>

The coding style guide requires blank lines between system and DPDK headers.
Does this file follow the correct include ordering with proper blank line
separation?

[ ... ]

> +static int
> +check_malloc_virt2iova(void)
> +{
> +	const size_t alloc_sz = 128;
> +	const size_t off = 32;
> +	struct rte_memseg *ms;
> +	char *p;
> +	rte_iova_t iova, iova_off;
> +
> +	p = rte_malloc("memory_autotest", alloc_sz, RTE_CACHE_LINE_SIZE);
> +	if (p == NULL) {
> +		printf("rte_malloc failed\n");
> +		return -1;
> +	}
> +
> +	iova = rte_mem_virt2iova(p);
> +	if (iova == RTE_BAD_IOVA) {
> +		printf("rte_mem_virt2iova failed for rte_malloc pointer\n");
> +		rte_free(p);
> +		return -1;
> +	}
> +
> +	ms = rte_mem_virt2memseg(p, NULL);
> +	if (ms == NULL) {
> +		printf("failed to resolve memseg for rte_malloc pointer\n");
> +		rte_free(p);
> +		return -1;
> +	}
> +
> +	if (rte_eal_iova_mode() == RTE_IOVA_PA) {
> +		if (ms->iova == RTE_BAD_IOVA || iova < ms->iova ||
> +					iova >= ms->iova + ms->len) {
> +			printf("translated iova is outside memseg range\n");
> +			rte_free(p);
> +			return -1;
> +		}
> +
> +		phys_addr_t physaddr = rte_mem_virt2phy(p);

The coding style guide prohibits declarations in the middle of code blocks.
Can this declaration be moved to the top of the function?

> +		if (physaddr == RTE_BAD_PHYS_ADDR || physaddr != iova) {
> +			printf("rte_mem_virt2phy failed for rte_malloc pointer\n");
> +			rte_free(p);
> +			return -1;
> +		}
> +	} else if (rte_eal_iova_mode() == RTE_IOVA_VA) {
> +		if (iova != (uintptr_t)p) {
> +			printf("rte_mem_virt2iova did not return VA in VA mode\n");
> +			rte_free(p);
> +			return -1;
> +		}
> +	}

[ ... ]

> diff --git a/lib/eal/freebsd/eal_memory.c b/lib/eal/freebsd/eal_memory.c
> index e56d149a22..6952b38630 100644
> --- a/lib/eal/freebsd/eal_memory.c
> +++ b/lib/eal/freebsd/eal_memory.c
> @@ -40,16 +40,28 @@ RTE_EXPORT_SYMBOL(rte_mem_virt2phy)
>  phys_addr_t
>  rte_mem_virt2phy(const void *virtaddr)
>  {
> -	/* XXX not implemented. This function is only used by
> -	 * rte_mempool_virt2iova() when hugepages are disabled. */
> -	(void)virtaddr;
> -	return RTE_BAD_IOVA;
> +	/* not implemented for FreeBSD when not using contigmem memory */
> +	if (virtaddr == NULL || rte_eal_iova_mode() != RTE_IOVA_PA)
> +		return RTE_BAD_IOVA;
> +	/* when IOVA == PA, return the IOVA */
> +	return rte_mem_virt2iova(virtaddr);
>  }
> +
>  RTE_EXPORT_SYMBOL(rte_mem_virt2iova)
>  rte_iova_t
>  rte_mem_virt2iova(const void *virtaddr)
>  {
> -	return rte_mem_virt2phy(virtaddr);
> +	if (virtaddr == NULL)
> +		return RTE_BAD_IOVA;
> +
> +	if (rte_eal_iova_mode() == RTE_IOVA_VA)
> +		return (uintptr_t)virtaddr;
> +
> +	const struct rte_memseg *ms = rte_mem_virt2memseg(virtaddr, NULL);

The coding style guide prohibits declarations in the middle of code blocks.
Can this declaration be moved to the top of the function?

> +	if (ms != NULL && ms->iova != RTE_BAD_IOVA)
> +		return ms->iova + RTE_PTR_DIFF(virtaddr, ms->addr);
> +
> +	return RTE_BAD_IOVA;
>  }