From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202508 header.b=sdpaFDqa; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id C24E25A0271 for ; Thu, 07 Aug 2025 08:18:02 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202508; t=1754547479; bh=MHNTLywdJyiWBgodCRDV/8EJGqSVQVjUyOMertjR+Pw=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=sdpaFDqaHd+wqhSZe+jC+fojQEnOl9QS7bHBB2WOdbyxagVR2A+ip29MU+w063pBJ Gf3LYjb+tTRkkWGtwftABolBwRG4CUH6PMfVgmr326vuySTLpfUsYEmM41iX6hpaQW ZtZbSbBWm+eYLwNfgjyKG8fqJ+XOzY3UJOqVYcQEVpL6SEejbu7qEs4GAcH0yMXw7o Uf8ih8ZW4S8uRWeGBy0V7Npj43WjpZr79GCxjs+3y58rI2hrCLA7xdq4GASyAFC+jX Bg7ACwZ94zB6O8b9VbFz+wTUDOj+OIWloZYTX7CTjCwnfzi98Fo7jSNmpFGsOBPzFv CGIzcA2Vtm+2g== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4byH5g1c9hz4xQ0; Thu, 7 Aug 2025 16:17:59 +1000 (AEST) Date: Thu, 7 Aug 2025 16:10:00 +1000 From: David Gibson To: Laurent Vivier Subject: Re: [PATCH v8 29/30] packet: Refactor vhost-user memory region handling Message-ID: References: <20250805154628.301343-1-lvivier@redhat.com> <20250805154628.301343-30-lvivier@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="E094sioMM2ovC7qi" Content-Disposition: inline In-Reply-To: <20250805154628.301343-30-lvivier@redhat.com> Message-ID-Hash: 5NBMOSNBQA76PYQN56LYWWVH2A44NSZN X-Message-ID-Hash: 5NBMOSNBQA76PYQN56LYWWVH2A44NSZN X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --E094sioMM2ovC7qi Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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. >=20 > 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. >=20 > Signed-off-by: Laurent Vivier > --- > 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(-) >=20 > 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" > =20 > +/** > + * 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, c= onst char *ptr, size_t len, > return -1; > } > =20 > - if (p->buf_size =3D=3D 0) { > + if (get_vdev_memory(p)) { > int ret; > =20 > - ret =3D vu_packet_check_range((void *)p->buf, ptr, len); > + ret =3D 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 =3D=3D -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 @@ > =20 > #include > #include "iov.h" > +#include "virtio.h" > =20 > /* 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[]; > }; > =20 > -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; > =20 > @@ -1479,8 +1479,8 @@ void tap_sock_update_pool(void *base, size_t size) > void tap_backend_init(struct ctx *c) > { > if (c->mode =3D=3D 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 q= emu_addr) > unsigned int i; > =20 > /* Find matching memory region. */ > - for (i =3D 0; i < dev->nregions; i++) { > - const struct vu_dev_region *r =3D &dev->regions[i]; > + for (i =3D 0; i < dev->memory.nregions; i++) { > + const struct vu_dev_region *r =3D &dev->memory.regions[i]; > =20 > if ((qemu_addr >=3D 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 =3D vmsg->payload.memory, *memory =3D &m; > unsigned int i; > =20 > - for (i =3D 0; i < vdev->nregions; i++) { > - const struct vu_dev_region *r =3D &vdev->regions[i]; > + for (i =3D 0; i < vdev->memory.nregions; i++) { > + const struct vu_dev_region *r =3D &vdev->memory.regions[i]; > =20 > if (r->mmap_addr) { > /* NOLINTNEXTLINE(performance-no-int-to-ptr) */ > @@ -437,12 +437,12 @@ static bool vu_set_mem_table_exec(struct vu_dev *vd= ev, > r->size + r->mmap_offset); > } > } > - vdev->nregions =3D memory->nregions; > + vdev->memory.nregions =3D memory->nregions; > =20 > debug("vhost-user nregions: %u", memory->nregions); > - for (i =3D 0; i < vdev->nregions; i++) { > + for (i =3D 0; i < vdev->memory.nregions; i++) { > struct vhost_user_memory_region *msg_region =3D &memory->regions[i]; > - struct vu_dev_region *dev_region =3D &vdev->regions[i]; > + struct vu_dev_region *dev_region =3D &vdev->memory.regions[i]; > void *mmap_addr; > =20 > debug("vhost-user region %d", i); > @@ -484,13 +484,7 @@ static bool vu_set_mem_table_exec(struct vu_dev *vde= v, > } > } > =20 > - /* As vu_packet_check_range() has no access to the number of > - * memory regions, mark the end of the array with mmap_addr =3D 0 > - */ > - ASSERT(vdev->nregions < VHOST_USER_MAX_RAM_SLOTS - 1); > - vdev->regions[vdev->nregions].mmap_addr =3D 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. > =20 > return false; > } > @@ -1106,8 +1100,8 @@ void vu_cleanup(struct vu_dev *vdev) > vq->vring.avail =3D 0; > } > =20 > - for (i =3D 0; i < vdev->nregions; i++) { > - const struct vu_dev_region *r =3D &vdev->regions[i]; > + for (i =3D 0; i < vdev->memory.nregions; i++) { > + const struct vu_dev_region *r =3D &vdev->memory.regions[i]; > =20 > 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 =3D 0; > + vdev->memory.nregions =3D 0; > =20 > vu_close_log(vdev); > =20 > 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, u= int64_t *plen, > return NULL; > =20 > /* Find matching memory region. */ > - for (i =3D 0; i < dev->nregions; i++) { > - const struct vu_dev_region *r =3D &dev->regions[i]; > + for (i =3D 0; i < dev->memory.nregions; i++) { > + const struct vu_dev_region *r =3D &dev->memory.regions[i]; > =20 > if ((guest_addr >=3D 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 > =20 > +/** > + * 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 =3D memory->regions; > + unsigned int i; > =20 > - for (dev_region =3D buf; dev_region->mmap_addr; dev_region++) { > - uintptr_t base_addr =3D dev_region->mmap_addr + > - dev_region->mmap_offset; > + for (i =3D 0; i < memory->nregions; i++) { > + uintptr_t base_addr =3D dev_region[i].mmap_addr + > + dev_region[i].mmap_offset; > /* NOLINTNEXTLINE(performance-no-int-to-ptr) */ > const char *base =3D (const char *)base_addr; > =20 > - ASSERT(base_addr >=3D dev_region->mmap_addr); > + ASSERT(base_addr >=3D dev_region[i].mmap_addr); > =20 > - if (len <=3D dev_region->size && base <=3D ptr && > - (size_t)(ptr - base) <=3D dev_region->size - len) > + if (len <=3D dev_region[i].size && base <=3D ptr && > + (size_t)(ptr - base) <=3D dev_region[i].size - len) > return 0; > } > =20 --=20 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 --E094sioMM2ovC7qi Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmiUQzEACgkQzQJF27ox 2GcFEw//WXIJkC6Rr18cDONigQNZnftjuZ8eZfp3JsDnd7CwgVE3AYqSXmtl6Yyy LLThAg3sx6oHSo5+HQOPnee4L98+2r3BmkeWcYmLaqeXJqtZdnA9dN0OTSZwzPDo RPtDYABxSmLpRaWoYmGz0wbkrx4+UdEDYEZKT5ixaQ5c165XkynzV88RzFdnm0jw dN6QeC7vVxRfKWNDgZaVht1lWRJGJLFA128Zk86Vb3OsU3La0fwCEx8FdCVe+s2Y 3vKOVBmAIcpmgtM6bsAMvfWAXp0Hhemrdg2CegofKnIxqYKuOZxazsmwdx+NSJbh BEHb1cImq7vA44WI2Ap+DU/raHyRxhWiCexMbhEMvRQfDmzcyrsLFH6TbWhEFtkD pSopXMR5rEyeSf56CL0s0+Sttml0nr3yIE31bhzYg49JEX0Xo5zy/94bg1vH9mgt sbUssMg6NK31o8uop04rRJFXF/KvOlK9Pz4nUKFDcBR0T06Xpd8CMPXNGhDMOJ7w QIUEGb1bl0PjQg3MK0pyNph6pCN8rahj4oUXrd4CCOojinRTVrEdOFPOFT+ENplJ njGyPUHlxb141ZE7YjtCaAEAqclfUru4V/ISBGdFuHnDOVbyKnjFsxTKjVG+Udu8 OPqOaj5+3rPhvr/VJLQjxqNQZIdfhnd1Zb25vc2Sr7DmkOANwE8= =ZZH/ -----END PGP SIGNATURE----- --E094sioMM2ovC7qi--