Re: [PATCH v8 29/30] packet: Refactor vhost-user memory region handling

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 11219 bytes --]

On Tue, Aug 05, 2025 at 05:46:27PM +0200, Laurent Vivier wrote:
> This patch refactors the handling of vhost-user memory regions by
> introducing a new `struct vdev_memory` to encapsulate the regions
> array and their count (`nregions`) within the main `vu_dev` structure.
> 
> This new `vdev_memory` structure is then passed to the packet pool by
> re-using the existing `p->buf` field. A `p->buf_size` of 0 indicates
> that `p->buf` holds a pointer to `struct vdev_memory` instead of a
> regular packet buffer. A new helper, `get_vdev_memory()`, is added to
> abstract this access pattern.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  packet.c     | 18 ++++++++++++++++--
>  packet.h     |  6 ++++--
>  tap.c        |  4 ++--
>  tap.h        |  1 -
>  vhost_user.c | 28 +++++++++++-----------------
>  virtio.c     |  4 ++--
>  virtio.h     | 18 ++++++++++++++----
>  vu_common.c  | 22 ++++++++++++----------
>  8 files changed, 61 insertions(+), 40 deletions(-)
> 
> diff --git a/packet.c b/packet.c
> index cbc43c2fc22d..4b93688509a4 100644
> --- a/packet.c
> +++ b/packet.c
> @@ -22,6 +22,20 @@
>  #include "util.h"
>  #include "log.h"
>  
> +/**
> + * get_vdev_memory() - Return a pointer to the memory regions of the pool
> + * @p:		Packet pool
> + *
> + * Return: Null if none, otherwise a pointer to vdev_memory structure
> + */
> +static struct vdev_memory *get_vdev_memory(const struct pool *p)
> +{
> +	if (p->buf_size)
> +		return NULL;
> +
> +	return (struct vdev_memory *)p->buf;
> +}
> +
>  /**
>   * packet_check_range() - Check if a memory range is valid for a pool
>   * @p:		Packet pool
> @@ -41,10 +55,10 @@ static int packet_check_range(const struct pool *p, const char *ptr, size_t len,
>  		return -1;
>  	}
>  
> -	if (p->buf_size == 0) {
> +	if (get_vdev_memory(p)) {
>  		int ret;
>  
> -		ret = vu_packet_check_range((void *)p->buf, ptr, len);
> +		ret = vu_packet_check_range(get_vdev_memory(p), ptr, len);

Seems like it would be marginally more natural to assign
get_vdev_memory() to a temporary in the if, then re-use it here.

>  		if (ret == -1)
>  			debug("cannot find region, %s:%i", func, line);
> diff --git a/packet.h b/packet.h
> index 43b9022075d1..e51cbd19fdc4 100644
> --- a/packet.h
> +++ b/packet.h
> @@ -8,6 +8,7 @@
>  
>  #include <stdbool.h>
>  #include "iov.h"
> +#include "virtio.h"
>  
>  /* Maximum size of a single packet stored in pool, including headers */
>  #define PACKET_MAX_LEN	((size_t)UINT16_MAX)
> @@ -15,7 +16,7 @@
>  /**
>   * struct pool - Generic pool of packets stored in a buffer
>   * @buf:	Buffer storing packet descriptors,
> - * 		a struct vu_dev_region array for passt vhost-user mode
> + * 		a struct vdev_region for passt vhost-user mode
>   * @buf_size:	Total size of buffer,
>   * 		0 for passt vhost-user mode
>   * @size:	Number of usable descriptors for the pool
> @@ -30,7 +31,8 @@ struct pool {
>  	struct iovec pkt[];
>  };
>  
> -int vu_packet_check_range(void *buf, const char *ptr, size_t len);
> +int vu_packet_check_range(struct vdev_memory *memory,
> +			  const char *ptr, size_t len);
>  void packet_add_do(struct pool *p, struct iov_tail *data,
>  		   const char *func, int line);
>  bool packet_get_do(const struct pool *p, const size_t idx,
> diff --git a/tap.c b/tap.c
> index bbc786468455..9fd00915bb01 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -1458,7 +1458,7 @@ static void tap_sock_tun_init(struct ctx *c)
>   * @base:	Buffer base
>   * @size	Buffer size
>   */
> -void tap_sock_update_pool(void *base, size_t size)
> +static void tap_sock_update_pool(void *base, size_t size)
>  {
>  	int i;
>  
> @@ -1479,8 +1479,8 @@ void tap_sock_update_pool(void *base, size_t size)
>  void tap_backend_init(struct ctx *c)
>  {
>  	if (c->mode == MODE_VU) {
> -		tap_sock_update_pool(NULL, 0);
>  		vu_init(c);
> +		tap_sock_update_pool(&c->vdev->memory, 0);
>  	} else {
>  		tap_sock_update_pool(pkt_buf, sizeof(pkt_buf));
>  	}
> diff --git a/tap.h b/tap.h
> index ce5510882d5d..21db4d219ecb 100644
> --- a/tap.h
> +++ b/tap.h
> @@ -115,7 +115,6 @@ void tap_handler_passt(struct ctx *c, uint32_t events,
>  		       const struct timespec *now);
>  int tap_sock_unix_open(char *sock_path);
>  void tap_sock_reset(struct ctx *c);
> -void tap_sock_update_pool(void *base, size_t size);
>  void tap_backend_init(struct ctx *c);
>  void tap_flush_pools(void);
>  void tap_handler(struct ctx *c, const struct timespec *now);
> diff --git a/vhost_user.c b/vhost_user.c
> index c1522d549f00..f97ec6064cac 100644
> --- a/vhost_user.c
> +++ b/vhost_user.c
> @@ -137,8 +137,8 @@ static void *qva_to_va(struct vu_dev *dev, uint64_t qemu_addr)
>  	unsigned int i;
>  
>  	/* Find matching memory region.  */
> -	for (i = 0; i < dev->nregions; i++) {
> -		const struct vu_dev_region *r = &dev->regions[i];
> +	for (i = 0; i < dev->memory.nregions; i++) {
> +		const struct vu_dev_region *r = &dev->memory.regions[i];
>  
>  		if ((qemu_addr >= r->qva) && (qemu_addr < (r->qva + r->size))) {
>  			/* NOLINTNEXTLINE(performance-no-int-to-ptr) */
> @@ -428,8 +428,8 @@ static bool vu_set_mem_table_exec(struct vu_dev *vdev,
>  	struct vhost_user_memory m = vmsg->payload.memory, *memory = &m;
>  	unsigned int i;
>  
> -	for (i = 0; i < vdev->nregions; i++) {
> -		const struct vu_dev_region *r = &vdev->regions[i];
> +	for (i = 0; i < vdev->memory.nregions; i++) {
> +		const struct vu_dev_region *r = &vdev->memory.regions[i];
>  
>  		if (r->mmap_addr) {
>  			/* NOLINTNEXTLINE(performance-no-int-to-ptr) */
> @@ -437,12 +437,12 @@ static bool vu_set_mem_table_exec(struct vu_dev *vdev,
>  			       r->size + r->mmap_offset);
>  		}
>  	}
> -	vdev->nregions = memory->nregions;
> +	vdev->memory.nregions = memory->nregions;
>  
>  	debug("vhost-user nregions: %u", memory->nregions);
> -	for (i = 0; i < vdev->nregions; i++) {
> +	for (i = 0; i < vdev->memory.nregions; i++) {
>  		struct vhost_user_memory_region *msg_region = &memory->regions[i];
> -		struct vu_dev_region *dev_region = &vdev->regions[i];
> +		struct vu_dev_region *dev_region = &vdev->memory.regions[i];
>  		void *mmap_addr;
>  
>  		debug("vhost-user region %d", i);
> @@ -484,13 +484,7 @@ static bool vu_set_mem_table_exec(struct vu_dev *vdev,
>  		}
>  	}
>  
> -	/* As vu_packet_check_range() has no access to the number of
> -	 * memory regions, mark the end of the array with mmap_addr = 0
> -	 */
> -	ASSERT(vdev->nregions < VHOST_USER_MAX_RAM_SLOTS - 1);
> -	vdev->regions[vdev->nregions].mmap_addr = 0;
> -
> -	tap_sock_update_pool(vdev->regions, 0);
> +	ASSERT(vdev->memory.nregions < VHOST_USER_MAX_RAM_SLOTS);

It looks like the assertion is changing threshold by one, and I'm not
sure why.

>  
>  	return false;
>  }
> @@ -1106,8 +1100,8 @@ void vu_cleanup(struct vu_dev *vdev)
>  		vq->vring.avail = 0;
>  	}
>  
> -	for (i = 0; i < vdev->nregions; i++) {
> -		const struct vu_dev_region *r = &vdev->regions[i];
> +	for (i = 0; i < vdev->memory.nregions; i++) {
> +		const struct vu_dev_region *r = &vdev->memory.regions[i];
>  
>  		if (r->mmap_addr) {
>  			/* NOLINTNEXTLINE(performance-no-int-to-ptr) */
> @@ -1115,7 +1109,7 @@ void vu_cleanup(struct vu_dev *vdev)
>  			       r->size + r->mmap_offset);
>  		}
>  	}
> -	vdev->nregions = 0;
> +	vdev->memory.nregions = 0;
>  
>  	vu_close_log(vdev);
>  
> diff --git a/virtio.c b/virtio.c
> index ed7842b4c78a..bd388c2dfc7f 100644
> --- a/virtio.c
> +++ b/virtio.c
> @@ -102,8 +102,8 @@ static void *vu_gpa_to_va(const struct vu_dev *dev, uint64_t *plen,
>  		return NULL;
>  
>  	/* Find matching memory region. */
> -	for (i = 0; i < dev->nregions; i++) {
> -		const struct vu_dev_region *r = &dev->regions[i];
> +	for (i = 0; i < dev->memory.nregions; i++) {
> +		const struct vu_dev_region *r = &dev->memory.regions[i];
>  
>  		if ((guest_addr >= r->gpa) &&
>  		    (guest_addr < (r->gpa + r->size))) {
> diff --git a/virtio.h b/virtio.h
> index 32757458ea95..b55cc4042521 100644
> --- a/virtio.h
> +++ b/virtio.h
> @@ -96,11 +96,22 @@ struct vu_dev_region {
>   */
>  #define VHOST_USER_MAX_RAM_SLOTS 32
>  
> +/**
> + * struct vdev_memory - Describes the shared memory regions for a vhost-user
> + * 			device
> + * @nregions:		Number of shared memory regions
> + * @regions:		Guest shared memory regions
> + */
> +struct vdev_memory {
> +	uint32_t nregions;
> +	struct vu_dev_region regions[VHOST_USER_MAX_RAM_SLOTS];
> +};
> +
>  /**
>   * struct vu_dev - vhost-user device information
>   * @context:			Execution context
> - * @nregions:			Number of shared memory regions
> - * @regions:			Guest shared memory regions
> + * @memory:			Shared memory regions
> + * @vq:				Virtqueues of the device
>   * @features:			Vhost-user features
>   * @protocol_features:		Vhost-user protocol features
>   * @log_call_fd:		Eventfd to report logging update
> @@ -109,8 +120,7 @@ struct vu_dev_region {
>   */
>  struct vu_dev {
>  	struct ctx *context;
> -	uint32_t nregions;
> -	struct vu_dev_region regions[VHOST_USER_MAX_RAM_SLOTS];
> +	struct vdev_memory memory;
>  	struct vu_virtq vq[VHOST_USER_MAX_QUEUES];
>  	uint64_t features;
>  	uint64_t protocol_features;
> diff --git a/vu_common.c b/vu_common.c
> index b77b21420c57..b716070ea3c3 100644
> --- a/vu_common.c
> +++ b/vu_common.c
> @@ -25,26 +25,28 @@
>  /**
>   * vu_packet_check_range() - Check if a given memory zone is contained in
>   * 			     a mapped guest memory region
> - * @buf:	Array of the available memory regions
> + * @memory:	Array of the available memory regions
>   * @ptr:	Start of desired data range
> - * @size:	Length of desired data range
> + * @len:	Length of desired data range
>   *
>   * Return: 0 if the zone is in a mapped memory region, -1 otherwise
>   */
> -int vu_packet_check_range(void *buf, const char *ptr, size_t len)
> +int vu_packet_check_range(struct vdev_memory *memory,
> +			  const char *ptr, size_t len)
>  {
> -	struct vu_dev_region *dev_region;
> +	struct vu_dev_region *dev_region = memory->regions;
> +	unsigned int i;
>  
> -	for (dev_region = buf; dev_region->mmap_addr; dev_region++) {
> -		uintptr_t base_addr = dev_region->mmap_addr +
> -			dev_region->mmap_offset;
> +	for (i = 0; i < memory->nregions; i++) {
> +		uintptr_t base_addr = dev_region[i].mmap_addr +
> +			dev_region[i].mmap_offset;
>  		/* NOLINTNEXTLINE(performance-no-int-to-ptr) */
>  		const char *base = (const char *)base_addr;
>  
> -		ASSERT(base_addr >= dev_region->mmap_addr);
> +		ASSERT(base_addr >= dev_region[i].mmap_addr);
>  
> -		if (len <= dev_region->size && base <= ptr &&
> -		    (size_t)(ptr - base) <= dev_region->size - len)
> +		if (len <= dev_region[i].size && base <= ptr &&
> +		    (size_t)(ptr - base) <= dev_region[i].size - len)
>  			return 0;
>  	}
>  

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]