-
Notifications
You must be signed in to change notification settings - Fork 881
Conversation
af67435
to
b6693c6
Compare
Thanks, Alexandro. Great to see you back with another big PR! Windows and Mac have been and will still be the Intel HAXM team's priority, and realistically, we wouldn't add a third host OS to our support matrix. However, if the open source community wants this feature and can provide enough help, there's no reason for us to stand in the way, not to mention that this is a great chance to improve the overall code quality of the project ;-) We'll accept this PR on these conditions:
What do you think? I have some other tasks in hand at the moment, but I'll try to respond to the major changes you have proposed in the next few days. The minor changes all look fine to me :-) |
Sounds good to me! I will provide support for Linux frontend. Additionally, the planned changes will ensure easier maintenance in the future (e.g. by reducing code duplication in platform-specific code). I will comment on this PR to discuss any further major changes that might affect generic or Windows/macOS-specific code. Once the patch is ready, I will run tests on the three platforms (of course, this implies taking care of QEMU-side patches myself) and let you know once everything is ready. Also on the topic of testing: After completing this PR, it might be a good moment to setup something like Travis CI? This should also reduce maintenance effort. |
Thanks! Indeed. Regarding the first proposed major change:
Agreed. Right now we have three kinds of files in a) Makefiles (e.g. Since Two more questions related to this topic:
I'll comment on the other proposed major changes later. |
Agreed.
Good idea! Agreed.
Agreed. I see that you have started doing this and have chosen The question is what's the best strategy to implement such a ubiquitous change, so as to avoid rebase headaches. Here's what I think:
Please let me know if this is reasonable or if you have better ideas. |
Good idea! I just started reading the Travis CI documentation, and it seems to work best with Linux Makefile projects. Since there's a risk of a common ( |
I didn't considered it yet, but that is much more convenient. For now, we could have each platform-specific build script (Makefile, Xcode-files, VS-files) in their corresponding
Agreed, I'll submit all refactoring-related changes as a separate PR.
We can execute arbitrary scripts on Travis (build scripts run in sudo-enabled virtual machines), so it should be possible as long as their hardware supports it and they virtualize EPT to allow nested virtualization.
Travis can handle Linux, macOS and Windows: |
Include folders give me quite some headaches. There's two issues:
Anyway, this comment is just brainstorming on my side. I still need to give this problem more thought. |
389d262
to
1c550f9
Compare
584c8fb
to
32d1d2e
Compare
@raphaelning Regarding this point, I've noticed there's a |
Ah, great question. Yes, I do know what they are. When I first took over the project, it didn't even have a Visual Studio project ( Later we decided to migrate to WDK 10 and Visual Studio 2015. I did that in two steps:
Step 1 ended up generating all the VS files we see now. Since they are autogenerated, they are much more complicated than the original I believe we can safely delete the old makefiles ( |
I completely agree. Thanks for the really nice summary.
I think
No. And I should take the blame. I created that header but wasn't sure where to put it, and ended up making the wrong decision. |
c02ad5d
to
707237c
Compare
@raphaelning Regarding the 32-bit Linux host issues I found: Now with proper logging I see:
The Intel SDM Vol. 3C claims at 30.4 VM Instruction Error Numbers that this code means:
Any clues as to debug which control field(s) might be invalid? EDIT#1: I've added VMCS dumps for a 32-bit Windows host and a 32-bit Linux host, before and after the following snippet in
These are the files: Does anything look suspicious?
EDIT#2: After diffing the files, aside from many expected differences in -haxm_warning: 400c VMX_EXIT_CONTROLS: 36dff # windows
+haxm_warning: 400c VMX_EXIT_CONTROLS: 236fff # linux |
@AlexAltea I'm afraid, I owe you an apology. Before my last comment (about the race condition in the install script) I did not read the script carefully enough. [ I guess it being late in the evening at my end did not help things. ] After having done a quick test of a0b270b, and still (!!) noticing the permission/ownership issue, I've had another look at the script, and have now come to the conclusion that we've got a "cart before the horse" situation. I'm now suggesting to re-arrange the order of the critical steps into the following order:
With this sequence of events I'm now confident that any attempt of "fixing" the permission/ownership afterwards won't be required. The current order of processing is: 3,1,2 (i.e. load the module first, then set up the group and the 'udev' rule). And that seems to explains (in hindsight) what has been observed up to now. I'm attaching a proposal of a re-worked 'haxm-install.sh' script for your perusal. |
@maronz Thanks a lot for tracking down the issue! I've added your patched file and amended the commit. |
Thanks for doing all the hard work to track this issue down to this simple diff! Apparently, the Linux driver incorrectly sets bit 9 (Host address-space size) and bit 21 (Load IA32_EFER), both which should be 0 for a 32-bit host OS (especially when the host kernel is 32-bit). With this information, it's very easy to identify the code that sets these bits: Lines 1360 to 1367 in 24fdff5
As I've pointed out in the above comment, the solution is to make sure |
@raphaelning Thank you! At first, I was sceptical about Now with 64-bit and 32-bit Linux hosts on the same grounds I'm going to fix the lingering VM/VCPU issues, after some checking I figured out I didn't implement open and release (i.e. close) file operations for them. I've already done it locally, which should fix the issue, but during I'll investigate why this happens and report back. |
The new commits look good to me, but the lingering device nodes issue is somehow still not completely fixed: (Before the test, I rebooted my computer and confirmed that
BTW, we have tested this PR (in its current state) on Windows and Mac without running into any issue, so we're ready to merge it. But if you want to add any Linux-specific patches, we can wait. |
@raphaelning Great! None of the remaining issues require changing core or Windows/Mac-specific code, so the tests won't be needed again. Once everything is ready from my side, I'll rewrite the commit history as:
This seems caused by the inode modification, which also caused the devices to be shown in red with unknown type |
@AlexAltea Sounds good, thanks! My previous comment coincided with 8f59564, but I was testing without this commit. It looks like an important fix though. |
I can confirm that edcd0ef has eradicated lingering VM/vCPU device nodes, and those device nodes now show up with the correct file type ( BTW, in the final patch set, I'd like to see d673697 ("Ensuring all VM/VCPU ioctl exit paths decrement the refcounter") remain a separate commit. |
@AlexAltea Congratulations!! The last commit (edcd0ef) appears to have really done the trick. I've been (silently) following the progress over the course of the day, and even on a 32-bit host the '/dev/hax_vm*/vcpu*' entries are appearing, as a new VM gets started, and disappearing (!!), as a VM run comes to an end. |
@raphaelning Sure thing! I'll insert that one as a separate commit right after "Added support for Linux hosts". If you want, I can submit an identical patch for MacOS in another PR. The only thing pending, is fixing the vm_munmap issue mentioned at #108 (comment). However, since it's commented out, it doesn't cause any problems for users: the only drawback is that we cannot unmap the "HAXM tunnel" page from userland. But at that point our main consumer (QEMU) is already finished, so exiting the process will free that page. With this in mind, I'd say we could proceed to merge this (after rewriting the commit history). Sounds good? Of course, I'll keep providing support for the Linux platform: fixing vm_munmap as well as maintaining the rest of the code will have top priority for me now. |
Some platform-specific functions such as `smp_mb` or `__fls` had no hax_/asm_-prefix, clashing against functions declared in kernel headers. Signed-off-by: Alexandro Sanchez Bach <[email protected]>
All done! |
That would be great, thanks! There's a small problem with the latest PR, though:
As you can see, these files are not executable, so
Could you fix this in a99ba16 and force push? |
Signed-off-by: Alexandro Sanchez Bach <[email protected]>
Signed-off-by: Alexandro Sanchez Bach <[email protected]>
Signed-off-by: Alexandro Sanchez Bach <[email protected]>
Signed-off-by: Alexandro Sanchez Bach <[email protected]>
@raphaelning Fixed! |
Great work! |
Motivation
Although previously (#4) Linux support for HAXM was dismissed due to KVM being around. There's still reasons for having Linux support in this project:
Additionally, we could take the chance to refactor a bit the codebase.
Changes
smp_mb
or__fls
had nohax_
/asm_
-prefix, clashing against functions declared in kernel headers.include/linux/*.h
, sources atlinux/*.c
, alongside Makefile+Kbuild files.Pending
Essential tasks:
Optional tasks that imply major changes to the codebase (needs discussion!):
hax_interface.h
viaHAX_IOCTL(name, code, size)
. Then implement theHAX_IOCTL
macro onhax_interface_{darwin,windows,linux}.h
to make the actual platform-specific ioctl definitions. This will reduce code duplication.