Message ID | 20190618165311.27066-4-clg@kaod.org |
---|---|
State | New |
Headers | show |
Series | None | expand |
On Tue, 18 Jun 2019 at 17:53, Cédric Le Goater <clg@kaod.org> wrote: > > From: Joel Stanley <joel@jms.id.au> > > 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 <joel@jms.id.au> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> 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(&tm); > +} 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
On Tue, 2 Jul 2019 at 19:19, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Tue, 18 Jun 2019 at 17:53, Cédric Le Goater <clg@kaod.org> wrote: > > > > From: Joel Stanley <joel@jms.id.au> > > > > 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 <joel@jms.id.au> > > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > > 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(&tm); > > +} > > 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
On Thu, 4 Jul 2019 at 08:49, Joel Stanley <joel@jms.id.au> wrote: > > On Tue, 2 Jul 2019 at 19:19, Peter Maydell <peter.maydell@linaro.org> wrote: > > > > On Tue, 18 Jun 2019 at 17:53, Cédric Le Goater <clg@kaod.org> wrote: > > > > > > From: Joel Stanley <joel@jms.id.au> > > > > > > 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 <joel@jms.id.au> > > > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > > > > 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(&tm); > > > +} > > > > 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
diff --git a/include/hw/timer/aspeed_rtc.h b/include/hw/timer/aspeed_rtc.h new file mode 100644 index 000000000000..1f1155a676c1 --- /dev/null +++ b/include/hw/timer/aspeed_rtc.h @@ -0,0 +1,31 @@ +/* + * ASPEED Real Time Clock + * Joel Stanley <joel@jms.id.au> + * + * Copyright 2019 IBM Corp + * SPDX-License-Identifier: GPL-2.0-or-later + */ +#ifndef ASPEED_RTC_H +#define ASPEED_RTC_H + +#include <stdint.h> + +#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 000000000000..19f061c846e8 --- /dev/null +++ b/hw/timer/aspeed_rtc.c @@ -0,0 +1,180 @@ +/* + * ASPEED Real Time Clock + * Joel Stanley <joel@jms.id.au> + * + * 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_UNLOCKED BIT(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(&tm); +} + +static uint32_t aspeed_rtc_get_counter(AspeedRtcState *rtc, int r) +{ + uint32_t year, cent; + struct tm now; + + qemu_get_timedate(&now, 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 vmstate_aspeed_rtc = { + .name = TYPE_ASPEED_RTC, + .version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_UINT32_ARRAY(reg, AspeedRtcState, 0x18), + VMSTATE_INT32(offset, AspeedRtcState), + VMSTATE_INT32(offset, AspeedRtcState), + VMSTATE_END_OF_LIST() + } +}; + +static void aspeed_rtc_realize(DeviceState *dev, Error **errp) +{ + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); + AspeedRtcState *s = ASPEED_RTC(dev); + + sysbus_init_irq(sbd, &s->irq); + + memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_rtc_ops, s, + "aspeed-rtc", 0x18ULL); + sysbus_init_mmio(sbd, &s->iomem); +} + +static void aspeed_rtc_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + + dc->realize = aspeed_rtc_realize; + dc->vmsd = &vmstate_aspeed_rtc; + dc->reset = aspeed_rtc_reset; +} + +static const TypeInfo aspeed_rtc_info = { + .name = TYPE_ASPEED_RTC, + .parent = TYPE_SYS_BUS_DEVICE, + .instance_size = sizeof(AspeedRtcState), + .class_init = aspeed_rtc_class_init, +}; + +static void aspeed_rtc_register_types(void) +{ + type_register_static(&aspeed_rtc_info); +} + +type_init(aspeed_rtc_register_types) diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs index 0e9a4530f848..123d92c9692c 100644 --- a/hw/timer/Makefile.objs +++ b/hw/timer/Makefile.objs @@ -41,7 +41,7 @@ obj-$(CONFIG_MC146818RTC) += mc146818rtc.o obj-$(CONFIG_ALLWINNER_A10_PIT) += allwinner-a10-pit.o common-obj-$(CONFIG_STM32F2XX_TIMER) += stm32f2xx_timer.o -common-obj-$(CONFIG_ASPEED_SOC) += aspeed_timer.o +common-obj-$(CONFIG_ASPEED_SOC) += aspeed_timer.o aspeed_rtc.o common-obj-$(CONFIG_SUN4V_RTC) += sun4v-rtc.o common-obj-$(CONFIG_CMSDK_APB_TIMER) += cmsdk-apb-timer.o diff --git a/hw/timer/trace-events b/hw/timer/trace-events index dcaf3d6da6c8..db02a9142cda 100644 --- a/hw/timer/trace-events +++ b/hw/timer/trace-events @@ -66,6 +66,10 @@ cmsdk_apb_dualtimer_read(uint64_t offset, uint64_t data, unsigned size) "CMSDK A cmsdk_apb_dualtimer_write(uint64_t offset, uint64_t data, unsigned size) "CMSDK APB dualtimer write: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u" cmsdk_apb_dualtimer_reset(void) "CMSDK APB dualtimer: reset" +# hw/timer/aspeed-rtc.c +aspeed_rtc_read(uint64_t addr, uint64_t value) "addr 0x%02" PRIx64 " value 0x%08" PRIx64 +aspeed_rtc_write(uint64_t addr, uint64_t value) "addr 0x%02" PRIx64 " value 0x%08" PRIx64 + # sun4v-rtc.c sun4v_rtc_read(uint64_t addr, uint64_t value) "read: addr 0x%" PRIx64 " value 0x%" PRIx64 sun4v_rtc_write(uint64_t addr, uint64_t value) "write: addr 0x%" PRIx64 " value 0x%" PRIx64