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(-)
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
+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' } }
---
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
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.
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?
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
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.
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
© 2016 - 2024 Red Hat, Inc.