From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: passt.top; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256 header.s=20251104 header.b=RrplXsnn; dkim-atps=neutral Received: from mail-wr1-x42e.google.com (mail-wr1-x42e.google.com [IPv6:2a00:1450:4864:20::42e]) by passt.top (Postfix) with ESMTPS id ECA835A0262 for ; Wed, 12 Aug 2026 19:17:22 +0200 (CEST) Received: by mail-wr1-x42e.google.com with SMTP id ffacd0b85a97d-471eeac43bfso990133f8f.3 for ; Wed, 12 Aug 2026 10:17:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1786555042; x=1787159842; darn=passt.top; h=in-reply-to:references:to:from:subject:cc:message-id:date :content-type:content-transfer-encoding:mime-version:from:to:cc :subject:date:message-id:reply-to:content-type; bh=kloZdTt26mBhXfKryKi1DzoKLb3aex8FjKAYjEaufzg=; b=RrplXsnnyrnBYTnQzxQQq/CthJjNu/NEkloXX5NpbY5B90h+uObv9ekwfaiTkXZsWr b6WOOdM1/ifG7CnQjmB8KgJybTvyGCzOD6hDAVQzgtx4SwZCtF8AJ5x1baZh4kbDxGlw 3gBJrRqghaVKCCTfSDdVF5YgDk4NzGWaoxZFfuqrZDDdr4SqGy9umD+M8d4bth3F1Lyv QppG/OJWRdXcaWhW7hlpVDWXSfFCbTq1xI1m+zY6A/Pjo2qzW1gAF+xWx/ZG27Fyj9qg Cl5WGdle1V/oytOQT0A8tiz37aa7Bq42xyGFVJr2F4XheSQL0IeGrDWHR99PYxsbGl8H 4f2Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1786555042; x=1787159842; h=in-reply-to:references:to:from:subject:cc:message-id:date :content-type:content-transfer-encoding:mime-version:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to :content-type; bh=kloZdTt26mBhXfKryKi1DzoKLb3aex8FjKAYjEaufzg=; b=dpnP0XaQdNeDBS9rl8zSUdatY5VViHkCKO5V3oetZqauTKmKpFsmsjMA4adAjvZjBz OFvbKg4ynq44/HK5ZR3/wdPdTpFEAZZrrXD/BvOKgXlJpTPd5yPPyJV+hREJE01WCN6V 0UC2//r6CCq+9E4+bFXpFpekohU+dwBZf04bAiv9QhV+pMu2JTkBJwoRUqtS8VU+KShK p6CAugcjY6YuqQVctse+iZtAJzuM8ftFO2nOcZw0hs8k6WjAQ0bHnbGoRlAXUE4Ez7wA Nwutd36pnZhbpw70L9of89xw9RXRu41h/XVxU7HD8Wx30uQ8MLFD/YCN32qBTrNVCQ7g HRBg== X-Gm-Message-State: AOJu0YxDBeF/S9zRsBYcYerBkVOEGM3kOPgJ/l72FuonEABEib9rcKTj ud7Ubbgj+zhkDglsBv5g98TYRvCoTkue7SCJWZpKS03Q0gyKs+LJtOgo X-Gm-Gg: AR+sD100habIT51cYn1cAXG7AJqkmfOA1PA3c5K2gDPE74O84SU2XDwkWt6OZRt0L02 mTZRlHoRbJ1PWZ+PsQkuYmpHYscQ3x2dYWTF9sVH+BZMTOc+gDCVCI+mcxi9Sb/WPfb/Dtn8ZqO siZ+/1myHoB+bAn2sd574tm4g8X9V/9nhVqvrDfvZLABkagSZYK5zD2n3cxXxS8X7ItRYxuhlaw OtcvKxAPgdS9RgrjcisXivXOBFfTM+mD0AEzAEiqorJp46RGQo4iMVrq+HgmLPFcdn+RLCBgIXn 3dBo4vmM5u7lnanDvb8ERxfAJ1Nb0WmZz5vyWwDSdeDujIqiOMi9+Y2i3W7MZgmXiFm4CxMXpgV nRe2334kAKbXcjgOUBtNIbOPlJErUi5kVyZ9IYDqQ6Cv+GfKc+nD44HBe/NHypvnuN8fgLvdZEm xYq7jQmBfTbogOo3InpgikgdaYy+09AwKBOCmaE0HMfUyhxkpDNnLxoCjMUMAe9hOW8cRgKpoS2 +s6kUG7G6gXXTkCIEP+tif2YMe8fDE= X-Received: by 2002:a05:6000:2c02:b0:47f:93b4:2def with SMTP id ffacd0b85a97d-48152b01892mr8437022f8f.28.1786555042233; Wed, 12 Aug 2026 10:17:22 -0700 (PDT) Received: from localhost ([196.157.64.51]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-48150c06b73sm8926266f8f.12.2026.08.12.10.17.21 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 12 Aug 2026 10:17:21 -0700 (PDT) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Wed, 12 Aug 2026 20:17:19 +0300 Message-Id: Subject: Re: [RFC v3 4/8] virtio: Define the pasta vhost interface From: "Ammar Yasser" To: "David Gibson" , "Ammar Yasser" X-Mailer: aerc 0.21.0 References: <20260802132155.870796-1-aerosound161@gmail.com> <20260802132155.870796-5-aerosound161@gmail.com> In-Reply-To: Message-ID-Hash: DLLN4EXCNN4HR3EHRX34KABQN5JB5WPQ X-Message-ID-Hash: DLLN4EXCNN4HR3EHRX34KABQN5JB5WPQ X-MailFrom: aerosound161@gmail.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top, eperezma@redhat.com X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Mon Aug 10, 2026 at 5:06 AM EEST, David Gibson wrote: >> =20 >> /* 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 ~=3D 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]; >> =20 >> 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 >> =20 >> +#include >> #include >> +#include >> #include >> =20 >> +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 >> =20 >> #define VNET_HLEN (sizeof(struct virtio_net_hdr_mrg_rxbuf)) >> =20 >> +/* 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=20 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_memo= ry) >> +#define VHOST_SET_VRING_NUM _IOW(VHOST_VIRTIO, 0x10, struct vhost_vrin= g_state) >> +#define VHOST_SET_VRING_ADDR _IOW(VHOST_VIRTIO, 0x11, struct vhost_vrin= g_addr) >> +#define VHOST_SET_VRING_KICK _IOW(VHOST_VIRTIO, 0x20, struct vhost_vrin= g_file) >> +#define VHOST_SET_VRING_CALL _IOW(VHOST_VIRTIO, 0x21, struct vhost_vrin= g_file) >> +#define VHOST_SET_VRING_ERR _IOW(VHOST_VIRTIO, 0x22, struct vhost_vrin= g_file) >> +#define VHOST_SET_BACKEND_FEATURES _IOW(VHOST_VIRTIO, 0x25, __u64) >> +#define VHOST_NET_SET_BACKEND _IOW(VHOST_VIRTIO, 0x30, struct vhost_vri= ng_file) > > It's probably preferable to #include 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; >> }; >> =20 >> +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, struc= t 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 */ >> --=20 >> 2.34.1 >>=20