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=202608 header.b=Ww30S1ON; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 986385A0262 for ; Mon, 10 Aug 2026 04:06:33 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202608; t=1786327589; bh=N/XMmN314NXmwN/Ee30JJfnRCO6m71qsutD6raF0FAA=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=Ww30S1ONcMT3hlcJSM1b2hfBvtrP3OpCuRbQKxMf634ULclN5PeXDYcBs7icwPdQs j+N2VCOAYGEvwc/oNo4SzYT//tAAJ3Ab2D/OEiRy26Aw5MutJ6gpJ+pacPdeN0MxRl KKu9jvyXiANewiVv+cN7M9Vj5POMqxbst7vazLBOACP/7GUwXkLaV+ML4IX7+jD0sC bYiHqJB3fwzlisA7xCbLERg8lFt/AOGoQpae+Zv8r++ULLkpHmbQvkfF5JKbUgkB7F vdeVrR3CNQKiimfSVp7rTOYmJHrl7JETAvF4lRvDGrDeGplhVWCTsF0RNkA6TCZTFK tWf/j1b8hf5XQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4hJJ5d71mHz4wBB; Mon, 10 Aug 2026 12:06:29 +1000 (AEST) Date: Mon, 10 Aug 2026 12:06:24 +1000 From: David Gibson To: Ammar Yasser Subject: Re: [RFC v3 4/8] virtio: Define the pasta vhost interface Message-ID: References: <20260802132155.870796-1-aerosound161@gmail.com> <20260802132155.870796-5-aerosound161@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="4jqYUy33uIspNlOb" Content-Disposition: inline In-Reply-To: <20260802132155.870796-5-aerosound161@gmail.com> Message-ID-Hash: K422GMUUCJFC6MTBLDPIPHCT7G34GAKD X-Message-ID-Hash: K422GMUUCJFC6MTBLDPIPHCT7G34GAKD 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, eperezma@redhat.com 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: --4jqYUy33uIspNlOb Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sun, Aug 02, 2026 at 01:21:51PM +0000, Ammar Yasser wrote: > - Include the ioctls needed in opening, initializing and registering > queues and fds with vhost-net > - Create the vq_state type which represents from pasta's perspective our > state of virtqueue processing > - Define the structures that will represent the actual descriptor > queues: vring_desc (the actual descriptor chain), vring_used_all (used > ring for each queue), vring_avail_all (available ring for each queue), > vhost_memory (the structure that carries the shape of the memory > region as it should be shared with the kernel) > - Increase PKT_BUF_BYTES by 1536 because the byte array that carries > the frames needs to also account for virtio_net_hdr_mrg_rxbuf header > sizes per each frame. > - Define the signatures of functions that will be used in setting up the > virtqueues with the kernel (set_vring_for_queue, setup_memory_table, > setup_vhost_net, setup_eventfds). > - Define the signature of rx_descriptor_handoff, which will be used to > mark descriptors as available again to the driver, and vhost_kick so > we can notify the driver of our updates >=20 > Signed-off-by: Ammar Yasser > --- > passt.h | 2 +- > virtio.h | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 77 insertions(+), 1 deletion(-) >=20 > diff --git a/passt.h b/passt.h > index c729316..88a1b03 100644 > --- a/passt.h > +++ b/passt.h > @@ -36,7 +36,7 @@ union epoll_ref; > ((uint8_t [ETH_ALEN]){0x9a, 0x55, 0x9a, 0x55, 0x9a, 0x55}) > =20 > /* Large enough for ~128 maximum size frames */ > -#define PKT_BUF_BYTES (8UL << 20) > +#define PKT_BUF_BYTES ((8UL << 20) + 1536) /* 128 * sizeof(virtio_net_h= dr_mrg_rxbuf) */ I think the rationale for this change needs to be clearer (granted, the comment here beforehand is also kind of confusing). IIRC - and based on the "~" in the comment, I don't think there's a strict requirement that this can hold 128 full frames - that's just setting a reasonable sense of scale, and then a round number was picked near it: ~64kiB * ~64 ~=3D 8MiB So, I'm not sure if this change is necessary - if it really is, we need a clearer analysis of why. > extern char pkt_buf [PKT_BUF_BYTES]; > =20 > diff --git a/virtio.h b/virtio.h > index 8f2ae06..2d1eef1 100644 > --- a/virtio.h > +++ b/virtio.h > @@ -9,14 +9,82 @@ > #ifndef VIRTIO_H > #define VIRTIO_H > =20 > +#include > #include > +#include > #include > =20 > +struct ctx; > + AIUI, virtio.h is supposed to expose the virtio interfaces to passt, not contain any passt specific logic, so including things that use struct ctx (even just by reference) is probably not a good idea. I believe this header was originally formed as a cut down version of one =66rom qemu or a virtio library, so I'd probably also restrict it to things that (conceptually) were in there. Laurent will know more about the history, since he introduced these for vhost-user. We also want to be clear what definitions are general to virtio versus which are specific to vhost-kernel versus vhost-user. > /* Maximum size of a virtqueue */ > #define VIRTQUEUE_MAX_SIZE 1024 > =20 > #define VNET_HLEN (sizeof(struct virtio_net_hdr_mrg_rxbuf)) > =20 > +/* Keep in sync with PKT_BUF_BYTES in passt.h */ > +#define PKT_BUF_BYTES ((8UL << 20) + 1536) /* 128 * sizeof(virtio_net_h= dr_mrg_rxbuf) */ Ouch. We really want to avoid that sort of duplication, even if it means splitting out a new small header included from both places. > +#define VHOST_NDESCS (PKT_BUF_BYTES / 65520) I'm not sure 65520 is the right number here. That's the max MTU at the IP level, but the packet buffer will also hold the 14 byte L2 header. I think you probably want one of the L2_MAX_LEN_* constants (or to define a new one for vhost-kernel). > +static_assert(!(VHOST_NDESCS & (VHOST_NDESCS - 1)), > + "Number of vhost descs must be a power of two by standard"); > + > + > +#define VIRTQUEUE_MAX_SIZE 1024 Duplicate #define. > + > +#define VHOST_VIRTIO 0xAF > +#define VHOST_GET_FEATURES _IOR(VHOST_VIRTIO, 0x00, __u64) > +#define VHOST_SET_FEATURES _IOW(VHOST_VIRTIO, 0x00, __u64) > +#define VHOST_SET_OWNER _IO(VHOST_VIRTIO, 0x01) > +#define VHOST_SET_MEM_TABLE _IOW(VHOST_VIRTIO, 0x03, struct vhost_memor= y) > +#define VHOST_SET_VRING_NUM _IOW(VHOST_VIRTIO, 0x10, struct vhost_vring= _state) > +#define VHOST_SET_VRING_ADDR _IOW(VHOST_VIRTIO, 0x11, struct vhost_vring= _addr) > +#define VHOST_SET_VRING_KICK _IOW(VHOST_VIRTIO, 0x20, struct vhost_vring= _file) > +#define VHOST_SET_VRING_CALL _IOW(VHOST_VIRTIO, 0x21, struct vhost_vring= _file) > +#define VHOST_SET_VRING_ERR _IOW(VHOST_VIRTIO, 0x22, struct vhost_vring= _file) > +#define VHOST_SET_BACKEND_FEATURES _IOW(VHOST_VIRTIO, 0x25, __u64) > +#define VHOST_NET_SET_BACKEND _IOW(VHOST_VIRTIO, 0x30, struct vhost_vrin= g_file) It's probably preferable to #include rather than restating these. > +/** > + * struct vq_state - Per-virtqueue local descriptor tracking > + * @num_free: Number of descriptors ready to be announced > + * to the kernel as available via rx_descriptor_handoff() > + * @last_used_idx: Number of used-ring entries consumed so far; > + * lagging read cursor vs. vring_used->idx (the > + * kernel's write cursor) > + */ > +extern struct vq_state { > + uint16_t num_free; > + uint16_t last_used_idx; > + uint16_t next_free; > +} vqs[2]; This is a vhost-kernel relevant view of a vq - we also have vhost-user relevant views, which is a bit confusing. Renaming and/or moving to a vhost-kernel specific header is probably a good idea. > +extern struct vring_desc vring_desc[2][VHOST_NDESCS]; Do we need these externs, or could we make these structures local to the .c file actually doing the vhost-kernel handling? > +union vring_avail_u { > + struct vring_avail avail; > + char buf[offsetof(struct vring_avail, ring[VHOST_NDESCS])]; > +}; > +#pragma GCC diagnostic push > +#pragma GCC diagnostic ignored "-Wpedantic" > +extern union vring_avail_u vring_avail_all[2]; > +#pragma GCC diagnostic pop > + > +union vring_used_u { > + struct vring_used used; > + char buf[offsetof(struct vring_used, ring[VHOST_NDESCS])]; > +}; > +#pragma GCC diagnostic push > +#pragma GCC diagnostic ignored "-Wpedantic" > +extern union vring_used_u vring_used_all[2]; > +#pragma GCC diagnostic pop > + > +#define N_VHOST_REGIONS 12 > +union vhost_memory_u { > + struct vhost_memory mem; > + char buf[offsetof(struct vhost_memory, regions[N_VHOST_REGIONS])]; > +}; > +extern union vhost_memory_u vhost_memory; > + > /** > * struct vu_ring - Virtqueue rings > * @num: Size of the queue > @@ -147,6 +215,7 @@ struct vu_virtq_element { > struct iovec *out_sg; > }; > =20 > +void vu_queue_notify(const struct vu_dev *dev, struct vu_virtq *vq); > /** > * has_feature() - Check a feature bit in a features set > * @features: Features set > @@ -199,4 +268,11 @@ void vu_queue_fill(const struct vu_dev *vdev, struct= vu_virtq *vq, > unsigned int idx); > void vu_queue_flush(const struct vu_dev *vdev, struct vu_virtq *vq, > unsigned int count); > +void set_vring_for_queue(struct ctx *c, int queue_idx, int tap_fd); > +int setup_memory_table(struct ctx *c); > +void setup_vhost_net(struct ctx *c); > +void setup_eventfds(struct ctx *c, int queue_idx); > +void rx_descriptor_handoff(struct ctx *c); > +void vhost_kick(struct vring_used *used, int kick_fd); > + We generally don't like to introduce function signatures separate from function implementations. It's sometimes a fuzzy line, but the idea is to split patches on logical concepts / subfeatures, not on different parts of the code for the same thing. That generally makes review easier. > #endif /* VIRTIO_H */ > --=20 > 2.34.1 >=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 --4jqYUy33uIspNlOb Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmp5MhMACgkQzQJF27ox 2GfbPA//e68DOM3/DG1Bu8vwHB9npXLqMV2Ncysth/j1sV7n/94MRihRg1hzFHDk isG3UCLo9f2pgVA/oDF9e6I6dzj+HUP9QxW9okQPv9yl9nqdXivFVf+Q0Sw5utUB P3r91hYbUEESReRmg/VDMm39vt93r1XEKFGPhUiVRLHDVMrSSoZOTLdB8qcV7x+l hLdva4tLPKHYsGmnPU+HzHsQhJFuwamVITqixKSzWFmU2TLApNEPjrS7O8HsnSdX uyabV4MQmnMlF12bNd2Lm4EUwDA2Qv7PBl2t8ZpIx7QReaAh6j1k6lKWk/hyWz7/ eX6/a037rWLckMEn1awb3KUJLg2JmsylxMoJ8faLA4Uv82hJ/UKkGrnRy1o9g4tH 6EcBm7Oma6+uFIYrZPpuwJVchmgeWkhBTsWDitcgKi1tvL3vv16dY8ftqVoYRIUV 269UsbHMwbGgUpPrkCJ8MUYe0p1k5bm2t+KUt2Q/jtZ5YiQIpuA2AYoCgxzJsZ2b wjgqnqwSiRZmQPA5vv7cS9mR1oQ+uFDn1pEqoiaHn2Z5lAkSERNjsccnHtkxLK0Y pnf45XkpH7wrR/MH6/gjO0O3BUOV7upu23sGeuYHvv0978crPFhsXwvcl+GkDAjq EWdq5fCNiL+qf2qOU2yW5P8zvmv80K2iOX6F+bCySc2k3oS3IHQ= =OKfI -----END PGP SIGNATURE----- --4jqYUy33uIspNlOb--