Skip to content

Conversation

@yannishuber
Copy link
Contributor

@yannishuber yannishuber commented Jul 10, 2020

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

Also somewhat related:

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 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

@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:

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 commented Jul 10, 2020

Huh. I don't know.

@yannishuber
Copy link
Contributor Author

@jaddr2line I will make some tests when I have time, to check if interruptions wake the scheduler as expected.

@deadprogram
Copy link
Member

Please make sure you rebase against dev that might get rid of the current build errors.

@niaow
Copy link
Member

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

@deadprogram I will do that thanks.

@jaddr2line So it should be safe once #1220 gets merged right?

@niaow
Copy link
Member

niaow commented Jul 11, 2020

Yeah. Although I should eventually make coroutines work too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants