Re: [RFC v3 00/10] Add vduse live migration features

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 11 Mar 2026 07:55:33 +0100
Eugenio Pérez <eperezma@redhat.com> 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.