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? > explicitly use either the vhost path of the tap_handler_pasta path and > so if vhost was requested the tap_handler_pasta path gets skipped. > > Add vhost bootstrapping to tap_sock_tun_init in case vhost was > requested. > > consume_one_rx_descriptor: fetch one descriptor from what the kernel has > advertised to us was written to, advance the tracking data structure to > reflect the last index we used, and the count of descriptors we are > ready to hand back to the kernel. return a void pointer to the start of > this descriptor's data > > tap_vhost_input: fetch descriptors using consume_one_rx_descriptor, > and skip over the vnet header, then build an iov tail from data and > queue it for processing, process the frames, and lastly, advertise to > the kernel that the descriptors are free to reuse > > Signed-off-by: Ammar Yasser > --- > passt.c | 6 ++++ > tap.c | 109 +++++++++++++++++++++++++++++++++++++++++++++++++++++--- > tap.h | 1 + > 3 files changed, 112 insertions(+), 4 deletions(-) > > diff --git a/passt.c b/passt.c > index 865b331..fdf9070 100644 > --- a/passt.c > +++ b/passt.c > @@ -305,6 +305,12 @@ static void passt_worker(void *opaque, int nfds, struct epoll_event *events) > case EPOLL_TYPE_CONF: > conf_handler(c, eventmask); > break; > + case EPOLL_TYPE_VHOST_CALL: > + tap_vhost_input(c, ref, &now); > + break; > + case EPOLL_TYPE_VHOST_ERROR: > + die("Error on vhost-kernel socket"); Eventually we probably want to try to reset and recover here, or even fall back to character device. die() is probably ok in the interim, though. > + break; > default: > /* Can't happen */ > assert(0); > diff --git a/tap.c b/tap.c > index dfa66c7..9d601a2 100644 > --- a/tap.c > +++ b/tap.c > @@ -13,6 +13,7 @@ > * > */ > > +#include "common.h" > #include > #include > #include > @@ -38,6 +39,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -61,6 +63,7 @@ > #include "vhost_user.h" > #include "vu_common.h" > #include "epoll_ctl.h" > +#include "virtio.h" > > /* Maximum allowed frame lengths (including L2 header) */ > > @@ -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. > 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". > + * @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. > +{ > + struct vring_used *used = &vring_used_all[0].used; > + uint32_t i; > + uint16_t used_idx, last_used; > + > + used_idx = le16toh(used->idx); > + > + smp_rmb(); > + > + /* if the kernel's last_used index matches our last_used_idx, then > + * we have finished consuming data. > + */ > + if (used_idx == vqs[0].last_used_idx) { > + *len = 0; > + return NULL; > + } > + > + /* 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. > + > + /* 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. > + 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. > + 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. > 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. > + > 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