Re: [RFC v3 4/8] virtio: Define the pasta vhost interface

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

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

On Sun, Aug 02, 2026 at 01:21:51PM +0000, Ammar Yasser wrote:
> - Include the ioctls needed in opening, initializing and registering
>   queues and fds with vhost-net
> - Create the vq_state type which represents from pasta's perspective our
>   state of virtqueue processing
> - Define the structures that will represent the actual descriptor
>   queues: vring_desc (the actual descriptor chain), vring_used_all (used
>   ring for each queue), vring_avail_all (available ring for each queue),
>   vhost_memory (the structure that carries the shape of the memory
>   region as it should be shared with the kernel)
> - Increase PKT_BUF_BYTES by 1536 because the byte array that carries
>   the frames needs to also account for virtio_net_hdr_mrg_rxbuf header
>   sizes per each frame.
> - Define the signatures of functions that will be used in setting up the
>   virtqueues with the kernel (set_vring_for_queue, setup_memory_table,
>   setup_vhost_net, setup_eventfds).
> - Define the signature of rx_descriptor_handoff, which will be used to
>   mark descriptors as available again to the driver, and vhost_kick so
>   we can notify the driver of our updates
> 
> Signed-off-by: Ammar Yasser <aerosound161@gmail.com>
> ---
>  passt.h  |  2 +-
>  virtio.h | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 77 insertions(+), 1 deletion(-)
> 
> diff --git a/passt.h b/passt.h
> index c729316..88a1b03 100644
> --- a/passt.h
> +++ b/passt.h
> @@ -36,7 +36,7 @@ union epoll_ref;
>  	((uint8_t [ETH_ALEN]){0x9a, 0x55, 0x9a, 0x55, 0x9a, 0x55})
>  
>  /* Large enough for ~128 maximum size frames */
> -#define PKT_BUF_BYTES		(8UL << 20)
> +#define PKT_BUF_BYTES		((8UL << 20) + 1536)	/* 128 * sizeof(virtio_net_hdr_mrg_rxbuf) */

I think the rationale for this change needs to be clearer (granted,
the comment here beforehand is also kind of confusing).  IIRC - and
based on the "~" in the comment, I don't think there's a strict
requirement that this can hold 128 full frames - that's just setting a
reasonable sense of scale, and then a round number was picked near it:
~64kiB * ~64 ~= 8MiB

So, I'm not sure if this change is necessary - if it really is, we
need a clearer analysis of why.

>  extern char pkt_buf		[PKT_BUF_BYTES];
>  
> diff --git a/virtio.h b/virtio.h
> index 8f2ae06..2d1eef1 100644
> --- a/virtio.h
> +++ b/virtio.h
> @@ -9,14 +9,82 @@
>  #ifndef VIRTIO_H
>  #define VIRTIO_H
>  
> +#include <assert.h>
>  #include <stdbool.h>
> +#include <stddef.h>
>  #include <linux/vhost_types.h>
>  
> +struct ctx;
> +

AIUI, virtio.h is supposed to expose the virtio interfaces to passt,
not contain any passt specific logic, so including things that use
struct ctx (even just by reference) is probably not a good idea.  I
believe this header was originally formed as a cut down version of one
from qemu or a virtio library, so I'd probably also restrict it to
things that (conceptually) were in there.  Laurent will know more
about the history, since he introduced these for vhost-user.

We also want to be clear what definitions are general to virtio versus
which are specific to vhost-kernel versus vhost-user.

>  /* Maximum size of a virtqueue */
>  #define VIRTQUEUE_MAX_SIZE 1024
>  
>  #define VNET_HLEN	(sizeof(struct virtio_net_hdr_mrg_rxbuf))
>  
> +/* Keep in sync with PKT_BUF_BYTES in passt.h */
> +#define PKT_BUF_BYTES		((8UL << 20) + 1536)	/* 128 * sizeof(virtio_net_hdr_mrg_rxbuf) */

Ouch.  We really want to avoid that sort of duplication, even if it
means splitting out a new small header included from both places.

> +#define VHOST_NDESCS (PKT_BUF_BYTES / 65520)

I'm not sure 65520 is the right number here.  That's the max MTU at
the IP level, but the packet buffer will also hold the 14 byte L2
header.  I think you probably want one of the L2_MAX_LEN_* constants
(or to define a new one for vhost-kernel).

> +static_assert(!(VHOST_NDESCS & (VHOST_NDESCS - 1)),
> +			 "Number of vhost descs must be a power of two by standard");
> +
> +
> +#define VIRTQUEUE_MAX_SIZE 1024

Duplicate #define.

> +
> +#define VHOST_VIRTIO         0xAF
> +#define VHOST_GET_FEATURES   _IOR(VHOST_VIRTIO, 0x00, __u64)
> +#define VHOST_SET_FEATURES   _IOW(VHOST_VIRTIO, 0x00, __u64)
> +#define VHOST_SET_OWNER	     _IO(VHOST_VIRTIO, 0x01)
> +#define VHOST_SET_MEM_TABLE  _IOW(VHOST_VIRTIO, 0x03, struct vhost_memory)
> +#define VHOST_SET_VRING_NUM  _IOW(VHOST_VIRTIO, 0x10, struct vhost_vring_state)
> +#define VHOST_SET_VRING_ADDR _IOW(VHOST_VIRTIO, 0x11, struct vhost_vring_addr)
> +#define VHOST_SET_VRING_KICK _IOW(VHOST_VIRTIO, 0x20, struct vhost_vring_file)
> +#define VHOST_SET_VRING_CALL _IOW(VHOST_VIRTIO, 0x21, struct vhost_vring_file)
> +#define VHOST_SET_VRING_ERR  _IOW(VHOST_VIRTIO, 0x22, struct vhost_vring_file)
> +#define VHOST_SET_BACKEND_FEATURES _IOW(VHOST_VIRTIO, 0x25, __u64)
> +#define VHOST_NET_SET_BACKEND _IOW(VHOST_VIRTIO, 0x30, struct vhost_vring_file)

It's probably preferable to #include <linux/vhost.h> rather than
restating these.

> +/**
> + * struct vq_state - Per-virtqueue local descriptor tracking
> + * @num_free:		Number of descriptors ready to be announced
> + *			to the kernel as available via rx_descriptor_handoff()
> + * @last_used_idx:	Number of used-ring entries consumed so far;
> + *			lagging read cursor vs. vring_used->idx (the
> + *			kernel's write cursor)
> + */
> +extern struct vq_state {
> +	uint16_t num_free;
> +	uint16_t last_used_idx;
> +	uint16_t next_free;
> +} vqs[2];

This is a vhost-kernel relevant view of a vq - we also have vhost-user
relevant views, which is a bit confusing.  Renaming and/or moving to a
vhost-kernel specific header is probably a good idea.


> +extern struct vring_desc vring_desc[2][VHOST_NDESCS];

Do we need these externs, or could we make these structures local to
the .c file actually doing the vhost-kernel handling?

> +union vring_avail_u {
> +	struct vring_avail avail;
> +	char buf[offsetof(struct vring_avail, ring[VHOST_NDESCS])];
> +};
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wpedantic"
> +extern union vring_avail_u vring_avail_all[2];
> +#pragma GCC diagnostic pop
> +
> +union vring_used_u {
> +	struct vring_used used;
> +	char buf[offsetof(struct vring_used, ring[VHOST_NDESCS])];
> +};
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wpedantic"
> +extern union vring_used_u vring_used_all[2];
> +#pragma GCC diagnostic pop
> +
> +#define N_VHOST_REGIONS 12
> +union vhost_memory_u {
> +	struct vhost_memory mem;
> +	char buf[offsetof(struct vhost_memory, regions[N_VHOST_REGIONS])];
> +};
> +extern union vhost_memory_u vhost_memory;
> +
>  /**
>   * struct vu_ring - Virtqueue rings
>   * @num:		Size of the queue
> @@ -147,6 +215,7 @@ struct vu_virtq_element {
>  	struct iovec *out_sg;
>  };
>  
> +void vu_queue_notify(const struct vu_dev *dev, struct vu_virtq *vq);
>  /**
>   * has_feature() - Check a feature bit in a features set
>   * @features:	Features set
> @@ -199,4 +268,11 @@ void vu_queue_fill(const struct vu_dev *vdev, struct vu_virtq *vq,
>  		   unsigned int idx);
>  void vu_queue_flush(const struct vu_dev *vdev, struct vu_virtq *vq,
>  		    unsigned int count);
> +void set_vring_for_queue(struct ctx *c, int queue_idx, int tap_fd);
> +int setup_memory_table(struct ctx *c);
> +void setup_vhost_net(struct ctx *c);
> +void setup_eventfds(struct ctx *c, int queue_idx);
> +void rx_descriptor_handoff(struct ctx *c);
> +void vhost_kick(struct vring_used *used, int kick_fd);
> +

We generally don't like to introduce function signatures separate from
function implementations.  It's sometimes a fuzzy line, but the idea
is to split patches on logical concepts / subfeatures, not on
different parts of the code for the same thing.  That generally makes
review easier.

>  #endif /* VIRTIO_H */
> -- 
> 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

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