Conversation
theduccom
commented
Aug 30, 2025
theduccom
commented
- Adds a spinlock that disables interrupts
- Adds a hartlock, which disables interrupts
Signed-off-by: theduccom <145524869+theduccom@users.noreply.github.com>
Signed-off-by: theduccom <145524869+theduccom@users.noreply.github.com>
Signed-off-by: theduccom <145524869+theduccom@users.noreply.github.com>
Signed-off-by: theduccom <145524869+theduccom@users.noreply.github.com>
Signed-off-by: theduccom <145524869+theduccom@users.noreply.github.com>
remexre
left a comment
remexre
left a comment
There was a problem hiding this comment.
sleepy so did not apply filter; feel free to push back on these if i'm being too nit-picky
| * | ||
| * Return: u32 1 if SIE bit in SSTATUS is set, 0 otherwise. | ||
| */ | ||
| static inline u32 get_irq_state(void) { |
There was a problem hiding this comment.
why not bool, and maybe a name like are_interrupts_enabled?
| static inline void irq_enable(void) { | ||
| u64 sstatus = csrr(RISCV64_CSR_SSTATUS); | ||
| csrw(RISCV64_CSR_SSTATUS, (sstatus | RISCV64_SSTATUS_SIE)); | ||
| } | ||
|
|
||
| /** | ||
| * irq_disable - Disables interrupt requests. | ||
| * | ||
| * Masks the SIE (supervisor interrupt enable) flag in the SSTATUS CSR. | ||
| */ | ||
| static inline void irq_disable(void) { | ||
| u64 sstatus = csrr(RISCV64_CSR_SSTATUS); | ||
| csrw(RISCV64_CSR_SSTATUS, (sstatus & ~RISCV64_SSTATUS_SIE)); | ||
| } |
There was a problem hiding this comment.
These should probably use CSRS and CSRC, so that they're atomic. Right now, context switches can happen between the csrr and csrw. I think you'll need to add those helpers to arch/riscv64/insns.h -- should just be a find-and-replace on csrw.
| * | ||
| * Disables interrupts to prevent the current thread from being moved | ||
| * to another hart. This does not provide mutual exclusion, nor does it | ||
| * prevent the thread from blocking. It only ensures that the |
There was a problem hiding this comment.
nor does it prevent the thread from blocking
Shouldn't it? Blocking operations should allow context-switching.
src/kernel/include/spinlock.h
Outdated
| * | ||
| * Initializes a spinlock that disables interrupts. | ||
| */ | ||
| void initlock(struct spinlock *sp, const char *name); |
There was a problem hiding this comment.
prefix the names of these with spinlock_, since we're planning on having multiple kinds of locks; e.g. spinlock_init, spinlock_lock, spinlock_unlock? (_acquire/_release is fine too, but should be consistent with hartlock)
src/kernel/spinlock.c
Outdated
| while (__sync_lock_test_and_set(&sp->state, 1) != 0) {} | ||
| __sync_synchronize(); |
There was a problem hiding this comment.
__sync functions are legacy per GCC docs; use stdatomic.h instead; using the right memory orderings probably actually helps performance in practice, too
src/kernel/spinlock.c
Outdated
| #include "spinlock.h" | ||
|
|
||
| /* Returns 1 if thread is holding the specified spinlock */ | ||
| u32 holding(struct spinlock *sp) { |
There was a problem hiding this comment.
should this return bool? should it be static?
| #include "insns.h" | ||
| #include "types.h" | ||
|
|
||
| #define RISCV64_SSTATUS_SIE (1UL << 1) |
There was a problem hiding this comment.
prefer constexpr to #define (better typechecking and error reporting)
src/kernel/spinlock.c
Outdated
| /* Returns 1 if thread is holding the specified spinlock */ | ||
| u32 holding(struct spinlock *sp) { | ||
| struct hart_locals *hart = get_hart_locals(); | ||
| return ((sp->state == 1) && (sp->hart == hart)); |
There was a problem hiding this comment.
either make state atomic or use atomic load here
| * was called. If interrupts were already disabled, they remain disabled. | ||
| */ | ||
| void hart_unlock(void); | ||
|
|
There was a problem hiding this comment.
is_hart_locked would be nice-to-have for e.g. get_hart_locals to assert on
| @@ -0,0 +1,40 @@ | |||
| #ifndef UKO_OS_KERNEL__IRQ_H | |||
There was a problem hiding this comment.
oh, didn't realize this was going to so closely correspond to what's in hartlock.c -- one thing you can do is to have an arch-independent foo.h, but put the .c under arch/
Signed-off-by: theduccom <145524869+theduccom@users.noreply.github.com>
Signed-off-by: theduccom <145524869+theduccom@users.noreply.github.com>
Signed-off-by: theduccom <145524869+theduccom@users.noreply.github.com>
Signed-off-by: theduccom <145524869+theduccom@users.noreply.github.com>
|
@theduccom Do you think you'll have time to get this up-to-date and respond to the review feedback? I'm planning on doing #9 soon, and I'll need hartlocks by then. |
Signed-off-by: theduccom <145524869+theduccom@users.noreply.github.com>
2 participants
