feat: add PS/2 keyboard interrupt driver#2532
Conversation
7c21b3f to
5d35a06
Compare
There was a problem hiding this comment.
Benchmark Results
Details
| Benchmark | Current: a7180a7 | Previous: 2e23902 | Performance Ratio |
|---|---|---|---|
| startup_benchmark Build Time | 79.29 s |
80.34 s |
0.99 ❗ |
| startup_benchmark File Size | 0.80 MB |
0.80 MB |
1.00 ❗ |
| Startup Time - 1 core | 0.75 s (±0.02 s) |
0.75 s (±0.02 s) |
1.01 |
| Startup Time - 2 cores | 0.75 s (±0.02 s) |
0.74 s (±0.02 s) |
1.01 |
| Startup Time - 4 cores | 0.75 s (±0.02 s) |
0.74 s (±0.02 s) |
1.00 |
| multithreaded_benchmark Build Time | 80.80 s |
82.11 s |
0.98 ❗ |
| multithreaded_benchmark File Size | 0.86 MB |
0.86 MB |
1.00 ❗ |
| Multithreaded Pi Efficiency - 2 Threads | 88.09 % (±7.19 %) |
85.89 % (±6.61 %) |
1.03 |
| Multithreaded Pi Efficiency - 4 Threads | 43.67 % (±2.15 %) |
43.43 % (±2.56 %) |
1.01 |
| Multithreaded Pi Efficiency - 8 Threads | 25.79 % (±1.10 %) |
25.76 % (±1.53 %) |
1.00 |
| micro_benchmarks Build Time | 76.54 s |
80.40 s |
0.95 ❗ |
| micro_benchmarks File Size | 0.86 MB |
0.86 MB |
1.00 ❗ |
| Scheduling time - 1 thread | 61.03 ticks (±2.90 ticks) |
62.65 ticks (±4.06 ticks) |
0.97 |
| Scheduling time - 2 threads | 33.09 ticks (±4.34 ticks) |
34.08 ticks (±4.10 ticks) |
0.97 |
| Micro - Time for syscall (getpid) | 3.45 ticks (±0.62 ticks) |
3.45 ticks (±0.58 ticks) |
1.00 |
| Memcpy speed - (built_in) block size 4096 | 84619.14 MByte/s (±58544.89 MByte/s) |
82448.38 MByte/s (±56997.13 MByte/s) |
1.03 |
| Memcpy speed - (built_in) block size 1048576 | 30856.54 MByte/s (±24854.11 MByte/s) |
30585.98 MByte/s (±24707.84 MByte/s) |
1.01 |
| Memcpy speed - (built_in) block size 16777216 | 29142.39 MByte/s (±23925.00 MByte/s) |
26340.06 MByte/s (±21720.96 MByte/s) |
1.11 |
| Memset speed - (built_in) block size 4096 | 84663.78 MByte/s (±58578.19 MByte/s) |
82292.76 MByte/s (±56891.50 MByte/s) |
1.03 |
| Memset speed - (built_in) block size 1048576 | 31571.83 MByte/s (±25285.64 MByte/s) |
31323.85 MByte/s (±25145.86 MByte/s) |
1.01 |
| Memset speed - (built_in) block size 16777216 | 29867.99 MByte/s (±24334.84 MByte/s) |
27104.68 MByte/s (±22209.94 MByte/s) |
1.10 |
| Memcpy speed - (rust) block size 4096 | 77771.03 MByte/s (±54245.34 MByte/s) |
74097.96 MByte/s (±51811.44 MByte/s) |
1.05 |
| Memcpy speed - (rust) block size 1048576 | 30604.58 MByte/s (±24754.87 MByte/s) |
30361.60 MByte/s (±24602.37 MByte/s) |
1.01 |
| Memcpy speed - (rust) block size 16777216 | 29182.69 MByte/s (±23979.22 MByte/s) |
27625.34 MByte/s (±22806.88 MByte/s) |
1.06 |
| Memset speed - (rust) block size 4096 | 78098.19 MByte/s (±54479.73 MByte/s) |
74373.47 MByte/s (±51976.48 MByte/s) |
1.05 |
| Memset speed - (rust) block size 1048576 | 31345.50 MByte/s (±25183.58 MByte/s) |
31110.89 MByte/s (±25033.24 MByte/s) |
1.01 |
| Memset speed - (rust) block size 16777216 | 29940.86 MByte/s (±24417.60 MByte/s) |
28386.93 MByte/s (±23265.03 MByte/s) |
1.05 |
| alloc_benchmarks Build Time | 77.10 s |
74.76 s |
1.03 ❗ |
| alloc_benchmarks File Size | 0.87 MB |
0.87 MB |
1.00 ❗ |
| Allocations - Allocation success | 91.31 % |
91.31 % |
1 |
| Allocations - Deallocation success | 100.00 % |
100.00 % |
1 |
| Allocations - Pre-fail Allocations | 61.44 % |
61.44 % |
1 |
| Allocations - Average Allocation time | 8132.05 Ticks (±153.10 Ticks) |
5860.58 Ticks (±98.43 Ticks) |
1.39 ❗ |
| Allocations - Average Allocation time (no fail) | 8691.22 Ticks (±194.19 Ticks) |
6554.81 Ticks (±92.86 Ticks) |
1.33 ❗ |
| Allocations - Average Deallocation time | 1846.29 Ticks (±415.82 Ticks) |
1805.01 Ticks (±250.35 Ticks) |
1.02 |
| mutex_benchmark Build Time | 76.01 s |
79.82 s |
0.95 ❗ |
| mutex_benchmark File Size | 0.86 MB |
0.86 MB |
1.00 ❗ |
| Mutex Stress Test Average Time per Iteration - 1 Threads | 12.18 ns (±0.43 ns) |
12.10 ns (±0.41 ns) |
1.01 |
| Mutex Stress Test Average Time per Iteration - 2 Threads | 40.10 ns (±2.45 ns) |
40.26 ns (±1.68 ns) |
1.00 |
This comment was automatically generated by workflow using github-action-benchmark.
2c424d5 to
f6c859b
Compare
mkroening
left a comment
There was a problem hiding this comment.
Thanks for the PR! :)
I was wondering why implement drivers for legacy devices instead of USB keyboards (xhci, usb-oxide, embassy-usb). I guess it is because of simplicity.
It would be great to discuss the high-level application-facing API, since that is the hardest to change once merged.
| ## Enables the PS/2 keyboard driver. | ||
| ## | ||
| ## This is only useful on PCs (x86-64). | ||
| keyboard = [] |
There was a problem hiding this comment.
Could you rename this to pc-keyboard? Also, please make sure that the docs don't imply that this is needed for keyboard support in general. Serial-based keyboard support is separate from this. Similar to the framebuffer, please also put a disclaimer that this does not make the kernel use the driver and instead exposes an API for applications that need to be ported.
| #[cfg(not(all(target_arch = "x86_64", feature = "keyboard")))] | ||
| #[hermit_macro::system] | ||
| #[unsafe(no_mangle)] | ||
| pub extern "C" fn sys_read_keyboard() -> u8 { | ||
| 0 | ||
| } |
There was a problem hiding this comment.
Similar to the framebuffer PR, please either merge the function definitions or (preferrably) just remove the symbol when the feature is disabled.
| } | ||
|
|
||
| pub(crate) fn install_handlers(handlers: InterruptHandlerMap) { | ||
| pub(crate) fn install_handlers(#[allow(unused_mut)] mut handlers: InterruptHandlerMap) { |
There was a problem hiding this comment.
Please move the allow to the function level instead of having it inline like this.
| pub(crate) fn install_handlers(#[allow(unused_mut)] mut handlers: InterruptHandlerMap) { | ||
| #[cfg(feature = "keyboard")] | ||
| { | ||
| use crate::arch::kernel::keyboard::get_keyboard_handler; |
There was a problem hiding this comment.
| use crate::arch::kernel::keyboard::get_keyboard_handler; | |
| use crate::arch::kernel::keyboard::get_keyboard_handler; | |
| data_port.write(config); | ||
| cmd_port.write(PS2_CMD_ENABLE_KEYBOARD); | ||
| } | ||
| fn keyboard_handler() { |
There was a problem hiding this comment.
Please move the handler to the top level outside of this function.
| while (cmd_port.read() & PS2_BUFFER_FULL) != 0 { | ||
| let _ = data_port.read(); | ||
| } |
There was a problem hiding this comment.
Why do we discard the device buffer instead of filling our read buffer?
| let mut cmd_port = Port::<u8>::new(PS2_CMD_PORT); | ||
| let mut data_port = Port::<u8>::new(PS2_DATA_PORT); |
There was a problem hiding this comment.
Do you think an abstraction similar to the proposal for BGA would make sense?
struct Ps2;
impl Ps2 {
fn read_data() -> u8;
fn write_data(data: u8);
fn status() -> u8;
fn command(command: u8);
}| /// Pops a scancode from the keyboard buffer, returning None if the buffer is empty. | ||
| pub fn pop_scancode() -> Option<u8> { |
There was a problem hiding this comment.
A scancode can never be zero, right? Returning Option<NonZero<u8>> would be preferable in that case.
| #[cfg(all(target_arch = "x86_64", feature = "keyboard"))] | ||
| #[hermit_macro::system] | ||
| #[unsafe(no_mangle)] | ||
| pub extern "C" fn sys_read_keyboard() -> u8 { | ||
| crate::kernel::keyboard::pop_scancode().unwrap_or(0) | ||
| } |
There was a problem hiding this comment.
I am not too sure about this API. Is the application supposed to busy loop on this and retrieve one event at a time?
What about doing something similar to Linux's event device (evdev) interface (Linux docs)? Reading from /dev/input/event0 would then fill a user buffer with input events and blocks if no events are there unless opened with O_NONBLOCK.
f6c859b to
0a1c568
Compare
0a1c568 to
a7180a7
Compare
This feature adds support for the PS2 legacy keyboard in Qemu x86_64.
Currently it does:
The systemcall returns 0 if the keyboard feature is disabled.
I have only tested this feature with C programs on a Mac using Qemu.