The Wayback Machine - https://web.archive.org/web/20201223155128/https://github.com/tinygo-org/tinygo/pull/1227
Skip to content

/ tinygo

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

riscv: allow interrupts to wake the scheduler #1227

Open
wants to merge 1 commit into
base: dev
from
Open

riscv: allow interrupts to wake the scheduler #1227

wants to merge 1 commit into from

Conversation

@yannishuber
Copy link
Contributor

@yannishuber yannishuber commented Jul 10, 2020 •
edited

This PR allows interrupts to wake the scheduler for the FE310 and the K210 RISC-V chips.

Depends on #1220
@yannishuber
Copy link
Contributor Author

@yannishuber yannishuber commented Jul 10, 2020

Also somewhat related:

tinygo/src/runtime/arch_tinygoriscv.go

Lines 84 to 90 in 04d097f

func waitForEvents() {
mask := riscv.DisableInterrupts()
if !runqueue.Empty() {
riscv.Asm("wfi")
}
riscv.EnableInterrupts(mask)
}

Isn't this blocking forever since we are waiting for an interrupt with interrupts disabled?
@niaow
Copy link
Member

@niaow niaow commented Jul 10, 2020

No. The interrupt is masked, so it will not run immediately. Instead what happens is the wfi finishes when the interrupt is pending, then interrupts are re-enabled, then the interrupt executes.
@yannishuber
Copy link
Contributor Author

@yannishuber yannishuber commented Jul 10, 2020

@jaddr2line All right but then shouldn't the riscv.DisableInterrupts() disable interrupts on the global mstatus level because according to the specification page 41:

The WFI instruction can also be executed when interrupts are disabled. The operation of WFI must
be unaffected by the global interrupt bits in mstatus (MIE/SIE/UIE) [...] but should honor the individual interrupt enables
(e.g, MTIE) (i.e., implementations should avoid resuming the hart if the interrupt is pending but
not individually enabled)

Because right now we disable interrupts in the lower individual interrupt enable register:

tinygo/src/device/riscv/riscv.go

Lines 25 to 37 in 04d097f

func DisableInterrupts() uintptr {
// Note: this can be optimized with a CSRRW instruction, which atomically
// swaps the value and returns the old value.
mask := MIE.Get()
MIE.Set(0)
return mask
}
// EnableInterrupts enables all interrupts again. The value passed in must be
// the mask returned by DisableInterrupts.
func EnableInterrupts(mask uintptr) {
MIE.Set(mask)
}

But maybe I understand this wrong.
@niaow
Copy link
Member

@niaow niaow commented Jul 10, 2020

Huh. I don't know.
@yannishuber
Copy link
Contributor Author

@yannishuber yannishuber commented Jul 10, 2020

@jaddr2line I will make some tests when I have time, to check if interruptions wake the scheduler as expected.
@yannishuber yannishuber force-pushed the yannishuber:riscv-wake-sched branch from 08cdfc1 to 6c037d1 Jul 10, 2020
@deadprogram
Copy link
Member

@deadprogram deadprogram commented Jul 11, 2020

Please make sure you rebase against dev that might get rid of the current build errors.
@niaow
Copy link
Member

@niaow niaow commented Jul 11, 2020

So, I don't think this is actually safe at this point. Right now, the coroutines implementation may end up with a goroutine suspending in the middle of a critical section due to how returns are implemented. In order to make this safe, we need to modify the coroutine task implementation to loop until it is actually fully suspended.
@yannishuber
Copy link
Contributor Author

@yannishuber yannishuber commented Jul 11, 2020

@deadprogram I will do that thanks.

@jaddr2line So it should be safe once #1220 gets merged right?
@niaow
Copy link
Member

@niaow niaow commented Jul 11, 2020

Yeah. Although I should eventually make coroutines work too.
@yannishuber
riscv: allow interrupts to wake the scheduler
Loading status checks…
6ecfa38
@yannishuber yannishuber force-pushed the yannishuber:riscv-wake-sched branch from 6c037d1 to 6ecfa38 Jul 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
@yannishuber @niaow @deadprogram
You can’t perform that action at this time.