Skip to content

x86: Add Keyboard support for i8042 controller#44

Draft
domnudragota wants to merge 17 commits into
ps2-incrementalfrom
ps2-keyboard-v2
Draft

x86: Add Keyboard support for i8042 controller#44
domnudragota wants to merge 17 commits into
ps2-incrementalfrom
ps2-keyboard-v2

Conversation

@domnudragota

@domnudragota domnudragota commented Sep 1, 2025

Copy link
Copy Markdown

Pull Request Overview

This pull request adds driver support for a keyboard device. It implements the client from the PS/2 controller

Testing Strategy

This pull request was tested by @domnudragota, keyboard can decode button presses + modifiers (no numpad yet)

TODO or Help Wanted

This pull request will need a capsule when porting the Process Console (?) to VGA. This is just the driver itself.

Documentation Updated

  • Updated the relevant files in /docs, or no updates are required.

Formatting

  • Ran make prepush.

This was referenced Sep 1, 2025
@domnudragota
domnudragota marked this pull request as ready for review September 2, 2025 10:59
@domnudragota
domnudragota changed the base branch from ps2-incremental to master September 2, 2025 11:00
@domnudragota
domnudragota changed the base branch from master to ps2-incremental September 2, 2025 11:05
@domnudragota domnudragota changed the title x86: Add Keyboard support for i8046 controller x86: Add Keyboard support for i8042 controller Sep 2, 2025
@domnudragota

Copy link
Copy Markdown
Author

Note: I'll move the ACK/RESEND part from the controller to the keyboard command engine only after I start work on the capsule part

@domnudragota

domnudragota commented Sep 2, 2025

Copy link
Copy Markdown
Author

Note for enqueue_command function and why it's public

  • Used by upcoming keyboard capsule to send short PS/2 sequences

  • Runs in deferred context (from controller's path) . Queues bytes; advances on ACK , retries on RESEND

  • Returns false if queue full or seq too long.

  • Ignores device-only responses when no command is in flight (prevents mis-decoding).

  • No completion callback yet; telemetry counters record ACK/RESEND/drop/send-error.

Comment thread chips/x86_q35/src/keyboard.rs
Comment thread chips/x86_q35/src/keyboard.rs Outdated
// Peek current entry at tail and the next byte to send
let (byte_opt, done) = {
let q = self.cmd_q.borrow();
let e = &q[self.q_tail.get()];

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to use indexing here?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first I though it might be useful, now I realized it can panic on any small slip, should be fixed in 96f6762

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain what this does and if it could actually OOB?

@domnudragota domnudragota Sep 8, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That block just peeks the current command in the ring buffer. q.get(tail) is bounds-checked; it returns None instead of panicking if the index were bad. Because we modulo the indexes and check q_count > 0 first, the index shouldn’t be OOB.
If a bug slipped through, the None path bails out (no panic).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update after d25859b,

the command queue now uses a small Fifo<CmdEntry, N>. We (hopefully) don't index with integers anymore; peek()/peek_mut() return an Option and push()/pop() handle wrap-around internally, so OOB cannot occur. Inside a CmdEntry, we only read bytes[idx] after verifying idx < len (is_done()), keeping that access safe.

Comment thread chips/x86_q35/src/keyboard.rs Outdated
} else {
// Logging
if b >= 0x20 && b != 0x7F {
debug!("ps2-kbd: ASCII '{}'", b as char);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
debug!("ps2-kbd: ASCII '{}'", b as char);

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we at least document these messages somewhere for people in the future ?, they are removed in 96f6762

Comment thread chips/x86_q35/src/keyboard.rs Outdated
if b >= 0x20 && b != 0x7F {
debug!("ps2-kbd: ASCII '{}'", b as char);
} else {
debug!("ps2-kbd: ASCII 0x{:02x}", b);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
debug!("ps2-kbd: ASCII 0x{:02x}", b);

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we at least document these messages somewhere for people in the future ?, they are removed in 96f6762

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by document?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a small comment to let people know they can use some commands to debug the controller + keyboard :D

Comment thread chips/x86_q35/src/keyboard.rs
Comment thread chips/x86_q35/src/keyboard.rs
@domnudragota
domnudragota marked this pull request as draft September 5, 2025 20:20

@alexandruradovici alexandruradovici left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, send it to upstream.

@domnudragota
domnudragota force-pushed the ps2-keyboard-v2 branch 2 times, most recently from 88296c7 to b91c7ad Compare September 12, 2025 09:18
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.

2 participants