[PATCH for-8.2 0/4] rtc devices: Avoid putting time_t in 32-bit variables

[PATCH for-8.2 0/4] rtc devices: Avoid putting time_t in 32-bit variables

[Peter Maydell]

This patchset was prompted by a couple of Coverity warnings
(CID 1507157, 1517772) which note that in the m48t59 RTC device model
we keep an offset in a time_t variable but then truncate it by
passing it to qemu_get_timedate(), which currently uses an 'int'
argument for its offset parameter.

We can fix the Coverity complaint by making qemu_get_timedate()
take a time_t; we should also correspondingly make the
qemu_timedate_diff() function return a time_t. However this
will only push the issue out to callers of qemu_timedate_diff()
if they are putting the result in a 32-bit variable or doing
32-bit arithmetic on it.

Luckily there aren't that many callers of qemu_timedate_diff()
and most of them already use either time_t or int64_t for the
calculations they do on its return value. The first three
patches fix devices which weren't doing that; patch four then
fixes the rtc.c functions. If I missed any callsites in devices
then hopefully Coverity will point them out.

This patchset is a migration compat break for the aspeed boards,
because the offset field in aspeed_rtc is in its vmstate struct.
We could in theory make this a compatible migration change, but
I don't believe we care about migration compat for these boards.

I've only tested this with 'make check' and 'make check-avocado',
which probably do not exercise these RTC devices much.

I've tagged this as for-8.2 because the code has been like this
forever. We might as well give ourselves plenty of time to see
if there's any unforeseen consequences of widening the type.

thanks
-- PMM

Peter Maydell (4):
  hw/rtc/m48t59: Use 64-bit arithmetic in set_alarm()
  hw/rtc/twl92230: Use int64_t for sec_offset and alm_sec
  hw/rtc/aspeed_rtc: Use 64-bit offset for holding time_t difference
  rtc: Use time_t for passing and returning time offsets

 include/hw/rtc/aspeed_rtc.h | 2 +-
 include/sysemu/rtc.h        | 4 ++--
 hw/rtc/aspeed_rtc.c         | 5 ++---
 hw/rtc/m48t59.c             | 2 +-
 hw/rtc/twl92230.c           | 4 ++--
 softmmu/rtc.c               | 4 ++--
 6 files changed, 10 insertions(+), 11 deletions(-)

-- 
2.34.1

[PATCH for-8.2 1/4] hw/rtc/m48t59: Use 64-bit arithmetic in set_alarm()

[Peter Maydell]

In the m48t59 device we almost always use 64-bit arithmetic when
dealing with time_t deltas.  The one exception is in set_alarm(),
which currently uses a plain 'int' to hold the difference between two
time_t values.  Switch to int64_t instead to avoid any possible
overflow issues.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/rtc/m48t59.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/rtc/m48t59.c b/hw/rtc/m48t59.c
index ec3e56e84fd..2e2c849985c 100644
--- a/hw/rtc/m48t59.c
+++ b/hw/rtc/m48t59.c
@@ -133,7 +133,7 @@ static void alarm_cb (void *opaque)
 
 static void set_alarm(M48t59State *NVRAM)
 {
-    int diff;
+    int64_t diff;
     if (NVRAM->alrm_timer != NULL) {
         timer_del(NVRAM->alrm_timer);
         diff = qemu_timedate_diff(&NVRAM->alarm) - NVRAM->time_offset;
-- 
2.34.1

[PATCH for-8.2 2/4] hw/rtc/twl92230: Use int64_t for sec_offset and alm_sec

[Peter Maydell]

In the twl92230 device, use int64_t for the two state fields
sec_offset and alm_sec, because we set these to values that
are either time_t or differences between two time_t values.

These fields aren't saved in vmstate anywhere, so we can
safely widen them.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I have a suspicion that really these fields *should* be
being migrated, but this device is only used in the n800
and n810 boards, so I'm not going to investigate how broken
migration/vmsave is there...
---
 hw/rtc/twl92230.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/rtc/twl92230.c b/hw/rtc/twl92230.c
index d8534dad949..64c61c3daeb 100644
--- a/hw/rtc/twl92230.c
+++ b/hw/rtc/twl92230.c
@@ -65,8 +65,8 @@ struct MenelausState {
         struct tm tm;
         struct tm new;
         struct tm alm;
-        int sec_offset;
-        int alm_sec;
+        int64_t sec_offset;
+        int64_t alm_sec;
         int next_comp;
     } rtc;
     uint16_t rtc_next_vmstate;
-- 
2.34.1

[PATCH for-8.2 3/4] hw/rtc/aspeed_rtc: Use 64-bit offset for holding time_t difference

[Peter Maydell]

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

[PATCH for-8.2 4/4] rtc: Use time_t for passing and returning time offsets

[Peter Maydell]

The functions qemu_get_timedate() and qemu_timedate_diff() take
and return a time offset as an integer. Coverity points out that
means that when an RTC device implementation holds an offset
as a time_t, as the m48t59 does, the time_t will get truncated.
(CID 1507157, 1517772).

The functions work with time_t internally, so make them use that type
in their APIs.

Note that this won't help any Y2038 issues where either the device
model itself is keeping the offset in a 32-bit integer, or where the
hardware under emulation has Y2038 or other rollover problems.  If we
missed any cases of the former then hopefully Coverity will warn us
about them since after this patch we'd be truncating a time_t in
assignments from qemu_timedate_diff().)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/sysemu/rtc.h | 4 ++--
 softmmu/rtc.c        | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/sysemu/rtc.h b/include/sysemu/rtc.h
index 159702b45b5..0fc8ad6fdf1 100644
--- a/include/sysemu/rtc.h
+++ b/include/sysemu/rtc.h
@@ -42,7 +42,7 @@
  * The behaviour of the clock whose value this function returns will
  * depend on the -rtc command line option passed by the user.
  */
-void qemu_get_timedate(struct tm *tm, int offset);
+void qemu_get_timedate(struct tm *tm, time_t offset);
 
 /**
  * qemu_timedate_diff: Return difference between a struct tm and the RTC
@@ -53,6 +53,6 @@ void qemu_get_timedate(struct tm *tm, int offset);
  * a timestamp one hour further ahead than the current RTC time
  * then this function will return 3600.
  */
-int qemu_timedate_diff(struct tm *tm);
+time_t qemu_timedate_diff(struct tm *tm);
 
 #endif
diff --git a/softmmu/rtc.c b/softmmu/rtc.c
index 4b2bf75dd67..4904581abeb 100644
--- a/softmmu/rtc.c
+++ b/softmmu/rtc.c
@@ -68,7 +68,7 @@ static time_t qemu_ref_timedate(QEMUClockType clock)
     return value;
 }
 
-void qemu_get_timedate(struct tm *tm, int offset)
+void qemu_get_timedate(struct tm *tm, time_t offset)
 {
     time_t ti = qemu_ref_timedate(rtc_clock);
 
@@ -85,7 +85,7 @@ void qemu_get_timedate(struct tm *tm, int offset)
     }
 }
 
-int qemu_timedate_diff(struct tm *tm)
+time_t qemu_timedate_diff(struct tm *tm)
 {
     time_t seconds;
 
-- 
2.34.1

Re: [PATCH for-8.2 3/4] hw/rtc/aspeed_rtc: Use 64-bit offset for holding time_t difference

[Cédric Le Goater]

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

Re: [PATCH for-8.2 3/4] hw/rtc/aspeed_rtc: Use 64-bit offset for holding time_t difference

[Peter Maydell]

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

Re: [PATCH for-8.2 3/4] hw/rtc/aspeed_rtc: Use 64-bit offset for holding time_t difference

[Cédric Le Goater]

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.

Re: [PATCH for-8.2 0/4] rtc devices: Avoid putting time_t in 32-bit variables

[Philippe Mathieu-Daudé]

+Markus

On 20/7/23 17:58, Peter Maydell wrote:
> This patchset was prompted by a couple of Coverity warnings
> (CID 1507157, 1517772) which note that in the m48t59 RTC device model
> we keep an offset in a time_t variable but then truncate it by
> passing it to qemu_get_timedate(), which currently uses an 'int'
> argument for its offset parameter.
> 
> We can fix the Coverity complaint by making qemu_get_timedate()
> take a time_t; we should also correspondingly make the
> qemu_timedate_diff() function return a time_t. However this
> will only push the issue out to callers of qemu_timedate_diff()
> if they are putting the result in a 32-bit variable or doing
> 32-bit arithmetic on it.
> 
> Luckily there aren't that many callers of qemu_timedate_diff()
> and most of them already use either time_t or int64_t for the
> calculations they do on its return value. The first three
> patches fix devices which weren't doing that; patch four then
> fixes the rtc.c functions. If I missed any callsites in devices
> then hopefully Coverity will point them out.

Do we need to change the type of the RTC_CHANGE event?

This is wrong, but to give an idea:

--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -553,47 +553,47 @@
  ##
  # @RTC_CHANGE:
  #
  # Emitted when the guest changes the RTC time.
  #
  # @offset: offset in seconds between base RTC clock (as specified by
  #     -rtc base), and new RTC clock value
  #
  # @qom-path: path to the RTC object in the QOM tree
  #
  # Note: This event is rate-limited.  It is not guaranteed that the RTC
  #     in the system implements this event, or even that the system has
  #     an RTC at all.
  #
  # Since: 0.13
  #
  # Example:
  #
  # <- { "event": "RTC_CHANGE",
  #      "data": { "offset": 78 },
  #      "timestamp": { "seconds": 1267020223, "microseconds": 435656 } }
  ##
  { 'event': 'RTC_CHANGE',
-  'data': { 'offset': 'int', 'qom-path': 'str' } }
+  'data': { 'offset': 'int64', 'qom-path': 'str' } }
---

Re: [PATCH for-8.2 1/4] hw/rtc/m48t59: Use 64-bit arithmetic in set_alarm()

[Philippe Mathieu-Daudé]

On 20/7/23 17:58, Peter Maydell wrote:
> In the m48t59 device we almost always use 64-bit arithmetic when
> dealing with time_t deltas.  The one exception is in set_alarm(),
> which currently uses a plain 'int' to hold the difference between two
> time_t values.  Switch to int64_t instead to avoid any possible
> overflow issues.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/rtc/m48t59.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH for-8.2 1/4] hw/rtc/m48t59: Use 64-bit arithmetic in set_alarm()

[Philippe Mathieu-Daudé]

On 20/7/23 17:58, Peter Maydell wrote:
> In the m48t59 device we almost always use 64-bit arithmetic when
> dealing with time_t deltas.  The one exception is in set_alarm(),
> which currently uses a plain 'int' to hold the difference between two
> time_t values.  Switch to int64_t instead to avoid any possible
> overflow issues.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/rtc/m48t59.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Similarly:

-- >8 --
--- a/hw/rtc/m48t59.c
+++ b/hw/rtc/m48t59.c
@@ -88,7 +88,7 @@ static M48txxInfo m48txx_sysbus_info[] = {
  static void alarm_cb (void *opaque)
  {
      struct tm tm;
-    uint64_t next_time;
+    int64_t next_time;
      M48t59State *NVRAM = opaque;
---

Re: [PATCH for-8.2 2/4] hw/rtc/twl92230: Use int64_t for sec_offset and alm_sec

[Philippe Mathieu-Daudé]

On 20/7/23 17:59, Peter Maydell wrote:
> In the twl92230 device, use int64_t for the two state fields
> sec_offset and alm_sec, because we set these to values that
> are either time_t or differences between two time_t values.
> 
> These fields aren't saved in vmstate anywhere, so we can
> safely widen them.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I have a suspicion that really these fields *should* be
> being migrated, but this device is only used in the n800
> and n810 boards, so I'm not going to investigate how broken
> migration/vmsave is there...
> ---
>   hw/rtc/twl92230.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH for-8.2 0/4] rtc devices: Avoid putting time_t in 32-bit variables

[Philippe Mathieu-Daudé]

Hi Peter,

On 20/7/23 17:58, Peter Maydell wrote:
> This patchset was prompted by a couple of Coverity warnings
> (CID 1507157, 1517772) which note that in the m48t59 RTC device model
> we keep an offset in a time_t variable but then truncate it by
> passing it to qemu_get_timedate(), which currently uses an 'int'
> argument for its offset parameter.
> 
> We can fix the Coverity complaint by making qemu_get_timedate()
> take a time_t; we should also correspondingly make the
> qemu_timedate_diff() function return a time_t. However this
> will only push the issue out to callers of qemu_timedate_diff()
> if they are putting the result in a 32-bit variable or doing
> 32-bit arithmetic on it.
> 
> Luckily there aren't that many callers of qemu_timedate_diff()
> and most of them already use either time_t or int64_t for the
> calculations they do on its return value. The first three
> patches fix devices which weren't doing that; patch four then
> fixes the rtc.c functions. If I missed any callsites in devices
> then hopefully Coverity will point them out.

PL031State::tick_offset is uint32_t, and pl031_get_count() also
returns that type. Is that expected?

Re: [PATCH for-8.2 4/4] rtc: Use time_t for passing and returning time offsets

[Philippe Mathieu-Daudé]

On 20/7/23 17:59, Peter Maydell wrote:
> The functions qemu_get_timedate() and qemu_timedate_diff() take
> and return a time offset as an integer. Coverity points out that
> means that when an RTC device implementation holds an offset
> as a time_t, as the m48t59 does, the time_t will get truncated.
> (CID 1507157, 1517772).
> 
> The functions work with time_t internally, so make them use that type
> in their APIs.
> 
> Note that this won't help any Y2038 issues where either the device
> model itself is keeping the offset in a 32-bit integer, or where the
> hardware under emulation has Y2038 or other rollover problems.  If we
> missed any cases of the former then hopefully Coverity will warn us
> about them since after this patch we'd be truncating a time_t in
> assignments from qemu_timedate_diff().)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   include/sysemu/rtc.h | 4 ++--
>   softmmu/rtc.c        | 4 ++--
>   2 files changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH for-8.2 1/4] hw/rtc/m48t59: Use 64-bit arithmetic in set_alarm()

[Peter Maydell]

On Fri, 21 Jul 2023 at 10:09, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 20/7/23 17:58, Peter Maydell wrote:
> > In the m48t59 device we almost always use 64-bit arithmetic when
> > dealing with time_t deltas.  The one exception is in set_alarm(),
> > which currently uses a plain 'int' to hold the difference between two
> > time_t values.  Switch to int64_t instead to avoid any possible
> > overflow issues.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >   hw/rtc/m48t59.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
>
> Similarly:
>
> -- >8 --
> --- a/hw/rtc/m48t59.c
> +++ b/hw/rtc/m48t59.c
> @@ -88,7 +88,7 @@ static M48txxInfo m48txx_sysbus_info[] = {
>   static void alarm_cb (void *opaque)
>   {
>       struct tm tm;
> -    uint64_t next_time;
> +    int64_t next_time;
>       M48t59State *NVRAM = opaque;

Why? next_time should always be positive here, I think.

-- PMM

Re: [PATCH for-8.2 0/4] rtc devices: Avoid putting time_t in 32-bit variables

[Peter Maydell]

On Fri, 21 Jul 2023 at 10:03, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> +Markus
>
> On 20/7/23 17:58, Peter Maydell wrote:
> > This patchset was prompted by a couple of Coverity warnings
> > (CID 1507157, 1517772) which note that in the m48t59 RTC device model
> > we keep an offset in a time_t variable but then truncate it by
> > passing it to qemu_get_timedate(), which currently uses an 'int'
> > argument for its offset parameter.
> >
> > We can fix the Coverity complaint by making qemu_get_timedate()
> > take a time_t; we should also correspondingly make the
> > qemu_timedate_diff() function return a time_t. However this
> > will only push the issue out to callers of qemu_timedate_diff()
> > if they are putting the result in a 32-bit variable or doing
> > 32-bit arithmetic on it.
> >
> > Luckily there aren't that many callers of qemu_timedate_diff()
> > and most of them already use either time_t or int64_t for the
> > calculations they do on its return value. The first three
> > patches fix devices which weren't doing that; patch four then
> > fixes the rtc.c functions. If I missed any callsites in devices
> > then hopefully Coverity will point them out.
>
> Do we need to change the type of the RTC_CHANGE event?
>
> This is wrong, but to give an idea:
>
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -553,47 +553,47 @@
>   ##
>   # @RTC_CHANGE:
>   #
>   # Emitted when the guest changes the RTC time.
>   #
>   # @offset: offset in seconds between base RTC clock (as specified by
>   #     -rtc base), and new RTC clock value
>   #
>   # @qom-path: path to the RTC object in the QOM tree
>   #
>   # Note: This event is rate-limited.  It is not guaranteed that the RTC
>   #     in the system implements this event, or even that the system has
>   #     an RTC at all.
>   #
>   # Since: 0.13
>   #
>   # Example:
>   #
>   # <- { "event": "RTC_CHANGE",
>   #      "data": { "offset": 78 },
>   #      "timestamp": { "seconds": 1267020223, "microseconds": 435656 } }
>   ##
>   { 'event': 'RTC_CHANGE',
> -  'data': { 'offset': 'int', 'qom-path': 'str' } }
> +  'data': { 'offset': 'int64', 'qom-path': 'str' } }
> ---

If I understand the 'Built-in Types' section in
docs/devel/qapi-code-gen.rst correctly, the QAPI 'int'
type is already 64 bits.

thanks
-- PMM

Re: [PATCH for-8.2 0/4] rtc devices: Avoid putting time_t in 32-bit variables

[Peter Maydell]

On Fri, 21 Jul 2023 at 10:16, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Hi Peter,
>
> On 20/7/23 17:58, Peter Maydell wrote:
> > This patchset was prompted by a couple of Coverity warnings
> > (CID 1507157, 1517772) which note that in the m48t59 RTC device model
> > we keep an offset in a time_t variable but then truncate it by
> > passing it to qemu_get_timedate(), which currently uses an 'int'
> > argument for its offset parameter.
> >
> > We can fix the Coverity complaint by making qemu_get_timedate()
> > take a time_t; we should also correspondingly make the
> > qemu_timedate_diff() function return a time_t. However this
> > will only push the issue out to callers of qemu_timedate_diff()
> > if they are putting the result in a 32-bit variable or doing
> > 32-bit arithmetic on it.
> >
> > Luckily there aren't that many callers of qemu_timedate_diff()
> > and most of them already use either time_t or int64_t for the
> > calculations they do on its return value. The first three
> > patches fix devices which weren't doing that; patch four then
> > fixes the rtc.c functions. If I missed any callsites in devices
> > then hopefully Coverity will point them out.
>
> PL031State::tick_offset is uint32_t, and pl031_get_count() also
> returns that type. Is that expected?

I think those fall into the category of "the device we are
modelling does not support 64-bit timestamps" -- the PL031
RTC_DR register is only 32 bits.

thanks
-- PMM

Re: [PATCH for-8.2 0/4] rtc devices: Avoid putting time_t in 32-bit variables

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Fri, 21 Jul 2023 at 10:03, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> +Markus
>>
>> On 20/7/23 17:58, Peter Maydell wrote:
>> > This patchset was prompted by a couple of Coverity warnings
>> > (CID 1507157, 1517772) which note that in the m48t59 RTC device model
>> > we keep an offset in a time_t variable but then truncate it by
>> > passing it to qemu_get_timedate(), which currently uses an 'int'
>> > argument for its offset parameter.
>> >
>> > We can fix the Coverity complaint by making qemu_get_timedate()
>> > take a time_t; we should also correspondingly make the
>> > qemu_timedate_diff() function return a time_t. However this
>> > will only push the issue out to callers of qemu_timedate_diff()
>> > if they are putting the result in a 32-bit variable or doing
>> > 32-bit arithmetic on it.
>> >
>> > Luckily there aren't that many callers of qemu_timedate_diff()
>> > and most of them already use either time_t or int64_t for the
>> > calculations they do on its return value. The first three
>> > patches fix devices which weren't doing that; patch four then
>> > fixes the rtc.c functions. If I missed any callsites in devices
>> > then hopefully Coverity will point them out.
>>
>> Do we need to change the type of the RTC_CHANGE event?

[...]

> If I understand the 'Built-in Types' section in
> docs/devel/qapi-code-gen.rst correctly, the QAPI 'int'
> type is already 64 bits.

Correct.  Also needlessly confusing.

Re: [PATCH for-8.2 0/4] rtc devices: Avoid putting time_t in 32-bit variables

[Philippe Mathieu-Daudé]

On 21/7/23 11:46, Peter Maydell wrote:
> On Fri, 21 Jul 2023 at 10:16, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Hi Peter,
>>
>> On 20/7/23 17:58, Peter Maydell wrote:
>>> This patchset was prompted by a couple of Coverity warnings
>>> (CID 1507157, 1517772) which note that in the m48t59 RTC device model
>>> we keep an offset in a time_t variable but then truncate it by
>>> passing it to qemu_get_timedate(), which currently uses an 'int'
>>> argument for its offset parameter.
>>>
>>> We can fix the Coverity complaint by making qemu_get_timedate()
>>> take a time_t; we should also correspondingly make the
>>> qemu_timedate_diff() function return a time_t. However this
>>> will only push the issue out to callers of qemu_timedate_diff()
>>> if they are putting the result in a 32-bit variable or doing
>>> 32-bit arithmetic on it.
>>>
>>> Luckily there aren't that many callers of qemu_timedate_diff()
>>> and most of them already use either time_t or int64_t for the
>>> calculations they do on its return value. The first three
>>> patches fix devices which weren't doing that; patch four then
>>> fixes the rtc.c functions. If I missed any callsites in devices
>>> then hopefully Coverity will point them out.
>>
>> PL031State::tick_offset is uint32_t, and pl031_get_count() also
>> returns that type. Is that expected?
> 
> I think those fall into the category of "the device we are
> modelling does not support 64-bit timestamps" -- the PL031
> RTC_DR register is only 32 bits.

Good, thanks for confirming.

Re: [PATCH for-8.2 0/4] rtc devices: Avoid putting time_t in 32-bit variables

[Peter Maydell]

On Thu, 20 Jul 2023 at 16:59, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> This patchset was prompted by a couple of Coverity warnings
> (CID 1507157, 1517772) which note that in the m48t59 RTC device model
> we keep an offset in a time_t variable but then truncate it by
> passing it to qemu_get_timedate(), which currently uses an 'int'
> argument for its offset parameter.
>
> We can fix the Coverity complaint by making qemu_get_timedate()
> take a time_t; we should also correspondingly make the
> qemu_timedate_diff() function return a time_t. However this
> will only push the issue out to callers of qemu_timedate_diff()
> if they are putting the result in a 32-bit variable or doing
> 32-bit arithmetic on it.
>
> Luckily there aren't that many callers of qemu_timedate_diff()
> and most of them already use either time_t or int64_t for the
> calculations they do on its return value. The first three
> patches fix devices which weren't doing that; patch four then
> fixes the rtc.c functions. If I missed any callsites in devices
> then hopefully Coverity will point them out.
>
> This patchset is a migration compat break for the aspeed boards,
> because the offset field in aspeed_rtc is in its vmstate struct.
> We could in theory make this a compatible migration change, but
> I don't believe we care about migration compat for these boards.
>
> I've only tested this with 'make check' and 'make check-avocado',
> which probably do not exercise these RTC devices much.
>
> I've tagged this as for-8.2 because the code has been like this
> forever. We might as well give ourselves plenty of time to see
> if there's any unforeseen consequences of widening the type.

8.2 dev cycle is now open, so I plan to take these through
the arm tree, since they're mostly arm timer devices.

thanks
-- PMM