[PATCH 0/2] g364fb: fix migration (or: fix migration for MIPS magnum machines)

[PATCH 0/2] g364fb: fix migration (or: fix migration for MIPS magnum machines)

[Mark Cave-Ayland]

I noticed whilst testing the previous dp8393x patchset that I would always
get a segfault whilst attempting to migrate the MIPS magnum machine.

A bit of detective work shows that the problem is an incorrect VMStateDescription
in the g364fb device which expects a G364State but instead receives a
G364SysBusState.

Looking back through the git history suggests that migration for this device
(and also the MIPS magnum machines) has been broken for several years, so patch
1 takes the opportunity to improve the migration stream for the framebuffer
first whilst patch 2 contains the actual fix.

Note that I don't use the MIPS magnum machines on a regular basis but the
patchset fixes the migration error and survives some light testing here.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


Mark Cave-Ayland (2):
  g364fb: use RAM memory region for framebuffer
  g364fb: add VMStateDescription for G364SysBusState

 hw/display/g364fb.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

-- 
2.20.1

[PATCH 1/2] g364fb: use RAM memory region for framebuffer

[Mark Cave-Ayland]

Since the migration stream is already broken, we can use this opportunity to
change the framebuffer so that it is migrated as a RAM memory region rather
than as an array of bytes.

In particular this helps the output of the analyze-migration.py tool which
no longer contains a huge array representing the framebuffer contents.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/g364fb.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/hw/display/g364fb.c b/hw/display/g364fb.c
index 8f1725432c..163d7f5391 100644
--- a/hw/display/g364fb.c
+++ b/hw/display/g364fb.c
@@ -22,6 +22,7 @@
 #include "hw/hw.h"
 #include "hw/irq.h"
 #include "hw/qdev-properties.h"
+#include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "qemu/module.h"
 #include "ui/console.h"
@@ -125,7 +126,7 @@ static void g364fb_draw_graphic8(G364State *s)
         xcursor = ycursor = -65;
     }
 
-    vram = s->vram + s->top_of_screen;
+    vram = memory_region_get_ram_ptr(&s->mem_vram) + s->top_of_screen;
     /* XXX: out of range in vram? */
     data_display = dd = surface_data(surface);
     snap = memory_region_snapshot_and_clear_dirty(&s->mem_vram, 0, s->vram_size,
@@ -274,6 +275,8 @@ static inline void g364fb_invalidate_display(void *opaque)
 
 static void g364fb_reset(G364State *s)
 {
+    uint8_t *vram = memory_region_get_ram_ptr(&s->mem_vram);
+
     qemu_irq_lower(s->irq);
 
     memset(s->color_palette, 0, sizeof(s->color_palette));
@@ -283,7 +286,7 @@ static void g364fb_reset(G364State *s)
     s->ctla = 0;
     s->top_of_screen = 0;
     s->width = s->height = 0;
-    memset(s->vram, 0, s->vram_size);
+    memset(vram, 0, s->vram_size);
     g364fb_invalidate_display(s);
 }
 
@@ -454,7 +457,6 @@ static const VMStateDescription vmstate_g364fb = {
     .minimum_version_id = 1,
     .post_load = g364fb_post_load,
     .fields = (VMStateField[]) {
-        VMSTATE_VBUFFER_UINT32(vram, G364State, 1, NULL, vram_size),
         VMSTATE_BUFFER_UNSAFE(color_palette, G364State, 0, 256 * 3),
         VMSTATE_BUFFER_UNSAFE(cursor_palette, G364State, 0, 9),
         VMSTATE_UINT16_ARRAY(cursor, G364State, 512),
@@ -474,15 +476,12 @@ static const GraphicHwOps g364fb_ops = {
 
 static void g364fb_init(DeviceState *dev, G364State *s)
 {
-    s->vram = g_malloc0(s->vram_size);
-
     s->con = graphic_console_init(dev, 0, &g364fb_ops, s);
 
     memory_region_init_io(&s->mem_ctrl, OBJECT(dev), &g364fb_ctrl_ops, s,
                           "ctrl", 0x180000);
-    memory_region_init_ram_ptr(&s->mem_vram, NULL, "vram",
-                               s->vram_size, s->vram);
-    vmstate_register_ram(&s->mem_vram, dev);
+    memory_region_init_ram(&s->mem_vram, NULL, "g364fb.vram", s->vram_size,
+                           &error_fatal);
     memory_region_set_log(&s->mem_vram, true, DIRTY_MEMORY_VGA);
 }
 
-- 
2.20.1

[PATCH 2/2] g364fb: add VMStateDescription for G364SysBusState

[Mark Cave-Ayland]

Currently when QEMU attempts to migrate the MIPS magnum machine it crashes due
to a mistake in the g364fb VMStateDescription configuration which expects a
G364SysBusState and not a G364State.

Resolve the issue by adding a new VMStateDescription for G364SysBusState and
embedding the existing vmstate_g364fb VMStateDescription inside it using
VMSTATE_STRUCT.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/g364fb.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/hw/display/g364fb.c b/hw/display/g364fb.c
index 163d7f5391..990ef3afdd 100644
--- a/hw/display/g364fb.c
+++ b/hw/display/g364fb.c
@@ -518,6 +518,16 @@ static Property g364fb_sysbus_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+static const VMStateDescription vmstate_g364fb_sysbus = {
+    .name = "g364fb-sysbus",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_STRUCT(g364, G364SysBusState, 1, vmstate_g364fb, G364State),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void g364fb_sysbus_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -526,7 +536,7 @@ static void g364fb_sysbus_class_init(ObjectClass *klass, void *data)
     set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
     dc->desc = "G364 framebuffer";
     dc->reset = g364fb_sysbus_reset;
-    dc->vmsd = &vmstate_g364fb;
+    dc->vmsd = &vmstate_g364fb_sysbus;
     device_class_set_props(dc, g364fb_sysbus_properties);
 }
 
-- 
2.20.1

Re: [PATCH 1/2] g364fb: use RAM memory region for framebuffer

[Philippe Mathieu-Daudé]

On 6/25/21 9:38 AM, Mark Cave-Ayland wrote:
> Since the migration stream is already broken, we can use this opportunity to
> change the framebuffer so that it is migrated as a RAM memory region rather
> than as an array of bytes.
> 
> In particular this helps the output of the analyze-migration.py tool which
> no longer contains a huge array representing the framebuffer contents.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/display/g364fb.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)

> @@ -454,7 +457,6 @@ static const VMStateDescription vmstate_g364fb = {
>      .minimum_version_id = 1,

Even if broken, I'd increase to version as good practice.

>      .post_load = g364fb_post_load,
>      .fields = (VMStateField[]) {
> -        VMSTATE_VBUFFER_UINT32(vram, G364State, 1, NULL, vram_size),
>          VMSTATE_BUFFER_UNSAFE(color_palette, G364State, 0, 256 * 3),
>          VMSTATE_BUFFER_UNSAFE(cursor_palette, G364State, 0, 9),
>          VMSTATE_UINT16_ARRAY(cursor, G364State, 512),

The vram pointer is now unused, we can remove it:

-- >8 --
diff --git a/hw/display/g364fb.c b/hw/display/g364fb.c
index 990ef3afdd8..11ad61fa73a 100644
--- a/hw/display/g364fb.c
+++ b/hw/display/g364fb.c
@@ -34,7 +34,6 @@

 typedef struct G364State {
     /* hardware */
-    uint8_t *vram;
     uint32_t vram_size;
     qemu_irq irq;
     MemoryRegion mem_vram;
---

Removing 'uint8_t *vram':
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 2/2] g364fb: add VMStateDescription for G364SysBusState

[Philippe Mathieu-Daudé]

On 6/25/21 9:38 AM, Mark Cave-Ayland wrote:
> Currently when QEMU attempts to migrate the MIPS magnum machine it crashes due
> to a mistake in the g364fb VMStateDescription configuration which expects a
> G364SysBusState and not a G364State.
> 
> Resolve the issue by adding a new VMStateDescription for G364SysBusState and
> embedding the existing vmstate_g364fb VMStateDescription inside it using
> VMSTATE_STRUCT.

Broken since 97a3f6ffbba ("g364fb: convert to qdev")?

> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/display/g364fb.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/display/g364fb.c b/hw/display/g364fb.c
> index 163d7f5391..990ef3afdd 100644
> --- a/hw/display/g364fb.c
> +++ b/hw/display/g364fb.c
> @@ -518,6 +518,16 @@ static Property g364fb_sysbus_properties[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> +static const VMStateDescription vmstate_g364fb_sysbus = {
> +    .name = "g364fb-sysbus",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_STRUCT(g364, G364SysBusState, 1, vmstate_g364fb, G364State),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static void g364fb_sysbus_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -526,7 +536,7 @@ static void g364fb_sysbus_class_init(ObjectClass *klass, void *data)
>      set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
>      dc->desc = "G364 framebuffer";
>      dc->reset = g364fb_sysbus_reset;
> -    dc->vmsd = &vmstate_g364fb;
> +    dc->vmsd = &vmstate_g364fb_sysbus;
>      device_class_set_props(dc, g364fb_sysbus_properties);
>  }
>  
> 

Re: [PATCH 1/2] g364fb: use RAM memory region for framebuffer

[Mark Cave-Ayland]

On 25/06/2021 09:37, Philippe Mathieu-Daudé wrote:

> On 6/25/21 9:38 AM, Mark Cave-Ayland wrote:
>> Since the migration stream is already broken, we can use this opportunity to
>> change the framebuffer so that it is migrated as a RAM memory region rather
>> than as an array of bytes.
>>
>> In particular this helps the output of the analyze-migration.py tool which
>> no longer contains a huge array representing the framebuffer contents.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/display/g364fb.c | 15 +++++++--------
>>   1 file changed, 7 insertions(+), 8 deletions(-)
> 
>> @@ -454,7 +457,6 @@ static const VMStateDescription vmstate_g364fb = {
>>       .minimum_version_id = 1,
> 
> Even if broken, I'd increase to version as good practice.

Sure - I'll go ahead and do that for a v2. The reason I wasn't too worried was 
because since the wrong struct is used for saving data, the outgoing stream is 
corrupted so all bets are off on being able to reload it(!).

>>       .post_load = g364fb_post_load,
>>       .fields = (VMStateField[]) {
>> -        VMSTATE_VBUFFER_UINT32(vram, G364State, 1, NULL, vram_size),
>>           VMSTATE_BUFFER_UNSAFE(color_palette, G364State, 0, 256 * 3),
>>           VMSTATE_BUFFER_UNSAFE(cursor_palette, G364State, 0, 9),
>>           VMSTATE_UINT16_ARRAY(cursor, G364State, 512),
> 
> The vram pointer is now unused, we can remove it:

Indeed. I remember thinking I had to do this, but clearly forgot. Will fix.

> -- >8 --
> diff --git a/hw/display/g364fb.c b/hw/display/g364fb.c
> index 990ef3afdd8..11ad61fa73a 100644
> --- a/hw/display/g364fb.c
> +++ b/hw/display/g364fb.c
> @@ -34,7 +34,6 @@
> 
>   typedef struct G364State {
>       /* hardware */
> -    uint8_t *vram;
>       uint32_t vram_size;
>       qemu_irq irq;
>       MemoryRegion mem_vram;
> ---
> 
> Removing 'uint8_t *vram':
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>


ATB,

Mark.

Re: [PATCH 2/2] g364fb: add VMStateDescription for G364SysBusState

[Mark Cave-Ayland]

On 25/06/2021 09:44, Philippe Mathieu-Daudé wrote:

> On 6/25/21 9:38 AM, Mark Cave-Ayland wrote:
>> Currently when QEMU attempts to migrate the MIPS magnum machine it crashes due
>> to a mistake in the g364fb VMStateDescription configuration which expects a
>> G364SysBusState and not a G364State.
>>
>> Resolve the issue by adding a new VMStateDescription for G364SysBusState and
>> embedding the existing vmstate_g364fb VMStateDescription inside it using
>> VMSTATE_STRUCT.
> 
> Broken since 97a3f6ffbba ("g364fb: convert to qdev")?

(goes and looks)

Wow that does appear to be correct - I gave up looking when I got to 8 years ago ;)
But yes, the bug is introduced by that commit: before you can see that 
register_savevm() was called with a G364State opaque whereas after the switch to QOM 
object the object reference for the VMStateDescription becomes G364SysBusState instead.

I'll add a "Fixes:" tag and send again.

>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/display/g364fb.c | 12 +++++++++++-
>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/display/g364fb.c b/hw/display/g364fb.c
>> index 163d7f5391..990ef3afdd 100644
>> --- a/hw/display/g364fb.c
>> +++ b/hw/display/g364fb.c
>> @@ -518,6 +518,16 @@ static Property g364fb_sysbus_properties[] = {
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>   
>> +static const VMStateDescription vmstate_g364fb_sysbus = {
>> +    .name = "g364fb-sysbus",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_STRUCT(g364, G364SysBusState, 1, vmstate_g364fb, G364State),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>>   static void g364fb_sysbus_class_init(ObjectClass *klass, void *data)
>>   {
>>       DeviceClass *dc = DEVICE_CLASS(klass);
>> @@ -526,7 +536,7 @@ static void g364fb_sysbus_class_init(ObjectClass *klass, void *data)
>>       set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
>>       dc->desc = "G364 framebuffer";
>>       dc->reset = g364fb_sysbus_reset;
>> -    dc->vmsd = &vmstate_g364fb;
>> +    dc->vmsd = &vmstate_g364fb_sysbus;
>>       device_class_set_props(dc, g364fb_sysbus_properties);
>>   }
>>   
>>


ATB,

Mark.

Re: [PATCH 0/2] g364fb: fix migration (or: fix migration for MIPS magnum machines)

[Mark Cave-Ayland]

On 25/06/2021 08:38, Mark Cave-Ayland wrote:

> I noticed whilst testing the previous dp8393x patchset that I would always
> get a segfault whilst attempting to migrate the MIPS magnum machine.
> 
> A bit of detective work shows that the problem is an incorrect VMStateDescription
> in the g364fb device which expects a G364State but instead receives a
> G364SysBusState.
> 
> Looking back through the git history suggests that migration for this device
> (and also the MIPS magnum machines) has been broken for several years, so patch
> 1 takes the opportunity to improve the migration stream for the framebuffer
> first whilst patch 2 contains the actual fix.
> 
> Note that I don't use the MIPS magnum machines on a regular basis but the
> patchset fixes the migration error and survives some light testing here.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> 
> Mark Cave-Ayland (2):
>    g364fb: use RAM memory region for framebuffer
>    g364fb: add VMStateDescription for G364SysBusState
> 
>   hw/display/g364fb.c | 27 ++++++++++++++++++---------
>   1 file changed, 18 insertions(+), 9 deletions(-)

Thanks for the review Phil, I've just sent v2 on its way. Does it make sense for this 
to be merged via the MIPS tree?


ATB,

Mark.

Re: [PATCH 0/2] g364fb: fix migration (or: fix migration for MIPS magnum machines)

[Philippe Mathieu-Daudé]

On 6/25/21 6:37 PM, Mark Cave-Ayland wrote:
> On 25/06/2021 08:38, Mark Cave-Ayland wrote:
> 
>> I noticed whilst testing the previous dp8393x patchset that I would
>> always
>> get a segfault whilst attempting to migrate the MIPS magnum machine.
>>
>> A bit of detective work shows that the problem is an incorrect
>> VMStateDescription
>> in the g364fb device which expects a G364State but instead receives a
>> G364SysBusState.
>>
>> Looking back through the git history suggests that migration for this
>> device
>> (and also the MIPS magnum machines) has been broken for several years,
>> so patch
>> 1 takes the opportunity to improve the migration stream for the
>> framebuffer
>> first whilst patch 2 contains the actual fix.
>>
>> Note that I don't use the MIPS magnum machines on a regular basis but the
>> patchset fixes the migration error and survives some light testing here.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>
>>
>> Mark Cave-Ayland (2):
>>    g364fb: use RAM memory region for framebuffer
>>    g364fb: add VMStateDescription for G364SysBusState
>>
>>   hw/display/g364fb.c | 27 ++++++++++++++++++---------
>>   1 file changed, 18 insertions(+), 9 deletions(-)
> 
> Thanks for the review Phil, I've just sent v2 on its way. Does it make
> sense for this to be merged via the MIPS tree?

Sure, I was going to suggest that too. Gerd could still beats me
since it is hw/display/ ;) I plan to send another pull request
during next week.