|SUCCESS| [v10,5/5] vhost_user: enable configure memory slots

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-08

I'll review this DPDK vhost patch series for correctness bugs, coding style violations, and guideline compliance.

commit 20260407224205.3910803-2-pravin.bathija at dell.com
Author: Pravin M Bathija <pravin.bathija at dell.com>

[PATCH v10 1/5] vhost: add user to mailmap and define to vhost hdr

This commit adds a user to the mailmap and defines
VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS to indicate support for
add/remove memory region functionality.

> diff --git a/.mailmap b/.mailmap
> index 0e0d83e1c6..cc44e27036 100644
> --- a/.mailmap
> +++ b/.mailmap
> @@ -1295,6 +1295,7 @@ Prateek Agarwal <prateekag at cse.iitb.ac.in>
>  Prathisna Padmasanan <prathisna.padmasanan at intel.com>
>  Praveen Kaligineedi <pkaligineedi at google.com>
>  Praveen Shetty <praveen.shetty at intel.com>
> +Pravin M Bathija <pravin.bathija at dell.com>
>  Pravin Pathak <pravin.pathak.dev at gmail.com> <pravin.pathak at intel.com>
>  Prince Takkar <ptakkar at marvell.com>
>  Priyalee Kushwaha <priyalee.kushwaha at intel.com>
> diff --git a/lib/vhost/rte_vhost.h b/lib/vhost/rte_vhost.h
> index 2f7c4c0080..a7f9700538 100644
> --- a/lib/vhost/rte_vhost.h
> +++ b/lib/vhost/rte_vhost.h
> @@ -109,6 +109,10 @@ extern "C" {
>  #define VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD 12
>  #endif
>  
> +#ifndef VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS
> +#define VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS 15
> +#endif

Does this ifndef guard align with the pattern used elsewhere in the
file? I see VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD has a guard, but
does VHOST_USER_PROTOCOL_F_STATUS below? This should be consistent.

> +
>  #ifndef VHOST_USER_PROTOCOL_F_STATUS
>  #define VHOST_USER_PROTOCOL_F_STATUS 16
>  #endif

---

commit 20260407224205.3910803-3-pravin.bathija at dell.com
Author: Pravin M Bathija <pravin.bathija at dell.com>

[PATCH v10 2/5] vhost_user: header defines for add/rem mem region

This commit adds enum message requests and data structures for
supporting add/remove memory regions.

> diff --git a/lib/vhost/vhost_user.h b/lib/vhost/vhost_user.h
> index ef486545ba..f8d921f7f1 100644
> --- a/lib/vhost/vhost_user.h
> +++ b/lib/vhost/vhost_user.h
> @@ -67,6 +67,9 @@ typedef enum VhostUserRequest {
>  	VHOST_USER_POSTCOPY_END = 30,
>  	VHOST_USER_GET_INFLIGHT_FD = 31,
>  	VHOST_USER_SET_INFLIGHT_FD = 32,
> +	VHOST_USER_GET_MAX_MEM_SLOTS = 36,
> +	VHOST_USER_ADD_MEM_REG = 37,
> +	VHOST_USER_REM_MEM_REG = 38,
>  	VHOST_USER_SET_STATUS = 39,
>  	VHOST_USER_GET_STATUS = 40,
>  } VhostUserRequest;

[ ... ]

> @@ -186,6 +194,7 @@ typedef struct __rte_packed_begin VhostUserMsg {
>  		struct vhost_vring_state state;
>  		struct vhost_vring_addr addr;
>  		VhostUserMemory memory;
> +		VhostUserSingleMemReg memory_single;
>  		VhostUserLog    log;
>  		struct vhost_iotlb_msg iotlb;
>  		VhostUserCryptoSessionParam crypto_session;

Does adding VhostUserSingleMemReg to this union maintain proper
alignment? Should __rte_aligned be applied to the VhostUserSingleMemReg
structure definition?

---

commit 20260407224205.3910803-4-pravin.bathija at dell.com
Author: Pravin M Bathija <pravin.bathija at dell.com>

[PATCH v10 3/5] vhost_user: support function defines for back-end

This commit defines common support functions for memory region
operations used by set memory table, add memory region, and remove
memory region message handlers.

> diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
> index 4bfb13fb98..1f96ecf963 100644
> --- a/lib/vhost/vhost_user.c
> +++ b/lib/vhost/vhost_user.c

[ ... ]

> +static int
> +async_dma_map_region(struct virtio_net *dev, struct rte_vhost_mem_region *reg, bool do_map)
> +{
> +	uint32_t i;
> +	int ret;
> +	uint64_t reg_start = reg->host_user_addr;
> +	uint64_t reg_end = reg_start + reg->size;
> +
> +	for (i = 0; i < dev->nr_guest_pages; i++) {
> +		struct guest_page *page = &dev->guest_pages[i];
> +
> +		/* Only process pages belonging to this region */
> +		if (page->host_user_addr < reg_start ||
> +		    page->host_user_addr >= reg_end)
> +			continue;

Does this code handle overflow correctly when reg_start + reg->size
exceeds UINT64_MAX? Should there be a check for this condition?

> +
> +		if (do_map) {
> +			ret = rte_vfio_container_dma_map(RTE_VFIO_DEFAULT_CONTAINER_FD,
> +					page->host_user_addr,
> +					page->host_iova,
> +					page->size);
> +			if (ret) {
> +				if (rte_errno == ENODEV)
> +					return 0;
> +
> +				VHOST_CONFIG_LOG(dev->ifname, ERR, "DMA engine map failed");
> +				return -1;
> +			}
> +		} else {
> +			ret = rte_vfio_container_dma_unmap(RTE_VFIO_DEFAULT_CONTAINER_FD,
> +					page->host_user_addr,
> +					page->host_iova,
> +					page->size);
> +			if (ret) {
> +				if (rte_errno == EINVAL)
> +					return 0;

If async_dma_map_region() partially unmaps regions before hitting this
error path, does the caller have any way to know which regions were
successfully unmapped? Should this function attempt to roll back partial
changes on failure?

[ ... ]

>  static void
> -free_mem_region(struct virtio_net *dev)
> +free_mem_region(struct rte_vhost_mem_region *reg)
> +{
> +	if (reg != NULL && reg->mmap_addr) {

Does this check follow the implicit comparison style guidelines? Should
it be written as "if (reg && reg->mmap_addr)"?

> +		munmap(reg->mmap_addr, reg->mmap_size);
> +		close(reg->fd);
> +		memset(reg, 0, sizeof(struct rte_vhost_mem_region));
> +	}
> +}

[ ... ]

> +static void
> +remove_guest_pages(struct virtio_net *dev, struct rte_vhost_mem_region *reg)
> +{
> +	uint64_t reg_start = reg->host_user_addr;
> +	uint64_t reg_end = reg_start + reg->size;
> +	uint32_t i, j = 0;
> +
> +	for (i = 0; i < dev->nr_guest_pages; i++) {
> +		if (dev->guest_pages[i].host_user_addr >= reg_start &&
> +		    dev->guest_pages[i].host_user_addr < reg_end)
> +			continue;

Does this range check handle overflow when reg_start + reg->size
overflows? The same concern applies here as in async_dma_map_region().

> +		if (j != i)
> +			dev->guest_pages[j] = dev->guest_pages[i];
> +		j++;
> +	}
> +	dev->nr_guest_pages = j;
> +}

[ ... ]

> +static int
> +vhost_user_initialize_memory(struct virtio_net **pdev)
> +{
> +	struct virtio_net *dev = *pdev;
> +	int numa_node = SOCKET_ID_ANY;
> +
> +	if (dev->mem != NULL) {

Does this follow the implicit comparison style? Should it be written as
"if (dev->mem)"?

> +		VHOST_CONFIG_LOG(dev->ifname, ERR,
> +			"memory already initialized, free it first");
> +		return -1;
> +	}

[ ... ]

> +	dev->mem = rte_zmalloc_socket("vhost-mem-table", sizeof(struct rte_vhost_memory) +
> +		sizeof(struct rte_vhost_mem_region) * VHOST_MEMORY_MAX_NREGIONS, 0, numa_node);
> +	if (dev->mem == NULL) {

Does this follow implicit comparison style? Should it be "if (!dev->mem)"?

> +		VHOST_CONFIG_LOG(dev->ifname, ERR, "failed to allocate memory for dev->mem");
> +		rte_free(dev->guest_pages);
> +		dev->guest_pages = NULL;
> +		return -1;
> +	}
> +
> +	return 0;
> +}

---

commit 20260407224205.3910803-5-pravin.bathija at dell.com
Author: Pravin M Bathija <pravin.bathija at dell.com>

[PATCH v10 4/5] vhost_user: Function defs for add/rem mem regions

This commit implements the message handlers for get max memory slots,
add memory region, and remove memory region along with vring
invalidation support.

> diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
> index 1f96ecf963..533a8cf3ab 100644
> --- a/lib/vhost/vhost_user.c
> +++ b/lib/vhost/vhost_user.c

[ ... ]

> +static int
> +vhost_user_get_max_mem_slots(struct virtio_net **pdev __rte_unused,
> +			struct vhu_msg_context *ctx,
> +			int main_fd __rte_unused)
> +{
> +	uint32_t max_mem_slots = VHOST_MEMORY_MAX_NREGIONS;
> +
> +	ctx->msg.payload.u64 = (uint64_t)max_mem_slots;
> +	ctx->msg.size = sizeof(ctx->msg.payload.u64);
> +	ctx->fd_num = 0;
> +
> +	return RTE_VHOST_MSG_RESULT_REPLY;
> +}

[ ... ]

> +static void
> +_dev_invalidate_vrings(struct virtio_net **pdev)
> +{
> +	struct virtio_net *dev = *pdev;
> +	uint32_t i;
> +
> +	for (i = 0; i < dev->nr_vring; i++) {
> +		struct vhost_virtqueue *vq = dev->virtqueue[i];
> +
> +		if (!vq)
> +			continue;
> +
> +		if (vq->desc || vq->avail || vq->used) {

Are these vring pointers protected by the queue lock throughout this
operation? Can another thread modify them during this check?

> +			vq_assert_lock(dev, vq);
> +
> +			/*
> +			 * If the memory table got updated, the ring addresses
> +			 * need to be translated again as virtual addresses have
> +			 * changed.
> +			 */
> +			vring_invalidate(dev, vq);
> +
> +			translate_ring_addresses(&dev, &vq);
> +		}
> +	}
> +
> +	*pdev = dev;
> +}

[ ... ]

> +static int
> +vhost_user_add_mem_reg(struct virtio_net **pdev,
> +			struct vhu_msg_context *ctx,
> +			int main_fd __rte_unused)
> +{
> +	uint32_t i;
> +	struct virtio_net *dev = *pdev;
> +	struct VhostUserMemoryRegion *region = &ctx->msg.payload.memory_single.region;
> +
> +	/* convert first region add to normal memory table set */
> +	if (dev->mem == NULL) {

Does this follow implicit comparison style? Should it be "if (!dev->mem)"?

> +		if (vhost_user_initialize_memory(pdev) < 0)
> +			goto close_msg_fds;
> +	}
> +
> +	/* make sure new region will fit */
> +	if (dev->mem->nregions >= VHOST_MEMORY_MAX_NREGIONS) {
> +		VHOST_CONFIG_LOG(dev->ifname, ERR, "too many memory regions already (%u)",
> +									dev->mem->nregions);

Does this line exceed 79 characters? The function call parameters should
be formatted to fit within the line length limit.

> +		goto close_msg_fds;
> +	}
> +
> +	/* make sure supplied memory fd present */
> +	if (ctx->fd_num != 1) {
> +		VHOST_CONFIG_LOG(dev->ifname, ERR, "fd count makes no sense (%u)", ctx->fd_num);
> +		goto close_msg_fds;
> +	}
> +
> +	/* Make sure no overlap in guest virtual address space */
> +	for (i = 0; i < dev->mem->nregions; i++) {
> +		struct rte_vhost_mem_region *current_region = &dev->mem->regions[i];
> +		uint64_t current_region_guest_start = current_region->guest_user_addr;
> +		uint64_t current_region_guest_end = current_region_guest_start
> +							+ current_region->size - 1;

Can this arithmetic overflow when current_region->size is large? Should
there be overflow checking here?

> +		uint64_t proposed_region_guest_start = region->userspace_addr;
> +		uint64_t proposed_region_guest_end = proposed_region_guest_