Hi Peter
On Fri, May 10, 2024 at 9:33 PM Peter Xu <pet...@redhat.com> wrote:
>
> 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.
IPMI KCS being a top-level section, the fields with an unsupported
version are filtered before reaching this code.
But since I can't see how a machine will have a specific version, it
only helps for backward migration, which is quite limited.
>
> 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.. :-(
Ack.
Do you think we should add a version on the wire for each VMSD? that
will likely be a format change.
>
> 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.
Works for me! thanks for figuring out how to get back the VirtioGPU* !
I'll send v2 with your patch.
>
> 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
>
--
Marc-André Lureau
