[Qemu-devel] [PATCH] timer/aspeed: fix vmstate version id

[Qemu-devel] [PATCH] timer/aspeed: fix vmstate version id

[Cédric Le Goater]

commit 1d3e65aa7ac5 ("hw/timer: Add value matching support to
aspeed_timer") increased the vmstate version of the timers array but
doing so, it broke migration. To fix, we also need to change the
vmstate version of the aspeed timer controller model.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---

 To test on a 'palmetto-bmc' board, download the latest built OpenBMC
 flash image :

   wget https://openpower.xyz/job/openbmc-build/distro=ubuntu,target=palmetto/lastSuccessfulBuild/artifact/deploy/images/palmetto/flash-palmetto

 start two instances of QEMU with :

   qemu-system-arm -m 512 -M palmetto-bmc -drive file=./flash-palmetto,format=raw,if=mtd -serial mon:stdio -nographic -nodefaults

 and :
 
   qemu-system-arm -m 512 -M palmetto-bmc -drive file=./flash-palmetto,format=raw,if=mtd -serial mon:stdio -nographic -nodefaults -incoming tcp::1234

 then, run migrate from the monitor:
 
   (qemu) migrate tcp:localhost:1234

 hw/timer/aspeed_timer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
index 50acbf530a3a..7df19bd9df91 100644
--- a/hw/timer/aspeed_timer.c
+++ b/hw/timer/aspeed_timer.c
@@ -498,8 +498,8 @@ static const VMStateDescription vmstate_aspeed_timer = {
 
 static const VMStateDescription vmstate_aspeed_timer_state = {
     .name = "aspeed.timerctrl",
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(ctrl, AspeedTimerCtrlState),
         VMSTATE_UINT32(ctrl2, AspeedTimerCtrlState),
-- 
2.13.6

Re: [Qemu-devel] [PATCH] timer/aspeed: fix vmstate version id

[Peter Maydell]

On 23 April 2018 at 07:40, Cédric Le Goater <clg@kaod.org> wrote:
> commit 1d3e65aa7ac5 ("hw/timer: Add value matching support to
> aspeed_timer") increased the vmstate version of the timers array but
> doing so, it broke migration. To fix, we also need to change the
> vmstate version of the aspeed timer controller model.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>
>  To test on a 'palmetto-bmc' board, download the latest built OpenBMC
>  flash image :
>
>    wget https://openpower.xyz/job/openbmc-build/distro=ubuntu,target=palmetto/lastSuccessfulBuild/artifact/deploy/images/palmetto/flash-palmetto
>
>  start two instances of QEMU with :
>
>    qemu-system-arm -m 512 -M palmetto-bmc -drive file=./flash-palmetto,format=raw,if=mtd -serial mon:stdio -nographic -nodefaults
>
>  and :
>
>    qemu-system-arm -m 512 -M palmetto-bmc -drive file=./flash-palmetto,format=raw,if=mtd -serial mon:stdio -nographic -nodefaults -incoming tcp::1234
>
>  then, run migrate from the monitor:
>
>    (qemu) migrate tcp:localhost:1234
>
>  hw/timer/aspeed_timer.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
> index 50acbf530a3a..7df19bd9df91 100644
> --- a/hw/timer/aspeed_timer.c
> +++ b/hw/timer/aspeed_timer.c
> @@ -498,8 +498,8 @@ static const VMStateDescription vmstate_aspeed_timer = {
>
>  static const VMStateDescription vmstate_aspeed_timer_state = {
>      .name = "aspeed.timerctrl",
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> +    .version_id = 2,
> +    .minimum_version_id = 2,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT32(ctrl, AspeedTimerCtrlState),
>          VMSTATE_UINT32(ctrl2, AspeedTimerCtrlState),

Wouldn't it be simpler to just fix the incorrect value in
the VMSTATE_STRUCT_ARRAY(timers, AspeedTimerCtrlState,
line ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] timer/aspeed: fix vmstate version id

[Cédric Le Goater]

On 04/23/2018 11:12 AM, Peter Maydell wrote:
>> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
>> index 50acbf530a3a..7df19bd9df91 100644
>> --- a/hw/timer/aspeed_timer.c
>> +++ b/hw/timer/aspeed_timer.c
>> @@ -498,8 +498,8 @@ static const VMStateDescription vmstate_aspeed_timer = {
>>
>>  static const VMStateDescription vmstate_aspeed_timer_state = {
>>      .name = "aspeed.timerctrl",
>> -    .version_id = 1,
>> -    .minimum_version_id = 1,
>> +    .version_id = 2,
>> +    .minimum_version_id = 2,
>>      .fields = (VMStateField[]) {
>>          VMSTATE_UINT32(ctrl, AspeedTimerCtrlState),
>>          VMSTATE_UINT32(ctrl2, AspeedTimerCtrlState),
> Wouldn't it be simpler to just fix the incorrect value in
> the VMSTATE_STRUCT_ARRAY(timers, AspeedTimerCtrlState,
> line ?

Yes. Also. 

Or bring back all the version ids to 1, as we never supported 
migration before.

Thanks,

C.

Re: [Qemu-devel] [PATCH] timer/aspeed: fix vmstate version id

[Peter Maydell]

On 23 April 2018 at 10:28, Cédric Le Goater <clg@kaod.org> wrote:
> On 04/23/2018 11:12 AM, Peter Maydell wrote:
>>> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
>>> index 50acbf530a3a..7df19bd9df91 100644
>>> --- a/hw/timer/aspeed_timer.c
>>> +++ b/hw/timer/aspeed_timer.c
>>> @@ -498,8 +498,8 @@ static const VMStateDescription vmstate_aspeed_timer = {
>>>
>>>  static const VMStateDescription vmstate_aspeed_timer_state = {
>>>      .name = "aspeed.timerctrl",
>>> -    .version_id = 1,
>>> -    .minimum_version_id = 1,
>>> +    .version_id = 2,
>>> +    .minimum_version_id = 2,
>>>      .fields = (VMStateField[]) {
>>>          VMSTATE_UINT32(ctrl, AspeedTimerCtrlState),
>>>          VMSTATE_UINT32(ctrl2, AspeedTimerCtrlState),
>> Wouldn't it be simpler to just fix the incorrect value in
>> the VMSTATE_STRUCT_ARRAY(timers, AspeedTimerCtrlState,
>> line ?
>
> Yes. Also.
>
> Or bring back all the version ids to 1, as we never supported
> migration before.

I think it's nice to at least do the "bump version" thing, so you
get a (hopefully comprehensible) error rather than just wrong
data if you do try a cross version migration, so I would
vote for just fixing the one thing that was wrong: the
number in VMSTATE_STRUCT_ARRAY is the version to be used of
the substruct, so it didn't need to be bumped in commit
1d3e65aa7a; the main version numbers for vmstate_aspeed_timer
did need to be bumped because part of the main struct changed.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] timer/aspeed: fix vmstate version id

[Cédric Le Goater]

On 04/23/2018 11:34 AM, Peter Maydell wrote:
> On 23 April 2018 at 10:28, Cédric Le Goater <clg@kaod.org> wrote:
>> On 04/23/2018 11:12 AM, Peter Maydell wrote:
>>>> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
>>>> index 50acbf530a3a..7df19bd9df91 100644
>>>> --- a/hw/timer/aspeed_timer.c
>>>> +++ b/hw/timer/aspeed_timer.c
>>>> @@ -498,8 +498,8 @@ static const VMStateDescription vmstate_aspeed_timer = {
>>>>
>>>>  static const VMStateDescription vmstate_aspeed_timer_state = {
>>>>      .name = "aspeed.timerctrl",
>>>> -    .version_id = 1,
>>>> -    .minimum_version_id = 1,
>>>> +    .version_id = 2,
>>>> +    .minimum_version_id = 2,
>>>>      .fields = (VMStateField[]) {
>>>>          VMSTATE_UINT32(ctrl, AspeedTimerCtrlState),
>>>>          VMSTATE_UINT32(ctrl2, AspeedTimerCtrlState),
>>> Wouldn't it be simpler to just fix the incorrect value in
>>> the VMSTATE_STRUCT_ARRAY(timers, AspeedTimerCtrlState,
>>> line ?
>>
>> Yes. Also.
>>
>> Or bring back all the version ids to 1, as we never supported
>> migration before.
> 
> I think it's nice to at least do the "bump version" thing, so you
> get a (hopefully comprehensible) error rather than just wrong
> data if you do try a cross version migration, so I would
> vote for just fixing the one thing that was wrong: the
> number in VMSTATE_STRUCT_ARRAY is the version to be used of
> the substruct, so it didn't need to be bumped in commit
> 1d3e65aa7a; the main version numbers for vmstate_aspeed_timer
> did need to be bumped because part of the main struct changed.

Yes. This is correct. I will resend with just that change on 
VMSTATE_STRUCT_ARRAY.

Thanks,

C.
 

Re: [Qemu-devel] [PATCH] timer/aspeed: fix vmstate version id

[Cédric Le Goater]

On 04/23/2018 11:34 AM, Peter Maydell wrote:
> On 23 April 2018 at 10:28, Cédric Le Goater <clg@kaod.org> wrote:
>> On 04/23/2018 11:12 AM, Peter Maydell wrote:
>>>> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
>>>> index 50acbf530a3a..7df19bd9df91 100644
>>>> --- a/hw/timer/aspeed_timer.c
>>>> +++ b/hw/timer/aspeed_timer.c
>>>> @@ -498,8 +498,8 @@ static const VMStateDescription vmstate_aspeed_timer = {
>>>>
>>>>  static const VMStateDescription vmstate_aspeed_timer_state = {
>>>>      .name = "aspeed.timerctrl",
>>>> -    .version_id = 1,
>>>> -    .minimum_version_id = 1,
>>>> +    .version_id = 2,
>>>> +    .minimum_version_id = 2,
>>>>      .fields = (VMStateField[]) {
>>>>          VMSTATE_UINT32(ctrl, AspeedTimerCtrlState),
>>>>          VMSTATE_UINT32(ctrl2, AspeedTimerCtrlState),
>>> Wouldn't it be simpler to just fix the incorrect value in
>>> the VMSTATE_STRUCT_ARRAY(timers, AspeedTimerCtrlState,
>>> line ?
>>
>> Yes. Also.
>>
>> Or bring back all the version ids to 1, as we never supported
>> migration before.
> 
> I think it's nice to at least do the "bump version" thing, so you
> get a (hopefully comprehensible) error rather than just wrong
> data if you do try a cross version migration, 

On that topic, the error message was :

	Missing section footer for aspeed.timerctrl

which is not very comprehensible for a version mismatch issue.

Thanks,

C. 

> so I would
> vote for just fixing the one thing that was wrong: the
> number in VMSTATE_STRUCT_ARRAY is the version to be used of
> the substruct, so it didn't need to be bumped in commit
> 1d3e65aa7a; the main version numbers for vmstate_aspeed_timer
> did need to be bumped because part of the main struct changed.
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH] timer/aspeed: fix vmstate version id

[Dr. David Alan Gilbert]

* Cédric Le Goater (clg@kaod.org) wrote:
> On 04/23/2018 11:34 AM, Peter Maydell wrote:
> > On 23 April 2018 at 10:28, Cédric Le Goater <clg@kaod.org> wrote:
> >> On 04/23/2018 11:12 AM, Peter Maydell wrote:
> >>>> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
> >>>> index 50acbf530a3a..7df19bd9df91 100644
> >>>> --- a/hw/timer/aspeed_timer.c
> >>>> +++ b/hw/timer/aspeed_timer.c
> >>>> @@ -498,8 +498,8 @@ static const VMStateDescription vmstate_aspeed_timer = {
> >>>>
> >>>>  static const VMStateDescription vmstate_aspeed_timer_state = {
> >>>>      .name = "aspeed.timerctrl",
> >>>> -    .version_id = 1,
> >>>> -    .minimum_version_id = 1,
> >>>> +    .version_id = 2,
> >>>> +    .minimum_version_id = 2,
> >>>>      .fields = (VMStateField[]) {
> >>>>          VMSTATE_UINT32(ctrl, AspeedTimerCtrlState),
> >>>>          VMSTATE_UINT32(ctrl2, AspeedTimerCtrlState),
> >>> Wouldn't it be simpler to just fix the incorrect value in
> >>> the VMSTATE_STRUCT_ARRAY(timers, AspeedTimerCtrlState,
> >>> line ?
> >>
> >> Yes. Also.
> >>
> >> Or bring back all the version ids to 1, as we never supported
> >> migration before.
> > 
> > I think it's nice to at least do the "bump version" thing, so you
> > get a (hopefully comprehensible) error rather than just wrong
> > data if you do try a cross version migration, 
> 
> On that topic, the error message was :
> 
> 	Missing section footer for aspeed.timerctrl
> 
> which is not very comprehensible for a version mismatch issue.

Was that before your patch?

The VMSTATE fields in the structure have no metadata stored
for them to be parsed with; if you send a different number
of array entries to the number the destination receives you end
up with a corrupt stream. Section footers are just a canary
that tells you something went wrong in that devices data.
It has no more information to give you a more detailed error.

Dave

> Thanks,
> 
> C. 
> 
> > so I would
> > vote for just fixing the one thing that was wrong: the
> > number in VMSTATE_STRUCT_ARRAY is the version to be used of
> > the substruct, so it didn't need to be bumped in commit
> > 1d3e65aa7a; the main version numbers for vmstate_aspeed_timer
> > did need to be bumped because part of the main struct changed.
> > 
> > thanks
> > -- PMM
> > 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] timer/aspeed: fix vmstate version id

[Cédric Le Goater]

>>> I think it's nice to at least do the "bump version" thing, so you
>>> get a (hopefully comprehensible) error rather than just wrong
>>> data if you do try a cross version migration, 
>>
>> On that topic, the error message was :
>>
>> 	Missing section footer for aspeed.timerctrl
>>
>> which is not very comprehensible for a version mismatch issue.
> 
> Was that before your patch?

yes.

> The VMSTATE fields in the structure have no metadata stored
> for them to be parsed with; if you send a different number
> of array entries to the number the destination receives you end
> up with a corrupt stream. Section footers are just a canary
> that tells you something went wrong in that devices data.
> It has no more information to give you a more detailed error.

OK. Thanks for the explanation.

C.