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.

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

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
> 

On Wed, 4 Nov 2020 at 07:10, Stafford Horne <shorne@gmail.com> wrote:
>
> 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.

My analysis is in the commit message -- the timer_update() function
will just happily do nothing if the timer is disabled. It seemed
cleaner to me to let the timer function just do the right thing
rather than having both layers of the code try to figure out if
there's no action to take.

> The code should be something like ((env->ttmr >> 30) == TIMER_NONE). But I
> haven't tested it.

TIMER_NONE and the other TIMER_* fields are defined as (n << 30),
and the mask TTMR_M is (3 << 30), so "(env->ttmr & TTMR_M) == TIMER_NONE"
would be the condition to check if we wanted to do it here. (That
matches how the code earlier in the function does it.)

thanks
-- PMM

On Wed, Nov 04, 2020 at 10:37:17AM +0000, Peter Maydell wrote:
> On Wed, 4 Nov 2020 at 07:10, Stafford Horne <shorne@gmail.com> wrote:
> >
> > 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.
> 
> My analysis is in the commit message -- the timer_update() function
> will just happily do nothing if the timer is disabled. It seemed
> cleaner to me to let the timer function just do the right thing
> rather than having both layers of the code try to figure out if
> there's no action to take.

If that's the case, then definitely remove it.  Sorry, I was just going off the
patch and didn't look into the code.

> > The code should be something like ((env->ttmr >> 30) == TIMER_NONE). But I
> > haven't tested it.
> 
> TIMER_NONE and the other TIMER_* fields are defined as (n << 30),
> and the mask TTMR_M is (3 << 30), so "(env->ttmr & TTMR_M) == TIMER_NONE"
> would be the condition to check if we wanted to do it here. (That
> matches how the code earlier in the function does it.)

Yeah, that is want I would want to do.  As I couldn't remember if there was a
mask variable available I just came up with the shift alternative.  Sorry, I was
in a bit of a hurry.

As for the patch.

Acked-by: Stafford Horne <shorne@gmail.com>

On Wed, 4 Nov 2020 at 11:44, Stafford Horne <shorne@gmail.com> wrote:
> As for the patch.
>
> Acked-by: Stafford Horne <shorne@gmail.com>

Thanks; I'll take this via target-arm.next since I'm putting
together a pullreq at the moment.

-- PMM