| Message ID | 1369175577-18130-1-git-send-email-mdroth@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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
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
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 >
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
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 >
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 >>
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 > >> >
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),
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(-)