Re: [PATCH v2 3/5] ramfb: implement migration support

[Laszlo Ersek <lersek@redhat.com>]

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,