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 > > 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(-) > > diff --git a/virtio.c b/virtio.c > index d7016cc..d89d485 100644 > --- a/virtio.c > +++ b/virtio.c > @@ -72,19 +72,47 @@ > * SUCH DAMAGE. > */ > > +#include "passt.h" > #include > +#include > #include > #include > #include > #include > #include > +#include > #include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > > #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(PAGE_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 > > -#define VIRTQUEUE_MAX_SIZE 1024 > +union vhost_memory_u vhost_memory = { > + .mem = { > + .nregions = N_VHOST_REGIONS, > + }, > +}; > > /** > * vu_gpa_to_va() - Translate guest physical address to our virtual address. > @@ -766,3 +794,256 @@ void vu_queue_flush(const struct vu_dev *vdev, struct vu_virtq *vq, > if ((uint16_t)(new - vq->signalled_used) < (uint16_t)(new - old)) > vq->signalled_used_valid = 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 = > + (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 = 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 = ioctl(vhost_fd, VHOST_SET_OWNER, NULL); > + if (rc < 0) > + die_perror("VHOST_SET_OWNER ioctl on /dev/vhost-net failed"); > + > + rc = 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 &= req_features; > + if (c->virtio_features != req_features) > + die("vhost does not support required features"); > + > + rc = 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 = 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 = 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 = { .index = queue_idx }; > + struct vhost_vring_file kick_file = { .index = queue_idx }; > + struct vhost_vring_file err_file = { .index = queue_idx }; > + > + struct vhost_vring_state state = { > + .index = queue_idx, > + .num = VHOST_NDESCS, > + }; > + union epoll_ref ref = { > + .type = EPOLL_TYPE_VHOST_CALL, > + .queue = queue_idx, > + }; > + struct epoll_event ev; > + int rc; > + > + call_file.fd = 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 = call_file.fd; > + > + rc = 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 = (struct epoll_event){ .data.u64 = ref.u64, .events = EPOLLIN }; > + rc = 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 = call_file.fd; > + > + err_file.fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC); > + if (err_file.fd < 0) > + die_perror("Failed to create error eventfd"); > + > + rc = 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 = EPOLL_TYPE_VHOST_ERROR; > + ref.fd = err_file.fd; > + ev.data.u64 = ref.u64; > + rc = 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 = err_file.fd; We'd generally prefer to use the existing epoll_add() helper, rather than open coding an EPOLL_CTL_ADD call. > + > + rc = 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 = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC); > + if (kick_file.fd < 0) > + die_perror("Failed to create kick eventfd"); > + > + rc = 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 = kick_file.fd; > + > + vqs[queue_idx].num_free = 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 == HVA. > + */ > +int setup_memory_table(struct ctx *c) { > +#define VHOST_MEMORY_REGION_PTR(addr, size) \ > + (struct vhost_memory_region) { \ > + .guest_phys_addr = (uintptr_t)addr, \ > + .memory_size = size, \ > + .userspace_addr = (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] = VHOST_MEMORY_REGION(pkt_buf); > + vhost_memory.mem.regions[1] = VHOST_MEMORY_REGION(eth_pad); > + > + /* tcp specific buffers */ > + vhost_memory.mem.regions[2] = VHOST_MEMORY_REGION(tcp_payload_tap_hdr); > + vhost_memory.mem.regions[3] = VHOST_MEMORY_REGION(tcp4_payload_ip); > + vhost_memory.mem.regions[4] = VHOST_MEMORY_REGION(tcp6_payload_ip); > + vhost_memory.mem.regions[5] = VHOST_MEMORY_REGION(tcp_payload); > + vhost_memory.mem.regions[6] = VHOST_MEMORY_REGION(tcp_eth_hdr); > + > + /* udp specific buffers */ > + vhost_memory.mem.regions[7] = VHOST_MEMORY_REGION(udp_payload); > + vhost_memory.mem.regions[8] = VHOST_MEMORY_REGION(udp_eth_hdr); > + vhost_memory.mem.regions[9] = VHOST_MEMORY_REGION(udp_iov_recv); > + vhost_memory.mem.regions[10] = VHOST_MEMORY_REGION(udp_mh_recv); > + vhost_memory.mem.regions[11] = 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 = 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 backend > + * @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 = c->fd_vhost; > + struct vhost_vring_addr addr = { > + .index = queue_idx, > + .desc_user_addr = (unsigned long)vring_desc[queue_idx], > + .avail_user_addr = (unsigned long)&vring_avail_all[queue_idx], > + .used_user_addr = (unsigned long)&vring_used_all[queue_idx], > + .log_guest_addr = (unsigned long)&vring_used_all[queue_idx], > + }; > + struct vhost_vring_file file = { > + .index = queue_idx, > + .fd = 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 = 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 == 0) { > + for (i = 0; i < VHOST_NDESCS; ++i) { > + vring_desc[0][i].addr = (uintptr_t)pkt_buf + > + i * (PKT_BUF_BYTES / VHOST_NDESCS); > + vring_desc[0][i].len = PKT_BUF_BYTES / VHOST_NDESCS; > + vring_desc[0][i].flags = VRING_DESC_F_WRITE; > + } > + > + for (i = 0; i < VHOST_NDESCS; ++i) > + vring_avail_all[0].avail.ring[i] = htole16(i); > + > + rx_descriptor_handoff(c); > + } > + > + if (queue_idx == 1) { > + for (i = 0; i < (VHOST_NDESCS - 1); ++i) { > + vring_desc[1][i].next = i+1; > + } > + } > + > + debug("qid: %d", file.index); > + debug("tap fd: %d", file.fd); > + rc = 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 += vqs[0].num_free; > + vqs[0].num_free = 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 kernel > + * 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) > + */ > +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 > + */ > + smp_mb(); > + > + if (!(used->flags & VRING_USED_F_NO_NOTIFY)) > + eventfd_write(kick_fd, 1); > +} > \ No newline at end of file ^ What diff said. -- 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