Re: [Qemu-devel] [PATCH] timer/aspeed: fix vmstate version id
>>> 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.
Re: [Qemu-devel] [PATCH] timer/aspeed: fix vmstate version id
* Cédric Le Goater (c...@kaod.org) wrote: > On 04/23/2018 11:34 AM, Peter Maydell wrote: > > On 23 April 2018 at 10:28, Cédric Le Goater 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 / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCH] timer/aspeed: fix vmstate version id
On 04/23/2018 11:34 AM, Peter Maydell wrote: > On 23 April 2018 at 10:28, Cédric Le Goater 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
On 04/23/2018 11:34 AM, Peter Maydell wrote: > On 23 April 2018 at 10:28, Cédric Le Goater 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
On 23 April 2018 at 10:28, Cédric Le Goater 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
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
On 23 April 2018 at 07:40, Cédric Le Goater 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 > --- > > 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
[Qemu-devel] [PATCH] timer/aspeed: fix vmstate version id
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 --- 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