On Tue, Nov 03, 2020 at 11:46:54AM +0000, Peter Maydell wrote:
> In the mtspr helper we attempt to check for "is the timer disabled"
> with "if (env->ttmr & TIMER_NONE)". This is wrong because TIMER_NONE
> is zero and the condition is always false (Coverity complains about
> the dead code.)
>
> The correct check would be to test whether the TTMR_M field in the
> register is equal to TIMER_NONE instead. However, the
> cpu_openrisc_timer_update() function checks whether the timer is
> enabled (it looks at cpu->env.is_counting, which is set to 0 via
> cpu_openrisc_count_stop() when the TTMR_M field is set to
> TIMER_NONE), so there's no need to check for "timer disabled" in the
> target/openrisc code. Instead, simply remove the dead code.
Thanks for the patch!
I think the check is needed, but it's coded wrong. The ttmr bits 31,30
are the timer mode. If they are equal to zero (TIMER_NONE) then it means
the timer is disabled and we can skip the timer update.
The code should be something like ((env->ttmr >> 30) == TIMER_NONE). But I
haven't tested it.
I may not have time to look at this, in the next few days so if you want to just
remove the code I am fine with that. It seems to be working fine as is anyway.
Also, we almost always have timers running in my workloads so the optimization
doesn't do much.
-Stafford
> Fixes: Coverity CID 1005812
> Signed-off-by: Peter Maydell <peter.mayd...@linaro.org>
> ---
> target/openrisc/sys_helper.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/target/openrisc/sys_helper.c b/target/openrisc/sys_helper.c
> index d9fe6c59489..41390d046f6 100644
> --- a/target/openrisc/sys_helper.c
> +++ b/target/openrisc/sys_helper.c
> @@ -176,9 +176,6 @@ void HELPER(mtspr)(CPUOpenRISCState *env, target_ulong
> spr, target_ulong rb)
>
> case TO_SPR(10, 1): /* TTCR */
> cpu_openrisc_count_set(cpu, rb);
> - if (env->ttmr & TIMER_NONE) {
> - return;
> - }
> cpu_openrisc_timer_update(cpu);
> break;
> #endif
> --
> 2.20.1
>
