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

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 --- 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 #include +#include #include

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