| Message ID | 20230720155902.1590362-4-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | rtc devices: Avoid putting time_t in 32-bit variables | expand |
On 7/20/23 17:59, Peter Maydell wrote: > In the aspeed_rtc device we store a difference between two time_t > values in an 'int'. This is not really correct when time_t could > be 64 bits. Enlarge the field to 'int64_t'. > > This is a migration compatibility break for the aspeed boards. > While we are changing the vmstate, remove the accidental > duplicate of the offset field. Ah yes. Thanks. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Cédric Le Goater <clg@kaod.org> > --- > I took "bump the migration version" as the simplest approach > here, because I don't think we care about migration compat > in this case. If we do I can write the alternate version of > the patch... I don't think we care much about migration compat and fyi, migration of aspeed machines broke a while ago. It still migrates if done before Linux is loaded. C. > --- > include/hw/rtc/aspeed_rtc.h | 2 +- > hw/rtc/aspeed_rtc.c | 5 ++--- > 2 files changed, 3 insertions(+), 4 deletions(-) > > diff --git a/include/hw/rtc/aspeed_rtc.h b/include/hw/rtc/aspeed_rtc.h > index df61e46059e..596dfebb46c 100644 > --- a/include/hw/rtc/aspeed_rtc.h > +++ b/include/hw/rtc/aspeed_rtc.h > @@ -18,7 +18,7 @@ struct AspeedRtcState { > qemu_irq irq; > > uint32_t reg[0x18]; > - int offset; > + int64_t offset; > > }; > > diff --git a/hw/rtc/aspeed_rtc.c b/hw/rtc/aspeed_rtc.c > index f6da7b666d6..fa861e2d494 100644 > --- a/hw/rtc/aspeed_rtc.c > +++ b/hw/rtc/aspeed_rtc.c > @@ -136,11 +136,10 @@ static const MemoryRegionOps aspeed_rtc_ops = { > > static const VMStateDescription vmstate_aspeed_rtc = { > .name = TYPE_ASPEED_RTC, > - .version_id = 1, > + .version_id = 2, > .fields = (VMStateField[]) { > VMSTATE_UINT32_ARRAY(reg, AspeedRtcState, 0x18), > - VMSTATE_INT32(offset, AspeedRtcState), > - VMSTATE_INT32(offset, AspeedRtcState), > + VMSTATE_INT64(offset, AspeedRtcState), > VMSTATE_END_OF_LIST() > } > };
On Thu, 20 Jul 2023 at 17:42, Cédric Le Goater <clg@kaod.org> wrote: > > On 7/20/23 17:59, Peter Maydell wrote: > > In the aspeed_rtc device we store a difference between two time_t > > values in an 'int'. This is not really correct when time_t could > > be 64 bits. Enlarge the field to 'int64_t'. > > > > This is a migration compatibility break for the aspeed boards. > > While we are changing the vmstate, remove the accidental > > duplicate of the offset field. > > Ah yes. Thanks. > > > > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > > Reviewed-by: Cédric Le Goater <clg@kaod.org> > > > > --- > > I took "bump the migration version" as the simplest approach > > here, because I don't think we care about migration compat > > in this case. If we do I can write the alternate version of > > the patch... > > > I don't think we care much about migration compat and fyi, migration > of aspeed machines broke a while ago. It still migrates if done before > Linux is loaded. Is that the "migration of AArch32 Secure state doesn't work properly" bug, or am I misremembering? -- PMM
On 7/20/23 18:45, Peter Maydell wrote: > On Thu, 20 Jul 2023 at 17:42, Cédric Le Goater <clg@kaod.org> wrote: >> >> On 7/20/23 17:59, Peter Maydell wrote: >>> In the aspeed_rtc device we store a difference between two time_t >>> values in an 'int'. This is not really correct when time_t could >>> be 64 bits. Enlarge the field to 'int64_t'. >>> >>> This is a migration compatibility break for the aspeed boards. >>> While we are changing the vmstate, remove the accidental >>> duplicate of the offset field. >> >> Ah yes. Thanks. >> >>> >>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >> >> >> Reviewed-by: Cédric Le Goater <clg@kaod.org> >> >> >>> --- >>> I took "bump the migration version" as the simplest approach >>> here, because I don't think we care about migration compat >>> in this case. If we do I can write the alternate version of >>> the patch... >> >> >> I don't think we care much about migration compat and fyi, migration >> of aspeed machines broke a while ago. It still migrates if done before >> Linux is loaded. > > Is that the "migration of AArch32 Secure state doesn't work > properly" bug, or am I misremembering? probably, arm926 is not impacted, arm1176 and cortex-a7 are. C.
diff --git a/include/hw/rtc/aspeed_rtc.h b/include/hw/rtc/aspeed_rtc.h index df61e46059e..596dfebb46c 100644 --- a/include/hw/rtc/aspeed_rtc.h +++ b/include/hw/rtc/aspeed_rtc.h @@ -18,7 +18,7 @@ struct AspeedRtcState { qemu_irq irq; uint32_t reg[0x18]; - int offset; + int64_t offset; }; diff --git a/hw/rtc/aspeed_rtc.c b/hw/rtc/aspeed_rtc.c index f6da7b666d6..fa861e2d494 100644 --- a/hw/rtc/aspeed_rtc.c +++ b/hw/rtc/aspeed_rtc.c @@ -136,11 +136,10 @@ static const MemoryRegionOps aspeed_rtc_ops = { static const VMStateDescription vmstate_aspeed_rtc = { .name = TYPE_ASPEED_RTC, - .version_id = 1, + .version_id = 2, .fields = (VMStateField[]) { VMSTATE_UINT32_ARRAY(reg, AspeedRtcState, 0x18), - VMSTATE_INT32(offset, AspeedRtcState), - VMSTATE_INT32(offset, AspeedRtcState), + VMSTATE_INT64(offset, AspeedRtcState), VMSTATE_END_OF_LIST() } };
In the aspeed_rtc device we store a difference between two time_t values in an 'int'. This is not really correct when time_t could be 64 bits. Enlarge the field to 'int64_t'. This is a migration compatibility break for the aspeed boards. While we are changing the vmstate, remove the accidental duplicate of the offset field. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- I took "bump the migration version" as the simplest approach here, because I don't think we care about migration compat in this case. If we do I can write the alternate version of the patch... --- include/hw/rtc/aspeed_rtc.h | 2 +- hw/rtc/aspeed_rtc.c | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-)