On 7/28/25 11:30 AM, Eugenio Perez Martin wrote:
On Tue, Jul 22, 2025 at 2:41 PM Jonah Palmer <jonah.pal...@oracle.com> wrote:Iterative live migration for virtio-net sends an initial VMStateDescription while the source is still active. Because data continues to flow for virtio-net, the guest's avail index continues to increment after last_avail_idx had already been sent. This causes the destination to often see something like this from virtio_error(): VQ 0 size 0x100 Guest index 0x0 inconsistent with Host index 0xc: delta 0xfff4 This patch suppresses this consistency check if we're loading the initial VMStateDescriptions via iterative migration and unsuppresses it for the stop-and-copy phase when the final VMStateDescriptions (carrying the correct indices) are loaded. A temporary VirtIODevMigration migration data structure is introduced here to represent the iterative migration process for a VirtIODevice. For now it just holds a flag to indicate whether or not the initial VMStateDescription was sent during the iterative live migration process. Signed-off-by: Jonah Palmer <jonah.pal...@oracle.com> --- hw/net/virtio-net.c | 13 +++++++++++++ hw/virtio/virtio.c | 32 ++++++++++++++++++++++++-------- include/hw/virtio/virtio.h | 6 ++++++ 3 files changed, 43 insertions(+), 8 deletions(-) diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index 86a6fe5b91..b7ac5e8278 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -3843,12 +3843,19 @@ static void virtio_net_save_cleanup(void *opaque) static int virtio_net_load_setup(QEMUFile *f, void *opaque, Error **errp) { + VirtIONet *n = opaque; + VirtIODevice *vdev = VIRTIO_DEVICE(n); + vdev->migration = g_new0(VirtIODevMigration, 1); + vdev->migration->iterative_vmstate_loaded = false; + return 0; } static int virtio_net_load_state(QEMUFile *f, void *opaque, int version_id) { VirtIONet *n = opaque; + VirtIODevice *vdev = VIRTIO_DEVICE(n); + VirtIODevMigration *mig = vdev->migration; uint64_t flag; flag = qemu_get_be64(f); @@ -3861,6 +3868,7 @@ static int virtio_net_load_state(QEMUFile *f, void *opaque, int version_id) case VNET_MIG_F_INIT_STATE: { vmstate_load_state(f, &vmstate_virtio_net, n, VIRTIO_NET_VM_VERSION); + mig->iterative_vmstate_loaded = true;This code will need to change if we send the status iteratively more than once. For example, if the guest changes the mac address, the number of vqs, etc.
Hopefully we can reach a solution where we'd only need to call the full vmstate_load_state(f, &vmstate_virtio_net, ...) for a virtio-net device once and then handle any changes afterwards individually.
Perhaps, maybe for simplicity, we could just send the sub-states/subsections (instead of the whole state again) iteratively if there were any changes in the fields that those sub-states/subsections govern.
Definitely something I'll keep in mind as this series develops.
In my opinion, we should set a flag named "in_iterative_migration" (or equivalent) in virtio_net_load_setup and clear it in virtio_net_load_cleanup. That's enough to tell in virtio_load if we should perform actions like checking for inconsistent indices.
I did actually try something like this but I realized that the .load_cleanup and .save_cleanup hooks actually fire at the very end of live migration (e.g. during the stop-and-copy phase). I thought they fired at the end of the iterative portion of live migration, but this didn't appear to be the case.
break; } default: @@ -3875,6 +3883,11 @@ static int virtio_net_load_state(QEMUFile *f, void *opaque, int version_id) static int virtio_net_load_cleanup(void *opaque) { + VirtIONet *n = opaque; + VirtIODevice *vdev = VIRTIO_DEVICE(n); + g_free(vdev->migration); + vdev->migration = NULL; + return 0; } diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 5534251e01..68957ee7d1 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -3222,6 +3222,7 @@ virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id) int32_t config_len; uint32_t num; uint32_t features; + bool inconsistent_indices; BusState *qbus = qdev_get_parent_bus(DEVICE(vdev)); VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus); VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev); @@ -3365,6 +3366,16 @@ virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id) if (vdev->vq[i].vring.desc) { uint16_t nheads; + /* + * Ring indices will be inconsistent during iterative migration. The actual + * indices will be sent later during the stop-and-copy phase. + */ + if (vdev->migration) { + inconsistent_indices = !vdev->migration->iterative_vmstate_loaded; + } else { + inconsistent_indices = false; + }Nit, "inconsistent_indices = vdev->migration && !vdev->migration->iterative_vmstate_loaded" ? I'm happy with the current "if else" too, but I think the one line is clearer. Your call :).
Ah, nice catch! I like the one-liner more :) Will change this for next series.
+ /* * VIRTIO-1 devices migrate desc, used, and avail ring addresses so * only the region cache needs to be set up. Legacy devices need @@ -3384,14 +3395,19 @@ virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id) continue; } - nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx; - /* Check it isn't doing strange things with descriptor numbers. */ - if (nheads > vdev->vq[i].vring.num) { - virtio_error(vdev, "VQ %d size 0x%x Guest index 0x%x " - "inconsistent with Host index 0x%x: delta 0x%x", - i, vdev->vq[i].vring.num, - vring_avail_idx(&vdev->vq[i]), - vdev->vq[i].last_avail_idx, nheads); + if (!inconsistent_indices) { + nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx; + /* Check it isn't doing strange things with descriptor numbers. */ + if (nheads > vdev->vq[i].vring.num) { + virtio_error(vdev, "VQ %d size 0x%x Guest index 0x%x " + "inconsistent with Host index 0x%x: delta 0x%x", + i, vdev->vq[i].vring.num, + vring_avail_idx(&vdev->vq[i]), + vdev->vq[i].last_avail_idx, nheads); + inconsistent_indices = true; + } + } + if (inconsistent_indices) { vdev->vq[i].used_idx = 0; vdev->vq[i].shadow_avail_idx = 0; vdev->vq[i].inuse = 0; diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h index 214d4a77e9..06b6e6ba65 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -98,6 +98,11 @@ enum virtio_device_endian { VIRTIO_DEVICE_ENDIAN_BIG, }; +/* VirtIODevice iterative live migration data structure */ +typedef struct VirtIODevMigration { + bool iterative_vmstate_loaded; +} VirtIODevMigration; + /** * struct VirtIODevice - common VirtIO structure * @name: name of the device @@ -151,6 +156,7 @@ struct VirtIODevice bool disable_legacy_check; bool vhost_started; VMChangeStateEntry *vmstate; + VirtIODevMigration *migration; char *bus_name; uint8_t device_endian; /** -- 2.47.1
