Patchwork wdt_i6300esb: fix vmstate versioning

login
register
mail settings
Submitter Michael Roth
Date May 21, 2013, 10:32 p.m.
Message ID <1369175577-18130-1-git-send-email-mdroth@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/245429/
State New
Headers show

Comments

Michael Roth - May 21, 2013, 10:32 p.m.
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 <nick@bytemark.co.uk>
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-stable@nongnu.org
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 hw/watchdog/wdt_i6300esb.c |   19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)
Laszlo Ersek - May 22, 2013, 8:25 a.m.
On 05/22/13 00:32, 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 <nick@bytemark.co.uk>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  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.

As a non-native speaker I think you mean "accept".

> +     *
> +     * 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),
> 

Otherwise looks good to me (which may not mean much :))

Laszlo
Amit Shah - May 23, 2013, 11:19 a.m.
On (Tue) 21 May 2013 [17:32:57], 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 <nick@bytemark.co.uk>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

Please fix the comment below per Laszlo's comment, and you can add:

Reviewed-by: Amit Shah <amit.shah@redhat.com>

		Amit
Michael Roth - June 12, 2013, 8:11 p.m.
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 <nick@bytemark.co.uk>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

CC'ing qemu-trivial. Looking to get this in for 1.5.1

> ---
>  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
>
Peter Maydell - June 12, 2013, 8:42 p.m.
On 12 June 2013 21:11, mdroth <mdroth@linux.vnet.ibm.com> wrote:
> 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 <nick@bytemark.co.uk>
>> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>
> CC'ing qemu-trivial. Looking to get this in for 1.5.1

This is a good patch but it definitely doesn't seem like
-trivial material to me. -trivial isn't "way to get patches
in that would otherwise fall through the cracks" :-)

thanks
-- PMM
Michael Roth - June 12, 2013, 9:07 p.m.
On Wed, Jun 12, 2013 at 09:42:14PM +0100, Peter Maydell wrote:
> On 12 June 2013 21:11, mdroth <mdroth@linux.vnet.ibm.com> wrote:
> > 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 <nick@bytemark.co.uk>
> >> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> >> Cc: qemu-stable@nongnu.org
> >> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> >
> > CC'ing qemu-trivial. Looking to get this in for 1.5.1
> 
> This is a good patch but it definitely doesn't seem like
> -trivial material to me. -trivial isn't "way to get patches
> in that would otherwise fall through the cracks" :-)

I honestly thought it was trivial once all the considerations were
documented, but reading through it all again makes my head hurt a
bit so you're probably right.

Anthony, Juan?

> 
> thanks
> -- PMM
>
Anthony Liguori - June 12, 2013, 9:17 p.m.
mdroth <mdroth@linux.vnet.ibm.com> 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 <nick@bytemark.co.uk>
>> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>
> 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
>>
Michael Roth - June 12, 2013, 9:27 p.m.
On Wed, Jun 12, 2013 at 04:17:53PM -0500, Anthony Liguori wrote:
> mdroth <mdroth@linux.vnet.ibm.com> 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 <nick@bytemark.co.uk>
> >> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> >> Cc: qemu-stable@nongnu.org
> >> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> >
> > CC'ing qemu-trivial. Looking to get this in for 1.5.1
> 
> v2?  There were some review comments that haven't been addressed.

Sorry, pinged the wrong email. v2 is to the latest:

http://lists.nongnu.org/archive/html/qemu-devel/2013-05/msg02991.html

> 
> 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
> >> 
>

Patch

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),