[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-arm] [PATCH] timer/aspeed: fix vmstate version id
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [Qemu-arm] [PATCH] timer/aspeed: fix vmstate version id |
Date: |
Mon, 23 Apr 2018 11:46:23 +0100 |
User-agent: |
Mutt/1.9.5 (2018-04-13) |
* Cédric Le Goater (address@hidden) wrote:
> On 04/23/2018 11:34 AM, Peter Maydell wrote:
> > On 23 April 2018 at 10:28, Cédric Le Goater <address@hidden> 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 / address@hidden / Manchester, UK