Re: [Qemu-devel] [PATCH] wdt_i6300esb: fix vmstate versioning
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 n...@bytemark.co.uk
Suggested-by: Peter Maydell peter.mayd...@linaro.org
Cc: qemu-sta...@nongnu.org
Signed-off-by: Michael Roth mdr...@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 = 1,
+.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
Re: [Qemu-devel] [PATCH] wdt_i6300esb: fix vmstate versioning
On 12 June 2013 21:11, mdroth mdr...@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 n...@bytemark.co.uk Suggested-by: Peter Maydell peter.mayd...@linaro.org Cc: qemu-sta...@nongnu.org Signed-off-by: Michael Roth mdr...@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
Re: [Qemu-devel] [PATCH] wdt_i6300esb: fix vmstate versioning
On Wed, Jun 12, 2013 at 09:42:14PM +0100, Peter Maydell wrote: On 12 June 2013 21:11, mdroth mdr...@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 n...@bytemark.co.uk Suggested-by: Peter Maydell peter.mayd...@linaro.org Cc: qemu-sta...@nongnu.org Signed-off-by: Michael Roth mdr...@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
Re: [Qemu-devel] [PATCH] wdt_i6300esb: fix vmstate versioning
mdroth mdr...@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 n...@bytemark.co.uk
Suggested-by: Peter Maydell peter.mayd...@linaro.org
Cc: qemu-sta...@nongnu.org
Signed-off-by: Michael Roth mdr...@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 = 1,
+.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
Re: [Qemu-devel] [PATCH] wdt_i6300esb: fix vmstate versioning
On Wed, Jun 12, 2013 at 04:17:53PM -0500, Anthony Liguori wrote:
mdroth mdr...@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 n...@bytemark.co.uk
Suggested-by: Peter Maydell peter.mayd...@linaro.org
Cc: qemu-sta...@nongnu.org
Signed-off-by: Michael Roth mdr...@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 = 1,
+.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
Re: [Qemu-devel] [PATCH] wdt_i6300esb: fix vmstate versioning
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 n...@bytemark.co.uk Suggested-by: Peter Maydell peter.mayd...@linaro.org Cc: qemu-sta...@nongnu.org Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com Please fix the comment below per Laszlo's comment, and you can add: Reviewed-by: Amit Shah amit.s...@redhat.com Amit
Re: [Qemu-devel] [PATCH] wdt_i6300esb: fix vmstate versioning
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 n...@bytemark.co.uk
Suggested-by: Peter Maydell peter.mayd...@linaro.org
Cc: qemu-sta...@nongnu.org
Signed-off-by: Michael Roth mdr...@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 = 1,
+.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
[Qemu-devel] [PATCH] wdt_i6300esb: fix vmstate versioning
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 n...@bytemark.co.uk
Suggested-by: Peter Maydell peter.mayd...@linaro.org
Cc: qemu-sta...@nongnu.org
Signed-off-by: Michael Roth mdr...@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 = 1,
+.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
