qemu-trivial
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-trivial] [Qemu-devel] [PATCH] RISC-V: Correct typo in RV32 per


From: Michael Clark
Subject: Re: [Qemu-trivial] [Qemu-devel] [PATCH] RISC-V: Correct typo in RV32 perf counters
Date: Mon, 30 Jul 2018 23:42:06 +1200



On Mon, 30 Jul 2018 at 10:46 PM, Peter Maydell <address@hidden> wrote:
On 25 May 2018 at 14:17, Richard Henderson <address@hidden> 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 <address@hidden>
>> Signed-off-by: Michael Clark <address@hidden>
>> ---
>>  target/riscv/op_helper.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Fixes: Coverity CID 1390849
> Reviewed-by: Richard Henderson <address@hidden>

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

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.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]