* [Qemu-devel] [PATCH] wdt_i6300esb: fix vmstate versioning
@ 2013-05-21 22:32 Michael Roth
2013-05-22 8:25 ` Laszlo Ersek
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Michael Roth @ 2013-05-21 22:32 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, qemu-stable, nick
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.
+ *
+ * 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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] wdt_i6300esb: fix vmstate versioning
2013-05-21 22:32 [Qemu-devel] [PATCH] wdt_i6300esb: fix vmstate versioning Michael Roth
@ 2013-05-22 8:25 ` Laszlo Ersek
2013-05-23 11:19 ` Amit Shah
2013-06-12 20:11 ` mdroth
2 siblings, 0 replies; 8+ messages in thread
From: Laszlo Ersek @ 2013-05-22 8:25 UTC (permalink / raw)
To: Michael Roth; +Cc: peter.maydell, nick, qemu-devel, qemu-stable
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] wdt_i6300esb: fix vmstate versioning
2013-05-21 22:32 [Qemu-devel] [PATCH] wdt_i6300esb: fix vmstate versioning Michael Roth
2013-05-22 8:25 ` Laszlo Ersek
@ 2013-05-23 11:19 ` Amit Shah
2013-06-12 20:11 ` mdroth
2 siblings, 0 replies; 8+ messages in thread
From: Amit Shah @ 2013-05-23 11:19 UTC (permalink / raw)
To: Michael Roth; +Cc: peter.maydell, nick, qemu-devel, qemu-stable
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] wdt_i6300esb: fix vmstate versioning
2013-05-21 22:32 [Qemu-devel] [PATCH] wdt_i6300esb: fix vmstate versioning Michael Roth
2013-05-22 8:25 ` Laszlo Ersek
2013-05-23 11:19 ` Amit Shah
@ 2013-06-12 20:11 ` mdroth
2013-06-12 20:42 ` Peter Maydell
2013-06-12 21:17 ` Anthony Liguori
2 siblings, 2 replies; 8+ messages in thread
From: mdroth @ 2013-06-12 20:11 UTC (permalink / raw)
To: qemu-trivial; +Cc: qemu-devel, peter.maydell, qemu-stable, nick
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] wdt_i6300esb: fix vmstate versioning
2013-06-12 20:11 ` mdroth
@ 2013-06-12 20:42 ` Peter Maydell
2013-06-12 21:07 ` mdroth
2013-06-12 21:17 ` Anthony Liguori
1 sibling, 1 reply; 8+ messages in thread
From: Peter Maydell @ 2013-06-12 20:42 UTC (permalink / raw)
To: mdroth; +Cc: qemu-trivial, qemu-devel, Anthony Liguori, qemu-stable, nick
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] wdt_i6300esb: fix vmstate versioning
2013-06-12 20:42 ` Peter Maydell
@ 2013-06-12 21:07 ` mdroth
0 siblings, 0 replies; 8+ messages in thread
From: mdroth @ 2013-06-12 21:07 UTC (permalink / raw)
To: Peter Maydell
Cc: Anthony Liguori, quintela, qemu-trivial, qemu-devel, qemu-stable, nick
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] wdt_i6300esb: fix vmstate versioning
2013-06-12 20:11 ` mdroth
2013-06-12 20:42 ` Peter Maydell
@ 2013-06-12 21:17 ` Anthony Liguori
2013-06-12 21:27 ` mdroth
1 sibling, 1 reply; 8+ messages in thread
From: Anthony Liguori @ 2013-06-12 21:17 UTC (permalink / raw)
To: mdroth, qemu-trivial; +Cc: peter.maydell, nick, qemu-devel, qemu-stable
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
>>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] wdt_i6300esb: fix vmstate versioning
2013-06-12 21:17 ` Anthony Liguori
@ 2013-06-12 21:27 ` mdroth
0 siblings, 0 replies; 8+ messages in thread
From: mdroth @ 2013-06-12 21:27 UTC (permalink / raw)
To: Anthony Liguori
Cc: qemu-trivial, peter.maydell, nick, qemu-devel, qemu-stable
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
> >>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-06-12 21:28 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-05-21 22:32 [Qemu-devel] [PATCH] wdt_i6300esb: fix vmstate versioning Michael Roth
2013-05-22 8:25 ` Laszlo Ersek
2013-05-23 11:19 ` Amit Shah
2013-06-12 20:11 ` mdroth
2013-06-12 20:42 ` Peter Maydell
2013-06-12 21:07 ` mdroth
2013-06-12 21:17 ` Anthony Liguori
2013-06-12 21:27 ` mdroth
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.