Implement basic ACLINT support for the MTIMER register #45

chiangkd · 2024-05-26T19:21:43Z

Replace the timer defined in the emu_state_t structure, which originally compared with the instruction counter and triggered software timer interrupts, with the MTIMER memory-mapped peripheral. For each HART (currently only one), there is an MTIMECMP register connected to the MTIMER device.

Additionally, use the TIMER Extension to program the clock for scheduling the next timer event.

static inline sbi_ret_t handle_sbi_ecall_TIMER(vm_t *vm, int32_t fid)
{
    emu_state_t *data = PRIV(vm);
    switch (fid) {
    case SBI_TIMER__SET_TIMER:
        data->aclint.mtimecmp = (((uint64_t) vm->x_regs[RV_R_A1])) << 32 |
                                (uint64_t) vm->x_regs[RV_R_A0];
        return (sbi_ret_t){SBI_SUCCESS, 0};
    default:
        return (sbi_ret_t){SBI_ERR_NOT_SUPPORTED, 0};
    }
}

Replacing timer with MTIMER perpheral also triggers timer interrupts normally.

Clarify that the MTIMER peripheral can also trigger timer interrupts within a scheduled period by monitoring /proc/interrupts reveals an increase in timer interrupts triggered by riscv-timer:

  1:        595  SiFive PLIC   1 Edge      ttyS0
  2:         19  SiFive PLIC   3 Edge      virtio1
  3:          0  SiFive PLIC   2 Edge      virtio0
  5:      58985  RISC-V INTC   5 Edge      riscv-timer
...
  1:        610  SiFive PLIC   1 Edge      ttyS0
  2:         19  SiFive PLIC   3 Edge      virtio1
  3:          0  SiFive PLIC   2 Edge      virtio0
  5:      59196  RISC-V INTC   5 Edge      riscv-timer
...
  1:        625  SiFive PLIC   1 Edge      ttyS0
  2:         19  SiFive PLIC   3 Edge      virtio1
  3:          0  SiFive PLIC   2 Edge      virtio0
  5:      59296  RISC-V INTC   5 Edge      riscv-timer

jserv · 2024-05-26T21:46:20Z

I would like to ask @ranvd for reviewing and collaborating.

aclint.c

jserv · 2024-05-26T22:56:22Z

Replace the timer defined in the emu_state_t structure, which originally compared with the instruction counter and triggered software timer interrupts, with the MTIMER memory-mapped peripheral. For each HART (currently only one), there is an MTIMECMP register connected to the MTIMER device.
Additionally, use the TIMER Extension to program the clock for scheduling the next timer event.

Could you provide some information on the approach used to validate ACLINT/MTIMER?

shengwen-tw · 2024-05-27T06:14:28Z

I'll take a look soon and maybe verify with virtio-gpu to see if there exists any potential problems.

shengwen-tw

The patch works with virtio-gpu without side effects.

chiangkd · 2024-05-27T17:15:47Z

main.c

@@ -143,6 +147,10 @@ static void mem_store(vm_t *vm, uint32_t addr, uint8_t width, uint32_t value)
            emu_update_vblk_interrupts(vm);
            return;
 #endif
+        case 0x43: /* CLINT */


Sure, I don't mind using 0x44 for CLINT since 0x43 is used by virtio-gpu. Thank you for pointing that out.

main.c

-        data->timer = (((uint64_t) vm->x_regs[RV_R_A1]) << 32) |
-                      (uint64_t) (vm->x_regs[RV_R_A0]);
+        data->aclint.mtimecmp = vm->x_regs[RV_R_A0];
+        data->aclint.mtimecmph = vm->x_regs[RV_R_A1];


aclint.c

+                            uint32_t *value)
+{
+    switch (addr) {
+    case 0x0:


shengwen-tw · 2024-05-27T14:04:53Z

main.c

-        else
-            vm.sip &= ~RV_INT_STI_BIT;
+        if (!++vm.mtime)
+            vm.mtimeh++;


@jserv:
mtimeh variable increases whenever mtime overflows to zero, is this a good way?
Also, mtimeh is reset to zero via overflowing.

We need something more elegant. See https://luckyresistor.me/2019/07/10/real-time-counter-and-integer-overflow/

Implement mtime as a Real-time counter (RTC) in #5ee9e9c and check interrupt with delta (difference between mtimecmp and mtime) is zero or negative (highest bit set)

chiangkd · 2024-05-27T19:12:57Z

Replace the timer defined in the emu_state_t structure, which originally compared with the instruction counter and triggered software timer interrupts, with the MTIMER memory-mapped peripheral. For each HART (currently only one), there is an MTIMECMP register connected to the MTIMER device.
Additionally, use the TIMER Extension to program the clock for scheduling the next timer event.

Could you provide some information on the approach used to validate ACLINT/MTIMER?

Replacing timer with MTIMER perpheral also triggers timer interrupts normally.

Clarify that the MTIMER peripheral can also trigger timer interrupts within a scheduled period by monitoring /proc/interrupts reveals an increase in timer interrupts triggered by riscv-timer:

  1:        595  SiFive PLIC   1 Edge      ttyS0
  2:         19  SiFive PLIC   3 Edge      virtio1
  3:          0  SiFive PLIC   2 Edge      virtio0
  5:      58985  RISC-V INTC   5 Edge      riscv-timer
...
  1:        610  SiFive PLIC   1 Edge      ttyS0
  2:         19  SiFive PLIC   3 Edge      virtio1
  3:          0  SiFive PLIC   2 Edge      virtio0
  5:      59196  RISC-V INTC   5 Edge      riscv-timer
...
  1:        625  SiFive PLIC   1 Edge      ttyS0
  2:         19  SiFive PLIC   3 Edge      virtio1
  3:          0  SiFive PLIC   2 Edge      virtio0
  5:      59296  RISC-V INTC   5 Edge      riscv-timer

ranvd · 2024-05-28T14:45:15Z

riscv.c

+        break;
+    case RV_CSR_TIMEH:
+        *value = vm->mtimeh;
+        break;


The code from lines 432 to 445 should be removed, or the code here may not be used while simulating.

Thank you for pointing that out.

jserv · 2024-06-01T04:34:13Z

Clarify that the MTIMER peripheral can also trigger timer interrupts within a scheduled period by monitoring /proc/interrupts reveals an increase in timer interrupts triggered by riscv-timer:

  1:        595  SiFive PLIC   1 Edge      ttyS0
  2:         19  SiFive PLIC   3 Edge      virtio1
  3:          0  SiFive PLIC   2 Edge      virtio0
  5:      58985  RISC-V INTC   5 Edge      riscv-timer
...
  1:        610  SiFive PLIC   1 Edge      ttyS0
  2:         19  SiFive PLIC   3 Edge      virtio1
  3:          0  SiFive PLIC   2 Edge      virtio0
  5:      59196  RISC-V INTC   5 Edge      riscv-timer
...
  1:        625  SiFive PLIC   1 Edge      ttyS0
  2:         19  SiFive PLIC   3 Edge      virtio1
  3:          0  SiFive PLIC   2 Edge      virtio0
  5:      59296  RISC-V INTC   5 Edge      riscv-timer

The above should appear in git commit messages for future reference purpose.

jserv · 2024-06-02T17:15:41Z

riscv_private.h

@@ -56,6 +56,10 @@ enum {

    /* S-mode (Supervisor Protection and Translation) */
    RV_CSR_SATP = 0x180, /**< Supervisor address translation and protection */
+
+    /* Unprivileged Timers */
+    RV_CSR_TIME = 0xC01,  /**< Timer for RDTIME instruction */


RDTIME is a pseudo-instruction. Quote from The RISC-V Instruction Set Manual : Volume I: User-Level ISA:

The RDTIME pseudo-instruction reads the low XLEN bits of the time CSR, which counts wall-clock real time that has passed from an arbitrary start time in the past. RDTIMEH is an RV32I-only instruction that reads bits 63–32 of the same real-time counter. The underlying 64-bit counter should never overflow in practice. The execution environment should provide a means of determining the period of the real-time counter (seconds/tick). The period must be constant. The real-time clocks of all hardware threads in a single user application should be synchronized to within one tick of the real-time clock. The environment should provide a means to determine the accuracy of the clock.

jserv · 2024-06-02T17:17:20Z

If this pull request is ready for reviewing, use git rebase -i to squash and rework the git commit message. Then, request the reviewers via GitHub website.

In this commit, replace the `timer` defined in the `emu_state_t` structure, which originally compared with the instruction counter and triggered software timer interrupts. Further, make 64-bit unsigned integer `mtime` with Real time counter (RTC) to prevent mtime overflow. Replacing timer with MTIMER perpheral also triggers timer interrupts normally. Clarify that the MTIMER peripheral can also trigger timer interrupts within a scheduled period by monitoring `/proc/interrupts` reveals an increase in timer interrupts triggered by `riscv-timer`: ``` 1: 595 SiFive PLIC 1 Edge ttyS0 2: 19 SiFive PLIC 3 Edge virtio1 3: 0 SiFive PLIC 2 Edge virtio0 5: 58985 RISC-V INTC 5 Edge riscv-timer ... 1: 610 SiFive PLIC 1 Edge ttyS0 2: 19 SiFive PLIC 3 Edge virtio1 3: 0 SiFive PLIC 2 Edge virtio0 5: 59196 RISC-V INTC 5 Edge riscv-timer ... 1: 625 SiFive PLIC 1 Edge ttyS0 2: 19 SiFive PLIC 3 Edge virtio1 3: 0 SiFive PLIC 2 Edge virtio0 5: 59296 RISC-V INTC 5 Edge riscv-timer ``` Additionally, the `vm_timer_t` structure is use to retrieve system-wide clock through `clock_gettime()`. The clock frequency is configured to 65MHz, which matches the `timebase-frequency` specified in `minimal.dts`.

jserv · 2024-06-13T00:30:58Z

I defer @ranvd for confirmation.

jserv · 2024-06-13T03:02:58Z

main.c

-            vm.sip &= ~RV_INT_STI_BIT;
+        aclint_timer_interrupts(&vm, &emu.aclint);
+        aclint_update_interrupts(&vm, &emu.aclint);
+


Remove this blank line.

jserv · 2024-06-13T03:04:13Z

riscv.c

             */
-            *value = vm->insn_count >> ((addr & (1 << 7)) ? 32 : 0);
+            switch (addr) {
+            case RV_CSR_INSTRET:


Add some comments here. E.g., denote the types.

ranvd · 2024-06-13T05:12:34Z

device.h

+    _(MTIME_LO, 0x7FF8)    \
+    _(MTIME_HI, 0x7FFC)


After enable the clint driver for Linux kernel, the "load access fault" show up. Maybe this is caused by the wrong address map here.

After correcting the error above, the kernel gets stuck after logging out [ 0.010981] clint: registering percpu irq failed [-16]. This error may occur in my implementation as well. I'm still figuring out the reason.

Thank you for pointing out my mistake!

jserv · 2024-06-15T16:34:49Z

Related pull request: #46

chiangkd · 2024-06-20T19:20:40Z

After correcting the error above, the kernel gets stuck after logging out [ 0.010981] clint: registering percpu irq failed [-16]. This error may occur in my implementation as well. I'm still figuring out the reason.

If CONFIG_RISCV_M_MODE is not enable, RV_IRQ_SOFT and RV_IRQ_TIMER default to supervisor level interrupts IRQ_S_SOFT (1) and IRQ_S_TIMER (5). However, in the device tree, the CLINT IRQ numbers are set to 3 and 7, which can trigger invalid hardware interrupts:

[    0.002716] clint: clint@4400000: invalid irq 0 (hwirq 3)
[    0.003920] Failed to initialize '/soc@F0000000/clint@4400000': -19

To resolve this, directly set CLINT IRQ number with 1 and 5 for supervior software and timer interrupt:

clint0: clint@4400000 {
    #interrupt-cells = <1>;
    compatible = "sifive,clint0";
    reg = <0x4400000 0x000C000>;
    interrupts-extended = <&cpu0_intc 1 &cpu0_intc 5>;
};

However, this setup triggers the following warning:

[    0.007611] genirq: Flags mismatch irq 5. 00004400 (clint-timer) vs. 00004400 (riscv-timer)

This indicates that both clint-timer and riscv-timer are reporting the same set of flags 0x00004400 (IRQF_PERCPU | IRQF_NO_SUSPEND) for IRQ 5. riscv-timer is managed by the riscv,cpu-intc interrupt controller and should not be altered.

Does it make sense to prioritize adding M-mode privilege levels support for semu first?

jserv · 2024-06-20T21:08:08Z

Does it make sense to prioritize adding M-mode privilege levels support for semu first?

Yes, M-mode is the highest privilege mode and controls all physical resources and interrupts. Could you check with @ranvd to confirm if there might be any overlapping efforts with pull request #46 ?

ranvd · 2024-06-21T14:25:35Z

This indicates that both clint-timer and riscv-timer are reporting the same set of flags 0x00004400 (IRQF_PERCPU | IRQF_NO_SUSPEND) for IRQ 5. riscv-timer is managed by the riscv,cpu-intc interrupt controller and should not be altered.

Thank you for providing this information! I reviewed some .config files from some RISC-V simulator repositories and noticed that they don't enable the CLINT driver. So, I navigated the Kconfig at arch/riscv/Kconfig, and I found that CLINT_TIMER is selected by RISCV if !MMU, and RISCV_TIMER is selected by RISCV if RISCV_SBI is set. Besides, RISCV_SBI depends on !RISCV_M_MODE, and RISCV_M_MODE is !MMU by default.

Since we are running the Linux kernel in supervisor mode, RISCV_M_MODE is not set, and MMU is set by default. Can we say that we can't enable both CLINT_TIMER and RISCV_TIMER at the same time? Or at least it is not recommended.

Does it make sense to prioritize adding M-mode privilege levels support for semu first?

So, according to the clues above, I suggest not implementing M-mode for semu since semu already support SBI for the Linux kernel.

chiangkd · 2024-06-23T03:49:15Z

So, according to the clues above, I suggest not implementing M-mode for semu since semu already support SBI for the Linux kernel.

I agree with the previous discussion. The /drivers/clocksource/timer-clint.c file provides supporting evidence:

...
 * Most of the M-mode (i.e. NoMMU) RISC-V systems usually have a
 * CLINT MMIO timer device.
...

Additionally, riscv-timer.c mentions:

...
 * All RISC-V systems have a timer attached to every hart.  These timers can
 * either be read from the "time" and "timeh" CSRs, and can use the SBI to
 * setup events, or directly accessed using MMIO registers.
...

As this PR overlaps with PR #46, it should be closed. I will create a new PR to implement time CSR for generating software timer interrupts, originally triggered by instruction count:

if (vm.insn_count > emu.timer)
    vm.sip |= RV_INT_STI_BIT;
else
    vm.sip &= ~RV_INT_STI_BIT;

This new feature can be integrated after PR #46 is complete.

jserv requested review from ChinYikMing and shengwen-tw May 26, 2024 21:26

jserv reviewed May 26, 2024

View reviewed changes

aclint.c Show resolved Hide resolved

jserv reviewed May 26, 2024

View reviewed changes

aclint.c Show resolved Hide resolved

shengwen-tw reviewed May 27, 2024

View reviewed changes

ranvd reviewed May 28, 2024

View reviewed changes

chiangkd force-pushed the semu_clint branch from 1a0177e to 5ee9e9c Compare June 2, 2024 17:06

jserv reviewed Jun 2, 2024

View reviewed changes

chiangkd force-pushed the semu_clint branch from 071c132 to 4fd0be6 Compare June 12, 2024 19:27

chiangkd force-pushed the semu_clint branch from 4fd0be6 to de48e61 Compare June 12, 2024 19:30

chiangkd requested a review from jserv June 12, 2024 19:35

jserv requested a review from ranvd June 13, 2024 00:27

jserv reviewed Jun 13, 2024

View reviewed changes

ranvd reviewed Jun 13, 2024

View reviewed changes

chiangkd closed this Jun 24, 2024

chiangkd deleted the semu_clint branch June 25, 2024 14:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement basic ACLINT support for the MTIMER register #45

Implement basic ACLINT support for the MTIMER register #45

chiangkd commented May 26, 2024 •

edited

Loading

jserv commented May 26, 2024

jserv commented May 26, 2024

shengwen-tw commented May 27, 2024

shengwen-tw left a comment

This comment was marked as resolved.

chiangkd May 27, 2024

This comment was marked as resolved.

This comment was marked as resolved.

shengwen-tw May 27, 2024

jserv May 28, 2024

chiangkd Jun 2, 2024

chiangkd commented May 27, 2024 •

edited

Loading

ranvd May 28, 2024 •

edited

Loading

chiangkd Jun 2, 2024

jserv commented Jun 1, 2024

jserv Jun 2, 2024

jserv commented Jun 2, 2024

jserv commented Jun 13, 2024

jserv Jun 13, 2024

jserv Jun 13, 2024

ranvd Jun 13, 2024 •

edited

Loading

ranvd Jun 13, 2024

chiangkd Jun 16, 2024

jserv commented Jun 15, 2024

chiangkd commented Jun 20, 2024

jserv commented Jun 20, 2024

ranvd commented Jun 21, 2024

chiangkd commented Jun 23, 2024

Implement basic ACLINT support for the MTIMER register #45

Implement basic ACLINT support for the MTIMER register #45

Conversation

chiangkd commented May 26, 2024 • edited Loading

jserv commented May 26, 2024

jserv commented May 26, 2024

shengwen-tw commented May 27, 2024

shengwen-tw left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chiangkd commented May 27, 2024 • edited Loading

ranvd May 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jserv commented Jun 1, 2024

Choose a reason for hiding this comment

jserv commented Jun 2, 2024

jserv commented Jun 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranvd Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jserv commented Jun 15, 2024

chiangkd commented Jun 20, 2024

jserv commented Jun 20, 2024

ranvd commented Jun 21, 2024

chiangkd commented Jun 23, 2024

chiangkd commented May 26, 2024 •

edited

Loading

chiangkd commented May 27, 2024 •

edited

Loading

ranvd May 28, 2024 •

edited

Loading

ranvd Jun 13, 2024 •

edited

Loading