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(-)

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

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.