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=RW1SBvkL; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 816315A026E for ; Thu, 13 Aug 2026 04:09:50 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202608; t=1786586982; bh=/AmQCz1/E73JiT5ytBaUvZsoz76jhHTGkCyPGPWlsM0=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=RW1SBvkLtBI51RLziMO+n1nikOjc3GV3400LOBVpVC54B7d4RM7CSy6Bo3axaBEu6 VfWbIoTU6zVCo3BwEyu+LmEv/rgwaSUbiocG3s1WorwWI6H7xo5reKVPHndL7Xi0hy lVsulN4oP6k3/s8pGKd5IGGe8n2HJ45B9ejOpQtjWsC8QDffdO5AnTv1N60bc2zLUA RjkTocKASC69DDNIbiOa2+Q/xN6YFvGsdpBohq+6p8gPkwBbkGol/QEtmI8sin4UAM wZRqWLvYgQAbSQy9k4qaJucefRCbnOBnfPkwq5hiDHO7IVdqFXIF4bnv1eUifD/+2q SgzVbMkLWRe/g== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4hL81y1D0vz4w2F; Thu, 13 Aug 2026 12:09:42 +1000 (AEST) Date: Thu, 13 Aug 2026 11:16:37 +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="LYLEG+QciBcPOweO" Content-Disposition: inline In-Reply-To: Message-ID-Hash: NV5KVLI24NUO6Q455UFJSI76K3FE3U6I X-Message-ID-Hash: NV5KVLI24NUO6Q455UFJSI76K3FE3U6I 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: --LYLEG+QciBcPOweO Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Aug 12, 2026 at 08:17:19PM +0300, Ammar Yasser wrote: > On Mon Aug 10, 2026 at 5:06 AM EEST, David Gibson wrote: > >> =20 > >> /* Large enough for ~128 maximum size frames */ > >> -#define PKT_BUF_BYTES (8UL << 20) > >> +#define PKT_BUF_BYTES ((8UL << 20) + 1536) /* 128 * sizeof(virtio_ne= t_hdr_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. >=20 > Because pkt_buf is now going to be the buffer where vhost guest->pasta > data but with the added size of the virtio_net_hdr_mrg_rxbuf for every > frame. So this is accounting for the worst case where the guest wants to > send a full 128 frames at maximum size at a time. Right, but that's not enough. AFAICT pkt_buf is sized to allow *roughly* 128 full packets, but it doesn't strictly have to be able to contain that many. If there's a reason it *must* have room for 128 full frames with vhost-kernel, that needs to be pointed out explicitly. > Will include this > rationale in the comment in the coming revision >=20 > > > >> 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 > > from 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. >=20 > We can either have a splitting comment that says something along the > lines of "from here onwards, all whats below if vhost-kernel code", or > even further, a brand new header. which will also solve the problem you > highlight above. Unless you have a preference to have them combined in > one header, i will go with the 2 header approach in the next revision Yes, I think new headers is the better idea. > >> /* 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_ne= t_hdr_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. >=20 > Ok >=20 > > > >> +#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). >=20 > My line of reasoning here is that "we typically expect, in the majority > of cases for an ethernet frame to span a single descriptor". evident by > how we eventually consume descriptors as we return the pointer into the > pkt_buf past the virtio_net header as the beginning of an ethernet > header. and this buffer should handle 128 frames (from the definition=20 > of PKT_BUF_BYTES) and so we want a denominator that yields a value as > close as possible to 128. Let me know if this isn't very sound. I will > investiagate the constants you mentioned anyways If the fundamental property you want is that you can fit 128 descriptors, then you should just define NDESCS as 128, and derive the buffer size and other things from that. If the basic property you want is something else, define that first and the rest in terms of it. > >> +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. >=20 > Noted >=20 > > > >> + > >> +#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_me= mory) > >> +#define VHOST_SET_VRING_NUM _IOW(VHOST_VIRTIO, 0x10, struct vhost_vr= ing_state) > >> +#define VHOST_SET_VRING_ADDR _IOW(VHOST_VIRTIO, 0x11, struct vhost_vr= ing_addr) > >> +#define VHOST_SET_VRING_KICK _IOW(VHOST_VIRTIO, 0x20, struct vhost_vr= ing_file) > >> +#define VHOST_SET_VRING_CALL _IOW(VHOST_VIRTIO, 0x21, struct vhost_vr= ing_file) > >> +#define VHOST_SET_VRING_ERR _IOW(VHOST_VIRTIO, 0x22, struct vhost_vr= ing_file) > >> +#define VHOST_SET_BACKEND_FEATURES _IOW(VHOST_VIRTIO, 0x25, __u64) > >> +#define VHOST_NET_SET_BACKEND _IOW(VHOST_VIRTIO, 0x30, struct vhost_v= ring_file) > > > > It's probably preferable to #include rather than > > restating these. >=20 > Ok >=20 > > > >> +/** > >> + * 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? >=20 > There are two places that need those structures. virtio.c and tap.c, > which contains the rx/tx queue processing logic hence the extern. It > makes sense to me that queue handling functions live in tap.c, since > thats where the other code for sending/receiving from the fd was. and > this code does the same work but for vhost. WDYT ? Ok, if it's used in multiple places then yes we need the externs. Splitting the series in the way it is makes that a bit difficult to see. > >> +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, str= uct 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. >=20 > Sure. Will combine with the implementation commit >=20 > > > >> #endif /* VIRTIO_H */ > >> --=20 > >> 2.34.1 > >>=20 >=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 --LYLEG+QciBcPOweO Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmp9GvAACgkQzQJF27ox 2GewZw/9GoSx3Lk1NHJ/UvElsCYpP4W0GDoduGAnWXH+S90Wl/uC7D3N/aKjSS9C dUxSqKg0mpxncgJcq5Rsjbcawn9GhvOBIic3EYiNXEgW8MiUh4+uBymQ9L3PT3co DNiYTEOGGSvqf5QgipDDpSUP4myFY2FMn9G8rxLYhf9PDIyQ0Ge9fOjrc4G0LvFZ 85QqBV7nHRc0kCMjGQFF7sHdB6gW0Xmd2xm5zHEBJYVFwxxpd0APZSa4doy0vHD+ KjM0lF3mU5gUFZuEX4UzGmMX8C1kHRiEvxnYQECewMnOPtgRorp+RUCL4gzUwv3f +9QpmTMFC0gnLXIS1l9SoKwNgylaEGM8UoBP6834NN/nE5W/6fsZHmt/+Ce4w4ZY dMJSpWvNkV/zrBIw2BTeMhc2yDi4aBY9qBFlsXIQoKpyVH9zsXQFAesbH1aSniD1 KAse35biJVGClthQ59EpCxfJBbUv1vTf0nhc2f2+KgpBQUZe+BY9nx/iceHaixhU QS0JGUiu4KxbY+T7I88kNmWO1bAmbKgCICtSvu6VLTIQTBT5DxshtwrPatASS8Tb Jphr7WxvBUg2VKNY/b8pjDi1WbWPWn3ffMbFIyxhQy0zKzURCZtgooxTSyNfWrgF +DfguCxSRe75z/zKYsfS3hj5tdev5lJV3huKYfqycAk5jJ4J5Os= =7AIr -----END PGP SIGNATURE----- --LYLEG+QciBcPOweO--