From: Alistair Francis <alistair23@gmail.com>
To: Atish Kumar Patra <atishp@rivosinc.com>
Cc: Peter Maydell <peter.maydell@linaro.org>,
qemu-devel@nongnu.org,
Daniel Henrique Barboza <dbarboza@ventanamicro.com>,
Alistair Francis <alistair.francis@wdc.com>
Subject: Re: [PULL 26/30] target/riscv: Do not setup pmu timer if OF is disabled
Date: Tue, 23 Jul 2024 10:43:46 +1000 [thread overview]
Message-ID: <CAKmqyKM_gcuNvR=2Rff29C5EAFTNiyz3XNTH5H4wt9W5JbmO+w@mail.gmail.com> (raw)
In-Reply-To: <CAHBxVyHZ1_zgg-V2aKMcYsZjWHwcfndt-rbTa0h8mp_Ufe7hjw@mail.gmail.com>
On Tue, Jul 23, 2024 at 9:33 AM Atish Kumar Patra <atishp@rivosinc.com> wrote:
>
> On Sat, Jul 20, 2024 at 8:19 AM Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Thu, 18 Jul 2024 at 03:15, Alistair Francis <alistair23@gmail.com> wrote:
> > >
> > > From: Atish Patra <atishp@rivosinc.com>
> > >
> > > 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 <dbarboza@ventanamicro.com>
> > > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > > Message-ID: <20240711-smcntrpmf_v7-v8-12-b7c38ae7b263@rivosinc.com>
> > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > ---
> > > 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
next prev parent reply other threads:[~2024-07-23 0:45 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-07-18 2:09 [PULL 00/30] riscv-to-apply queue Alistair Francis
2024-07-18 2:09 ` [PULL 01/30] target/riscv: Add zimop extension Alistair Francis
2024-07-18 2:09 ` [PULL 02/30] disas/riscv: Support zimop disassemble Alistair Francis
2024-07-18 2:09 ` [PULL 03/30] target/riscv: Add zcmop extension Alistair Francis
2024-07-18 2:09 ` [PULL 04/30] disas/riscv: Support zcmop disassemble Alistair Francis
2024-07-18 2:09 ` [PULL 05/30] target/riscv: Support Zama16b extension Alistair Francis
2024-07-22 23:32 ` Alistair Francis
2024-07-23 1:15 ` LIU Zhiwei
2024-07-18 2:09 ` [PULL 06/30] target/riscv: Move gen_amo before implement Zabha Alistair Francis
2024-07-18 2:09 ` [PULL 07/30] target/riscv: Add AMO instructions for Zabha Alistair Francis
2024-07-18 2:09 ` [PULL 08/30] target/riscv: Move gen_cmpxchg before adding amocas.[b|h] Alistair Francis
2024-07-18 2:09 ` [PULL 09/30] target/riscv: Add amocas.[b|h] for Zabha Alistair Francis
2024-07-18 2:09 ` [PULL 10/30] target/riscv: Expose zabha extension as a cpu property Alistair Francis
2024-07-18 2:09 ` [PULL 11/30] disas/riscv: Support zabha disassemble Alistair Francis
2024-07-18 2:09 ` [PULL 12/30] target/riscv: Validate the mode in write_vstvec Alistair Francis
2024-07-18 2:09 ` [PULL 13/30] disas/riscv: Add decode for Zawrs extension Alistair Francis
2024-07-18 2:09 ` [PULL 14/30] target/riscv/kvm: update KVM regs to Linux 6.10-rc5 Alistair Francis
2024-07-18 2:09 ` [PULL 15/30] target/riscv: Combine set_mode and set_virt functions Alistair Francis
2024-07-18 2:09 ` [PULL 16/30] target/riscv: Fix the predicate functions for mhpmeventhX CSRs Alistair Francis
2024-07-18 2:09 ` [PULL 17/30] target/riscv: Add cycle & instret privilege mode filtering properties Alistair Francis
2024-07-18 2:10 ` [PULL 18/30] target/riscv: Add cycle & instret privilege mode filtering definitions Alistair Francis
2024-07-18 2:10 ` [PULL 19/30] target/riscv: Add cycle & instret privilege mode filtering support Alistair Francis
2024-07-18 2:10 ` [PULL 20/30] target/riscv: Only set INH fields if priv mode is available Alistair Francis
2024-07-18 2:10 ` [PULL 21/30] target/riscv: Implement privilege mode filtering for cycle/instret Alistair Francis
2024-07-20 14:43 ` Peter Maydell
2024-07-22 23:24 ` Atish Kumar Patra
2025-08-21 9:25 ` Philippe Mathieu-Daudé
2025-10-03 1:39 ` Alistair Francis
2024-07-18 2:10 ` [PULL 22/30] target/riscv: Save counter values during countinhibit update Alistair Francis
2024-07-18 2:10 ` [PULL 23/30] target/riscv: Enforce WARL behavior for scounteren/hcounteren Alistair Francis
2024-07-18 2:10 ` [PULL 24/30] target/riscv: Start counters from both mhpmcounter and mcountinhibit Alistair Francis
2024-07-18 2:10 ` [PULL 25/30] target/riscv: More accurately model priv mode filtering Alistair Francis
2024-07-18 2:10 ` [PULL 26/30] target/riscv: Do not setup pmu timer if OF is disabled Alistair Francis
2024-07-20 15:19 ` Peter Maydell
2024-07-22 23:33 ` Atish Kumar Patra
2024-07-23 0:43 ` Alistair Francis [this message]
2024-07-23 0:49 ` Richard Henderson
2024-07-24 19:00 ` Daniel Henrique Barboza
2024-07-24 21:30 ` Richard Henderson
2024-07-25 14:05 ` Peter Maydell
2024-07-18 2:10 ` [PULL 27/30] target/riscv: Expose the Smcntrpmf config Alistair Francis
2024-07-18 2:10 ` [PULL 28/30] target/riscv: raise an exception when CSRRS/CSRRC writes a read-only CSR Alistair Francis
2024-07-18 2:10 ` [PULL 29/30] hw/riscv/virt.c: re-insert and deprecate 'riscv, delegate' Alistair Francis
2024-07-18 2:10 ` [PULL 30/30] roms/opensbi: Update to v1.5 Alistair Francis
2024-07-18 21:01 ` [PULL 00/30] riscv-to-apply queue Richard Henderson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAKmqyKM_gcuNvR=2Rff29C5EAFTNiyz3XNTH5H4wt9W5JbmO+w@mail.gmail.com' \
--to=alistair23@gmail.com \
--cc=alistair.francis@wdc.com \
--cc=atishp@rivosinc.com \
--cc=dbarboza@ventanamicro.com \
--cc=peter.maydell@linaro.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).