[PULL,26/30] target/riscv: Do not setup pmu timer if OF is disabled

Commit Message

Alistair Francis July 18, 2024, 2:10 a.m. UTC

From: Atish Patra <[e-mail]>

The timer is setup function is invoked in both hpmcounter
write and mcountinhibit write path. If the OF bit set, the
LCOFI interrupt is disabled. There is no benefitting in
setting up the qemu timer until LCOFI is cleared to indicate
that interrupts can be fired again.

Reviewed-by: Daniel Henrique Barboza <[e-mail]>
Signed-off-by: Atish Patra <[e-mail]>
Message-ID: <[e-mail]>
Signed-off-by: Alistair Francis <[e-mail]>
---
 target/riscv/pmu.c | 56 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 44 insertions(+), 12 deletions(-)

Comments

Peter Maydell July 20, 2024, 3:19 p.m. UTC | #1

On Thu, 18 Jul 2024 at 03:15, Alistair Francis <[e-mail]> wrote:
>
> From: Atish Patra <[e-mail]>
>
> The timer is setup function is invoked in both hpmcounter
> write and mcountinhibit write path. If the OF bit set, the
> LCOFI interrupt is disabled. There is no benefitting in
> setting up the qemu timer until LCOFI is cleared to indicate
> that interrupts can be fired again.
> Reviewed-by: Daniel Henrique Barboza <[e-mail]>
> Signed-off-by: Atish Patra <[e-mail]>
> Message-ID: <[e-mail]>
> Signed-off-by: Alistair Francis <[e-mail]>
> ---
>  target/riscv/pmu.c | 56 ++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 44 insertions(+), 12 deletions(-)
>
> diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> index a4729f6c53..3cc0b3648c 100644
> --- a/target/riscv/pmu.c
> +++ b/target/riscv/pmu.c
> @@ -416,14 +416,49 @@ int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
>      return 0;
>  }

Hi; I was looking at an issue Coverity flagged up with this code (CID
1558461, 1558463):

> +static bool pmu_hpmevent_is_of_set(CPURISCVState *env, uint32_t ctr_idx)
> +{
> +    target_ulong mhpmevent_val;
> +    uint64_t of_bit_mask;
> +
> +    if (riscv_cpu_mxl(env) == MXL_RV32) {
> +        mhpmevent_val = env->mhpmeventh_val[ctr_idx];
> +        of_bit_mask = MHPMEVENTH_BIT_OF;
> +     } else {
> +        mhpmevent_val = env->mhpmevent_val[ctr_idx];
> +        of_bit_mask = MHPMEVENT_BIT_OF;

MHPMEVENT_BIT_OF is defined as BIT_ULL(63)...

> +    }
> +
> +    return get_field(mhpmevent_val, of_bit_mask);

...but we pass it to get_field(), whose definition is:

#define get_field(reg, mask) (((reg) & \
                 (uint64_t)(mask)) / ((mask) & ~((mask) << 1)))

Notice that part of this expression is "(mask) << 1". So Coverity complains
that we took a constant value and shifted it right off the top.

I think this is probably a false positive, but why is target/riscv
using its own ad-hoc macros for extracting bitfields? We have
a standard set of extract/deposit macros in bitops.h, and not
using them makes the riscv code harder to read for people who
are used to the rest of the codebase (e.g. to figure out if this
Coverity issue is a false positive I would need to look at these
macros to figure out what exactly they're doing).

thanks
-- PMM

Atish Kumar Patra July 22, 2024, 11:33 p.m. UTC | #2

On Sat, Jul 20, 2024 at 8:19 AM Peter Maydell <[e-mail]> wrote:
>
> On Thu, 18 Jul 2024 at 03:15, Alistair Francis <[e-mail]> wrote:
> >
> > From: Atish Patra <[e-mail]>
> >
> > The timer is setup function is invoked in both hpmcounter
> > write and mcountinhibit write path. If the OF bit set, the
> > LCOFI interrupt is disabled. There is no benefitting in
> > setting up the qemu timer until LCOFI is cleared to indicate
> > that interrupts can be fired again.
> > Reviewed-by: Daniel Henrique Barboza <[e-mail]>
> > Signed-off-by: Atish Patra <[e-mail]>
> > Message-ID: <[e-mail]>
> > Signed-off-by: Alistair Francis <[e-mail]>
> > ---
> >  target/riscv/pmu.c | 56 ++++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 44 insertions(+), 12 deletions(-)
> >
> > diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> > index a4729f6c53..3cc0b3648c 100644
> > --- a/target/riscv/pmu.c
> > +++ b/target/riscv/pmu.c
> > @@ -416,14 +416,49 @@ int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
> >      return 0;
> >  }
>
> Hi; I was looking at an issue Coverity flagged up with this code (CID
> 1558461, 1558463):
>
> > +static bool pmu_hpmevent_is_of_set(CPURISCVState *env, uint32_t ctr_idx)
> > +{
> > +    target_ulong mhpmevent_val;
> > +    uint64_t of_bit_mask;
> > +
> > +    if (riscv_cpu_mxl(env) == MXL_RV32) {
> > +        mhpmevent_val = env->mhpmeventh_val[ctr_idx];
> > +        of_bit_mask = MHPMEVENTH_BIT_OF;
> > +     } else {
> > +        mhpmevent_val = env->mhpmevent_val[ctr_idx];
> > +        of_bit_mask = MHPMEVENT_BIT_OF;
>
> MHPMEVENT_BIT_OF is defined as BIT_ULL(63)...
>
> > +    }
> > +
> > +    return get_field(mhpmevent_val, of_bit_mask);
>
> ...but we pass it to get_field(), whose definition is:
>
> #define get_field(reg, mask) (((reg) & \
>                  (uint64_t)(mask)) / ((mask) & ~((mask) << 1)))
>
> Notice that part of this expression is "(mask) << 1". So Coverity complains
> that we took a constant value and shifted it right off the top.
>
> I think this is probably a false positive, but why is target/riscv
> using its own ad-hoc macros for extracting bitfields? We have
> a standard set of extract/deposit macros in bitops.h, and not

Thanks for pointing those out. I checked the get_field usage from the
beginning of riscv support 6 years back.
There are tons of users of get_field in a bunch of riscv sources. I
guess it was just added once and everybody kept using it
without switching to generic functions.

@Alistair Francis : Are there any other reasons ?

If not, I can take a stab at fixing those if nobody is looking at them already.

> using them makes the riscv code harder to read for people who
> are used to the rest of the codebase (e.g. to figure out if this
> Coverity issue is a false positive I would need to look at these
> macros to figure out what exactly they're doing).
>
> thanks
> -- PMM

Alistair Francis July 23, 2024, 12:43 a.m. UTC | #3

On Tue, Jul 23, 2024 at 9:33 AM Atish Kumar Patra <[e-mail]> wrote:
>
> On Sat, Jul 20, 2024 at 8:19 AM Peter Maydell <[e-mail]> wrote:
> >
> > On Thu, 18 Jul 2024 at 03:15, Alistair Francis <[e-mail]> wrote:
> > >
> > > From: Atish Patra <[e-mail]>
> > >
> > > The timer is setup function is invoked in both hpmcounter
> > > write and mcountinhibit write path. If the OF bit set, the
> > > LCOFI interrupt is disabled. There is no benefitting in
> > > setting up the qemu timer until LCOFI is cleared to indicate
> > > that interrupts can be fired again.
> > > Reviewed-by: Daniel Henrique Barboza <[e-mail]>
> > > Signed-off-by: Atish Patra <[e-mail]>
> > > Message-ID: <[e-mail]>
> > > Signed-off-by: Alistair Francis <[e-mail]>
> > > ---
> > >  target/riscv/pmu.c | 56 ++++++++++++++++++++++++++++++++++++----------
> > >  1 file changed, 44 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> > > index a4729f6c53..3cc0b3648c 100644
> > > --- a/target/riscv/pmu.c
> > > +++ b/target/riscv/pmu.c
> > > @@ -416,14 +416,49 @@ int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
> > >      return 0;
> > >  }
> >
> > Hi; I was looking at an issue Coverity flagged up with this code (CID
> > 1558461, 1558463):
> >
> > > +static bool pmu_hpmevent_is_of_set(CPURISCVState *env, uint32_t ctr_idx)
> > > +{
> > > +    target_ulong mhpmevent_val;
> > > +    uint64_t of_bit_mask;
> > > +
> > > +    if (riscv_cpu_mxl(env) == MXL_RV32) {
> > > +        mhpmevent_val = env->mhpmeventh_val[ctr_idx];
> > > +        of_bit_mask = MHPMEVENTH_BIT_OF;
> > > +     } else {
> > > +        mhpmevent_val = env->mhpmevent_val[ctr_idx];
> > > +        of_bit_mask = MHPMEVENT_BIT_OF;
> >
> > MHPMEVENT_BIT_OF is defined as BIT_ULL(63)...
> >
> > > +    }
> > > +
> > > +    return get_field(mhpmevent_val, of_bit_mask);
> >
> > ...but we pass it to get_field(), whose definition is:
> >
> > #define get_field(reg, mask) (((reg) & \
> >                  (uint64_t)(mask)) / ((mask) & ~((mask) << 1)))
> >
> > Notice that part of this expression is "(mask) << 1". So Coverity complains
> > that we took a constant value and shifted it right off the top.
> >
> > I think this is probably a false positive, but why is target/riscv
> > using its own ad-hoc macros for extracting bitfields? We have
> > a standard set of extract/deposit macros in bitops.h, and not
>
> Thanks for pointing those out. I checked the get_field usage from the
> beginning of riscv support 6 years back.
> There are tons of users of get_field in a bunch of riscv sources. I
> guess it was just added once and everybody kept using it
> without switching to generic functions.

I think you are right about that

>
> @Alistair Francis : Are there any other reasons ?

Not that I know of

>
> If not, I can take a stab at fixing those if nobody is looking at them already.

That would be great!

Alistair

>
> > using them makes the riscv code harder to read for people who
> > are used to the rest of the codebase (e.g. to figure out if this
> > Coverity issue is a false positive I would need to look at these
> > macros to figure out what exactly they're doing).
> >
> > thanks
> > -- PMM

Richard Henderson July 23, 2024, 12:49 a.m. UTC | #4

On 7/23/24 10:43, Alistair Francis wrote:
>>> #define get_field(reg, mask) (((reg) & \
>>>                   (uint64_t)(mask)) / ((mask) & ~((mask) << 1)))
>>>
>>> Notice that part of this expression is "(mask) << 1". So Coverity complains
>>> that we took a constant value and shifted it right off the top.
>>>
>>> I think this is probably a false positive, but why is target/riscv
>>> using its own ad-hoc macros for extracting bitfields? We have
>>> a standard set of extract/deposit macros in bitops.h, and not
>>
>> Thanks for pointing those out. I checked the get_field usage from the
>> beginning of riscv support 6 years back.
>> There are tons of users of get_field in a bunch of riscv sources. I
>> guess it was just added once and everybody kept using it
>> without switching to generic functions.
> 
> I think you are right about that

I think this macro comes from spike, and it was copied in with a bunch of other basic isa 
defines at the start of the qemu port.


r~

Daniel Henrique Barboza July 24, 2024, 7 p.m. UTC | #5

On 7/22/24 8:33 PM, Atish Kumar Patra wrote:
> On Sat, Jul 20, 2024 at 8:19 AM Peter Maydell <[e-mail]> wrote:
>>
>> On Thu, 18 Jul 2024 at 03:15, Alistair Francis <[e-mail]> wrote:
>>>
>>> From: Atish Patra <[e-mail]>
>>>
>>> The timer is setup function is invoked in both hpmcounter
>>> write and mcountinhibit write path. If the OF bit set, the
>>> LCOFI interrupt is disabled. There is no benefitting in
>>> setting up the qemu timer until LCOFI is cleared to indicate
>>> that interrupts can be fired again.
>>> Reviewed-by: Daniel Henrique Barboza <[e-mail]>
>>> Signed-off-by: Atish Patra <[e-mail]>
>>> Message-ID: <[e-mail]>
>>> Signed-off-by: Alistair Francis <[e-mail]>
>>> ---
>>>   target/riscv/pmu.c | 56 ++++++++++++++++++++++++++++++++++++----------
>>>   1 file changed, 44 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
>>> index a4729f6c53..3cc0b3648c 100644
>>> --- a/target/riscv/pmu.c
>>> +++ b/target/riscv/pmu.c
>>> @@ -416,14 +416,49 @@ int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
>>>       return 0;
>>>   }
>>
>> Hi; I was looking at an issue Coverity flagged up with this code (CID
>> 1558461, 1558463):
>>
>>> +static bool pmu_hpmevent_is_of_set(CPURISCVState *env, uint32_t ctr_idx)
>>> +{
>>> +    target_ulong mhpmevent_val;
>>> +    uint64_t of_bit_mask;
>>> +
>>> +    if (riscv_cpu_mxl(env) == MXL_RV32) {
>>> +        mhpmevent_val = env->mhpmeventh_val[ctr_idx];
>>> +        of_bit_mask = MHPMEVENTH_BIT_OF;
>>> +     } else {
>>> +        mhpmevent_val = env->mhpmevent_val[ctr_idx];
>>> +        of_bit_mask = MHPMEVENT_BIT_OF;
>>
>> MHPMEVENT_BIT_OF is defined as BIT_ULL(63)...
>>
>>> +    }
>>> +
>>> +    return get_field(mhpmevent_val, of_bit_mask);
>>
>> ...but we pass it to get_field(), whose definition is:
>>
>> #define get_field(reg, mask) (((reg) & \
>>                   (uint64_t)(mask)) / ((mask) & ~((mask) << 1)))
>>
>> Notice that part of this expression is "(mask) << 1". So Coverity complains
>> that we took a constant value and shifted it right off the top.
>>
>> I think this is probably a false positive, but why is target/riscv
>> using its own ad-hoc macros for extracting bitfields? We have
>> a standard set of extract/deposit macros in bitops.h, and not
> 
> Thanks for pointing those out. I checked the get_field usage from the
> beginning of riscv support 6 years back.
> There are tons of users of get_field in a bunch of riscv sources. I
> guess it was just added once and everybody kept using it
> without switching to generic functions.

I'm not sure about which generic functions we're supposed to use in replace of
get_field/set_field, at least as far as bitops.h goes. extract64/deposit64 has a
different API (it uses start bit + length, not a mask) and it would require
a lot of RISC-V code to be adapted to this format.

Looking a little further I see that registerfields.h has the equivalent of what
we're using here: FIELD_EX64 / FIELD_SEX64. But these macros seems to be tied with
the abstractions used in the header, i.e. registers created with REG32/REG64 and
fields/masks created with the FIELD() macro. The header smmuv3-internal.h makes
use of them so we could use that as a base.

Hopefully Peter/Richard can correct me if I'm wrong and point to the right
direction. Thanks,


Daniel


> 
> @Alistair Francis : Are there any other reasons ?
> 
> If not, I can take a stab at fixing those if nobody is looking at them already.
> 
>> using them makes the riscv code harder to read for people who
>> are used to the rest of the codebase (e.g. to figure out if this
>> Coverity issue is a false positive I would need to look at these
>> macros to figure out what exactly they're doing).
>>
>> thanks
>> -- PMM

Richard Henderson July 24, 2024, 9:30 p.m. UTC | #6

On 7/25/24 05:00, Daniel Henrique Barboza wrote:
> I'm not sure about which generic functions we're supposed to use in replace of
> get_field/set_field, at least as far as bitops.h goes. extract64/deposit64 has a
> different API (it uses start bit + length, not a mask) and it would require
> a lot of RISC-V code to be adapted to this format.
> 
> Looking a little further I see that registerfields.h has the equivalent of what
> we're using here: FIELD_EX64 / FIELD_SEX64. But these macros seems to be tied with
> the abstractions used in the header, i.e. registers created with REG32/REG64 and
> fields/masks created with the FIELD() macro. The header smmuv3-internal.h makes
> use of them so we could use that as a base.
> 
> Hopefully Peter/Richard can correct me if I'm wrong and point to the right
> direction. Thanks,

Yes, registerfields.h is the proper replacement.


r~

Peter Maydell July 25, 2024, 2:05 p.m. UTC | #7

On Sat, 20 Jul 2024 at 16:19, Peter Maydell <[e-mail]> wrote:
>
> On Thu, 18 Jul 2024 at 03:15, Alistair Francis <[e-mail]> wrote:
> >
> > From: Atish Patra <[e-mail]>
> >
> > The timer is setup function is invoked in both hpmcounter
> > write and mcountinhibit write path. If the OF bit set, the
> > LCOFI interrupt is disabled. There is no benefitting in
> > setting up the qemu timer until LCOFI is cleared to indicate
> > that interrupts can be fired again.
> > Reviewed-by: Daniel Henrique Barboza <[e-mail]>
> > Signed-off-by: Atish Patra <[e-mail]>
> > Message-ID: <[e-mail]>
> > Signed-off-by: Alistair Francis <[e-mail]>
> > ---
> >  target/riscv/pmu.c | 56 ++++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 44 insertions(+), 12 deletions(-)
> >
> > diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> > index a4729f6c53..3cc0b3648c 100644
> > --- a/target/riscv/pmu.c
> > +++ b/target/riscv/pmu.c
> > @@ -416,14 +416,49 @@ int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
> >      return 0;
> >  }
>
> Hi; I was looking at an issue Coverity flagged up with this code (CID
> 1558461, 1558463):
>
> > +static bool pmu_hpmevent_is_of_set(CPURISCVState *env, uint32_t ctr_idx)
> > +{
> > +    target_ulong mhpmevent_val;
> > +    uint64_t of_bit_mask;
> > +
> > +    if (riscv_cpu_mxl(env) == MXL_RV32) {
> > +        mhpmevent_val = env->mhpmeventh_val[ctr_idx];
> > +        of_bit_mask = MHPMEVENTH_BIT_OF;
> > +     } else {
> > +        mhpmevent_val = env->mhpmevent_val[ctr_idx];
> > +        of_bit_mask = MHPMEVENT_BIT_OF;
>
> MHPMEVENT_BIT_OF is defined as BIT_ULL(63)...
>
> > +    }
> > +
> > +    return get_field(mhpmevent_val, of_bit_mask);
>
> ...but we pass it to get_field(), whose definition is:
>
> #define get_field(reg, mask) (((reg) & \
>                  (uint64_t)(mask)) / ((mask) & ~((mask) << 1)))
>
> Notice that part of this expression is "(mask) << 1". So Coverity complains
> that we took a constant value and shifted it right off the top.
>
> I think this is probably a false positive

Having worked through some examples I'm happy that this is
a false positive and the expression used here does the
right thing even when the mask goes right up to bit 63.
So I've closed the Coverity issues as false positives;
whether we decide it's worth converting the riscv target to
use QEMU's more standard accessor macros is a separate issue.

If we do want to keep these get_field/set_field macros,
could we add a documentation comment that says what they
do, including description of what the arguments are?

thanks
-- PMM

Patch

diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
index a4729f6c53..3cc0b3648c 100644
--- a/target/riscv/pmu.c
+++ b/target/riscv/pmu.c
@@ -416,14 +416,49 @@  int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
     return 0;
 }
 
+static bool pmu_hpmevent_is_of_set(CPURISCVState *env, uint32_t ctr_idx)
+{
+    target_ulong mhpmevent_val;
+    uint64_t of_bit_mask;
+
+    if (riscv_cpu_mxl(env) == MXL_RV32) {
+        mhpmevent_val = env->mhpmeventh_val[ctr_idx];
+        of_bit_mask = MHPMEVENTH_BIT_OF;
+     } else {
+        mhpmevent_val = env->mhpmevent_val[ctr_idx];
+        of_bit_mask = MHPMEVENT_BIT_OF;
+    }
+
+    return get_field(mhpmevent_val, of_bit_mask);
+}
+
+static bool pmu_hpmevent_set_of_if_clear(CPURISCVState *env, uint32_t ctr_idx)
+{
+    target_ulong *mhpmevent_val;
+    uint64_t of_bit_mask;
+
+    if (riscv_cpu_mxl(env) == MXL_RV32) {
+        mhpmevent_val = &env->mhpmeventh_val[ctr_idx];
+        of_bit_mask = MHPMEVENTH_BIT_OF;
+     } else {
+        mhpmevent_val = &env->mhpmevent_val[ctr_idx];
+        of_bit_mask = MHPMEVENT_BIT_OF;
+    }
+
+    if (!get_field(*mhpmevent_val, of_bit_mask)) {
+        *mhpmevent_val |= of_bit_mask;
+        return true;
+    }
+
+    return false;
+}
+
 static void pmu_timer_trigger_irq(RISCVCPU *cpu,
                                   enum riscv_pmu_event_idx evt_idx)
 {
     uint32_t ctr_idx;
     CPURISCVState *env = &cpu->env;
     PMUCTRState *counter;
-    target_ulong *mhpmevent_val;
-    uint64_t of_bit_mask;
     int64_t irq_trigger_at;
     uint64_t curr_ctr_val, curr_ctrh_val;
     uint64_t ctr_val;
@@ -439,12 +474,9 @@  static void pmu_timer_trigger_irq(RISCVCPU *cpu,
         return;
     }
 
-    if (riscv_cpu_mxl(env) == MXL_RV32) {
-        mhpmevent_val = &env->mhpmeventh_val[ctr_idx];
-        of_bit_mask = MHPMEVENTH_BIT_OF;
-     } else {
-        mhpmevent_val = &env->mhpmevent_val[ctr_idx];
-        of_bit_mask = MHPMEVENT_BIT_OF;
+    /* Generate interrupt only if OF bit is clear */
+    if (pmu_hpmevent_is_of_set(env, ctr_idx)) {
+        return;
     }
 
     counter = &env->pmu_ctrs[ctr_idx];
@@ -477,9 +509,7 @@  static void pmu_timer_trigger_irq(RISCVCPU *cpu,
     }
 
     if (cpu->pmu_avail_ctrs & BIT(ctr_idx)) {
-        /* Generate interrupt only if OF bit is clear */
-        if (!(*mhpmevent_val & of_bit_mask)) {
-            *mhpmevent_val |= of_bit_mask;
+        if (pmu_hpmevent_set_of_if_clear(env, ctr_idx)) {
             riscv_cpu_update_mip(env, MIP_LCOFIP, BOOL_TO_MASK(1));
         }
     }
@@ -502,7 +532,9 @@  int riscv_pmu_setup_timer(CPURISCVState *env, uint64_t value, uint32_t ctr_idx)
     RISCVCPU *cpu = env_archcpu(env);
     PMUCTRState *counter = &env->pmu_ctrs[ctr_idx];
 
-    if (!riscv_pmu_counter_valid(cpu, ctr_idx) || !cpu->cfg.ext_sscofpmf) {
+    /* No need to setup a timer if LCOFI is disabled when OF is set */
+    if (!riscv_pmu_counter_valid(cpu, ctr_idx) || !cpu->cfg.ext_sscofpmf ||
+        pmu_hpmevent_is_of_set(env, ctr_idx)) {
         return -1;
     }