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

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

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

On Wed, Aug 12, 2026 at 08:57:01PM +0300, Ammar Yasser wrote:
> > Similar to notes on the earlier patches, I think this will be clearer
> > with the new vhost-kernel related functions in new .c and .h files,
> > rather than mixed in with the general virtio helpers.
> 
> Yeah, thats the approach i will go with
> 
> >> +
> >> +
> >> +struct vq_state vqs[2];
> >
> > 'vqs' isn't really an adequate name for a global variable, especially
> > since this is fully global, not 'static'.
> 
> Noted
> 
> >> +/**
> >> + * setup_vhost_net() - Open and negotiate features on /dev/vhost-net
> >> + * @c:			Execution context; c->fd_vhost is set on success
> >> + *
> >> + */
> >> +void setup_vhost_net(struct ctx *c)
> >> +{
> >> +	static const uint64_t req_features =
> >> +		(1ULL << VIRTIO_F_VERSION_1) | (1ULL << VHOST_NET_F_VIRTIO_NET_HDR);
> >
> > Since it's also 'const' anyway, I'm not sure the 'static' does
> > anything useful.
> 
> Ok
> 
> >
> >> +	int vhost_fd, rc;
> >> +
> >> +	vhost_fd = open("/dev/vhost-net", O_RDWR | O_NONBLOCK | O_CLOEXEC);
> >> +	if (vhost_fd < 0)
> >> +		die_perror("Failed to open /dev/vhost-net");
> >
> > We probably want to be able to fall back to the /dev/net/tun character
> > device if vhost doesn't work.  So making this setup function fallible
> > would be preferable to using die() on errors.
> 
> This would be preferrable in the auto mode you described in an earlier
> message. But i think for an explicit request to vhost, we should fail.
> let me know what you think though

Yes, I agree.

> >> +	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).
> 
> Noted
> 
> >
> >> +	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.
> 
> Noted
> 
> >
> >> +	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.
> 
> Noted
> 
> >
> >> +
> >> +	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.
> 
> Noted. Will add a more explicit comment
> 
> >
> >> + */
> >> +int setup_memory_table(struct ctx *c) {
> >> +#define VHOST_MEMORY_REGION_PTR(addr, size) \
> >> +    (struct vhost_memory_region) { \
> >> +        .guest_phys_addr = (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.
> 
> Ok. will go with another name
> 
> >
> >> +
> >> +	/* 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
> 
> You mean every protocol file registers its own memory regions through
> calling the macro and taking as input the vhost_memory struct ?

More or less, yes.  Specifically I was thinking that this function
would call, say, tcp_vhost_regions() etc.

> It will be better in the sense that it will remove the need for making
> those buffers public. But i think it will be harder to follow from a
> readability perspective. WDYT ?

It's a trade off.  It's a bit harder to collate the complete set of
memory regions.  However it avoids making the buffers public and makes
it clearer when looking at the TCP code that part of "its" memory is
shared via vhost.  It also makes it a little easier to keep the shared
regions in sync if we change the data structures that (say) TCP uses
internally.

> >> + * rx_descriptor_handoff() - Batch-announce freed RX descriptors to the kernel
> >> + * @c:		Execution context
> >> + *
> >> + * Bumps avail.idx by the number of descriptors accumulated in
> >> + * vqs[0].num_free (from prior consume_one_rx_descriptor() calls),
> >> + * then resets the counter to zero. The kernel will see the new
> >> + * avail.idx and consume the freshly-available descriptors.
> >> + *
> >> + */
> >> +void rx_descriptor_handoff(struct ctx *c)
> >
> > Can this be static?  That's the sort of thing that's harder to review
> > when signatures are split from implementations.  If not, it should
> > have a properly prefixed name.
> 
> If by static you mean it gets defined in only one place and not exported
> then its used in two places (virtio.c and tap.c). Unless you want a
> duplicate definition which i personally am not in favor of.

Right, again, because the uses in different places are split across
patches, that makes it harder to see that it's used in multiple places.

> Can you please clarify what you mean by a prefixed name ? Not sure i
> understand what a proper prefix to a function like this would be

Usually (though not always) functions exported from one module to be
used in others have a name that starts with the module name.  So in
this case it would be vhost_something_something().

> >> +{
> >> +	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?
> 
> Yep, like the rest, both tap.c and virtio.c call it
> 
> >
> >> +	/* Ensure that the read to used->flags doesn't get reordered to be
> >> +	* above the avail.idx update 
> >> +	*/
> >> +	smp_mb();
> >> +
> >> +	if (!(used->flags & VRING_USED_F_NO_NOTIFY))
> >> +		eventfd_write(kick_fd, 1);
> >> +}
> >> \ No newline at end of file
> >     ^
> >     What diff said.
> 
> Noted
> 

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