From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: passt.top; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256 header.s=20251104 header.b=fWABzVcP; dkim-atps=neutral Received: from mail-wm1-x32c.google.com (mail-wm1-x32c.google.com [IPv6:2a00:1450:4864:20::32c]) by passt.top (Postfix) with ESMTPS id B21975A0262 for ; Wed, 12 Aug 2026 19:57:05 +0200 (CEST) Received: by mail-wm1-x32c.google.com with SMTP id 5b1f17b1804b1-4954a32cf1eso5838845e9.3 for ; Wed, 12 Aug 2026 10:57:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1786557425; x=1787162225; darn=passt.top; h=in-reply-to:references:from:subject:cc:to:message-id:date :content-type:content-transfer-encoding:mime-version:from:to:cc :subject:date:message-id:reply-to:content-type; bh=lQXFOWD43PAR/1RVNRr/ShLCcDD2OugNiPsVqQPpwR4=; b=fWABzVcPnGZKypMOJ7MHQ77HdskC2hQlVYGe9ViO13WEuOT1Q/qNoZDFE7mWoqI34E LQCrK988wdTG4ZOlRgPlc6nCpS5nSUAqOXqTat0zCi4faaBq7roGx982UNC+jeAcSWSi IrZiiCPoUn3+ysnS++mVk4tbRp+2J2bW58zwSlvcsmChLjLRo2r8hzAtvt5YqvuYiKHp 6Pe9OJFAt6TrdfF3Bsf5vFUyXKHGmDMG01Lo+xHFI+2xQ850c8IIol8j2O0CNFemPMno KXwZ4eH/hYxlE2JGo3+nh9MXoRjAxrzKnNPjCttProuLk8Vz0+N00IFrgT9azPqOl3zL FYtg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1786557425; x=1787162225; h=in-reply-to:references:from:subject:cc:to:message-id:date :content-type:content-transfer-encoding:mime-version:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to :content-type; bh=lQXFOWD43PAR/1RVNRr/ShLCcDD2OugNiPsVqQPpwR4=; b=P6knRFppkvYGAYR+h5c9TEvVojKEVKIBKSKRkYCJetuaGVtORfQLJlkJzuZXMuGMLr +2syNbp7136+nn6FqULbZEcsH12o0ffyf8ZQz+XV0afM0UY32ulbwiD4h8vdGu+6wKYE qBZ5yBrAxrBfrRuFr203enozh/Kuhpy9B+oJbUDSsW7ae0yixfk48taZKoeMwwAs6Px3 WEPhWpneOGig2iK2s50oV1oMhqMKQsNyMbQ0fpKZwVUwrst+a6js9d/KzaT40lQ+09tr DS2SSuCMABGZw/Fo28uD/+nXGSaPD8g7bMnxqBxGv6hlhos9uNc19x7TVWDsjntpnwd9 6rlg== X-Gm-Message-State: AOJu0YxT24EUKebOWEGyhMfYMWaZeO1E7Fkb2l9N1Ink90jWDRVFCtQ4 RZSVgTgDHHfvf9lCj2HsqitWPtWgYJqek/hzsMngaHiabCk3kU5nq720 X-Gm-Gg: AR+sD11uzddwEq/HDVrE/0Lli485SREdP6pHDW/6yniq+sOZsuPnjKSG9N6ChT7WQjU rqCdhX+9+IY9vIhLmNJLWHWLbX5SkI8Xgy2oY0KpeDIiZSxBpHwY8I4auchpf4gBscvEvdKYmDi pMquUcPzfoLt8nAIabDH0AVAexFY4Njpae85l1160cjSJi4jwUhvGoB3pA3GMKRa4UzGTQOfIff RE//m8JTE2MPdHH9Vihhs4WSNrtu2vePHEOGk3JfqB5hIXdxFi4W0KP0/Z1baeVNJ9qxdjvQpo5 th72OY7/WmW+9M+p74KZIsmsRYWb9nqfrKImJAZjFuO46INdRQ91RzPKIFp8vWt5vlocWQXpg2U QTfDve9eV1XxL5DQhYKSLhbGZJs0VyCauOJQ+t7q3VqcuLJZMxSPNXhC96Zx+AgcA1zVGoNLlYd HSf/4Ax6mG/NbiZeMBBzl9VXawkeWoiwqg7G3cCdxuIA1uWKPyE+X9iBA6nDW2ImvR1KVgnQvde iMkRMmiORVTPcLgEPOb0n9kdBB1CkE1WuwKZjbzYw== X-Received: by 2002:a05:600c:c494:b0:499:7a36:88b with SMTP id 5b1f17b1804b1-4997c0c4983mr75724605e9.5.1786557424983; Wed, 12 Aug 2026 10:57:04 -0700 (PDT) Received: from localhost ([196.157.64.51]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-49981b0f4afsm6896985e9.5.2026.08.12.10.57.02 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 12 Aug 2026 10:57:03 -0700 (PDT) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Wed, 12 Aug 2026 20:57:01 +0300 Message-Id: To: "David Gibson" , "Ammar Yasser" Subject: Re: [RFC v3 5/8] virtio: Implement the pasta vhost functions From: "Ammar Yasser" X-Mailer: aerc 0.21.0 References: <20260802132155.870796-1-aerosound161@gmail.com> <20260802132155.870796-6-aerosound161@gmail.com> In-Reply-To: Message-ID-Hash: VPFREH4JRULT6YYNW36R53M3LX4XJM2H X-Message-ID-Hash: VPFREH4JRULT6YYNW36R53M3LX4XJM2H X-MailFrom: aerosound161@gmail.com 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: > 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. Yeah, thats the approach i will go with >> + >> + >> +struct vq_state vqs[2]; > > 'vqs' isn't really an adequate name for a global variable, especially > since this is fully global, not 'static'. Noted >> +/** >> + * 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. Ok > >> + 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. This would be preferrable in the auto mode you described in an earlier message. But i think for an explicit request to vhost, we should fail. let me know what you think though > >> + 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 = queue >> + * @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). Noted > >> + 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. Noted > >> + 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 EPOLLI= N }; >> + 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. Noted > >> + >> + 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 (queu= e %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 ca= n >> + * 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. Noted. Will add a more explicit comment > >> + */ >> +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. Ok. will go with another name > >> + >> + /* 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 You mean every protocol file registers its own memory regions through calling the macro and taking as input the vhost_memory struct ? It will be better in the sense that it will remove the need for making those buffers public. But i think it will be harder to follow from a readability perspective. WDYT ? >> + * 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. If by static you mean it gets defined in only one place and not exported then its used in two places (virtio.c and tap.c). Unless you want a duplicate definition which i personally am not in favor of. Can you please clarify what you mean by a prefixed name ? Not sure i understand what a proper prefix to a function like this would be > >> +{ >> + 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 k= ernel >> + * virtio thread is actively reading descriptors or no >> + * @kick_fd: Which fd to notify about (will differ by which queue we ar= e about >> + * to announce availability in)=20 >> + */ >> +void vhost_kick(struct vring_used *used, int kick_fd) { > > Again, does this need to be global? Yep, like the rest, both tap.c and virtio.c call it > >> + /* 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. Noted