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=Qi1QcXUh; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 773CC5A0269 for ; Mon, 10 Aug 2026 08:34:14 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202608; t=1786343650; bh=PVzUbpbQ7qTCgk9mwTU3zMjs0ubp4+esJQDMDumWdnU=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=Qi1QcXUhMpOWsBKnv6IqdF0J2f4YWQjKptIcMYapfPQeaCse9hUy/BAhHJ8T6AT6c ED5Rohl2aH0gnTntn7MkPpIqrMOSVTw4OOKnTk7PWAFCC5aKv2XaYqI/DUoVGLbXoy sG2aPT3qJPbd6n0Ce2LrSSy8niJtoyxO2tQ/uMV0oU3caTGS4cOi8viqg5yyBlsxG8 8ATlGpC+1N8TP6aHBA5oEAzWw5ZhuvInPsDkUQG/6NpEpyK6LSDMYMr3Z0bxvosncj mJVj6DTaTynI+aEOwcYFKUegztRrFmPz4Azf0ANNHX2is3cm+EUTHDz5ILhwMafLfd anrEWe2MBRAXA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4hJQ2V3FPkz4w91; Mon, 10 Aug 2026 16:34:10 +1000 (AEST) Date: Mon, 10 Aug 2026 16:34:04 +1000 From: David Gibson To: Ammar Yasser Subject: Re: [RFC v3 5/8] virtio: Implement the pasta vhost functions Message-ID: References: <20260802132155.870796-1-aerosound161@gmail.com> <20260802132155.870796-6-aerosound161@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="M6AcMBjePTP7S+j0" Content-Disposition: inline In-Reply-To: <20260802132155.870796-6-aerosound161@gmail.com> Message-ID-Hash: COTGDYTOMZA5QW6JREZFHTSBGHGXE4AR X-Message-ID-Hash: COTGDYTOMZA5QW6JREZFHTSBGHGXE4AR 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: --M6AcMBjePTP7S+j0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sun, Aug 02, 2026 at 01:21:52PM +0000, Ammar Yasser wrote: > - setup_vhost_net: try to open /dev/vhost-net and negotiate the needed > features on the device file descriptor. Set the descriptor on > c->fd_host on success. > - setup_eventfds: setup the call, kick and err files for a given > queue_index. > - setup_memory_table: create the memory table object that gets shared > with the kernel so it writes directly to the global pkt_buf on rx, and > when we receive data from the host and eventually write it the > protocol specific buffers the kernel is able to read from them. we > share only buffers for tcp and udp as they are the only two protocols > that will have vhost acceleration. besides, other protocols don't > have a global buffer from which they allocate packets to send to the > guest and would be trickier to support >=20 > Signed-off-by: Ammar Yasser Similar to notes on the earlier patches, I think this will be clearer with the new vhost-kernel related functions in new .c and .h files, rather than mixed in with the general virtio helpers. > --- > virtio.c | 283 ++++++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 282 insertions(+), 1 deletion(-) >=20 > diff --git a/virtio.c b/virtio.c > index d7016cc..d89d485 100644 > --- a/virtio.c > +++ b/virtio.c > @@ -72,19 +72,47 @@ > * SUCH DAMAGE. > */ > =20 > +#include "passt.h" > #include > +#include > #include > #include > #include > #include > #include > +#include > #include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > =20 > #include "util.h" > #include "virtio.h" > #include "vhost_user.h" > +#include "tcp_buf.h" > +#include "epoll_ctl.h" > +#include "udp.h" > + > + > +struct vq_state vqs[2]; 'vqs' isn't really an adequate name for a global variable, especially since this is fully global, not 'static'. > + > +struct vring_desc vring_desc[2][VHOST_NDESCS] __attribute__((aligned(PAG= E_SIZE))); > + > +#pragma GCC diagnostic push > +#pragma GCC diagnostic ignored "-Wpedantic" > +union vring_avail_u vring_avail_all[2] __attribute__((aligned(PAGE_SIZE)= )); > +union vring_used_u vring_used_all[2] __attribute__((aligned(PAGE_SIZE))); > +#pragma GCC diagnostic pop > =20 > -#define VIRTQUEUE_MAX_SIZE 1024 > +union vhost_memory_u vhost_memory =3D { > + .mem =3D { > + .nregions =3D N_VHOST_REGIONS, > + }, > +}; > =20 > /** > * vu_gpa_to_va() - Translate guest physical address to our virtual addr= ess. > @@ -766,3 +794,256 @@ void vu_queue_flush(const struct vu_dev *vdev, stru= ct vu_virtq *vq, > if ((uint16_t)(new - vq->signalled_used) < (uint16_t)(new - old)) > vq->signalled_used_valid =3D false; > } > + > +/** > + * setup_vhost_net() - Open and negotiate features on /dev/vhost-net > + * @c: Execution context; c->fd_vhost is set on success > + * > + */ > +void setup_vhost_net(struct ctx *c) > +{ > + static const uint64_t req_features =3D > + (1ULL << VIRTIO_F_VERSION_1) | (1ULL << VHOST_NET_F_VIRTIO_NET_HDR); Since it's also 'const' anyway, I'm not sure the 'static' does anything useful. > + int vhost_fd, rc; > + > + vhost_fd =3D open("/dev/vhost-net", O_RDWR | O_NONBLOCK | O_CLOEXEC); > + if (vhost_fd < 0) > + die_perror("Failed to open /dev/vhost-net"); We probably want to be able to fall back to the /dev/net/tun character device if vhost doesn't work. So making this setup function fallible would be preferable to using die() on errors. > + rc =3D ioctl(vhost_fd, VHOST_SET_OWNER, NULL); > + if (rc < 0) > + die_perror("VHOST_SET_OWNER ioctl on /dev/vhost-net failed"); > + > + rc =3D ioctl(vhost_fd, VHOST_GET_FEATURES, &c->virtio_features); > + if (rc < 0) > + die_perror("VHOST_GET_FEATURES ioctl on /dev/vhost-net failed"); > + > + debug("vhost features: %lx", c->virtio_features); > + debug("req features: %lx", req_features); > + > + c->virtio_features &=3D req_features; > + if (c->virtio_features !=3D req_features) > + die("vhost does not support required features"); > + > + rc =3D ioctl(vhost_fd, VHOST_SET_FEATURES, &c->virtio_features); > + if (rc < 0) > + die_perror("VHOST_SET_FEATURES ioctl on /dev/vhost-net failed"); > + > + c->fd_vhost =3D vhost_fd; > +} > + > +/** > + * setup_eventfds() - Set up call/kick eventfds and vring size for one q= ueue > + * @c: Execution context; c->fd_vhost must already be set > + * @queue_idx: Index of the queue (vring) to configure > + * > + */ > +void setup_eventfds(struct ctx *c, int queue_idx) > +{ > + int vhost_fd =3D c->fd_vhost; We use (when possible) the "reverse christmas tree" convention for ordering locals, which would but this further down (see CONTRIBUTING.md for more details). > + struct vhost_vring_file call_file =3D { .index =3D queue_idx }; > + struct vhost_vring_file kick_file =3D { .index =3D queue_idx }; > + struct vhost_vring_file err_file =3D { .index =3D queue_idx }; > + > + struct vhost_vring_state state =3D { > + .index =3D queue_idx, > + .num =3D VHOST_NDESCS, > + }; > + union epoll_ref ref =3D { > + .type =3D EPOLL_TYPE_VHOST_CALL, > + .queue =3D queue_idx, > + }; > + struct epoll_event ev; > + int rc; > + > + call_file.fd =3D eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC); > + if (call_file.fd < 0) > + die_perror("Failed to create call eventfd"); This error isn't particularly meaningful to an end user, since it doesn't mention vhost-kernel at all. > + ref.fd =3D call_file.fd; > + > + rc =3D ioctl(vhost_fd, VHOST_SET_VRING_CALL, &call_file); > + if (rc < 0) > + die_perror("VHOST_SET_VRING_CALL ioctl on /dev/vhost-net failed"); > + > + ev =3D (struct epoll_event){ .data.u64 =3D ref.u64, .events =3D EPOLLIN= }; > + rc =3D epoll_ctl(c->epollfd, EPOLL_CTL_ADD, ref.fd, &ev); > + if (rc < 0) > + die_perror("Failed to add call eventfd to epoll"); > + c->vq[queue_idx].call_fd =3D call_file.fd; > + > + err_file.fd =3D eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC); > + if (err_file.fd < 0) > + die_perror("Failed to create error eventfd"); > + > + rc =3D ioctl(vhost_fd, VHOST_SET_VRING_ERR, &err_file); > + if (rc < 0) > + die_perror("VHOST_SET_VRING_ERR ioctl on /dev/vhost-net failed"); > + > + ref.type =3D EPOLL_TYPE_VHOST_ERROR; > + ref.fd =3D err_file.fd; > + ev.data.u64 =3D ref.u64; > + rc =3D epoll_ctl(c->epollfd, EPOLL_CTL_ADD, ref.fd, &ev); > + if (rc < 0) > + die_perror("Failed to add error eventfd to epoll"); > + c->vq[queue_idx].err_fd =3D err_file.fd; We'd generally prefer to use the existing epoll_add() helper, rather than open coding an EPOLL_CTL_ADD call. > + > + rc =3D ioctl(vhost_fd, VHOST_SET_VRING_NUM, &state); > + if (rc < 0) { > + die_perror("VHOST_SET_VRING_NUM ioctl on /dev/vhost-net failed (queue = %d)", > + queue_idx); > + } > + > + kick_file.fd =3D eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC); > + if (kick_file.fd < 0) > + die_perror("Failed to create kick eventfd"); > + > + rc =3D ioctl(vhost_fd, VHOST_SET_VRING_KICK, &kick_file); > + if (rc < 0) { > + die_perror("VHOST_SET_VRING_KICK ioctl on /dev/vhost-net failed (queue= %d)", > + queue_idx); > + } > + > + c->vq[queue_idx].kick_fd =3D kick_file.fd; > + > + vqs[queue_idx].num_free =3D VHOST_NDESCS; > +} > + > +/** > + * setup_memory_table() - Register the GPA/HVA translation table > + * @c: Execution context; c->fd_vhost must already be set > + * > + * pasta has no real guest, so container->host addresses are 1:1 and can > + * be interpreted directly rather than translated. This is a little unclear, I'd say explicitly that we use a mapping where GPA =3D=3D HVA. > + */ > +int setup_memory_table(struct ctx *c) { > +#define VHOST_MEMORY_REGION_PTR(addr, size) \ > + (struct vhost_memory_region) { \ > + .guest_phys_addr =3D (uintptr_t)addr, \ > + .memory_size =3D size, \ > + .userspace_addr =3D (uintptr_t)addr, \ > + } > +#define VHOST_MEMORY_REGION(elem) VHOST_MEMORY_REGION_PTR(&elem, sizeof(= elem)) "elem" is probably not a good name here. Here it's any contiguous variable, but in the vhost-user code, "elem" means a specific data structure within the descriptor rings. > + > + /* general purpose buffers */ > + vhost_memory.mem.regions[0] =3D VHOST_MEMORY_REGION(pkt_buf); > + vhost_memory.mem.regions[1] =3D VHOST_MEMORY_REGION(eth_pad); > + > + /* tcp specific buffers */ > + vhost_memory.mem.regions[2] =3D VHOST_MEMORY_REGION(tcp_payload_tap_= hdr); > + vhost_memory.mem.regions[3] =3D VHOST_MEMORY_REGION(tcp4_payload_ip); > + vhost_memory.mem.regions[4] =3D VHOST_MEMORY_REGION(tcp6_payload_ip); > + vhost_memory.mem.regions[5] =3D VHOST_MEMORY_REGION(tcp_payload); > + vhost_memory.mem.regions[6] =3D VHOST_MEMORY_REGION(tcp_eth_hdr); > + > + /* udp specific buffers */ > + vhost_memory.mem.regions[7] =3D VHOST_MEMORY_REGION(udp_payload); > + vhost_memory.mem.regions[8] =3D VHOST_MEMORY_REGION(udp_eth_hdr); > + vhost_memory.mem.regions[9] =3D VHOST_MEMORY_REGION(udp_iov_recv); > + vhost_memory.mem.regions[10] =3D VHOST_MEMORY_REGION(udp_mh_recv); > + vhost_memory.mem.regions[11] =3D VHOST_MEMORY_REGION(udp_meta); Not sure if there would be value in delegating these to helpers in tcp.c and udp.c > + vhost_memory.mem.nregions =3D 12; > + > + return ioctl(c->fd_vhost, VHOST_SET_MEM_TABLE, &vhost_memory.mem); > +} > + > + > + > +/** > + * set_vring_for_queue() - Register a vring's addresses and bind its bac= kend > + * @c: Execution context; c->fd_vhost must already be set > + * @queue_idx: Index of the queue (vring) to configure > + * @tap_fd: Tap fd to bind as this queue's backend > + * > + */ > +void set_vring_for_queue(struct ctx *c, int queue_idx, int tap_fd) > +{ > + int vhost_fd =3D c->fd_vhost; > + struct vhost_vring_addr addr =3D { > + .index =3D queue_idx, > + .desc_user_addr =3D (unsigned long)vring_desc[queue_idx], > + .avail_user_addr =3D (unsigned long)&vring_avail_all[queue_idx], > + .used_user_addr =3D (unsigned long)&vring_used_all[queue_idx], > + .log_guest_addr =3D (unsigned long)&vring_used_all[queue_idx], > + }; > + struct vhost_vring_file file =3D { > + .index =3D queue_idx, > + .fd =3D tap_fd, > + }; > + unsigned int i; > + int rc; > + > + debug("qid: %d", queue_idx); > + debug("vhost desc addr: 0x%llx", addr.desc_user_addr); > + debug("vhost avail addr: 0x%llx", addr.avail_user_addr); > + debug("vhost used addr: 0x%llx", addr.used_user_addr); > + > + rc =3D ioctl(vhost_fd, VHOST_SET_VRING_ADDR, &addr); > + if (rc < 0) > + die_perror("VHOST_SET_VRING_ADDR ioctl on /dev/vhost-net failed"); > + > + if (queue_idx =3D=3D 0) { > + for (i =3D 0; i < VHOST_NDESCS; ++i) { > + vring_desc[0][i].addr =3D (uintptr_t)pkt_buf + > + i * (PKT_BUF_BYTES / VHOST_NDESCS); > + vring_desc[0][i].len =3D PKT_BUF_BYTES / VHOST_NDESCS; > + vring_desc[0][i].flags =3D VRING_DESC_F_WRITE; > + } > + > + for (i =3D 0; i < VHOST_NDESCS; ++i) > + vring_avail_all[0].avail.ring[i] =3D htole16(i); > + > + rx_descriptor_handoff(c); > + } > + > + if (queue_idx =3D=3D 1) { > + for (i =3D 0; i < (VHOST_NDESCS - 1); ++i) { > + vring_desc[1][i].next =3D i+1; > + } > + } > + > + debug("qid: %d", file.index); > + debug("tap fd: %d", file.fd); > + rc =3D ioctl(vhost_fd, VHOST_NET_SET_BACKEND, &file); > + if (rc < 0) > + die_perror("VHOST_NET_SET_BACKEND ioctl on /dev/vhost-net failed"); > +} > + > +/** > + * rx_descriptor_handoff() - Batch-announce freed RX descriptors to the = kernel > + * @c: Execution context > + * > + * Bumps avail.idx by the number of descriptors accumulated in > + * vqs[0].num_free (from prior consume_one_rx_descriptor() calls), > + * then resets the counter to zero. The kernel will see the new > + * avail.idx and consume the freshly-available descriptors. > + * > + */ > +void rx_descriptor_handoff(struct ctx *c) Can this be static? That's the sort of thing that's harder to review when signatures are split from implementations. If not, it should have a properly prefixed name. > +{ > + smp_wmb(); > + > + if (!vqs[0].num_free) > + return; > + > + vring_avail_all[0].avail.idx +=3D vqs[0].num_free; > + vqs[0].num_free =3D 0; > + vhost_kick(&vring_used_all[0].used, c->vq[0].kick_fd); > +} > + > + > +/** > + * vhost_kick() - Notify the kernel that new descriptors are available > + * @used: The vring_used queue. Taken as a parameter to check if the ke= rnel > + * virtio thread is actively reading descriptors or no > + * @kick_fd: Which fd to notify about (will differ by which queue we are= about > + * to announce availability in)=20 > + */ > +void vhost_kick(struct vring_used *used, int kick_fd) { Again, does this need to be global? > + /* Ensure that the read to used->flags doesn't get reordered to be > + * above the avail.idx update=20 > + */ > + smp_mb(); > + > + if (!(used->flags & VRING_USED_F_NO_NOTIFY)) > + eventfd_write(kick_fd, 1); > +} > \ No newline at end of file ^ What diff said. --=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 --M6AcMBjePTP7S+j0 Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmp5cNYACgkQzQJF27ox 2Gc3oA//cnPm0Ibt6YZDEi5ioFvydX1ylcGRN4yPLxYp2XZDqdf8HXwTD613bGMo uwDUyIj37VNVko43pX/KbNrezdkVomAO6K9/lnNojIIJ7Ocem2sOX0Eq+nSsTyj/ EEbig2YL6dyyb3g+E/G/lFfQkGaazV1o16qaYFUyM9IhSISbTGwrPJ3aCsQSGpMO t8WHdvmC2xGTAA0uGR4u1EWz9NX4WIKQH1ILAUmzP3xD5fgA3GupRqJcu06JsCTZ 50GK9sqp0I+ypLTkeUevBBjjcgwdDe3gFben5UmAtgTD8dXPmHSmI8UcpB4kQYu2 FQZdGoFGpwuZyG0Y393ctCbGBItaydDsJdnMtQK02ceCHy4gT85ED7CR38Wi0MHM ybMj5If7drmRuMYSsruZMzTANjelXHow4t/VcszlFfs5yisZIwVS9vMfBC+X1hYe jiPCOmcBnXv3VpOYK+KMoxh4HFXhAtJo3G4C8WIiKnHR0iNdjApjSRtDiqnNAxWt ozel9K+L9i1A9L0AyuQqyYNaEQWK1kmGofD40ZUsSg2taIbHtDh0iu0LaUO8WCmc wVw5XxcUtbXy0jT2BluO7Mak7TsZw8sCaqgodweVhUcY6SYm+BCWm8c/IlG4Mawp cC8cP9kLOD6TDjjgKxjBd308IiksOX3qX9uaHTWk+t37B+LtAOs= =F2o0 -----END PGP SIGNATURE----- --M6AcMBjePTP7S+j0--