[PATCH] hw/display/vga-isa: Fix migration of the isa-vga device

[PATCH] hw/display/vga-isa: Fix migration of the isa-vga device

[Thomas Huth]

From: Thomas Huth <thuth@redhat.com>

QEMU currently crashes when migrating a guest that uses the
isa-vga device as display. This happens because vga_isa_class_initfn()
registers a vmsd for vmstate_vga_common that operates on VGACommonState.
But the isa-vga device is derived from ISADevice, not from VGACommonState,
so the migration code tries to fill in the data for VGACommonState to
the memory that is a ISADevice instead, which is of cause causing trouble.

We need an indirection here as it's also e.g. done in vga-pci.c, so
that the migration data gets filled into the right location.

While we're at it, also drop the "global_vmstate = true" here. Since
migration was broken for this device during the last 15 years (!) anyway,
we don't have to worry about maintaining backward compatibility with this
switch for older versions of QEMU anymore.

Fixes: 7435b791ca9 ("vga-isa: convert to qdev")
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/display/vga-isa.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/hw/display/vga-isa.c b/hw/display/vga-isa.c
index 95d85ff69a5..5f55c884a1b 100644
--- a/hw/display/vga-isa.c
+++ b/hw/display/vga-isa.c
@@ -32,6 +32,7 @@
 #include "qemu/timer.h"
 #include "hw/core/loader.h"
 #include "hw/core/qdev-properties.h"
+#include "migration/vmstate.h"
 #include "ui/console.h"
 #include "qom/object.h"
 
@@ -62,7 +63,6 @@ static void vga_isa_realizefn(DeviceState *dev, Error **errp)
     MemoryRegion *vga_io_memory;
     const MemoryRegionPortio *vga_ports, *vbe_ports;
 
-    s->global_vmstate = true;
     if (!vga_common_init(s, OBJECT(dev), errp)) {
         return;
     }
@@ -88,6 +88,15 @@ static void vga_isa_realizefn(DeviceState *dev, Error **errp)
     rom_add_vga(VGABIOS_FILENAME);
 }
 
+static const VMStateDescription vmstate_vga_isa = {
+    .name = "vga-isa",
+    .version_id = 1,
+    .fields = (const VMStateField[]) {
+        VMSTATE_STRUCT(state, ISAVGAState, 0, vmstate_vga_common, VGACommonState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const Property vga_isa_properties[] = {
     DEFINE_PROP_UINT32("vgamem_mb", ISAVGAState, state.vram_size_mb, 8),
 };
@@ -98,7 +107,7 @@ static void vga_isa_class_initfn(ObjectClass *klass, const void *data)
 
     dc->realize = vga_isa_realizefn;
     device_class_set_legacy_reset(dc, vga_isa_reset);
-    dc->vmsd = &vmstate_vga_common;
+    dc->vmsd = &vmstate_vga_isa;
     device_class_set_props(dc, vga_isa_properties);
     set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
 }
-- 
2.53.0

Re: [PATCH] hw/display/vga-isa: Fix migration of the isa-vga device

[Peter Maydell]

On Thu, 26 Mar 2026 at 11:35, Thomas Huth <thuth@redhat.com> wrote:
>
> From: Thomas Huth <thuth@redhat.com>
>
> QEMU currently crashes when migrating a guest that uses the
> isa-vga device as display. This happens because vga_isa_class_initfn()
> registers a vmsd for vmstate_vga_common that operates on VGACommonState.
> But the isa-vga device is derived from ISADevice, not from VGACommonState,
> so the migration code tries to fill in the data for VGACommonState to
> the memory that is a ISADevice instead, which is of cause causing trouble.
>
> We need an indirection here as it's also e.g. done in vga-pci.c, so
> that the migration data gets filled into the right location.
>
> While we're at it, also drop the "global_vmstate = true" here. Since
> migration was broken for this device during the last 15 years (!) anyway,
> we don't have to worry about maintaining backward compatibility with this
> switch for older versions of QEMU anymore.

That means the only remaining users of global_vmstate = true are:
 * TYPE_VGA_MMIO : used only by the MIPS Jazz board, which is not
   versioned, so we are OK with migration compat breaks
 * TYPE_ISA_CIRRUS_VGA

I don't suppose that migration is also currently broken for Cirrus ? :-)

-- PMM

Re: [PATCH] hw/display/vga-isa: Fix migration of the isa-vga device

[Thomas Huth]

On 26/03/2026 12.59, Peter Maydell wrote:
> On Thu, 26 Mar 2026 at 11:35, Thomas Huth <thuth@redhat.com> wrote:
>>
>> From: Thomas Huth <thuth@redhat.com>
>>
>> QEMU currently crashes when migrating a guest that uses the
>> isa-vga device as display. This happens because vga_isa_class_initfn()
>> registers a vmsd for vmstate_vga_common that operates on VGACommonState.
>> But the isa-vga device is derived from ISADevice, not from VGACommonState,
>> so the migration code tries to fill in the data for VGACommonState to
>> the memory that is a ISADevice instead, which is of cause causing trouble.
>>
>> We need an indirection here as it's also e.g. done in vga-pci.c, so
>> that the migration data gets filled into the right location.
>>
>> While we're at it, also drop the "global_vmstate = true" here. Since
>> migration was broken for this device during the last 15 years (!) anyway,
>> we don't have to worry about maintaining backward compatibility with this
>> switch for older versions of QEMU anymore.
> 
> That means the only remaining users of global_vmstate = true are:
>   * TYPE_VGA_MMIO : used only by the MIPS Jazz board, which is not
>     versioned, so we are OK with migration compat breaks
>   * TYPE_ISA_CIRRUS_VGA
> 
> I don't suppose that migration is also currently broken for Cirrus ? :-)

No, Cirrus seems to be fine. But I think we should likely do something like 
commit 1fcfdc435a3e25ab9037f6f7b8ab for the isa-cirrus-vga device, too, so 
we can at least get rid of this in a couple of years...

  Thomas

Re: [PATCH] hw/display/vga-isa: Fix migration of the isa-vga device

[Fabiano Rosas]

Thomas Huth <thuth@redhat.com> writes:

> From: Thomas Huth <thuth@redhat.com>
>
> QEMU currently crashes when migrating a guest that uses the
> isa-vga device as display. This happens because vga_isa_class_initfn()
> registers a vmsd for vmstate_vga_common that operates on VGACommonState.
> But the isa-vga device is derived from ISADevice, not from VGACommonState,
> so the migration code tries to fill in the data for VGACommonState to
> the memory that is a ISADevice instead, which is of cause causing trouble.
>
> We need an indirection here as it's also e.g. done in vga-pci.c, so
> that the migration data gets filled into the right location.
>
> While we're at it, also drop the "global_vmstate = true" here. Since
> migration was broken for this device during the last 15 years (!) anyway,
> we don't have to worry about maintaining backward compatibility with this
> switch for older versions of QEMU anymore.
>
> Fixes: 7435b791ca9 ("vga-isa: convert to qdev")
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/display/vga-isa.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/hw/display/vga-isa.c b/hw/display/vga-isa.c
> index 95d85ff69a5..5f55c884a1b 100644
> --- a/hw/display/vga-isa.c
> +++ b/hw/display/vga-isa.c
> @@ -32,6 +32,7 @@
>  #include "qemu/timer.h"
>  #include "hw/core/loader.h"
>  #include "hw/core/qdev-properties.h"
> +#include "migration/vmstate.h"
>  #include "ui/console.h"
>  #include "qom/object.h"
>  
> @@ -62,7 +63,6 @@ static void vga_isa_realizefn(DeviceState *dev, Error **errp)
>      MemoryRegion *vga_io_memory;
>      const MemoryRegionPortio *vga_ports, *vbe_ports;
>  
> -    s->global_vmstate = true;
>      if (!vga_common_init(s, OBJECT(dev), errp)) {
>          return;
>      }
> @@ -88,6 +88,15 @@ static void vga_isa_realizefn(DeviceState *dev, Error **errp)
>      rom_add_vga(VGABIOS_FILENAME);
>  }
>  
> +static const VMStateDescription vmstate_vga_isa = {
> +    .name = "vga-isa",
> +    .version_id = 1,
> +    .fields = (const VMStateField[]) {
> +        VMSTATE_STRUCT(state, ISAVGAState, 0, vmstate_vga_common, VGACommonState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const Property vga_isa_properties[] = {
>      DEFINE_PROP_UINT32("vgamem_mb", ISAVGAState, state.vram_size_mb, 8),
>  };
> @@ -98,7 +107,7 @@ static void vga_isa_class_initfn(ObjectClass *klass, const void *data)
>  
>      dc->realize = vga_isa_realizefn;
>      device_class_set_legacy_reset(dc, vga_isa_reset);
> -    dc->vmsd = &vmstate_vga_common;
> +    dc->vmsd = &vmstate_vga_isa;
>      device_class_set_props(dc, vga_isa_properties);
>      set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
>  }

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH] hw/display/vga-isa: Fix migration of the isa-vga device

[Peter Maydell]

On Thu, 26 Mar 2026 at 12:04, Thomas Huth <thuth@redhat.com> wrote:
>
> On 26/03/2026 12.59, Peter Maydell wrote:
> > On Thu, 26 Mar 2026 at 11:35, Thomas Huth <thuth@redhat.com> wrote:
> >>
> >> From: Thomas Huth <thuth@redhat.com>
> >>
> >> QEMU currently crashes when migrating a guest that uses the
> >> isa-vga device as display. This happens because vga_isa_class_initfn()
> >> registers a vmsd for vmstate_vga_common that operates on VGACommonState.
> >> But the isa-vga device is derived from ISADevice, not from VGACommonState,
> >> so the migration code tries to fill in the data for VGACommonState to
> >> the memory that is a ISADevice instead, which is of cause causing trouble.
> >>
> >> We need an indirection here as it's also e.g. done in vga-pci.c, so
> >> that the migration data gets filled into the right location.
> >>
> >> While we're at it, also drop the "global_vmstate = true" here. Since
> >> migration was broken for this device during the last 15 years (!) anyway,
> >> we don't have to worry about maintaining backward compatibility with this
> >> switch for older versions of QEMU anymore.
> >
> > That means the only remaining users of global_vmstate = true are:
> >   * TYPE_VGA_MMIO : used only by the MIPS Jazz board, which is not
> >     versioned, so we are OK with migration compat breaks
> >   * TYPE_ISA_CIRRUS_VGA
> >
> > I don't suppose that migration is also currently broken for Cirrus ? :-)
>
> No, Cirrus seems to be fine. But I think we should likely do something like
> commit 1fcfdc435a3e25ab9037f6f7b8ab for the isa-cirrus-vga device, too, so
> we can at least get rid of this in a couple of years...

If we're going to do that it would be nice to get that in for 11.0.

I do notice that the Cirrus devices have their own vmstate handle
saving and restoring all the common VGA state, instead of using
vmstate_vga_common, and they miss out all the vbe entries at
the bottom...

-- PMM