Re: [RFC v3 5/8] virtio: Implement the pasta vhost functions

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 13408 bytes --]

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 <aerosound161@gmail.com>

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 <assert.h>
> +#include <limits.h>
>  #include <stddef.h>
>  #include <endian.h>
>  #include <string.h>
>  #include <errno.h>
>  #include <sys/eventfd.h>
> +#include <sys/ioctl.h>
>  #include <sys/socket.h>
> +#include <limits.h>
> +#include <stdint.h>
> +#include <fcntl.h>
> +#include <stdbool.h>
> +#include <stdlib.h>
> +#include <sys/eventfd.h>
> +#include <netinet/in.h>
>  
>  #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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]