HPM unprivileged CSRs

[jm4rtin]

The HPM contains unprivileged read-only shadows of the machine-mode CSRs. It's not entirely clear if these are required to be implemented in a machine mode only configuration to be standard compliant; however, it is recommended that they be included if the matching machine-mode CSR is implemented. See this issue for more details: riscv/riscv-isa-manual#543

EM decided to include these in our internal version of the core due to the linked discussion and because they are implemented in the Imperas ISS. Proposal is to add these CSRs regardless of if the core includes user mode or not. If there is no objections I'll add a PR to add these CSRs as well as implement mcounteren for the eventual enabling of user mode.

[Silabs-ArjanB]

Hi @jm4rtin This topic is not too well described in the RISC-V specs indeed. Originally we thought that the 'counter extension' related to counters/timers, irrespective of privilege. Now it is however quite clear to me: The M-mode CSRs are mandatory. The unprivileged CSRs are what is called the 'counter extension' and we did indeed not implement that . (When we orignally defined the CV32E40P parameters/features we wanted the counter extension and what we actually meant was that we wanted the bare minimum (i.e. M-mode only part) that was needed to replace the RI5CY proprietary performance counters).

To me it seems quite clear that the 'counter extension' (i.e. chapter 10 of the unpriv spec) is optional. Most of the CSRs would be simple shadows, but I would be curious how you would deal with the 'time(h)' CSRs as they are different.

I am not sure how you interpret mcounteren. Whether we implement the Counter extension or not, mcounteren will not impact any CSR as CV32E40P does not have User mode (even not if you would add the CSRs you are asking for). (We did actually already implement mcounteren as it is mandatory, it just is always 0).

My preference would be to not add these CSRs, or if we agree to add them, to guard all related circuitry with a top level parameter.

[jm4rtin]

I see it a bit differently. Yes, the machine-mode CSRs are required but they call all be tied to 0:

All counters should be implemented, but a legal implementation is to hard-wire both the counter and its corresponding event selector to 0.

I read this as the "counter extension" is the implementation of the actual counters and not just a couple unprivileged read-only shadow CSRs. So from this interpretation if you wish to exclude the "counter extension" then you would tie all the counters to 0. If you choose to implement the "counter extension" then you can implement as many counters as you wish; however, it is recommended to include the unprivileged CSRs as well - irrespective of user/supervisor exists in the implementation.

To me it seems quite clear that the 'counter extension' (i.e. chapter 10 of the unpriv spec) is optional. Most of the CSRs would be simple shadows, but I would be curious how you would deal with the 'time(h)' CSRs as they are different.

The time CSR is a bit of an oddball. If you don't tie this implementation to 0 it probably has to be an input into the core. I don't think we want to add memory-mapped things into the core - I consider such things implementation specific. Of course we could provide an example or "standard" solution (just like for eventual CLIC). I would probably take the approach of PULP_SECURE and tie the signal off in the cv32e40p_core top level for inclusion later in CV32E40.

I am not sure how you interpret mcounteren. Whether we implement the Counter extension or not, mcounteren will not impact any CSR as CV32E40P does not have User mode (even not if you would add the CSRs you are asking for). (We did actually already implement mcounteren as it is mandatory, it just is always 0).

I agree mcounteren does not have impact for CV32E40P. It would have impact on CV32E40. I prefer not totally breaking user mode. It's fine to not verify it but if it's simple and/or the designer has time I would rather just implement the user mode portion of features and hide it behind PULP_SECURE (which is a terrible name - a topic for another day). That way it is there when CV32E40 starts and there will hopefully be less headache.

It might be good to have a top level parameter for the HPM but in my view it should remove the actual counters themselves and not just a few unprivileged mode CSRs.

[Silabs-ArjanB]

Hi @jm4rtin Maybe it is easiest if we have a meeting on this topic once both Paul and I are back in office (would be about two weeks from now). I can try to get some clarification on which CSRs are officially part of the 'unprivileged spec counters extension' in the official RISC-V github repos (either by adding to your ticket or by starting a new thread). (In my opinion the statement above about the 'all counters should be implemented' only refers to the 'counters id 3 and upward (as we enabled via a top level parameter', not the 'timers'. Anyway, best to ask the RISC-V organisation indeed as it is their spec. I am all in favor of sticking to the official spec (once we are aligned on what that actually means). (I added a follow up question to https://github.com/riscv/riscv-isa-manual/issues/543)

[jm4rtin]

@Silabs-ArjanB That's fine, there is no rush. Hopefully we will get more clarification from RISC-V in the meantime.

[Silabs-ArjanB]

Hi @jm4rtin , @MikeOpenHWGroup, as we are not getting any clear response on our questions related to this issue in the RISC-V organization, it would probably be fastest/easiest from an RTL point of view to just implement the unprivileged CSRs.

I discussed it with @silabs-PaulZ yesterday and our preference for 'time' would be to implement it as allowed by the Privileged spec: 'Implementations can convert reads of the time CSR into loads to the memory-mapped mtime register'. This would then imply that we add a new pin 'mtime_addr_i' on the top level such that the correct load can be generated in the decoder. As adding the requested unprivileged CSRs will be negligible from an area point of view we would be fine if no new top level parameter is added (and the CSRs are added unconditionally).

As far as I am concerned it is Mike's call (whatever is least verification work):

We go ahead now and we add the mentioned unprivileged CSRs and verify them (but I believe they are part of https://github.com/openhwgroup/core-v-verif/pull/187 already, so it is probably not a big deal either to verify them), or we wait until we get a clear answer on whether these CSRs are required (and if so, we still add them and verify them, and if not, we stick to the current RTL).

[MikeOpenHWGroup]

I'm confused. mtime is not listed as a CSR in the user manual and I am not aware of a pull-request to add it. Its not included in any verification planning at this time.

Perhaps I am really confused and this discussions references a specific field of a different CSR, but I cannot find that anywhere either.

[jm4rtin]

@MikeOpenHWGroup Yes, mtime is not implemented in the core or listed in the user manual but it is part of the RISC-V privileged specification in section 3.1.10. The question is how to make the HPM compliant with the privileged specification (and to a lesser extent the ISS). There are two main things:

1. Unprivileged shadow CSRs. For example the CSR hpmcounter3 is a read-only unprivileged shadow of the mhpmcounter3 privileged CSR. These are unprivileged registers, not user mode registers. As such, even if user mode does not exist the consensus seems to be (correct me if I am wrong) these should be present if the machine mode only privileged version / HPM is implemented. This came from the following discussion (which has not been fully resolved yet): riscv/riscv-isa-manual#543
2. If the unprivileged version of these registers is implemented that means the time CSR should also exist. This is a read-only shadow of mtime. Here, mtime is not actually a CSR but a memory-mapped register. This memory-mapped register mtime doesn't exist in the current core.

[MikeOpenHWGroup]

Thanks @jm4rtin that clears up a lot of confusion for me. Dumb question: is there any reason we should should not follow the ibex implementation in this regard? To wit:

The User Mode time(h) registers are not implemented in Ibex. Any access to these registers will trap.
It is recommended that trap handler software provides a means of accessing platform-defined
mtime(h) timers where available.

I understand our situation is somewhat different as we do not support User mode. Nevertheless, this looks to be a simpler path.

[Silabs-ArjanB]

Hi @jm4rtin , I again discussed with Paul and I checked the Ariane implementation (which is similar to Ibex). We are fine with trap implementation for time(h) as well (the exception handler for this will be relatively difficult however because our mtval is always 0). So, if you want you can go ahead with a pull request for the timer/counter unprivileged CSRs (no parameter is required as this will hardly add any gates). Could you fix #448 in the same pull request?

[jm4rtin]

@Silabs-ArjanB CSR mcounteren is still there for PULP_SECURE=1 (but removed for a machine mode only configuration) and is behaving as expected. Hopefully there will be less design work when we do CV32E40 later :smile: Feel free to review the PR (#450). Let me know if you want the documentation to mention the (m)time CSR should be implemented as an exception handler (maybe propose where to put it?).

[Silabs-ArjanB]

Hi @jm4rtin The related pull request looks good, thanks! I also reviewed the doc and indeed proposed to add some time(h) remarks to be added in the Counters chapter.

[jm4rtin]

For the record, I don't think the "Ibex" approach to the mtime and time(h) registers is the best. Such an implementation requires a decent amount of software to decode the instruction and take corrective action. It would be very latent. Not saying change it, just that it's worth looking at again in the future.

[Silabs-ArjanB]

@jm4rtin Agreed, the software approach is very messy. It would already help if we would actually implement mtval completely (in the future).

[Silabs-ArjanB]

Hi @jm4rtin https://github.com/openhwgroup/cv32e40p/pull/450 has been merged; can this issue be closed now?