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.
Zong Li (6):
dt-bindings: riscv: Add YAML documentation for PMU
riscv: dts: sifive: Add DT support for PMU
riscv: add definition of hpmcounter CSRs
riscv: perf: Add raw event support
riscv: perf: introduce DT mechanism
riscv: remove PMU menu of Kconfig
.../devicetree/bindings/riscv/pmu.yaml | 59 +++
arch/riscv/Kconfig | 13 -
arch/riscv/boot/dts/sifive/fu540-c000.dtsi | 13 +
arch/riscv/include/asm/csr.h | 58 +++
arch/riscv/include/asm/perf_event.h | 100 ++--
arch/riscv/kernel/Makefile | 2 +-
arch/riscv/kernel/perf_event.c | 471 +++++++++++-------
7 files changed, 471 insertions(+), 245 deletions(-)
create mode 100644 Documentation/devicetree/bindings/riscv/pmu.yaml
--
2.27.0
On Mon, Jun 29, 2020 at 6:23 PM Zong Li <[email protected]> wrote:
>
> On Mon, Jun 29, 2020 at 4:28 PM Anup Patel <[email protected]> wrote:
> >
> > On Mon, Jun 29, 2020 at 11:22 AM Zong Li <[email protected]> wrote:
> > >
> > > On Mon, Jun 29, 2020 at 12:53 PM Anup Patel <[email protected]> wrote:
> > > >
> > > > On Mon, Jun 29, 2020 at 8:49 AM Zong Li <[email protected]> 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
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.
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.
Maybe you can refine the above ideas and send a proposal to the
UnixPlatformSpec mailing list ??
Regards,
Anup
>
> > Also, the RISC-V PMU driver can be implemented such that it will
> > work for RV32, RV64, NoMMU RV32, and NoMMU RV64.
> >
> > Regards,
> > Anup
> >
> > > [1] https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/f19TmCNP6yA
> > >
> > > > Regards,
> > > > Anup
> > > >
> > > > >
> > > > > Zong Li (6):
> > > > > dt-bindings: riscv: Add YAML documentation for PMU
> > > > > riscv: dts: sifive: Add DT support for PMU
> > > > > riscv: add definition of hpmcounter CSRs
> > > > > riscv: perf: Add raw event support
> > > > > riscv: perf: introduce DT mechanism
> > > > > riscv: remove PMU menu of Kconfig
> > > > >
> > > > > .../devicetree/bindings/riscv/pmu.yaml | 59 +++
> > > > > arch/riscv/Kconfig | 13 -
> > > > > arch/riscv/boot/dts/sifive/fu540-c000.dtsi | 13 +
> > > > > arch/riscv/include/asm/csr.h | 58 +++
> > > > > arch/riscv/include/asm/perf_event.h | 100 ++--
> > > > > arch/riscv/kernel/Makefile | 2 +-
> > > > > arch/riscv/kernel/perf_event.c | 471 +++++++++++-------
> > > > > 7 files changed, 471 insertions(+), 245 deletions(-)
> > > > > create mode 100644 Documentation/devicetree/bindings/riscv/pmu.yaml
> > > > >
> > > > > --
> > > > > 2.27.0
> > > > >
On Mon, Jun 29, 2020 at 4:28 PM Anup Patel <[email protected]> wrote:
>
> On Mon, Jun 29, 2020 at 11:22 AM Zong Li <[email protected]> wrote:
> >
> > On Mon, Jun 29, 2020 at 12:53 PM Anup Patel <[email protected]> wrote:
> > >
> > > On Mon, Jun 29, 2020 at 8:49 AM Zong Li <[email protected]> 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.
> Also, the RISC-V PMU driver can be implemented such that it will
> work for RV32, RV64, NoMMU RV32, and NoMMU RV64.
>
> Regards,
> Anup
>
> > [1] https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/f19TmCNP6yA
> >
> > > Regards,
> > > Anup
> > >
> > > >
> > > > Zong Li (6):
> > > > dt-bindings: riscv: Add YAML documentation for PMU
> > > > riscv: dts: sifive: Add DT support for PMU
> > > > riscv: add definition of hpmcounter CSRs
> > > > riscv: perf: Add raw event support
> > > > riscv: perf: introduce DT mechanism
> > > > riscv: remove PMU menu of Kconfig
> > > >
> > > > .../devicetree/bindings/riscv/pmu.yaml | 59 +++
> > > > arch/riscv/Kconfig | 13 -
> > > > arch/riscv/boot/dts/sifive/fu540-c000.dtsi | 13 +
> > > > arch/riscv/include/asm/csr.h | 58 +++
> > > > arch/riscv/include/asm/perf_event.h | 100 ++--
> > > > arch/riscv/kernel/Makefile | 2 +-
> > > > arch/riscv/kernel/perf_event.c | 471 +++++++++++-------
> > > > 7 files changed, 471 insertions(+), 245 deletions(-)
> > > > create mode 100644 Documentation/devicetree/bindings/riscv/pmu.yaml
> > > >
> > > > --
> > > > 2.27.0
> > > >
On Mon, Jun 29, 2020 at 11:22 AM Zong Li <[email protected]> wrote:
>
> On Mon, Jun 29, 2020 at 12:53 PM Anup Patel <[email protected]> wrote:
> >
> > On Mon, Jun 29, 2020 at 8:49 AM Zong Li <[email protected]> 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.
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.
Also, the RISC-V PMU driver can be implemented such that it will
work for RV32, RV64, NoMMU RV32, and NoMMU RV64.
Regards,
Anup
> [1] https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/f19TmCNP6yA
>
> > Regards,
> > Anup
> >
> > >
> > > Zong Li (6):
> > > dt-bindings: riscv: Add YAML documentation for PMU
> > > riscv: dts: sifive: Add DT support for PMU
> > > riscv: add definition of hpmcounter CSRs
> > > riscv: perf: Add raw event support
> > > riscv: perf: introduce DT mechanism
> > > riscv: remove PMU menu of Kconfig
> > >
> > > .../devicetree/bindings/riscv/pmu.yaml | 59 +++
> > > arch/riscv/Kconfig | 13 -
> > > arch/riscv/boot/dts/sifive/fu540-c000.dtsi | 13 +
> > > arch/riscv/include/asm/csr.h | 58 +++
> > > arch/riscv/include/asm/perf_event.h | 100 ++--
> > > arch/riscv/kernel/Makefile | 2 +-
> > > arch/riscv/kernel/perf_event.c | 471 +++++++++++-------
> > > 7 files changed, 471 insertions(+), 245 deletions(-)
> > > create mode 100644 Documentation/devicetree/bindings/riscv/pmu.yaml
> > >
> > > --
> > > 2.27.0
> > >
On Mon, Jun 29, 2020 at 12:53 PM Anup Patel <[email protected]> wrote:
>
> On Mon, Jun 29, 2020 at 8:49 AM Zong Li <[email protected]> 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
[1] https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/f19TmCNP6yA
> Regards,
> Anup
>
> >
> > Zong Li (6):
> > dt-bindings: riscv: Add YAML documentation for PMU
> > riscv: dts: sifive: Add DT support for PMU
> > riscv: add definition of hpmcounter CSRs
> > riscv: perf: Add raw event support
> > riscv: perf: introduce DT mechanism
> > riscv: remove PMU menu of Kconfig
> >
> > .../devicetree/bindings/riscv/pmu.yaml | 59 +++
> > arch/riscv/Kconfig | 13 -
> > arch/riscv/boot/dts/sifive/fu540-c000.dtsi | 13 +
> > arch/riscv/include/asm/csr.h | 58 +++
> > arch/riscv/include/asm/perf_event.h | 100 ++--
> > arch/riscv/kernel/Makefile | 2 +-
> > arch/riscv/kernel/perf_event.c | 471 +++++++++++-------
> > 7 files changed, 471 insertions(+), 245 deletions(-)
> > create mode 100644 Documentation/devicetree/bindings/riscv/pmu.yaml
> >
> > --
> > 2.27.0
> >
On Mon, Jun 29, 2020 at 8:49 AM Zong Li <[email protected]> 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.
Regards,
Anup
>
> Zong Li (6):
> dt-bindings: riscv: Add YAML documentation for PMU
> riscv: dts: sifive: Add DT support for PMU
> riscv: add definition of hpmcounter CSRs
> riscv: perf: Add raw event support
> riscv: perf: introduce DT mechanism
> riscv: remove PMU menu of Kconfig
>
> .../devicetree/bindings/riscv/pmu.yaml | 59 +++
> arch/riscv/Kconfig | 13 -
> arch/riscv/boot/dts/sifive/fu540-c000.dtsi | 13 +
> arch/riscv/include/asm/csr.h | 58 +++
> arch/riscv/include/asm/perf_event.h | 100 ++--
> arch/riscv/kernel/Makefile | 2 +-
> arch/riscv/kernel/perf_event.c | 471 +++++++++++-------
> 7 files changed, 471 insertions(+), 245 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/riscv/pmu.yaml
>
> --
> 2.27.0
>
On Mon, Jun 29, 2020 at 9:23 PM Anup Patel <[email protected]> wrote:
>
> On Mon, Jun 29, 2020 at 6:23 PM Zong Li <[email protected]> wrote:
> >
> > On Mon, Jun 29, 2020 at 4:28 PM Anup Patel <[email protected]> wrote:
> > >
> > > On Mon, Jun 29, 2020 at 11:22 AM Zong Li <[email protected]> wrote:
> > > >
> > > > On Mon, Jun 29, 2020 at 12:53 PM Anup Patel <[email protected]> wrote:
> > > > >
> > > > > On Mon, Jun 29, 2020 at 8:49 AM Zong Li <[email protected]> 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.
> 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.
> Maybe you can refine the above ideas and send a proposal to the
> UnixPlatformSpec mailing list ??
>
Ok, let us talk about the details in that.
> Regards,
> Anup
>
> >
> > > Also, the RISC-V PMU driver can be implemented such that it will
> > > work for RV32, RV64, NoMMU RV32, and NoMMU RV64.
> > >
> > > Regards,
> > > Anup
> > >
> > > > [1] https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/f19TmCNP6yA
> > > >
> > > > > Regards,
> > > > > Anup
> > > > >
> > > > > >
> > > > > > Zong Li (6):
> > > > > > dt-bindings: riscv: Add YAML documentation for PMU
> > > > > > riscv: dts: sifive: Add DT support for PMU
> > > > > > riscv: add definition of hpmcounter CSRs
> > > > > > riscv: perf: Add raw event support
> > > > > > riscv: perf: introduce DT mechanism
> > > > > > riscv: remove PMU menu of Kconfig
> > > > > >
> > > > > > .../devicetree/bindings/riscv/pmu.yaml | 59 +++
> > > > > > arch/riscv/Kconfig | 13 -
> > > > > > arch/riscv/boot/dts/sifive/fu540-c000.dtsi | 13 +
> > > > > > arch/riscv/include/asm/csr.h | 58 +++
> > > > > > arch/riscv/include/asm/perf_event.h | 100 ++--
> > > > > > arch/riscv/kernel/Makefile | 2 +-
> > > > > > arch/riscv/kernel/perf_event.c | 471 +++++++++++-------
> > > > > > 7 files changed, 471 insertions(+), 245 deletions(-)
> > > > > > create mode 100644 Documentation/devicetree/bindings/riscv/pmu.yaml
> > > > > >
> > > > > > --
> > > > > > 2.27.0
> > > > > >
On Tue, Jun 30, 2020 at 12:07 PM Zong Li <[email protected]> wrote:
>
> On Mon, Jun 29, 2020 at 9:23 PM Anup Patel <[email protected]> wrote:
> >
> > On Mon, Jun 29, 2020 at 6:23 PM Zong Li <[email protected]> wrote:
> > >
> > > On Mon, Jun 29, 2020 at 4:28 PM Anup Patel <[email protected]> wrote:
> > > >
> > > > On Mon, Jun 29, 2020 at 11:22 AM Zong Li <[email protected]> wrote:
> > > > >
> > > > > On Mon, Jun 29, 2020 at 12:53 PM Anup Patel <[email protected]> wrote:
> > > > > >
> > > > > > On Mon, Jun 29, 2020 at 8:49 AM Zong Li <[email protected]> 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.
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.
>
> > Maybe you can refine the above ideas and send a proposal to the
> > UnixPlatformSpec mailing list ??
> >
>
> Ok, let us talk about the details in that.
Regards,
Anup
>
>
> > Regards,
> > Anup
> >
> > >
> > > > Also, the RISC-V PMU driver can be implemented such that it will
> > > > work for RV32, RV64, NoMMU RV32, and NoMMU RV64.
> > > >
> > > > Regards,
> > > > Anup
> > > >
> > > > > [1] https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/f19TmCNP6yA
> > > > >
> > > > > > Regards,
> > > > > > Anup
> > > > > >
> > > > > > >
> > > > > > > Zong Li (6):
> > > > > > > dt-bindings: riscv: Add YAML documentation for PMU
> > > > > > > riscv: dts: sifive: Add DT support for PMU
> > > > > > > riscv: add definition of hpmcounter CSRs
> > > > > > > riscv: perf: Add raw event support
> > > > > > > riscv: perf: introduce DT mechanism
> > > > > > > riscv: remove PMU menu of Kconfig
> > > > > > >
> > > > > > > .../devicetree/bindings/riscv/pmu.yaml | 59 +++
> > > > > > > arch/riscv/Kconfig | 13 -
> > > > > > > arch/riscv/boot/dts/sifive/fu540-c000.dtsi | 13 +
> > > > > > > arch/riscv/include/asm/csr.h | 58 +++
> > > > > > > arch/riscv/include/asm/perf_event.h | 100 ++--
> > > > > > > arch/riscv/kernel/Makefile | 2 +-
> > > > > > > arch/riscv/kernel/perf_event.c | 471 +++++++++++-------
> > > > > > > 7 files changed, 471 insertions(+), 245 deletions(-)
> > > > > > > create mode 100644 Documentation/devicetree/bindings/riscv/pmu.yaml
> > > > > > >
> > > > > > > --
> > > > > > > 2.27.0
> > > > > > >
On Tue, Jun 30, 2020 at 3:40 PM Anup Patel <[email protected]> wrote:
>
> On Tue, Jun 30, 2020 at 12:07 PM Zong Li <[email protected]> wrote:
> >
> > On Mon, Jun 29, 2020 at 9:23 PM Anup Patel <[email protected]> wrote:
> > >
> > > On Mon, Jun 29, 2020 at 6:23 PM Zong Li <[email protected]> wrote:
> > > >
> > > > On Mon, Jun 29, 2020 at 4:28 PM Anup Patel <[email protected]> wrote:
> > > > >
> > > > > On Mon, Jun 29, 2020 at 11:22 AM Zong Li <[email protected]> wrote:
> > > > > >
> > > > > > On Mon, Jun 29, 2020 at 12:53 PM Anup Patel <[email protected]> wrote:
> > > > > > >
> > > > > > > On Mon, Jun 29, 2020 at 8:49 AM Zong Li <[email protected]> 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.
> 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.
>
> >
> > > Maybe you can refine the above ideas and send a proposal to the
> > > UnixPlatformSpec mailing list ??
> > >
> >
> > Ok, let us talk about the details in that.
>
> Regards,
> Anup
>
> >
> >
> > > Regards,
> > > Anup
> > >
> > > >
> > > > > Also, the RISC-V PMU driver can be implemented such that it will
> > > > > work for RV32, RV64, NoMMU RV32, and NoMMU RV64.
> > > > >
> > > > > Regards,
> > > > > Anup
> > > > >
> > > > > > [1] https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/f19TmCNP6yA
> > > > > >
> > > > > > > Regards,
> > > > > > > Anup
> > > > > > >
> > > > > > > >
> > > > > > > > Zong Li (6):
> > > > > > > > dt-bindings: riscv: Add YAML documentation for PMU
> > > > > > > > riscv: dts: sifive: Add DT support for PMU
> > > > > > > > riscv: add definition of hpmcounter CSRs
> > > > > > > > riscv: perf: Add raw event support
> > > > > > > > riscv: perf: introduce DT mechanism
> > > > > > > > riscv: remove PMU menu of Kconfig
> > > > > > > >
> > > > > > > > .../devicetree/bindings/riscv/pmu.yaml | 59 +++
> > > > > > > > arch/riscv/Kconfig | 13 -
> > > > > > > > arch/riscv/boot/dts/sifive/fu540-c000.dtsi | 13 +
> > > > > > > > arch/riscv/include/asm/csr.h | 58 +++
> > > > > > > > arch/riscv/include/asm/perf_event.h | 100 ++--
> > > > > > > > arch/riscv/kernel/Makefile | 2 +-
> > > > > > > > arch/riscv/kernel/perf_event.c | 471 +++++++++++-------
> > > > > > > > 7 files changed, 471 insertions(+), 245 deletions(-)
> > > > > > > > create mode 100644 Documentation/devicetree/bindings/riscv/pmu.yaml
> > > > > > > >
> > > > > > > > --
> > > > > > > > 2.27.0
> > > > > > > >
On Tue, Jun 30, 2020 at 1:34 PM Zong Li <[email protected]> wrote:
>
> On Tue, Jun 30, 2020 at 3:40 PM Anup Patel <[email protected]> wrote:
> >
> > On Tue, Jun 30, 2020 at 12:07 PM Zong Li <[email protected]> wrote:
> > >
> > > On Mon, Jun 29, 2020 at 9:23 PM Anup Patel <[email protected]> wrote:
> > > >
> > > > On Mon, Jun 29, 2020 at 6:23 PM Zong Li <[email protected]> wrote:
> > > > >
> > > > > On Mon, Jun 29, 2020 at 4:28 PM Anup Patel <[email protected]> wrote:
> > > > > >
> > > > > > On Mon, Jun 29, 2020 at 11:22 AM Zong Li <[email protected]> wrote:
> > > > > > >
> > > > > > > On Mon, Jun 29, 2020 at 12:53 PM Anup Patel <[email protected]> wrote:
> > > > > > > >
> > > > > > > > On Mon, Jun 29, 2020 at 8:49 AM Zong Li <[email protected]> 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 ??
Regards,
Anup
>
> >
> > >
> > > > Maybe you can refine the above ideas and send a proposal to the
> > > > UnixPlatformSpec mailing list ??
> > > >
> > >
> > > Ok, let us talk about the details in that.
> >
> > Regards,
> > Anup
> >
> > >
> > >
> > > > Regards,
> > > > Anup
> > > >
> > > > >
> > > > > > Also, the RISC-V PMU driver can be implemented such that it will
> > > > > > work for RV32, RV64, NoMMU RV32, and NoMMU RV64.
> > > > > >
> > > > > > Regards,
> > > > > > Anup
> > > > > >
> > > > > > > [1] https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/f19TmCNP6yA
> > > > > > >
> > > > > > > > Regards,
> > > > > > > > Anup
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Zong Li (6):
> > > > > > > > > dt-bindings: riscv: Add YAML documentation for PMU
> > > > > > > > > riscv: dts: sifive: Add DT support for PMU
> > > > > > > > > riscv: add definition of hpmcounter CSRs
> > > > > > > > > riscv: perf: Add raw event support
> > > > > > > > > riscv: perf: introduce DT mechanism
> > > > > > > > > riscv: remove PMU menu of Kconfig
> > > > > > > > >
> > > > > > > > > .../devicetree/bindings/riscv/pmu.yaml | 59 +++
> > > > > > > > > arch/riscv/Kconfig | 13 -
> > > > > > > > > arch/riscv/boot/dts/sifive/fu540-c000.dtsi | 13 +
> > > > > > > > > arch/riscv/include/asm/csr.h | 58 +++
> > > > > > > > > arch/riscv/include/asm/perf_event.h | 100 ++--
> > > > > > > > > arch/riscv/kernel/Makefile | 2 +-
> > > > > > > > > arch/riscv/kernel/perf_event.c | 471 +++++++++++-------
> > > > > > > > > 7 files changed, 471 insertions(+), 245 deletions(-)
> > > > > > > > > create mode 100644 Documentation/devicetree/bindings/riscv/pmu.yaml
> > > > > > > > >
> > > > > > > > > --
> > > > > > > > > 2.27.0
> > > > > > > > >
On Tue, Jun 30, 2020 at 3:48 PM Anup Patel <[email protected]> wrote:
>
> On Tue, Jun 30, 2020 at 1:34 PM Zong Li <[email protected]> wrote:
> >
> > On Tue, Jun 30, 2020 at 3:40 PM Anup Patel <[email protected]> wrote:
> > >
> > > On Tue, Jun 30, 2020 at 12:07 PM Zong Li <[email protected]> wrote:
> > > >
> > > > On Mon, Jun 29, 2020 at 9:23 PM Anup Patel <[email protected]> wrote:
> > > > >
> > > > > On Mon, Jun 29, 2020 at 6:23 PM Zong Li <[email protected]> wrote:
> > > > > >
> > > > > > On Mon, Jun 29, 2020 at 4:28 PM Anup Patel <[email protected]> wrote:
> > > > > > >
> > > > > > > On Mon, Jun 29, 2020 at 11:22 AM Zong Li <[email protected]> wrote:
> > > > > > > >
> > > > > > > > On Mon, Jun 29, 2020 at 12:53 PM Anup Patel <[email protected]> wrote:
> > > > > > > > >
> > > > > > > > > On Mon, Jun 29, 2020 at 8:49 AM Zong Li <[email protected]> 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 CSRs
3. counter_idx = 32 or higher are for software counters provided by
SBI implementation
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. The availability of SBI PMU extension can be checked
using sbi_probe_extension() SBI call.
Regards,
Anup
On Tue, 2020-06-30 at 17:08 +0530, Anup Patel wrote:
> On Tue, Jun 30, 2020 at 3:48 PM Anup Patel <[email protected]>
> wrote:
> > On Tue, Jun 30, 2020 at 1:34 PM Zong Li <[email protected]> wrote:
> > > On Tue, Jun 30, 2020 at 3:40 PM Anup Patel <[email protected]>
> > > wrote:
> > > > On Tue, Jun 30, 2020 at 12:07 PM Zong Li <[email protected]>
> > > > wrote:
> > > > > On Mon, Jun 29, 2020 at 9:23 PM Anup Patel <
> > > > > [email protected]> wrote:
> > > > > > On Mon, Jun 29, 2020 at 6:23 PM Zong Li <[email protected]
> > > > > > > wrote:
> > > > > > > On Mon, Jun 29, 2020 at 4:28 PM Anup Patel <
> > > > > > > [email protected]> wrote:
> > > > > > > > On Mon, Jun 29, 2020 at 11:22 AM Zong Li <
> > > > > > > > [email protected]> wrote:
> > > > > > > > > On Mon, Jun 29, 2020 at 12:53 PM Anup Patel <
> > > > > > > > > [email protected]> wrote:
> > > > > > > > > > On Mon, Jun 29, 2020 at 8:49 AM Zong Li <
> > > > > > > > > > [email protected]> 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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
--
Regards,
Atish
On Tue, Jun 30, 2020 at 5:52 PM Alan Kao <[email protected]> wrote:
>
> On Mon, Jun 29, 2020 at 11:19:09AM +0800, Zong Li 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.
> >
> > Zong Li (6):
> > dt-bindings: riscv: Add YAML documentation for PMU
> > riscv: dts: sifive: Add DT support for PMU
> > riscv: add definition of hpmcounter CSRs
> > riscv: perf: Add raw event support
> > riscv: perf: introduce DT mechanism
> > riscv: remove PMU menu of Kconfig
> >
>
> DT-based PMU registration looks good to me. Together with Anup's feedback,
> we can anticipate that the following items will be:
>
> - rewrite RISC-V PMU to a platform driver
> - propose SBI PMU extention
> - fixes: RV32 counter access, namings, etc.
>
> Yes, all are good directions towards better counting (`perf stat`) function.
> But as the original author of RISC-V perf port, please allow me to address
> the fundamental problems of RISC-V perf, again [0][1][2][3], that the sampling
> (`perf record`) function never earned enough respect. Counting gives you a
> shallow view regarding an application, while sampling demystifies one for you.
>
> The problems are three-fold
> (1) Interrupt
> Sampling in perf requires that a HPM raises an interrupt when it overflows.
> Making RISC-V perf platform driver or not has nothing to do with this. This
> requires more discussions in TGs.
> (2) S-mode access to PMU CSRs
> This is also addressed in this patch set but to me, it is kind of like a
> SBI-solves-them-all mindset to me. Perf event is for performance monitoring
> thus we should eliminate any possible overhead if we can. Setting event masks
> through SBI calls for counting maybe OK, but if we really take sampling and
> interrupt handling into consideration, it is questionable if it is still a
> viable way.
> (3) Registers, registers, registers
> There is just no enough CSR/function for perf sampling. The previous proposal
> explains why [2].
>
> Perf sampling is off-topic but somehow related, so I bring it up here just
> for your information.
>
> As this patch set goes v2, the PMU porting guide in [0] should be removed since
> it contains no useful information anymore.
>
> [0] Documentation/riscv/pmu.rst
> [1] https://www.youtube.com/watch?v=Onvlcl4e2IU
> [2] https://github.com/riscv/riscv-isa-manual/issues/402
> This proposal has been posted in Privileged Spec Task Group, in
> https://lists.riscv.org/g/tech-privileged-archive/message/488?p=,,,20,0,0,0::Created,,Proposal,20,2,40,32306071
> but never receive any feedback.
> [3] https://lists.riscv.org/g/tech-unixplatformspec/message/84
> I intended to discuss [2] in the Unixplatform Spec Task Group at the
> online meeting, but obviously people were too busy knowing who the new
> RISC-V CTO is and what he has done to even follow the agenda.
>
Sorry. The last meeting's agenda was derailed for numerous reasons.
Are you okay with discussing this during the next meeting ?
I have not scheduled one yet but will probably schedule it on next
Wednesday (8th July) if there is no objection.
I can check with Anup if he can present the SBI PMU extension as well.
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
--
Regards,
Atish
On Mon, Jun 29, 2020 at 11:19:09AM +0800, Zong Li 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.
>
> Zong Li (6):
> dt-bindings: riscv: Add YAML documentation for PMU
> riscv: dts: sifive: Add DT support for PMU
> riscv: add definition of hpmcounter CSRs
> riscv: perf: Add raw event support
> riscv: perf: introduce DT mechanism
> riscv: remove PMU menu of Kconfig
>
DT-based PMU registration looks good to me. Together with Anup's feedback,
we can anticipate that the following items will be:
- rewrite RISC-V PMU to a platform driver
- propose SBI PMU extention
- fixes: RV32 counter access, namings, etc.
Yes, all are good directions towards better counting (`perf stat`) function.
But as the original author of RISC-V perf port, please allow me to address
the fundamental problems of RISC-V perf, again [0][1][2][3], that the sampling
(`perf record`) function never earned enough respect. Counting gives you a
shallow view regarding an application, while sampling demystifies one for you.
The problems are three-fold
(1) Interrupt
Sampling in perf requires that a HPM raises an interrupt when it overflows.
Making RISC-V perf platform driver or not has nothing to do with this. This
requires more discussions in TGs.
(2) S-mode access to PMU CSRs
This is also addressed in this patch set but to me, it is kind of like a
SBI-solves-them-all mindset to me. Perf event is for performance monitoring
thus we should eliminate any possible overhead if we can. Setting event masks
through SBI calls for counting maybe OK, but if we really take sampling and
interrupt handling into consideration, it is questionable if it is still a
viable way.
(3) Registers, registers, registers
There is just no enough CSR/function for perf sampling. The previous proposal
explains why [2].
Perf sampling is off-topic but somehow related, so I bring it up here just
for your information.
As this patch set goes v2, the PMU porting guide in [0] should be removed since
it contains no useful information anymore.
[0] Documentation/riscv/pmu.rst
[1] https://www.youtube.com/watch?v=Onvlcl4e2IU
[2] https://github.com/riscv/riscv-isa-manual/issues/402
This proposal has been posted in Privileged Spec Task Group, in
https://lists.riscv.org/g/tech-privileged-archive/message/488?p=,,,20,0,0,0::Created,,Proposal,20,2,40,32306071
but never receive any feedback.
[3] https://lists.riscv.org/g/tech-unixplatformspec/message/84
I intended to discuss [2] in the Unixplatform Spec Task Group at the
online meeting, but obviously people were too busy knowing who the new
RISC-V CTO is and what he has done to even follow the agenda.
On Tue, Jun 30, 2020 at 6:19 PM Anup Patel <[email protected]> wrote:
>
> On Tue, Jun 30, 2020 at 1:34 PM Zong Li <[email protected]> wrote:
> >
> > On Tue, Jun 30, 2020 at 3:40 PM Anup Patel <[email protected]> wrote:
> > >
> > > On Tue, Jun 30, 2020 at 12:07 PM Zong Li <[email protected]> wrote:
> > > >
> > > > On Mon, Jun 29, 2020 at 9:23 PM Anup Patel <[email protected]> wrote:
> > > > >
> > > > > On Mon, Jun 29, 2020 at 6:23 PM Zong Li <[email protected]> wrote:
> > > > > >
> > > > > > On Mon, Jun 29, 2020 at 4:28 PM Anup Patel <[email protected]> wrote:
> > > > > > >
> > > > > > > On Mon, Jun 29, 2020 at 11:22 AM Zong Li <[email protected]> wrote:
> > > > > > > >
> > > > > > > > On Mon, Jun 29, 2020 at 12:53 PM Anup Patel <[email protected]> wrote:
> > > > > > > > >
> > > > > > > > > On Mon, Jun 29, 2020 at 8:49 AM Zong Li <[email protected]> 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 ??
Your next version 5) is good to my case, we need a way to set the
mhpmeventN. Thanks.
We don't need to configure enable/disable now, but it would be good if
we can set
mcounteren and mcountinhib through SBI calls at runtime.
>
> Regards,
> Anup
>
> >
> > >
> > > >
> > > > > Maybe you can refine the above ideas and send a proposal to the
> > > > > UnixPlatformSpec mailing list ??
> > > > >
> > > >
> > > > Ok, let us talk about the details in that.
> > >
> > > Regards,
> > > Anup
> > >
> > > >
> > > >
> > > > > Regards,
> > > > > Anup
> > > > >
> > > > > >
> > > > > > > Also, the RISC-V PMU driver can be implemented such that it will
> > > > > > > work for RV32, RV64, NoMMU RV32, and NoMMU RV64.
> > > > > > >
> > > > > > > Regards,
> > > > > > > Anup
> > > > > > >
> > > > > > > > [1] https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/f19TmCNP6yA
> > > > > > > >
> > > > > > > > > Regards,
> > > > > > > > > Anup
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Zong Li (6):
> > > > > > > > > > dt-bindings: riscv: Add YAML documentation for PMU
> > > > > > > > > > riscv: dts: sifive: Add DT support for PMU
> > > > > > > > > > riscv: add definition of hpmcounter CSRs
> > > > > > > > > > riscv: perf: Add raw event support
> > > > > > > > > > riscv: perf: introduce DT mechanism
> > > > > > > > > > riscv: remove PMU menu of Kconfig
> > > > > > > > > >
> > > > > > > > > > .../devicetree/bindings/riscv/pmu.yaml | 59 +++
> > > > > > > > > > arch/riscv/Kconfig | 13 -
> > > > > > > > > > arch/riscv/boot/dts/sifive/fu540-c000.dtsi | 13 +
> > > > > > > > > > arch/riscv/include/asm/csr.h | 58 +++
> > > > > > > > > > arch/riscv/include/asm/perf_event.h | 100 ++--
> > > > > > > > > > arch/riscv/kernel/Makefile | 2 +-
> > > > > > > > > > arch/riscv/kernel/perf_event.c | 471 +++++++++++-------
> > > > > > > > > > 7 files changed, 471 insertions(+), 245 deletions(-)
> > > > > > > > > > create mode 100644 Documentation/devicetree/bindings/riscv/pmu.yaml
> > > > > > > > > >
> > > > > > > > > > --
> > > > > > > > > > 2.27.0
> > > > > > > > > >
On Tue, Jun 30, 2020 at 7:38 PM Anup Patel <[email protected]> wrote:
>
> On Tue, Jun 30, 2020 at 3:48 PM Anup Patel <[email protected]> wrote:
> >
> > On Tue, Jun 30, 2020 at 1:34 PM Zong Li <[email protected]> wrote:
> > >
> > > On Tue, Jun 30, 2020 at 3:40 PM Anup Patel <[email protected]> wrote:
> > > >
> > > > On Tue, Jun 30, 2020 at 12:07 PM Zong Li <[email protected]> wrote:
> > > > >
> > > > > On Mon, Jun 29, 2020 at 9:23 PM Anup Patel <[email protected]> wrote:
> > > > > >
> > > > > > On Mon, Jun 29, 2020 at 6:23 PM Zong Li <[email protected]> wrote:
> > > > > > >
> > > > > > > On Mon, Jun 29, 2020 at 4:28 PM Anup Patel <[email protected]> wrote:
> > > > > > > >
> > > > > > > > On Mon, Jun 29, 2020 at 11:22 AM Zong Li <[email protected]> wrote:
> > > > > > > > >
> > > > > > > > > On Mon, Jun 29, 2020 at 12:53 PM Anup Patel <[email protected]> wrote:
> > > > > > > > > >
> > > > > > > > > > On Mon, Jun 29, 2020 at 8:49 AM Zong Li <[email protected]> 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 CSRs
> 3. counter_idx = 32 or higher are for software counters provided by
> SBI implementation
>
> 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. The availability of SBI PMU extension can be checked
> using sbi_probe_extension() SBI call.
>
It seems to me that we could separate the function id for MHPMCOUNTER
and SBI PMU counters in SBI PMU extension. For example, use MSB of function
id to distinguish them. It would be more clear to propose.
OTOH, do we also need to define a series of event types of SBI PMU counters?
> Regards,
> Anup
On Wed, Jul 1, 2020 at 2:57 AM Atish Patra <[email protected]> wrote:
>
> On Tue, 2020-06-30 at 17:08 +0530, Anup Patel wrote:
> > On Tue, Jun 30, 2020 at 3:48 PM Anup Patel <[email protected]>
> > wrote:
> > > On Tue, Jun 30, 2020 at 1:34 PM Zong Li <[email protected]> wrote:
> > > > On Tue, Jun 30, 2020 at 3:40 PM Anup Patel <[email protected]>
> > > > wrote:
> > > > > On Tue, Jun 30, 2020 at 12:07 PM Zong Li <[email protected]>
> > > > > wrote:
> > > > > > On Mon, Jun 29, 2020 at 9:23 PM Anup Patel <
> > > > > > [email protected]> wrote:
> > > > > > > On Mon, Jun 29, 2020 at 6:23 PM Zong Li <[email protected]
> > > > > > > > wrote:
> > > > > > > > On Mon, Jun 29, 2020 at 4:28 PM Anup Patel <
> > > > > > > > [email protected]> wrote:
> > > > > > > > > On Mon, Jun 29, 2020 at 11:22 AM Zong Li <
> > > > > > > > > [email protected]> wrote:
> > > > > > > > > > On Mon, Jun 29, 2020 at 12:53 PM Anup Patel <
> > > > > > > > > > [email protected]> wrote:
> > > > > > > > > > > On Mon, Jun 29, 2020 at 8:49 AM Zong Li <
> > > > > > > > > > > [email protected]> 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 ?
>
Sounds good to me.
> > 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 ?
Yes, I think it does.
>
>
> > The availability of SBI PMU extension can be checked
> > using sbi_probe_extension() SBI call.
> >
> > Regards,
> > Anup
> >
> > _______________________________________________
> > linux-riscv mailing list
> > [email protected]
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
>
> --
> Regards,
> Atish
Tue, Jun 30, 2020 at 06:02:43PM -0700, Atish Patra wrote:
> On Tue, Jun 30, 2020 at 5:52 PM Alan Kao <[email protected]> wrote:
> >
> > On Mon, Jun 29, 2020 at 11:19:09AM +0800, Zong Li 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.
> > >
> > > Zong Li (6):
> > > dt-bindings: riscv: Add YAML documentation for PMU
> > > riscv: dts: sifive: Add DT support for PMU
> > > riscv: add definition of hpmcounter CSRs
> > > riscv: perf: Add raw event support
> > > riscv: perf: introduce DT mechanism
> > > riscv: remove PMU menu of Kconfig
> > >
> >
> > DT-based PMU registration looks good to me. Together with Anup's feedback,
> > we can anticipate that the following items will be:
> >
> > - rewrite RISC-V PMU to a platform driver
> > - propose SBI PMU extention
> > - fixes: RV32 counter access, namings, etc.
> >
> > Yes, all are good directions towards better counting (`perf stat`) function.
> > But as the original author of RISC-V perf port, please allow me to address
> > the fundamental problems of RISC-V perf, again [0][1][2][3], that the sampling
> > (`perf record`) function never earned enough respect. Counting gives you a
> > shallow view regarding an application, while sampling demystifies one for you.
> >
> > The problems are three-fold
> > (1) Interrupt
> > Sampling in perf requires that a HPM raises an interrupt when it overflows.
> > Making RISC-V perf platform driver or not has nothing to do with this. This
> > requires more discussions in TGs.
> > (2) S-mode access to PMU CSRs
> > This is also addressed in this patch set but to me, it is kind of like a
> > SBI-solves-them-all mindset to me. Perf event is for performance monitoring
> > thus we should eliminate any possible overhead if we can. Setting event masks
> > through SBI calls for counting maybe OK, but if we really take sampling and
> > interrupt handling into consideration, it is questionable if it is still a
> > viable way.
> > (3) Registers, registers, registers
> > There is just no enough CSR/function for perf sampling. The previous proposal
> > explains why [2].
> >
> > Perf sampling is off-topic but somehow related, so I bring it up here just
> > for your information.
> >
> > As this patch set goes v2, the PMU porting guide in [0] should be removed since
> > it contains no useful information anymore.
> >
> > [0] Documentation/riscv/pmu.rst
> > [1] https://www.youtube.com/watch?v=Onvlcl4e2IU
> > [2] https://github.com/riscv/riscv-isa-manual/issues/402
> > This proposal has been posted in Privileged Spec Task Group, in
> > https://lists.riscv.org/g/tech-privileged-archive/message/488?p=,,,20,0,0,0::Created,,Proposal,20,2,40,32306071
> > but never receive any feedback.
> > [3] https://lists.riscv.org/g/tech-unixplatformspec/message/84
> > I intended to discuss [2] in the Unixplatform Spec Task Group at the
> > online meeting, but obviously people were too busy knowing who the new
> > RISC-V CTO is and what he has done to even follow the agenda.
> >
>
> Sorry. The last meeting's agenda was derailed for numerous reasons.
> Are you okay with discussing this during the next meeting ?
> I have not scheduled one yet but will probably schedule it on next
> Wednesday (8th July) if there is no objection.
> I can check with Anup if he can present the SBI PMU extension as well.
Thanks for the oppertunity.
But I don't think that the time is enough for every important topic to be
covered. What I provided in the previous citation [2] is a proposal,
which need expert to judge and critique after thorough reading.
The TG Chair should decide the priority of the items. If there is any chance
for our proposal, I can give brief introductions.
>
> >
> > _______________________________________________
> > linux-riscv mailing list
> > [email protected]
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
>
>
>
> --
> Regards,
> Atish
On Wed, Jul 1, 2020 at 8:52 AM Alan Kao <[email protected]> wrote:
>
> On Mon, Jun 29, 2020 at 11:19:09AM +0800, Zong Li 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.
> >
> > Zong Li (6):
> > dt-bindings: riscv: Add YAML documentation for PMU
> > riscv: dts: sifive: Add DT support for PMU
> > riscv: add definition of hpmcounter CSRs
> > riscv: perf: Add raw event support
> > riscv: perf: introduce DT mechanism
> > riscv: remove PMU menu of Kconfig
> >
>
> DT-based PMU registration looks good to me. Together with Anup's feedback,
> we can anticipate that the following items will be:
>
> - rewrite RISC-V PMU to a platform driver
> - propose SBI PMU extention
> - fixes: RV32 counter access, namings, etc.
>
> Yes, all are good directions towards better counting (`perf stat`) function.
> But as the original author of RISC-V perf port, please allow me to address
> the fundamental problems of RISC-V perf, again [0][1][2][3], that the sampling
> (`perf record`) function never earned enough respect. Counting gives you a
> shallow view regarding an application, while sampling demystifies one for you.
>
> The problems are three-fold
> (1) Interrupt
> Sampling in perf requires that a HPM raises an interrupt when it overflows.
> Making RISC-V perf platform driver or not has nothing to do with this. This
> requires more discussions in TGs.
> (2) S-mode access to PMU CSRs
> This is also addressed in this patch set but to me, it is kind of like a
> SBI-solves-them-all mindset to me. Perf event is for performance monitoring
> thus we should eliminate any possible overhead if we can. Setting event masks
> through SBI calls for counting maybe OK, but if we really take sampling and
> interrupt handling into consideration, it is questionable if it is still a
> viable way.
> (3) Registers, registers, registers
> There is just no enough CSR/function for perf sampling. The previous proposal
> explains why [2].
>
> Perf sampling is off-topic but somehow related, so I bring it up here just
> for your information.
>
Agree, sampling is an important measurement for perf, we should integrate it
to perf as soon as possible after overflow interrupt mechanism is standardized.
> As this patch set goes v2, the PMU porting guide in [0] should be removed since
> it contains no useful information anymore.
>
It seems that the document mentioned some hook functions, it is good for me to
reserve this document, maybe we could try to give some modification. I
would check that. Thanks
> [0] Documentation/riscv/pmu.rst
> [1] https://www.youtube.com/watch?v=Onvlcl4e2IU
> [2] https://github.com/riscv/riscv-isa-manual/issues/402
> This proposal has been posted in Privileged Spec Task Group, in
> https://lists.riscv.org/g/tech-privileged-archive/message/488?p=,,,20,0,0,0::Created,,Proposal,20,2,40,32306071
> but never receive any feedback.
> [3] https://lists.riscv.org/g/tech-unixplatformspec/message/84
> I intended to discuss [2] in the Unixplatform Spec Task Group at the
> online meeting, but obviously people were too busy knowing who the new
> RISC-V CTO is and what he has done to even follow the agenda.
>
On Wed, Jul 1, 2020 at 6:48 AM Alan Kao <[email protected]> wrote:
>
> On Mon, Jun 29, 2020 at 11:19:09AM +0800, Zong Li 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.
> >
> > Zong Li (6):
> > dt-bindings: riscv: Add YAML documentation for PMU
> > riscv: dts: sifive: Add DT support for PMU
> > riscv: add definition of hpmcounter CSRs
> > riscv: perf: Add raw event support
> > riscv: perf: introduce DT mechanism
> > riscv: remove PMU menu of Kconfig
> >
>
> DT-based PMU registration looks good to me. Together with Anup's feedback,
> we can anticipate that the following items will be:
>
> - rewrite RISC-V PMU to a platform driver
> - propose SBI PMU extention
> - fixes: RV32 counter access, namings, etc.
>
> Yes, all are good directions towards better counting (`perf stat`) function.
> But as the original author of RISC-V perf port, please allow me to address
> the fundamental problems of RISC-V perf, again [0][1][2][3], that the sampling
> (`perf record`) function never earned enough respect. Counting gives you a
> shallow view regarding an application, while sampling demystifies one for you.
>
> The problems are three-fold
> (1) Interrupt
> Sampling in perf requires that a HPM raises an interrupt when it overflows.
> Making RISC-V perf platform driver or not has nothing to do with this. This
> requires more discussions in TGs.
> (2) S-mode access to PMU CSRs
> This is also addressed in this patch set but to me, it is kind of like a
> SBI-solves-them-all mindset to me. Perf event is for performance monitoring
> thus we should eliminate any possible overhead if we can. Setting event masks
> through SBI calls for counting maybe OK, but if we really take sampling and
> interrupt handling into consideration, it is questionable if it is still a
> viable way.
Yes, we should certainly not have any SBI call for reading the PMU counter.
The S-mode software should always have direct access to the actual counter
value (i.e. CSR for HW counters and memory location for SBI specific counters).
The SBI calls that we have been discussing here only deal with describing
counters and configuring it.
> (3) Registers, registers, registers
> There is just no enough CSR/function for perf sampling. The previous proposal
> explains why [2].
>
> Perf sampling is off-topic but somehow related, so I bring it up here just
> for your information.
I agree with 1) and 2) limitations mentioned above. We certainly need a
RISC-V PMU extension in RISC-V privilege spec. Maybe you can propose
creating a working-group for this ??
My worry is that defining RISC-V PMU extension will take time and meanwhile
more HW will show-up this year and next year which will have the same set of
basic HPMCOUNTER CSRs. We are trying to brainstorm the best thing we can
do when we have just HPMCOUNTER CSRs accessible to S-mode. The SBI
PMU extension discussed here only tries to complement existing HPMCOUNTER
CSRs so that SOC designers can at least provide implementation specific CSRs
for configuring HW counters. The SBI PMU extension won't be able to solve the
counter overflow detection so we will have to depend on software techniques to
detect overflow.
>
> As this patch set goes v2, the PMU porting guide in [0] should be removed since
> it contains no useful information anymore.
I agree. This guide should be either updated or removed.
>
> [0] Documentation/riscv/pmu.rst
> [1] https://www.youtube.com/watch?v=Onvlcl4e2IU
> [2] https://github.com/riscv/riscv-isa-manual/issues/402
> This proposal has been posted in Privileged Spec Task Group, in
> https://lists.riscv.org/g/tech-privileged-archive/message/488?p=,,,20,0,0,0::Created,,Proposal,20,2,40,32306071
> but never receive any feedback.
> [3] https://lists.riscv.org/g/tech-unixplatformspec/message/84
> I intended to discuss [2] in the Unixplatform Spec Task Group at the
> online meeting, but obviously people were too busy knowing who the new
> RISC-V CTO is and what he has done to even follow the agenda.
>
Regards,
Anup
On Wed, Jul 1, 2020 at 7:44 AM Zong Li <[email protected]> wrote:
>
> On Wed, Jul 1, 2020 at 2:57 AM Atish Patra <[email protected]> wrote:
> >
> > On Tue, 2020-06-30 at 17:08 +0530, Anup Patel wrote:
> > > On Tue, Jun 30, 2020 at 3:48 PM Anup Patel <[email protected]>
> > > wrote:
> > > > On Tue, Jun 30, 2020 at 1:34 PM Zong Li <[email protected]> wrote:
> > > > > On Tue, Jun 30, 2020 at 3:40 PM Anup Patel <[email protected]>
> > > > > wrote:
> > > > > > On Tue, Jun 30, 2020 at 12:07 PM Zong Li <[email protected]>
> > > > > > wrote:
> > > > > > > On Mon, Jun 29, 2020 at 9:23 PM Anup Patel <
> > > > > > > [email protected]> wrote:
> > > > > > > > On Mon, Jun 29, 2020 at 6:23 PM Zong Li <[email protected]
> > > > > > > > > wrote:
> > > > > > > > > On Mon, Jun 29, 2020 at 4:28 PM Anup Patel <
> > > > > > > > > [email protected]> wrote:
> > > > > > > > > > On Mon, Jun 29, 2020 at 11:22 AM Zong Li <
> > > > > > > > > > [email protected]> wrote:
> > > > > > > > > > > On Mon, Jun 29, 2020 at 12:53 PM Anup Patel <
> > > > > > > > > > > [email protected]> wrote:
> > > > > > > > > > > > On Mon, Jun 29, 2020 at 8:49 AM Zong Li <
> > > > > > > > > > > > [email protected]> 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 ?
> >
>
> Sounds good to me.
>
> > > 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 ?
>
> Yes, I think it does.
>
Here's v3 of SBI PMU extension which tries to cover CYCLE CSR, INSTRET CSR,
HPMCOUNTER CSRs and software counters of the SBI implementation.
To define SBI PMU extension, we first define counter_idx which is a unique
number assigned to a counter and event_idx which is an encoded number
representing event to be monitored.
The SBI PMU event_idx is 15bit number encoded as follows:
event_idx[14:12] = type
event_idx[11:0] = code
If event_idx.type == 0 then it is HARDWARE event and event_idx.code can
be one of the following:
enum sbi_pmu_hw_id {
/*
* Common hardware events, generalized by the kernel:
*/
PERF_COUNT_HW_CPU_CYCLES = 0,
PERF_COUNT_HW_INSTRUCTIONS = 1,
PERF_COUNT_HW_CACHE_REFERENCES = 2,
PERF_COUNT_HW_CACHE_MISSES = 3,
PERF_COUNT_HW_BRANCH_INSTRUCTIONS = 4,
PERF_COUNT_HW_BRANCH_MISSES = 5,
PERF_COUNT_HW_BUS_CYCLES = 6,
PERF_COUNT_HW_STALLED_CYCLES_FRONTEND = 7,
PERF_COUNT_HW_STALLED_CYCLES_BACKEND = 8,
PERF_COUNT_HW_REF_CPU_CYCLES = 9,
PERF_COUNT_HW_MAX, /* non-ABI */
};
(NOTE: Same as described in <linux_source>/include/uapi/linux/perf_event.h)
If event_idx.type == 1 then it is CACHE event and event_idx.code is encoded
as follows:
event_idx.code[11:4] = cache_id
event_idx.code[3:1] = op_id
event_idx.code[0:0] = result_id
enum sbi_pmu_hw_cache_id {
PERF_COUNT_HW_CACHE_L1D = 0,
PERF_COUNT_HW_CACHE_L1I = 1,
PERF_COUNT_HW_CACHE_LL = 2,
PERF_COUNT_HW_CACHE_DTLB = 3,
PERF_COUNT_HW_CACHE_ITLB = 4,
PERF_COUNT_HW_CACHE_BPU = 5,
PERF_COUNT_HW_CACHE_NODE = 6,
PERF_COUNT_HW_CACHE_MAX, /* non-ABI */
};
enum sbi_pmu_hw_cache_op_id {
PERF_COUNT_HW_CACHE_OP_READ = 0,
PERF_COUNT_HW_CACHE_OP_WRITE = 1,
PERF_COUNT_HW_CACHE_OP_PREFETCH = 2,
PERF_COUNT_HW_CACHE_OP_MAX, /* non-ABI */
};
enum sbi_pmu_hw_cache_op_result_id {
PERF_COUNT_HW_CACHE_RESULT_ACCESS = 0,
PERF_COUNT_HW_CACHE_RESULT_MISS = 1,
PERF_COUNT_HW_CACHE_RESULT_MAX, /* non-ABI */
};
(NOTE: Same as described in <linux_source>/include/uapi/linux/perf_event.h)
If event_idx.type == 2 then it is RAW event and event_idx.code is just
a RAW event number.
In future, more event_idx can be defined without breaking ABI compatibility
of SBI calls.
Based on above definition of counter_idx definition, we can potentially have
the following SBI calls:
1. SBI_PMU_NUM_COUNTERS
This call will return the number of COUNTERs
2. SBI_PMU_COUNTER_DESCRIBE
This call takes two parameters: 1) counter_idx 2) physical address of 4k page
It will write the description of SBI PMU counter at specified physical
address. The details of the SBI PMU counter written at specified physical
address are as follows:
1. Name (64 bytes)
2. CSR_Offset (4 bytes)
(E.g. CSR_Offset == 0x2 imply CSR 0xC02)
(E.g. CSR_Offset == 0xffffffff means it is SBI implementation counter)
3. CSR_Width (4 bytes)
(Number of CSR bits implemented in HW)
4. Event bitmap (2048 bytes) (i.e. 1-bit for each possible event_idx)
(If bit corresponding to a event_idx is 1 then event_idx is supported
by the counter)
5. Any thing else ??
3. 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 counters not mapped
to any CSR (i.e. only for counters with CSR_Offset == 0xffffffff).
4. SBI_PMU_COUNTER_START
This call takes two parameters: 1) counter_idx 2) event_idx
It will inform SBI implementation to configure and start/enable specified
counter on the calling HART to monitor specified event. This SBI call will
fail for counters which are not present.
5. 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 counters which are not present.
From above, the RISC-V PMU driver will use most of the SBI calls at boot time.
Only SBI_PMU_COUNTER_START to used once before using the counter. The reading
of counter is by reading CSR (for CSR_Offset != 0xffffffff) OR by reading
memory location (for CSR_Offset == 0xffffffff). The counter overflow handling
will have to be done in software by Linux kernel.
The information returned by SBI_PMU_NUM_COUNTERS and SBI_PMU_COUNTER_DESCRIBE
can be passed via DT/ACPI but it will be difficult to maintain because we
have hardware counters and SBI implementation counters both provided by SBI
PMU extension. The SBI implementation counters are specific to underlying
SBI implementation so we will have to keep counters/events described in
DT/ACPI in-sync with underlying SBI implementation.
Regards,
Anup