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

["Ammar Yasser" <aerosound161@gmail.com>]

On Mon Aug 10, 2026 at 5:06 AM EEST, David Gibson wrote:
>>  
>>  /* 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.

Because pkt_buf is now going to be the buffer where vhost guest->pasta
data but with the added size of the virtio_net_hdr_mrg_rxbuf for every
frame. So this is accounting for the worst case where the guest wants to
send a full 128 frames at maximum size at a time. Will include this
rationale in the comment in the coming revision

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

We can either have a splitting comment that says something along the
lines of "from here onwards, all whats below if vhost-kernel code", or
even further, a brand new header. which will also solve the problem you
highlight above. Unless you have a preference to have them combined in
one header, i will go with the 2 header approach in the next revision

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

Ok

>
>> +#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).

My line of reasoning here is that "we typically expect, in the majority
of cases for an ethernet frame to span a single descriptor". evident by
how we eventually consume descriptors as we return the pointer into the
pkt_buf past the virtio_net header as the beginning of an ethernet
header. and this buffer should handle 128 frames (from the definition 
of PKT_BUF_BYTES) and so we want a denominator that yields a value as
close as possible to 128. Let me know if this isn't very sound. I will
investiagate the constants you mentioned anyways

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

Noted

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

Ok

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

There are two places that need those structures. virtio.c and tap.c,
which contains the rx/tx queue processing logic hence the extern. It
makes sense to me that queue handling functions live in tap.c, since
thats where the other code for sending/receiving from the fd was. and
this code does the same work but for vhost. WDYT ?

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

Sure. Will combine with the implementation commit

>
>>  #endif /* VIRTIO_H */
>> -- 
>> 2.34.1
>>