| Message ID | 1369240371-21253-1-git-send-email-mdroth@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Wed, May 22, 2013 at 11:32:51AM -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> ACK. I guess no one uses watchdog much, or they don't try to migrate those guests :-( Rich.
On 05/22/13 18: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> > --- > v2: > * Fixed s/except/accept/ typo (Laszlo) > > hw/watchdog/wdt_i6300esb.c | 19 ++++++++++++++++--- > 1 file changed, 16 insertions(+), 3 deletions(-) Always alert to make a difference :), Reviewed-by: Laszlo Ersek <lersek@redhat.com>
On (Wed) 22 May 2013 [11:32:51], 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> > --- > v2: > * Fixed s/except/accept/ typo (Laszlo) Reviewed-by: Amit Shah <amit.shah@redhat.com> Amit
On Wed, May 22, 2013 at 11:32:51AM -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> Ping, looking to pull this in for 1.5.1 > --- > v2: > * Fixed s/except/accept/ typo (Laszlo) > > 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..05af0b1 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 accept 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 Tue, Jun 11, 2013 at 04:53:51PM -0500, mdroth wrote: > On Wed, May 22, 2013 at 11:32:51AM -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> > > Ping, looking to pull this in for 1.5.1 Anthony, Juan? Not sure if this is on your radar. Looking to get it applied prior to stable freeze tomorrow. > > > --- > > v2: > > * Fixed s/except/accept/ typo (Laszlo) > > > > 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..05af0b1 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 accept 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 > >
mdroth <mdroth@linux.vnet.ibm.com> writes: > On Tue, Jun 11, 2013 at 04:53:51PM -0500, mdroth wrote: >> On Wed, May 22, 2013 at 11:32:51AM -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> >> >> Ping, looking to pull this in for 1.5.1 > > Anthony, Juan? Not sure if this is on your radar. Looking to get it > applied prior to stable freeze tomorrow. I'll pick it up. Regards, Anthony Liguori > >> >> > --- >> > v2: >> > * Fixed s/except/accept/ typo (Laszlo) >> > >> > 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..05af0b1 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 accept 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 >> >
mdroth <mdroth@linux.vnet.ibm.com> wrote: > On Tue, Jun 11, 2013 at 04:53:51PM -0500, mdroth wrote: >> On Wed, May 22, 2013 at 11:32:51AM -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> >> >> Ping, looking to pull this in for 1.5.1 > > Anthony, Juan? Not sure if this is on your radar. Looking to get it > applied prior to stable freeze tomorrow. Reviewed-by: Juan Quintela <quintela@redhat.com> I guess that the value was already there, not that the version field hasn't been abused lot of times. I agree that sizeof() makes for an interesting version number, especially if padding enter the equation.
Applied. Thanks. Regards, Anthony Liguori
diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c index 1407fba..05af0b1 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 accept 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> --- v2: * Fixed s/except/accept/ typo (Laszlo) hw/watchdog/wdt_i6300esb.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)