Re: acpi_piix4 migration issue

[Paolo Bonzini <pbonzini@redhat.com>]

Il 28/10/2012 20:40, Marcelo Tosatti ha scritto:
> 
> qemu-kvm 1.2 -> qemu-1.3 migration fails with
> 
> Unknown savevm section type 48
> load of migration failed
> 
> Due to a fix in acpi_piix4 in qemu-kvm (attached at the end of the
> message). 
> 
> The problem is that qemu-kvm correctly uses 2 bytes for sts and 
> 2 bytes for en fields (which is their allocated size), while qemu 
> uses 4*2 bytes for each.
> 
> The fix present in qemu-kvm is correct, but, having it in qemu 1.3 would break
> qemu 1.2 -> qemu 1.3 migration (while allowing qemu-kvm 1.2 -> qemu 1.3
> migration).
> 
> Any opinions on what to do?

Bump the .version_id and .minimum_version_id to 2 and load the QEMU 1.2
state via .load_state_old.

qemu-kvm 1.2 -> qemu 1.3 migration would be broken.  qemu-kvm
downstreams that care can leave .minimum_version_id to 1.

Paolo

>     >>
>     >> +#define VMSTATE_GPE_ARRAY(_field, _state)                            \
>     >> + {                                                                   \
>     >> +     .name       = (stringify(_field)),                              \
>     >> +     .version_id = 0,                                                \
>     >> +     .num        = GPE_LEN,                                          \
>     >> +     .info       =&vmstate_info_uint16,                             \
>     >> +     .size       = sizeof(uint16_t),                                 \
>     >> +     .flags      = VMS_ARRAY | VMS_POINTER,                          \
>     >> +     .offset     = vmstate_offset_pointer(_state, _field, uint8_t),  \
>     >> + }
>     >> +
>     >>   static const VMStateDescription vmstate_gpe = {
>     >>       .name = "gpe",
>     >>       .version_id = 1,
>     >>       .minimum_version_id = 1,
>     >>       .minimum_version_id_old = 1,
>     >>       .fields      = (VMStateField []) {
>     >> -        VMSTATE_UINT16(sts, struct gpe_regs),
>     >> -        VMSTATE_UINT16(en, struct gpe_regs),
>     >> +        VMSTATE_GPE_ARRAY(sts, ACPIGPE),
>     >> +        VMSTATE_GPE_ARRAY(en, ACPIGPE),
>     >>           VMSTATE_END_OF_LIST()
>     >>       }
>     >>   };
>     >
>     > I'm no vmstate expert, but this does look odd.  Why both VMS_ARRAY and
>     > VMS_POINTER? aren't we trying to save/restore a simple 16-bit value?  Or
>     > at least we did before this patch.
>     
>     That's right. the difference is, the new member type became uint8_t*.
>     Does the following help?
>     
>     Signed-off-by: Avi Kivity <avi@redhat.com>
> 
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index d65a7e9..9dc6f43 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -221,10 +221,9 @@ static int vmstate_acpi_post_load(void *opaque, int version_id)
>   {                                                                   \
>       .name       = (stringify(_field)),                              \
>       .version_id = 0,                                                \
> -     .num        = GPE_LEN,                                          \
>       .info       = &vmstate_info_uint16,                             \
>       .size       = sizeof(uint16_t),                                 \
> -     .flags      = VMS_ARRAY | VMS_POINTER,                          \
> +     .flags      = VMS_SINGLE | VMS_POINTER,                         \
>       .offset     = vmstate_offset_pointer(_state, _field, uint8_t),  \
>   }
>  
> 
> 
> 
>