Re: [PATCH v2 3/7] virtio: Add the pasta vhost-net interface and implementation

[Laurent Vivier <lvivier@redhat.com>]

On 10/16/26 22:49, aerosouund wrote:
> From: Ammar Yasser <aerosound161@gmail.com>
> 
> Add vhost.c and vhost.h which define the api and implementation for
> pasta vhost acceleration
> 
> - vq_state/vhost_vq_state[]: our own view of each virtqueue, which the
>    kernel doesn't track for us: descriptors ready to hand back, how far
>    we've read the used ring, and the next descriptor to allocate
> - vring_desc, vring_avail_all, vring_used_all, vhost_memory: the rings
>    and the region table shared with the kernel
> - vhost_setup_net: open /dev/vhost-net and negotiate VIRTIO_F_VERSION_1
>    and VHOST_NET_F_VIRTIO_NET_HDR. Failure is fatal only if
>    --vhost-kernel is set to 'on'.
> - vhost_setup_eventfds: create, register and watch the call, kick and
>    error eventfds for one queue
> - vhost_setup_memory_table: register the buffers the kernel may touch.
>    pasta has no guest, so each region maps guest_phys_addr onto the
>    identical userspace_addr, making GPA translation an identity mapping.
>    This function uses the *_register_memory_regions functions created per
>    protocol and for general purpose buffers.
> - vhost_set_vring: publish a vring's addresses, bind the tap fd as its
>    backend and lay out its descriptors
> - vhost_rx_descriptor_handoff, vhost_kick: hand consumed descriptors back
>    and notify the kernel unless it says it's already polling
> 
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> Signed-off-by: Ammar Yasser <aerosound161@gmail.com>
> ---
>   Makefile  |   2 +-
>   tcp_buf.c |  22 +++-
>   tcp_buf.h |   4 +
>   udp.c     |  20 ++++
>   udp.h     |   3 +
>   util.c    |  18 ++++
>   util.h    |   2 +
>   vhost.c   | 295 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   vhost.h   |  91 +++++++++++++++++
>   9 files changed, 455 insertions(+), 2 deletions(-)
>   create mode 100644 vhost.c
>   create mode 100644 vhost.h
> 
...
> diff --git a/vhost.c b/vhost.c
> new file mode 100644
> index 0000000..a69c4b5
> --- /dev/null
> +++ b/vhost.c
...
> +/**
> + * vhost_set_vring() - Register a vring's addresses and bind its backend
> + * @c:		Execution context; c->vhost.fd 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 vhost_set_vring(struct ctx *c, int queue_idx, int tap_fd)
> +{
> +	int vhost_fd = c->vhost.fd;
> +	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],

I think it would be better to use uintptr_t for the casts.

> +	};
> +	struct vhost_vring_file file = {
> +		.index = queue_idx,
> +		.fd = tap_fd,
> +	};
> +	unsigned int i;
> +	int rc;
> +
> +	rc = ioctl(vhost_fd, VHOST_SET_VRING_ADDR, &addr);
> +	if (rc < 0)
> +		die_perror("VHOST_SET_VRING_ADDR ioctl failed, queue %d",
> +			   queue_idx);
> +
> +	if (queue_idx == VHOST_AVAIL_Q_IDX) {


VHOST_AVAIL_Q_IDX (0) is not really meaningful, and if in the future we want to support 
multiqueue we should use something like VHOST_USER_IS_QUEUE_RX(queue_idx)

> +		for (i = 0; i < VHOST_NDESCS; ++i) {
> +			vring_desc[VHOST_AVAIL_Q_IDX][i].addr =
> +				(uintptr_t)pkt_buf + i * VHOST_DESC_BYTES;
> +			vring_desc[VHOST_AVAIL_Q_IDX][i].len = VHOST_DESC_BYTES;
> +			vring_desc[VHOST_AVAIL_Q_IDX][i].flags =
> +				VRING_DESC_F_WRITE;
> +		}
> +
> +		for (i = 0; i < VHOST_NDESCS; ++i)
> +			vring_avail_all[VHOST_AVAIL_Q_IDX].ring[i] =
> +				htole16(i);
> +
> +		vhost_rx_descriptor_handoff(c);
> +	}
> +
> +	if (queue_idx == VHOST_USED_Q_IDX) {

something like VHOST_USER_IS_QUEUE_TX(queue_idx), perhaps we can rename them 
VHOST_IS_QUEUE_TX()/VHOST_IS_QUEUE_RX().

Please add a comment explaining .addr and .len will be set in tap_send_frames_vhost()

> +		for (i = 0; i < (VHOST_NDESCS - 1); ++i)
> +			vring_desc[VHOST_USED_Q_IDX][i].next = i + 1;
> +	}
> +
> +	rc = ioctl(vhost_fd, VHOST_NET_SET_BACKEND, &file);
> +	if (rc < 0)
> +		die_perror("VHOST_NET_SET_BACKEND ioctl failed, queue %d",
> +			   queue_idx);
> +}
> +
> +/**
> + * vhost_rx_descriptor_handoff() - Announce freed from-guest descriptors
> + * @c:		Execution context
> + *
> + * Bumps avail.idx by the number of descriptors accumulated in
> + * vhost_vq_state[VHOST_AVAIL_Q_IDX].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 vhost_rx_descriptor_handoff(struct ctx *c)

perhaps you can add queue_idx in the parameters.

> +{
> +	smp_wmb();
> +
> +	if (!vhost_vq_state[VHOST_AVAIL_Q_IDX].num_free)
> +		return;
> +
> +	vring_avail_all[VHOST_AVAIL_Q_IDX].idx +=
> +		vhost_vq_state[VHOST_AVAIL_Q_IDX].num_free;
> +	vhost_vq_state[VHOST_AVAIL_Q_IDX].num_free = 0;
> +	vhost_kick(&vring_used_all[VHOST_AVAIL_Q_IDX],
> +		   c->vhost.vq[VHOST_AVAIL_Q_IDX].kick_fd);
> +}
> +
> +/**
> + * vhost_kick() - Notify the kernel that new descriptors are available
> + * @used:	Used ring of the queue we're announcing on, checked to see
> + *		whether the kernel's virtio thread is already reading
> + *		descriptors and doesn't want to be notified
> + * @kick_fd:	Kick eventfd of that same queue
> + */
> +void vhost_kick(struct vring_used_pasta *used, int kick_fd)
> +{
> +	/* Ensure that the read of 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);
> +}