[PATCH] RISC-V: Correct typo in RV32 perf counters

[Michael Clark]

This patch enables mhpmcounter3h through mhpmcounter31h on RV32.
Previously the RV32 h versions (high 32-bits of 64-bit counters)
of these counters would trap with an illegal instruction instead
of returning 0 as intended.

Reported-by: Richard Henderson <r...@twiddle.net>
Signed-off-by: Michael Clark <m...@sifive.com>
---
target/riscv/op_helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index 3abf52453cfc..1f6dc9a85852 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -406,7 +406,7 @@ target_ulong csr_read_helper(CPURISCVState *env, target_ulong csrno)
return 0;
}
#if defined(TARGET_RISCV32)
- if (csrno >= CSR_MHPMCOUNTER3 && csrno <= CSR_MHPMCOUNTER31) {
+ if (csrno >= CSR_MHPMCOUNTER3H && csrno <= CSR_MHPMCOUNTER31H) {
return 0;
}
#endif
--
2.7.0

[Richard Henderson]

On 05/24/2018 11:24 PM, Michael Clark wrote:
> This patch enables mhpmcounter3h through mhpmcounter31h on RV32.
> Previously the RV32 h versions (high 32-bits of 64-bit counters)
> of these counters would trap with an illegal instruction instead
> of returning 0 as intended.
>
> Reported-by: Richard Henderson <r...@twiddle.net>
> Signed-off-by: Michael Clark <m...@sifive.com>
> ---
> target/riscv/op_helper.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)

Fixes: Coverity CID 1390849
Reviewed-by: Richard Henderson <richard....@linaro.org>


r~

[Peter Maydell]

Ping -- Coverity is still complaining about this -- did this
patch get lost?

thanks
-- PMM

[Michael Clark]

Sort of. I assumed it would go into a trivial queue.

Feel free to apply, however it’s going to create (another) rebase conflict against my ‘for-upstream’ queue as this code has gone away, hence it is not in my queue.

The bug is fixed in my tree in an alternate way as we have overhauled the CSR system to support atomic read/modify/write CSRs, and the new code is table driven versus using jumbo switch statements.

When we transcribed this code to the new CSR system the bug disappeared as a consequence of the  nature of the new mechanism which matches the table listings in the RISC-V Privileged ISA manual which makes this type of bug much more obvious:

https://github.com/riscv/riscv-qemu/blob/qemu-for-upstream/target/riscv/csr.c#L912-L919

My tree is up-to-date as of 3.0-rc2, and has been rebased against Alistair’s changes but I have a feeling we are still missing reviews.

We also have additional CSR fixes (such as vectored interrupts) that depend on the context of the new CSR system and we don’t have enough bandwidth to maintain a backport to the old code in upstream QEMU.

I guess it is problematic going via my tree as last time I looked we didn’t have enough reviews.

I can include this change in my pull for 3.1, along with the patches in my tree that have Reviewed-by and fix the rebase conflicts for anything that depends on new context (hopefully not creating new bugs during that process).

I haven’t reposted the code that has not been reviewed as I’ll do that when I have a chance to reorder and split my queue based on what has been reviewed and what hasn’t. 

I’ve deferred that because i’m still working on new code that will be shipping from the SiFive tree (for which i’m currently writing test cases for).

Michael.

[Peter Maydell]

On 30 July 2018 at 12:42, Michael Clark <m...@sifive.com> wrote:
>
>
> On Mon, 30 Jul 2018 at 10:46 PM, Peter Maydell <peter....@linaro.org>
> wrote:
>>
>> On 25 May 2018 at 14:17, Richard Henderson <r...@twiddle.net> wrote:
>> > On 05/24/2018 11:24 PM, Michael Clark wrote:
>> >> This patch enables mhpmcounter3h through mhpmcounter31h on RV32.
>> >> Previously the RV32 h versions (high 32-bits of 64-bit counters)
>> >> of these counters would trap with an illegal instruction instead
>> >> of returning 0 as intended.
>> >>
>> >> Reported-by: Richard Henderson <r...@twiddle.net>
>> >> Signed-off-by: Michael Clark <m...@sifive.com>
>> >> ---
>> >> target/riscv/op_helper.c | 2 +-
>> >> 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > Fixes: Coverity CID 1390849
>> > Reviewed-by: Richard Henderson <richard....@linaro.org>
>>
>> Ping -- Coverity is still complaining about this -- did this
>> patch get lost?
>
>
> Sort of. I assumed it would go into a trivial queue.

As a general rule of thumb, patches which apply to an area
of code which has a maintainer (especially when that area is
in active development, as riscv is), usually go through that
maintainer's tree. Picking up and shepherding patches like this
into master is one of the things maintainers do in QEMU's system.
The -trivial queue mostly exists for things which would otherwise
fall through the cracks between different subsystems.

> Feel free to apply, however it’s going to create (another) rebase conflict
> against my ‘for-upstream’ queue as this code has gone away, hence it is not
> in my queue.

That sort of possibility for conflicts is why it makes more sense
to put even fairly minor patches through maintainer trees rather
than via -trivial).

> I can include this change in my pull for 3.1, along with the patches in my
> tree that have Reviewed-by and fix the rebase conflicts for anything that
> depends on new context (hopefully not creating new bugs during that
> process).

If the code has been removed then you don't need to put this
patch in your pullrequest for 3.1, because you already have
changes that make it moot. (Given that Coverity spotted
it rather than an actual user it doesn't seem like a bug fix
urgent enough to put in 3.0.)

thanks
-- PMM