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

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

[Michael Roth]

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(-)

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

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

[Richard W.M. Jones]

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.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

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

[Laszlo Ersek]

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>

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

[Amit Shah]

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

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

[mdroth]

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
> 

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 <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
> > 

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

[Anthony Liguori]

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
>> > 

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

[Juan Quintela]

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.

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

[Anthony Liguori]

Applied.  Thanks.

Regards,

Anthony Liguori