Re: [Qemu-devel] [PATCH v2] wdt_i6300esb: fix vmstate versioning

[mdroth]

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 <address@hidden>
> > Suggested-by: Peter Maydell <address@hidden>
> > Cc: address@hidden
> > Signed-off-by: Michael Roth <address@hidden>
> 
> 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
> >