riscvarchive / riscv-qemu

QEMU with RISC-V (RV64G, RV32G) Emulation Support
385 stars 154 forks source link

Hardware Performance Monitor Privileged V1.10 Support #98

Closed song10 closed 6 years ago

song10 commented 6 years ago

My riscv-qemu HEAD is 05e0ed

The read access logic for CRS_INSTRET, and CSR_CYCLE are priv-1.9.1 only, though HPM CSRs for both priv-1.9.1 and priv-1.10 are there.

michaeljclark commented 6 years ago

Can you be more specific about what the particular issue is?

The HPM CSRs all return 0, if enabled and where introduced in privileged ISA v1.9.1.

There have been changes in this area recently, towards spec compliance. Can you try this branch:

CSR_TIME/CSR_TIMEH are now emulated by bbl the same way as spike handles them.

NonerKao commented 6 years ago

@michaeljclark I checkout the branch. To be specific:

Any comments are appreciated.

michaeljclark commented 6 years ago

We've changed mcycle[h], minstret[h] and misa to discard writes and removing the messages. Writes to these registers are optional.

https://github.com/riscv/riscv-qemu/pull/115

Okay. I see what you mean now:

Yes, it appears that we have the v1.9.1 behavior. It needs some conditional logic added to implement the priv-v1.10 behavior. We have both sets of variables but are missing the conditional logic to enable to disable the respective set of CSRs and to read the new CSRs in priv v1.10

I can work on a fix.

NonerKao commented 6 years ago

Thanks for the help. This is a critical feature for developing perf at this stage.

michaeljclark commented 6 years ago

There is a fix for this in the qemu-2.13-bug-fixes branch:

michaeljclark commented 6 years ago

This is fixed in master (https://github.com/riscv/riscv-qemu/commit/8c59f5c1b5aabbad92871bf62bb302fef017e322) so closing this issue.

Note the new mcounteren/scounteren CSRs will now work correctly for privileged ISA v1.10 as the counteren state is now shared between privileged ISA v1.9.1 and v1.10, however, the unsupported performance counters will simply return 0. It should be enough to allow testing perf.