Re: [Qemu-stable] [Qemu-devel] [PATCH] wdt_i6300esb: fix vmstate versioning

[Anthony Liguori]

mdroth <address@hidden> writes:

> On Tue, May 21, 2013 at 05:32:57PM -0500, Michael Roth wrote:
>> When this VMSD was introduced it's version fields were set to
>> sizeof(I6300State), making them essentially random from build to build,
>> version to version.
>> 
>> To fix this, we lock in a high version id and low minimum version id to
>> support old->new migration from all prior versions of this device's
>> state. This should work since the device state has not changed since
>> its introduction.
>> 
>> The potentially breaks migration from 1.5+ to 1.5, but since the
>> versioning was essentially random prior to this patch, new->old
>> migration was not consistently functional to begin with.
>> 
>> Reported-by: Nicholas Thomas <address@hidden>
>> Suggested-by: Peter Maydell <address@hidden>
>> Cc: address@hidden
>> Signed-off-by: Michael Roth <address@hidden>
>
> CC'ing qemu-trivial. Looking to get this in for 1.5.1

v2?  There were some review comments that haven't been addressed.

Regards,

Anthony Liguori

>
>> ---
>>  hw/watchdog/wdt_i6300esb.c |   19 ++++++++++++++++---
>>  1 file changed, 16 insertions(+), 3 deletions(-)
>> 
>> diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c
>> index 1407fba..851b664 100644
>> --- a/hw/watchdog/wdt_i6300esb.c
>> +++ b/hw/watchdog/wdt_i6300esb.c
>> @@ -374,9 +374,22 @@ static const MemoryRegionOps i6300esb_ops = {
>>  
>>  static const VMStateDescription vmstate_i6300esb = {
>>      .name = "i6300esb_wdt",
>> -    .version_id = sizeof(I6300State),
>> -    .minimum_version_id = sizeof(I6300State),
>> -    .minimum_version_id_old = sizeof(I6300State),
>> +    /* With this VMSD's introduction, version_id/minimum_version_id were
>> +     * erroneously set to sizeof(I6300State), causing a somewhat random
>> +     * version_id to be set for every build. This eventually broke
>> +     * migration.
>> +     *
>> +     * To correct this without breaking old->new migration for older 
>> versions
>> +     * of QEMU, we've set version_id to a value high enough to exceed all 
>> past
>> +     * values of sizeof(I6300State) across various build environments, and 
>> have
>> +     * reset minimum_version_id_old/minimum_version_id to 1, since this VMSD
>> +     * has never changed and thus can except all past versions.
>> +     *
>> +     * For future changes we can treat these values as we normally would.
>> +     */
>> +    .version_id = 10000,
>> +    .minimum_version_id = 1,
>> +    .minimum_version_id_old = 1,
>>      .fields      = (VMStateField []) {
>>          VMSTATE_PCI_DEVICE(dev, I6300State),
>>          VMSTATE_INT32(reboot_enabled, I6300State),
>> -- 
>> 1.7.9.5
>>