On Wed, 11 Mar 2026 07:55:33 +0100 Eugenio Pérez <[email protected]> wrote:
> This series introduces features to the VDUSE (vDPA Device in Userspace) driver > to support Live Migration. > > Currently, DPDK does not support VDUSE devices live migration because the > driver lacks a mechanism to suspend the device and quiesce the rings to > initiate the switchover. This series implements the suspend operation to > address this limitation. > > Furthermore, enabling Live Migration for devices with control virtqueue needs > two additional features. Both of them are included in this series. > > * Address Spaces (ASID) support: This allows QEMU to isolate and intercept the > device's CVQ. By doing so, QEMU is able to migrate the device status > transparently, without requiring the device to support state save and > restore. > * QUEUE_ENABLE: This allows QEMU to control when the dataplane virtqueues are > enabled. This ensures the dataplane is started after the device > configuration has been fully restores via the CVQ. > > Last but not least, it enables the VIRTIO_NET_F_STATUS feature. This allows > the > device to signal the driver that it needs to send gratuitous ARP with > VIRTIO_NET_S_ANNOUNCE, reducing the Live Migration downtime. > > v3: > * Replace incorrect '%lx' DEBUG print format specifier with PRIx64 > > v2: > * Following latest comments on kernel series about VDUSE features, not > checking > API version but only check if VDUSE_GET_FEATURES success. > * Move the start and last declarations in the braces as gcc 8 does not like > them interleaved with statements. Actually, I think the move was a mistake > in > the first version. > https://mails.dpdk.org/archives/test-report/2026-February/958175.html > > Eugenio Pérez (1): > uapi: align VDUSE header for ASID > > Maxime Coquelin (6): > vhost: introduce ASID support > vhost: add VDUSE API version negotiation > vhost: add virtqueues groups support to VDUSE > vhost: add ASID support to VDUSE IOTLB operations > vhost: claim VDUSE support for API version 1 > vhost: add net status feature to VDUSE > > Super User (3): > uapi: Align vduse.h for enable and suspend VDUSE messages > vhost: Support VDUSE QUEUE_READY feature > vhost: Support vduse suspend feature > > kernel/linux/uapi/linux/vduse.h | 114 +++++++++++++++- > lib/vhost/iotlb.c | 226 ++++++++++++++++++++------------ > lib/vhost/iotlb.h | 14 +- > lib/vhost/vduse.c | 197 +++++++++++++++++++++++++--- > lib/vhost/vduse.h | 3 +- > lib/vhost/vhost.c | 16 +-- > lib/vhost/vhost.h | 16 +-- > lib/vhost/vhost_user.c | 11 +- > 8 files changed, 460 insertions(+), 137 deletions(-) > As an experiment tried Google Gemini with AGENTS file to review this patch. Summary of Findings The patch series successfully introduces ASID support and VDUSE API version 1 features (virtqueue groups, net status, queue ready, and suspend) to the vhost library. However, there are a few correctness issues, most notably a resource leak in the IOTLB initialization error path, and several style nits regarding pointer comparisons. --- Correctness Bugs (High Priority) 1. Resource Leak in vhost_user_iotlb_init (lib/vhost/iotlb.c) In Patch 02/10, the newly introduced vhost_user_iotlb_init function fails to correctly clean up resources when one of the ASID IOTLB initializations fails. 1 +int 2 +vhost_user_iotlb_init(struct virtio_net *dev) 3 +{ 4 + int i; 5 + 6 + for (i = 0; i < IOTLB_MAX_ASID; i++) 7 + if (vhost_user_iotlb_init_one(dev, i) < 0) 8 + goto fail; 9 + 10 + return 0; 11 +fail: 12 + while (i--) 13 + { 14 + rte_free(dev->iotlb[i]->pool); 15 + dev->iotlb[i]->pool = NULL; 16 + } 17 + 18 + return -1; 19 +} Issues: - Leak of iotlb structures: The loop only frees the pool member but does not rte_free(dev->iotlb[i]). - Incomplete cleanup of the failed index: If vhost_user_iotlb_init_one(dev, i) fails after allocating dev->iotlb[i] but before returning, the while (i--) loop will skip the current index i, leaking its resources entirely. - Zero-index failure: if i=0 fails, the while(i--) loop will not execute at all, leaking all resources allocated for index 0. Recommendation: The fail block should call vhost_user_iotlb_destroy(dev) or be rewritten to properly free both the pool and the iotlb structure for all indices up to and including the one that failed. 2. Potential NULL Pointer Dereference in VDUSE_SUSPEND Handler (lib/vhost/vduse.c) In Patch 10/10, the VDUSE_SUSPEND case in vduse_events_handler accesses dev->notify_ops without a NULL check, unlike the VDUSE_SET_VQ_READY handler in Patch 09/10. 1 + case VDUSE_SUSPEND: 2 + ... 3 + for (i = 0; dev->notify_ops->vring_state_changed && 4 + i < dev->nr_vring; i++) { Issue: If dev->notify_ops is NULL, this will cause a segmentation fault. While DRIVER_OK is checked, it is safer and more consistent to explicitly check dev->notify_ops before access. --- Style and Process Compliance 3. Implicit Pointer Comparisons The DPDK coding style (per AGENTS.md) requires explicit comparisons for pointers (e.g., if (p == NULL)). Several patches use implicit comparisons (if (!p)). Instances include: - Patch 02/10: if (!dev->iotlb[asid]) and if (!dev->iotlb[asid]->pool) in vhost_user_iotlb_init_one. - Patch 09/10: if (!dev->notify_ops || !dev->notify_ops->vring_state_changed) in VDUSE_SET_VQ_READY handler. - Patch 09/10: if (!dev->notify_ops) in vduse_device_start. 4. Confusing Log Message in VDUSE_SET_VQ_READY (lib/vhost/vduse.c) In Patch 09/10: 1 + VHOST_CONFIG_LOG(dev->ifname, ERR, 2 + "Unexpected ready message with feature %d", 3 + !!(dev->vduse_features & 4 + RTE_BIT64(VDUSE_F_QUEUE_READY))); Since this is inside a !(dev->vduse_features & ...) block, it will always log "feature 0". It would be clearer to state that the VDUSE_F_QUEUE_READY feature was not negotiated. 5. Non-inclusive Naming in vduse_iotlb_miss (lib/vhost/vduse.c) Patch 02/10 and 05/10 modify vduse_iotlb_miss. While not introduced by this patch, the function uses the term miss, which is generally acceptable but should be noted if a more descriptive term like iotlb_entry_fetch_failure is preferred in the future. (Note: This is a minor observation and does not strictly violate the prohibited terminology list). --- Additional Observations - UAPI Headers: The alignment with upcoming Linux kernel VDUSE headers (ASID, groups, suspend) is correctly handled in patches 01 and 08. - Experimental Tags: New internal logic is correctly added without exposing new public APIs, so __rte_experimental tags were not required for these changes. - Static Linking: The use of supported_vduse_features as a static constant correctly avoids namespace pollution.
