Refactor HID Keyboard Reset support#823
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release/202502 #823 +/- ##
================================================
Coverage ? 2.79%
================================================
Files ? 10
Lines ? 3180
Branches ? 81
================================================
Hits ? 89
Misses ? 3091
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I do have a concern. Consider a more complex system that has multiple keyboards, and therefore multiple SimpleTextInEx protocols Consplitter (SimpleTextInEx , with no device path) With this change, USB KB1, USB KB2 would each have a keypress handler installed to handle CTRL+ALT+DEL, but Consplitter and PS2 KB1 would not have anything registered. Would it be better to move the registration into the console splitter? That way there could be a single place that would handle resets? |
Your understanding is correct - there would be a reset notify linked to each SimpleTextInEx produced by the UefiHidV2. This is consistent (at least semantically) with what would happen today if you were using standard EDK2 drivers instead of this stack, though. The difference is where the hook happens - with existing implementation (in this driver, as well as in the other EDK2 keyboard drivers) the hook happens at the device layer during keystroke processing. This PR changes it to happen at the key registration layer - which is a better point in my view, and more consistent with what I think the right long-term direction is (see second part of my comment below). PS2 KB1 would have it's own reset trigger here: https://github.com/microsoft/mu_basecore/blob/afb0c811c17d4943fde9816e0f2ec9eff9ccd1c0/MdeModulePkg/Bus/Isa/Ps2KeyboardDxe/Ps2KbdCtrller.c#L1352.
It would be much better, IMHO, but would require a fairly coordinated and broad change across EDK2 since all the existing keyboard drivers implement this at the device layer. This PR preps for that by removing the reset logic to a key registration and out of the bowels of the keystroke handler. Eventually I envision this behind a cargo feature gate See also comments here: tianocore/edk2#12038 |
the SimpleTextInputEx protocol, instead of being handled in the keystroke handler.
f2a8137 to
0fad3be
Compare
## Description This PR changes how CTRL-ALT-DEL resets are handled by the HID stack. Previously, they were handled by the lower layer key processing code; but this caused them to be handled at an implementation-defined TPL based on what TPL the input reports are generated at by the HID I/O layer. This change moves the CTRL-ALT-DEL to use the existing SimpleTextInEx key registration infrastructure to handle the reset logic, effectively moving the reset handling up to a higher level of abstraction. This also simplifies disabling this capability behind a feature flag should it be desirable in the future to separate reset logic from the HID stack entirely. - [x] Impacts functionality? - [ ] Impacts security? - [ ] Breaking change? - [ ] Includes tests? - [ ] Includes documentation? ## How This Was Tested Verified CTRL-ALT-DEL resets system from UEFI shell and that reset occurs at TPL_CALLBACK. ## Integration Instructions N/A
## Description This PR changes how CTRL-ALT-DEL resets are handled by the HID stack. Previously, they were handled by the lower layer key processing code; but this caused them to be handled at an implementation-defined TPL based on what TPL the input reports are generated at by the HID I/O layer. This change moves the CTRL-ALT-DEL to use the existing SimpleTextInEx key registration infrastructure to handle the reset logic, effectively moving the reset handling up to a higher level of abstraction. This also simplifies disabling this capability behind a feature flag should it be desirable in the future to separate reset logic from the HID stack entirely. - [x] Impacts functionality? - [ ] Impacts security? - [ ] Breaking change? - [ ] Includes tests? - [ ] Includes documentation? ## How This Was Tested Verified CTRL-ALT-DEL resets system from UEFI shell and that reset occurs at TPL_CALLBACK. ## Integration Instructions N/A
## Description This PR changes how CTRL-ALT-DEL resets are handled by the HID stack. Previously, they were handled by the lower layer key processing code; but this caused them to be handled at an implementation-defined TPL based on what TPL the input reports are generated at by the HID I/O layer. This change moves the CTRL-ALT-DEL to use the existing SimpleTextInEx key registration infrastructure to handle the reset logic, effectively moving the reset handling up to a higher level of abstraction. This also simplifies disabling this capability behind a feature flag should it be desirable in the future to separate reset logic from the HID stack entirely. - [x] Impacts functionality? - [ ] Impacts security? - [ ] Breaking change? - [ ] Includes tests? - [ ] Includes documentation? ## How This Was Tested Verified CTRL-ALT-DEL resets system from UEFI shell and that reset occurs at TPL_CALLBACK. ## Integration Instructions N/A
## Description This PR changes how CTRL-ALT-DEL resets are handled by the HID stack. Previously, they were handled by the lower layer key processing code; but this caused them to be handled at an implementation-defined TPL based on what TPL the input reports are generated at by the HID I/O layer. This change moves the CTRL-ALT-DEL to use the existing SimpleTextInEx key registration infrastructure to handle the reset logic, effectively moving the reset handling up to a higher level of abstraction. This also simplifies disabling this capability behind a feature flag should it be desirable in the future to separate reset logic from the HID stack entirely. - [x] Impacts functionality? - [ ] Impacts security? - [ ] Breaking change? - [ ] Includes tests? - [ ] Includes documentation? ## How This Was Tested Verified CTRL-ALT-DEL resets system from UEFI shell and that reset occurs at TPL_CALLBACK. ## Integration Instructions N/A
Description
This PR changes how CTRL-ALT-DEL resets are handled by the HID stack. Previously, they were handled by the lower layer key processing code; but this caused them to be handled at an implementation-defined TPL based on what TPL the input reports are generated at by the HID I/O layer.
This change moves the CTRL-ALT-DEL to use the existing SimpleTextInEx key registration infrastructure to handle the reset logic, effectively moving the reset handling up to a higher level of abstraction.
This also simplifies disabling this capability behind a feature flag should it be desirable in the future to separate reset logic from the HID stack entirely.
How This Was Tested
Verified CTRL-ALT-DEL resets system from UEFI shell and that reset occurs at TPL_CALLBACK.
Integration Instructions
N/A