Re: [PATCH for-8.2 3/4] hw/rtc/aspeed_rtc: Use 64-bit offset for holding time_t difference
On 7/20/23 18:45, Peter Maydell wrote: On Thu, 20 Jul 2023 at 17:42, Cédric Le Goater 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 Reviewed-by: Cédric Le Goater --- 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.
Re: [PATCH for-8.2 3/4] hw/rtc/aspeed_rtc: Use 64-bit offset for holding time_t difference
On Thu, 20 Jul 2023 at 17:42, Cédric Le Goater 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 > > > Reviewed-by: Cédric Le Goater > > > > --- > > 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
Re: [PATCH for-8.2 3/4] hw/rtc/aspeed_rtc: Use 64-bit offset for holding time_t difference
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
Reviewed-by: Cédric Le Goater
---
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()
}
};
[PATCH for-8.2 3/4] hw/rtc/aspeed_rtc: Use 64-bit offset for holding time_t difference
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
---
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(-)
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()
}
};
--
2.34.1
