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

["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.

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

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

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

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

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