[v2,3/5] ramfb: implement migration support

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Implementing RAMFB migration is quite straightforward. One caveat is to
treat the whole RAMFBCfg as a blob, since that's what is exposed to the
guest directly. This avoid having to fiddle with endianness issues if we
were to migrate fields individually as integers.

The following patches turns the migration only on machine >= 8.2.

Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=1859424

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/display/ramfb.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

Hi

On Mon, Oct 2, 2023 at 3:12 PM <marcandre.lureau@redhat.com> wrote:
>
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Implementing RAMFB migration is quite straightforward. One caveat is to
> treat the whole RAMFBCfg as a blob, since that's what is exposed to the
> guest directly. This avoid having to fiddle with endianness issues if we
> were to migrate fields individually as integers.
>
> The following patches turns the migration only on machine >= 8.2.
>
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=1859424
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/display/ramfb.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
> index 79b9754a58..4aaaa7d653 100644
> --- a/hw/display/ramfb.c
> +++ b/hw/display/ramfb.c
> @@ -12,6 +12,7 @@
>   */
>
>  #include "qemu/osdep.h"
> +#include "migration/vmstate.h"
>  #include "qapi/error.h"
>  #include "hw/loader.h"
>  #include "hw/display/ramfb.h"
> @@ -28,6 +29,8 @@ struct QEMU_PACKED RAMFBCfg {
>      uint32_t stride;
>  };
>
> +typedef struct RAMFBCfg RAMFBCfg;
> +
>  struct RAMFBState {
>      DisplaySurface *ds;
>      uint32_t width, height;
> @@ -115,6 +118,23 @@ void ramfb_display_update(QemuConsole *con, RAMFBState *s)
>      dpy_gfx_update_full(con);
>  }
>
> +static int ramfb_post_load(void *opaque, int version_id)
> +{
> +    ramfb_fw_cfg_write(opaque, 0, 0);
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_ramfb = {
> +    .name = "ramfb",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .post_load = ramfb_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_BUFFER_UNSAFE(cfg, RAMFBState, 0, sizeof(RAMFBCfg)),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  RAMFBState *ramfb_setup(Error **errp)
>  {
>      FWCfgState *fw_cfg = fw_cfg_find();
> @@ -127,6 +147,7 @@ RAMFBState *ramfb_setup(Error **errp)
>
>      s = g_new0(RAMFBState, 1);
>
> +    vmstate_register(NULL, 0, &vmstate_ramfb, s);

wip:
I am going to make it attached to the actual device.

On 10/2/23 14:01, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Oct 2, 2023 at 3:12 PM <marcandre.lureau@redhat.com> wrote:
>>
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> Implementing RAMFB migration is quite straightforward. One caveat is to
>> treat the whole RAMFBCfg as a blob, since that's what is exposed to the
>> guest directly. This avoid having to fiddle with endianness issues if we
>> were to migrate fields individually as integers.
>>
>> The following patches turns the migration only on machine >= 8.2.
>>
>> Fixes:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1859424
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  hw/display/ramfb.c | 21 +++++++++++++++++++++
>>  1 file changed, 21 insertions(+)
>>
>> diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
>> index 79b9754a58..4aaaa7d653 100644
>> --- a/hw/display/ramfb.c
>> +++ b/hw/display/ramfb.c
>> @@ -12,6 +12,7 @@
>>   */
>>
>>  #include "qemu/osdep.h"
>> +#include "migration/vmstate.h"
>>  #include "qapi/error.h"
>>  #include "hw/loader.h"
>>  #include "hw/display/ramfb.h"
>> @@ -28,6 +29,8 @@ struct QEMU_PACKED RAMFBCfg {
>>      uint32_t stride;
>>  };
>>
>> +typedef struct RAMFBCfg RAMFBCfg;
>> +
>>  struct RAMFBState {
>>      DisplaySurface *ds;
>>      uint32_t width, height;
>> @@ -115,6 +118,23 @@ void ramfb_display_update(QemuConsole *con, RAMFBState *s)
>>      dpy_gfx_update_full(con);
>>  }
>>
>> +static int ramfb_post_load(void *opaque, int version_id)
>> +{
>> +    ramfb_fw_cfg_write(opaque, 0, 0);
>> +    return 0;
>> +}
>> +
>> +static const VMStateDescription vmstate_ramfb = {
>> +    .name = "ramfb",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .post_load = ramfb_post_load,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_BUFFER_UNSAFE(cfg, RAMFBState, 0, sizeof(RAMFBCfg)),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>>  RAMFBState *ramfb_setup(Error **errp)
>>  {
>>      FWCfgState *fw_cfg = fw_cfg_find();
>> @@ -127,6 +147,7 @@ RAMFBState *ramfb_setup(Error **errp)
>>
>>      s = g_new0(RAMFBState, 1);
>>
>> +    vmstate_register(NULL, 0, &vmstate_ramfb, s);
> 
> wip:
> I am going to make it attached to the actual device.

I'm really curious about that -- I think it's going to be better, and
it'll teach me stuff about migration!

Laszlo

On 10/2/23 13:11, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Implementing RAMFB migration is quite straightforward. One caveat is to
> treat the whole RAMFBCfg as a blob, since that's what is exposed to the
> guest directly. This avoid having to fiddle with endianness issues if we
> were to migrate fields individually as integers.
> 
> The following patches turns the migration only on machine >= 8.2.
> 
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=1859424
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/display/ramfb.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
> index 79b9754a58..4aaaa7d653 100644
> --- a/hw/display/ramfb.c
> +++ b/hw/display/ramfb.c
> @@ -12,6 +12,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "migration/vmstate.h"
>  #include "qapi/error.h"
>  #include "hw/loader.h"
>  #include "hw/display/ramfb.h"
> @@ -28,6 +29,8 @@ struct QEMU_PACKED RAMFBCfg {
>      uint32_t stride;
>  };
>  
> +typedef struct RAMFBCfg RAMFBCfg;
> +
>  struct RAMFBState {
>      DisplaySurface *ds;
>      uint32_t width, height;
> @@ -115,6 +118,23 @@ void ramfb_display_update(QemuConsole *con, RAMFBState *s)
>      dpy_gfx_update_full(con);
>  }
>  
> +static int ramfb_post_load(void *opaque, int version_id)
> +{
> +    ramfb_fw_cfg_write(opaque, 0, 0);
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_ramfb = {
> +    .name = "ramfb",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .post_load = ramfb_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_BUFFER_UNSAFE(cfg, RAMFBState, 0, sizeof(RAMFBCfg)),

I just couldn't figure out, from code review, why VMSTATE_BUFFER would
not work here. So I applied your patches, changed this like follows:

diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
index 077fd2fa2c31..04bf01059994 100644
--- a/hw/display/ramfb.c
+++ b/hw/display/ramfb.c
@@ -131,7 +131,7 @@ static const VMStateDescription vmstate_ramfb = {
     .minimum_version_id = 1,
     .post_load = ramfb_post_load,
     .fields = (VMStateField[]) {
-        VMSTATE_BUFFER_UNSAFE(cfg, RAMFBState, 0, sizeof(RAMFBCfg)),
+        VMSTATE_BUFFER(cfg, RAMFBState),
         VMSTATE_END_OF_LIST()
     }
 };

and tried to build it.

I got a wall of error messages about cryptic macro nesting.

I'm quite annoyed that nearly none of the VMSTATE_ macros are
documented; even git-blame tends to be unhelpful. Ultimately though,
there was one useful bit in the wall of error messages: the error was
related to "type_check_array".

Upon reviewing type_check_array, my impression is that VMSTATE_BUFFER is
suitable only for *array fields*. I randomly picked an existent example,
namely

static const VMStateDescription bulk_in_vmstate = {
    .name = "CCID BulkIn state",
    .version_id = 1,
    .minimum_version_id = 1,
    .fields = (VMStateField[]) {
        VMSTATE_BUFFER(data, BulkIn),
        VMSTATE_UINT32(len, BulkIn),
        VMSTATE_UINT32(pos, BulkIn),
        VMSTATE_END_OF_LIST()
    }
};

from "hw/usb/dev-smartcard-reader.c", and sure enough, "data" is an
array field in BulkIn:

typedef struct BulkIn {
    uint8_t  data[BULK_IN_BUF_SIZE];
    uint32_t len;
    uint32_t pos;
} BulkIn;

So that's the reason.

Again, annoying lack of documentation, but I agree that your
VMSTATE_BUFFER_UNSAFE application is judicious.

Thanks!
Laszlo



> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  RAMFBState *ramfb_setup(Error **errp)
>  {
>      FWCfgState *fw_cfg = fw_cfg_find();
> @@ -127,6 +147,7 @@ RAMFBState *ramfb_setup(Error **errp)
>  
>      s = g_new0(RAMFBState, 1);
>  
> +    vmstate_register(NULL, 0, &vmstate_ramfb, s);
>      rom_add_vga("vgabios-ramfb.bin");
>      fw_cfg_add_file_callback(fw_cfg, "etc/ramfb",
>                               NULL, ramfb_fw_cfg_write, s,