|SUCCESS| [v13,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/164005

_AI Code Review_

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

Commit: (SHA not provided in patch file)
Author: Pravin M Bathija <pravin.bathija at dell.com>
Subject: [PATCH v13 1/5] vhost: add user to mailmap and define to vhost hdr

This patchset adds support for dynamically adding and removing memory
regions in the vhost-user backend, enabling memory hotplug scenarios.

> 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 need the ifndef guard? Is this expected to be defined
elsewhere, or can it be an unconditional define?

> diff --git a/lib/vhost/vhost_user.h b/lib/vhost/vhost_user.h
> index ef486545ba..6435816534 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;
> @@ -91,6 +94,11 @@ typedef struct VhostUserMemory {
>  	VhostUserMemoryRegion regions[VHOST_MEMORY_MAX_NREGIONS];
>  } VhostUserMemory;
>  
> +typedef struct VhostUserMemRegMsg {
> +	uint64_t padding;
> +	VhostUserMemoryRegion region;
> +} VhostUserMemRegMsg;

This struct should be marked with __rte_packed to ensure proper
alignment when sent over the wire. Does this need explicit packing to
match the vhost-user protocol layout?

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

[ ... ]

> diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
> index 4bfb13fb98..0ee3fe7a5e 100644
> --- a/lib/vhost/vhost_user.c
> +++ b/lib/vhost/vhost_user.c
> @@ -171,20 +171,27 @@ get_blk_size(int fd)
>  	return ret == -1 ? (uint64_t)-1 : (uint64_t)stat.st_blksize;
>  }
>  
> -static void
> -async_dma_map(struct virtio_net *dev, bool do_map)
> +static int
> +async_dma_map_region(struct virtio_net *dev, struct rte_vhost_mem_region *reg, bool do_map)
>  {
> -	int ret = 0;
>  	uint32_t i;
> -	struct guest_page *page;
> +	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;
>  
> -	if (do_map) {
> -		for (i = 0; i < dev->nr_guest_pages; i++) {
> -			page = &dev->guest_pages[i];
> +		if (do_map) {
>  			ret = rte_vfio_container_dma_map(RTE_VFIO_DEFAULT_CONTAINER_FD,
> -							 page->host_user_addr,
> -							 page->host_iova,
> -							 page->size);
> +					page->host_user_addr,
> +					page->host_iova,
> +					page->size);

Does this alignment preserve the original tab/space pattern? Code
should use tabs for indentation with spaces for alignment.

> @@ -199,33 +206,57 @@ async_dma_map(struct virtio_net *dev, bool do_map)
>  				 * normal case in async path. This is a workaround.
>  				 */
>  				if (rte_errno == ENODEV)
> -					return;
> +					return 0;

The function now returns int instead of void. Does this code properly
handle partial mapping failures during the loop? If DMA mapping fails
after successfully mapping some pages, does cleanup occur?

> @@ -233,12 +254,10 @@ free_mem_region(struct virtio_net *dev)
>  	if (dev->async_copy && rte_vfio_is_enabled("vfio"))
>  		async_dma_map(dev, false);
>  
> -	for (i = 0; i < dev->mem->nregions; i++) {
> +	for (i = 0; i < VHOST_MEMORY_MAX_NREGIONS; i++) {
>  		reg = &dev->mem->regions[i];
> -		if (reg->host_user_addr) {
> -			munmap(reg->mmap_addr, reg->mmap_size);
> -			close(reg->fd);
> -		}
> +		if (reg->mmap_addr)
> +			free_mem_region(reg);
>  	}

Why iterate over all possible regions instead of just nregions? Does
this create unnecessary overhead?

> +static void
> +free_mem_region(struct rte_vhost_mem_region *reg)
> +{
> +	if (reg != NULL && reg->mmap_addr) {
> +		munmap(reg->mmap_addr, reg->mmap_size);
> +		close(reg->fd);
> +		memset(reg, 0, sizeof(struct rte_vhost_mem_region));
> +	}
> +}

Does this code check if munmap succeeds? Should errors be logged if
munmap or close fail?

> @@ -808,8 +837,10 @@ hua_to_alignment(struct rte_vhost_memory *mem, void *ptr)
>  	uint32_t i;
>  	uintptr_t hua = (uintptr_t)ptr;
>  
> -	for (i = 0; i < mem->nregions; i++) {
> +	for (i = 0; i < VHOST_MEMORY_MAX_NREGIONS; i++) {
>  		r = &mem->regions[i];
> +		if (r->host_user_addr == 0)
> +			continue;

Does this zero-check correctly distinguish uninitialized regions from
legitimately mapped regions at address zero? While unlikely, is zero
a valid mmap address?

> +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) {
> +		VHOST_CONFIG_LOG(dev->ifname, ERR,
> +			"memory already initialized, free it first");
> +		return -1;
> +	}

Does this represent a programming error that should never occur, or
a legitimate condition? Should this be an assert or handled differently?

> +	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);

This line exceeds 79 characters. Should it be wrapped?

> +	if (dev->mem == NULL) {
> +		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;
> +}

[ ... ]

> +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;

According to DPDK coding style, explicit null checks should be used.
Should this be if (vq == NULL)?

> +		if (vq->desc || vq->avail || vq->used) {

Similarly, should these be explicit null pointer checks?

> +/*
> + * Macro wrapper that performs the compile-time lock assertion with the
> + * correct message ID at the call site, then calls the implementation.
> + */
> +#define dev_invalidate_vrings(pdev, id) do { \
> +	static_assert(id ## _LOCK_ALL_QPS, \
> +		#id " handler is not declared as locking all queue pairs"); \
> +	_dev_invalidate_vrings(pdev); \
> +} while (0)

[ ... ]

> +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.memreg.region;
> +
> +	/* convert first region add to normal memory table set */
> +	if (dev->mem == NULL) {
> +		if (vhost_user_initialize_memory(pdev) < 0)
> +			goto close_msg_fds;
> +	}

Does this properly handle the case where SET_MEM_TABLE was never called?
Is this the intended behavior?

> +	/* 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);
> +		goto close_msg_fds;
> +	}

This log line exceeds 79 characters. Should it be wrapped?

> +	/* 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;
> +		uint64_t proposed_region_guest_start = region->userspace_addr;
> +		uint64_t proposed_region_guest_end = proposed_region_guest_start
> +							+ region->memory_size - 1;

Does this calculation handle potential overflow when adding size - 1?
Can size be zero, leading to underflow?

> +		if (!((proposed_region_guest_end < current_region_guest_start) ||
> +			(proposed_region_guest_start > current_region_guest_end))) {

According to DPDK coding style, avoid unnecessary parentheses. Can this
be simplified?

> +			VHOST_CONFIG_LOG(dev->ifname, ERR,
> +				"requested memory region overlaps with another region");
> +			VHOST_CONFIG_LOG(dev->ifname, ERR,
> +				"\tRequested region address:0x%" PRIx64,
> +				region->userspace_addr);

Are tabs in log messages intentional? This may display incorrectly in
log output.

> +	/* New region goes at the end of the contiguous array */
> +	struct rte_vhost_mem_region *reg = &dev->mem->regions[dev->mem->nregions];
> +
> +	reg->guest_phys_addr = region->guest_phys_addr;
>