Implement Windows host support (WHPX) - #90
Open
mrexodia wants to merge 14 commits into
Open
Conversation
mrexodia
force-pushed
the
windows-support
branch
2 times, most recently
from
April 16, 2026 00:02
ffd5e6e to
eecacb6
Compare
Author
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4ec901c72
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
mrexodia
force-pushed
the
windows-support
branch
2 times, most recently
from
April 16, 2026 21:25
da9924d to
a5d828a
Compare
mrexodia
marked this pull request as ready for review
April 16, 2026 22:12
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5d828aa3a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
mrexodia
force-pushed
the
windows-support
branch
2 times, most recently
from
April 22, 2026 20:49
9dd34a8 to
16a68d3
Compare
|
+1 |
Author
|
I rebased, workflows are currently running on my repo: https://github.com/mrexodia/gondolin/actions/runs/26248033650 |
Author
|
https://github.com/mrexodia/gondolin/actions/runs/26251373997 CI is green again! |
mrexodia
force-pushed
the
windows-support
branch
from
July 10, 2026 12:35
d8da2e1 to
a582b6d
Compare
secret-host-suggestions.ts hardcoded a POSIX /tmp fallback that was never updated when the rest of the codebase moved to os.tmpdir() for Windows support, so `--host-secret NAME` (without an explicit @host) threw ENOENT on Windows before the VM even started.
quoteCmdArg caret-escaped characters (including embedded double quotes) inside a quoted argument, which corrupts cmd.exe's own command-line parsing rather than producing a literal character - verified empirically against a real cmd.exe that this silently broke the whole invocation. Also drops the unnecessary `call` prefix when re-exec'ing through cmd.exe: `call`'s own argument re-parsing doesn't respect caret-escaped metacharacters the way a plain `/s`-wrapped command line does, which is what actually made metacharacter escaping (&, |, <, >) work correctly. Arguments containing a literal double quote are now rejected outright rather than silently mis-invoked, since there's no reliable way to deliver one to a .bat/.cmd file's argv through cmd.exe. Adds unit tests plus a real cmd.exe end-to-end test (skipped off Windows) since this logic previously had no coverage at all.
host/src/qemu/img.ts and host/src/sandbox/server-options.ts each independently reimplemented near-identical Windows QEMU-family binary discovery (probe via --version, search ProgramW6432/ProgramFiles install roots, dedupe, pick first responder), differing only in the target binary basename - a fix to the install-path heuristics had to be applied in both places or they'd silently drift apart. Extracts the shared logic into qemu/locate-binary.ts and rewires both call sites, plus the third near-duplicate in the test WHPX probe helper.
…ints Standalone addition so later commits (and a later dedup pass) have a single place to construct a throwaway win32-vs-unix test endpoint, instead of every test file hand-rolling the same platform branch.
selectRngObject() returned null on win32, so every Windows-hosted guest booted with no virtio-rng device at all - a real capability regression vs Linux/macOS hosts, previously undocumented and untested. Uses QEMU's cross-platform rng-builtin backend instead (host getrandom()/BCryptGenRandom, no /dev/urandom-equivalent device file needed, available since QEMU 5.0).
qemuCanInitializeAccel probes WHPX support via synchronous spawnSync with up to a 1.5s timeout, invoked from selectAccel on every sandbox/VM creation. spawnSync blocks the entire Node event loop (not just the calling async context), so the first WHPX probe for a given qemuPath stalls any other in-flight sandbox's I/O in the same host process. Adds an async primeAccelProbeCache that performs the same probe via non-blocking spawn and populates the same cache selectAccel reads from. resolveSandboxServerOptionsAsync (the path SandboxServer.create uses, i.e. the recommended API) awaits it before the synchronous accel selection runs, so the common case never blocks. The synchronous SandboxServer constructor has no async alternative available and keeps today's existing (unworsened) blocking behavior.
The QMP idle-pause monitor was hardcoded to a Unix domain socket (defaultQmpSocketPath, executeQmpCommand, and the buildQemuArgs -qmp arg), unlike every other virtio/network socket in this codebase which already went through the cross-platform LocalEndpoint abstraction. Nothing rejected setting sandbox.qemuIdlePauseMs on Windows, so a user who opted in would hit a broken QMP connection attempt at runtime with no test ever having exercised that path. Reserves a loopback TCP port for the QMP endpoint on Windows (QEMU's -qmp chardev supports tcp:host:port just as well as unix:path) and resolves it lazily since the port reservation needs an async bind/close round trip that can't happen in the constructor - start() awaits it before spawning qemu, guarded by a new `starting` flag so a concurrent start() call can't slip past the child-process check while that's in flight. The one await point is skipped entirely when idle-pause isn't configured, so the common case stays fully synchronous like before. Un-skips and rewrites the "idle pause uses a short QMP socket" test to exercise a real QMP round trip over whichever transport the platform actually uses, instead of being unconditionally skipped on Windows.
skipIfBackendUnavailable skipped the entire suite on win32 regardless of backend - including "qemu", the backend this codebase's Windows support actually uses - citing a "Windows QEMU suite" that doesn't exist anywhere in the repo. VM boot+exec, rootfs-resize, VFS mount parity, and network-egress-mediation-via-httpHooks assertions never ran on Windows for the one backend that's supported there. Now only krun (genuinely unsupported on Windows) is skipped; qemu falls through to the existing shouldSkipVmTests() hardware-accel gate like every other platform.
All ~16 path-traversal/symlink-escape security tests (the guard that stops a sandboxed guest from escaping a --mount-hostfs mount root via symlinks) were unconditionally skipped on win32, on the assumption symlinks always need elevation there - not true under Developer Mode or an elevated process, and this left the guard completely untested wherever it's actually capable of running. Replaces the platform check with a one-time real symlink-creation probe, so the tests run for real wherever they can. Running them for real surfaced one genuine, safe platform behavior difference: Windows' RemoveDirectory (unlike POSIX rmdir) doesn't refuse to operate on a directory symlink/junction, it removes the reparse point itself without following it - equally safe (the real target is never touched) but doesn't throw ENOTDIR. The affected test now asserts the platform-appropriate outcome instead of assuming POSIX semantics everywhere.
The sole test for recoverGitBashPathListSpec mocked runCygpath to
return a hardcoded string instead of exercising real `cygpath -u -p`
output, so a real-world cygpath format difference (spaces, drive
letters, the trailing non-path "ro" token) could silently break the
recovery path while the test kept passing.
Replaces the mock's fixture with a value verified against a real
cygpath.exe invocation (the previous fixture used a path nested under
a typical Git-for-Windows install root, which coincidentally collapses
under cygpath's own POSIX root mount - a misleading, environment-
dependent "realistic-looking" value that wasn't actually representative).
Adds a real end-to-end test exercising defaultRunCygpath's actual
execFileSync("cygpath", ...) call, gated on cygpath being available.
…elper Six test files each hand-rolled their own win32-vs-unix endpoint constructor with slightly different literal values (fixed vs ephemeral ports, different path construction), instead of using the shared makeTestEndpoint helper - a future change to Windows loopback- endpoint defaults would need to be found and updated in six places, and missing one would leave that file silently asserting stale transport semantics.
…less of host OS Used the native `path` module (path.join/path.delimiter/path.extname), which follows the actual host OS's conventions rather than Windows'. deps.platform lets callers simulate win32 path resolution from a non-Windows host (exactly what the new unit tests do), but on a real Linux/macOS CI runner path.delimiter is ":" and path.join uses "/" - so a simulated "PATH=C:\a;C:\tools" never split correctly and the .cmd-shim-on-PATH test failed for real on Linux CI. Switches to path.win32 explicitly, since this function's entire purpose is resolving a Windows PATH/PATHEXT lookup irrespective of what platform happens to be running the code.
|
Thank you for your efforts, I hope this gets reviewed and merged next round or two, maybe pi is tying their hands up, or they need a ping. I was testing things out, I am using QEMU emulator version 11.1.0 (v11.1.0-12130-ge470268ff4) with intel i7-7700hq on a laptop. I ran into problems with the test that determines if whpx will work or not, but my experiments show that qemu will still run with accel whpx if you bypass it. D:\qemu>qemu-system-x86_64 -accel whpx -machine none -nodefaults -display none -S Just prints out: DetailsI attached a full -trace enable=* log of the command. trace log excerpt:Then exits with errorlevel 3: But I changed: To: dmesg output:Don't know if there is a good way to tell if it is actually very accelerated but I am getting on cpuid leaf 0x40000000 Where if there is no accel paramter specified or when using TCG I get TCGTCGTCGTCG They both seem to take the same cpu usage. Additional things I experimented with:
I did modify controller.js to use cpu=max / cpu=host and they timed out and had double cpu usage. Gave up on that. Other things I tried were kernel-irqchip=on, and stuff like config.append + " idle=halt nohz=on clocksource=tsc" and other things did not seem to reduce usage or at least significantly. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #21
Session: https://pi.dev/session/#009bb13df5408d6e7dff80631eb6df69
Review: https://pi.dev/session/#79f60c5f2455ebc93d66e3a31b0febbd
Manual testing report (this is what I ran myself to confirm things work): https://gist.github.com/mrexodia/b3261b1a5d05b37a7834d36f997ded74
Currently working on getting the GitHub Actions to actually pass (also tests Windows), might take a little bit...
Happy to do some more work to clean this up into separate commits if you are willing to merge this in principle. I glanced over the code and most things looked reasonable, but I only focused on getting it to actually work for now.
Contribution Agreement
Please ensure this PR follows the guidelines in
CONTRIBUTING.md. (NOTE: this file does not exist in the repo)By submitting this pull request, I confirm the following:
I understand that the entity Earendil Inc. (incorporated in the state of Delaware in 2025) needs some rights from me in order to utilize my contributions in this PR. As a contributor I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Earendil Inc. can use, modify, copy, and redistribute my contributions, under Earendil Inc.'s choice of terms.