Skip to content

Add basic PS/2 keyboard.#211

Merged
jordanhendricks merged 4 commits intooxidecomputer:masterfrom
jordanhendricks:jhendricks/keyboard-pr
Sep 22, 2022
Merged

Add basic PS/2 keyboard.#211
jordanhendricks merged 4 commits intooxidecomputer:masterfrom
jordanhendricks:jhendricks/keyboard-pr

Conversation

@jordanhendricks
Copy link
Contributor

@jordanhendricks jordanhendricks commented Sep 17, 2022

This PR implements basic support for a PS/2 keyboard.

Keyboard Input

Input keyboard data comes from the VNC server in the form of a keysym. The VNC server has a reference to the controller, which has a method to pass input key data (key_event); this part of the emulation emulates the keyboard device itself. The keyboard/controller emulation translates the keysym into an appropriate scan code based on what the guest has configured (either scan code set 1 or scan code set 2), then it writes the scan code to an internal keyboard buffer, and notifies the guest via interrupt that keyboard data is available.

Translating from keysyms to scan codes

Other keyboard device emulation implementations I looked at use giant arrays as tables to map keysyms and scan code sets. Personally, I found reading these tables to be a bit of a challenge for humans, though it might be easier to write programs in. For example, indexing into a table based on an ASCII value (ASCII values are the same as their keysym value) is easy for computers, but not easy to do as a human trying to make sense of the table. Making giant global arrays that provide more type safety is also more challenging to do in Rust than in C.

I also found that other emulations sometimes divorced the translation of keysym to each scan code set. For example, the C bhvye implementation will translate keysyms to scan code set 2 first, then translate to set code 1, which I found confusing to read and reason about. I think the idea here is that scan code set 2 is what real hardware generally sends, while translating to set 1 is for backwards compatability. This more closely matches the architecture of what "real" hardware does, but as far as I can tell it doesn't offer much in terms of making the code easier to reason about. It's possible there's a performance benefit there, but I wanted to simplify my approach unless proven otherwise.

As such, I had the following goals in mind when thinking about how to structure the translation bits:

  • As much as possible, we want the compiler to force us to have a scan code mapping for every new keysym.
  • Because codes from either set 1 or set 2 can be requested, there should be a clean way to get either scan code from each keysym.
  • The translation needs to be debuggable in case of errors or variations in what clients send (e.g., from foreign keyboards).

To address these, I did the following:

  • Keysyms are represented as an enum, so that we can ensure code that translates for them matches all possible patterns. Unknown keys are explicitly ignored in match statements, instead of caught in a catch-all match case.
  • If translation is successful, a single keyevent representation structure, KeyEventRep, unifies all possible values for the keysym. Translations are performed in a giant try_from implementation that unifies all the mappings in a single, more human-readable place. The unified translation struct still allows us flexibility to change our approach if we decide there's a good reason to split up the translation further, like C bhyve does.

Unknown keys

The set of keysyms is much larger than the set of scan codes, meaning that not all keysyms may map cleanly to a scan code set. If we can't map a key to a scan code, we drop the key at the keyboard controller level and log a trace-level error. It's important that unknown keys do not cause any fatal errors.

Debugging keyboard behavior with dtrace probes

Testing this PR for every possible keysym / scan code mapping for this PR is a daunting prospect, so I thought a bit about how we might be able to potential issues with this code not caught in testing, to build confidence we can fix issues down the road as needed.

One possibility is that a user notices a key not behaving as expected. In that case, we would want access to the underlying keysym, the scan code we think it should be, and which scan code set it's from. For several reasons, the biggest of which is to not accidentally create a keylogger, we don't want to be logging keyevents. But I added a dtrace probe in the input path of the controller to observe these details. Anyone with access to the dtrace probe should already have sufficient privileges to snoop on keystrokes if they wish, so I think this is safe.

Keyevent probe

Here's an example use of the probe after pressing the A key in a vnc client connected to a running probe instance, then shift + A. The probe fires twice for each key press, with the make code and break code:

$ pfexec dtrace -n '_ZN8propolis2hw7ps2ctrl7PS2Ctrl8keyevent17h35fae73b22259f30E:ps2ctrl_keyevent { printf("keysym=0x%x, set=%d, s1=0x%x, s2=0x%x, s3=0x%x, s4=0x%x", arg0, arg1, arg2, arg3, arg4, arg5); }'

# A key pressed
  3     46 _ZN8propolis2hw7ps2ctrl7PS2Ctrl8keyevent17h35fae73b22259f30E:ps2ctrl_keyevent keysym=0x61, set=1, s1=0x1e, s2=0x0, s3=0x0, s4=0x0
  3     46 _ZN8propolis2hw7ps2ctrl7PS2Ctrl8keyevent17h35fae73b22259f30E:ps2ctrl_keyevent keysym=0x61, set=1, s1=0x9e, s2=0x0, s3=0x0, s4=0x0

# left shift + A pressed (left shift make + A make + A break + left shift break)
 12     46 _ZN8propolis2hw7ps2ctrl7PS2Ctrl8keyevent17h35fae73b22259f30E:ps2ctrl_keyevent keysym=0xffe2, set=1, s1=0x36, s2=0x0, s3=0x0, s4=0x0
 12     46 _ZN8propolis2hw7ps2ctrl7PS2Ctrl8keyevent17h35fae73b22259f30E:ps2ctrl_keyevent keysym=0x41, set=1, s1=0x1e, s2=0x0, s3=0x0, s4=0x0
 12     46 _ZN8propolis2hw7ps2ctrl7PS2Ctrl8keyevent17h35fae73b22259f30E:ps2ctrl_keyevent keysym=0x41, set=1, s1=0x9e, s2=0x0, s3=0x0, s4=0x0
 12     46 _ZN8propolis2hw7ps2ctrl7PS2Ctrl8keyevent17h35fae73b22259f30E:ps2ctrl_keyevent keysym=0xffe2, set=1, s1=0xb6, s2=0x0, s3=0x0, s4=0x0

Dropped key probe

Another possibility is that a given key that isn't working is not recognized by the controller and is being dropped internally. I also added a probe for these cases.

For example, the Menu key is mapped for USB keyboards, but is not present in the scan code sets. When I press the Menu key, the probe fires, with the underlying keysym value available for debugging:

$ pfexec dtrace -n '_ZN8propolis2hw7ps2ctrl7PS2Ctrl8keyevent17h35fae73b22259f30E:ps2ctrl_keyevent_dropped { printf("keysym=0x%x, is_pressed=%d, set=%d", arg0, arg1, arg2) }'
dtrace: description '_ZN8propolis2hw7ps2ctrl7PS2Ctrl8keyevent17h35fae73b22259f30E:ps2ctrl_keyevent_dropped ' matched 1 probe
CPU     ID                    FUNCTION:NAME
  8     24 _ZN8propolis2hw7ps2ctrl7PS2Ctrl8keyevent17h35fae73b22259f30E:ps2ctrl_keyevent_dropped keysym=0xff67, is_pressed=1, set=1
  8     24 _ZN8propolis2hw7ps2ctrl7PS2Ctrl8keyevent17h35fae73b22259f30E:ps2ctrl_keyevent_dropped keysym=0xff67, is_pressed=0, set=1

Testing

I tested this PR by trying every keyboard combination I could think of on my own keyboard, including combinations with the num lock, caps lock, and shift keys on. This only exercised scan code set 1, as both VMs I tested with (alpine linux and windows) requested scan code set 1 codes.

I confirmed that the windows key on a windows VM produces a windows menu.

Some gaps in testing include:

  • Scan Code Set 2 is completely untested, as neither VM I used asked for it
  • foreign keyboard support

Notes for reviewers

  1. Let me know if there are more tests you'd like to see.
  2. I think it might also be used to have a counter for how many keys we've dropped, as the probe will only show us it happening live. We might want to answer the question of how many keys we've dropped since the guest has existed, to get a sense of whether it's happening a lot.
  3. I added some more comments to the controller and to the new keyboard code, but if there's more documentation you'd like to see, let me know. I've been deep enough in keyboard-land for awhile that I've forgotten what it's like to approach these topics with fresh eyes.

propolis-client = { path = "../../lib/propolis-client" }
propolis-server-config = { path = "../../crates/propolis-server-config" }
rfb = { git = "https://github.com/oxidecomputer/rfb", branch = "main" }
#rfb = { git = "https://github.com/oxidecomputer/rfb", branch = "main" }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: will remove these (and other) local rfb deps when the associated rfb PR (oxidecomputer/rfb#8) is merged

Copy link
Contributor

@pfmooney pfmooney left a comment

Choose a reason for hiding this comment

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

Thanks for adding so much in the way of docs


use super::keyboard::KeyEventRep;

/// PS/2 Controller (Intel 8042) Emulation
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding a nice big theory statement

&& !state.ctrl_cfg.contains(CtrlCfg::PRI_CLOCK_DIS)
&& state.pri_port.has_output(),
);
if state.ctrl_cfg.contains(CtrlCfg::PRI_INTR_EN)
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that this is no longer updating the level of the interrupt pin, but rather pulsing it events, maybe change the function name to better reflect its usage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we chatted about this offline -- update might be misleading, but I can't think of a better name either. punting for now


const PS2K_TYPEMATIC_MASK: u8 = 0x7f;

// XXX(JPH): Is there a reason this needs to be this size?
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I just picked an arbitrary for this when I scraped together the initial PS/2 emulation

pub struct CtrlOutPort: u8 {
// bit 0: system reset
// should always be set to 1
// XXX(JPH): why does this work
Copy link
Contributor

Choose a reason for hiding this comment

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

We are doing none of the work to emulate A20. Its function is a big lie if a guest actually tries to use it.

Copy link
Contributor

@luqmana luqmana left a comment

Choose a reason for hiding this comment

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

Woo! Love all the comments. There were a couple scan codes I think got mixed up?

@jordanhendricks
Copy link
Contributor Author

587c21e addresses most of the review feedback so far.

@jordanhendricks
Copy link
Contributor Author

Update: I had some merge issues with 587c21e and not all the changes I made made it into the commit. Will post another commit.

Co-authored-by: Luqman Aden <me@luqman.ca>
@jordanhendricks
Copy link
Contributor Author

okay, I posted a new commit -- c92f324 addresses most of the review feedback

@jordanhendricks
Copy link
Contributor Author

jordanhendricks commented Sep 22, 2022

@luqmana and I discussed how this PR does not implement some of the quirks for certain keys that change in behavior based on whether other keys are pressed (such as Insert or Delete). As this will take some rework to handle, and I intended this PR to be a baseline level of support for the keyboard, with the idea that we can fix it up incrementally, I filed #213 to track the gaps, and would like to move forward with merging this PR as is.

@jordanhendricks jordanhendricks merged commit ee23701 into oxidecomputer:master Sep 22, 2022
@jordanhendricks jordanhendricks deleted the jhendricks/keyboard-pr branch September 22, 2022 16:53
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