It was defined in tap.h, put it on its own so that future callers won't need to depend on all definitions in tap.h Signed-off-by: Ammar Yasser --- tap.h | 9 +-------- tap_hdr.h | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+), 8 deletions(-) create mode 100644 tap_hdr.h diff --git a/tap.h b/tap.h index b335933..1625975 100644 --- a/tap.h +++ b/tap.h @@ -10,6 +10,7 @@ #include #include "passt.h" +#include "tap_hdr.h" /** L2_MAX_LEN_PASTA - Maximum frame length for pasta mode (with L2 header) * @@ -38,14 +39,6 @@ struct udphdr; -/** - * struct tap_hdr - tap backend specific headers - * @vnet_len: Frame length (for qemu socket transport) - */ -struct tap_hdr { - uint32_t vnet_len; -} __attribute__((packed)); - /** * tap_hdr_iov() - struct iovec for a tap header * @c: Execution context diff --git a/tap_hdr.h b/tap_hdr.h new file mode 100644 index 0000000..15bdb5b --- /dev/null +++ b/tap_hdr.h @@ -0,0 +1,19 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later + * Copyright (c) 2021 Red Hat GmbH + * Author: Stefano Brivio + */ + +#ifndef TAP_HDR_H +#define TAP_HDR_H + +#include + +/** + * struct tap_hdr - tap backend specific headers + * @vnet_len: Frame length (for qemu socket transport) + */ +struct tap_hdr { + uint32_t vnet_len; +} __attribute__((packed)); + +#endif /* TAP_HDR_H */ -- 2.34.1

The --vhost controls whether or no to use vhost acceleration for pasta. The EPOLL_TYPE_VHOST_CALL means that kernel wants to notify us about data it has written. EPOLL_TYPE_VHOST_ERROR means the kernel encountered an internal error on vhost and wants to notify us that something went wrong. Add the fd_vhost field on the context which will carry the file descriptor of the device and will indicate that the setup was successful. The vq field contains the kick, call and err file descriptors for both queues. Also add vhost (mark if vhost acceleration was requested) and virtio_features field. Signed-off-by: Ammar Yasser --- conf.c | 6 ++++++ epoll_type.h | 4 ++++ passt.c | 3 +++ passt.h | 15 +++++++++++++++ 4 files changed, 28 insertions(+) diff --git a/conf.c b/conf.c index faf2681..c31546e 100644 --- a/conf.c +++ b/conf.c @@ -736,6 +736,7 @@ pasta_opts: " default: auto\n" " --host-lo-to-ns-lo Translate host-loopback forwards to\n" " namespace loopback\n" + " --vhost\t\tUse vhost-kernel acceleration\n" " --userns NSPATH Target user namespace to join\n" " --netns PATH|NAME Target network namespace to join\n" " --netns-only Don't join existing user namespace\n" @@ -766,6 +767,7 @@ enum passt_modes conf_mode(int argc, char *argv[]) int vhost_user = 0; const struct option optvu[] = { {"vhost-user", no_argument, &vhost_user, 1 }, + {"vhost", no_argument, NULL, 0 }, { 0 }, }; char argv0[PATH_MAX], *basearg0; @@ -1309,6 +1311,7 @@ void conf(struct ctx *c, int argc, char **argv) {"ipv4-only", no_argument, NULL, '4' }, {"ipv6-only", no_argument, NULL, '6' }, {"one-off", no_argument, NULL, '1' }, + {"vhost", no_argument, &c->vhost, 1 }, {"tcp-ports", required_argument, NULL, 't' }, {"udp-ports", required_argument, NULL, 'u' }, {"tcp-ns", required_argument, NULL, 'T' }, @@ -1838,6 +1841,9 @@ void conf(struct ctx *c, int argc, char **argv) die("--no-copy-addrs needs --config-net"); } + if (c->vhost && c->mode != MODE_PASTA) + die("--vhost is only available in pasta mode"); + if (c->mode == MODE_PASTA && c->splice_only) { if (c->no_splice) die("--splice-only is incompatible with --no-splice"); diff --git a/epoll_type.h b/epoll_type.h index 061325a..e3206d1 100644 --- a/epoll_type.h +++ b/epoll_type.h @@ -50,6 +50,10 @@ enum epoll_type { EPOLL_TYPE_CONF_LISTEN, /* Configuration socket */ EPOLL_TYPE_CONF, + /* vhost-kernel call socket */ + EPOLL_TYPE_VHOST_CALL, + /* vhost-kernel error socket */ + EPOLL_TYPE_VHOST_ERROR, EPOLL_NUM_TYPES, }; diff --git a/passt.c b/passt.c index 5054551..865b331 100644 --- a/passt.c +++ b/passt.c @@ -65,6 +65,7 @@ char pkt_buf[PKT_BUF_BYTES] __attribute__ ((aligned(PAGE_SIZE))); struct ctx passt_ctx = { .pidfile_fd = -1, .fd_tap = -1, + .fd_vhost = -1, .fd_tap_listen = -1, .fd_control_listen = -1, .fd_repair_listen = -1, @@ -92,6 +93,8 @@ char *epoll_type_str[] = { [EPOLL_TYPE_NL_NEIGH] = "netlink neighbour notifier socket", [EPOLL_TYPE_CONF_LISTEN] = "configuration listening socket", [EPOLL_TYPE_CONF] = "configuration socket", + [EPOLL_TYPE_VHOST_CALL] = "vhost-kernel call socket", + [EPOLL_TYPE_VHOST_ERROR] = "vhost-kernel error socket", }; static_assert(ARRAY_SIZE(epoll_type_str) == EPOLL_NUM_TYPES, "epoll_type_str[] doesn't match enum epoll_type"); diff --git a/passt.h b/passt.h index 51ccd4f..c729316 100644 --- a/passt.h +++ b/passt.h @@ -183,8 +183,11 @@ struct ip6_ctx { * @fd_repair_listen: File descriptor for listening TCP_REPAIR socket, if any * @fd_repair: Connected AF_UNIX socket for TCP_REPAIR helper * @our_tap_mac: Pasta/passt's MAC on the tap link + * @fd_vhost: File descriptor for /dev/vhost-net, set on vhost setup * @guest_mac: MAC address of guest or namespace, seen or configured * @hash_secret: 128-bit secret for siphash functions + * @virtio_features: Negotiated virtio feature bits + * @vhost: Enable vhost-kernel acceleration for pasta * @ifi4: Template interface for IPv4, -1: none, 0: IPv4 disabled * @ip4: IPv4 configuration * @dns_search: DNS search list @@ -202,6 +205,8 @@ struct ip6_ctx { * @no_udp: Disable UDP operation * @udp: Context for UDP protocol handler * @no_icmp: Disable ICMP operation + * @vq: Per-virtqueue eventfd descriptors for vhost-kernel + * ([0] is RX, [1] is TX; each holds kick_fd, call_fd, err_fd) * @mtu: MTU passed via DHCP/NDP * @no_dns: Do not source/use DNS servers for any purpose * @no_dns_search: Do not source/use domain search lists for any purpose @@ -258,11 +263,14 @@ struct ctx { int fd_control; int fd_repair_listen; int fd_repair; + + int fd_vhost; unsigned char our_tap_mac[ETH_ALEN]; unsigned char guest_mac[ETH_ALEN]; uint16_t mtu; uint64_t hash_secret[2]; + uint64_t virtio_features; int ifi4; struct ip4_ctx ip4; @@ -288,6 +296,12 @@ struct ctx { struct udp_ctx udp; int no_icmp; + struct { + int kick_fd; + int call_fd; + int err_fd; + } vq[2]; + int no_dns; int no_dns_search; int no_dhcp_dns; @@ -300,6 +314,7 @@ struct ctx { int splice_only; int host_lo_to_ns_lo; int freebind; + int vhost; bool chroot_fallback; int low_wmem; -- 2.34.1

- 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 --- 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]; + +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); + 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"); + + 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; + 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"); + 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; + + 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. + */ +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)) + + /* 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); + 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) +{ + 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) { + /* 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 -- 2.34.1

- tap_send_single was changed to directly call tap_send_frames_passt/pasta instead of relying on tap_send_frames because protocols that use tap_send_single won't use vhost acceleration, only tcp and udp will. The function was also moved lower in the file to by the time its called tap_send_frames_* functions are defined and a forward declaration isn't needed - extend udp_meta_t to instead of always carrying a tap_hdr, to carry a union of a tap_hdr and a virtio_net header because if vhost is used the will the latter type of header and then build an iov from this virtio net header in udp.c - create iov_from_virtio_net_hdr to build an iov from the vnet header - in the case that vhost is requested, set the first frame fragment to be a virtio_net header through calling iov_from_virtio_net_hdr - convert the tcp_payload_tap_hdr array to be an array of virtio_net header. in the case where we aren't using vhost, we will just be using the first 4 bytes of it - add a boolean argumen to tap_send_frames_pasta to determine if vhost will be used Signed-off-by: Ammar Yasser --- tap.c | 100 +++++++++++++++++++++++++++++++----------------------- tcp_buf.c | 39 +++++++++++++++++---- tcp_buf.h | 4 +-- udp.c | 28 ++++++++++++++- udp.h | 6 +++- 5 files changed, 125 insertions(+), 52 deletions(-) diff --git a/tap.c b/tap.c index 9d601a2..e177eef 100644 --- a/tap.c +++ b/tap.c @@ -124,45 +124,6 @@ unsigned long tap_l2_max_len(const struct ctx *c) return 0; /* Unreachable, for cppcheck's sake */ } -/** - * tap_send_single() - Send a single frame - * @c: Execution context - * @data: Packet buffer - * @l2len: Total L2 packet length - */ -void tap_send_single(const struct ctx *c, const void *data, size_t l2len) -{ - uint8_t padded[ETH_ZLEN] = { 0 }; - struct iovec iov[2]; - size_t iovcnt = 0; - uint32_t vnet_len; - - if (l2len < ETH_ZLEN) { - memcpy(padded, data, l2len); - data = padded; - l2len = ETH_ZLEN; - } - - vnet_len = htonl(l2len); - - switch (c->mode) { - case MODE_PASST: - iov[iovcnt] = IOV_OF_LVALUE(vnet_len); - iovcnt++; - /* fall through */ - case MODE_PASTA: - iov[iovcnt].iov_base = (void *)data; - iov[iovcnt].iov_len = l2len; - iovcnt++; - - tap_send_frames(c, iov, iovcnt, 1); - break; - case MODE_VU: - vu_send_single(c, data, l2len); - break; - } -} - /** * tap_push_l2h() - Build an L2 header for an inbound packet * @c: Execution context @@ -415,7 +376,7 @@ void tap_icmp6_send(const struct ctx *c, */ static size_t tap_send_frames_pasta(const struct ctx *c, const struct iovec *iov, - size_t bufs_per_frame, size_t nframes) + size_t bufs_per_frame, size_t nframes, bool vhost) { size_t nbufs = bufs_per_frame * nframes; size_t i; @@ -498,6 +459,59 @@ static size_t tap_send_frames_passt(const struct ctx *c, return i / bufs_per_frame; } +/** + * tap_send_single() - Send a single frame + * @c: Execution context + * @data: Packet buffer + * @l2len: Total L2 packet length + */ +void tap_send_single(const struct ctx *c, const void *data, size_t l2len) +{ + uint8_t padded[ETH_ZLEN] = { 0 }; + struct iovec iov[2]; + size_t iovcnt = 0; + int m = 0; + uint32_t vnet_len; + + if (l2len < ETH_ZLEN) { + memcpy(padded, data, l2len); + data = padded; + l2len = ETH_ZLEN; + } + + vnet_len = htonl(l2len); + switch (c->mode) { + case MODE_PASST: + /* create an iov for the length */ + iov[iovcnt] = IOV_OF_LVALUE(vnet_len); + iovcnt++; + /* create the data iov */ + iov[iovcnt].iov_base = (void *)data; + iov[iovcnt].iov_len = l2len; + iovcnt++; + m = tap_send_frames_passt(c, iov, iovcnt, 1); + break; + case MODE_PASTA: + /* don't create a length iov in the case of pasta */ + iov[iovcnt].iov_base = (void *)data; + iov[iovcnt].iov_len = l2len; + iovcnt++; + + m = tap_send_frames_pasta(c, iov, iovcnt, 1, false); + break; + case MODE_VU: + m = vu_send_single(c, data, l2len); + break; + } + + if (m < 1) + debug("tap: failed to send a single frame"); + + pcap_multiple(iov, iovcnt, m, + c->mode == MODE_PASST ? sizeof(uint32_t) : + (c->fd_vhost != -1) ? VNET_HLEN : 0); +} + /** * tap_send_frames() - Send out multiple prepared frames * @c: Execution context @@ -523,7 +537,8 @@ size_t tap_send_frames(const struct ctx *c, const struct iovec *iov, switch (c->mode) { case MODE_PASTA: - m = tap_send_frames_pasta(c, iov, bufs_per_frame, nframes); + /* use vhost in pasta sending only if the vhost setup was actually successful, indicated by a non-zero fd_vhost */ + m = tap_send_frames_pasta(c, iov, bufs_per_frame, nframes, ((c->fd_vhost != -1))); break; case MODE_PASST: m = tap_send_frames_passt(c, iov, bufs_per_frame, nframes); @@ -539,7 +554,8 @@ size_t tap_send_frames(const struct ctx *c, const struct iovec *iov, nframes - m, nframes); pcap_multiple(iov, bufs_per_frame, m, - c->mode == MODE_PASST ? sizeof(uint32_t) : 0); + c->mode == MODE_PASST ? sizeof(uint32_t) : + (c->fd_vhost != -1) ? VNET_HLEN : 0); return m; } diff --git a/tcp_buf.c b/tcp_buf.c index 4452337..080e4cb 100644 --- a/tcp_buf.c +++ b/tcp_buf.c @@ -21,6 +21,7 @@ #include #include +#include #include "util.h" #include "ip.h" @@ -38,7 +39,7 @@ /* Ethernet header for IPv4 and IPv6 frames */ struct ethhdr tcp_eth_hdr[TCP_FRAMES_MEM]; -struct tap_hdr tcp_payload_tap_hdr[TCP_FRAMES_MEM]; +struct virtio_net_hdr_mrg_rxbuf tcp_payload_tap_hdr[TCP_FRAMES_MEM]; /* IP headers for IPv4 and IPv6 */ struct iphdr tcp4_payload_ip[TCP_FRAMES_MEM]; @@ -71,6 +72,14 @@ void tcp_update_l2_buf(const unsigned char *eth_d) eth_update_mac(&tcp_eth_hdr[i], eth_d, NULL); } +static inline struct iovec iov_from_virtio_net_hdr(struct virtio_net_hdr_mrg_rxbuf *hdr) +{ + return (struct iovec){ + .iov_base = hdr, + .iov_len = sizeof(*hdr), + }; +} + /** * tcp_sock_iov_init() - Initialise scatter-gather L2 buffers for IPv4 sockets * @c: Execution context @@ -89,7 +98,14 @@ void tcp_sock_iov_init(const struct ctx *c) for (i = 0; i < TCP_FRAMES_MEM; i++) { struct iovec *iov = tcp_l2_iov[i]; - iov[TCP_IOV_TAP] = tap_hdr_iov(c, &tcp_payload_tap_hdr[i]); + /* If we are using pasta with vhost acceleration, the first entry in the tcp buffers + * should point to a virtio_net header, otherwise a tap header + */ + if ((c->fd_vhost != -1)) + iov[TCP_IOV_TAP] = iov_from_virtio_net_hdr(&tcp_payload_tap_hdr[i]); + else + iov[TCP_IOV_TAP] = tap_hdr_iov(c, (struct tap_hdr *)&tcp_payload_tap_hdr[i]); + iov[TCP_IOV_ETH].iov_len = sizeof(struct ethhdr); iov[TCP_IOV_PAYLOAD].iov_base = &tcp_payload[i]; iov[TCP_IOV_ETH_PAD].iov_base = eth_pad; @@ -135,8 +151,7 @@ void tcp_payload_flush(const struct ctx *c, const struct timespec *now) { size_t m; - m = tap_send_frames(c, &tcp_l2_iov[0][0], TCP_NUM_IOVS, - tcp_payload_used); + m = tap_send_frames(c, &tcp_l2_iov[0][0], TCP_NUM_IOVS, tcp_payload_used); if (m != tcp_payload_used) { tcp_revert_seq(c, &tcp_frame_conns[m], &tcp_l2_iov[m], tcp_payload_used - m, now); @@ -177,7 +192,7 @@ static void tcp_l2_buf_fill_headers(const struct ctx *c, { struct iov_tail tail = IOV_TAIL(&iov[TCP_IOV_PAYLOAD], 1, 0); struct tcphdr th_storage, *th = IOV_REMOVE_HEADER(&tail, th_storage); - struct tap_hdr *taph = iov[TCP_IOV_TAP].iov_base; + const struct flowside *tapside = TAPFLOW(conn); const struct in_addr *a4 = inany_v4(&tapside->oaddr); struct ethhdr *eh = iov[TCP_IOV_ETH].iov_base; @@ -192,7 +207,19 @@ static void tcp_l2_buf_fill_headers(const struct ctx *c, l2len = tcp_fill_headers(c, conn, eh, ip4h, ip6h, th, &tail, iov_tail_size(&tail), csum_flags, seq); - tap_hdr_update(taph, l2len); + + /* when in pasta mode, the length of the tap header iov is zero, so this + * l2len write doesn't do anything. But this function gets called from both + * pasta and passt. Make the l2len write in case we are in passt mode, denoted by + * fd_vhost being -1 since this field won't get initialized at all in passt. + */ + if (c->mode == MODE_PASST) { + iov[TCP_IOV_TAP].iov_len = sizeof(struct tap_hdr); + struct tap_hdr *taph = iov[TCP_IOV_TAP].iov_base; + tap_hdr_update(taph, l2len); + } else if (c->fd_vhost == -1) { /* pasta mode but without vhost */ + iov[TCP_IOV_TAP].iov_len = 0; + } } /** diff --git a/tcp_buf.h b/tcp_buf.h index c749038..da48cd0 100644 --- a/tcp_buf.h +++ b/tcp_buf.h @@ -9,7 +9,7 @@ #include "tcp_conn.h" #include "tcp_internal.h" -void tcp_sock_iov_init(); +void tcp_sock_iov_init(const struct ctx *c); void tcp_payload_flush(const struct ctx *c, const struct timespec *now); int tcp_buf_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn, uint32_t already_sent, const struct timespec *now); @@ -20,7 +20,7 @@ int tcp_buf_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags, #define TCP_FRAMES \ (c->mode == MODE_PASTA ? 1 : TCP_FRAMES_MEM) -extern struct tap_hdr tcp_payload_tap_hdr[TCP_FRAMES_MEM]; +extern struct virtio_net_hdr_mrg_rxbuf tcp_payload_tap_hdr[TCP_FRAMES_MEM]; extern struct ethhdr tcp_eth_hdr[TCP_FRAMES_MEM]; extern struct tcp_payload_t tcp_payload[TCP_FRAMES_MEM]; diff --git a/udp.c b/udp.c index d05ee66..5f90dce 100644 --- a/udp.c +++ b/udp.c @@ -103,6 +103,7 @@ #include #include #include +#include #include "checksum.h" #include "util.h" @@ -119,6 +120,18 @@ #include "udp_vu.h" #include "epoll_ctl.h" +/* UDP header and data for inbound messages */ +struct udp_payload_t udp_payload[UDP_MAX_FRAMES]; + +/* Ethernet headers for IPv4 and IPv6 frames */ +struct ethhdr udp_eth_hdr[UDP_MAX_FRAMES]; + +/* IOVs and msghdr arrays for receiving datagrams from sockets */ +struct iovec udp_iov_recv [UDP_MAX_FRAMES]; +struct mmsghdr udp_mh_recv [UDP_MAX_FRAMES]; + +/* Pre-cooked headers for UDP packets */ +struct udp_meta_t udp_meta[UDP_MAX_FRAMES]; #define UDP_TIMEOUT "/proc/sys/net/netfilter/nf_conntrack_udp_timeout" #define UDP_TIMEOUT_STREAM \ @@ -216,7 +229,20 @@ static void udp_iov_init_one(const struct ctx *c, size_t i) *siov = IOV_OF_LVALUE(payload->data); tiov[UDP_IOV_ETH] = IOV_OF_LVALUE(udp_eth_hdr[i]); - tiov[UDP_IOV_TAP] = tap_hdr_iov(c, &meta->taph); + /* if the vhost tap fd is initialized, this is a sign for us that + * we will be using virtio transport. make the iov that is supposed + * to point to a tap header point instead to a virtio_net_mrg_rxbuf + */ + if (c->fd_vhost != -1) { + struct iovec vnet_iov = { + .iov_base = (void *)(&meta->vnet_hdr), + .iov_len = sizeof(meta->vnet_hdr) + }; + tiov[UDP_IOV_TAP] = vnet_iov; + } else { + tiov[UDP_IOV_TAP] = tap_hdr_iov(c, &meta->taph); + } + tiov[UDP_IOV_PAYLOAD].iov_base = payload; tiov[UDP_IOV_ETH_PAD].iov_base = eth_pad; diff --git a/udp.h b/udp.h index b7a367c..3464b74 100644 --- a/udp.h +++ b/udp.h @@ -9,6 +9,7 @@ #include #include #include +#include #include "tap_hdr.h" #include "fwd.h" @@ -49,7 +50,10 @@ extern struct mmsghdr udp_mh_recv [UDP_MAX_FRAMES]; struct udp_meta_t { struct ipv6hdr ip6h; struct iphdr ip4h; - struct tap_hdr taph; + union { + struct virtio_net_hdr_mrg_rxbuf vnet_hdr; + struct tap_hdr taph; +}; #ifdef __AVX2__ } __attribute__((aligned(32))); #else -- 2.34.1

On Sun, Aug 02, 2026 at 01:21:48PM +0000, Ammar Yasser wrote:

It was defined in tap.h, put it on its own so that future callers won't need to depend on all definitions in tap.h

Signed-off-by: Ammar Yasser

Reviewed-by: David Gibson

--- tap.h | 9 +-------- tap_hdr.h | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+), 8 deletions(-) create mode 100644 tap_hdr.h

diff --git a/tap.h b/tap.h index b335933..1625975 100644 --- a/tap.h +++ b/tap.h @@ -10,6 +10,7 @@ #include

#include "passt.h" +#include "tap_hdr.h"

/** L2_MAX_LEN_PASTA - Maximum frame length for pasta mode (with L2 header) * @@ -38,14 +39,6 @@

struct udphdr;

-/** - * struct tap_hdr - tap backend specific headers - * @vnet_len: Frame length (for qemu socket transport) - */ -struct tap_hdr { - uint32_t vnet_len; -} __attribute__((packed)); - /** * tap_hdr_iov() - struct iovec for a tap header * @c: Execution context diff --git a/tap_hdr.h b/tap_hdr.h new file mode 100644 index 0000000..15bdb5b --- /dev/null +++ b/tap_hdr.h @@ -0,0 +1,19 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later + * Copyright (c) 2021 Red Hat GmbH + * Author: Stefano Brivio + */ + +#ifndef TAP_HDR_H +#define TAP_HDR_H + +#include + +/** + * struct tap_hdr - tap backend specific headers + * @vnet_len: Frame length (for qemu socket transport) + */ +struct tap_hdr { + uint32_t vnet_len; +} __attribute__((packed)); + +#endif /* TAP_HDR_H */ -- 2.34.1

-- 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

On Sun, Aug 02, 2026 at 01:21:50PM +0000, Ammar Yasser wrote:

The --vhost controls whether or no to use vhost acceleration for pasta. The EPOLL_TYPE_VHOST_CALL means that kernel wants to notify us about data it has written. EPOLL_TYPE_VHOST_ERROR means the kernel encountered an internal error on vhost and wants to notify us that something went wrong.

Add the fd_vhost field on the context which will carry the file descriptor of the device and will indicate that the setup was successful. The vq field contains the kick, call and err file descriptors for both queues.

Also add vhost (mark if vhost acceleration was requested) and virtio_features field.

A couple of concerns about the command line interface. IIUC, once this is all ready the difference between vhost-kernel and regular tuntap should be neither guest-visible nor user-visible. So, assuming it does perform better, we'll probably want to make it the default. So, making this explicitly an opt-in probably isn't what we want. Maybe --vhost on|off|auto would be a better idea, defaulting to off now, but auto (use if available) in future. "on" would be if the user definitely wants vhost, and would rather we exit than fall back if it's not available. A more minor concern is that there's the potential confusion between this and --vhost-user. Not immediately sure what we can do about this, since that's just reflecting the confusingly similar names of the underlying features.

Signed-off-by: Ammar Yasser --- conf.c | 6 ++++++ epoll_type.h | 4 ++++ passt.c | 3 +++ passt.h | 15 +++++++++++++++ 4 files changed, 28 insertions(+)

diff --git a/conf.c b/conf.c index faf2681..c31546e 100644 --- a/conf.c +++ b/conf.c @@ -736,6 +736,7 @@ pasta_opts: " default: auto\n" " --host-lo-to-ns-lo Translate host-loopback forwards to\n" " namespace loopback\n" + " --vhost\t\tUse vhost-kernel acceleration\n" " --userns NSPATH Target user namespace to join\n" " --netns PATH|NAME Target network namespace to join\n" " --netns-only Don't join existing user namespace\n" @@ -766,6 +767,7 @@ enum passt_modes conf_mode(int argc, char *argv[]) int vhost_user = 0; const struct option optvu[] = { {"vhost-user", no_argument, &vhost_user, 1 }, + {"vhost", no_argument, NULL, 0 }, { 0 }, }; char argv0[PATH_MAX], *basearg0; @@ -1309,6 +1311,7 @@ void conf(struct ctx *c, int argc, char **argv) {"ipv4-only", no_argument, NULL, '4' }, {"ipv6-only", no_argument, NULL, '6' }, {"one-off", no_argument, NULL, '1' }, + {"vhost", no_argument, &c->vhost, 1 }, {"tcp-ports", required_argument, NULL, 't' }, {"udp-ports", required_argument, NULL, 'u' }, {"tcp-ns", required_argument, NULL, 'T' }, @@ -1838,6 +1841,9 @@ void conf(struct ctx *c, int argc, char **argv) die("--no-copy-addrs needs --config-net"); }

+ if (c->vhost && c->mode != MODE_PASTA) + die("--vhost is only available in pasta mode"); + if (c->mode == MODE_PASTA && c->splice_only) { if (c->no_splice) die("--splice-only is incompatible with --no-splice"); diff --git a/epoll_type.h b/epoll_type.h index 061325a..e3206d1 100644 --- a/epoll_type.h +++ b/epoll_type.h @@ -50,6 +50,10 @@ enum epoll_type { EPOLL_TYPE_CONF_LISTEN, /* Configuration socket */ EPOLL_TYPE_CONF, + /* vhost-kernel call socket */ + EPOLL_TYPE_VHOST_CALL, + /* vhost-kernel error socket */ + EPOLL_TYPE_VHOST_ERROR,

EPOLL_NUM_TYPES, }; diff --git a/passt.c b/passt.c index 5054551..865b331 100644 --- a/passt.c +++ b/passt.c @@ -65,6 +65,7 @@ char pkt_buf[PKT_BUF_BYTES] __attribute__ ((aligned(PAGE_SIZE))); struct ctx passt_ctx = { .pidfile_fd = -1, .fd_tap = -1, + .fd_vhost = -1, .fd_tap_listen = -1, .fd_control_listen = -1, .fd_repair_listen = -1, @@ -92,6 +93,8 @@ char *epoll_type_str[] = { [EPOLL_TYPE_NL_NEIGH] = "netlink neighbour notifier socket", [EPOLL_TYPE_CONF_LISTEN] = "configuration listening socket", [EPOLL_TYPE_CONF] = "configuration socket", + [EPOLL_TYPE_VHOST_CALL] = "vhost-kernel call socket", + [EPOLL_TYPE_VHOST_ERROR] = "vhost-kernel error socket", }; static_assert(ARRAY_SIZE(epoll_type_str) == EPOLL_NUM_TYPES, "epoll_type_str[] doesn't match enum epoll_type"); diff --git a/passt.h b/passt.h index 51ccd4f..c729316 100644 --- a/passt.h +++ b/passt.h @@ -183,8 +183,11 @@ struct ip6_ctx { * @fd_repair_listen: File descriptor for listening TCP_REPAIR socket, if any * @fd_repair: Connected AF_UNIX socket for TCP_REPAIR helper * @our_tap_mac: Pasta/passt's MAC on the tap link + * @fd_vhost: File descriptor for /dev/vhost-net, set on vhost setup * @guest_mac: MAC address of guest or namespace, seen or configured * @hash_secret: 128-bit secret for siphash functions + * @virtio_features: Negotiated virtio feature bits + * @vhost: Enable vhost-kernel acceleration for pasta * @ifi4: Template interface for IPv4, -1: none, 0: IPv4 disabled * @ip4: IPv4 configuration * @dns_search: DNS search list @@ -202,6 +205,8 @@ struct ip6_ctx { * @no_udp: Disable UDP operation * @udp: Context for UDP protocol handler * @no_icmp: Disable ICMP operation + * @vq: Per-virtqueue eventfd descriptors for vhost-kernel + * ([0] is RX, [1] is TX; each holds kick_fd, call_fd, err_fd) * @mtu: MTU passed via DHCP/NDP * @no_dns: Do not source/use DNS servers for any purpose * @no_dns_search: Do not source/use domain search lists for any purpose @@ -258,11 +263,14 @@ struct ctx { int fd_control; int fd_repair_listen; int fd_repair; + + int fd_vhost; unsigned char our_tap_mac[ETH_ALEN]; unsigned char guest_mac[ETH_ALEN]; uint16_t mtu;

uint64_t hash_secret[2]; + uint64_t virtio_features;

int ifi4; struct ip4_ctx ip4; @@ -288,6 +296,12 @@ struct ctx { struct udp_ctx udp; int no_icmp;

+ struct { + int kick_fd; + int call_fd; + int err_fd; + } vq[2]; + int no_dns; int no_dns_search; int no_dhcp_dns; @@ -300,6 +314,7 @@ struct ctx { int splice_only; int host_lo_to_ns_lo; int freebind; + int vhost; bool chroot_fallback;

int low_wmem;

It might be a bit neater to put the new vhost-kernel related fields into a substructure, rather than spreading them across struct ctx. At some point it would be nice to make the various tap backends a bit more pluggable / independent from each other. Doing that's obviously not in scope for this series, but keeping it's internal data all together will at least not make that job harder in the future.

-- 2.34.1

-- 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

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

On Sun, Aug 02, 2026 at 01:21:54PM +0000, Ammar Yasser wrote:

- tap_send_single was changed to directly call tap_send_frames_passt/pasta instead of relying on tap_send_frames because protocols that use tap_send_single won't use vhost acceleration, only tcp and udp will. The function was also moved lower in the file to by the time its called tap_send_frames_* functions are defined and a forward declaration isn't needed

Sorry, I'm not really following why the vhost change requires tap_send_single() to bypass tap_send_frames().

- extend udp_meta_t to instead of always carrying a tap_hdr, to carry a union of a tap_hdr and a virtio_net header because if vhost is used the will the latter type of header and then build an iov from this virtio net header in udp.c

In fact, the intended role of tap_hdr itself was to be a union of whatever "below L2" headers we might need for all our backends. It was never very obvious because there was just the vnet_len (for the qemu -net socket protocol) and nothing at all (for tuntap via the chardev). With vhost-user, in theory it should have gained a virtio_net_mrg_rxbuf branch, but the vhost-user paths are different enough that we never quite needed it. Here for vhost-kernel, though, we should add the virtio_net header inside tap_hdr, rather than making a union including tap_hdr. tap_hdr_iov() and tap_hdr_update() should be updated to handle that case as well.

- create iov_from_virtio_net_hdr to build an iov from the vnet header - in the case that vhost is requested, set the first frame fragment to be a virtio_net header through calling iov_from_virtio_net_hdr - convert the tcp_payload_tap_hdr array to be an array of virtio_net header. in the case where we aren't using vhost, we will just be using the first 4 bytes of it - add a boolean argumen to tap_send_frames_pasta to determine if vhost will be used

Signed-off-by: Ammar Yasser --- tap.c | 100 +++++++++++++++++++++++++++++++----------------------- tcp_buf.c | 39 +++++++++++++++++---- tcp_buf.h | 4 +-- udp.c | 28 ++++++++++++++- udp.h | 6 +++- 5 files changed, 125 insertions(+), 52 deletions(-)

diff --git a/tap.c b/tap.c index 9d601a2..e177eef 100644 --- a/tap.c +++ b/tap.c @@ -124,45 +124,6 @@ unsigned long tap_l2_max_len(const struct ctx *c) return 0; /* Unreachable, for cppcheck's sake */ }

-/** - * tap_send_single() - Send a single frame - * @c: Execution context - * @data: Packet buffer - * @l2len: Total L2 packet length - */ -void tap_send_single(const struct ctx *c, const void *data, size_t l2len) -{ - uint8_t padded[ETH_ZLEN] = { 0 }; - struct iovec iov[2]; - size_t iovcnt = 0; - uint32_t vnet_len; - - if (l2len < ETH_ZLEN) { - memcpy(padded, data, l2len); - data = padded; - l2len = ETH_ZLEN; - } - - vnet_len = htonl(l2len); - - switch (c->mode) { - case MODE_PASST: - iov[iovcnt] = IOV_OF_LVALUE(vnet_len); - iovcnt++; - /* fall through */ - case MODE_PASTA: - iov[iovcnt].iov_base = (void *)data; - iov[iovcnt].iov_len = l2len; - iovcnt++; - - tap_send_frames(c, iov, iovcnt, 1); - break; - case MODE_VU: - vu_send_single(c, data, l2len); - break; - } -} - /** * tap_push_l2h() - Build an L2 header for an inbound packet * @c: Execution context @@ -415,7 +376,7 @@ void tap_icmp6_send(const struct ctx *c, */ static size_t tap_send_frames_pasta(const struct ctx *c, const struct iovec *iov, - size_t bufs_per_frame, size_t nframes) + size_t bufs_per_frame, size_t nframes, bool vhost) { size_t nbufs = bufs_per_frame * nframes; size_t i; @@ -498,6 +459,59 @@ static size_t tap_send_frames_passt(const struct ctx *c, return i / bufs_per_frame; }

+/** + * tap_send_single() - Send a single frame + * @c: Execution context + * @data: Packet buffer + * @l2len: Total L2 packet length + */ +void tap_send_single(const struct ctx *c, const void *data, size_t l2len) +{ + uint8_t padded[ETH_ZLEN] = { 0 }; + struct iovec iov[2]; + size_t iovcnt = 0; + int m = 0; + uint32_t vnet_len; + + if (l2len < ETH_ZLEN) { + memcpy(padded, data, l2len); + data = padded; + l2len = ETH_ZLEN; + } + + vnet_len = htonl(l2len); + switch (c->mode) { + case MODE_PASST: + /* create an iov for the length */ + iov[iovcnt] = IOV_OF_LVALUE(vnet_len); + iovcnt++; + /* create the data iov */ + iov[iovcnt].iov_base = (void *)data; + iov[iovcnt].iov_len = l2len; + iovcnt++; + m = tap_send_frames_passt(c, iov, iovcnt, 1); + break; + case MODE_PASTA: + /* don't create a length iov in the case of pasta */ + iov[iovcnt].iov_base = (void *)data; + iov[iovcnt].iov_len = l2len; + iovcnt++; + + m = tap_send_frames_pasta(c, iov, iovcnt, 1, false);

You explicitly disable vhost here, even if available. As I've said before, tap_send_single() is a slow path that doesn't really need vhost acceleration. However, there's also not really a reason *not* to use vhost unless it simplifies things. So far this seems to be adding (slightly) complexity to avoid using vhost, and it's not clear to me if there's a simplification elsewhere that makes it worth it.

+ break; + case MODE_VU: + m = vu_send_single(c, data, l2len); + break; + } + + if (m < 1) + debug("tap: failed to send a single frame"); + + pcap_multiple(iov, iovcnt, m, + c->mode == MODE_PASST ? sizeof(uint32_t) : + (c->fd_vhost != -1) ? VNET_HLEN : 0); +} + /** * tap_send_frames() - Send out multiple prepared frames * @c: Execution context @@ -523,7 +537,8 @@ size_t tap_send_frames(const struct ctx *c, const struct iovec *iov,

switch (c->mode) { case MODE_PASTA: - m = tap_send_frames_pasta(c, iov, bufs_per_frame, nframes); + /* use vhost in pasta sending only if the vhost setup was actually successful, indicated by a non-zero fd_vhost */ + m = tap_send_frames_pasta(c, iov, bufs_per_frame, nframes, ((c->fd_vhost != -1))); break; case MODE_PASST: m = tap_send_frames_passt(c, iov, bufs_per_frame, nframes); @@ -539,7 +554,8 @@ size_t tap_send_frames(const struct ctx *c, const struct iovec *iov, nframes - m, nframes);

pcap_multiple(iov, bufs_per_frame, m, - c->mode == MODE_PASST ? sizeof(uint32_t) : 0); + c->mode == MODE_PASST ? sizeof(uint32_t) : + (c->fd_vhost != -1) ? VNET_HLEN : 0);

Rather than this nested ?: expression we should make a helper that gives the necessary length to exclude tap_hdr. tap_hdr_iov() should then use the same helper.

return m; } diff --git a/tcp_buf.c b/tcp_buf.c index 4452337..080e4cb 100644 --- a/tcp_buf.c +++ b/tcp_buf.c @@ -21,6 +21,7 @@ #include

#include +#include

#include "util.h" #include "ip.h" @@ -38,7 +39,7 @@ /* Ethernet header for IPv4 and IPv6 frames */ struct ethhdr tcp_eth_hdr[TCP_FRAMES_MEM];

-struct tap_hdr tcp_payload_tap_hdr[TCP_FRAMES_MEM]; +struct virtio_net_hdr_mrg_rxbuf tcp_payload_tap_hdr[TCP_FRAMES_MEM];

/* IP headers for IPv4 and IPv6 */ struct iphdr tcp4_payload_ip[TCP_FRAMES_MEM]; @@ -71,6 +72,14 @@ void tcp_update_l2_buf(const unsigned char *eth_d) eth_update_mac(&tcp_eth_hdr[i], eth_d, NULL); }

+static inline struct iovec iov_from_virtio_net_hdr(struct virtio_net_hdr_mrg_rxbuf *hdr) +{ + return (struct iovec){ + .iov_base = hdr, + .iov_len = sizeof(*hdr), + }; +}

As noted, this should become a new branch of tap_hdr_iov()...

/** * tcp_sock_iov_init() - Initialise scatter-gather L2 buffers for IPv4 sockets * @c: Execution context @@ -89,7 +98,14 @@ void tcp_sock_iov_init(const struct ctx *c) for (i = 0; i < TCP_FRAMES_MEM; i++) { struct iovec *iov = tcp_l2_iov[i];

- iov[TCP_IOV_TAP] = tap_hdr_iov(c, &tcp_payload_tap_hdr[i]); + /* If we are using pasta with vhost acceleration, the first entry in the tcp buffers + * should point to a virtio_net header, otherwise a tap header + */ + if ((c->fd_vhost != -1)) + iov[TCP_IOV_TAP] = iov_from_virtio_net_hdr(&tcp_payload_tap_hdr[i]); + else + iov[TCP_IOV_TAP] = tap_hdr_iov(c, (struct tap_hdr *)&tcp_payload_tap_hdr[i]);

..absorbing this if.

iov[TCP_IOV_ETH].iov_len = sizeof(struct ethhdr); iov[TCP_IOV_PAYLOAD].iov_base = &tcp_payload[i]; iov[TCP_IOV_ETH_PAD].iov_base = eth_pad; @@ -135,8 +151,7 @@ void tcp_payload_flush(const struct ctx *c, const struct timespec *now) { size_t m;

- m = tap_send_frames(c, &tcp_l2_iov[0][0], TCP_NUM_IOVS, - tcp_payload_used); + m = tap_send_frames(c, &tcp_l2_iov[0][0], TCP_NUM_IOVS, tcp_payload_used); if (m != tcp_payload_used) { tcp_revert_seq(c, &tcp_frame_conns[m], &tcp_l2_iov[m], tcp_payload_used - m, now); @@ -177,7 +192,7 @@ static void tcp_l2_buf_fill_headers(const struct ctx *c, { struct iov_tail tail = IOV_TAIL(&iov[TCP_IOV_PAYLOAD], 1, 0); struct tcphdr th_storage, *th = IOV_REMOVE_HEADER(&tail, th_storage); - struct tap_hdr *taph = iov[TCP_IOV_TAP].iov_base; + const struct flowside *tapside = TAPFLOW(conn); const struct in_addr *a4 = inany_v4(&tapside->oaddr); struct ethhdr *eh = iov[TCP_IOV_ETH].iov_base; @@ -192,7 +207,19 @@ static void tcp_l2_buf_fill_headers(const struct ctx *c,

l2len = tcp_fill_headers(c, conn, eh, ip4h, ip6h, th, &tail, iov_tail_size(&tail), csum_flags, seq); - tap_hdr_update(taph, l2len); + + /* when in pasta mode, the length of the tap header iov is zero, so this + * l2len write doesn't do anything. But this function gets called from both + * pasta and passt. Make the l2len write in case we are in passt mode, denoted by + * fd_vhost being -1 since this field won't get initialized at all in passt. + */ + if (c->mode == MODE_PASST) { + iov[TCP_IOV_TAP].iov_len = sizeof(struct tap_hdr); + struct tap_hdr *taph = iov[TCP_IOV_TAP].iov_base; + tap_hdr_update(taph, l2len); + } else if (c->fd_vhost == -1) { /* pasta mode but without vhost */ + iov[TCP_IOV_TAP].iov_len = 0; + }

Similarly this if should go inside tap_hdr_update().

}

/** diff --git a/tcp_buf.h b/tcp_buf.h index c749038..da48cd0 100644 --- a/tcp_buf.h +++ b/tcp_buf.h @@ -9,7 +9,7 @@ #include "tcp_conn.h" #include "tcp_internal.h"

-void tcp_sock_iov_init(); +void tcp_sock_iov_init(const struct ctx *c); void tcp_payload_flush(const struct ctx *c, const struct timespec *now); int tcp_buf_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn, uint32_t already_sent, const struct timespec *now); @@ -20,7 +20,7 @@ int tcp_buf_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags, #define TCP_FRAMES \ (c->mode == MODE_PASTA ? 1 : TCP_FRAMES_MEM)

-extern struct tap_hdr tcp_payload_tap_hdr[TCP_FRAMES_MEM]; +extern struct virtio_net_hdr_mrg_rxbuf tcp_payload_tap_hdr[TCP_FRAMES_MEM]; extern struct ethhdr tcp_eth_hdr[TCP_FRAMES_MEM]; extern struct tcp_payload_t tcp_payload[TCP_FRAMES_MEM];

diff --git a/udp.c b/udp.c index d05ee66..5f90dce 100644 --- a/udp.c +++ b/udp.c @@ -103,6 +103,7 @@ #include #include #include +#include

#include "checksum.h" #include "util.h" @@ -119,6 +120,18 @@ #include "udp_vu.h" #include "epoll_ctl.h"

+/* UDP header and data for inbound messages */ +struct udp_payload_t udp_payload[UDP_MAX_FRAMES]; + +/* Ethernet headers for IPv4 and IPv6 frames */ +struct ethhdr udp_eth_hdr[UDP_MAX_FRAMES]; + +/* IOVs and msghdr arrays for receiving datagrams from sockets */ +struct iovec udp_iov_recv [UDP_MAX_FRAMES]; +struct mmsghdr udp_mh_recv [UDP_MAX_FRAMES]; + +/* Pre-cooked headers for UDP packets */ +struct udp_meta_t udp_meta[UDP_MAX_FRAMES];

I'm slightly confused. I see these added, but I don't see the existing static declarations removed. It's also not clear to me why these need to become non-static.

#define UDP_TIMEOUT "/proc/sys/net/netfilter/nf_conntrack_udp_timeout" #define UDP_TIMEOUT_STREAM \ @@ -216,7 +229,20 @@ static void udp_iov_init_one(const struct ctx *c, size_t i) *siov = IOV_OF_LVALUE(payload->data);

tiov[UDP_IOV_ETH] = IOV_OF_LVALUE(udp_eth_hdr[i]); - tiov[UDP_IOV_TAP] = tap_hdr_iov(c, &meta->taph); + /* if the vhost tap fd is initialized, this is a sign for us that + * we will be using virtio transport. make the iov that is supposed + * to point to a tap header point instead to a virtio_net_mrg_rxbuf + */ + if (c->fd_vhost != -1) { + struct iovec vnet_iov = { + .iov_base = (void *)(&meta->vnet_hdr), + .iov_len = sizeof(meta->vnet_hdr) + }; + tiov[UDP_IOV_TAP] = vnet_iov; + } else { + tiov[UDP_IOV_TAP] = tap_hdr_iov(c, &meta->taph); + }

Again, this logic should go inside tap_hdr_iov().

+ tiov[UDP_IOV_PAYLOAD].iov_base = payload; tiov[UDP_IOV_ETH_PAD].iov_base = eth_pad;

diff --git a/udp.h b/udp.h index b7a367c..3464b74 100644 --- a/udp.h +++ b/udp.h @@ -9,6 +9,7 @@ #include #include #include +#include

#include "tap_hdr.h" #include "fwd.h" @@ -49,7 +50,10 @@ extern struct mmsghdr udp_mh_recv [UDP_MAX_FRAMES]; struct udp_meta_t { struct ipv6hdr ip6h; struct iphdr ip4h; - struct tap_hdr taph; + union { + struct virtio_net_hdr_mrg_rxbuf vnet_hdr; + struct tap_hdr taph; +}; #ifdef __AVX2__ } __attribute__((aligned(32))); #else -- 2.34.1

-- 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

On Mon Aug 10, 2026 at 5:06 AM EEST, David Gibson wrote:

...

/* Large enough for ~128 maximum size frames */ -#define PKT_BUF_BYTES (8UL << 20) +#define PKT_BUF_BYTES ((8UL << 20) + 1536) /* 128 * sizeof(virtio_net_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 ~= 8MiB

So, I'm not sure if this change is necessary - if it really is, we need a clearer analysis of why.

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. Will include this rationale in the comment in the coming revision

...

extern char pkt_buf [PKT_BUF_BYTES];

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

+#include #include +#include #include

+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.

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

...

/* Maximum size of a virtqueue */ #define VIRTQUEUE_MAX_SIZE 1024

#define VNET_HLEN (sizeof(struct virtio_net_hdr_mrg_rxbuf))

+/* Keep in sync with PKT_BUF_BYTES in passt.h */ +#define PKT_BUF_BYTES ((8UL << 20) + 1536) /* 128 * sizeof(virtio_net_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.

Ok

...

+#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).

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 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

...

+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.

Noted

...

+ +#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_memory) +#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_vring_file)

It's probably preferable to #include rather than restating these.

Ok

...

+/** + * 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?

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 ?

...

+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; };

+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.

Sure. Will combine with the implementation commit

...

#endif /* VIRTIO_H */ -- 2.34.1

On Mon Aug 10, 2026 at 10:34 AM EEST, David Gibson wrote:

On Sun, Aug 02, 2026 at 01:21:53PM +0000, Ammar Yasser wrote:

...

Add a new function called tap_vhost_input that will be used to indicate that the guest is trying to send data to us through the rx queue and the kernel is informing us to handle this data.

We will however still recieve epoll events on the fd normally. We should

"the fd" is not very clear here, there are a bunch of fds. I'm guessing you mean the character device fd?

Yeah. And tying it back to your comment about the fact that we shouldn't call epoll add with the device fd if the user requested vhost means that this comment will be redundant anyways, will delete it. For the record, while debugging performance, the fact that we received a double kick on the device fd and the queue fd was a major slow down reason. I did remove epoll registration for the device in this experimental branch. Will include it in my next revision.

...

@@ -1359,7 +1362,8 @@ void tap_handler_pasta(struct ctx *c, uint32_t events, if (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR)) die("Disconnect event on /dev/net/tun device, exiting");

- if (events & EPOLLIN) + /* don't proceed with the normal tap processing in case vhost acceleration was required */ + if (events & EPOLLIN && !c->vhost)

Seems like it would be more elegant to remove the character device fd from the epoll set (or at least remove EPOLLIN) when vhost is activated, rather than take it's events then ignore the.

Yeah, agreed as per my comment above.

...

tap_pasta_input(c, now); }

@@ -1514,6 +1518,90 @@ void tap_listen_handler(struct ctx *c, uint32_t events) tap_start_connection(c); }

+/** + * consume_one_rx_descriptor() - Consume one used RX descriptor from the kernel

I'd suggest avoiding "Rx" or "Tx" terminology throughout, using "toguest" or "fromguest" instead. Note that for extra confusion Rx here is meaning "fromguest", but for vhost-user it means "toguest".

Noted

...

+ * @len: Set to the length of data written by the kernel + * + * Pops a single entry from the used ring. Advances vqs[0].last_used_idx + * (the number of entries we have consumed) and vqs[0].num_free (the count + * of descriptors awaiting refill announcement). + * + * NOTE: This function assumes the kernel is going to post single descriptors + * always, No chains. If that changes, we would need to increment num_free + * as we advertise back to the kernel the free descriptors by the length of the chain. + * + * Return: pointer to the packet buffer, or NULL if no data is available + */ +static void *consume_one_rx_descriptor(unsigned *len)

For a length of data, we'd typically use a size_t rather than a bare unsigned.

Noted

...

+ /* read the last index we consumed */ + last_used = vqs[0].last_used_idx % VHOST_NDESCS; + /* read the index of what the */ + i = le32toh(used->ring[last_used].id); + *len = le32toh(used->ring[last_used].len); + + if (i != last_used) { + die("vhost: id %u at used position %u != %u", i, last_used, i); + }

Braces not necessary for single line if blocks.

Ok

...

+ + /* the kernel has queued for us something we cannot receive */ + if (*len > PKT_BUF_BYTES/VHOST_NDESCS) {

This seems oddly circular: deriving the maximum frame size from the buffer and number of descriptors, when the number of descriptors was derived from the buffer size and maximum frame size. Better to use (or add) an L2_MAX_LEN_XX constant.

Will do

...

+ die("vhost: id %d len %u > %zu", i, *len, PKT_BUF_BYTES/VHOST_NDESCS); + } + + vqs[0].last_used_idx++; + vqs[0].num_free++; + return pkt_buf + i * (PKT_BUF_BYTES/VHOST_NDESCS); +} + + +/** + * tap_vhost_input() - Handler for new data on the tun socket to hypervisor vq + * @c: Execution context + * @ref: epoll reference + * @now: Current timestamp + */ +void tap_vhost_input(struct ctx *c, union epoll_ref ref, const struct timespec *now) +{ + eventfd_read(ref.fd, (eventfd_t[]){ 0 }); + + tap_flush_pools(); + + struct virtio_net_hdr_mrg_rxbuf *hdr; + struct iov_tail data; + unsigned len;

My convention, we don't use inline declarations, even though we could in C11 - move these up to the top.

...

+ + while ((hdr = consume_one_rx_descriptor(&len))) { + if (len < sizeof(*hdr)) {

I think you want to check that at least an ethernet (L2) header is there as well as the virtio_net_hdr_mrg_rxbuf: both tap_pasta_input() and tap_passt_input() appear to check for this.

Sure, i can add that. But just for my knowledge is there any case where there can be a virtio_net header without an ethernet header ?

...

+ warn("vhost: invalid len %u", len); + continue; + } + + /* skip over the vnet header, we wanna add the packet without it*/ + data = IOV_TAIL_FROM_BUF((void *)(hdr+1), len - sizeof(*hdr), 0); + tap_add_packet(c, &data, now); + } + + tap_handler(c, now); + rx_descriptor_handoff(c); +} + /** * tap_ns_tun() - Get tuntap fd in namespace * @c: Execution context @@ -1524,16 +1612,15 @@ void tap_listen_handler(struct ctx *c, uint32_t events) */ static int tap_ns_tun(void *arg) { - struct ifreq ifr = { .ifr_flags = IFF_TAP | IFF_NO_PI }; - int flags = O_RDWR | O_NONBLOCK | O_CLOEXEC; struct ctx *c = (struct ctx *)arg; + struct ifreq ifr = { .ifr_flags = IFF_TAP | IFF_NO_PI }; int fd, rc;

c->fd_tap = -1; memcpy(ifr.ifr_name, c->pasta_ifn, IFNAMSIZ); ns_enter(c);

- fd = open("/dev/net/tun", flags); + fd = open("/dev/net/tun", O_RDWR | O_NONBLOCK | O_CLOEXEC);

This change seems unrelated to the actual aim of this patch.

Woops, removing

...

if (fd < 0) die_perror("Failed to open() /dev/net/tun");

@@ -1561,6 +1648,20 @@ static void tap_sock_tun_init(struct ctx *c) die("Failed to set up tap device in namespace"); }

+ /* initialize the vhost-net dev file descriptor */ + if (c->vhost) { + setup_vhost_net(c); + + for (int i = 0; i < ARRAY_SIZE(c->vq); i++) + setup_eventfds(c, i); + + if (setup_memory_table(c) < 0) + die_perror("VHOST_SET_MEM_TABLE ioctl on /dev/vhost-net failed"); + + for (int i = 0; i < ARRAY_SIZE(c->vq); i++) + set_vring_for_queue(c, i, c->fd_tap); + }

It's not clear to me if this code block needs to execute within the guest namespace. If not, it should probably be in tap_sock_tun_init() rather than tap_ns_tun(). If it _does_ have to be in tap_ns_tun(), then tap_ns_tun() probably needs a rename, since it's now doing rather more in the vhost case that just getting a device fd.

Not sure i follow. its already in tap_sock_tun_init not tap_ns_tun. But generally speaking it needs to be after tap_ns_tun. because it relies on the device fd that tap_ns_tun has created in the namespace

...

+ pasta_ns_conf(c);

if (!c->splice_only) diff --git a/tap.h b/tap.h index 1625975..eb02da8 100644 --- a/tap.h +++ b/tap.h @@ -66,6 +66,7 @@ static inline void tap_hdr_update(struct tap_hdr *thdr, size_t l2len) thdr->vnet_len = htonl(l2len); }

+void tap_vhost_input(struct ctx *c, union epoll_ref ref, const struct timespec *now); unsigned long tap_l2_max_len(const struct ctx *c); void *tap_push_l2h(const struct ctx *c, void *buf, const void *src_mac, uint16_t proto); -- 2.34.1

On Mon Aug 10, 2026 at 12:18 PM EEST, David Gibson wrote:

On Sun, Aug 02, 2026 at 01:21:55PM +0000, Ammar Yasser wrote:

...

Callers will specify whether vhost should be used or no by passing the vhost argument to tap_send_frames_pasta. If specified, sending will go through a function called tap_send_frames_vhost, which pops a descriptor from the queue shared with the kernel and sets the address of that descriptor to be the base address of a single iov from the group of buffers_per_frame * nframes iovs we pass to the function

Signed-off-by: Ammar Yasser

Sorry, didn't spot this earlier in the series: if these are based on Eugenio's earlier patches, they should have his Signed-off-by in addition to your own.

They have significantly diverged in both order and structure so there's no clear mapping between mine and his anymore. if there doesn't need to be a mapping i'm happy to add his Signed-off-by. Otherwise, i did mention his original series in the cover letter

...

--- tap.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 107 insertions(+), 1 deletion(-)

diff --git a/tap.c b/tap.c index e177eef..66bb71d 100644 --- a/tap.c +++ b/tap.c @@ -361,7 +361,32 @@ void tap_icmp6_send(const struct ctx *c, }

/** - * tap_send_frames_pasta() - Send multiple frames to the pasta tap + * tx_reap() - Reclaim the descriptors the kernel has already processed + */ +static void tx_reap(void) { + struct vring_used *used = &vring_used_all[1].used; + uint16_t used_idx = le16toh(used->idx); + + smp_rmb(); + + /* increment last_used_idx until it reaches the kernel's used index */ + while (vqs[1].last_used_idx != used_idx) { + uint16_t desc_id = le32toh(used->ring[vqs[1].last_used_idx % VHOST_NDESCS].id); + + for (;;) { + /* keep going until we find a descriptor without the next flag */ + vqs[1].num_free++; + if (!(le16toh(vring_desc[1][desc_id].flags) & VRING_DESC_F_NEXT)) + break; + /* this descriptor wasn't the last, set desc_id to the next one and keep going */ + desc_id = le16toh(vring_desc[1][desc_id].next); + } + vqs[1].last_used_idx++; + } +} + +/** + * tap_send_frames_vhost() - Send multiple frames to the pasta tap * @c: Execution context * @iov: Array of buffers * @bufs_per_frame: Number of buffers (iovec entries) per frame @@ -371,6 +396,84 @@ void tap_icmp6_send(const struct ctx *c, * @bufs_per_frame contiguous buffers representing a single frame. * * Return: number of frames successfully sent + */ +static size_t tap_send_frames_vhost(const struct ctx *c, + const struct iovec *iov, + size_t bufs_per_frame, size_t nframes) +{ + size_t i; + size_t processed_frames = 0;

Reverse christmas tree.

Noted

...

+ + /* update our local counters first */ + tx_reap(); + + #define AVAIL_Q(i)(vring_avail_all[i].avail)

# for preprocessor directives should always go in column 0.

Ok

...

+ + for (i = 0; i < nframes; i++) { + size_t j; + + if (vqs[1].num_free < bufs_per_frame) + break;

IIRC, we were discussing during the last call that at least for the first version it's fine if we synchronously wait for buffers to be available. Since then it occurred to me that another acceptable option would be to simply drop frames if there are no available buffers (this is kind of how IP is designed to work - congestion is reported implicitly as packet loss). Do whichever is easier for now and it can be refined later.

Will do

...

+ + /* set the index of the avail ring in the tx queue to be our last_used_idx */ + uint16_t head = vqs[1].next_free % VHOST_NDESCS;

No inline decls.

Ok

On Wed, Aug 12, 2026 at 09:17:30PM +0300, Ammar Yasser wrote:

On Mon Aug 10, 2026 at 10:34 AM EEST, David Gibson wrote:

...

On Sun, Aug 02, 2026 at 01:21:53PM +0000, Ammar Yasser wrote:

...

Add a new function called tap_vhost_input that will be used to indicate that the guest is trying to send data to us through the rx queue and the kernel is informing us to handle this data.

We will however still recieve epoll events on the fd normally. We should

"the fd" is not very clear here, there are a bunch of fds. I'm guessing you mean the character device fd?

Yeah. And tying it back to your comment about the fact that we shouldn't call epoll add with the device fd if the user requested vhost means that this comment will be redundant anyways, will delete it.

For the record, while debugging performance, the fact that we received a double kick on the device fd and the queue fd was a major slow down reason. I did remove epoll registration for the device in this experimental branch. Will include it in my next revision.

Oh, excellent :).

...

...

@@ -1359,7 +1362,8 @@ void tap_handler_pasta(struct ctx *c, uint32_t events, if (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR)) die("Disconnect event on /dev/net/tun device, exiting");

- if (events & EPOLLIN) + /* don't proceed with the normal tap processing in case vhost acceleration was required */ + if (events & EPOLLIN && !c->vhost)

Seems like it would be more elegant to remove the character device fd from the epoll set (or at least remove EPOLLIN) when vhost is activated, rather than take it's events then ignore the.

Yeah, agreed as per my comment above.

...

...

tap_pasta_input(c, now); }

@@ -1514,6 +1518,90 @@ void tap_listen_handler(struct ctx *c, uint32_t events) tap_start_connection(c); }

+/** + * consume_one_rx_descriptor() - Consume one used RX descriptor from the kernel

I'd suggest avoiding "Rx" or "Tx" terminology throughout, using "toguest" or "fromguest" instead. Note that for extra confusion Rx here is meaning "fromguest", but for vhost-user it means "toguest".

Noted

...

...

+ * @len: Set to the length of data written by the kernel + * + * Pops a single entry from the used ring. Advances vqs[0].last_used_idx + * (the number of entries we have consumed) and vqs[0].num_free (the count + * of descriptors awaiting refill announcement). + * + * NOTE: This function assumes the kernel is going to post single descriptors + * always, No chains. If that changes, we would need to increment num_free + * as we advertise back to the kernel the free descriptors by the length of the chain. + * + * Return: pointer to the packet buffer, or NULL if no data is available + */ +static void *consume_one_rx_descriptor(unsigned *len)

For a length of data, we'd typically use a size_t rather than a bare unsigned.

Noted

...

...

+ /* read the last index we consumed */ + last_used = vqs[0].last_used_idx % VHOST_NDESCS; + /* read the index of what the */ + i = le32toh(used->ring[last_used].id); + *len = le32toh(used->ring[last_used].len); + + if (i != last_used) { + die("vhost: id %u at used position %u != %u", i, last_used, i); + }

Braces not necessary for single line if blocks.

Ok

...

...

+ + /* the kernel has queued for us something we cannot receive */ + if (*len > PKT_BUF_BYTES/VHOST_NDESCS) {

This seems oddly circular: deriving the maximum frame size from the buffer and number of descriptors, when the number of descriptors was derived from the buffer size and maximum frame size. Better to use (or add) an L2_MAX_LEN_XX constant.

Will do

...

...

+ die("vhost: id %d len %u > %zu", i, *len, PKT_BUF_BYTES/VHOST_NDESCS); + } + + vqs[0].last_used_idx++; + vqs[0].num_free++; + return pkt_buf + i * (PKT_BUF_BYTES/VHOST_NDESCS); +} + + +/** + * tap_vhost_input() - Handler for new data on the tun socket to hypervisor vq + * @c: Execution context + * @ref: epoll reference + * @now: Current timestamp + */ +void tap_vhost_input(struct ctx *c, union epoll_ref ref, const struct timespec *now) +{ + eventfd_read(ref.fd, (eventfd_t[]){ 0 }); + + tap_flush_pools(); + + struct virtio_net_hdr_mrg_rxbuf *hdr; + struct iov_tail data; + unsigned len;

My convention, we don't use inline declarations, even though we could in C11 - move these up to the top.

...

+ + while ((hdr = consume_one_rx_descriptor(&len))) { + if (len < sizeof(*hdr)) {

I think you want to check that at least an ethernet (L2) header is there as well as the virtio_net_hdr_mrg_rxbuf: both tap_pasta_input() and tap_passt_input() appear to check for this.

Sure, i can add that. But just for my knowledge is there any case where there can be a virtio_net header without an ethernet header ?

Maybe. It might be possible to pass in tap device in with --fd, but have it misconfigured as a "tun" device (raw IP) instead of "tap" (virtual ethernet). Not sure if that's currently possible to combine with vhost-kernel, but it's close enough to possible that it makes sense to be cautious. I'm also not sure what would happen if the guest used an AF_PACKET socket to send malformed Ethernet frames. There's a good chance it would get filtered before reaching us, but still, better to check.

...

...

+ warn("vhost: invalid len %u", len); + continue; + } + + /* skip over the vnet header, we wanna add the packet without it*/ + data = IOV_TAIL_FROM_BUF((void *)(hdr+1), len - sizeof(*hdr), 0); + tap_add_packet(c, &data, now); + } + + tap_handler(c, now); + rx_descriptor_handoff(c); +} + /** * tap_ns_tun() - Get tuntap fd in namespace * @c: Execution context @@ -1524,16 +1612,15 @@ void tap_listen_handler(struct ctx *c, uint32_t events) */ static int tap_ns_tun(void *arg) { - struct ifreq ifr = { .ifr_flags = IFF_TAP | IFF_NO_PI }; - int flags = O_RDWR | O_NONBLOCK | O_CLOEXEC; struct ctx *c = (struct ctx *)arg; + struct ifreq ifr = { .ifr_flags = IFF_TAP | IFF_NO_PI }; int fd, rc;

c->fd_tap = -1; memcpy(ifr.ifr_name, c->pasta_ifn, IFNAMSIZ); ns_enter(c);

- fd = open("/dev/net/tun", flags); + fd = open("/dev/net/tun", O_RDWR | O_NONBLOCK | O_CLOEXEC);

This change seems unrelated to the actual aim of this patch.

Woops, removing

...

...

if (fd < 0) die_perror("Failed to open() /dev/net/tun");

@@ -1561,6 +1648,20 @@ static void tap_sock_tun_init(struct ctx *c) die("Failed to set up tap device in namespace"); }

+ /* initialize the vhost-net dev file descriptor */ + if (c->vhost) { + setup_vhost_net(c); + + for (int i = 0; i < ARRAY_SIZE(c->vq); i++) + setup_eventfds(c, i); + + if (setup_memory_table(c) < 0) + die_perror("VHOST_SET_MEM_TABLE ioctl on /dev/vhost-net failed"); + + for (int i = 0; i < ARRAY_SIZE(c->vq); i++) + set_vring_for_queue(c, i, c->fd_tap); + }

It's not clear to me if this code block needs to execute within the guest namespace. If not, it should probably be in tap_sock_tun_init() rather than tap_ns_tun(). If it _does_ have to be in tap_ns_tun(), then tap_ns_tun() probably needs a rename, since it's now doing rather more in the vhost case that just getting a device fd.

Not sure i follow. its already in tap_sock_tun_init not tap_ns_tun. But generally speaking it needs to be after tap_ns_tun. because it relies on the device fd that tap_ns_tun has created in the namespace

Oh, sorry, I misread the diff.

...

...

+ pasta_ns_conf(c);

if (!c->splice_only) diff --git a/tap.h b/tap.h index 1625975..eb02da8 100644 --- a/tap.h +++ b/tap.h @@ -66,6 +66,7 @@ static inline void tap_hdr_update(struct tap_hdr *thdr, size_t l2len) thdr->vnet_len = htonl(l2len); }

+void tap_vhost_input(struct ctx *c, union epoll_ref ref, const struct timespec *now); unsigned long tap_l2_max_len(const struct ctx *c); void *tap_push_l2h(const struct ctx *c, void *buf, const void *src_mac, uint16_t proto); -- 2.34.1

-- 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

On Thu Aug 13, 2026 at 5:01 AM EEST, David Gibson wrote:

On Wed, Aug 12, 2026 at 09:39:33PM +0300, Ammar Yasser wrote:

...

On Mon Aug 10, 2026 at 12:01 PM EEST, David Gibson wrote:

...

On Sun, Aug 02, 2026 at 01:21:54PM +0000, Ammar Yasser wrote:

...

- tap_send_single was changed to directly call tap_send_frames_passt/pasta instead of relying on tap_send_frames because protocols that use tap_send_single won't use vhost acceleration, only tcp and udp will. The function was also moved lower in the file to by the time its called tap_send_frames_* functions are defined and a forward declaration isn't needed

Sorry, I'm not really following why the vhost change requires tap_send_single() to bypass tap_send_frames().

In a previous review from you on the original work by Eugenio you said you don't want tap_send_frames to take a vhost boolean parameter because its a pasta only value to a function that is used by passt and pasta. If tap_send_single relies on tap_send_frames (assuming it takes no vhost bool), and deducing whether to use vhost or no is based on c->vhost then arp, icmp, ndp.. and other single send protos end up using vhost as well. which won't work since those protos don't have a concrete memory region i can make the kernel aware of.

Ah, I see. This is the key point the commit message needs to explain: some of the users of tap_send_single() *can't* use the vhost path, because of the shared buffers.

Got it. Will clarify this

...

And obvioiusly doing a protocol check inside tap_send_frames is not super cool either.

Let me know your opinion on this!

...

...

- extend udp_meta_t to instead of always carrying a tap_hdr, to carry a union of a tap_hdr and a virtio_net header because if vhost is used the will the latter type of header and then build an iov from this virtio net header in udp.c

In fact, the intended role of tap_hdr itself was to be a union of whatever "below L2" headers we might need for all our backends. It was never very obvious because there was just the vnet_len (for the qemu -net socket protocol) and nothing at all (for tuntap via the chardev). With vhost-user, in theory it should have gained a virtio_net_mrg_rxbuf branch, but the vhost-user paths are different enough that we never quite needed it.

Here for vhost-kernel, though, we should add the virtio_net header inside tap_hdr, rather than making a union including tap_hdr. tap_hdr_iov() and tap_hdr_update() should be updated to handle that case as well.

To make sure i understand, you're saying that tap_hdr itself should be a union of vnet_len and virtio_net_mrg_rxbuf ?

Yes.

...

i don't disagree in principal,

(English usage nit: in this context it's "principle", not "principal")

Haha sorry, typing fast is the culprit here. will take note in the future

...

...

...

+/** + * tap_send_single() - Send a single frame + * @c: Execution context + * @data: Packet buffer + * @l2len: Total L2 packet length + */ +void tap_send_single(const struct ctx *c, const void *data, size_t l2len) +{ + uint8_t padded[ETH_ZLEN] = { 0 }; + struct iovec iov[2]; + size_t iovcnt = 0; + int m = 0; + uint32_t vnet_len; + + if (l2len < ETH_ZLEN) { + memcpy(padded, data, l2len); + data = padded; + l2len = ETH_ZLEN; + } + + vnet_len = htonl(l2len); + switch (c->mode) { + case MODE_PASST: + /* create an iov for the length */ + iov[iovcnt] = IOV_OF_LVALUE(vnet_len); + iovcnt++; + /* create the data iov */ + iov[iovcnt].iov_base = (void *)data; + iov[iovcnt].iov_len = l2len; + iovcnt++; + m = tap_send_frames_passt(c, iov, iovcnt, 1); + break; + case MODE_PASTA: + /* don't create a length iov in the case of pasta */ + iov[iovcnt].iov_base = (void *)data; + iov[iovcnt].iov_len = l2len; + iovcnt++; + + m = tap_send_frames_pasta(c, iov, iovcnt, 1, false);

You explicitly disable vhost here, even if available. As I've said before, tap_send_single() is a slow path that doesn't really need vhost acceleration. However, there's also not really a reason *not* to use vhost unless it simplifies things. So far this seems to be adding (slightly) complexity to avoid using vhost, and it's not clear to me if there's a simplification elsewhere that makes it worth it.

As per above comment, its a correctness problem. single send protocols won't work with vhost in the current state, thats why i went the extra mile to avoid it. let me know if i am misunderstanding anything

Right, I missed that constraint. Might be worth putting it in a comment, since it's not necessarily obvious from this point in the code.

Will do

...

...

...

/** diff --git a/tcp_buf.h b/tcp_buf.h index c749038..da48cd0 100644 --- a/tcp_buf.h +++ b/tcp_buf.h @@ -9,7 +9,7 @@ #include "tcp_conn.h" #include "tcp_internal.h"

-void tcp_sock_iov_init(); +void tcp_sock_iov_init(const struct ctx *c); void tcp_payload_flush(const struct ctx *c, const struct timespec *now); int tcp_buf_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn, uint32_t already_sent, const struct timespec *now); @@ -20,7 +20,7 @@ int tcp_buf_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags, #define TCP_FRAMES \ (c->mode == MODE_PASTA ? 1 : TCP_FRAMES_MEM)

-extern struct tap_hdr tcp_payload_tap_hdr[TCP_FRAMES_MEM]; +extern struct virtio_net_hdr_mrg_rxbuf tcp_payload_tap_hdr[TCP_FRAMES_MEM]; extern struct ethhdr tcp_eth_hdr[TCP_FRAMES_MEM]; extern struct tcp_payload_t tcp_payload[TCP_FRAMES_MEM];

diff --git a/udp.c b/udp.c index d05ee66..5f90dce 100644 --- a/udp.c +++ b/udp.c @@ -103,6 +103,7 @@ #include #include #include +#include

#include "checksum.h" #include "util.h" @@ -119,6 +120,18 @@ #include "udp_vu.h" #include "epoll_ctl.h"

+/* UDP header and data for inbound messages */ +struct udp_payload_t udp_payload[UDP_MAX_FRAMES]; + +/* Ethernet headers for IPv4 and IPv6 frames */ +struct ethhdr udp_eth_hdr[UDP_MAX_FRAMES]; + +/* IOVs and msghdr arrays for receiving datagrams from sockets */ +struct iovec udp_iov_recv [UDP_MAX_FRAMES]; +struct mmsghdr udp_mh_recv [UDP_MAX_FRAMES]; + +/* Pre-cooked headers for UDP packets */ +struct udp_meta_t udp_meta[UDP_MAX_FRAMES];

I'm slightly confused. I see these added, but I don't see the existing static declarations removed. It's also not clear to me why these need to become non-static.

The static declarations were removed in the second patch. This addition is misplaced, will fix.

Ok.

...

But yeah they do need to be non static because of the need to share them with the kernel in virtio.c

Does that mean the *only* reference in virtio.c is just to get their address/size to put into the shared memory table? If that's the case delegating to a helper within udp.c to register them into the table seems like a better approach.

Ok. will do this in the next revision