[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PULL 38/43] hw/loongarch: Add LoongArch ls7a rtc device support
From: |
Peter Maydell |
Subject: |
Re: [PULL 38/43] hw/loongarch: Add LoongArch ls7a rtc device support |
Date: |
Tue, 28 Jun 2022 12:05:02 +0100 |
On Tue, 7 Jun 2022 at 00:34, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> From: Xiaojuan Yang <yangxiaojuan@loongson.cn>
>
> This patch add ls7a rtc device support.
>
> Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
> Signed-off-by: Song Gao <gaosong@loongson.cn>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Message-Id: <20220606124333.2060567-39-yangxiaojuan@loongson.cn>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Hi; Coverity points out some issues with this code, and I
noticed a few more reading through it.
> +static inline void toymatch_val_to_time(uint64_t val, struct tm *tm)
> +{
> + tm->tm_sec = FIELD_EX32(val, TOY_MATCH, SEC);
> + tm->tm_min = FIELD_EX32(val, TOY_MATCH, MIN);
> + tm->tm_hour = FIELD_EX32(val, TOY_MATCH, HOUR);
> + tm->tm_mday = FIELD_EX32(val, TOY_MATCH, DAY);
> + tm->tm_mon = FIELD_EX32(val, TOY_MATCH, MON) - 1;
> + tm->tm_year += (FIELD_EX32(val, TOY_MATCH, YEAR) - (tm->tm_year & 0x3f));
> +}
> +
> +static void toymatch_write(LS7ARtcState *s, struct tm *tm, uint64_t val, int
> num)
> +{
Why does this function take a pointer to a struct tm? The callsites
all pass in an entirely uninitialized struct tm and don't try to
read from it after the call. It would be clearer to just define
the struct tm as a local in this function.
> + int64_t now, expire_time;
> +
> + /* it do not support write when toy disabled */
> + if (toy_enabled(s)) {
> + s->toymatch[num] = val;
> + /* caculate expire time */
> + now = qemu_clock_get_ms(rtc_clock);
> + toymatch_val_to_time(val, tm);
> + expire_time = now + (qemu_timedate_diff(tm) - s->offset_toy) * 1000;
Coverity complains (CID 1489766) that we end up using uninitialized
fields in the struct tm here. There's two reasons for that:
(1) toymatch_val_to_time() doesn't set all the fields in the struct,
and we never zero-initialized the struct. This accounts for
tm_gmtoff, tm_isdst, tm_wday, tm_yday, tm_zone. You need to look
at whether any of those ought to be initialized, and set them.
Zero-init the struct to make Coverity happy about the rest of them.
(2) toymatch_val_to_time() sets tm_year based on the previous value
of tm_year. This doesn't make sense if the struct isn't initialized.
What was the intention here ?
> + timer_mod(s->toy_timer[num], expire_time);
> + }
> +}
> +static void ls7a_toy_start(LS7ARtcState *s)
> +{
> + int i;
> + uint64_t expire_time, now;
> + struct tm tm;
Coverity issue CID 1489763: we don't zero initialize the struct tm here,
but we don't individually initialize all its fields. In particular
the tm_wday field is never set up and Coverity complains it might
be used uninitialized.
The easy fix is to zero-init everything:
struct tm tm = {};
> + /*
> + * need to recaculate toy offset
typo: "recalculate" (here and in other comments above and below)
> + * and expire time when enable it.
> + */
> + toy_val_to_time_mon(s->save_toy_mon, &tm);
> + toy_val_to_time_year(s->save_toy_year, &tm);
> +
> + s->offset_toy = qemu_timedate_diff(&tm);
> + now = qemu_clock_get_ms(rtc_clock);
> +
> + /* recaculate expire time and enable timer */
> + for (i = 0; i < TIMER_NUMS; i++) {
> + toymatch_val_to_time(s->toymatch[i], &tm);
> + expire_time = now + (qemu_timedate_diff(&tm) - s->offset_toy) * 1000;
> + timer_mod(s->toy_timer[i], expire_time);
> + }
> +}
> +static void toy_timer_cb(void *opaque)
> +{
> + LS7ARtcState *s = opaque;
> +
> + if (toy_enabled(s)) {
> + qemu_irq_pulse(s->irq);
> + }
> +}
> +
> +static void rtc_timer_cb(void *opaque)
> +{
> + LS7ARtcState *s = opaque;
> +
> + if (rtc_enabled(s)) {
> + qemu_irq_pulse(s->irq);
> + }
Does the real hardware really pulse the IRQ line?
> +}
> +
> +static void ls7a_rtc_realize(DeviceState *dev, Error **errp)
> +{
> + int i;
> + SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> + LS7ARtcState *d = LS7A_RTC(sbd);
> + memory_region_init_io(&d->iomem, NULL, &ls7a_rtc_ops,
> + (void *)d, "ls7a_rtc", 0x100);
> +
> + sysbus_init_irq(sbd, &d->irq);
> +
> + sysbus_init_mmio(sbd, &d->iomem);
> + for (i = 0; i < TIMER_NUMS; i++) {
> + d->toymatch[i] = 0;
> + d->rtcmatch[i] = 0;
> + d->toy_timer[i] = timer_new_ms(rtc_clock, toy_timer_cb, d);
> + d->rtc_timer[i] = timer_new_ms(rtc_clock, rtc_timer_cb, d);
> + }
> + d->offset_toy = 0;
> + d->offset_rtc = 0;
> + d->save_toy_mon = 0;
> + d->save_toy_year = 0;
> + d->save_rtc = 0;
This device is missing an implementation of the reset method,
and a lot of this looks like it is code that ought to be in reset.
> +
> + create_unimplemented_device("mmio fallback 1", 0x10013ffc, 0x4);
This call to create_unimplemented_device() is wrong -- device realize
code must not map anything into the system memory map. That is up to
board or SoC level code to do. I'm not sure what it's trying to do,
but it should be done some other way.
> +}
thanks
-- PMM
- [PULL 14/43] target/loongarch: Add floating point load/store instruction translation, (continued)
- [PULL 14/43] target/loongarch: Add floating point load/store instruction translation, Richard Henderson, 2022/06/06
- [PULL 16/43] target/loongarch: Add disassembler, Richard Henderson, 2022/06/06
- [PULL 36/43] Enable common virtio pci support for LoongArch, Richard Henderson, 2022/06/06
- [PULL 12/43] target/loongarch: Add floating point conversion instruction translation, Richard Henderson, 2022/06/06
- [PULL 15/43] target/loongarch: Add branch instruction translation, Richard Henderson, 2022/06/06
- [PULL 18/43] target/loongarch: Add system emulation introduction, Richard Henderson, 2022/06/06
- [PULL 21/43] target/loongarch: Implement qmp_query_cpu_definitions(), Richard Henderson, 2022/06/06
- [PULL 23/43] target/loongarch: Add LoongArch interrupt and exception handle, Richard Henderson, 2022/06/06
- [PULL 29/43] target/loongarch: Add timer related instructions support., Richard Henderson, 2022/06/06
- [PULL 38/43] hw/loongarch: Add LoongArch ls7a rtc device support, Richard Henderson, 2022/06/06
- Re: [PULL 38/43] hw/loongarch: Add LoongArch ls7a rtc device support,
Peter Maydell <=
- [PULL 22/43] target/loongarch: Add MMU support for LoongArch CPU., Richard Henderson, 2022/06/06
- [PULL 32/43] hw/intc: Add LoongArch ls7a interrupt controller support(PCH-PIC), Richard Henderson, 2022/06/06
- [PULL 20/43] target/loongarch: Add basic vmstate description of CPU., Richard Henderson, 2022/06/06
- [PULL 26/43] target/loongarch: Add LoongArch IOCSR instruction, Richard Henderson, 2022/06/06
- [PULL 35/43] hw/loongarch: Add irq hierarchy for the system, Richard Henderson, 2022/06/06
- [PULL 17/43] target/loongarch: Add target build suport, Richard Henderson, 2022/06/06
- [PULL 39/43] hw/loongarch: Add LoongArch load elf function., Richard Henderson, 2022/06/06
- [PULL 19/43] target/loongarch: Add CSRs definition, Richard Henderson, 2022/06/06
- [PULL 24/43] target/loongarch: Add constant timer support, Richard Henderson, 2022/06/06