Re: [PATCH v5 28/29] packet: add memory regions information into pool

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 17 Apr 2025 18:51:35 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> Do not hack anymore the buf pointer to detect and manage memory regions
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  packet.c     |  4 ++--
>  packet.h     | 20 +++++++++++++-------
>  tap.c        | 23 +++++++++++++++--------
>  tap.h        |  1 -
>  vhost_user.c | 28 +++++++++++-----------------
>  virtio.c     |  4 ++--
>  virtio.h     |  8 ++++++--
>  vu_common.c  | 20 +++++++++++---------
>  8 files changed, 60 insertions(+), 48 deletions(-)
> 
> diff --git a/packet.c b/packet.c
> index 53ede9fcb509..0a2ad28d3d25 100644
> --- a/packet.c
> +++ b/packet.c
> @@ -41,10 +41,10 @@ static int packet_check_range(const struct pool *p, const char *ptr, size_t len,
>  		return -1;
>  	}
>  
> -	if (p->buf_size == 0) {
> +	if (p->memory) {
>  		int ret;
>  
> -		ret = vu_packet_check_range((void *)p->buf, ptr, len);
> +		ret = vu_packet_check_range(p->memory, ptr, len);
>  
>  		if (ret == -1)
>  			debug("cannot find region, %s:%i", func, line);
> diff --git a/packet.h b/packet.h
> index bbea04eac455..e951187a7ef0 100644
> --- a/packet.h
> +++ b/packet.h
> @@ -9,20 +9,22 @@
>  #include <stdbool.h>
>  #include "iov.h"
>  
> +struct vdev_memory;

If I drop this forward declaration, gcc is still happy. I guess you
needed it in a previous version, but not anymore.

> +
>  /* Maximum size of a single packet stored in pool, including headers */
>  #define PACKET_MAX_LEN	((size_t)UINT16_MAX)
>  
>  /**
>   * 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
> + * @memory:	Memory regions defined for vhost-user, NULL otherwise
> + * @buf:	Buffer storing packet descriptors or iovec entries,
>   * @buf_size:	Total size of buffer,
> - * 		0 for passt vhost-user mode
>   * @size:	Number of usable descriptors for the pool
>   * @count:	Number of used descriptors for the pool
>   * @pkt:	Descriptors: see macros below
>   */
>  struct pool {
> +	struct vdev_memory *memory;
>  	char *buf;
>  	size_t buf_size;
>  	size_t size;
> @@ -30,7 +32,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_data_do(const struct pool *p, const size_t idx,
> @@ -46,6 +49,7 @@ void pool_flush(struct pool *p);
>  
>  #define PACKET_POOL_DECL(_name, _size)					\
>  struct _name ## _t {							\
> +	struct vdev_memory *memory;					\
>  	char *buf;							\
>  	size_t buf_size;						\
>  	size_t size;							\
> @@ -53,15 +57,17 @@ struct _name ## _t {							\
>  	struct iovec pkt[_size];					\
>  }
>  
> -#define PACKET_POOL_INIT_NOCAST(_size, _buf, _buf_size)			\
> +#define PACKET_POOL_INIT_NOCAST(_size, _buf, _buf_size, _memory)	\
>  {									\
>  	.buf_size = _buf_size,						\
>  	.buf = _buf,							\
>  	.size = _size,							\
> +	.memory = _memory						\
>  }
>  
> -#define PACKET_INIT(name, size, buf, buf_size)				\
> -	(struct name ## _t) PACKET_POOL_INIT_NOCAST(size, buf, buf_size)
> +#define PACKET_INIT(name, size, buf, buf_size, memory)			\
> +	(struct name ## _t)						\
> +		PACKET_POOL_INIT_NOCAST(size, buf, buf_size, memory)
>  
>  #define PACKET_POOL_NOINIT(name, size)					\
>  	PACKET_POOL_DECL(name, size) name ## _storage;			\
> diff --git a/tap.c b/tap.c
> index 615609d4ded2..419b09b4cee7 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -1453,17 +1453,23 @@ static void tap_sock_tun_init(struct ctx *c)
>   * tap_sock_update_pool() - Set the buffer base and size for the pool of packets
>   * @base:	Buffer base
>   * @size	Buffer size
> + * @memory:	vhost-user memory regions
>   */
> -void tap_sock_update_pool(void *base, size_t size)
> +static void tap_sock_update_pool(void *base, size_t size,
> +				 struct vdev_memory *memory)
>  {
>  	int i;
>  
> -	pool_tap4_storage = PACKET_INIT(pool_tap4, TAP_MSGS_IP4, base, size);
> -	pool_tap6_storage = PACKET_INIT(pool_tap6, TAP_MSGS_IP6, base, size);
> +	pool_tap4_storage = PACKET_INIT(pool_tap4, TAP_MSGS_IP4, base, size,
> +					memory);
> +	pool_tap6_storage = PACKET_INIT(pool_tap6, TAP_MSGS_IP6, base, size,
> +					memory);
>  
>  	for (i = 0; i < TAP_SEQS; i++) {
> -		tap4_l4[i].p = PACKET_INIT(pool_l4, UIO_MAXIOV, base, size);
> -		tap6_l4[i].p = PACKET_INIT(pool_l4, UIO_MAXIOV, base, size);
> +		tap4_l4[i].p = PACKET_INIT(pool_l4, UIO_MAXIOV, base, size,
> +					   memory);
> +		tap6_l4[i].p = PACKET_INIT(pool_l4, UIO_MAXIOV, base, size,
> +					   memory);
>  	}
>  }
>  
> @@ -1474,12 +1480,13 @@ void tap_sock_update_pool(void *base, size_t size)
>   */
>  void tap_backend_init(struct ctx *c)
>  {
> +	struct vdev_memory *memory = NULL;
> +
>  	if (c->mode == MODE_VU) {
> -		tap_sock_update_pool(NULL, 0);
>  		vu_init(c);
> -	} else {
> -		tap_sock_update_pool(pkt_buf, sizeof(pkt_buf));
> +		memory = &c->vdev->memory;
>  	}
> +	tap_sock_update_pool(pkt_buf, sizeof(pkt_buf), memory);
>  
>  	if (c->fd_tap != -1) { /* Passed as --fd */
>  		ASSERT(c->one_off);
> diff --git a/tap.h b/tap.h
> index 954ad440a287..655b6ed9d2fd 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 105f77a5df2b..e53ea21909e3 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 = msg->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);
>  
>  	return false;
>  }
> @@ -1107,8 +1101,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) */
> @@ -1116,7 +1110,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 bc2b89a7f4a7..2b0a8d0dc4c5 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 7a370bdeae65..5acd4f19f0ed 100644
> --- a/virtio.h
> +++ b/virtio.h
> @@ -96,6 +96,11 @@ struct vu_dev_region {
>   */
>  #define VHOST_USER_MAX_RAM_SLOTS 32
>  
> +struct vdev_memory {

It would be nice to have the usual struct documentation.

> +	uint32_t nregions;
> +	struct vu_dev_region regions[VHOST_USER_MAX_RAM_SLOTS];
> +};
> +
>  /**
>   * struct vu_dev - vhost-user device information
>   * @context:			Execution context
> @@ -109,8 +114,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;

...and to update the comment to struct vu_dev_region accordingly.

>  	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..19fe22935a1e 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:	Allowed memory regions

...is there any particular reason why they were "available", earlier,
and now they are "allowed" instead? I'm not sure if it's an oversight
or it's actually intended.

>   * @ptr:	Start of desired data range
>   * @size:	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;
>  	}
>  

-- 
Stefano