RE: [PATCH v2 1/3] hw/watchdog: wdt_ibex_aon.c: Implement the watchdog for the OpenTitan

[Dong, Eddie]

Hi Tyler:

> +}
> +
> +/* Called when the bark timer expires */ static void
> +ibex_aon_barker_expired(void *opaque) {
This may happen during ibex_aon_update_count(), right? 

> +    IbexAONTimerState *s = IBEX_AON_TIMER(opaque);
> +    if (ibex_aon_update_count(s) &&
        This may happen during ibex_aon_update_count().
        Nested call may lead to incorrect s->regs[R_WDOG_COUNT] & s->wdog_last. 

    Can you elaborate? The timers for bark and bite are not updated in 
"update_count".

When 1st ibex_aon_update_count() is executing, and is in the place PPP (after 
updating s->regs[R_WDOG_COUNT] but before updating s->wdog_last), a timer 
(barker) may happen.
Inside ibex_aon_barker_expired(), it calls ibex_aon_update_count() again (nest 
call), and update s->regs[R_WDOG_COUNT] & s->wdog_last, with the new value.
After the nest execution ends, and returns to the initial point (PPP) , the 
s->wdog_last is updated (with the value of 1st execution time), this leads to 
mismatch of s->regs[R_WDOG_COUNT] & s->wdog_last.

This case may not be triggered at normal case, but if the guest read 
A_WDOG_COUNT, the 1st ibex_aon_update_count() does execute, and bring the 
potential issue.

I think we can solve the problem, by not updating s->regs[R_WDOG_COUNT] & 
s->wdog_last in the timer call back API.  The update is not necessary given 
that the stored value is anyway not the current COUNT. We only need to update 
when the guest write the COUNT.


> +        s->regs[R_WDOG_COUNT] >= s->regs[R_WDOG_BARK_THOLD]) {
> +        s->regs[R_INTR_STATE] |= (1 << 1);
> +        qemu_irq_raise(s->bark_irq);
> +    }
> +}
> +


THX  Eddie