On Mon, Aug 3, 2020 at 10:57 PM Anup Patel <Anup.Patel@...> wrote:

Hi Greg,

 

We have SBI_PMU_COUNTER_START/STOP calls where the SBI implementation will update the MCOUNTINHIBIT bits.

The SBI_PMU_COUNTER_START call also take parameter for initial value of counter so we don’t need HPMCOUNTER CSR to be writeable in S-mode and this will also avoid alias CSRs.

 

I think we can remove BIT[28] in your proposal.

 

Regards,

Anup

 

From: tech-privileged@... <tech-privileged@...> On Behalf Of Greg Favor
Sent: 04 August 2020 10:42
To: Anup Patel <Anup.Patel@...>
Cc: alankao <alankao@...>; tech-privileged@...
Subject: Re: [RISC-V] [tech-privileged] A proposal to enhance RISC-V HPM (Hardware Performance Monitor)

 

Anup,

 

Thanks.  Comments below.

 

Greg

 

On Mon, Aug 3, 2020 at 9:25 PM Anup Patel <Anup.Patel@...> wrote:

Hi Greg,

 

Few comments on your proposal (https://lists.riscv.org/g/tech-privileged/message/205):

 

1. The BIT[31] is not required because we already have MCOUNTINHIBIT CSR

 

This is up in the air for inclusion or not in the proposal.  As solely a bit that software can set/clear to start/stop a counter, the argument for having this bit is weak.  Although SBI calls for writing to the mhpmevent CSR for a counter would need some way to recognize when the associated bit in mcountinhibit needs to be set or cleared.  But with this Active bit in mhpmevent itself, no special support is needed (i.e. the writing of event_info into the upper part of mhpmevent takes care of whatever all bits are there).

 

The argument for this bit in mhpmevent grows when one allows for hardware setting and clearing of the bit.  For example, in response to a cross-trigger from the debug Trigger Module, e.g. to start counting when a certain instruction executed and to stop counting when another address is executed.  Or to start/stop counting in response to another counter overflowing after N occurrences of some event.  In essence, for counting more complex types of event conditions, particularly in debug scenarios and less so in straight perf mon scenarios.

 

Currently cross-trigger capabilities like these aren't standardized but, irrespective of whether they get standardized or not, having a standard Active bit provides the framework for a design to have whatever mechanisms it desires.  And note that hardware manipulation of mcountinhibit bits would be a change to the architectural definition of mcountinhibit.  This isn't a forcing issue, but having this Active bit in mhpmevent sidesteps that issue.

 

But even with all this, it is still up in the air whether people want or don't want to standardize this separate counter control bit as part of a counter extension.  We'll see where people fall on this.

 

2. The BIT[28] contradicts CSR number semantics of HPMCOUNTER CSR because currently all HPMCOUNTER CSRs are “User-Read-Only”.

 

Good point.  To support this feature (which some others have also been requesting) will require defining an alias CSR for each hpmcounter CSR that is "User-Read-Write".

 

Having two User aliases of the same CSR is conceptually not pretty, but this is simple and seems like a necessary evil for supporting this feature.

 

Like above, we'll have to see if the interest in this feature is significant enough to warrant adding read/write hpmcounter aliases.

 

3. We need to align “event_info” definition in SBI PMU Extension to consider your prosed bits in MHPMEVENT CSRs.

 

In my mind event_info simply fills in all the higher bits of mhpmevent that are not written by event_idx - which I believe was to be the default code path in the SBI PMU code.  (This, of course, applies for future implementations that choose to organize their mhpmevent registers in this simple manner.  Implementations are free to organize their mhpmevent CSR differently and supply corresponding implementation-specific SBI code.)  In other words (for RV64):

 

mhpmevent[19:16] = event_idx.type

mhpmevent[15:  0] = event_idx.code

mhpmevent[63:20] = event_info[43:0]

 

Greg

 

 

Regards,

Anup