On Fri, 2021-07-09 at 23:04 +0530, Mayuresh Chitale wrote:

> +====== PCIe cache coherency
> +PCIe transactions that are not marked as No_snoop and access memory
> that is

Greg had few suggestion to change this statement.

"PCIe transactions that are marked with a No Snoop bit of zero and
access memory.."

Then what I was referring to would be another dedicated watchdog timer (secure one) managed by the secure software (management mode software) running in RISC-V TEE, right?

Shall we define that secure watchdog timer for the server extension now? Or just wait until we have a clear picture of TEE and OpenSBI support for TEE?

On Thu, Jul 8, 2021 at 8:24 PM Abner Chang <renba.chang@...> wrote:

If the watchdog timer remains un-refreshed for a second period,
+then a second-stage watchdog timeout occurs.

Does it mean the second-stage watchdog timer would be timeout 1 second after the first-stage watchdog timer has not been updated?

What the sentence is actually referring to is the following:

The first watchdog timeout is based on a first programmable timeout period, and the second watchdog timeout is based on a second (separately) programmable timeout period.

On the server platform, firmware usually updates the watchdog timer in the background even under OS in runtime. When the watchdog interrupt is triggered, the firmware takes over, sends the log to BMC, and asks BMC to either shutdown or reset the system according to scenarios. We don't like to see OS or hypervisor in the middle to delay the system recovery, for example, OS or processor is somehow halted and the temperature in the box goes pretty high rapidly.

Is 1 second too long for the crisis recovery? Should the first-stage watchdog timer be able to configure as either M or S mode interrupt? So firmware can handle the crisis immediately.

Note that we're spec'ing similar behaviors to what others, like SBSA, spec.  Also note that, in SBSA for example, there are both non-secure and secure watchdogs.  The above watchdog is equivalent to a non-secure watchdog.  Whereas I think what you're thinking of corresponds to a secure watchdog timer - which is controlled by secure software (analogous to M-mode software).  Whereas the non-secure watchdog is controlled by non-secure software, i.e. the OS or hypervisor.

Greg

Mayuresh Chitale <mchitale@...> 於 2021年7月7日 週三 上午1:42寫道：

This patch describes requirements for the watchdog timer
for the server extension.

Signed-off-by: Greg Favor <gfavor@...>
Signed-off-by: Mayuresh Chitale <mchitale@...>
---
 riscv-platform-spec.adoc | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/riscv-platform-spec.adoc b/riscv-platform-spec.adoc
index 87ab7f8..3b5728a 100644
--- a/riscv-platform-spec.adoc
+++ b/riscv-platform-spec.adoc
@@ -470,6 +470,28 @@ https://lists.riscv.org/g/tech-privileged/message/404[Sstc] extension.
 ** Platforms are required to delegate the supervisor timer interrupt to 'S'
 mode. If the 'H' extension is implemented then the platforms are required to
 delegate the virtual supervisor timer interrupt to 'VS' mode.
+
+===== Watchdog Timers
+Implementation of a two-stage watchdog timer, as defined in the WatchDog Timer
+appendix footnote:[Watchdog Timer Appendix (TBD)] ,is required. Software must
+periodically refresh the watchdog timer, otherwise a first-stage watchdog
+timeout occurs. If the watchdog timer remains un-refreshed for a second period,
+then a second-stage watchdog timeout occurs.

Does it mean the second-stage watchdog timer would be timeout 1 second after the first-stage watchdog timer has not been updated?

On the server platform, firmware usually updates the watchdog timer in the background even under OS in runtime. When the watchdog interrupt is triggered, the firmware takes over, sends the log to BMC, and asks BMC to either shutdown or reset the system according to scenarios. We don't like to see OS or hypervisor in the middle to delay the system recovery, for example, OS or processor is somehow halted and the temperature in the box goes pretty high rapidly.

Is 1 second too long for the crisis recovery? Should the first-stage watchdog timer be able to configure as either M or S mode interrupt? So firmware can handle the crisis immediately.

Abner

+
+If a first-stage watchdog timeout occurs, a Supervisor-level interrupt request
+is generated and sent to the system interrupt controller, targeting a specific
+hart.

+
+If a second-stage watchdog timeout occurs, a system-level interrupt request is
+generated and sent to a system component more privileged than Supervisor-mode
+such as:
+
+- The system interrupt controller, with a Machine-level interrupt request
+targeting a specific hart
+- A platform management processor
+- Dedicated reset control logic
+
+The resultant action taken is platform-specific.
 * PCI-E

 ==== Secure Boot
--
2.17.1

If the watchdog timer remains un-refreshed for a second period,
+then a second-stage watchdog timeout occurs.

Does it mean the second-stage watchdog timer would be timeout 1 second after the first-stage watchdog timer has not been updated?

On the server platform, firmware usually updates the watchdog timer in the background even under OS in runtime. When the watchdog interrupt is triggered, the firmware takes over, sends the log to BMC, and asks BMC to either shutdown or reset the system according to scenarios. We don't like to see OS or hypervisor in the middle to delay the system recovery, for example, OS or processor is somehow halted and the temperature in the box goes pretty high rapidly.

Is 1 second too long for the crisis recovery? Should the first-stage watchdog timer be able to configure as either M or S mode interrupt? So firmware can handle the crisis immediately.

This patch describes requirements for the watchdog timer
for the server extension.

Signed-off-by: Greg Favor <gfavor@...>
Signed-off-by: Mayuresh Chitale <mchitale@...>
---
 riscv-platform-spec.adoc | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/riscv-platform-spec.adoc b/riscv-platform-spec.adoc
index 87ab7f8..3b5728a 100644
--- a/riscv-platform-spec.adoc
+++ b/riscv-platform-spec.adoc
@@ -470,6 +470,28 @@ https://lists.riscv.org/g/tech-privileged/message/404[Sstc] extension.
 ** Platforms are required to delegate the supervisor timer interrupt to 'S'
 mode. If the 'H' extension is implemented then the platforms are required to
 delegate the virtual supervisor timer interrupt to 'VS' mode.
+
+===== Watchdog Timers
+Implementation of a two-stage watchdog timer, as defined in the WatchDog Timer
+appendix footnote:[Watchdog Timer Appendix (TBD)] ,is required. Software must
+periodically refresh the watchdog timer, otherwise a first-stage watchdog
+timeout occurs. If the watchdog timer remains un-refreshed for a second period,
+then a second-stage watchdog timeout occurs.

+
+If a first-stage watchdog timeout occurs, a Supervisor-level interrupt request
+is generated and sent to the system interrupt controller, targeting a specific
+hart.

+
+If a second-stage watchdog timeout occurs, a system-level interrupt request is
+generated and sent to a system component more privileged than Supervisor-mode
+such as:
+
+- The system interrupt controller, with a Machine-level interrupt request
+targeting a specific hart
+- A platform management processor
+- Dedicated reset control logic
+
+The resultant action taken is platform-specific.
 * PCI-E

 ==== Secure Boot
--
2.17.1

> +* Platform software shall configure ECAM I/O regions such that the
> effective
> +memory type (PMA + PBMT) is UC.

UC is not present in abbreviation section. Moreover, PMA + PBMT seems
bit ambiguous to me. It wouldn't hurt to be more verbose here.

> +Platform software would likely configure these per root port regions
> such that
> +their effective memory type (PMA + PBMT) is UC. 

same comment as above.

> +====== PCIe cache coherency
> +PCIe transactions that are not marked as No_snoop and access memory
> that is

/s/No_snoop/No Snoop ?

On Sun, 2021-06-20 at 21:32 +0800, Abner Chang wrote:
> From: Abner Chang <abner.chang@...>
>
> This is the commit for creating the patches for
> widely review in Platform Spec HSC task group
>
> Signed-off-by: Abner Chang <abner.chang@...>
> ---
>  riscv-plic.adoc | 306 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 306 insertions(+)
>  create mode 100644 riscv-plic.adoc
>
> diff --git a/riscv-plic.adoc b/riscv-plic.adoc
> new file mode 100644
> index 0000000..b770e0e
> --- /dev/null
> +++ b/riscv-plic.adoc
> @@ -0,0 +1,306 @@
> += *RISC-V Platform-Level Interrupt Controller Specification*
> +
> +== Copyright and license information
> +
> +This RISC-V PLIC specification is
> +
> +[%hardbreaks]
> +(C) 2017 Drew Barbier <drew@...>
> +(C) 2018-2019 Palmer Dabbelt <palmer@...>
> +(C) 2019 Abner Chang, Hewlett Packard Enterprise <
> abner.chang@...>
> +
> +It is licensed under the Creative Commons Attribution 4.0
> International
> +License (CC-BY 4.0).  The full license text is available at
> +https://creativecommons.org/licenses/by/4.0/.
> +
> +== Introduction
> +
> +This document contains the RISC-V platform-level interrupt controller
> (PLIC)
> +specification, which defines an interrupt controller specifically
> designed to
> +work in the context of RISC-V systems.  The PLIC multiplexes various
> device
> +interrupts onto the external interrupt lines of Hart contexts, with
> +hardware support for interrupt priorities. +
> +This specification defines the general PLIC architecture and operation
> parameters.
> +The PLIC which claimed as PLIC-Compliant standard PLIC should follow
> the
> +implementations mentioned in sections below.
> +
> +.Figure 1 RISC-V PLIC Interrupt Architecture Block Diagram
> +image::Images/PLIC.jpg[GitHub,1000,643, link=
> https://github.com/riscv/riscv-plic-spec/blob/master/Images/PLIC.jpg]
> +
> +== RISC-V PLIC Operation Parameters
> +
> +General PLIC operation parameter register blocks are defined in this
> spec, those are: +
> +
> +- *Interrupt Priorities registers:* +
> +   The interrupt priority for each interrupt source. +
> +
> +- *Interrupt Pending Bits registers:* +
> +   The interrupt pending status of each interrupt source. +
> +  
> +- *Interrupt Enables registers:* +
> +   The enablement of interrupt source of each context. +
> +
> +- *Priority Thresholds registers:* +
> +   The interrupt priority threshold of each context. +
> +
> +- *Interrupt Claim registers:* +
> +   The register to acquire interrupt source ID of each context. +
> +  
> +- *Interrupt Completion registers:* +
> +   The register to send interrupt completion message to the associated
> gateway. +
> +
> ++
> +
> +Below is the figure of PLIC Operation Parameter Block Diagram,
> +
> +.Figure 2 PLIC Operation Parameter Block Diagram
> +image::Images/PLICArch.jpg[GitHub, link=
> https://github.com/riscv/riscv-plic-spec/blob/master/Images/PLICArch.jpg
> ]
> +
> +== Memory Map
> +
> +The `base address of PLIC Memory Map` is platform implementation-
> specific.
> +
> +*PLIC Memory Map*
> +
> +       base + 0x000000: Reserved (interrupt source 0 does not exist)
> +       base + 0x000004: Interrupt source 1 priority
> +       base + 0x000008: Interrupt source 2 priority
> +       ...
> +       base + 0x000FFC: Interrupt source 1023 priority
> +       base + 0x001000: Interrupt Pending bit 0-31
> +       base + 0x00107C: Interrupt Pending bit 992-1023
> +       ...     
> +       base + 0x002000: Enable bits for sources 0-31 on context 0
> +       base + 0x002004: Enable bits for sources 32-63 on context 0
> +       ...
> +       base + 0x00207F: Enable bits for sources 992-1023 on context 0
> +       base + 0x002080: Enable bits for sources 0-31 on context 1
> +       base + 0x002084: Enable bits for sources 32-63 on context
> 1     
> +       ...
> +       base + 0x0020FF: Enable bits for sources 992-1023 on context 1
> +       base + 0x002100: Enable bits for sources 0-31 on context 2
> +       base + 0x002104: Enable bits for sources 32-63 on context
> 2     
> +       ...
> +       base + 0x00217F: Enable bits for sources 992-1023 on context 2
> +       ...
> +       base + 0x1F1F80: Enable bits for sources 0-31 on context 15871
> +       base + 0x1F1F84: Enable bits for sources 32-63 on context
> 15871         
> +       base + 0x1F1FFF: Enable bits for sources 992-1023 on context
> 15871
> +       ...     
> +       base + 0x1FFFFC: Reserved
> +       base + 0x200000: Priority threshold for context 0
> +       base + 0x200004: Claim/complete for context 0
> +       base + 0x200008: Reserved
> +       ...
> +       base + 0x200FFC: Reserved
> +       base + 0x201000: Priority threshold for context 1
> +       base + 0x201004: Claim/complete for context 1
> +       ...
> +       base + 0x3FFE000: Priority threshold for context 15871
> +       base + 0x3FFE004: Claim/complete for context 15871
> +       base + 0x3FFE008: Reserved
> +       ...     
> +       base + 0x3FFFFFC: Reserved
> +       
> +Sections below describe the control register blocks of PLIC operation
> parameters.
> +
> +== Register Width
> +
> +The memory map register width is in 32-bit.
> +
> +== Interrupt Priorities
> +
> +If PLIC supports Interrupt Priorities, then each PLIC interrupt source
> can be assigned a priority by writing to its 32-bit
> +memory-mapped `priority` register.  A priority value of 0 is reserved
> to mean ''never interrupt'' and effectively
> +disables the interrupt. Priority 1 is the lowest active priority while
> the maximum level of priority depends on
> +PLIC implementation. Ties between global interrupts of the same
> priority are broken by the Interrupt ID; interrupts
> +with the lowest ID have the highest
> +effective priority. +
> + +
> +The base address of Interrupt Source Priority block within PLIC Memory
> Map region is fixed at 0x000000.
> +
> +[cols="15%,20%,20%,45%"]
> +|===
> +| *PLIC Register Block Name*| *Function*|*Register Block Size in
> Byte*| *Description*
> +|Interrupt Source Priority
> +|Interrupt Source Priority #0 to #1023
> +|1024 * 4 = 4096(0x1000) bytes
> +|This is a continuously memory block which contains PLIC Interrupt
> Source Priority. Total 1024 Interrupt Source Priority
> +in this memory block. Interrupt Source Priority #0 is reserved which
> indicates it does not exist.
> +|===
> +
> +*PLIC Interrupt Source Priority Memory Map* +
> +
> +       0x000000: Reserved (interrupt source 0 does not exist)
> +       0x000004: Interrupt source 1 priority
> +       0x000008: Interrupt source 2 priority
> +       ...
> +       0x000FFC: Interrupt source 1023 priority
> +
> +== Interrupt Pending Bits
> +
> +The current status of the interrupt source pending bits in the PLIC
> core can be
> +read from the pending array, organized as 32-bit register.  The
> pending bit
> +for interrupt ID N is stored in bit (N mod 32) of word (N/32).  Bit 0
> +of word 0, which represents the non-existent interrupt source 0, is
> hardwired
> +to zero.
> +
> +A pending bit in the PLIC core can be cleared by setting the
> associated enable
> +bit then performing a claim. +
> + +
> +The base address of Interrupt Pending Bits block within PLIC Memory
> Map region is fixed at 0x001000.
> +
> +[cols="15%,20%,20%,45%"]
> +|===
> +| *PLIC Register Block Name* | *Function*|*Register Block Size in
> Byte*| *Description*
> +|Interrupt Pending Bits
> +|Interrupt Pending Bit of Interrupt Source #0 to #N
> +|1024 / 8 = 128(0x80) bytes
> +|This is a continuously memory block contains PLIC Interrupt Pending
> Bits. Each Interrupt Pending Bit occupies 1-bit from this register
> block.
> +|===
> +
> +*PLIC Interrupt Pending Bits Memory Map* +
> +
> +       0x001000: Interrupt Source #0 to #31 Pending Bits
> +       ...
> +       0x00107C: Interrupt Source #992 to #1023 Pending Bits
> +
> +
> +== Interrupt Enables
> +
> +Each global interrupt can be enabled by setting the corresponding bit
> in the
> +`enables` register. The `enables` registers are accessed as a
> contiguous array
> +of 32-bit registers, packed the same way as the `pending` bits. Bit 0
> of enable
> +register 0 represents the non-existent interrupt ID 0 and is hardwired
> to 0.
> +PLIC has 15872 Interrupt Enable blocks for the contexts. The `context`
> is referred
> +to the specific privilege mode in the specific Hart of specific RISC-V
> processor
> +instance. How PLIC organizes interrupts for the contexts (Hart and
> privilege mode)
> +is out of RISC-V PLIC specification scope, however it must be spec-out
> in vendor's
> +PLIC specification. +
> + +
> +The base address of Interrupt Enable Bits block within PLIC Memory Map
> region is fixed at 0x002000. +
> + +
> +[cols="15%,20%,20%,45%"]
> +|===
> +| *PLIC Register Block Name* | *Function*|*Register Block Size in
> Byte*| *Description*
> +|Interrupt Enable Bits
> +|Interrupt Enable Bit of Interrupt Source #0 to #1023 for 15872
> contexts
> +|(1024 / 8) * 15872 = 2031616(0x1f0000) bytes
> +|This is a continuously memory block contains PLIC Interrupt Enable
> Bits of 15872 contexts.
> +Each Interrupt Enable Bit occupies 1-bit from this register block and
> total 15872 Interrupt
> +Enable Bit blocks
> +|===
> +
> +*PLIC Interrupt Enable Bits Memory Map* +
> +
> +       0x002000: Interrupt Source #0 to #31 Enable Bits on context 0
> +       ...
> +       0x00207F: Interrupt Source #992 to #1023 Enable Bits on context
> 0
> +       0x002080: Interrupt Source #0 to #31 Enable Bits on context 1
> +       ...
> +       0x0020FF: Interrupt Source #992 to #1023 Enable Bits on context
> 1
> +       0x002100: Interrupt Source #0 to #31 Enable Bits on context 2
> +       ...
> +       0x00217F: Interrupt Source #992 to #1023 Enable Bits on context
> 2
> +       0x002180: Interrupt Source #0 to #31 Enable Bits on context 3
> +       ...
> +       0x0021FF: Interrupt Source #992 to #1023 Enable Bits on context
> 3
> +       ...
> +       ...
> +       ...
> +       0x1F1F80: Interrupt Source #0 to #31 on context 15871   
> +       ...     
> +       0x1F1F80: Interrupt Source #992 to #1023 on context 15871
> +       
> +== Priority Thresholds
> +
> +PLIC provides context based `threshold register` for the settings of a
> interrupt priority
> +threshold of each context. The `threshold register` is a WARL field.
> The PLIC will mask all
> +PLIC interrupts of a priority less than or equal to `threshold`.  For
> example,
> +a`threshold` value of zero permits all interrupts with non-zero
> priority. +
> + +
> +The base address of Priority Thresholds register block is located at
> 4K alignement starts
> +from offset 0x200000.
> +
> +[cols="15%,20%,20%,45%"]
> +|===
> +| *PLIC Register Block Name* | *Function*|*Register Block Size in
> Byte*| *Description*
> +|Priority Threshold
> +|Priority Threshold for 15872 contexts
> +|4096 * 15872 = 65011712(0x3e00000) bytes
> +|This is the register of Priority Thresholds setting for each conetxt
> +|===
> +
> +*PLIC Interrupt Priority Thresholds Memory Map* +
> +
> +       0x200000: Priority threshold for context 0
> +       0x201000: Priority threshold for context 1
> +       0x202000: Priority threshold for context 2
> +       0x203000: Priority threshold for context 3
> +       ...
> +       ...
> +       ...
> +       0x3FFF000: Priority threshold for context 15871
> +       
> +== Interrupt Claim Process
> +
> +The PLIC can perform an interrupt claim by reading the
> `claim/complete`
> +register, which returns the ID of the highest priority pending
> interrupt or
> +zero if there is no pending interrupt.  A successful claim will also
> atomically
> +clear the corresponding pending bit on the interrupt source. +
> +The PLIC can perform a claim at any time and the claim operation is
> not affected
> +by the setting of the priority threshold register. +
> +The Interrupt Claim Process register is context based and is located
> at
> +(4K alignement + 4) starts from offset 0x200000.
> +
> +[cols="15%,20%,20%,45%"]
> +|===
> +| *PLIC Register Block Name* | *Function*|*Register Block Size in
> Byte*| *Description*
> +|Interrupt Claim Register
> +|Interrupt Claim Process for 15872 contexts
> +|4096 * 15872 = 65011712(0x3e00000) bytes
> +|This is the register used to acquire interrupt ID for each conetxt
> +|===
> +
> +*PLIC Interrupt Claim Process Memory Map* +
> +
> +       0x200004: Interrupt Claim Process for context 0
> +       0x201004: Interrupt Claim Process for context 1
> +       0x202004: Interrupt Claim Process for context 2
> +       0x203004: Interrupt Claim Process for context 3
> +       ...
> +       ...
> +       ...
> +       0x3FFF004: Interrupt Claim Process for context 15871
> +       
> +## Interrupt Completion
> +
> +The PLIC signals it has completed executing an interrupt handler by
> writing the
> +interrupt ID it received from the claim to the `claim/complete`
> register.  The
> +PLIC does not check whether the completion ID is the same as the last
> claim ID
> +for that target.  If the completion ID does not match an interrupt
> source that
> +is currently enabled for the target, the completion is silently
> ignored. +
> +The Interrupt Completion registers are context based and located at
> the same address
> +with Interrupt Claim Process register, which is at (4K alignement + 4)
> starts from
> +offset 0x200000.
> + +
> +[cols="15%,20%,20%,45%"]
> +|===
> +| *PLIC Register Block Name* | *Registers*|*Register Block Size in
> Byte*| *Description*
> +|Interrupt Completion Register
> +|Interrupt Completion  for 15872 contexts
> +|4096 * 15872 = 65011712(0x3e00000) bytes
> +|This is register to write to complete Interrupt process
> +|===
> +
> +*PLIC Interrupt Completion Memory Map* +
> +
> +       0x200004: Interrupt Completion for context 0
> +       0x201004: Interrupt Completion for context 1
> +       0x202004: Interrupt Completion for context 2
> +       0x203004: Interrupt Completion for context 3
> +       ...
> +       ...
> +       ...
> +       0x3FFF004: Interrupt Completion for context 15871
> +

I think it will be good to provide additional clarification in the
beginning about the number of interrupts/contexts. Something along the
lines:

The PLIC specification supports up-to 1024 interrupts and 15872
contexts. But the actual number of interrupts and context depends on
the PLIC implementation. However, the implement must adhere to the
offset of each register within the PLIC block.

Apart from that, LGTM.
Reviewed-by: Atish Patra <atish.patra@...>

--
Regards,
Atish