Re: [Qemu-devel] [PATCH 2/3] vga: Add endian control register

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3908 bytes --]

On Tue, Sep 23, 2014 at 02:30:56PM +0200, Gerd Hoffmann wrote:
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> Include the endian state in the migration stream as an optional
> subsection which we only include when the endian isn't the default,
> thus enabling backward compatibility of the common case.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> Changes by kraxel:
>  * Remove bochs dispi interface changes.  We'll do that in
>    a different way to make sure we don't conflict with
>    possible future bochs dispi interface changes.
>  * keep live migration bits.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/display/vga.c     | 41 +++++++++++++++++++++++++++++++++++++----
>  hw/display/vga_int.h |  4 +++-
>  2 files changed, 40 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/display/vga.c b/hw/display/vga.c
> index 3f02984..fd16806 100644
> --- a/hw/display/vga.c
> +++ b/hw/display/vga.c
> @@ -1508,7 +1508,8 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
>      if (s->line_offset != s->last_line_offset ||
>          disp_width != s->last_width ||
>          height != s->last_height ||
> -        s->last_depth != depth) {
> +        s->last_depth != depth ||
> +        s->last_byteswap != byteswap) {
>          if (depth == 32 || (depth == 16 && !byteswap)) {
>              pixman_format_code_t format =
>                  qemu_default_pixman_format(depth, !byteswap);
> @@ -1526,6 +1527,7 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
>          s->last_height = height;
>          s->last_line_offset = s->line_offset;
>          s->last_depth = depth;
> +        s->last_byteswap = byteswap;
>          full_update = 1;
>      } else if (is_buffer_shared(surface) &&
>                 (full_update || surface_data(surface) != s->vram_ptr
> @@ -1789,6 +1791,7 @@ void vga_common_reset(VGACommonState *s)
>      s->cursor_start = 0;
>      s->cursor_end = 0;
>      s->cursor_offset = 0;
> +    s->big_endian_fb = s->default_endian_fb;
>      memset(s->invalidated_y_table, '\0', sizeof(s->invalidated_y_table));
>      memset(s->last_palette, '\0', sizeof(s->last_palette));
>      memset(s->last_ch_attr, '\0', sizeof(s->last_ch_attr));
> @@ -2020,6 +2023,28 @@ static int vga_common_post_load(void *opaque, int version_id)
>      return 0;
>  }
>  
> +static bool vga_endian_state_needed(void *opaque)
> +{
> +    VGACommonState *s = opaque;
> +
> +    /*
> +     * Only send the endian state if it's different from the
> +     * default one, thus ensuring backward compatibility for
> +     * migration of the common case
> +     */
> +    return s->default_endian_fb != s->big_endian_fb;
> +}
> +
> +const VMStateDescription vmstate_vga_endian = {
> +    .name = "vga.endian",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8_EQUAL(big_endian_fb, VGACommonState),

I don't think this is right.  If I'm remembering how the vmstate
macros work, this will abort if big_endian_fb isn't equal on source
and destination.  Which means as soon as it's possible to actually
change the big_endian_fb value, migrations will start failing/

I think this should instead be VMSTATE_BOOL()

[snip]
> -    bool big_endian_fb;
> +    uint8_t big_endian_fb;
> +    uint8_t default_endian_fb;
>      /* hardware mouse cursor support */
>      uint32_t invalidated_y_table[VGA_MAX_HEIGHT / 32];
>      void (*cursor_invalidate)(struct VGACommonState *s);

In which case you also don't need to change the type of the
big_endian_fb field.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]