Re: [Qemu-devel] [PATCH v2 03/21] hw: timer: Add ASPEED RTC device
On Thu, 4 Jul 2019 at 08:49, Joel Stanley wrote:
>
> On Tue, 2 Jul 2019 at 19:19, Peter Maydell wrote:
> >
> > On Tue, 18 Jun 2019 at 17:53, Cédric Le Goater wrote:
> > >
> > > From: Joel Stanley
> > >
> > > The RTC is modeled to provide time and date functionality. It is
> > > initialised at zero to match the hardware.
> > >
> > > There is no modelling of the alarm functionality, which includes the IRQ
> > > line. As there is no guest code to exercise this function that is
> > > acceptable for now.
> > >
> > > Signed-off-by: Joel Stanley
> > > Reviewed-by: Peter Maydell
> >
> > Hi; Coverity complains about this function (CID 1402782):
> >
> > > +static void aspeed_rtc_calc_offset(AspeedRtcState *rtc)
> > > +{
> > > +struct tm tm;
> > > +uint32_t year, cent;
> > > +uint32_t reg1 = rtc->reg[COUNTER1];
> > > +uint32_t reg2 = rtc->reg[COUNTER2];
> > > +
> > > +tm.tm_mday = (reg1 >> 24) & 0x1f;
> > > +tm.tm_hour = (reg1 >> 16) & 0x1f;
> > > +tm.tm_min = (reg1 >> 8) & 0x3f;
> > > +tm.tm_sec = (reg1 >> 0) & 0x3f;
> > > +
> > > +cent = (reg2 >> 16) & 0x1f;
> > > +year = (reg2 >> 8) & 0x7f;
> > > +tm.tm_mon = ((reg2 >> 0) & 0x0f) - 1;
> > > +tm.tm_year = year + (cent * 100) - 1900;
> > > +
> > > +rtc->offset = qemu_timedate_diff();
> > > +}
> >
> > because the tm_wday field of 'struct tm tm' is not initialized
> > before we call qemu_timedate_diff(). This is a false
> > positive because the "read" of this field is just the place
> > in qemu_timedate_diff() that does "struct tm tmp = *tm;"
> > before calling mktime(), and mktime() ignores tm_wday.
> > We could make Coverity happy by using a struct initializer
> > on 'tm' here; on the other hand we don't do that anywhere else
> > which calls qemu_timedate_diff(), so maybe I should just mark
> > this a false positive?
>
> I don't have an opinion on which option to take. Perhaps mark it as a
> false positive?
Yeah, seems reasonable.
-- PMM
Re: [Qemu-devel] [PATCH v2 03/21] hw: timer: Add ASPEED RTC device
On Tue, 2 Jul 2019 at 19:19, Peter Maydell wrote:
>
> On Tue, 18 Jun 2019 at 17:53, Cédric Le Goater wrote:
> >
> > From: Joel Stanley
> >
> > The RTC is modeled to provide time and date functionality. It is
> > initialised at zero to match the hardware.
> >
> > There is no modelling of the alarm functionality, which includes the IRQ
> > line. As there is no guest code to exercise this function that is
> > acceptable for now.
> >
> > Signed-off-by: Joel Stanley
> > Reviewed-by: Peter Maydell
>
> Hi; Coverity complains about this function (CID 1402782):
>
> > +static void aspeed_rtc_calc_offset(AspeedRtcState *rtc)
> > +{
> > +struct tm tm;
> > +uint32_t year, cent;
> > +uint32_t reg1 = rtc->reg[COUNTER1];
> > +uint32_t reg2 = rtc->reg[COUNTER2];
> > +
> > +tm.tm_mday = (reg1 >> 24) & 0x1f;
> > +tm.tm_hour = (reg1 >> 16) & 0x1f;
> > +tm.tm_min = (reg1 >> 8) & 0x3f;
> > +tm.tm_sec = (reg1 >> 0) & 0x3f;
> > +
> > +cent = (reg2 >> 16) & 0x1f;
> > +year = (reg2 >> 8) & 0x7f;
> > +tm.tm_mon = ((reg2 >> 0) & 0x0f) - 1;
> > +tm.tm_year = year + (cent * 100) - 1900;
> > +
> > +rtc->offset = qemu_timedate_diff();
> > +}
>
> because the tm_wday field of 'struct tm tm' is not initialized
> before we call qemu_timedate_diff(). This is a false
> positive because the "read" of this field is just the place
> in qemu_timedate_diff() that does "struct tm tmp = *tm;"
> before calling mktime(), and mktime() ignores tm_wday.
> We could make Coverity happy by using a struct initializer
> on 'tm' here; on the other hand we don't do that anywhere else
> which calls qemu_timedate_diff(), so maybe I should just mark
> this a false positive?
I don't have an opinion on which option to take. Perhaps mark it as a
false positive?
Cheers,
Joel
Re: [Qemu-devel] [PATCH v2 03/21] hw: timer: Add ASPEED RTC device
On Tue, 18 Jun 2019 at 17:53, Cédric Le Goater wrote:
>
> From: Joel Stanley
>
> The RTC is modeled to provide time and date functionality. It is
> initialised at zero to match the hardware.
>
> There is no modelling of the alarm functionality, which includes the IRQ
> line. As there is no guest code to exercise this function that is
> acceptable for now.
>
> Signed-off-by: Joel Stanley
> Reviewed-by: Peter Maydell
Hi; Coverity complains about this function (CID 1402782):
> +static void aspeed_rtc_calc_offset(AspeedRtcState *rtc)
> +{
> +struct tm tm;
> +uint32_t year, cent;
> +uint32_t reg1 = rtc->reg[COUNTER1];
> +uint32_t reg2 = rtc->reg[COUNTER2];
> +
> +tm.tm_mday = (reg1 >> 24) & 0x1f;
> +tm.tm_hour = (reg1 >> 16) & 0x1f;
> +tm.tm_min = (reg1 >> 8) & 0x3f;
> +tm.tm_sec = (reg1 >> 0) & 0x3f;
> +
> +cent = (reg2 >> 16) & 0x1f;
> +year = (reg2 >> 8) & 0x7f;
> +tm.tm_mon = ((reg2 >> 0) & 0x0f) - 1;
> +tm.tm_year = year + (cent * 100) - 1900;
> +
> +rtc->offset = qemu_timedate_diff();
> +}
because the tm_wday field of 'struct tm tm' is not initialized
before we call qemu_timedate_diff(). This is a false
positive because the "read" of this field is just the place
in qemu_timedate_diff() that does "struct tm tmp = *tm;"
before calling mktime(), and mktime() ignores tm_wday.
We could make Coverity happy by using a struct initializer
on 'tm' here; on the other hand we don't do that anywhere else
which calls qemu_timedate_diff(), so maybe I should just mark
this a false positive?
thanks
-- PMM
[Qemu-devel] [PATCH v2 03/21] hw: timer: Add ASPEED RTC device
From: Joel Stanley
The RTC is modeled to provide time and date functionality. It is
initialised at zero to match the hardware.
There is no modelling of the alarm functionality, which includes the IRQ
line. As there is no guest code to exercise this function that is
acceptable for now.
Signed-off-by: Joel Stanley
Reviewed-by: Peter Maydell
---
include/hw/timer/aspeed_rtc.h | 31 ++
hw/timer/aspeed_rtc.c | 180 ++
hw/timer/Makefile.objs| 2 +-
hw/timer/trace-events | 4 +
4 files changed, 216 insertions(+), 1 deletion(-)
create mode 100644 include/hw/timer/aspeed_rtc.h
create mode 100644 hw/timer/aspeed_rtc.c
diff --git a/include/hw/timer/aspeed_rtc.h b/include/hw/timer/aspeed_rtc.h
new file mode 100644
index ..1f1155a676c1
--- /dev/null
+++ b/include/hw/timer/aspeed_rtc.h
@@ -0,0 +1,31 @@
+/*
+ * ASPEED Real Time Clock
+ * Joel Stanley
+ *
+ * Copyright 2019 IBM Corp
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#ifndef ASPEED_RTC_H
+#define ASPEED_RTC_H
+
+#include
+
+#include "hw/hw.h"
+#include "hw/irq.h"
+#include "hw/sysbus.h"
+
+typedef struct AspeedRtcState {
+SysBusDevice parent_obj;
+
+MemoryRegion iomem;
+qemu_irq irq;
+
+uint32_t reg[0x18];
+int offset;
+
+} AspeedRtcState;
+
+#define TYPE_ASPEED_RTC "aspeed.rtc"
+#define ASPEED_RTC(obj) OBJECT_CHECK(AspeedRtcState, (obj), TYPE_ASPEED_RTC)
+
+#endif /* ASPEED_RTC_H */
diff --git a/hw/timer/aspeed_rtc.c b/hw/timer/aspeed_rtc.c
new file mode 100644
index ..19f061c846e8
--- /dev/null
+++ b/hw/timer/aspeed_rtc.c
@@ -0,0 +1,180 @@
+/*
+ * ASPEED Real Time Clock
+ * Joel Stanley
+ *
+ * Copyright 2019 IBM Corp
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "hw/timer/aspeed_rtc.h"
+#include "qemu/log.h"
+#include "qemu/timer.h"
+
+#include "trace.h"
+
+#define COUNTER1(0x00 / 4)
+#define COUNTER2(0x04 / 4)
+#define ALARM (0x08 / 4)
+#define CONTROL (0x10 / 4)
+#define ALARM_STATUS(0x14 / 4)
+
+#define RTC_UNLOCKEDBIT(1)
+#define RTC_ENABLED BIT(0)
+
+static void aspeed_rtc_calc_offset(AspeedRtcState *rtc)
+{
+struct tm tm;
+uint32_t year, cent;
+uint32_t reg1 = rtc->reg[COUNTER1];
+uint32_t reg2 = rtc->reg[COUNTER2];
+
+tm.tm_mday = (reg1 >> 24) & 0x1f;
+tm.tm_hour = (reg1 >> 16) & 0x1f;
+tm.tm_min = (reg1 >> 8) & 0x3f;
+tm.tm_sec = (reg1 >> 0) & 0x3f;
+
+cent = (reg2 >> 16) & 0x1f;
+year = (reg2 >> 8) & 0x7f;
+tm.tm_mon = ((reg2 >> 0) & 0x0f) - 1;
+tm.tm_year = year + (cent * 100) - 1900;
+
+rtc->offset = qemu_timedate_diff();
+}
+
+static uint32_t aspeed_rtc_get_counter(AspeedRtcState *rtc, int r)
+{
+uint32_t year, cent;
+struct tm now;
+
+qemu_get_timedate(, rtc->offset);
+
+switch (r) {
+case COUNTER1:
+return (now.tm_mday << 24) | (now.tm_hour << 16) |
+(now.tm_min << 8) | now.tm_sec;
+case COUNTER2:
+cent = (now.tm_year + 1900) / 100;
+year = now.tm_year % 100;
+return ((cent & 0x1f) << 16) | ((year & 0x7f) << 8) |
+((now.tm_mon + 1) & 0xf);
+default:
+g_assert_not_reached();
+}
+}
+
+static uint64_t aspeed_rtc_read(void *opaque, hwaddr addr,
+unsigned size)
+{
+AspeedRtcState *rtc = opaque;
+uint64_t val;
+uint32_t r = addr >> 2;
+
+switch (r) {
+case COUNTER1:
+case COUNTER2:
+if (rtc->reg[CONTROL] & RTC_ENABLED) {
+rtc->reg[r] = aspeed_rtc_get_counter(rtc, r);
+}
+/* fall through */
+case CONTROL:
+val = rtc->reg[r];
+break;
+case ALARM:
+case ALARM_STATUS:
+default:
+qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx "\n", __func__, addr);
+return 0;
+}
+
+trace_aspeed_rtc_read(addr, val);
+
+return val;
+}
+
+static void aspeed_rtc_write(void *opaque, hwaddr addr,
+ uint64_t val, unsigned size)
+{
+AspeedRtcState *rtc = opaque;
+uint32_t r = addr >> 2;
+
+switch (r) {
+case COUNTER1:
+case COUNTER2:
+if (!(rtc->reg[CONTROL] & RTC_UNLOCKED)) {
+break;
+}
+/* fall through */
+case CONTROL:
+rtc->reg[r] = val;
+aspeed_rtc_calc_offset(rtc);
+break;
+case ALARM:
+case ALARM_STATUS:
+default:
+qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx "\n", __func__, addr);
+break;
+}
+trace_aspeed_rtc_write(addr, val);
+}
+
+static void aspeed_rtc_reset(DeviceState *d)
+{
+AspeedRtcState *rtc = ASPEED_RTC(d);
+
+rtc->offset = 0;
+memset(rtc->reg, 0, sizeof(rtc->reg));
+}
+
+static const MemoryRegionOps aspeed_rtc_ops = {
+.read = aspeed_rtc_read,
+.write = aspeed_rtc_write,
+.endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static const VMStateDescription
