On Tue, 2020-06-30 at 17:08 +0530, Anup Patel wrote: > On Tue, Jun 30, 2020 at 3:48 PM Anup Patel <a...@brainfault.org> > wrote: > > On Tue, Jun 30, 2020 at 1:34 PM Zong Li <zong...@sifive.com> wrote: > > > On Tue, Jun 30, 2020 at 3:40 PM Anup Patel <a...@brainfault.org> > > > wrote: > > > > On Tue, Jun 30, 2020 at 12:07 PM Zong Li <zong...@sifive.com> > > > > wrote: > > > > > On Mon, Jun 29, 2020 at 9:23 PM Anup Patel < > > > > > a...@brainfault.org> wrote: > > > > > > On Mon, Jun 29, 2020 at 6:23 PM Zong Li <zong...@sifive.com > > > > > > > wrote: > > > > > > > On Mon, Jun 29, 2020 at 4:28 PM Anup Patel < > > > > > > > a...@brainfault.org> wrote: > > > > > > > > On Mon, Jun 29, 2020 at 11:22 AM Zong Li < > > > > > > > > zong...@sifive.com> wrote: > > > > > > > > > On Mon, Jun 29, 2020 at 12:53 PM Anup Patel < > > > > > > > > > a...@brainfault.org> wrote: > > > > > > > > > > On Mon, Jun 29, 2020 at 8:49 AM Zong Li < > > > > > > > > > > zong...@sifive.com> wrote: > > > > > > > > > > > This patch set adds raw event support on RISC-V. > > > > > > > > > > > In addition, we > > > > > > > > > > > introduce the DT mechanism to make our perf more > > > > > > > > > > > generic and common. > > > > > > > > > > > > > > > > > > > > > > Currently, we set the hardware events by writing > > > > > > > > > > > the mhpmeventN CSRs, it > > > > > > > > > > > would raise an illegal instruction exception and > > > > > > > > > > > trap into m-mode to > > > > > > > > > > > emulate event selector CSRs access. It doesn't > > > > > > > > > > > make sense because we > > > > > > > > > > > shouldn't write the m-mode CSRs in s-mode. > > > > > > > > > > > Ideally, we should set event > > > > > > > > > > > selector through standard SBI call or the shadow > > > > > > > > > > > CSRs of s-mode. We have > > > > > > > > > > > prepared a proposal of a new SBI extension, > > > > > > > > > > > called "PMU SBI extension", > > > > > > > > > > > but we also discussing the feasibility of > > > > > > > > > > > accessing these PMU CSRs on > > > > > > > > > > > s-mode at the same time, such as delegation > > > > > > > > > > > mechanism, so I was > > > > > > > > > > > wondering if we could use SBI calls first and > > > > > > > > > > > make the PMU SBI extension > > > > > > > > > > > as legacy when s-mode access mechanism is > > > > > > > > > > > accepted by Foundation? or > > > > > > > > > > > keep the current situation to see what would > > > > > > > > > > > happen in the future. > > > > > > > > > > > > > > > > > > > > > > This patch set also introduces the DT mechanism, > > > > > > > > > > > we don't want to add too > > > > > > > > > > > much platform-dependency code in perf like other > > > > > > > > > > > architectures, so we > > > > > > > > > > > put the mapping of generic hardware events to DT, > > > > > > > > > > > then we can easy to > > > > > > > > > > > transfer generic hardware events to vendor's own > > > > > > > > > > > hardware events without > > > > > > > > > > > any platfrom-dependency stuff in our perf. > > > > > > > > > > > > > > > > > > > > Please re-write this series to have RISC-V PMU > > > > > > > > > > driver as a regular > > > > > > > > > > platform driver as drivers/perf/riscv_pmu.c. > > > > > > > > > > > > > > > > > > > > The PMU related sources will have to be removed > > > > > > > > > > from arch/riscv. > > > > > > > > > > > > > > > > > > > > Based on implementation of final > > > > > > > > > > drivers/perf/riscv_pmu.c we will > > > > > > > > > > come-up with drivers/perf/riscv_sbi_pmu.c driver > > > > > > > > > > for SBI perf counters. > > > > > > > > > > > > > > > > > > > > > > > > > > > > There are some different ways to implement perf, and > > > > > > > > > current > > > > > > > > > implementation seems to be consensus when perf was > > > > > > > > > introduced at the > > > > > > > > > beginning [0][1]. I don't persist to which one, I > > > > > > > > > could change the > > > > > > > > > implementation as you mentioned if it is a new > > > > > > > > > consensus one. > > > > > > > > > > > > > > > > > > [0] > > > > > > > > > https://github.com/riscv/riscv-linux/pull/124#issuecomment-367563910 > > > > > > > > > > > > > > > > I would not recommend taking the original RISC-V linux > > > > > > > > fork as reference. > > > > > > > > > > > > > > > > Rather we should study how things are done on other > > > > > > > > architectures. > > > > > > > > > > > > > > > > I really appreciate the attempt to make RISC-V PMU > > > > > > > > driver depend on DT > > > > > > > > but if we are going this route then we should maximize > > > > > > > > the use of Linux > > > > > > > > platform driver framework. In fact, whenever possible > > > > > > > > we should integrate > > > > > > > > RISC-V features as platform drivers under the drivers/ > > > > > > > > directory. > > > > > > > > > > > > > > > > > > > > > > OK, I would change the implementation to platform driver > > > > > > > if there is no > > > > > > > other voice. > > > > > > > > > > > > > > > I thought about SBI PMU counters as well. In future, we > > > > > > > > can easily > > > > > > > > expose SBI PMU counters as RAW events in the same RISC- > > > > > > > > V PMU > > > > > > > > driver. The sbi_probe_extension() can be used in RISC-V > > > > > > > > PMU driver > > > > > > > > to check for SBI PMU counters so no special provisions > > > > > > > > needed in DT > > > > > > > > for SBI PMU counters. > > > > > > > > > > > > > > > > > > > > > > I thought about probing raw events by SBI extension too, > > > > > > > I'm interested if you > > > > > > > have more detail about this. > > > > > > > > > > > > > > It seems to me that it is a little bit hard to return all > > > > > > > events > > > > > > > through one SBI call, > > > > > > > so I thought we could map the generic hardware events and > > > > > > > maintain their own > > > > > > > raw events by each platform in OpenSBI. But eventually, I > > > > > > > thought the > > > > > > > DT mechanism > > > > > > > is more clear and easy than that. Let me know if you have > > > > > > > any ideas about > > > > > > > probe function. Thanks. > > > > > > > > > > > > We can design SBI calls such that no SBI call is required > > > > > > to read > > > > > > the perf counter. > > > > > > > > > > > > The sbi_probe_extension() will only be used to check > > > > > > whether > > > > > > underlying SBI implementation supports SBI PMU extension. > > > > > > > > > > > > As-per my initial thoughts, we can potentially have the > > > > > > following SBI calls: > > > > > > > > > > > > 1. SBI_PMU_NUM_COUNTERS > > > > > > This call will return the number of SBI PMU counters > > > > > > 2. SBI_PMU_COUNTER_DESCRIBE > > > > > > This call takes two parameters: 1) physical address 2) > > > > > > counter index > > > > > > It will write the description of SBI PMU counter at > > > > > > specified > > > > > > physical address. > > > > > > The details of the SBI PMU counter will include name, > > > > > > type, etc > > > > > > > > > > The main things are that we need to pass the information of > > > > > raw events > > > > > and the information of mapping of generic hardware events. > > > > > Maybe > > > > > this information could be passed by this SBI call. > > > > > > > > > > > 3. SBI_PMU_COUNTER_START > > > > > > This call takes two parameters: 1) physical address 2) > > > > > > counter index > > > > > > It will inform SBI implementation to start counting > > > > > > specified counter on the > > > > > > calling HART. The counter value will be written to the > > > > > > specified physical > > > > > > address whenever it changes. > > > > > > > > > > I would prefer to read the counter directly on s-mode. Spec > > > > > already defines the > > > > > mechanism to allow that. But this way would still work if we > > > > > couldn't > > > > > read counters > > > > > on s-mode. > > > > > > > > The SBI PMU counters have nothing to do with RISC-V PMU > > > > counters because > > > > these are counters provided by SBI implementation. > > > > > > > > All-in-all, we have three types of counters: > > > > 1. PMU counters defined by RISC-V privilege spec. These are > > > > TIME, > > > > INSRET, and CYCLE CSRs. > > > > 2. Implementation specific counters accessed via HPMCOUNTER > > > > CSRs. > > > > 3. SBI PMU counters for traps taken and processed by M-mode > > > > runtime > > > > firmware. Examples: number of misaligned load/store, number of > > > > illegal > > > > instructions, number of SBI RFENCE calls, number of SBI IPI > > > > calls, etc. > > > > > > > > The DT based RISC-V PMU platform driver being discussed in this > > > > email > > > > thread only addresses points 1) and 2) above. > > > > > > > > > > OK, sounds good, I misunderstood your ideas, I mixed the 2) and > > > 3) > > > and see them as the same thing. Many thanks for the clear > > > explanation. > > > > Cool, we are on the same page till here. > > > > > > For point 3) above, we need to first define SBI PMU extension. > > > > Once SBI > > > > PMU extension is defined, we can have separate SBI PMU driver > > > > in Linux > > > > or extend RISC-V PMU driver to register additonal counters > > > > based on > > > > SBI PMU extension. > > > > > > > > I never suggested to access RISC-V HPMCOUNTER CSRs via SBI > > > > calls > > > > so DT based RISC-V PMU platform driver (for 1) and 2) above) is > > > > good > > > > to have. The SBI PMU extension is a separate topic. > > > > > > > > > > 4. SBI_PMU_COUNTER_STOP > > > > > > This call takes one parameter: 1) counter index > > > > > > It will inform SBI implementation to stop counting > > > > > > specified counters on > > > > > > the calling HART. > > > > > > > > > > > > The above calls are generic enough to support any number of > > > > > > counters > > > > > > and we don't need any SBI call to read the counter. We can > > > > > > also assume > > > > > > all counters to be of fixed 64bit width. In fact, even > > > > > > Hypervisors can support > > > > > > it's own SBI PMU counters with SBI PMU extension. > > > > > > > > > > > > We still need to think more about the above calls because > > > > > > above SBI > > > > > > calls are just initial ideas. > > > > > > > > > > > > > > > > We also need a SBI call to set the event selector to specify > > > > > which event > > > > > is monitored. > > > > > > > > SBI_PMU_COUNTER_START will do that. > > > > > > I'm not sure whether this SBI call is only for SBI PMU counter > > > and > > > it's own events. > > > For 2), it needs one SBI call to set the events, we just set the > > > event selector > > > by writing m-mode CSRs on s-mode now. If this SBI call could > > > serve 2) > > > and 3) both, > > > we don't need another SBI call. > > > > Can you elaborate more ?? > > > > Is the SBI call for 2) needed to enable/disable counters in > > MCOUNTEREN CSR ? > > > > Currently, OpenSBI enables all counters by default but I see the > > need > > to enable/disable HPMCOUNTER on-demand from perf event start/stop. > > > > I hope we don't need any other implementation specific CSR to be > > programmed > > for enabling/disabling counters on SiFive Unleashed ?? > > > > Here's the next version of SBI PMU extension, which tries to address > both > 2) and 3). In other words, it covers all HPMCOUNTER CSRs and software > counters of SBI implementation. > > To define SBI PMU extension, we first define counter_idx which is a > unique > number assigned to a counter: > 1. counter_idx = 0 to 2 are for CYCLE, TIME, and INSTRET > 2. counter_idx = 3 to 31 are for HPMCOUNTER > 3. counter_idx = 32 or higher are for software counters counters > provided by SBI implementation >
The number of HPMCOUNTER may increase in future. Right ? How about using a higher starting idx for software counters from SBI impolementation ? > The counter_idx == 1 (i.e. TIME CSR) is always enabled when > underlying > HW implements it. Otherwise it is always disabled. > > Based on above definition of counter_idx definition, we can > potentially have > the following SBI calls: > > 1. SBI_PMU_NUM_HPMCOUNTER > This call will return the number of HPMCOUNTER CSRs > 2. SBI_PMU_NUM_SOFTWARE > This call will return the number of software counters provided by > SBI implementation > 3. SBI_PMU_COUNTER_DESCRIBE > This call takes two parameters: 1) counter_idx 2) physical > address > It will write the description of SBI PMU counter at specified > physical address. > The details of the SBI PMU counter will include name, type, > width, > events etc > 4. SBI_PMU_COUNTER_SET_PHYS_ADDR > This call takes two parameters: 1) counter_idx 2) physical > address > It will set the physical address where SBI implementation will > write > the software counter. This SBI call is only for software counters > (i.e. > counter_idx >= 32) so it will fail for other counters. > 5. SBI_PMU_COUNTER_SELECT_EVENT > This call takes two parameters: 1) counter_idx 2) event number > It will select a particular HW event to monitor in a HPMCOUNTER > CSR. > This SBI call is only for HPMCOUNTER CSRs (i.e 3 <= counter_idx > <= 31) > 6. SBI_PMU_COUNTER_START > This call takes one parameter: 1) counter_idx > It will inform SBI implementation to start/enable specified > counter on the > calling HART. This SBI call will fail for counter_idx == 1 and > counters > which are not present. > 7. SBI_PMU_COUNTER_STOP > This call takes one parameter: 1) counter_idx > It will inform SBI implementation to stop/disable specified > counters on > the calling HART. This SBI call will fail for counter_idx == 1 > and counters > which are not present. > > The above described SBI calls can be conveniently implemented in > M-mode runtime firmware (OpenSBI) and various hypervisors (Xvisor, > KVM, etc). > > We can have a single RISC-V PMU driver using above SBI calls which > can be used natively in HS-mode and Guest/VM in VS-mode. Of course, > we won't need any information to be passed in DT/ACPI for this driver > and it can be under arch/riscv/kernel because without DT/ACPI it > can't > be a platform driver. We still need the information in DT for mapping generic hardware events. No ? > The availability of SBI PMU extension can be checked > using sbi_probe_extension() SBI call. > > Regards, > Anup > > _______________________________________________ > linux-riscv mailing list > linux-ri...@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv -- Regards, Atish