On Fri, 2021-06-04 at 19:05 +0000, Atish Patra wrote:

On Fri, 2021-06-04 at 20:48 +0800, Bin Meng wrote:

Hi Heinrich,

On Fri, Jun 4, 2021 at 8:13 PM Heinrich Schuchardt <   
xypron.glpk@...> wrote:

a
On 6/4/21 11:57 AM, Bin Meng wrote:

Signed-off-by: Bin Meng <bmeng.cn@...>
---

  riscv-sbi.adoc | 6 ++++++
  1 file changed, 6 insertions(+)

diff --git a/riscv-sbi.adoc b/riscv-sbi.adoc
index 11c30c3..8696f97 100644
--- a/riscv-sbi.adoc
+++ b/riscv-sbi.adoc
@@ -34,6 +34,7 @@ https://creativecommons.org/licenses/by/4.0/.
  * Improved SBI introduction secion
  * Improved documentation of SBI hart state managment
extension
  * Added suspend function to SBI hart state managment
extension
+* Clarified that a SBI extension cannot be partially
implemented

  === Version 0.2

@@ -51,6 +52,11 @@ abstraction for platform (or hypervisor)
specific functionality. The design
  of the SBI follows the general RISC-V philosophy of having a
small core along
  with a set of optional modular extensions.

+SBI extensions as whole are optional but if a SBI <abc>
extension compliant

%s/a SBI/an SBI/ (as you will pronounce SBI as as-bee-aye)

Sure. Will send a new patch to fix other places in the same file.

+with SBI v0.X spec is implemented then all functions of SBI
<abc> extension
+as defined in SBI v0.X are assumed to be present. Basically, a
SBI extension

Can we do away with all the placeholders?

How about:

"SBI extensions as whole are optional but they shall not be
partially
implemented: If sbi_probe_extension() signals that an extension
is
available, it must be implemented in total and conform to the SBI
version reported by sbi_get_spec_version()."

This one is more verbose but sounds better to me. May be we should
just
explicitly say that "all functions belonging to that extension must
be
implemented" similar to the below version.

Hi Bin,
Are you planning to send v2 for this patch or I can modify the text and
merge?

How about:

If sbi_probe_extension() signals that an extension is available,
all
functions that conform to the SBI version reported by
sbi_get_spec_version() must be implemented.

Regards,
Bin

--
Regards,
Atish

--
Regards,
Atish

On Mon, 2021-06-07 at 22:56 +0800, Abner Chang wrote:

Atish Patra <Atish.Patra@...> 於 2021年6月5日 週六 上午3:14寫道：

On Sat, 2021-05-29 at 22:42 +0800, Abner Chang wrote:

From: Abner Chang <renba.chang@...>

Initial description of PLIC  CLINT section of Linux-2022
platform.

On v6 commit,
  Remove the changes in Embedded-2022 section.

On v5 commit,
- Remove CLINT from platform spec
- Require ACLINT on Linux2020 platform and have a link to   

https://github.com/riscv/riscv-aclint/blob/master/riscv-aclint.adoc
.

- Remove Machine mode timer from previous patch because that is
in
the scope of ACLINT
- For Embedded-2022 platform, mention Machine mode timer and
refer

to

ACLINT for the definition of registers

On v4 commit,
- PLIC section with [DEPRECATED] in Linux- 2022 chapter
- CLINT section in Linux- 2022 chapter for M-mode timer. We don't
mention
  IPI because AIA already supported it.
- In Embedded-2022 Machine mode timer section, CLINT is not
mandatory.
- Separate section in appendix for the Machine mode timer
registers

On v3 commit,
- Address review comments.

On v2 commit,
- CLINT is not deprecated.

- Add a standalone section for Machine Mode Timer in System
Peripherals.
  Do you think this is a good place for Machine Mode Timer?
  @Mayuresh, please check if you are ok with this change, not
sure

if

this
  overlaps with your text or not (The timer setion). I can remove
this
  if you prefer to put this with your patch.

- In Embedded-2022, refer to Machine Mode Timer in System

Peripherals

  section and CLINT in Linux-2022 Platform.
  @Alistair, is this ok?

On v1 commit,
- Not sure where to put the [DEPRECATED].
- Change the reference of PLIC in section 2.2.2. Interrupt

Controller

to
  1.1.3.2 PLIC + CLINT section.

Signed-off-by: Abner Chang <renba.chang@...>
Cc: Alistair Francis <alistair.francis@...>
Cc: Sunil V L <sunilvl@...>
Cc: Mayuresh Chitale <mchitale@...>
---
 riscv-platform-spec.adoc | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/riscv-platform-spec.adoc b/riscv-platform-spec.adoc
index 160c74a..c0ee75d 100644
--- a/riscv-platform-spec.adoc
+++ b/riscv-platform-spec.adoc
@@ -49,9 +49,24 @@ include::profiles.adoc[]
 * Start Address
 
 ==== Interrupt Controller
-* AIA
-* PLIC + CLINT
-* Interrupt Assignments
+===== AIA[[AIA]]
+===== PLIC[DEPRECATED][[PLIC]]
+The Platform Level Interrupt Controller (PLIC) provides
facilities
to route
+the non-local interrupts to the external interrupt of a hart

context

+with a given privilege mode in a given hart. The number of non-

local

interrupt
+sources supported by PLIC and how does each of them connect to
the
hart
+context is PLIC core implementation-specific. +
+(Refer to   

https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc[RISC-V

 PLIC Specification]

IIRC, PLIC spec was never reviewed widely. As this group is more
active
now, tt would be good to send it as a separate patch so we can do a
detailed review of that as well.

Hi Atish,
Do you mean to send the patch of PLIC spec on 
https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc?

Yes. I think it was not reviewed in the past. At least that's what I
remember. If I am wrong about that, it's fine.

I am just concerned about semantics rather than technical details.

+for the implementation reference of PLIC operation parameters)
+
+===== ACLINT
+Linux-2020 platform 

We have now official names for the PLATFORM spec. We should refer
to
that.

Yes, it's fair.

Abner

requires the Advanced Core Local Interruptor (ACLINT,
+refer to
+ 

https://github.com/riscv/riscv-aclint/blob/master/riscv-aclint.adoc[RISC-V

 ACLINT Specification])
+to provide facilities to route inter-processor interrupt and

Machine

mode timer
+interrupt to each RISC-V processor hart.
+

+===== Interrupt Assignments
 
 ==== System Peripherals
 * UART/Serial Console
@@ -289,8 +304,8 @@ Any RISC-V system that uses at least RV32/64G

can

meet the Embedded-2022
 specification.
 
 ==== Interrupt Controller
-Embedded systems are recommended to use a spec compliant
-https://github.com/riscv/riscv-plic-spec[PLIC], a spec compliant
+Embedded systems are recommended to use a spec compliant
<<PLIC,PLIC>>,
+a spec compliant
 

https://github.com/riscv/riscv-fast-interrupt/blob/master/clic.adoc[CLIC]

 or both a CLIC and and PLIC.
 

--
Regards,
Atish

Στις 2021-06-07 07:03, Anup Patel έγραψε:

Let's have a simple SBI DMA sync extension in SBI v0.4 spec.
The shared code pages between M-mode and S-mode will have it's own
Challenges and we will have to define more stuff in SBI spec to support
this (see above).

Totally agree with you, I just thought it'd be a good opportunity to bring this up so that we can discuss it at some point, let's have something that works and we can optimize it later on.

It seems CMO extension might freeze sooner than we think (others can
comment on this). If CMO extension is frozen by year end then we can
trap-n-emulate CMO instructions instead of SBI DMA sync extension. If
it does not freeze by year end then we will have to go ahead with
SBI DMA sync extension as stop-gap solution.

The CMOs TG has a meeting today, I'll try and join and ask for updates on this.

This patch adds SBI performance monitoring unit (PMU) extension which
allows S-mode (or VS-mode) software to configure hardware (or firmware)
performance counters with help of M-mode (or HS-mode) software.

Signed-off-by: Anup Patel <anup.patel@...>
Signed-off-by: Atish Patra <atish.patra@...>
---
Changes since v7:
- Added distinct NOTE in hardware general events to describe what it
means to halt CPU clock
- Updated hardware RAW event definition to align with Scofpmf extension
- Re-ordered filter bits in config_flags parameter to match Scofpmf extension
Changes since v6:
- Additional NOTEs for SBI_PMU_CFG_FLAG_SKIP_MATCH,
SBI_PMU_CFG_FLAG_AUTO_START, and SBI_PMU_START_SET_INIT_VALUE flags
Changes since v5:
- Improved NOTEs on SBI_PMU_HW_CPU_CYCLES, SBI_PMU_HW_REF_CPU_CYCLES,
and SBI_PMU_HW_BUS_CYCLES events as suggested by Greg Favor
- Re-ordered paramters of sbi_pmu_counter_config_matching() so that
uint64_t parameter is last one
- Added NOTEs for config_flags[3:7] bits which will be used for event
filtering based on privilege mode
- Allow sbi_pmu_counter_start() and sbi_pmu_counter_stop() to work on
a set of counters
- Re-ordered function numbering to make sbi_pmu_counter_fw_read() as
last function
Changes since v4:
- Simplified "NOTE" section for RAW events
- Resized tables
- Added "SBI version" column to function list table
- Added "NOTE" section for SBI_PMU_HW_BUS_CYCLES and SBI_PMU_HW_REF_CPU_CYCLE
- Explicity mention that event_data is 64 bits wide
- Only lower 56 bits of event_data are used for RAW events to align with
upcoming Sscof extension
Changes since v3:
- The new "sscof" extension requires the event info to be 64 bit.
- Improves:
1. the counter start/stop function ids with a appropriate error codes
2. An additional flag parameter to accomodate "sscof extension".
- Renames:
a. software counter to firmware counter to avoid ambiguity with
kernel software events.
b. event_info to event_data to avoid ambiguity with counter info
---
riscv-sbi.adoc | 465 +++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 465 insertions(+)

diff --git a/riscv-sbi.adoc b/riscv-sbi.adoc
index 11c30c3..68e42fe 100644
--- a/riscv-sbi.adoc
+++ b/riscv-sbi.adoc
@@ -34,6 +34,7 @@ https://creativecommons.org/licenses/by/4.0/.
* Improved SBI introduction secion
* Improved documentation of SBI hart state managment extension
* Added suspend function to SBI hart state managment extension
+* Added performance monitoring unit extension

=== Version 0.2

@@ -114,6 +115,8 @@ error codes.
| SBI_ERR_DENIED | -4
| SBI_ERR_INVALID_ADDRESS | -5
| SBI_ERR_ALREADY_AVAILABLE | -6
+| SBI_ERR_ALREADY_STARTED | -7
+| SBI_ERR_ALREADY_STOPPED | -8
|===

An `ECALL` with an unsupported SBI extension ID (*EID*) or an unsupported SBI
@@ -1092,6 +1095,468 @@ The possible error codes returned in `sbiret.error` are shown in the
| sbi_system_reset | 0.3 | 0 | 0x53525354
|===

+== Performance Monitoring Unit Extension (EID #0x504D55 "PMU")
+
+The RISC-V hardware performance counters such as `mcycle`, `minstret`, and
+`mhpmcounterX` CSRs are accessible as read-only from supervisor-mode using
+`cycle`, `instret`, and `hpmcounterX` CSRs. The SBI performance monitoring
+unit (PMU) extension is an interface for supervisor-mode to configure and
+use the RISC-V hardware performance counters with assistance from the
+machine-mode (or hypervisor-mode). These hardware performance counters
+can only be started, stopped, or configured from machine-mode using
+`mcountinhibit` and `mhpmeventX` CSRs. Due to this, a machine-mode SBI
+implementation may choose to disallow SBI PMU extension if `mcountinhibit`
+CSR is not implemented by the RISC-V platform.
+
+A RISC-V platform generally supports monitoring of various hardware events
+using a limited number of hardware performance counters which are up to
+64 bits wide. In addition, a SBI implementation can also provide firmware
+performance counters which can monitor firmware events such as number of
+misaligned load/store instructions, number of RFENCEs, number of IPIs, etc.
+The firmware counters are always 64 bits wide.
+
+The SBI PMU extension provides:
+
+1. An interface for supervisor-mode software to discover and configure
+ per-HART hardware/firmware counters
+2. A typical https://en.wikipedia.org/wiki/Perf_(Linux)[perf] compatible
+ interface for hardware/firmware performance counters and events
+3. Full access to microarchitecture's raw event encodings
+
+To define SBI PMU extension calls, we first define important entities
+`counter_idx`, `event_idx`, and `event_data`. The `counter_idx` is a
+logical number assigned to each hardware/firmware counter. The `event_idx`
+represents a hardware (or firmware) event whereas the `event_data` is
+64 bits wide and represents additional configuration (or parameters) for
+a hardware (or firmware) event.
+
+The event_idx is a 20 bits wide number encoded as follows:
+[source, C]
+----
+ event_idx[19:16] = type
+ event_idx[15:0] = code
+----
+
+=== Event: Hardware general events (Type #0)
+
+The `event_idx.type` (i.e. *event type*) should be `0x0` for all hardware
+general events and each hardware general event is identified by an unique
+`event_idx.code` (i.e. *event code*) described in the
+<<table_pmu_hardware_events>> below.
+
+[#table_pmu_hardware_events]
+.PMU Hardware Events
+[cols="6,1,4", width=95%, align="center", options="header"]
+|===
+| General Event Name | Code | Description
+| SBI_PMU_HW_NO_EVENT | 0 | Unused event because
+ `event_idx` cannot be zero
+| SBI_PMU_HW_CPU_CYCLES | 1 | Event for each CPU cycle
+| SBI_PMU_HW_INSTRUCTIONS | 2 | Event for each completed
+ instruction
+| SBI_PMU_HW_CACHE_REFERENCES | 3 | Event for cache hit
+| SBI_PMU_HW_CACHE_MISSES | 4 | Event for cache miss
+| SBI_PMU_HW_BRANCH_INSTRUCTIONS | 5 | Event for a branch instruction
+| SBI_PMU_HW_BRANCH_MISSES | 6 | Event for a branch misprediction
+| SBI_PMU_HW_BUS_CYCLES | 7 | Event for each BUS cycle
+| SBI_PMU_HW_STALLED_CYCLES_FRONTEND | 8 | Event for a stalled cycle in
+ microarchitecture frontend
+| SBI_PMU_HW_STALLED_CYCLES_BACKEND | 9 | Event for a stalled cycle in
+ microarchitecture backend
+| SBI_PMU_HW_REF_CPU_CYCLES | 10 | Event for each reference
+ CPU cycle
+|===
+
+*NOTE:* The `event_data` (i.e. *event data*) is unused for hardware
+general events and all non-zero values of `event_data` are reserved
+for future use.
+
+*NOTE:* A RISC-V platform might halt the CPU clock when it enters WAIT
+state using the WFI instruction or enters platform specific SUSPEND state
+using the SBI HSM HART suspend call.
+
+*NOTE:* The *SBI_PMU_HW_CPU_CYCLES* event counts CPU clock cycles as
+counted by the `cycle` CSR. These may be variable frequency cycles, and
+are not counted when the CPU clock is halted.
+
+*NOTE:* The *SBI_PMU_HW_REF_CPU_CYCLES* counts fixed-frequency clock
+cycles while the CPU clock is not halted. The fixed-frequency of counting
+might, for example, be the same frequency at which the `mtime` CSR counts.
+
+*NOTE:* The *SBI_PMU_HW_BUS_CYCLES* counts fixed-frequency clock cycles.
+The fixed-frequency of counting might be the same frequency at which the
+`mtime` CSR counts, or may be the frequency of the clock at the boundary
+between the HART (and it's private caches) and the rest of the system.
+
+=== Event: Hardware cache events (Type #1)
+
+The `event_idx.type` (i.e. *event type*) should be `0x1` for all hardware
+cache events and each hardware cache event is identified by an unique
+`event_idx.code` (i.e. *event code*) which is encoded as follows:
+
+[source, C]
+----
+ event_idx.code[15:3] = cache_id
+ event_idx.code[2:1] = op_id
+ event_idx.code[0:0] = result_id
+----
+
+Below tables show possible values of: `event_idx.code.cache_id` (i.e.
+*cache event id*), `event_idx.code.op_id` (i.e. *cache operation id*)
+and `event_idx.code.result_id` (i.e. *cache result id*).
+
+[#table_pmu_cache_event_id]
+.PMU Cache Event ID
+[cols="6,2,4", width=95%, align="center", options="header"]
+|===
+| Cache Event Name | Event ID | Description
+| SBI_PMU_HW_CACHE_L1D | 0 | Level1 data cache event
+| SBI_PMU_HW_CACHE_L1I | 1 | Level1 instruction cache event
+| SBI_PMU_HW_CACHE_LL | 2 | Last level cache event
+| SBI_PMU_HW_CACHE_DTLB | 3 | Data TLB event
+| SBI_PMU_HW_CACHE_ITLB | 4 | Instruction TLB event
+| SBI_PMU_HW_CACHE_BPU | 5 | Branch predictor unit event
+| SBI_PMU_HW_CACHE_NODE | 6 | NUMA node cache event
+|===
+
+[#table_pmu_cache_ops_id]
+.PMU Cache Operation ID
+[cols="6,2,4", width=95%, align="center", options="header"]
+|===
+| Cache Operation Name | Operation ID | Description
+| SBI_PMU_HW_CACHE_OP_READ | 0 | Read cache line
+| SBI_PMU_HW_CACHE_OP_WRITE | 1 | Write cache line
+| SBI_PMU_HW_CACHE_OP_PREFETCH | 2 | Prefetch cache line
+|===
+
+[#table_pmu_cache_result_id]
+.PMU Cache Operation Result ID
+[cols="6,2,4", width=95%, align="center", options="header"]
+|===
+| Cache Result Name | Result ID | Description
+| SBI_PMU_HW_CACHE_RESULT_ACCESS | 0 | Cache access
+| SBI_PMU_HW_CACHE_RESULT_MISS | 1 | Cache miss
+|===
+
+*NOTE:* The `event_data` (i.e. *event data*) is unused for hardware cache
+events and all non-zero values of `event_data` are reserved for future use.
+
+=== Event: Hardware raw events (Type #2)
+
+The `event_idx.type` (i.e. *event type*) should be `0x2` for all hardware
+raw events and `event_idx.code` (i.e. *event code*) should be zero.
+
+On RISC-V platform with 32 bits wide `mhpmeventX` CSRs, the `event_data`
+configuration (or parameter) should have the 32-bit value to to be programmed
+in the `mhpmeventX` CSR.
+
+On RISC-V platform with 64 bits wide `mhpmeventX` CSRs, the `event_data`
+configuration (or parameter) should have the 48-bit value to to be programmed
+in the lower 48-bits of `mhpmeventX` CSR and the SBI implementation shall
+determine the value to be progammed in the upper 16 bits of `mhpmeventX`
+CSR.
+
+*Note:* The RISC-V platform hardware implementation may choose to define
+the expected value to be written to `mhpmeventX` CSR for a hardware event.
+In case of hardware general/cache events, the RISC-V platform hardware
+implementation may use the zero-extended `event_idx` as the expected
+value for simplicity.
+
+=== Event: Firmware events (Type #15)
+
+The `event_idx.type` (i.e. *event type*) should be `0xf` for all firmware
+events and each firmware event is identified by an unqiue `event_idx.code`
+(i.e. *event code*) described in the <<table_pmu_firmware_events>> below.
+
+[#table_pmu_firmware_events]
+.PMU Firmware Events
+[cols="6,1,4", width=95%, align="center", options="header"]
+|===
+| Firmware Event Name | Code | Description
+| SBI_PMU_FW_MISALIGNED_LOAD | 0 | Misaligned load trap event
+| SBI_PMU_FW_MISALIGNED_STORE | 1 | Misaligned store trap event
+| SBI_PMU_FW_ACCESS_LOAD | 2 | Load access trap event
+| SBI_PMU_FW_ACCESS_STORE | 3 | Store access trap event
+| SBI_PMU_FW_ILLEGAL_INSN | 4 | Illegal instruction trap event
+| SBI_PMU_FW_SET_TIMER | 5 | Set timer event
+| SBI_PMU_FW_IPI_SENT | 6 | Sent IPI to other HART event
+| SBI_PMU_FW_IPI_RECEIVED | 7 | Received IPI from other
+ HART event
+| SBI_PMU_FW_FENCE_I_SENT | 8 | Sent FENCE.I request to
+ other HART event
+| SBI_PMU_FW_FENCE_I_RECEIVED | 9 | Received FENCE.I request
+ from other HART event
+| SBI_PMU_FW_SFENCE_VMA_SENT | 10 | Sent SFENCE.VMA request
+ to other HART event
+| SBI_PMU_FW_SFENCE_VMA_RECEIVED | 11 | Received SFENCE.VMA request
+ from other HART event
+| SBI_PMU_FW_SFENCE_VMA_ASID_SENT | 12 | Sent SFENCE.VMA with ASID
+ request to other HART event
+| SBI_PMU_FW_SFENCE_VMA_ASID_RECEIVED | 13 | Received SFENCE.VMA with ASID
+ request from other HART event
+| SBI_PMU_FW_HFENCE_GVMA_SENT | 14 | Sent HFENCE.GVMA request to
+ other HART event
+| SBI_PMU_FW_HFENCE_GVMA_RECEIVED | 15 | Received HFENCE.GVMA request
+ from other HART event
+| SBI_PMU_FW_HFENCE_GVMA_VMID_SENT | 16 | Sent HFENCE.GVMA with VMID
+ request to other HART event
+| SBI_PMU_FW_HFENCE_GVMA_VMID_RECEIVED | 17 | Received HFENCE.GVMA with VMID
+ request from other HART event
+| SBI_PMU_FW_HFENCE_VVMA_SENT | 18 | Sent HFENCE.VVMA request to
+ other HART event
+| SBI_PMU_FW_HFENCE_VVMA_RECEIVED | 19 | Received HFENCE.VVMA request
+ from other HART event
+| SBI_PMU_FW_HFENCE_VVMA_ASID_SENT | 20 | Sent HFENCE.VVMA with ASID
+ request to other HART event
+| SBI_PMU_FW_HFENCE_VVMA_ASID_RECEIVED | 21 | Received HFENCE.VVMA with ASID
+ request from other HART event
+|===
+
+*NOTE:* the `event_data` (i.e. *event data*) is unused for firmware events
+and all non-zero values of `event_data` are reserved for future use.
+
+=== Function: Get number of counters (FID #0)
+
+[source, C]
+----
+struct sbiret sbi_pmu_num_counters()
+----
+
+*Returns* the number of counters (both hardware and firmware) in
+`sbiret.value` and always returns `SBI_SUCCESS` in sbiret.error.
+
+=== Function: Get details of a counter (FID #1)
+
+[source, C]
+----
+struct sbiret sbi_pmu_counter_get_info(unsigned long counter_idx)
+----
+
+Get details about the specified counter such as underlying CSR number,
+width of the counter, type of counter hardware/firmware, etc.
+
+The `counter_info` returned by this SBI call is encoded as follows:
+[source, C]
+----
+ counter_info[11:0] = CSR (12bit CSR number)
+ counter_info[17:12] = Width (One less than number of bits in CSR)
+ counter_info[XLEN-2:18] = Reserved for future use
+ counter_info[XLEN-1] = Type (0 = hardware and 1 = firmware)
+----
+
+If `counter_info.type == 1` then `counter_info.csr` and `counter_info.width`
+should be ignored.
+
+*Returns* the `counter_info` described above in `sbiret.value`.
+
+The possible error codes returned in `sbiret.error` are shown in the
+<<table_pmu_counter_get_info_errors>> below.
+
+[#table_pmu_counter_get_info_errors]
+.PMU Counter Get Info Errors
+[cols="2,3", width=90%, align="center", options="header"]
+|===
+| Error code | Description
+| SBI_SUCCESS | `counter_info` read successfully.
+| SBI_ERR_INVALID_PARAM | `counter_idx` points to an invalid counter.
+|===
+
+=== Function: Find and configure a matching counter (FID #2)
+
+[source, C]
+----
+struct sbiret sbi_pmu_counter_config_matching(unsigned long counter_idx_base,
+ unsigned long counter_idx_mask,
+ unsigned long config_flags,
+ unsigned long event_idx,
+ uint64_t event_data)
+----
+
+Find and configure a counter from a set of counters which is not started
+(or enabled) and can monitor the specified event. The `counter_idx_base`
+and `counter_idx_mask` parameters represent the set of counters whereas
+the `event_idx` represent the event to be monitored and `event_data`
+represents any additional event configuration.
+
+The `config_flags` parameter represent additional counter configuration
+and filter flags. The bit defintions of the `config_flags` parameter are
+shown in the <<table_pmu_counter_cfg_match_flags>> below.
+
+[#table_pmu_counter_cfg_match_flags]
+.PMU Counter Config Match Flags
+[cols="3,1,2", width=90%, align="center", options="header"]
+|===
+| Flag Name | Bits | Description
+| SBI_PMU_CFG_FLAG_SKIP_MATCH | 0:0 | Skip the counter matching
+| SBI_PMU_CFG_FLAG_CLEAR_VALUE| 1:1 | Clear (or zero) the counter
+ value in counter configuration
+| SBI_PMU_CFG_FLAG_AUTO_START | 2:2 | Start the counter after
+ configuring a matching counter
+| SBI_PMU_CFG_FLAG_SET_VUINH | 3:3 | Event counting inhibited +
+ in VU-mode
+| SBI_PMU_CFG_FLAG_SET_VSINH | 4:4 | Event counting inhibited +
+ in VS-mode
+| SBI_PMU_CFG_FLAG_SET_UINH | 5:5 | Event counting inhibited +
+ in U-mode
+| SBI_PMU_CFG_FLAG_SET_SINH | 6:6 | Event counting inhibited +
+ in S-mode
+| SBI_PMU_CFG_FLAG_SET_MINH | 7:7 | Event counting inhibited +
+ in M-mode
+| *RESERVED* | 8:(XLEN-1) | All non-zero values are
+ reserved for future use
+|===
+
+*NOTE:* When *SBI_PMU_CFG_FLAG_SKIP_MATCH* is set in `config_flags`, the
+SBI implementation will unconditionaly select the first counter from the
+set of counters specified by the `counter_idx_base` and `counter_idx_mask`.
+
+*NOTE:* The *SBI_PMU_CFG_FLAG_AUTO_START* flag in `config_flags` has no
+impact on the counter value.
+
+*NOTE:* The `config_flags[3:7]` bits are event filtering hints so these
+can be ignored or overriden by the SBI implemenation for security concerns
+or due to lack of event filtering support in the underlying RISC-V platform.
+
+*Returns* the `counter_idx` in `sbiret.value` upon success.
+
+In case of failure, the possible error codes returned in `sbiret.error` are
+shown in the <<table_pmu_counter_cfg_match_errors>> below.
+
+[#table_pmu_counter_cfg_match_errors]
+.PMU Counter Config Match Errors
+[cols="2,3", width=90%, align="center", options="header"]
+|===
+| Error code | Description
+| SBI_SUCCESS | counter found and configured successfully.
+| SBI_ERR_INVALID_PARAM | set of counters has an invalid counter.
+| SBI_ERR_NOT_SUPPORTED | none of the counters can monitor specified event.
+|===
+
+=== Function: Start a set of counters (FID #4)
+
+[source, C]
+----
+struct sbiret sbi_pmu_counter_start(unsigned long counter_idx_base,
+ unsigned long counter_idx_mask,
+ unsigned long start_flags,
+ uint64_t initial_value)
+----
+
+Start or enable a sef of counters on the calling HART with the specified
+initial value. The `counter_idx_base` and `counter_idx_mask` parameters
+represent the set of counters whereas the `initial_value` parameter
+specifies the initial value of the counter.
+
+The bit defintions of the `start_flags` parameter are shown in the
+<<table_pmu_counter_start_flags>> below.
+
+[#table_pmu_counter_start_flags]
+.PMU Counter Start Flags
+[cols="3,1,2", width=90%, align="center", options="header"]
+|===
+| Flag Name | Bits | Description
+| SBI_PMU_START_SET_INIT_VALUE | 0:0 | Set the value of counters
+ based on the `initial_value`
+ parameter
+| *RESERVED* | 1:(XLEN-1) | All non-zero values are
+ reserved for future use
+|===
+
+*NOTE:* When SBI_PMU_START_SET_INIT_VALUE is not set in `start_flags`,
+the counter value will not be modified and event counting will start
+from current counter value.
+
+The possible error codes returned in `sbiret.error` are shown in the
+<<table_pmu_counter_start_errors>> below.
+
+[#table_pmu_counter_start_errors]
+.PMU Counter Start Errors
+[cols="2,3", width=90%, align="center", options="header"]
+|===
+| Error code | Description
+| SBI_SUCCESS | counter started successfully.
+| SBI_ERR_INVALID_PARAM | some of the counters specified in parameters
+ are invalid.
+| SBI_ERR_ALREADY_STARTED | some of the counters specified in parameters
+ are already started.
+|===
+
+=== Function: Stop a set of counters (FID #4)
+
+[source, C]
+----
+struct sbiret sbi_pmu_counter_stop(unsigned long counter_idx_base,
+ unsigned long counter_idx_mask,
+ unsigned long stop_flags)
+----
+
+Stop or disable a set of counters on the calling HART. The `counter_idx_base`
+and `counter_idx_mask` parameters represent the set of counters. The bit
+defintions of the `stop_flags` parameter are shown in the
+<<table_pmu_counter_stop_flags>> below.
+
+[#table_pmu_counter_stop_flags]
+.PMU Counter Stop Flags
+[cols="3,1,2", width=90%, align="center", options="header"]
+|===
+| Flag Name | Bits | Description
+| SBI_PMU_STOP_FLAG_RESET | 0:0 | Reset the counter to event mapping.
+| *RESERVED* | 1:(XLEN-1) | All non-zero values are reserved
+ for future use
+|===
+
+The possible error codes returned in `sbiret.error` are shown in the
+<<table_pmu_counter_stop_errors>> below.
+
+[#table_pmu_counter_stop_errors]
+.PMU Counter Stop Errors
+[cols="2,3", width=90%, align="center", options="header"]
+|===
+| Error code | Description
+| SBI_SUCCESS | counter stopped successfully.
+| SBI_ERR_INVALID_PARAM | some of the counters specified in parameters
+ are invalid.
+| SBI_ERR_ALREADY_STOPPED | some of the counters specified in parameters
+ are already stopped.
+|===
+
+=== Function: Read a firmware counter (FID #5)
+
+[source, C]
+----
+struct sbiret sbi_pmu_counter_fw_read(unsigned long counter_idx)
+----
+
+Provide the current value of a firmware counter in `sbiret.value`.
+
+The possible error codes returned in `sbiret.error` are shown in the
+<<table_pmu_counter_fw_read_errors>> below.
+
+[#table_pmu_counter_fw_read_errors]
+.PMU Counter Firmware Read Errors
+[cols="2,3", width=90%, align="center", options="header"]
+|===
+| Error code | Description
+| SBI_SUCCESS | firmware counter read successfully.
+| SBI_ERR_INVALID_PARAM | `counter_idx` points to a hardware counter
+ or an invalid counter.
+|===
+
+=== Function Listing
+
+[#table_pmu_function_list]
+.PMU Function List
+[cols="5,2,1,2", width=80%, align="center", options="header"]
+|===
+| Function Name | SBI Version | FID | EID
+| sbi_pmu_num_counters | 0.3 | 0 | 0x504D55
+| sbi_pmu_counter_get_info | 0.3 | 1 | 0x504D55
+| sbi_pmu_counter_config_matching | 0.3 | 2 | 0x504D55
+| sbi_pmu_counter_start | 0.3 | 3 | 0x504D55
+| sbi_pmu_counter_stop | 0.3 | 4 | 0x504D55
+| sbi_pmu_counter_fw_read | 0.3 | 5 | 0x504D55
+|===
+
== Experimental SBI Extension Space (EIDs #0x08000000 - #0x08FFFFFF)

No management.
--
2.25.1

Thx Anup,

On Thu, Jun 3, 2021 at 11:13 PM Anup Patel <anup.patel@...> wrote:

This patch adds SBI direct memory access synchronize (DSYN)) extension
which allows S-mode (or VS-mode) software to explicitly synchronize
memory with assistance from the M-mode (or HS-mode).

Signed-off-by: Anup Patel <anup.patel@...>
---
riscv-sbi.adoc | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 95 insertions(+)

diff --git a/riscv-sbi.adoc b/riscv-sbi.adoc
index 79d98a6..0eb2898 100644
--- a/riscv-sbi.adoc
+++ b/riscv-sbi.adoc
@@ -27,6 +27,10 @@ https://creativecommons.org/licenses/by/4.0/.
[preface]
== Change Log

+=== Version 0.4-rc0
+
+* Added direct memory access synchronize extension
+
=== Version 0.3-rc0

* Improved document styling and naming conventions
@@ -1550,6 +1554,97 @@ The possible error codes returned in `sbiret.error` are shown in the
| sbi_pmu_counter_fw_read | 0.3 | 5 | 0x504D55
|===

+== Direct Memory Access Synchronize Extension (EID #0x4453594e "DSYN")
+
+A RISC-V platform will generally have direct memory access
+(https://en.wikipedia.org/wiki/Direct_memory_access[DMA]) capable devices.
+These DMA capable devices can sometimes be non-coherent with HART caches (i.e.
+I/O non-coherent) hence requiring explicit cache flush and/or invalidate from
+HART to synchronize memory with the DMA capable device. The SBI direct memory
+access synchronize (DSYN) extension is an interface for supervisor-mode to
+explicitly synchronize memory region with assistance from the machine-mode
+(or hypervisor-mode).
+
+=== Function: DMA Synchronize (FID #0)
+
+[source, C]
+----
+struct sbiret sbi_dma_synchronize(unsigned long addr, unsigned long size,
+ unsigned long direction)
+----
+
+Synchronize a memory region for non-coherent DMA capable devices based on
+`addr`, `size` and `direction` paramenters. The `addr` and `size` parameter
+represent the physical address and size of memory region whereas `direction`
+parameter represents the direction of synchronization with possible values
+shown in <<table_dma_sync_direction_list>> below.
+
+[#table_dma_sync_direction_list]
+.DMA Synchronize Directions
+[cols="4,1,5", width=95%, align="center", options="header"]
+|===
+| Direction Name | Value | Description
+| SBI_DMA_SYNC_BIDIRECTIONAL | 0 | Data direction isn't known. +
+ +
+ The DMA synchronization in this
+ direction must be done: +
+ * once before the memory region is
+ handed off to the device. +
+ * once before the memory region is
+ accessed after being used by the
+ device.
+| SBI_DMA_SYNC_TO_DEVICE | 1 | Data is going from the memory region
+ to the device. +
+ +
+ The DMA synchronization in this
+ direction must be done after the last
+ modification of the memory region by
+ the supervisor-mode and before region
+ is handed off to the device.
+| SBI_DMA_SYNC_FROM_DEVICE | 2 | Data is coming from the device to
+ the memory region. +
+ +
+ The DMA synchronization in this
+ direction must be before the
+ supervisor-mode accesses memory region
+ that may have been updated by the
+ device.
+| SBI_DMA_SYNC_NONE | 3 | No data direction. +
+ +
+ This is only for debugging and does
+ not do any DMA synchronization.
+| *RESERVED* | > 3 | Reserved for future use
+|===
+
+The possible error codes returned in `sbiret.error` are shown in the
+<<table_dma_sync_errors>> below.
+
+[#table_dma_sync_errors]
+.DMA Synchronize Errors
+[cols="1,2", width=100%, align="center", options="header"]
+|===
+| Error code | Description
+| SBI_SUCCESS | memory synchronized successfully.
+| SBI_ERR_INVALID_PARAM | `direction` is not valid.
+| SBI_ERR_INVALID_ADDRESS | memory region pointed by `addr` and `size`
+ parameter is not valid possibly due to
+ following reasons: +
+ * It is not a valid physical address range. +
+ * The memory address range is prohibited by
+ PMP to access in supervisor-mode.
+| SBI_ERR_FAILED | memory synchroinzation failed for unknown reasons.
+|===
+
+=== Function Listing
+
+[#table_dsyn_function_list]
+.DSYN Function List
+[cols="5,2,1,2", width=80%, align="center", options="header"]
+|===
+| Function Name | SBI Version | FID | EID
+| sbi_dma_synchronize | 0.4 | 0 | 0x4453594e
+|===
+
== Experimental SBI Extension Space (EIDs #0x08000000 - #0x08FFFFFF)

No management.
--
2.25.1

Reviewed-by: Guo Ren <guoren@...>

It's Okay for us, I'll follow that. May I write a new version of Linux
implementation with the framework, or you've begun preparing that
Linux patches?

--
Best Regards
Guo Ren

ML: https://lore.kernel.org/linux-csky/

Στις 2021-06-04 23:01, Atish Patra έγραψε:

The firmware code will still be executed while the priv mode is S-mode
right ?
Wouldn't that violate the priv spec ?

M-mode can share a code region with S-mode using PMP/ePMP and let S-mode map that region as executable on its address space. With the current PMP M-mode can define a region as RX for example and both M-mode and S/U-mode will have RX permissions there, with ePMP M-mode can share a code region with S/U-mode that can be RX for M-mode and just X for S/U-mode. I'm obviously talking about small code snippets without any dependencies and references to external symbols etc. A function that flushes the cache for example can be written in such a way.

I'm not very
passionate about this, after all an ecall isn't that expensive and a
DMA
sync is not an operation that happens very frequently, but maybe it's a
good opportunity to talk about this approach.

That's what I am thinking. The only additional cost is just a "ecall
and mret".
IMO, there will be noticeable difference in performance in vDSO-like
interface where S-mode is trying to read something that M-mode
provides. Thus, the base function list are likely candidates [1].
However, the OS makes those SBI calls once during the boot. Thus, it
wouldn't benefit that much.

I was thinking that as part of the extension, we can have an SBI call that would return the address/length of the shared code region (in physical memory) and offsets for each function within that region. The OS will do the SBI call upon registering that SBI extension and will just use the provided function pointers to directly execute code from the shared region. If we are looking for a scenario with a high rate of syncs (lots of packets per second) there will be a noticeable performance difference between a function call and an SBI call, on the other had on such scenarios I'd expect to use the coherent API instead of the non-coherent one.

On Fri, 2021-06-04 at 12:31 +0300, Nick Kossifidis wrote:

Στις 2021-06-03 18:13, Anup Patel έγραψε:

This patch adds SBI direct memory access synchronize (DSYN))
extension
which allows S-mode (or VS-mode) software to explicitly synchronize
memory with assistance from the M-mode (or HS-mode).

Signed-off-by: Anup Patel <anup.patel@...>

Thanks for working on this, it seems simple and clean, some thoughts:

a) I also prefer DMAS or something with DMA in the name, and fixed-
sized
arguments.

Agreed.

b) Device and CPU don't necessarily have the same view of the memory,
we
need to define that physical address is the address the CPU sees.

c) Custom CMOs may only accept virtual addresses instead of physical,
in
which case we'll need to switch them back to virtual in the firmware.
Upon registration SBI may tell the OS if it accepts physical or virtual
addresses and the OS can act accordingly (switch cpu_addr to physical
or
not).

d) Since these operations may also be implemented with custom
instructions (instead of e.g. a register write somewhere) I agree that
keeping the code in the firmware makes more sense than allowing custom
instructions in the kernel, on the other hand these operations are
supposed to be performed on S-mode and doing an ecall for them adds a
bit of an overhead. This extension would be a good candidate for using
the vDSO-like interface we discussed at some point. M-mode could share
a
code region with S-mode (both PMP and ePMP allow this scenario) and
during registration of the extension, SBI will return the physical
address of the region, its size and a set of offsets for the different
functions in there (in this case only one function). 

The firmware code will still be executed while the priv mode is S-mode
right ?

Wouldn't that violate the priv spec ?

I'm not very
passionate about this, after all an ecall isn't that expensive and a
DMA
sync is not an operation that happens very frequently, but maybe it's a
good opportunity to talk about this approach.

That's what I am thinking. The only additional cost is just a "ecall
and mret".

IMO, there will be noticeable difference in performance in vDSO-like
interface where S-mode is trying to read something that M-mode
provides. Thus, the base function list are likely candidates [1].
However, the OS makes those SBI calls once during the boot. Thus, it
wouldn't benefit that much.


[1] https://github.com/riscv/riscv-sbi-doc/blob/master/riscv-sbi.adoc#function-listing

Regards,
Nick

--
Regards,
Atish

On Thu, 2021-06-03 at 23:16 +0200, Heinrich Schuchardt wrote:

%s/secion/section/
%s/managment/management/
%s/implemenation/implementation/
%s/requestd/requested/
%s/hierarchial/hierarchical/
%s/inititated/initiated/
%s/recieves/receives/
%s/rententive/retentive/

Signed-off-by: Heinrich Schuchardt <xypron.glpk@...>
---
 riscv-sbi.adoc | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/riscv-sbi.adoc b/riscv-sbi.adoc
index 11c30c3..4b6f2c3 100644
--- a/riscv-sbi.adoc
+++ b/riscv-sbi.adoc
@@ -31,9 +31,9 @@ https://creativecommons.org/licenses/by/4.0/.

 * Improved document styling and naming conventions
 * Added SBI system reset extension
-* Improved SBI introduction secion
-* Improved documentation of SBI hart state managment extension
-* Added suspend function to SBI hart state managment extension
+* Improved SBI introduction section
+* Improved documentation of SBI hart state management extension
+* Added suspend function to SBI hart state management extension

 === Version 0.2

@@ -52,7 +52,7 @@ of the SBI follows the general RISC-V philosophy of
having a small core along
 with a set of optional modular extensions.

 The higher privilege software providing SBI interface to the
supervisor-mode
-software is referred to as a SBI implemenation or Supervisor Execution
+software is referred to as a SBI implementation or Supervisor
Execution
 Environment (SEE). A SBI implementation (or SEE) can be platform
runtime
 firmware executing in machine-mode (M-mode) (see below <<fig_intro1>>)
or
 it can be some hypervisor executing in hypervisor-mode (HS-mode) (see
below
@@ -741,7 +741,7 @@ along with a unique **HSM state id** for each
state:
                                in the **STOPPED** state.
 | 4        | SUSPENDED       | This hart is in a platform specific
suspend
                                (or low power) state.
-| 5        | SUSPEND_PENDING | The hart has requestd to put itself in
a
+| 5        | SUSPEND_PENDING | The hart has requested to put itself in
a
                                platform specific low power state from
the
                                **STARTED** state and the SBI
implementation
                                is still working to get the hart in the
@@ -764,22 +764,22 @@ image::riscv-sbi-hsm.png[]
 A platform can have multiple harts grouped into a hierarchical
topology
 groups (namely cores, clusters, nodes, etc) with separate platform
specific
 low-power states for each hierarchical group. These platform specific
-low-power states of hierarchial topology groups can be represented as
+low-power states of hierarchical topology groups can be represented as
 platform specific suspend states of a hart. A SBI implementation can
 utilize the suspend states of higher topology groups using one of the
 following approaches:

 . *Platform-coordinated:* In this approach, when a hart becomes idle
the
-  supervisor-mode power-managment software will request deepest
suspend
+  supervisor-mode power-management software will request deepest
suspend
   state for the hart and higher topology groups. A SBI implementation
   should choose a suspend state at higher topology group which is:
 .. Not deeper than the specified suspend state
 .. Wake-up latency is not higher than the wake-up latency of the
    specified suspend state
-. *OS-inititated:* In this approach, the supervisor-mode power-
managment
+. *OS-initiated:* In this approach, the supervisor-mode power-
management
   software will directly request a suspend state for higher topology
group
   after the last hart in that group becomes idle. When a hart becomes
idle,
-  the supervisor-mode power-managment software will always select
suspend
+  the supervisor-mode power-management software will always select
suspend
   state for the hart itself but it will select a suspend state for a
higher
   topology group only if the hart is the last running hart in the
group.
   A SBI implementation should:
@@ -815,7 +815,7 @@ below.
 |===

 This call is asynchronous -- more specifically, the `sbi_hart_start()`
may
-return before target hart starts executing as long as the SBI
implemenation
+return before target hart starts executing as long as the SBI
implementation
 is capable of ensuring the return code is accurate. It is recommended
that
 if the SBI implementation is a platform runtime firmware executing in
 machine-mode (M-mode) then it MUST configure PMP and other M-mode
state
@@ -915,13 +915,13 @@ struct sbiret sbi_hart_suspend(uint32_t
suspend_type,
                                unsigned long opaque)
 ----

-Request the SBI implementation to put the calling hart in a platform
specfic
+Request the SBI implementation to put the calling hart in a platform
specific
 suspend (or low power) state specified by the `suspend_type`
parameter. The
 hart will automatically come out of suspended state and resume normal
-execution when it recieves an interrupt or platform specific hardware
event.
+execution when it receives an interrupt or platform specific hardware
event.

 The platform specific suspend states for a hart can be either
retentive
-or non-rententive in nature. A retentive suspend state will preserve
hart
+or non-retentive in nature. A retentive suspend state will preserve
hart
 register and CSR values for all privilege modes whereas a non-
retentive
 suspend state will not preserve hart register and CSR values.

--
2.30.2

Reviewed-by: Atish Patra <atish.patra@...>
--
Regards,
Atish

On Fri, 2021-06-04 at 21:00 +0800, Bin Meng wrote:

%s/a SBI/an SBI/
%s/A SBI/An SBI/

Signed-off-by: Bin Meng <bmeng.cn@...>
---

 riscv-sbi.adoc | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/riscv-sbi.adoc b/riscv-sbi.adoc
index 11c30c3..b02332e 100644
--- a/riscv-sbi.adoc
+++ b/riscv-sbi.adoc
@@ -52,8 +52,8 @@ of the SBI follows the general RISC-V philosophy of
having a small core along
 with a set of optional modular extensions.
 
 The higher privilege software providing SBI interface to the
supervisor-mode
-software is referred to as a SBI implemenation or Supervisor Execution
-Environment (SEE). A SBI implementation (or SEE) can be platform
runtime
+software is referred to as an SBI implemenation or Supervisor
Execution
+Environment (SEE). An SBI implementation (or SEE) can be platform
runtime
 firmware executing in machine-mode (M-mode) (see below <<fig_intro1>>)
or
 it can be some hypervisor executing in hypervisor-mode (HS-mode) (see
below
 <<fig_intro2>>).
@@ -124,7 +124,7 @@ the specification simple and easily adaptable for
all RISC-V ISA types (i.e.
 RV32, RV64 and RV128). In case the data is defined as 32bit wide,
higher
 privilege software must ensure that it only uses 32 bit data only.
 
-If a SBI function needs to pass a list of harts to the higher
privilege mode,
+If an SBI function needs to pass a list of harts to the higher
privilege mode,
 it must use a hart mask as defined below. This is applicable to any
extensions
 defined in or after v0.2.
 
@@ -765,13 +765,13 @@ A platform can have multiple harts grouped into a
hierarchical topology
 groups (namely cores, clusters, nodes, etc) with separate platform
specific
 low-power states for each hierarchical group. These platform specific
 low-power states of hierarchial topology groups can be represented as
-platform specific suspend states of a hart. A SBI implementation can
+platform specific suspend states of a hart. An SBI implementation can
 utilize the suspend states of higher topology groups using one of the
 following approaches:
 
 . *Platform-coordinated:* In this approach, when a hart becomes idle
the
   supervisor-mode power-managment software will request deepest
suspend
-  state for the hart and higher topology groups. A SBI implementation
+  state for the hart and higher topology groups. An SBI implementation
   should choose a suspend state at higher topology group which is:
 .. Not deeper than the specified suspend state
 .. Wake-up latency is not higher than the wake-up latency of the
@@ -782,7 +782,7 @@ following approaches:
   the supervisor-mode power-managment software will always select
suspend
   state for the hart itself but it will select a suspend state for a
higher
   topology group only if the hart is the last running hart in the
group.
-  A SBI implementation should:
+  An SBI implementation should:
 .. Never choose a suspend state for higher topology group different
from
    the specified suspend state
 .. Always prefer most recent suspend state requested for higher
topology

Reviewed-by: Atish Patra <atish.patra@...>

--
Regards,
Atish

On Sat, 2021-05-29 at 22:42 +0800, Abner Chang wrote:

From: Abner Chang <renba.chang@...>

Initial description of PLIC  CLINT section of Linux-2022 platform.

On v6 commit,
  Remove the changes in Embedded-2022 section.

On v5 commit,
- Remove CLINT from platform spec
- Require ACLINT on Linux2020 platform and have a link to
https://github.com/riscv/riscv-aclint/blob/master/riscv-aclint.adoc.
- Remove Machine mode timer from previous patch because that is in
the scope of ACLINT
- For Embedded-2022 platform, mention Machine mode timer and refer to
ACLINT for the definition of registers

On v4 commit,
- PLIC section with [DEPRECATED] in Linux- 2022 chapter
- CLINT section in Linux- 2022 chapter for M-mode timer. We don't
mention
  IPI because AIA already supported it.
- In Embedded-2022 Machine mode timer section, CLINT is not
mandatory.
- Separate section in appendix for the Machine mode timer registers

On v3 commit,
- Address review comments.

On v2 commit,
- CLINT is not deprecated.

- Add a standalone section for Machine Mode Timer in System
Peripherals.
  Do you think this is a good place for Machine Mode Timer?
  @Mayuresh, please check if you are ok with this change, not sure if
this
  overlaps with your text or not (The timer setion). I can remove
this
  if you prefer to put this with your patch.

- In Embedded-2022, refer to Machine Mode Timer in System Peripherals
  section and CLINT in Linux-2022 Platform.
  @Alistair, is this ok?

On v1 commit,
- Not sure where to put the [DEPRECATED].
- Change the reference of PLIC in section 2.2.2. Interrupt Controller
to
  1.1.3.2 PLIC + CLINT section.

Signed-off-by: Abner Chang <renba.chang@...>
Cc: Alistair Francis <alistair.francis@...>
Cc: Sunil V L <sunilvl@...>
Cc: Mayuresh Chitale <mchitale@...>
---
 riscv-platform-spec.adoc | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/riscv-platform-spec.adoc b/riscv-platform-spec.adoc
index 160c74a..c0ee75d 100644
--- a/riscv-platform-spec.adoc
+++ b/riscv-platform-spec.adoc
@@ -49,9 +49,24 @@ include::profiles.adoc[]
 * Start Address
 
 ==== Interrupt Controller
-* AIA
-* PLIC + CLINT
-* Interrupt Assignments
+===== AIA[[AIA]]
+===== PLIC[DEPRECATED][[PLIC]]
+The Platform Level Interrupt Controller (PLIC) provides facilities
to route
+the non-local interrupts to the external interrupt of a hart context
+with a given privilege mode in a given hart. The number of non-local
interrupt
+sources supported by PLIC and how does each of them connect to the
hart
+context is PLIC core implementation-specific. +
+(Refer to
https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc[RISC-V
 PLIC Specification]

IIRC, PLIC spec was never reviewed widely. As this group is more active
now, tt would be good to send it as a separate patch so we can do a
detailed review of that as well.

I am just concerned about semantics rather than technical details.

+for the implementation reference of PLIC operation parameters)
+
+===== ACLINT
+Linux-2020 platform 

We have now official names for the PLATFORM spec. We should refer to
that.

requires the Advanced Core Local Interruptor (ACLINT,
+refer to
+
https://github.com/riscv/riscv-aclint/blob/master/riscv-aclint.adoc[RISC-V
 ACLINT Specification])
+to provide facilities to route inter-processor interrupt and Machine
mode timer
+interrupt to each RISC-V processor hart.
+

+===== Interrupt Assignments
 
 ==== System Peripherals
 * UART/Serial Console
@@ -289,8 +304,8 @@ Any RISC-V system that uses at least RV32/64G can
meet the Embedded-2022
 specification.
 
 ==== Interrupt Controller
-Embedded systems are recommended to use a spec compliant
-https://github.com/riscv/riscv-plic-spec[PLIC], a spec compliant
+Embedded systems are recommended to use a spec compliant
<<PLIC,PLIC>>,
+a spec compliant
 
https://github.com/riscv/riscv-fast-interrupt/blob/master/clic.adoc[CLIC]
 or both a CLIC and and PLIC.
 

--
Regards,
Atish

On Fri, 2021-06-04 at 20:48 +0800, Bin Meng wrote:

Hi Heinrich,

On Fri, Jun 4, 2021 at 8:13 PM Heinrich Schuchardt <
xypron.glpk@...> wrote:

a
On 6/4/21 11:57 AM, Bin Meng wrote:

Signed-off-by: Bin Meng <bmeng.cn@...>
---

  riscv-sbi.adoc | 6 ++++++
  1 file changed, 6 insertions(+)

diff --git a/riscv-sbi.adoc b/riscv-sbi.adoc
index 11c30c3..8696f97 100644
--- a/riscv-sbi.adoc
+++ b/riscv-sbi.adoc
@@ -34,6 +34,7 @@ https://creativecommons.org/licenses/by/4.0/.
  * Improved SBI introduction secion
  * Improved documentation of SBI hart state managment extension
  * Added suspend function to SBI hart state managment extension
+* Clarified that a SBI extension cannot be partially implemented

  === Version 0.2

@@ -51,6 +52,11 @@ abstraction for platform (or hypervisor)
specific functionality. The design
  of the SBI follows the general RISC-V philosophy of having a
small core along
  with a set of optional modular extensions.

+SBI extensions as whole are optional but if a SBI <abc>
extension compliant

%s/a SBI/an SBI/ (as you will pronounce SBI as as-bee-aye)

Sure. Will send a new patch to fix other places in the same file.

+with SBI v0.X spec is implemented then all functions of SBI
<abc> extension
+as defined in SBI v0.X are assumed to be present. Basically, a
SBI extension

Can we do away with all the placeholders?

How about:

"SBI extensions as whole are optional but they shall not be
partially
implemented: If sbi_probe_extension() signals that an extension is
available, it must be implemented in total and conform to the SBI
version reported by sbi_get_spec_version()."

This one is more verbose but sounds better to me. May be we should just
explicitly say that "all functions belonging to that extension must be
implemented" similar to the below version.

How about:

If sbi_probe_extension() signals that an extension is available, all
functions that conform to the SBI version reported by
sbi_get_spec_version() must be implemented.

Regards,
Bin

--
Regards,
Atish

%s/a SBI/an SBI/
%s/A SBI/An SBI/

Signed-off-by: Bin Meng <bmeng.cn@...>
---

riscv-sbi.adoc | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/riscv-sbi.adoc b/riscv-sbi.adoc
index 11c30c3..b02332e 100644
--- a/riscv-sbi.adoc
+++ b/riscv-sbi.adoc
@@ -52,8 +52,8 @@ of the SBI follows the general RISC-V philosophy of having a small core along
with a set of optional modular extensions.

The higher privilege software providing SBI interface to the supervisor-mode
-software is referred to as a SBI implemenation or Supervisor Execution
-Environment (SEE). A SBI implementation (or SEE) can be platform runtime
+software is referred to as an SBI implemenation or Supervisor Execution
+Environment (SEE). An SBI implementation (or SEE) can be platform runtime
firmware executing in machine-mode (M-mode) (see below <<fig_intro1>>) or
it can be some hypervisor executing in hypervisor-mode (HS-mode) (see below
<<fig_intro2>>).
@@ -124,7 +124,7 @@ the specification simple and easily adaptable for all RISC-V ISA types (i.e.
RV32, RV64 and RV128). In case the data is defined as 32bit wide, higher
privilege software must ensure that it only uses 32 bit data only.

-If a SBI function needs to pass a list of harts to the higher privilege mode,
+If an SBI function needs to pass a list of harts to the higher privilege mode,
it must use a hart mask as defined below. This is applicable to any extensions
defined in or after v0.2.

@@ -765,13 +765,13 @@ A platform can have multiple harts grouped into a hierarchical topology
groups (namely cores, clusters, nodes, etc) with separate platform specific
low-power states for each hierarchical group. These platform specific
low-power states of hierarchial topology groups can be represented as
-platform specific suspend states of a hart. A SBI implementation can
+platform specific suspend states of a hart. An SBI implementation can
utilize the suspend states of higher topology groups using one of the
following approaches:

. *Platform-coordinated:* In this approach, when a hart becomes idle the
supervisor-mode power-managment software will request deepest suspend
- state for the hart and higher topology groups. A SBI implementation
+ state for the hart and higher topology groups. An SBI implementation
should choose a suspend state at higher topology group which is:
.. Not deeper than the specified suspend state
.. Wake-up latency is not higher than the wake-up latency of the
@@ -782,7 +782,7 @@ following approaches:
the supervisor-mode power-managment software will always select suspend
state for the hart itself but it will select a suspend state for a higher
topology group only if the hart is the last running hart in the group.
- A SBI implementation should:
+ An SBI implementation should:
.. Never choose a suspend state for higher topology group different from
the specified suspend state
.. Always prefer most recent suspend state requested for higher topology
--
2.25.1

Hi Heinrich,

On Fri, Jun 4, 2021 at 8:13 PM Heinrich Schuchardt <xypron.glpk@...> wrote:

a
On 6/4/21 11:57 AM, Bin Meng wrote:

Signed-off-by: Bin Meng <bmeng.cn@...>
---

riscv-sbi.adoc | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/riscv-sbi.adoc b/riscv-sbi.adoc
index 11c30c3..8696f97 100644
--- a/riscv-sbi.adoc
+++ b/riscv-sbi.adoc
@@ -34,6 +34,7 @@ https://creativecommons.org/licenses/by/4.0/.
* Improved SBI introduction secion
* Improved documentation of SBI hart state managment extension
* Added suspend function to SBI hart state managment extension
+* Clarified that a SBI extension cannot be partially implemented

=== Version 0.2

@@ -51,6 +52,11 @@ abstraction for platform (or hypervisor) specific functionality. The design
of the SBI follows the general RISC-V philosophy of having a small core along
with a set of optional modular extensions.

+SBI extensions as whole are optional but if a SBI <abc> extension compliant

%s/a SBI/an SBI/ (as you will pronounce SBI as as-bee-aye)

Sure. Will send a new patch to fix other places in the same file.

+with SBI v0.X spec is implemented then all functions of SBI <abc> extension
+as defined in SBI v0.X are assumed to be present. Basically, a SBI extension

Can we do away with all the placeholders?

How about:

"SBI extensions as whole are optional but they shall not be partially
implemented: If sbi_probe_extension() signals that an extension is
available, it must be implemented in total and conform to the SBI
version reported by sbi_get_spec_version()."

How about:

If sbi_probe_extension() signals that an extension is available, all
functions that conform to the SBI version reported by
sbi_get_spec_version() must be implemented.

Regards,
Bin

a
On 6/4/21 11:57 AM, Bin Meng wrote:

Signed-off-by: Bin Meng <bmeng.cn@...>
---

riscv-sbi.adoc | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/riscv-sbi.adoc b/riscv-sbi.adoc
index 11c30c3..8696f97 100644
--- a/riscv-sbi.adoc
+++ b/riscv-sbi.adoc
@@ -34,6 +34,7 @@ https://creativecommons.org/licenses/by/4.0/.
* Improved SBI introduction secion
* Improved documentation of SBI hart state managment extension
* Added suspend function to SBI hart state managment extension
+* Clarified that a SBI extension cannot be partially implemented

=== Version 0.2

@@ -51,6 +52,11 @@ abstraction for platform (or hypervisor) specific functionality. The design
of the SBI follows the general RISC-V philosophy of having a small core along
with a set of optional modular extensions.

+SBI extensions as whole are optional but if a SBI <abc> extension compliant

%s/a SBI/an SBI/ (as you will pronounce SBI as as-bee-aye)

+with SBI v0.X spec is implemented then all functions of SBI <abc> extension
+as defined in SBI v0.X are assumed to be present. Basically, a SBI extension

Can we do away with all the placeholders?

How about:

"SBI extensions as whole are optional but they shall not be partially
implemented: If sbi_probe_extension() signals that an extension is
available, it must be implemented in total and conform to the SBI
version reported by sbi_get_spec_version()."

Best regards

Heinrich

+cannot be partially implemented.
+
The higher privilege software providing SBI interface to the supervisor-mode
software is referred to as a SBI implemenation or Supervisor Execution
Environment (SEE). A SBI implementation (or SEE) can be platform runtime

Signed-off-by: Bin Meng <bmeng.cn@...>
---

riscv-sbi.adoc | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/riscv-sbi.adoc b/riscv-sbi.adoc
index 11c30c3..8696f97 100644
--- a/riscv-sbi.adoc
+++ b/riscv-sbi.adoc
@@ -34,6 +34,7 @@ https://creativecommons.org/licenses/by/4.0/.
* Improved SBI introduction secion
* Improved documentation of SBI hart state managment extension
* Added suspend function to SBI hart state managment extension
+* Clarified that a SBI extension cannot be partially implemented

=== Version 0.2

@@ -51,6 +52,11 @@ abstraction for platform (or hypervisor) specific functionality. The design
of the SBI follows the general RISC-V philosophy of having a small core along
with a set of optional modular extensions.

+SBI extensions as whole are optional but if a SBI <abc> extension compliant
+with SBI v0.X spec is implemented then all functions of SBI <abc> extension
+as defined in SBI v0.X are assumed to be present. Basically, a SBI extension
+cannot be partially implemented.
+
The higher privilege software providing SBI interface to the supervisor-mode
software is referred to as a SBI implemenation or Supervisor Execution
Environment (SEE). A SBI implementation (or SEE) can be platform runtime
--
2.25.1

On Fri, Jun 4, 2021 at 5:33 PM Anup Patel <Anup.Patel@...> wrote:

-----Original Message-----
From: Bin Meng <bmeng.cn@...>
Sent: 04 June 2021 14:50
To: Anup Patel <Anup.Patel@...>
Cc: tech-unixplatformspec@...; Atish Patra
<Atish.Patra@...>
Subject: Re: [RISC-V] [tech-unixplatformspec] [PATCH] Add direct memory
access synchronize extension

On Fri, Jun 4, 2021 at 5:06 PM Anup Patel <Anup.Patel@...> wrote:

-----Original Message-----
From: Bin Meng <bmeng.cn@...>
Sent: 04 June 2021 14:12
To: Anup Patel <Anup.Patel@...>
Cc: tech-unixplatformspec@...; Atish Patra
<Atish.Patra@...>
Subject: Re: [RISC-V] [tech-unixplatformspec] [PATCH] Add direct
memory access synchronize extension

On Fri, Jun 4, 2021 at 4:26 PM Anup Patel <Anup.Patel@...> wrote:

-----Original Message-----
From: Bin Meng <bmeng.cn@...>
Sent: 04 June 2021 13:07
To: Anup Patel <Anup.Patel@...>
Cc: tech-unixplatformspec@...; Atish Patra
<Atish.Patra@...>
Subject: Re: [RISC-V] [tech-unixplatformspec] [PATCH] Add direct
memory access synchronize extension

On Thu, Jun 3, 2021 at 11:47 PM Anup Patel <Anup.Patel@...>

wrote:

-----Original Message-----
From: tech-unixplatformspec@... <tech-
unixplatformspec@...> On Behalf Of Bin Meng
Sent: 03 June 2021 21:02
To: Anup Patel <Anup.Patel@...>
Cc: tech-unixplatformspec@...; Atish Patra
<Atish.Patra@...>
Subject: Re: [RISC-V] [tech-unixplatformspec] [PATCH] Add
direct memory access synchronize extension

On Thu, Jun 3, 2021 at 11:13 PM Anup Patel
<anup.patel@...>

wrote:

This patch adds SBI direct memory access synchronize
(DSYN)) extension which allows S-mode (or VS-mode)
software to explicitly synchronize memory with assistance
from the M-mode (or

HS-mode).

Signed-off-by: Anup Patel <anup.patel@...>
---
riscv-sbi.adoc | 95
++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 95 insertions(+)

diff --git a/riscv-sbi.adoc b/riscv-sbi.adoc index
79d98a6..0eb2898
100644
--- a/riscv-sbi.adoc
+++ b/riscv-sbi.adoc
@@ -27,6 +27,10 @@

https://creativecommons.org/licenses/by/4.0/.

[preface]
== Change Log

+=== Version 0.4-rc0

What's our policy of bumping up versions?

This extension is meant for SBI v0.4 based on discussion with Atish.

We will be soon freezing SBI v0.3.

Do we have policies, or planning/schedule of versions?

We have not documented a detailed policy/plan/schedule for all SBI
spec versions.

What is the version supposed to be used for?

The set of functions (or definition of functions) provided by a
SBI extension can change over time so the SBI spec version helps
us differentiate this changes.

For example, SBI HSM extension defined in SBI v0.2 does not
include SBI HSM suspend call but the v0.3 does include SBI HSM
suspend call so the Linux CPUIDLE driver will check both SBI spec
version and availability of SBI HSM extension before using SBI HSM

suspend call.

Any function not supported, OS can make the SBI call, and check its
return value against SBI_ERR_NOT_SUPPORTED. I don't believe an
arbitrary version number really helps here.

We can't blindly call a SBI function just to check if it is present or not.

For example, the SBI HSM suspend call will suspend the current CPU and
the CPU will not resume until some interrupt/resume event happens.

If CPU is successfully suspended, then the function is implemented by SBI
firmware. I don't see why I need to care about the version number.
If suspend function is not available, then SBI_ERR_NOT_SUPPORTED is
returned.

That's why we need to use combination of SBI spec version and SBI
probe extension to know whether a particular SBI function is available
or not.

I would like to re-iterate that SBI extensions as whole are optional
but if a SBI <abc> extension compliant with SBI v0.X spec is
implemented then all functions of SBI <abc> extension as defined in
SBI v0.X are assumed to be present. Basically, a SBI extension cannot
be partially implemented.

Is this clearly documented?

Argh, this should have been documented in the introduction chapter.

I can send a patch.

Also, newly defined SBI extensions won't be available on firmware
implementing older SBI spec version so S-mode software should
always probe SBI extensions based on SBI spec version.

For example, SBI SRST extension will be available in only in
firmware implementing SBI v0.3 or higher.

Like you said, SRST extension can be probed. The version number is
not needed.

Checking both SBI spec version before doing SBI probe helps us avoid
unnecessary SBI probe.

Then why did we invent the probe function in the first place? We can rely on
SBI version anyway and maintain a big function matrix in the OS, but as we
introduce more and more extensions over time, I don't think that's scalable.
Checking SBI spec version before doing SBI probe does not help much
compared to a simple probe without caring about version number.

SBI spec = calling convention + a set of SBI extension

SBI extension = a set of SBI functions

We have the SBI extension probing in SBI spec so that SBI implementation
can skip SBI extension for which some other HW mechanism is available.

For example, SBI TIMER extension is not required when underlying HW
has RISC-V Sstc extension proposed by Greg

I know probe() can be helpful. I just don't see the value of using
version number to determine whether a certain SBI extension is
avaiable.

Regards,
Bin