Re: [PATCH] target/openrisc: Remove dead code attempting to check "is timer disabled"

[Stafford Horne]

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.maydell@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
>