riscv/riscv-isa-manual

Does mcountinhibit Inhibit Only Unprivileged Counters, or Both Machine and Unprivileged Counters?

cebarobot opened this issue · 9 comments

cebarobot commented

I'm confusing about the mcountinhibit CSR. Does mcountinhibit inhibit only Unprivileged Counters, or both Machine and Unprivileged Counters?

The Privileged ISA manual says:

When the CY, IR, or HPMn bit in the mcountinhibit register is clear, the cycle, instret, or hpmcountern register increments as usual. When the CY, IR, or HPM_n_ bit is set, the corresponding counter does not increment.

It only refers to Unprivileged Counters (cycle, instret, or hpmcountern) and not mensions the Machine Counters (mcycle, minstret, or mhpmcountern) at all.

As cycle, instret and hpmcountern is a read-only shadow of mcycle, minstret and mhpmcountern, there is an intuitive understanding based on the text: "mcountinhibit` only inhibits Unprivileged Counters":

When the CY, IR, or HPM_n_ is set, the hart makes a data copy of mcycle, minstret and mhpmcountern at that moment.
Reading Unprivileged Counters returns that copy, while machine counters keep going.

In that understanding, the purpose of mcountinhibit (reduce energy consumption) make no sense, which leads to more confusion.

I also checked some open-source implementation, and they did as below:

It seems that "inhibit both machine and unprivileged counter" is the correct answer. If so, could we add some clarification on this issue in the ISA manual?

aswaterman commented

I think the ISA manual is already saying the right thing.

The spec states that "The cycle, instret, and hpmcountern CSRs are read-only shadows of mcycle, minstret, and mhpmcountern, respectively." That means that e.g. cycle does not contain its own state. So if cycle stops counting, mcycle must have stopped counting, as well.

It might've been clearer if the mcountinhibit spec referred to e.g. mcycle instead of cycle, but the meaning is the same either way, because of the "read-only shadow" definition.

From what you described, it sounds like all of the open-source implementations you mention are correctly interpreting the spec.

cebarobot commented

It might've been clearer if the mcountinhibit spec referred to e.g. mcycle instead of cycle, but the meaning is the same either way, because of the "read-only shadow" definition.

I agree with that. It's better to refer to mcycle in spec. I'd like to open an PR to fix this.

Also, cycle, instret, or hpmcountern may be non-existent if Zicntr or Zihpm is not implemented. From this perspective, it's also better to use mcycle, minstret and mhpmcountern.

aswaterman commented

Valid point. Would you like to prepare a PR that makes the appropriate changes to this section? I think it’s just a simple search-and-replace; no new text should be needed.

cebarobot commented

Valid point. Would you like to prepare a PR that makes the appropriate changes to this section? I think it’s just a simple search-and-replace; no new text should be needed.

I think it's better if you could just commit the changes on the main branch. Preparing a PR is a bit troublesome after all.

aswaterman commented

I was politely trying to ask you to do the work for me. I guess I’ll try once more.

cebarobot commented

I was politely trying to ask you to do the work for me. I guess I’ll try once more.

Sorry, I just misunderstood it. I'm doing that.

aswaterman commented

Thank you for doing so!

cebarobot commented

Thanks a lot :)