qemu-devel
[Top][All Lists]
Advanced

[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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]