[PATCH v3 01/15] docs: Add device tree bindings for SBI PMU extension

Fri Jun 25 20:34:20 PDT 2021

On Sat, Jun 26, 2021 at 7:14 AM Jessica Clarke <jrtc27 at jrtc27.com> wrote:
>
> On 26 Jun 2021, at 02:39, Atish Patra <atishp at atishpatra.org> wrote:
> >
> > On Fri, Jun 25, 2021 at 6:01 PM Jessica Clarke <jrtc27 at jrtc27.com> wrote:
> >>
> >> On 26 Jun 2021, at 01:57, Atish Patra <atish.patra at wdc.com> wrote:
> >>>
> >>> SBI PMU extension requires a firmware to be aware of the event to
> >>> counter/mhpmevent mappings supported by the hardware. One approach
> >>> is to encode that information in the device tree.
> >>>
> >>> Define a device tree binding that allows a hardware to describe
> >>> all the PMU mappings required in concise format.
> >>>
> >>> Reviewed-by: Anup Patel <anup.patel at wdc.com>
> >>> Signed-off-by: Atish Patra <atish.patra at wdc.com>
> >>> ---
> >>> docs/pmu_support.md | 83 +++++++++++++++++++++++++++++++++++++++++++++
> >>> 1 file changed, 83 insertions(+)
> >>> create mode 100644 docs/pmu_support.md
> >>>
> >>> diff --git a/docs/pmu_support.md b/docs/pmu_support.md
> >>> new file mode 100644
> >>> index 000000000000..8535a1dccbeb
> >>> --- /dev/null
> >>> +++ b/docs/pmu_support.md
> >>> @@ -0,0 +1,83 @@
> >>> +OpenSBI SBI PMU extension support
> >>> +==================================
> >>> +SBI PMU extension supports allow supervisor software to configure/start/stop
> >>> +any performance counter at anytime. Thus, an user can leverage full
> >>> +capability of performance analysis tools such as perf if SBI PMU extension is
> >>> +enabled. The OpenSBI implementation makes the following assumptions about the
> >>> +hardware platform.
> >>> +
> >>> + * MCOUNTINHIBIT CSR must be implemented in the hardware. Otherwise, SBI PMU
> >>> +extension will not be enabled.
> >>> +
> >>> + * The platform must provide information about PMU event to counter mapping
> >>> +via device tree or platform specific hooks. Otherwise, SBI PMU extension will
> >>> +not be enabled.
> >>> +
> >>> + * The platforms should provide information about the PMU event selector values
> >>> +that should be encoded in the expected value of MHPMEVENTx while configuring
> >>> +MHPMCOUNTERx for that specific event. This can be done via a device tree or
> >>> +platform specific hooks. The exact value to be written to he MHPMEVENTx is
> >>> +completely platform specific. Generic platform writes a default value <xyz> to
> >>> +the MHPMEVENTx CSR where <xyz> is formatted as described below.
> >>> +```
> >>> +     xyz[0:19]    : event_idx
> >>> +        xyz[20:XLEN] : event_data[0:(XLEN-20)]
> >>> +
> >>> +```
> >>> +
> >>> +SBI PMU Device Tree Bindings
> >>> +----------------------------
> >>> +
> >>> +Platforms may choose to describe PMU event selector and event to counter mapping
> >>> +values via device tree. The following sections describes the PMU DT node
> >>> +bindings in details.
> >>> +
> >>> +* **compatible** (Mandatory) - The compatible string of SBI PMU device tree node.
> >>> +This DT property must have the value **riscv,pmu**.
> >>> +
> >>> +* **opensbi,event-to-mhpmevent**(Optional) - It represents an ONE-to-ONE mapping
> >>> +between a PMU event and the event selector value that platform expects to be
> >>> +written to the MHPMEVENTx CSR for that event. The mapping is encoded in a
> >>> +table format where each row represents an event. The first column represent the
> >>> +event idx where the 2nd & 3rd column represent the event selector value that
> >>> +should be encoded in the expected value to be written in MHPMEVENTx.
> >>> +This property shouldn't encode any raw hardware event.
> >>> +
> >>> +* **opensbi,event-to-counters**(Optional) - It represents a MANY-to-MANY
> >>> +mapping between a range of events and all the MHPMCOUNTERx in a bitmap format
> >>> +that can be used to monitor these range of events. The information is encoded in
> >>> +a table format where each row represent a certain range of events and
> >>> +corresponding counters. The first column represents starting of the pmu event id
> >>> +and 2nd column represents the end of the pmu event id. The third column
> >>> +represent a bitmap of all the MHPMCOUNTERx. This property is mandatory if
> >>> +event-to-mhpmevent is present. Otherwise, it can be omitted. This property
> >>> +shouldn't encode any raw event.
> >>> +
> >>> +* **opensbi,raw-event-to-counters**(Optional) - It represents an ONE-to-MANY
> >>> +mapping between a raw event and all the MHPMCOUNTERx in a bitmap format that can
> >>> +be used to monitor that raw event. The information is encoded in a table format
> >>> +where each raw represent a specific raw event. The first column stores the
> >>> +expected event selector value that should be encoded in the expected value to be
> >>> +written in MHPMEVENTx. The second column stores a bitmap of all the MHPMCOUNTERx
> >>> +that can be used for monitoring the specified event.
> >>> +
> >>> +*Note:* A platform may choose to provide the mapping between event & counters
> >>> +via platform hooks rather than the device tree.
> >>> +
> >>> +### Example
> >>> +
> >>> +```
> >>> +pmu {
> >>> +      compatible                     = "riscv,pmu";
> >>> +      interrupts                     = <0x100>;
> >>> +      interrupt-parent                       = <&plic>
> >>> +      opensbi,event-to-mhpmevent     = <0x0000B  0x0000 0x0001>,
> >>> +      opensbi,event-to-counters      = <0x00001 0x00001 0x00000001>,
> >>> +                                               <0x00002 0x00002 0x00000004>,
> >>> +                                               <0x00003 0x0000A 0x00000ff8>,
> >>> +                                               <0x10000 0x10033 0x000ff000>,
> >>> +      opensbi,raw-event-to-counters  = <0x0000 0x0002 0x00000f8>,
> >>> +                                       <0x0000 0x0003 0x00000ff0>,
> >>
> >> If this is a generic specification then the node name should not
> >> include the implementation-specific opensbi string in the name. If this
> >> is not a generic specification then it should not be in riscv-sbi-doc.
> >>
> >
> > The DT binding is OpenSBI specific. The SBI specification doesn't say
> > anything about how SBI implementation/platform
> > wants to encode the counter/event mapping.
>
> Other DT-using implementations will need to specify the same thing.
> Either they’ll be forced to use opensbi strings, which is wrong, or
> they’ll have to invent their own and we have two ways to say the same
> thing. If at least one of Xvisor, KVM, RustSBI, Diosix or another SBI
> implementation is also using an FDT (which seems highly likely since
> ACPI on RISC-V is currently a pie in the sky) and wishing to implement
> the PMU extension then it will have to communicate exactly the same
> information as OpenSBI here. This is not specific to OpenSBI; it should
> be an implementation-agnostic string.

The mhpmeventX, mhpmcounterX, mcountinhibit, and other mHPM CSRs
are only accessible to M-mode so only SBI implementations running in
M-mode can configure these CSRs. This means none of the hypervisors
(Xvisor, KVM, etc) can ever directly configure the mHPM counters. Due to
this we have standard events defined by the SBI PMU spec and all
S-mode SBI PMU implementations (Hypervisors) and SBI PMU users
(Linux, BSD, etc) should only care about the standard events defined by
the SBI PMU spec.

The term "opensbi" in DT property is objectionable then "opensbi,xyz" DT
properties can be renamed to something like:
firmware,event-to-mhpmevent
firmware,event-to-mhpmcounters
firmware,raw-event-to-mhpmcounters

(@Atish are you okay with the above renaming ?)

Other DT properties, namely "compatible", "interrupts", and
"interrupt-parent" are not required by SBI PMU users (Linux, Hypervisors)
running in S-mode because:
1) SBI PMU extension can be discovered using sbi_probe_extension()
2) Scofpmf extension standardizes the per-HART overflow interrupt number

Regards,
Anup