emulation: emulate counters only if counteren is not set

[poemonsense]

According to the RISC-V spec, [m|s]counteren determines whether or not access to the corresponding hardware counter is permitted in the next implemented privilege mode. When any bit is clear, attempts to read the counter will cause an illegal-instruction exception.

riscv-pk sets all bits of counteren to ones during boot. It double-checks the counteren during emulation of CSR instructions to ensure the corresponding counter is disabled (not permitted) for the next privilege mode.

The emulation aborts if the counteren is set, where the corresponding attempt to read the counter should NOT cause the illegal-instruction exception, and thus we are NOT expected to emulate it.

---

Above is the Git commit messages. I'm writing some words here for explanation.

Here's what I understand on the [m|s]counteren. Each bit of it indicates the existence for next privilege modes (take S mode as an example). It a bit is set, it's legal (no illegal-instruction exception) to read it in S mode. If it is clear, it's illegal to read it in S mode. In the latter case, there will be an illegal-instruction exception, and M mode software may emulate it.

Here's what I understand on riscv-pk. During boot, it sets all bits of [m|s]counteren in machine/minit.c. In machine/emulation.c, in the handler of illegal-instruction exceptions, specifically in the handler for emulated CSR instructions, we read the counteren and decide whether we should emulate them. If the counteren bit is set, there should not be any exception, and thus there should be no need for us to emulate it. So, we return -1 to abort. Only if it is cleared, the counter is illegal in S mode, and we emulate it.

Following these assumption, we first read mcounteren. If the exception was from U-mode, we further read scounteren. Since counters in U-mode are legal only if both of them are set, we use mcounteren & scounteren for U-mode. Next, we will use the counteren bits to decide whether we should and need to emulate the corresponding counter.

So, to sum up, I think the current riscv-pk is wrong in requiring the counteren to be 1 for emulation. It should require the counteren bit to be 0 so that we need to emulate it in M-mode.

There are several related issues. 1) https://github.com/riscv-software-src/riscv-pk/issues/213. For S-mode, we read mcounteren; for U-mode, we read mcounteren & scounteren. 2) https://github.com/riscv-software-src/riscv-pk/pull/37. This is the first PR that introduces this behavior by fixing it. I'm assuming the original version is correct but this PR is incorrectly fixing it.

Since this behavior has been here for at least 7 or 8 years, I'm not quite sure if I'm missing something, given that no one else reports this as a bug. The priv spec has gone through several major revisions. I don't know all the history here.

For us, we are previously running riscv-pk correctly because we don't have write masks for [m|s]counteren. Thus, we always have counteren to be ones after boot, despite not supporting accessing the counters in S and U mode. This violates the RISC-V spec as far as I've understood. Recently after we fix the write masks for [m|s]counteren, we encounter a kernel panic under rdtime. Then, I locate the root cause to this weird check logic in riscv-pk and report it now.

@aswaterman Given you are the designer for both riscv-pk/Spike/RISC-V ISA, would you mind take a look at this? Do I misread the RISC-V spec and/or riscv-pk source code? Much thanks.

If I'm reading it correctly, I'm also wondering why we need these counterens in the ISA spec. S mode does not read mcounteren and thus it does not know whether it can safely execute rdtime. But, M mode reads and writes it for what? Without this CSR, M mode software simply always emulates it. What will happen if the counteren does not exist?

[poemonsense]

I understand your idea on validating whether the OS allows U-mode access. But there seems to be some conflict on the Zicntr extension.

Here're my concerns on the RISC-V spec for these counters:

1) Does Zicntr require the read instructions to not raise any illegal-instruction exceptions? In other words, if Zicntr is implemented, is it RISC-V compatible for the implementation to raise an illegal-instruction exception for rdtime?

2) If Zicntr is not implemented, where I assume the counteren will be read-only zeros, should riscv-pk emulate the counters?

Since RISC-V is separating the counters into a separate and optional ISA extension, the above questions are not addressed in the ISA spec.

[poemonsense]

If the answers for 1) and 2) are both yes, I can change the PR to adding support for checking whether Zicntr is implemented and always allowing emulation if not Zicntr implemented.

[aswaterman]

The problem is that "if Zicntr is implemented" is ambiguous. In general, you have to describe the execution-environment context when describing whether an extension is implemented.

pk is designed to cope with the possibility that Zicntr is not fully implemented at M-mode because rdtime raises an exception. And its job is to make S-mode and U-mode perceive that Zicntr is implemented.

With that said, the counter-enable bits might be writable even if the counter itself is unimplemented. This is necessary to trick S-mode and U-mode into believing that the counter is implemented.

The current code in pk correctly supports the case that Zicntr is implemented and the case that Zicntr is not fully implemented. I do not see the need for any changes.

[poemonsense]

The current code in pk correctly supports the case that Zicntr is implemented and the case that Zicntr is not fully implemented. I do not see the need for any changes.

What about an environment with counteren bits being always read-only zeros? Is it compatible with the RISC-V architecture?

If you are saying that "implemented" of an ISA extension depends in the execution-environment context, I would then suggest the ISA spec adding similar words for the counteren bits as the mstatus.fs. In the ISA spec, mstatus.fs is required to be writable for M-mode software to emulate the FPU. If that's the same case for counters, I suggest adding similar requirements to the counteren CSR. Even if the implementation does not support reading cycle/time/instret in U/S mode at all, but it wants the M-mode software to emulate them (to behave like Zicntr implemented), it should not have the counteren bits read-only zeros. With such words, I believe all implementations should have a writable counteren.

For example, the current Spike implementation will have read-only zero counter-enable bits if the Zicntr extension is not enabled via the ISA string. riscv-pk will fail to emulate the counters for Spike and cause kernel panic during kernel boot. (However, if this is what you want for an implementation that does not even implement anything for Zicntr, if it's expected for an implementation without writable counteren bits to report kernel panic during Linux boot, then riscv-pk requires no changes)

[aswaterman]

I see your point, but I believe you are hitting on a profile issue, rather than an ISA issue. (If we had the concept of profiles years ago, we probably would’ve treated FS the same way.)

The RVA profiles already mandate a 1:1 relationship with writable scounteren bits and accessible counters at the supervisor level. What pk is doing is helping to implement that abstraction for hardware that lacks rdtime.

I’m still not clear on what problem we are trying to solve here. Can we perhaps close this issue?

[poemonsense]

The RVA profiles already mandate a 1:1 relationship with writable scounteren bits and accessible counters at the supervisor level. What pk is doing is helping to implement that abstraction for hardware that lacks rdtime.

Is it Sscounterenw?

I’m still not clear on what problem we are trying to solve here. Can we perhaps close this issue?

I think the only issue here for riscv-pk is, if the hardware have all counter-enable bits read-only zeros, does riscv-pk emulate the counters for it? It is not only about rdtime, but also for rdcycle, etc.

The other issue for ISA is, while you have made it clear, an implementation can claim the Zicntr implementation even if it does not support rdtime (without trapping into M-mode). What about other counters? Can we claim we support Zicntr even if rdcycle will cause an illegal-instruction exception?

Thanks very much for your time. Please feel free to close this issue if the answer to the first question for riscv-pk is "no".

[aswaterman]

I think the only issue here for riscv-pk is, if the hardware have all counter-enable bits read-only zeros, does riscv-pk emulate the counters for it? It is not only about rdtime, but also for rdcycle, etc.

rdtime is the important case, but this codebase does also provide legacy support for systems that have mcycle but not cycle. This is probably very uncommon in current implementations, but since I don't see a great reason to eliminate this compatibility layer, I would prefer that we not do so.

(To answer your second question, no, an execution environment cannot claim Zicntr support if reading cycle incurs a visible trap. But if the environment emulates the access so that the trap becomes invisible, that is conformant to Zicntr. Specifically, implementations that have mcycle but don't have cycle can claim to support Zicntr at S-mode/U-mode if we trap and emulate cycle accesses into mcycle accesses.)

[poemonsense]

Thanks for your response. I've understood the cases here. I now think this is exactly the same case as D/F extensions with mstatus.FS and software FPU emulation.

It can be seen as a profile issue as you have pointed out. I believe you are correct that this should be quite uncommon in modern RISC-V implementations, at least for those targeting the RVA profile.

However, this is not really a pure ISA issue because this includes the interaction with M-mode hardware (SBI issue? environment issue? software issue?). Similarly as what the ISA does for mstatus.FS, I think it would be better if we can add some explanation words to the Zicntr extension ISA (writable is required for emulation).

Thanks again for your patience.

[aswaterman]

I think it would be better if we can add some explanation words to the Zicntr extension ISA

I do see the motivation, but of course any normative change is problematic. So it's best to define new extensions that restrict the behavior, as we did with Sscounterenw, rather than trying to change existing extensions.

Is there value in doing so? The RVA/RVB profiles are already OK in this regard, because of Sscounterenw. It would help M-mode software a little bit, and likewise non-RVA/RVB implementations, but these are already cases where there is limited expectation of software compatibility. Speaking only for myself, I'm not sure this is a problem worth fixing.

Thanks again for your patience.

I think you have been the more patient one, so thank you!

[poemonsense]

Is there value in doing so? The RVA/RVB profiles are already OK in this regard, because of Sscounterenw. It would help M-mode software a little bit, and likewise non-RVA/RVB implementations, but these are already cases where there is limited expectation of software compatibility. Speaking only for myself, I'm not sure this is a problem worth fixing.

Well, I think this simply depends on how RISC-V is going to extend the ISA. If the community agrees more on adding new extensions and mandating them in profiles, it's good. The other way of extending existing extensions is good as well. It's simply a choice to move forward. Clearly RISC-V chooses the former one and it does work well. Hopefully this issue would help others by clarifying the design considerations here.