Implement virtio-input for keyboard and mouse#122
Implement virtio-input for keyboard and mouse#122Mes0903 wants to merge 8 commits intosysprog21:masterfrom
Conversation
|
@Mes0903 I have started the AI code review. It will take a few minutes to complete. |
|
@Mes0903 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 17 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="virtio-input.c">
<violation number="1" location="virtio-input.c:314">
P2: Handle virtq_avail.idx wraparound in the size check; the current signed subtraction can miss a delta larger than QueueNum after wrap, allowing an invalid queue state to proceed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@Mes0903 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 17 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="window-events.c">
<violation number="1" location="window-events.c:141">
P2: Stale `was_empty` snapshot can cause a missed wakeup. The emptiness check is taken before the event is published, so between the snapshot and the release-store of `head`, the consumer can drain the queue and enter `poll(-1)`. The producer then skips `window_wake_backend()` because `was_empty` was `false`, leaving the new event undelivered until the next timer interrupt.
Since `window_wake_backend()` is a cheap non-blocking pipe write, the simplest fix is to always wake after publishing.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@Mes0903 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 17 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="window-events.c">
<violation number="1" location="window-events.c:147">
P2: Dropping queued input events on overflow can lose key/button release events and leave guest input state stuck.</violation>
</file>
<file name="virtio-input.c">
<violation number="1" location="virtio-input.c:602">
P1: VirtIO spec violation: unrecognized `select` values should yield `size = 0`, not a load fault. The VirtIO spec requires the device to set `size` to zero when the requested config selector is not available. Returning `false` here propagates up as `RV_EXC_LOAD_FAULT`, which would crash a guest probing unsupported selectors.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| * is dropped. | ||
| */ | ||
| if (next == tail) | ||
| return false; |
There was a problem hiding this comment.
P2: Dropping queued input events on overflow can lose key/button release events and leave guest input state stuck.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At window-events.c, line 147:
<comment>Dropping queued input events on overflow can lose key/button release events and leave guest input state stuck.</comment>
<file context>
@@ -0,0 +1,335 @@
+ * is dropped.
+ */
+ if (next == tail)
+ return false;
+
+ window_event_queue[head] = *event;
</file context>
There was a problem hiding this comment.
The current host-side input path uses a bounded single-producer/single-consumer queue between the SDL/main thread and the emulator thread. When the queue is full, newly translated input events are dropped in order to keep the producer non-blocking.
I think this is acceptable for lossy events such as mouse motion or wheel input, but it is more problematic for stateful events such as keyboard keys and mouse buttons. If a press event reaches the guest but the corresponding release event is dropped during queue overflow, the guest may continue to observe that key or button as pressed even though the host has already released it.
A typical failure mode looks like this:
- a key/button press is queued and delivered successfully
- the queue fills up
- the matching release event is dropped
- the guest never sees the release
- the guest input state remains inconsistent until some later event happens to correct it
I checked QEMU before proposing a fix here. QEMU does maintain some keyboard-side state, but that is mainly used for filtering suspicious key-up events and for releasing all pressed keys during console/focus transitions. It does not appear to implement a general overflow-recovery mechanism that reconstructs dropped key/button events after a bounded input queue fills up. In practice, QEMU also accepts lossy behavior in its queued input path rather than trying to repair state after overflow.
Because of that, I do not think this should block the current PR by itself. I added an comment for the tradeoff clearly for now. If it becomes a practical problem we can address it in a follow-up.
A possible future improvement would be to add a separate host-side key/button state tracker, similar in spirit to QEMU’s keyboard state handling, and use it to reconcile guest-visible state after specific transitions. However, doing that cleanly in the current producer/consumer design would add noticeable complexity, so I do not think it is worth folding into this PR right now.
There was a problem hiding this comment.
Thanks for the detailed analysis and context—acknowledging the tradeoff and that it shouldn’t block this PR; we can revisit if it becomes a practical issue.
|
@Mes0903 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 17 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="virtio-input.c">
<violation number="1" location="virtio-input.c:458">
P3: Comment incorrectly states `SEMU_INPUT_PROP_POINTER = 1` — the actual value is `0x00` (it is the bit-position index, not the resulting byte value). This could mislead someone modifying the bitmap logic later.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@Mes0903 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 17 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Makefile">
<violation number="1" location="Makefile:179">
P1: Use the repo’s normalized feature check for `ENABLE_VIRTIOINPUT`; the current literal `== 1` check can skip required objects when users pass `true`/`yes`, while compile-time feature macros still enable virtio-input code.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@Mes0903 I have started the AI code review. It will take a few minutes to complete. |
|
The virtio-input feature added in this branch slightly increases boot time: SDL initialization is attempted on every run, and two additional virtio device objects are initialized even in the headless environment used by the netdev test. As a result, the netdev CI test now times out. The netdev test script runs two network-device tests back to back: on macOS, it exercises both user-mode networking and vmnet, each with a 900-second per-statement Although I previously made optimizations for the headless-mode case, they still do not seem to be sufficient. Therefore, for now, I have added a temporary commit to extend the timeout to 30 minutes, so that we can stay focused on the vinput implementation first. |
|
@Mes0903 I have started the AI code review. It will take a few minutes to complete. |
|
@Mes0903 I have started the AI code review. It will take a few minutes to complete. |
Update guest kernel configuration to enable virtio-input. Co-authored-by: Shengwen Cheng <shengwen1997.tw@gmail.com>
Add virtio-input device emulation for keyboard and mouse and connect it to the SDL window backend so host input can reach guest userspace. Introducing virtio-input is not only a matter of adding another virtio device. Once input comes from an SDL window, the emulator also needs a well defined ownership model for the UI loop and a reliable shutdown path between the window side and the emulator side. There are several requirements that need to be satisfied at the same time. 1. SDL video handling should stay on the process main thread, especially on macOS 2. the emulator loop still needs to keep running while the window loop is alive 3. closing the window must be able to stop the emulator even when all harts are idle and 'semu_run()' is blocked in 'poll(-1)' 4. the same shutdown path should work in normal execution and in 'gdbstub' driven execution Keeping the older execution model and simply attaching a window backend to it would be a poor fit. The previous structure entered 'semu_run()' directly from 'main()'. That worked before a real window backend existed, but once SDL is introduced the main thread needs to be reserved for the UI path. A close request would also be only a shared flag unless it is translated into an event that can wake the blocked poll loop. Without that wakeup path the emulator can stay asleep after the user closes the window, which then prevents clean shutdown. This is not really a free design choice once an SDL window backend is introduced. The official SDL documentation [1] notes that most graphics backends are not thread-safe and that SDL video functions should only be called from the application's main thread. This is especially important on macOS, where creating or driving the SDL window from a helper thread is not a valid model. Therefore, the practical structure is to reserve the original main thread for 'g_window.window_main_loop()' and move 'semu_run()' or 'semu_run_debug()' to a worker thread through 'emu_thread_func()'. This follows the same high level direction used by QEMU when a UI backend needs to keep its event loop on the main thread, while the emulator core continues running in the background. 'main()' now checks whether the selected backend provides 'window_main_loop()'. If it does, the original main thread stays inside 'g_window.window_main_loop()', while 'emu_thread_func()' runs 'semu_run()' or 'semu_run_debug()' on a background worker thread. When the emulator exits it calls 'g_window.window_shutdown()' so the window side can leave its loop and 'pthread_join()' can complete cleanly. The shutdown path is completed with a wake pipe. When multi-hart mode is combined with a backend that uses 'window_main_loop()', 'semu_init()' creates 'wake_fd[0]' and 'wake_fd[1]' and 'window_set_wake_fd()' passes the write end to the backend. 'window_shutdown_sw()' sets the shared close flag and performs a best-effort wake so a blocked SMP 'poll(-1)' returns promptly. Single-hart mode never sleeps in that path, and backends without 'window_main_loop()' do not need the pipe. At the device level, add 'virtio-input.c' and 'virtio-input-codes.h' to implement separate keyboard and mouse devices. The implementation covers the MMIO device state, event queues, configuration space, evdev code tables, event injection helpers, and the device registration needed by the existing virtio-mmio framework. The virtio-input configuration path supports the selectors used by the Linux driver, including device names, serial strings, device IDs, property bitmaps, event bitmaps, and absolute axis information. Common virtio-mmio registers remain aligned 32-bit accesses, while the device specific configuration region at 'Config' accepts byte and halfword accesses so the guest can read 'select', 'subsel', 'size', and related fields in the format expected by the virtio-input specification. This commit also tightens the advertised capabilities so the device describes only what the frontend can really generate. Keyboard 'EV_KEY' bits are derived from the SDL key map in 'window-events.c'. Mouse properties advertise 'INPUT_PROP_POINTER', and the mouse event bitmap includes wheel axes. Bitmap sizes are reported using the minimum byte count needed by the highest populated bit instead of claiming a larger fixed size. Both 'EVENTQ' and 'STATUSQ' are handled. 'EVENTQ' writes keyboard and mouse events into guest supplied buffers and updates the used ring only when events are actually delivered. 'STATUSQ' is drained and acknowledged so guest LED updates can make forward progress and the queue does not stall. LED state is not reflected back to the host keyboard because SDL does not provide a portable API for controlling host keyboard LEDs. On the frontend side, add 'window.h', 'window-sw.c', and 'window-events.c'. 'window.h' defines the backend contract used by the emulator core. 'window-sw.c' initializes SDL, creates the window, keeps the close flag, and falls back to headless mode if SDL cannot provide a usable window. 'window-events.c' translates SDL keyboard, mouse button, mouse motion, and mouse wheel events into Linux input events and terminates each sequence with 'SYN_REPORT'. Repeated SDL keydown events are dropped once 'EV_REP' is advertised so the guest kernel remains the single owner of key repeat timing. Mouse wheel input is translated to 'REL_WHEEL' and 'REL_HWHEEL', and 'SDL_MOUSEWHEEL_FLIPPED' is normalized back to standard evdev direction. At this stage the SDL thread still calls the virtio-input update helpers directly, so 'virtio_input_mutex' protects the shared device state across the emulator thread and the window event thread. A later commit moves that ownership boundary onto the emulator thread. The rest of the system is wired up accordingly. 'device.h' defines 'virtio_input_state_t', the new IRQ lines, and the wake pipe stored in 'emu_state'. 'virtio.h' exposes the common virtio-mmio fields used by the implementation. 'minimal.dts' adds separate keyboard and mouse virtio-mmio nodes at '0x4900000' and '0x5000000' with IRQs '7' and '8'. 'feature.h' and 'Makefile' add 'SEMU_FEATURE_VIRTIOINPUT', gate the build on SDL2 availability, and compile 'window-sw.o' and 'window-events.o' only when virtio-input is enabled. If 'sdl2-config' is missing, virtio-input is disabled automatically rather than breaking other build configurations. '.gitignore' is updated for the extra test artifacts. For testing, add '.ci/test-vinput.sh' as a smoke test that boots the guest, logs in, checks that '/dev/input/event*' exists, and verifies that '/proc/bus/input/devices' reports virtio input devices. Update '.github/actions/setup-semu/action.yml' and '.github/workflows/main.yml' to install SDL2 and run this test on both Linux and macOS. CI uses an offscreen SDL video driver so the device enumeration path is exercised without requiring a visible desktop window. Manual end to end verification is still useful for the actual event delivery path. Run 'make check' so 'semu' opens the SDL window. Inside the guest, run 'hexdump /dev/input/event0'. Then focus the SDL window and type keys, move the mouse, or scroll the wheel. New input records should appear in the hexdump output as the guest receives events. [1]: https://wiki.libsdl.org/SDL3/FAQDevelopment Co-authored-by: Shengwen Cheng <shengwen1997.tw@gmail.com>
The current virtio-input path gives the SDL thread direct access to guest visible virtio-input state. SDL input events are translated in 'window-events.c' and then immediately forwarded to 'virtio_input_update_key()', 'virtio_input_update_mouse_button_state()', 'virtio_input_update_cursor()', and 'virtio_input_update_scroll()'. Those helpers eventually enter 'virtio_queue_event_update()', which updates 'EVENTQ', writes guest buffers, advances the used ring, and sets 'InterruptStatus'. At the same time, the emulator thread checks 'virtio_input_irq_pending()' from 'emu_tick_peripherals()' in order to assert or clear the keyboard and mouse PLIC lines. Since 'emu_tick_peripherals()' runs once per sixty four guest instructions, this check sits in a very hot path. That ownership model creates an awkward tradeoff. If 'virtio_input_irq_pending()' reads 'InterruptStatus' without synchronization, the SDL thread and the emulator thread can race on the same 'virtio_input_state_t'. If the helper takes 'virtio_input_mutex' instead, the hot path pays a mutex lock and unlock cost every time it polls the keyboard and mouse devices. The lock based version keeps the state coherent, but it also makes a very small piece of shared state drag the whole hot path through cross thread synchronization. There are two broad ways to resolve this. One option is to keep the current ownership model and turn 'InterruptStatus' into a dedicated atomic flag while leaving the rest of the device state under 'virtio_input_mutex'. The other is to move the ownership boundary earlier, so the SDL thread stops touching virtio-input device state entirely and only passes host input to the emulator thread. This commit takes the second approach. The new structure introduces a host side backend event queue in 'window.h' and 'window-events.c'. SDL input is still collected and translated on the main thread, but it is now stored as a tagged 'window_event_t' payload in a bounded single producer single consumer ring. The queue carries keyboard, mouse button, mouse motion, and mouse wheel events in a host centric form. It does not update guest memory, virtqueues, or virtio registers directly. 'emu_tick_peripherals()' in 'main.c' now drains that queue on the emulator thread via 'virtio_input_drain_window_events()'. Each queued host event is dispatched there into the existing 'virtio_input_update_*()' helpers, which continue to build the corresponding evdev style event sequences and write them into 'EVENTQ' through 'virtio_queue_event_update()'. With this arrangement, the guest facing virtio-input state stays under emulator thread ownership, and the SDL thread only owns host event collection. This ownership change also simplifies the interrupt pending path. 'virtio_input_irq_pending()' now only checks 'InterruptStatus'. The helper stays in the hot path, but the hot path no longer needs to take 'virtio_input_mutex' just to observe whether a used ring interrupt is pending. The mutex remains in place for descriptor processing, queue updates, status queue draining, MMIO accesses, and other multi field operations inside 'virtio-input.c'. The wake pipe is generalized at the same time. The pipe stored in 'emu_state' is no longer only a window close wakeup path. It is now a backend work wakeup path. After publishing an event, the producer sets a 'wake_pending' gate and only calls 'window_wake_backend()' when that gate transitions false -> true, so a whole drain batch is coalesced into one best-effort pipe write. After draining queued events, 'window_rearm_wake()' clears the gate and rechecks the queue so the consumer does not return to 'poll()' if the producer raced in the gap. This keeps input delivery responsive even when all harts are idle and the emulator is sleeping in 'poll(-1)', without writing to the wake pipe for every single SDL event. At the interface level, 'window.h' now defines the backend event types, the tagged event payload, and the queue related helper declarations. 'window-events.c' contains the queue implementation, SDL event translation, and backend wakeup logic. 'window-sw.c' routes both user close and emulator driven shutdown through the same backend wake path. 'device.h' updates the wake pipe description so it documents backend work in general rather than window close only. This design keeps the split between the window side and the emulator side explicit. The window side owns host input collection. The emulator side owns guest visible virtio-input state and interrupt generation. That separation removes the mutex based interrupt pending check from the hot path and gives the input path the same message passing direction that the window backend already uses for other cross thread work. The existing virtio-input smoke test still passes after this change, and the guest continues to enumerate the keyboard and mouse devices correctly.
The current config read path treats an unsupported value in 'select' as a hard read failure. 'virtio_input_cfg_read()' returns false, and 'virtio_input_reg_read()' propagates that failure when the guest reads 'VIRTIO_INPUT_REG_SIZE'. That makes an ordinary guest probe of an unsupported selector look like an MMIO fault. Instead of seeing a zero-sized config payload, the guest can observe an 'RV_EXC_LOAD_FAULT' when it reads back 'size' after writing an unknown 'select' or 'subsel'. The root cause is that the code mixes two different questions. One is whether the MMIO access itself is valid. The other is whether the requested selector names a config field implemented by this device. The first should decide whether the read fails. The second should only decide what data is returned in 'cfg' and what value appears in 'size'. Fix this by clearing the config payload up front, defaulting 'size' to zero, and treating unknown selectors as a successful read that leaves that zero-sized result in place. Since 'virtio_input_cfg_read()' no longer has a real failure path, make it return 'void' and simplify the 'VIRTIO_INPUT_REG_SIZE' handling accordingly.
Manual testing of virtio-input currently relies on guest-side tools such as 'hexdump /dev/input/eventX'. That confirms that input reaches the guest, but it is not very convenient when checking SDL-side event translation in real time. Mouse motion and wheel activity are especially awkward to inspect from the raw evdev stream. Add a build-time input debug switch for the SDL translation path. When 'ENABLE_INPUT_DEBUG=1' is set in the Makefile, the build defines 'SEMU_INPUT_DEBUG' and enables verbose logging in 'window-events.c'. With the flag enabled, the window backend prints the host input events that are being translated into backend queue entries. The output includes: - keyboard press and release events, including the SDL scancode name and translated Linux input key code - mouse button press and release events, including the host button number and translated Linux input button code - mouse motion events, including x/y coordinates - mouse wheel events, including dx/dy deltas - explicit queue-full drop messages for any input event that is discarded before reaching the emulator thread The logging is emitted at the SDL event translation point, before the event is handed to the emulator thread. This makes manual input testing much easier to interpret than the guest's raw input bytes while still showing exactly what enters the host-side virtio-input path.
Since the pull request has not been finalized yet, the corresponding 'Image' has not been uploaded to the blob. Therefore, this commit is added to allow the CI tests to run. In addition, due to GitHub Actions caching behavior, we found that the 'Image' must be deleted and re-downloaded during testing for the new 'Image' to take effect. To address this, this commit adds an extra step in the script to remove the 'Image' and download it again. This commit will be removed once the code review is completed.
|
@Mes0903 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 18 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="window-events.c">
<violation number="1" location="window-events.c:149">
P1: Dropping all events on queue overflow can lose key/button release edges and leave the guest input state stuck.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
The macOS netdev CI test timed out again. From the log, it appears to be stuck at |
The netdev test script runs two network-device tests back to back: on macOS it exercises both user-mode networking and vmnet, each with a 900-second per-statement expect timeout, and the step can take up to roughly 30 minutes in the worst case. The previous 20-minute limit was not enough to cover both tests when the guest is slow to configure the network interface or complete the ping exchange. The virtio-input feature added in this branch also increases boot time slightly: SDL initialisation is attempted on every run, and two extra virtio device objects are initialised even in the headless environment used by the netdev test. This further tightens the margin against the old deadline.
8e22e1f to
c6ab212
Compare
This pull request is derived from #34 and #121, implements VirtIO-Input keyboard and mouse devices for semu, enabling host keyboard and mouse input to reach the guest through an SDL-backed window path, in conformance with the VirtIO specification v1.3.
VirtIO-Input
VIRTIO_INPUT_CFG_ID_NAMEVIRTIO_INPUT_CFG_ID_SERIALVIRTIO_INPUT_CFG_ID_DEVIDSVIRTIO_INPUT_CFG_PROP_BITSVIRTIO_INPUT_CFG_EV_BITSVIRTIO_INPUT_CFG_ABS_INFOEVENTQandSTATUSQEVENTQis used for keyboard and mouse event deliverySTATUSQis drained and acknowledged so guest LED updates can make forward progress without stalling the queuePrerequisites
Linux
macOS
Running semu
If SDL is available, semu should open a window for input testing.
Basic guest verification
After the guest has booted, verify that the VirtIO-Input devices are present:
ls /dev/input/event* cat /proc/bus/input/devicesThe guest should expose
/dev/input/event*nodes, and/proc/bus/input/devicesshould report VirtIO input devices.Manual end-to-end verification
Launch semu:
If SDL is available, semu should open a window for input testing. With
ENABLE_INPUT_DEBUG=1, semu also prints the host input events translated by the SDL backend, including:In the guest, verify that the VirtIO-Input devices are present:
ls /dev/input/event* cat /proc/bus/input/devicesOptionally inspect the guest event stream directly:
Focus the SDL window and:
You should see the translated host-side events in semu's debug output, and new records should appear in the guest input event stream.
Automated testing
.ci/test-vinput.shas a smoke test/dev/input/event*exists/proc/bus/input/devicesreports VirtIO input devicesSDL_VIDEODRIVER=offscreenin CI so the guest-visible deviceenumeration path can be validated without requiring a visible desktop
window
Summary by cubic
Add
virtio-inputkeyboard and mouse over VirtIO‑MMIO with an SDL-backed window so host input reaches the guest. Host events are queued on the main thread and drained on the emulator thread via an SPSC queue, with a wake pipe for prompt delivery and clean shutdown, including gdbstub and window-close handling.New Features
EV_REP, drop host autorepeat), mouse buttons, absolute pointer, and wheel (REL_WHEEL/REL_HWHEEL).size=0; drainSTATUSQfor LED updates (no host LED control).EV_KEYbitmap from the SDL key map,INPUT_PROP_POINTER, and ABS ranges sized to a 1024×768 window.ENABLE_INPUT_DEBUG=1prints translated events and queue-full drops..ci/test-vinput.shverifies/dev/input/event*and/proc/bus/input/deviceson Linux/macOS.Dependencies
SDL2; ifsdl2-configis missing,virtio-inputis disabled; headless/offscreen path for CI.CONFIG_INPUT_EVDEVandCONFIG_VIRTIO_INPUT; CI installsSDL2and runs.ci/test-vinput.shwithSDL_VIDEODRIVER=offscreen, forcing freshImage/rootfs.cpiodownloads.Written for commit d3a28e5. Summary will update on new commits.