Hi, Marc-André,
On Fri, May 10, 2024 at 12:39:34PM +0400, Marc-André Lureau wrote:
> Since we don't have per VMSD version information on the wire, nested
> struct versioning is quite limited and cumbersome. I am not sure it
> can be changed without breaking the stream format, and whether it's
> worthwhile.
Right that's a major pain, and actually I just notice it..
I think it'll be much, much simpler if we keep vmsd version on the wire for
each VMSD (including struct fields), then it makes more sense to me.
Then when I went back and see again the VSTRUCT thing... I can hardly
understand what it is doing, and also how it works at all.
Look at the current only IPMI user, who has:
VMSTATE_VSTRUCT_V(kcs, ISAIPMIKCSDevice, 2, vmstate_IPMIKCS,
IPMIKCS, 2),
It is setting both vmsd version and struct_version to 2. I can't tell why
it matters then if anyway both of the fields are the same..
When we do save(), there is:
} else if (field->flags & VMS_STRUCT) {
ret = vmstate_save_state(f, field->vmsd, curr_elem,
vmdesc_loop);
} else if (field->flags & VMS_VSTRUCT) {
ret = vmstate_save_state_v(f, field->vmsd, curr_elem,
vmdesc_loop,
field->struct_version_id, errp);
When we load():
} else if (field->flags & VMS_STRUCT) {
ret = vmstate_load_state(f, field->vmsd, curr_elem,
field->vmsd->version_id);
} else if (field->flags & VMS_VSTRUCT) {
ret = vmstate_load_state(f, field->vmsd, curr_elem,
field->struct_version_id);
} else {
In this case, passing in struct_version==version should have zero effect
afaict, because the default behavior is passing in vmsd->version_id anyway.
Moreover, now I highly doubt whether the VMS_STRUCT whole thing makes sense
at all as you mentioned. Especially on the load side, here we should rely
on vmstate_load_state() taking the last parameter as version_id on the
wire. Here we're passing in the struct's version_id or struct_version_id,
and neither of them makes sense to me... if we miss that version_id
information, afaiu we should simply fix it and put it on the wire.. It'll
break migration, we may need to work that out, but I don't see a better
way. Keeping it like this like a nightmare to me.. :-(
Irrelevant of all these mess.. For this specific problem, what I meant is
exactly what Michael was requesting too (hopefully), I'd want to avoid
further extending the complexity in this area. I have a patch attached at
last which I also tested 8.2<->9.0 bi-directional migrations and it worked
for me when I smoked it. Please have a look to see whether that makes
sense and at the meantime avoid most of the tricks.
I'd also like to mention one more thing just in case this can cause some
more attention to virtio guys..
Normally I ran vmstate-static-checker.py before softfreeze, and I did it
for 9.0 too without seeing this problem. It isn't raised because all
virtio devices are using the "self managed" VMSTATE_VIRTIO_DEVICE to
migrate. In that case I am out of luck. We can further extend what
Fabiano mentioned in the other thread to cover migration stream validations
in the future, but just to mention IMHO that needs extra work, and may work
most likely the same as vmstate static checker but just waste many more cpu
resources. It'll be good if someone could still help move virtio towards
like most of the rest devices, or at least get covered by the static
checker, too. But that definitely is a separate topic too.. so we can
address the immediate breakage first.
Thanks,
==8<==
>From a24ef99670fa7102da461d795aed4a957bad86b1 Mon Sep 17 00:00:00 2001
From: Peter Xu <pet...@redhat.com>
Date: Fri, 10 May 2024 12:33:34 -0400
Subject: [PATCH] fix gpu
Signed-off-by: Peter Xu <pet...@redhat.com>
---
include/hw/virtio/virtio-gpu.h | 2 +-
hw/core/machine.c | 1 +
hw/display/virtio-gpu.c | 21 +++++++++++++++------
3 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index ed44cdad6b..e128501bdc 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -176,7 +176,7 @@ typedef struct VGPUDMABuf {
struct VirtIOGPU {
VirtIOGPUBase parent_obj;
-
+ uint8_t vmstate_version;
uint64_t conf_max_hostmem;
VirtQueue *ctrl_vq;
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 4ff60911e7..8f6f0dda7c 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -42,6 +42,7 @@ GlobalProperty hw_compat_8_2[] = {
{ "migration", "zero-page-detection", "legacy"},
{ TYPE_VIRTIO_IOMMU_PCI, "granule", "4k" },
{ TYPE_VIRTIO_IOMMU_PCI, "aw-bits", "64" },
+ { "virtio-gpu-device", "x-vmstate-version", "1" },
};
const size_t hw_compat_8_2_len = G_N_ELEMENTS(hw_compat_8_2);
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index ae831b6b3e..c53f55404c 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1166,6 +1166,14 @@ static void virtio_gpu_cursor_bh(void *opaque)
virtio_gpu_handle_cursor(&g->parent_obj.parent_obj, g->cursor_vq);
}
+static bool vmstate_after_v2(void *opaque, int version)
+{
+ struct VirtIOGPUBase *base = container_of(opaque, VirtIOGPUBase, scanout);
+ struct VirtIOGPU *gpu = container_of(base, VirtIOGPU, parent_obj);
+
+ return gpu->vmstate_version >= 2;
+}
+
static const VMStateDescription vmstate_virtio_gpu_scanout = {
.name = "virtio-gpu-one-scanout",
.version_id = 2,
@@ -1181,12 +1189,12 @@ static const VMStateDescription
vmstate_virtio_gpu_scanout = {
VMSTATE_UINT32(cursor.hot_y, struct virtio_gpu_scanout),
VMSTATE_UINT32(cursor.pos.x, struct virtio_gpu_scanout),
VMSTATE_UINT32(cursor.pos.y, struct virtio_gpu_scanout),
- VMSTATE_UINT32_V(fb.format, struct virtio_gpu_scanout, 2),
- VMSTATE_UINT32_V(fb.bytes_pp, struct virtio_gpu_scanout, 2),
- VMSTATE_UINT32_V(fb.width, struct virtio_gpu_scanout, 2),
- VMSTATE_UINT32_V(fb.height, struct virtio_gpu_scanout, 2),
- VMSTATE_UINT32_V(fb.stride, struct virtio_gpu_scanout, 2),
- VMSTATE_UINT32_V(fb.offset, struct virtio_gpu_scanout, 2),
+ VMSTATE_UINT32_TEST(fb.format, struct
virtio_gpu_scanout,vmstate_after_v2),
+ VMSTATE_UINT32_TEST(fb.bytes_pp, struct
virtio_gpu_scanout,vmstate_after_v2),
+ VMSTATE_UINT32_TEST(fb.width, struct
virtio_gpu_scanout,vmstate_after_v2),
+ VMSTATE_UINT32_TEST(fb.height, struct
virtio_gpu_scanout,vmstate_after_v2),
+ VMSTATE_UINT32_TEST(fb.stride, struct
virtio_gpu_scanout,vmstate_after_v2),
+ VMSTATE_UINT32_TEST(fb.offset, struct
virtio_gpu_scanout,vmstate_after_v2),
VMSTATE_END_OF_LIST()
},
};
@@ -1659,6 +1667,7 @@ static Property virtio_gpu_properties[] = {
DEFINE_PROP_BIT("blob", VirtIOGPU, parent_obj.conf.flags,
VIRTIO_GPU_FLAG_BLOB_ENABLED, false),
DEFINE_PROP_SIZE("hostmem", VirtIOGPU, parent_obj.conf.hostmem, 0),
+ DEFINE_PROP_UINT8("x-vmstate-version", VirtIOGPU, vmstate_version, 2),
DEFINE_PROP_END_OF_LIST(),
};
--
2.44.0
--
Peter Xu
