Codebase cleanup and assembly refactoring #64

AlexAltea · 2018-06-19T01:28:56Z

I just cleaned up parts of the codebase, hoping to make an improvement to overall readability. I hope it's not too biased with my own tastes. If so, please feel free to reject any change and I will rewrite the patch cherry-picking only the commits you deem acceptable.

Summary

On e74a4ec:
- Added VMX_EXIT_ prefix to VMX exit reasons enum. Goal is to avoid polluting the global namespace with ambiguous names such as "CPUID_INSTRUCTION" that are confusing without context (e.g. it could refer to the "cpuid" opcode, or the "cpuid" exit reason, or anything else involving that instruction).
- Removed _INSTRUCTION suffix from VMX exit reasons enum. Goal is to keep consistency with exit_* handlers (which don't have such such suffix). It's also less verbose this way.
- Added a couple of missing exit reasons (57-64), for the sake of completeness.
- Renamed exceptions to VECTOR_XX, to remain consistent with the Intel SDM.
- Fixed few English grammar issues.
On 6f693d4:
- Updated the exit handler list and config_t to use designated initializers
- Fixed small typos.
On 5ea877d:
- Replaced platform-specific *.asm files with general NASM files.
- Using NASM macros to make most assembly functions cross-platform (32-bit/64-bit), and to automate the generation of certain functions, via function_* macros.
- Replaced the vmx_error_t with vmx_result_t enum type with VMX_SUCCEED (0), VMX_FAIL_INVALID (1), VMX_FAIL_VALID (2) values. Transforming EFLAGS into these values happens in the assembly functions via the macro vmx_check. The new enum type vmx_error_t contains the error codes for VMX_FAIL_VALID. This change required renaming a couple of variables.
- Replaced vmx_error_t cpu_vmentry_failed(...) to void cpu_vmentry_failed(...) since the return value is useless and always ignored by the caller.
- Renamed load_kernel_ldt to set_kernel_ldt to remain consistent with IDT/GDT setters.
- Moved few VMX-specific function declarations from asm.h to vmx.h.
- Replaced platform-specific wrappers for IA32 assembly functions with generic wrapper.

As always, based on the changes you accept, I'll rewrite the Git history to avoid polluting the repository.

raphaelning · 2018-06-19T08:23:42Z

I hope it's not too biased with my own tastes.

Not at all! On the contrary, I'm really grateful that you took your time to improve the code quality of HAXM. I can see that you spot something wrong and actually went on to fix it--that's really admirable.

I've reviewed the first three patches and I think they are good to go. Please allow me more time on the last one, which is especially exciting because it is another big step toward a unified assembler for all supported platforms.

raphaelning

I guess you haven't tested the new NASM code yet. I have probably overlooked some issues, which you'll run into when you build and debug the code ;-)

raphaelning · 2018-06-19T08:59:59Z

core/ia32.asm

+    %define reg_arg1_32  ecx
+    %define reg_arg1_64  rcx
+    %define reg_arg1     reg_arg1_64
+    %define reg_arg2_16  cx


According to

https://docs.microsoft.com/en-us/cpp/build/overview-of-x64-calling-conventions

arg2 is found in *DX.

raphaelning · 2018-06-19T09:07:31Z

core/ia32.asm

+    %define reg_ret_32   eax
+    %define reg_ret      reg_ret_32
+%elifidn __CONV__, x64_systemv
+    %define reg_arg1_16  si


According to

https://wiki.osdev.org/System_V_ABI#x86-64

arg1 is found in *DI, whereas arg2 is found in *SI.

raphaelning · 2018-06-19T09:52:53Z

core/ia32.asm

+    ret
+
+function __fls
+    bsr eax, ecx


eax => reg_ret_32, ecx => reg_arg1_32

The original implementation initializes the return value to 0 before invoking BSR, which is necessary, because if the input is 0, BSR may not just return 0. See the description of the BSR instruction in Intel SDM Vol. 2A:

If the content source operand is 0, the content of the destination operand is undefined.

raphaelning · 2018-06-19T10:02:30Z

core/ia32.asm

+function __handle_cpuid
+%ifidn __BITS__, 64
+    push rbx
+    mov r8, rcx


rcx => reg_arg1

raphaelning · 2018-06-20T03:23:27Z

core/ia32.asm

+    ret
+%else
+    rdtsc
+    mov [reg_arg2 + qword_struct.lo], eax


reg_arg2 => reg_arg1

raphaelning · 2018-06-20T03:48:05Z

core/ia32.asm

+    mov edx, esi
+    wrmsr
+    push esi
+    push edi


raphaelning · 2018-06-20T05:22:00Z

core/ia32.asm

+
+function get_kernel_rflags
+    pushf
+    pop ax


ax => reg_ret_16

raphaelning · 2018-06-20T05:28:14Z

core/ia32.asm

+    mov rcx, reg_arg1
+    rdmsr
+    shl rdx, 32
+    or rax, rdx


rax => reg_ret

raphaelning · 2018-06-20T05:28:42Z

core/ia32.asm

+%ifidn __BITS__, 64
+    rdtsc
+    shl rdx, 32
+    or rax, rdx


rax => reg_ret

raphaelning · 2018-06-20T05:37:01Z

core/haxlib.vcxproj

    <NASM Include="emulate_ops.asm">
      <FileType>Document</FileType>
    </NASM>
+    <NASM Include="ia32.asm" />


nit: So we don't really need to specify the FileType tag? Then shall we just delete line 181?

Not sure what caused the <FileType> tag to be added, but it's unnecessary. It's deleted now.

AlexAltea · 2018-06-20T10:12:58Z

Sorry for the issues, I was waiting for the other PR to be merged to rebase and review+test the code. There's still a build error under 32-bit configurations caused by mismatching calling conventions. I will fix it and let you know when everything's ready.

raphaelning · 2018-06-20T10:54:35Z

Sure.

There's still an annoying issue under 32-bit configuration regarding calling conventions.

I see. I managed to dig out your old comment about the previous Win32 linking issue. Not sure if it's related to this one:

The problem is that haxlib.vcxproj and IntelHaxm.vcxproj are stdcall by default, and tests.vcxproj is cdecl by default.

As mentioned earlier, the solution would be to explicitly stating the correct calling convention. However, changing tests.vcxproj to stdcall causes an issue with Google Test, and changing haxlib/IntelHaxm to cdecl causes an issue with MASM code.

The only solution left was explicitly stating that these two emulator functions are stdcall, but that's a suboptimal fix. I'm still thinking how to properly deal with this situation.

I also noticed that the new ia32.asm uses the fastcall convention for Win32, whereas emulate_ops.asm (fastop_dispatch()) uses cdecl. How do you choose the calling convention?

raphaelning

I didn't expect you to finish all the NASM porting work with this PR--thanks a lot! Since the cover page still says the PR is "not ready to merge", we haven't started testing it. Today I only reviewed the updated ia32.asm; will finish the rest tomorrow.

raphaelning · 2018-06-25T11:00:54Z

core/ia32.asm

+;
+%ifidn __CONV__, x32_cdecl
+;
+; Although cdecl does not place arguments in registers, we simulate fastcall


This is neat, but I do have a concern--see below.

raphaelning · 2018-06-25T11:10:42Z

core/ia32.asm

+    global %1
+    %1:
+%ifidn __CONV__, x32_cdecl
+    mov reg_arg1, [esp + 0x4]


What if the function has no argument or only one argument? We will waste a couple of instructions, and perhaps access invalid memory (highly unlikely, but I'm not sure if we can rule out the possibility).

How about adding a second parameter to %macro function, for declaring the number of parameters the function takes?

How about adding a second parameter to %macro function, for declaring the number of parameters the function takes?

Agreed. I will do that.

raphaelning · 2018-06-25T11:11:32Z

core/ia32.asm

+%ifidn __BITS__, 64
+    push rbx
+    mov r8, reg_arg1
+    mov rax, [r8 + vcpu_state._rax]


vcpu_state => cpuid_args

raphaelning · 2018-06-26T08:11:42Z

core/vmx_ops.asm

+%endmacro
+
+function asm_vmxon
+    vmxon [reg_arg1]


I think this would require the C function to be declared differently:

vmx_error_t ASMCALL asm_vmxon(paddr_t *vmxon_addr_ptr);

where vmxon_addr_ptr points to the 8-byte buffer that stores the physical address of the VMXON region, i.e. there are two levels of indirection.

This is because the instruction requires an m64 operand:

This operand is always 64 bits and is always in memory.

I think it is designed this way so that one can pass a long (> 32-bit) physical address to it in 32-bit mode. Note that in 32-bit mode, vmxon_addr_ptr itself is 32 bits and fits in reg_arg1, which is ECX.

Since we are renaming the function anyway, changing its C signature shouldn't be a big deal. The new C function will have to be called like this:

paddr_t vmxon_addr = hax_page_pa(cpu_data->vmxon_page); vmx_error_t err = asm_vmxon(&vmxon_addr);

That should make sure vmxon_addr is always stored in memory (on the stack), so we can keep the assembly code simple.

raphaelning · 2018-06-26T08:13:49Z

core/vmx_ops.asm

+    vmx_check
+
+function asm_vmclear
+    vmclear [reg_arg1]


Same here about the m64 operand. We could change the C function signature to:

vmx_error_t ASMCALL asm_vmclear(paddr_t *vmcs_addr_ptr);

and call it like this:

paddr_t vmcs_addr = hax_page_pa(cpu_data->vmcs_page); vmx_error_t err = asm_vmclear(&vmcs_addr);

raphaelning · 2018-06-26T08:20:37Z

core/vmx_ops.asm

+    vmclear [reg_arg1]
+    vmx_check
+
+function asm_vmptrld


Same here about the m64 operand.

raphaelning · 2018-06-26T08:21:00Z

core/vmx_ops.asm

+    vmx_check
+
+function asm_vmptrst
+    vmptrst [reg_arg1]


Same here about the m64 operand.

raphaelning · 2018-06-26T08:28:55Z

core/vmx_ops.asm

+    global %1
+    %1:
+%ifidn __CONV__, x32_cdecl
+    mov reg_arg1, [esp + 0x4]


As I've commented on ia32.asm, we should make this conditional on the number of arguments that the function takes.

raphaelning · 2018-06-26T08:29:40Z

core/vmx_ops.asm

+    ret
+
+function asm_vmxrun
+    ; TODO


I see that you have removed the old __vmxrun implementation. It's also okay if you keep it and address this TODO in a future PR.

This is implemented now.

raphaelning · 2018-06-26T08:30:02Z

core/vmx_ops.asm

+    ret
+
+function asm_invept
+    ; TODO


This is implemented now.

raphaelning · 2018-06-26T09:03:58Z

core/ia32.asm

+    ret
+
+function get_kernel_gdt
+    sgdt [reg_arg1]


nit: Does sgdt not require the operand to be labeled fword ptr like lgdt does?

No, the keyword is not necessary. In fact, fword is not even defined in NASM. See:
https://www.nasm.us/doc/nasmdoci.html

Interesting. But it makes sense, because lgdt, etc. don't really use other (standard) operand sizes, so removing fword ptr doesn't really create any ambiguity.

raphaelning · 2018-06-26T09:04:33Z

core/ia32.asm

+    ret
+
+function get_kernel_idt
+    sidt [reg_arg1]


nit: Is fword ptr not necessary?

No (see comment above).

raphaelning · 2018-06-29T07:54:33Z

darwin/hax_driver/com_intel_hax/asm/vmcs.c

-#include "../../../../include/hax.h"
-#include "../../../../core/include/vcpu.h"
-
-void hax_disable_preemption(preempt_flag *eflags)


The Windows version of this function is defined in windows/hax_wrapper.c and implemented with pure C code. I have no idea why the Mac version is defined in vmcs.c, but we can't just delete it. Instead, let's move it into darwin/..../hax_wrapper.cpp, and I think it's okay to keep the inline assembly.

I just did that: #70. We could merge it now, but then you would have to rebase this entire PR. Let us know what works best for you.

raphaelning · 2018-06-29T07:54:59Z

darwin/hax_driver/com_intel_hax/asm/vmcs.c

-    hax_disable_irq();
-}
-
-void hax_enable_preemption(preempt_flag *eflags)


Same here. Let's move this one into hax_wrapper.cpp as well.

raphaelning · 2018-06-29T08:55:23Z

I wonder if we should split this PR into multiple ones, so we can merge commits early, as soon as they become ready. Right now we have a small PR #69, which is ready to merge but has some conflicts with this PR, at least with commit b67284a (vmx_ops.asm). If we can merge part of this PR first, I think you'll have an easier time rebasing the rest of it.

AlexAltea · 2018-07-02T06:10:25Z

Sorry, last week and part of this week I was/will be mostly AFK for work reasons. Don't worry about merging other PRs first, I don't mind rebasing. Also, I expect to finish this PR next weekend. Thanks for your other comments, I will try to update my patch as soon as possible :-)

AlexAltea · 2018-07-05T02:05:23Z

Thanks for merging the recent PRs, they had really useful stuff! Rebasing this patch was really simple. Sorry for the delays, I don't like keeping PRs idling, but I got overwhelmed by real-life work.

All features intended for this PR are finished now. There's more to come, but that's for another PR since this one is already big enough. Since your last review, I've fixed the issues brought up by your comments and additionally:

On 881c2f9:
- Implemented asm_vmxrun and asm_invept.
On b346cc4:
- Replaced the vmx_error_t with vmx_result_t enum type with VMX_SUCCEED (0), VMX_FAIL_INVALID (1), VMX_FAIL_VALID (2) values. Transforming EFLAGS into these values happens in the assembly functions via the macro vmx_check. The new enum type vmx_error_t contains the error codes for VMX_FAIL_VALID. This change required renaming a couple of variables.
- Replaced vmx_error_t cpu_vmentry_failed(...) to void cpu_vmentry_failed(...) since the return value is #useless and always ignored by the caller.
- Renamed load_kernel_ldt to set_kernel_ldt to remain consistent with IDT/GDT setters.
- Moved few VMX-specific function declarations from asm.h to vmx.h.

This branch build successfully under Windows in Debug/Release and Win32/x64 configurations. I haven't got time to build it under macOS or test the current changes yet.

Once you approve the patch, I'll rewrite the history into three commits:

Renamed exception vectors and VMX exit reasons.
Using designated initializers.
Ported platform-specific assembly sources to NASM.

raphaelning

There's more to come, but that's for another PR since this one is already big enough.

Sounds good, thanks!

This branch build successfully under Windows in Debug/Release and Win32/x64 configurations. I haven't got time to build it under macOS or test the current changes yet.

Cool. If you address my new comments (there aren't many), I think the code will be good enough to be put to the test.

raphaelning · 2018-07-05T06:15:14Z

core/ia32.asm

+%ifidn __CONV__, x32_cdecl
+%if %2 >= 1
+    mov reg_arg1, [esp + 0x4]
+%elif %2 >= 2


With %elif, the condition is equivalent to %2 < 1 && %2 >= 2 which is always false. I'd rewrite this part as below:

%if %2 >= 3 %error "Unsupported number of arguments" %else %if %2 >= 1 mov reg_arg1, [esp + 0x4] %endif %if %2 >= 2 mov reg_arg2, [esp + 0x8] %endif %endif

raphaelning · 2018-07-05T06:19:44Z

core/ia32.asm

+%ifidn __BITS__, 64
+    push rbx
+    mov r8, reg_arg1
+    mov rax, [r8 + cpuid_args._eax]


rax => eax, because cpuid_args_t::eax is declared as uint32_t.

raphaelning · 2018-07-05T06:24:33Z

core/ia32.asm

+    push rbx
+    mov r8, reg_arg1
+    mov rax, [r8 + cpuid_args._eax]
+    mov rcx, [r8 + cpuid_args._ecx]


rcx => ecx

raphaelning · 2018-07-05T06:25:11Z

core/ia32.asm

+    mov rax, [r8 + cpuid_args._eax]
+    mov rcx, [r8 + cpuid_args._ecx]
+    cpuid
+    mov [r8 + cpuid_args._eax], rax


rax => eax

raphaelning · 2018-07-05T06:25:24Z

core/ia32.asm

+    mov rcx, [r8 + cpuid_args._ecx]
+    cpuid
+    mov [r8 + cpuid_args._eax], rax
+    mov [r8 + cpuid_args._ebx], rbx


rbx => ebx

raphaelning · 2018-07-05T06:29:46Z

core/ia32.asm

+    .hi      resd 1
+endstruc
+
+struc vcpu_state


This is unused now, so I'd remove it.

raphaelning · 2018-07-05T06:43:52Z

core/vmx_ops.asm

+%ifidn __CONV__, x32_cdecl
+%if %2 >= 1
+    mov reg_arg1, [esp + 0x4]
+%elif %2 >= 2


We can't use %elif here. I've suggested an alternative in ia32.asm.

raphaelning · 2018-07-05T07:19:53Z

core/cpu.c

+    if (result == VMX_SUCCEED) {
        cpu_data->vmm_flag |= VMXON_HAX;
    } else {
        bool fatal = true;


GitHub doesn't allow me to comment on the unchanged lines below, but the Mac-specific code should not be excluded from the refactoring:

err & VMX_FAIL_INVALID => result == VMX_FAIL_INVALID

__vmptrst() => paddr_t vmcs_addr; asm_vmptrst(&vmcs_addr);

err => result

raphaelning · 2018-07-05T08:01:36Z

core/vmx_ops.asm

+    ; push the state
+    push reg_arg1
+    cmp reg_arg2_16, 1h
+    mov rax, rcx


rcx => reg_arg1

raphaelning · 2018-07-05T09:24:36Z

core/vmx_ops.asm

+    push eax
+    push ebx
+    ; push the state
+    push reg_arg1


Here we need to copy the state pointer to eax as the original 32-bit MASM code used to do:

mov eax, reg_arg1 push eax

Otherwise, since reg_arg1 == ecx != eax, we'll be loading ECX with the wrong data at line 317. And we can't fix that by simply replacing eax with reg_arg1 in lines 317-323, because line 317 will modify ECX and thus reg_arg1.

raphaelning · 2018-07-05T10:09:31Z

Once you approve the patch, I'll rewrite the history into three commits

Yes, that's a good plan. Please feel free to incorporate the miscellaneous changes (spelling, coding style, etc.) into these three at your convenience.

AlexAltea · 2018-07-06T22:40:26Z

All done, patch history is clean now. An overview of every change is updated at #64 (comment).

raphaelning · 2018-07-09T09:12:39Z

Thanks! Clang was able to spot a few more issues when we tried to build this PR on Mac (sorry I still don't know how to comment on unchanged code):

paniced => panicked
- https://github.com/AlexAltea/haxm/blob/10597c3a90c0d41b26d23bcd17a69c83633c7d9e/include/darwin/hax_mac.h#L200
vmx_error_t => vmx_result_t (I'm actually very surprised that VC++ didn't catch this--we should check our compiler settings)
- Multiple occurrences in core/include/cpu.h and core/cpu.c
%lx => %x (because vmx_result_t is essentially an int):

In addition, my colleague @wcwang ran a few tests on Windows, and some of them resulted in a host freeze. He can tell you more about that.

wcwang · 2018-07-09T09:55:51Z

When I launched Debian in QEMU, which guest image is debian_squeeze_i386_desktop.qcow2, the host would be frozen once the launch command was entered. No QEMU window popped up or logs output in command prompt. Similar results occurred on Android virtual device launch. You may try to reproduce this issue on Windows.

raphaelning · 2018-07-09T10:03:08Z

darwin/hax_driver/com_intel_hax/intelhaxm.xcodeproj/project.pbxproj

 		43C9A9E7138DDA93000A1071 /* hax_host.h in Headers */ = {isa = PBXBuildFile; fileRef = 43C9A9E6138DDA93000A1071 /* hax_host.h */; };
 		43F857E013931E75008A93D6 /* com_intel_hax_mem.h in Headers */ = {isa = PBXBuildFile; fileRef = 43F857DE13931E75008A93D6 /* com_intel_hax_mem.h */; };
 		43F857E113931E75008A93D6 /* com_intel_hax_mem.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 43F857DF13931E75008A93D6 /* com_intel_hax_mem.cpp */; };
-		4BCC4E0513FB6729005E4BE4 /* ia32.c in Sources */ = {isa = PBXBuildFile; fileRef = 4BCC4E0213FB6729005E4BE4 /* ia32.c */; };


ia32.asm and vmx_ops.asm haven't been added to the Xcode project. We're trying to do that and see if we can reproduce the host freeze issue on Mac too.

raphaelning · 2018-07-09T10:44:00Z

I was able to fix the compiler errors and add the two new .asm files to Xcode. Here are the patches:
haxm-PR64-mac-build-fix.zip

Still, when loading intelhaxm.kext I get:

(kernel) kxld[com.intel.kext.intelhaxm]: The following symbols are unresolved for this kext:
(kernel) kxld[com.intel.kext.intelhaxm]:        __vmx_vmwrite
(kernel) kxld[com.intel.kext.intelhaxm]:        __vmx_vmwrite_64
(kernel) kxld[com.intel.kext.intelhaxm]:        __vmx_vmwrite_natural
(kernel) kxld[com.intel.kext.intelhaxm]:        _btr
(kernel) kxld[com.intel.kext.intelhaxm]:        _fxrstor
(kernel) kxld[com.intel.kext.intelhaxm]:        _fxsave
(kernel) kxld[com.intel.kext.intelhaxm]:        _ia32_rdmsr
(kernel) kxld[com.intel.kext.intelhaxm]:        _ia32_wrmsr
(kernel) kxld[com.intel.kext.intelhaxm]:        _rdtsc
(kernel) kxld[com.intel.kext.intelhaxm]:        _vmx_vmread
(kernel) kxld[com.intel.kext.intelhaxm]:        _vmx_vmread_64
(kernel) kxld[com.intel.kext.intelhaxm]:        _vmx_vmread_natural
(kernel) Can't load kext com.intel.kext.intelhaxm - link failed.

I looked into a few of them, and they are actually C wrappers around asm_xxx, but only exist for Windows (windows/{i386, amd64}/wrapper.c). We could add a asm/wrapper.c for Mac too, but I guess it would be better to move these wrappers to core/ so as to further reduce code duplication?

AlexAltea · 2018-07-09T17:46:31Z

Thanks for the report, testing and for the macOS-specific patch. I'll update my PR, and try to find and fix the bugs found by @wcwang.

I'm actually very surprised that VC++ didn't catch this--we should check our compiler settings

Seems like a compiler bug in MSVC, see minimal example at: https://godbolt.org/g/KDjcGJ. Other compilers (gcc/clang/icc) either throw warnings/errors for this. Even MSVC throws an error when building as C++. I've reported the issue, but let's see if they even bother with it, as they tend to heavily prioritize C++ over C.

I guess it would be better to move these wrappers to core/ so as to further reduce code duplication?

Will do so.

I'll let you know with another comment once all reported issues are fixed.

raphaelning · 2018-07-10T08:56:59Z

Seems like a compiler bug in MSVC, [...] I've reported the issue, but let's see if they even bother with it, as they tend to heavily prioritize C++ over C.

Thanks for the information! Before they fix this bug, we'll just continue to rely on Clang to detect coding errors.

Regarding the host freeze issue, I wonder how you plan to debug it and narrow it down. We'd like to help, but all we are sure about now is that the bug lies in the third/last commit, which is very big. To make debugging easier, could you consider splitting it further, e.g.:

Replacing the old vmcs.{asm, c} with vmx_ops.asm, vmx_error_t with vmx_result_t.
- With some more effort, this one could be split into 2-3 commits, beginning with the introduction of vmx_result_t and its integration with the old assembly code, before plugging in vmx_ops.asm.
Replacing the old ia32.{asm, c}, segments.{asm, c} with ia32.asm, moving wrapper.c into core/.

Or if you have a better idea, let us know :-)

raphaelning · 2018-07-10T09:20:16Z

BTW:

You can reproduce the host freeze bug on Windows regardless of the guest OS used, or even with no guest OS (i.e. the boot ROM test): qemu-system-x86_64 -accel hax
My colleague @junxiaoc managed to get rid of the host freeze issue by restoring the old vmcs.asm, but the guest still wouldn't boot up. This means we probably have bugs in both vmx_ops.asm and the new ia32.asm. If they are in separate patches, we can deal with them one at a time.

junxiaoc · 2018-07-10T10:53:11Z

We just tried to only pick up ia32.asm(original ia32.asm and segments.asm) related change, qemu could boot up with guest OS.
So freeze issue might be related with vmx_ops or vmx_result_t related changes.

raphaelning

We were able to fix the host freeze bug with a simple change :-) The entire PR seems to be working fine on Windows now, although we are still running more tests to confirm that...

raphaelning · 2018-07-11T08:14:39Z

core/vmx_ops.asm

+endstruc
+
+%macro vmx_check 0
+    pushf


pushf => pushfw

This turns out to be the root cause of the host freeze issue! Although the pushf mnemonic is used by the Intel SDM for the 16-bit (operand size) variant of the instruction, it is still ambiguous, and in this case gets assembled into pushfq by NASM for win64. pushfw, on the other hand, is unambiguous.

raphaelning · 2018-07-11T08:15:19Z

core/ia32.asm

+    ret
+
+function get_kernel_rflags, 0
+    pushf


pushf => pushfw (see my other comment)

AlexAltea · 2018-07-11T09:54:52Z

Thanks a lot for tracking the bug down. I've now applied the pushfw-fix and macOS patch.

We'd like to help, but all we are sure about now is that the bug lies in the third/last commit, which is very big. To make debugging easier, could you consider splitting it further [...]

Sorry I couldn't made it in time, I am mostly AFK until tomorrow. I'll deal with the linking issues mentioned at #64 (comment) then.

raphaelning

Sorry I couldn't made it in time

No worries. There's no real need to split the last commit anymore :-)

We tested the 32-bit code and found one issue so far, which should be easy to fix. Will let you know tomorrow if there's more.

I'll deal with the linking issues mentioned at #64 (comment) then.

Thanks, and please feel free to create a separate commit for it.

raphaelning · 2018-07-11T13:36:25Z

core/ia32.asm

+%else
+    mov ecx, reg_arg1
+    rdmsr
+    mov [reg_arg2 + qword_struct.lo], eax


The Win32 driver causes a BSOD as soon as it's loaded, and this is the culprit: Here, reg_arg2 == edx, but EDX was just overwritten by rdmsr. So we have to save reg_arg2 to another register (e.g. EBX, which itself may need to be saved to the stack first) before calling rdmsr, and use that register as the base address here.

nit: To keep the code portable (e.g. in case we want to support other calling conventions), we could surround cdecl-specific logic with %ifidn __CONV__, x32_cdecl .. %endif

we could surround cdecl-specific logic with %ifidn CONV, x32_cdecl .. %endif

Or perhaps %ifidn reg_arg2, edx .. %endif. Anyway, it's not easy to achieve portability across different calling conventions, since we probably have cdecl-specific logic elsewhere in the code. So it's perfectly fine if you just explain in a comment why we must save reg_arg2.

BTW, I've noticed that line 234, which is equivalent to mov ecx, ecx, isn't optimized out by NASM, even for the Release build, and even if I manually run nasm with -Ox. This isn't a big issue at all, but do you think NASM should be smarter in recognizing this no-op? Or maybe it doesn't want to be too smart, because we could just want to waste some CPU cycles?

but do you think NASM should be smarter in recognizing this no-op? Or maybe it doesn't want to be too smart, because we could just want to waste some CPU cycles?

I believe that's out-of-scope for NASM: it assembles the source as-is and only optimizes code when the instruction mnemonics are ambiguous and allow different variants. E.g. it will prioritize imm8 over imm32 when the constants is in range [0x00, 0xFF]. There's more info at:
https://www.nasm.us/doc/nasmdoc2.html#section-2.1.23

As you suggested, the only real solution is writing calling-convention specific implementations of functions that have side-effects on "specific" GPRs (just asm_rdmsr and asm_wrmsr). There might be more such pseudo-nop's out there, but I don't see them as a big issue either. Fortunately, most functions are simple enough that can be implemented just with the reg_* macros.

I see, thanks! BTW, with asm_rdmsr fixed, the new code seems to work well on Win32, so I think we are very close to getting this PR merged!

raphaelning · 2018-07-11T13:36:36Z

core/ia32.asm

+    mov ecx, reg_arg1
+    rdmsr
+    mov [reg_arg2 + qword_struct.lo], eax
+    mov [reg_arg2 + qword_struct.hi], edx


AlexAltea · 2018-07-14T11:28:31Z

All fixed. I've implemented this in separate commits (c2af1d5 and 62a7eb7) as you requested, but it's worth considering to merge them to the 3rd commit to avoid breaking git-bisect on macOS.

Also, two things worth mentioning:

I've renamed ia32.asm to ia32_ops.asm and added the generic C wrappers at ia32.c. This is to remain consistent with emulate_ops.asm and vmx_ops.asm.
Note: I don't really like this "_ops" suffix. Ideally, I would name such cases: file.s, file.c, file.h with file := {emulate, ia32, vmx}, since I feel that. they all belong together. However, this causes linking issues as both file.s, file.c generate file.o, hence that annoying meaningless suffix.
I've removed hax_enable_irq and hax_disable_irq from macOS' hax_wrapper.cpp file in favor of the generic wrapper at ia32.c, that relies on our NASM implementations rather than macOS kernel function ml_set_interrupts_enabled. I hope this is fine, if not I'll revert this.

raphaelning

However, this causes linking issues as both file.s, file.c generate file.o, hence that annoying meaningless suffix.

I see. Another solution is to move *.S to another directory, e.g. core/asm/, but I'm totally fine with the renaming approach.

I've removed hax_enable_irq and hax_disable_irq from macOS' hax_wrapper.cpp file in favor of the generic wrapper at ia32.c, that relies on our NASM implementations rather than macOS kernel function ml_set_interrupts_enabled. I hope this is fine, if not I'll revert this.

I'd actually prefer calling the macOS kernel function, even though it would create platform-dependent code paths (#ifdef __MACH__), because:

I think we should only resort to assembly when the same functionality cannot be implemented in C or is not already available through a C wrapper.
ml_set_interrupts_enabled() seems to do a little more than just cli/sti (see source code here). The latest PR causes a host freeze on Mac, and I wonder if that's because of this change.

it's worth considering to merge them to the 3rd commit to avoid breaking git-bisect on macOS.

Agreed. Please do that after we get the new commits working. At the moment tests are failing on both Windows and Mac (with different symptoms), and we're trying to debug the issue.

raphaelning · 2018-07-16T03:17:24Z

core/ia32.c

+extern void ASMCALL asm_enable_irq(void);
+extern void ASMCALL asm_disable_irq(void);
+
+extern void asm_btr(uint8 *addr, uint bit);


This is already declared in asm.h (with ASMCALL modifier). Remove?

raphaelning · 2018-07-16T03:17:38Z

core/ia32.c

+extern void ASMCALL asm_disable_irq(void);
+
+extern void asm_btr(uint8 *addr, uint bit);
+extern void asm_bts(uint8 *addr, uint bit);


raphaelning · 2018-07-16T03:21:17Z

core/ia32.c

+extern void asm_btr(uint8 *addr, uint bit);
+extern void asm_bts(uint8 *addr, uint bit);
+
+extern void asm_fxinit(void);


Already declared in asm.h (with ASMCALL modifier). Remove?

raphaelning · 2018-07-16T03:21:28Z

core/ia32.c

+extern void asm_bts(uint8 *addr, uint bit);
+
+extern void asm_fxinit(void);
+extern void asm_fxsave(mword *addr);


raphaelning · 2018-07-16T03:21:39Z

core/ia32.c

+
+extern void asm_fxinit(void);
+extern void asm_fxsave(mword *addr);
+extern void asm_fxrstor(mword *addr);


raphaelning · 2018-07-16T04:11:38Z

core/ia32.c

+}
+
+void _vmx_vmwrite_natural(struct vcpu_t *vcpu, const char *name,
+    component_index_t component,


nit: Align with the first parameter.

raphaelning · 2018-07-16T04:11:46Z

core/ia32.c

+
+void _vmx_vmwrite_natural(struct vcpu_t *vcpu, const char *name,
+    component_index_t component,
+    uint64 source_val)


nit: Same here.

raphaelning · 2018-07-16T04:21:17Z

core/include/vmx.h

+uint64 vmx_vmread_64(struct vcpu_t *vcpu, component_index_t component);
+
+void _vmx_vmwrite(struct vcpu_t *vcpu, const char *name,
+    component_index_t component, mword source_val);


nit: Insert more spaces to align with the first parameter (struct vcpu_t *).

raphaelning · 2018-07-16T04:21:30Z

core/include/vmx.h

+void _vmx_vmwrite(struct vcpu_t *vcpu, const char *name,
+    component_index_t component, mword source_val);
+void _vmx_vmwrite_natural(struct vcpu_t *vcpu, const char *name,
+    component_index_t component, uint64 source_val);


nit: Same here.

raphaelning · 2018-07-16T04:21:38Z

core/include/vmx.h

+void _vmx_vmwrite_natural(struct vcpu_t *vcpu, const char *name,
+    component_index_t component, uint64 source_val);
+void _vmx_vmwrite_64(struct vcpu_t *vcpu, const char *name,
+    component_index_t component, uint64 source_val);


nit: Same here.

AlexAltea · 2018-07-16T08:29:02Z

All fixed.

I'd actually prefer calling the macOS kernel function, even though it would create platform-dependent code paths

I fully agree with your reasoning. I've reverted that change and used #ifndef __MACH__ to remove the generic assembly-based function from macOS.

raphaelning · 2018-07-16T11:07:23Z

Thanks! The new code looks all good, and works well on Windows. However, most Mac tests are broken, resulting in a host kernel panic. We're still trying to root cause the issue. For now, let me post the macOS kernel panic logs, in case you can spot an obvious problem.

So far we've seen two kinds of macOS kernel panic:

a)

*** Panic Report ***
panic(cpu 3 caller 0xffffff8008987d3d): Kernel trap at 0x0000000000000189, type 14=page fault, registers:
CR0: 0x000000008001003b, CR2: 0x0000000000000189, CR3: 0x0000000040a1d055, CR4: 0x00000000003627e0
RAX: 0x0000000000000000, RBX: 0xffffff8044613800, RCX: 0x0000000000000000, RDX: 0x0000000000000663
RSP: 0xffffff913b4839b0, RBP: 0x0000000000000000, RSI: 0x0000000000000018, RDI: 0x0000000000000000
R8:  0x0000000000000000, R9:  0x0000000000000000, R10: 0x0000000000000000, R11: 0x0000000000000000
R12: 0x000000002c100801, R13: 0xffffff913b483a40, R14: 0xffffff802bdd9c00, R15: 0x0000000000000000
RFL: 0x0000000000010086, RIP: 0x0000000000000189, CS:  0x0000000000000008, SS:  0x0000000000000010
Fault CR2: 0x0000000000000189, Error code: 0x0000000000000010, Fault CPU: 0x3, PL: 0, VF: 0

Backtrace (CPU 3), Frame : Return Address
0xffffff913b483460 : 0xffffff800886d166 
0xffffff913b4834b0 : 0xffffff8008995824 
0xffffff913b4834f0 : 0xffffff8008987b10 
0xffffff913b483570 : 0xffffff800881f180 
0xffffff913b483590 : 0xffffff800886cbdc 
0xffffff913b4836c0 : 0xffffff800886c99c 
0xffffff913b483720 : 0xffffff8008987d3d 
0xffffff913b4838a0 : 0xffffff800881f180 
0xffffff913b4838c0 : 0x189 

BSD process name corresponding to current thread: qemu-system-x86_

Mac OS version:
17F77

Kernel version:
Darwin Kernel Version 17.6.0: Tue May  8 15:22:16 PDT 2018; root:xnu-4570.61.1~1/RELEASE_X86_64
Kernel UUID: CC1699EE-71B5-3AF1-AAA3-6DBD16222B92
Kernel slide:     0x0000000008600000
Kernel text base: 0xffffff8008800000
__HIB  text base: 0xffffff8008700000
System model name: MacBookPro14,2 (Mac-CAD6701F7CEA0921)

System uptime in nanoseconds: 35247773639496
last loaded kext at 35152996138307: com.intel.kext.intelhaxm	1 (addr 0xffffff7f8e030000, size 229376)
last unloaded kext at 35152574121238: com.intel.kext.intelhaxm	7.2.0 (addr 0xffffff7f8e030000, size 126976)

b)

*** Panic Report ***
mp_kdp_enter(): 2, 3, 4 TIMED-OUT WAITING FOR NMI-ACK, PROCEEDING
panic(cpu 2 caller 0xffffff801fb87d3d): Kernel trap at 0xffffff801faba11c, type 14=page fault, registers:
CR0: 0x000000008001003b, CR2: 0x0000000000000016, CR3: 0x00000001b34d410b, CR4: 0x00000000003627e0
RAX: 0x0000000000000000, RBX: 0xffffff802029c670, RCX: 0xffffff9156a83900, RDX: 0x0000000000000004
RSP: 0xffffff9156a83820, RBP: 0xffffff9156a838f0, RSI: 0x0000000000000004, RDI: 0xffffff802029c670
R8:  0x0000000000000ff4, R9:  0x0000000000000000, R10: 0xffffc90000022000, R11: 0xffffffff81141e00
R12: 0x0000000000000000, R13: 0x0000000000000000, R14: 0x0000000000000004, R15: 0x0000000000000000
RFL: 0x0000000000010046, RIP: 0xffffff801faba11c, CS:  0x0000000000000008, SS:  0x0000000000000010
Fault CR2: 0x0000000000000016, Error code: 0x0000000000000000, Fault CPU: 0x2, PL: 0, VF: 0

Backtrace (CPU 2), Frame : Return Address
0xffffff9156a832d0 : 0xffffff801fa6d166 
0xffffff9156a83320 : 0xffffff801fb95824 
0xffffff9156a83360 : 0xffffff801fb87b10 
0xffffff9156a833e0 : 0xffffff801fa1f180 
0xffffff9156a83400 : 0xffffff801fa6cbdc 
0xffffff9156a83530 : 0xffffff801fa6c99c 
0xffffff9156a83590 : 0xffffff801fb87d3d 
0xffffff9156a83710 : 0xffffff801fa1f180 
0xffffff9156a83730 : 0xffffff801faba11c 
0xffffff9156a838f0 : 0xffffff801fa78e9a 
0xffffff9156a83940 : 0xffffff802005e456 
0xffffff9156a83960 : 0xffffff7fa55407ba 
No mapping exists for frame pointer
Backtrace terminated-invalid frame pointer 0xffff88005fc03f98
      Kernel Extensions in backtrace:
         com.intel.kext.intelhaxm(1.0)[C0020148-1BDC-365A-8793-D893A66DA7CB]@0xffffff7fa553c000->0xffffff7fa5573fff

BSD process name corresponding to current thread: qemu-system-x86_

Mac OS version:
17F77

Kernel version:
Darwin Kernel Version 17.6.0: Tue May  8 15:22:16 PDT 2018; root:xnu-4570.61.1~1/RELEASE_X86_64
Kernel UUID: CC1699EE-71B5-3AF1-AAA3-6DBD16222B92
Kernel slide:     0x000000001f800000
Kernel text base: 0xffffff801fa00000
__HIB  text base: 0xffffff801f900000
System model name: MacBookPro14,2 (Mac-CAD6701F7CEA0921)

System uptime in nanoseconds: 262242529783
last loaded kext at 253616550554: com.intel.kext.intelhaxm	1 (addr 0xffffff7fa553c000, size 229376)
last unloaded kext at 253108159779: com.intel.kext.intelhaxm	7.2.0 (addr 0xffffff7fa553c000, size 126976)

raphaelning · 2018-07-17T04:26:57Z

The backtrace in crash dump b) gives us some clue. The HAXM function that shows up there is chunk_alloc() (core/chunk.c), whose assembly code immediately follows that of vmx_ops.asm in the disassembly of the KEXT binary (otool -tVj darwin/hax_driver/com_intel_hax/build/Release/intelhaxm.kext/Contents/MacOS/intelhaxm):

_asm_vmxrun:
00000000000045aa	9c 	pushfq
00000000000045ab	41 50 	pushq	%r8
00000000000045ad	41 51 	pushq	%r9
00000000000045af	41 52 	pushq	%r10
00000000000045b1	41 53 	pushq	%r11
00000000000045b3	41 54 	pushq	%r12
00000000000045b5	41 55 	pushq	%r13
00000000000045b7	41 56 	pushq	%r14
00000000000045b9	41 57 	pushq	%r15
00000000000045bb	51 	pushq	%rcx
00000000000045bc	52 	pushq	%rdx
00000000000045bd	56 	pushq	%rsi
00000000000045be	57 	pushq	%rdi
00000000000045bf	55 	pushq	%rbp
00000000000045c0	50 	pushq	%rax
00000000000045c1	53 	pushq	%rbx
00000000000045c2	bb 14 6c 00 00 	movl	$0x6c14, %ebx
00000000000045c7	48 89 e0 	movq	%rsp, %rax
00000000000045ca	48 83 e8 08 	subq	$0x8, %rax
00000000000045ce	0f 79 d8 	vmwriteq	%rax, %rbx
00000000000045d1	5b 	popq	%rbx
00000000000045d2	58 	popq	%rax
00000000000045d3	50 	pushq	%rax
00000000000045d4	53 	pushq	%rbx
00000000000045d5	57 	pushq	%rdi
00000000000045d6	66 83 fe 01 	cmpw	$0x1, %si
00000000000045da	48 89 f8 	movq	%rdi, %rax
00000000000045dd	48 8b 48 08 	movq	0x8(%rax), %rcx
00000000000045e1	48 8b 50 10 	movq	0x10(%rax), %rdx
00000000000045e5	48 8b 58 18 	movq	0x18(%rax), %rbx
00000000000045e9	48 8b 68 28 	movq	0x28(%rax), %rbp
00000000000045ed	48 8b 70 30 	movq	0x30(%rax), %rsi
00000000000045f1	48 8b 78 38 	movq	0x38(%rax), %rdi
00000000000045f5	4c 8b 40 40 	movq	0x40(%rax), %r8
00000000000045f9	4c 8b 48 48 	movq	0x48(%rax), %r9
00000000000045fd	4c 8b 50 50 	movq	0x50(%rax), %r10
0000000000004601	4c 8b 58 58 	movq	0x58(%rax), %r11
0000000000004605	4c 8b 60 60 	movq	0x60(%rax), %r12
0000000000004609	4c 8b 68 68 	movq	0x68(%rax), %r13
000000000000460d	4c 8b 70 70 	movq	0x70(%rax), %r14
0000000000004611	4c 8b 78 78 	movq	0x78(%rax), %r15
0000000000004615	48 8b 00 	movq	qword_struct.lo(%rax), %rax
0000000000004618	74 05 	je	asm_vmxrun.resume
000000000000461a	0f 01 c2 	vmlaunch
000000000000461d	eb 47 	jmp	asm_vmxrun.exit_entry_fail
asm_vmxrun.resume:
000000000000461f	0f 01 c3 	vmresume
0000000000004622	eb 42 	jmp	asm_vmxrun.exit_entry_fail
asm_vmxrun.exit_entry:
0000000000004624	57 	pushq	%rdi
0000000000004625	48 8b 7c 24 08 	movq	0x8(%rsp), %rdi
000000000000462a	48 89 07 	movq	%rax, qword_struct.lo(%rdi)
000000000000462d	48 89 4f 08 	movq	%rcx, 0x8(%rdi)
0000000000004631	48 89 57 10 	movq	%rdx, 0x10(%rdi)
0000000000004635	59 	popq	%rcx
0000000000004636	48 89 5f 18 	movq	%rbx, 0x18(%rdi)
000000000000463a	48 89 6f 28 	movq	%rbp, 0x28(%rdi)
000000000000463e	48 89 77 30 	movq	%rsi, 0x30(%rdi)
0000000000004642	48 89 4f 38 	movq	%rcx, 0x38(%rdi)
0000000000004646	4c 89 47 40 	movq	%r8, 0x40(%rdi)
000000000000464a	4c 89 4f 48 	movq	%r9, 0x48(%rdi)
000000000000464e	4c 89 57 50 	movq	%r10, 0x50(%rdi)
0000000000004652	4c 89 5f 58 	movq	%r11, 0x58(%rdi)
0000000000004656	4c 89 67 60 	movq	%r12, 0x60(%rdi)
000000000000465a	4c 89 6f 68 	movq	%r13, 0x68(%rdi)
000000000000465e	4c 89 77 70 	movq	%r14, 0x70(%rdi)
0000000000004662	4c 89 7f 78 	movq	%r15, 0x78(%rdi)
asm_vmxrun.exit_entry_fail:
0000000000004666	5b 	popq	%rbx
0000000000004667	5b 	popq	%rbx
0000000000004668	58 	popq	%rax
0000000000004669	5d 	popq	%rbp
000000000000466a	5f 	popq	%rdi
000000000000466b	5e 	popq	%rsi
000000000000466c	5a 	popq	%rdx
000000000000466d	59 	popq	%rcx
000000000000466e	41 5f 	popq	%r15
0000000000004670	41 5e 	popq	%r14
0000000000004672	41 5d 	popq	%r13
0000000000004674	41 5c 	popq	%r12
0000000000004676	41 5b 	popq	%r11
0000000000004678	41 5a 	popq	%r10
000000000000467a	41 59 	popq	%r9
000000000000467c	41 58 	popq	%r8
000000000000467e	66 9c 	pushfw
0000000000004680	66 58 	popw	%ax
0000000000004682	66 a9 40 00 	testw	$vcpu_state._r8, %ax
0000000000004686	75 14 	jne	..@31.fail_valid
0000000000004688	66 a9 01 00 	testw	$0x1, %ax
000000000000468c	75 07 	jne	..@31.fail_invalid
000000000000468e	b8 00 00 00 00 	movl	$qword_struct.lo, %eax
0000000000004693	eb 0c 	jmp	..@31.continue
..@31.fail_invalid:
0000000000004695	b8 01 00 00 00 	movl	$0x1, %eax
000000000000469a	eb 05 	jmp	..@31.continue
..@31.fail_valid:
000000000000469c	b8 02 00 00 00 	movl	$0x2, %eax
..@31.continue:
00000000000046a1	9d 	popfq
00000000000046a2	c3 	retq
_vmx_get_rip:
00000000000046a3	48 b8 97 47 00 00 00 00 00 00 	movabsq	$0x4797, %rax
00000000000046ad	c3 	retq
_chunk_alloc:
00000000000046ae	55 	pushq	%rbp
00000000000046af	48 89 e5 	movq	%rsp, %rbp
00000000000046b2	41 57 	pushq	%r15
00000000000046b4	41 56 	pushq	%r14
00000000000046b6	41 55 	pushq	%r13
00000000000046b8	41 54 	pushq	%r12
00000000000046ba	53 	pushq	%rbx
00000000000046bb	50 	pushq	%rax
00000000000046bc	48 89 d3 	movq	%rdx, %rbx
00000000000046bf	49 89 f6 	movq	%rsi, %r14
00000000000046c2	49 89 fc 	movq	%rdi, %r12
00000000000046c5	48 85 db 	testq	%rbx, %rbx
00000000000046c8	74 34 	je	0x46fe
00000000000046ca	66 41 f7 c4 ff 0f 	testw	$0xfff, %r12w
00000000000046d0	74 55 	je	0x4727
00000000000046d2	48 8d 05 4b 49 02 00 	leaq	_default_hax_log_level(%rip), %rax
00000000000046d9	41 bf ea ff ff ff 	movl	$0xffffffea, %r15d
00000000000046df	83 38 04 	cmpl	$0x4, qword_struct.lo(%rax)
00000000000046e2	0f 8f f8 00 00 00 	jg	0x47e0
00000000000046e8	48 8d 3d 93 4b 01 00 	leaq	0x14b93(%rip), %rdi ## literal pool for: "haxm_error: chunk_alloc: base_uva 0x%llx is not page aligned.\n"
00000000000046ef	31 c0 	xorl	%eax, %eax
00000000000046f1	4c 89 e6 	movq	%r12, %rsi
00000000000046f4	e8 00 00 00 00 	callq	_printf
00000000000046f9	e9 e2 00 00 00 	jmp	0x47e0
00000000000046fe	48 8d 05 1f 49 02 00 	leaq	_default_hax_log_level(%rip), %rax
0000000000004705	41 bf ea ff ff ff 	movl	$0xffffffea, %r15d
000000000000470b	83 38 04 	cmpl	$0x4, qword_struct.lo(%rax)
000000000000470e	0f 8f cc 00 00 00 	jg	0x47e0
0000000000004714	48 8d 3d 3f 4b 01 00 	leaq	0x14b3f(%rip), %rdi ## literal pool for: "haxm_error: chunk_alloc: chunk is NULL\n"
000000000000471b	31 c0 	xorl	%eax, %eax
000000000000471d	e8 00 00 00 00 	callq	_printf
0000000000004722	e9 b9 00 00 00 	jmp	0x47e0
0000000000004727	66 41 f7 c6 ff 0f 	testw	$0xfff, %r14w
000000000000472d	74 2c 	je	0x475b
000000000000472f	48 8d 05 ee 48 02 00 	leaq	_default_hax_log_level(%rip), %rax
0000000000004736	41 bf ea ff ff ff 	movl	$0xffffffea, %r15d
000000000000473c	83 38 04 	cmpl	$0x4, qword_struct.lo(%rax)
000000000000473f	0f 8f 9b 00 00 00 	jg	0x47e0
0000000000004745	48 8d 3d 75 4b 01 00 	leaq	0x14b75(%rip), %rdi ## literal pool for: "haxm_error: chunk_alloc: size 0x%llx is not page aligned.\n"
000000000000474c	31 c0 	xorl	%eax, %eax
000000000000474e	4c 89 f6 	movq	%r14, %rsi
0000000000004751	e8 00 00 00 00 	callq	_printf
0000000000004756	e9 85 00 00 00 	jmp	0x47e0
000000000000475b	bf 18 00 00 00 	movl	$vcpu_state._rbx, %edi
0000000000004760	31 f6 	xorl	%esi, %esi
0000000000004762	e8 16 c2 00 00 	callq	_hax_vmalloc
0000000000004767	49 89 c5 	movq	%rax, %r13
000000000000476a	4d 85 ed 	testq	%r13, %r13
000000000000476d	74 4d 	je	0x47bc
000000000000476f	4d 89 65 08 	movq	%r12, 0x8(%r13)
0000000000004773	4d 89 75 10 	movq	%r14, 0x10(%r13)
0000000000004777	4c 89 e7 	movq	%r12, %rdi
000000000000477a	4c 89 f6 	movq	%r14, %rsi
000000000000477d	4c 89 ea 	movq	%r13, %rdx
0000000000004780	e8 95 1b 01 00 	callq	_hax_pin_user_pages
0000000000004785	41 89 c7 	movl	%eax, %r15d
0000000000004788	45 85 ff 	testl	%r15d, %r15d
000000000000478b	74 4d 	je	0x47da
000000000000478d	48 8d 05 90 48 02 00 	leaq	_default_hax_log_level(%rip), %rax
0000000000004794	83 38 04 	cmpl	$0x4, qword_struct.lo(%rax)
0000000000004797	7f 14 	jg	0x47ad
0000000000004799	48 8d 3d 84 4b 01 00 	leaq	0x14b84(%rip), %rdi ## literal pool for: "haxm_error: hax_chunk: pin user pages failed, uva: 0x%llx, size: 0x%llx.\n"
00000000000047a0	31 c0 	xorl	%eax, %eax
00000000000047a2	4c 89 e6 	movq	%r12, %rsi
00000000000047a5	4c 89 f2 	movq	%r14, %rdx
00000000000047a8	e8 00 00 00 00 	callq	_printf
00000000000047ad	be 18 00 00 00 	movl	$vcpu_state._rbx, %esi
00000000000047b2	4c 89 ef 	movq	%r13, %rdi
00000000000047b5	e8 74 c3 00 00 	callq	_hax_vfree
00000000000047ba	eb 24 	jmp	0x47e0
00000000000047bc	48 8d 05 61 48 02 00 	leaq	_default_hax_log_level(%rip), %rax
00000000000047c3	41 bf f4 ff ff ff 	movl	$0xfffffff4, %r15d
00000000000047c9	83 38 04 	cmpl	$0x4, qword_struct.lo(%rax)
00000000000047cc	7f 12 	jg	0x47e0
00000000000047ce	48 8d 3d 27 4b 01 00 	leaq	0x14b27(%rip), %rdi ## literal pool for: "haxm_error: hax_chunk: vmalloc failed.\n"
00000000000047d5	e9 41 ff ff ff 	jmp	0x471b
00000000000047da	4c 89 2b 	movq	%r13, qword_struct.lo(%rbx)
00000000000047dd	45 31 ff 	xorl	%r15d, %r15d
00000000000047e0	44 89 f8 	movl	%r15d, %eax
00000000000047e3	48 83 c4 08 	addq	$0x8, %rsp
00000000000047e7	5b 	popq	%rbx
00000000000047e8	41 5c 	popq	%r12
00000000000047ea	41 5d 	popq	%r13
00000000000047ec	41 5e 	popq	%r14
00000000000047ee	41 5f 	popq	%r15
00000000000047f0	5d 	popq	%rbp
00000000000047f1	c3 	retq

Apparently, the host RIP returned by vmx_get_rip is wrong:

00000000000046a3	48 b8 97 47 00 00 00 00 00 00 	movabsq	$0x4797, %rax

where 0x4797 falls in the middle of chunk_alloc. So the first VM exit would restore the wrong host RIP, eventually leading to a macOS kernel panic.

But the disassembly of vmx_ops.o gives movabsq $asm_vmxrun.exit_entry, %rax, which is correct. So it seems the linker (clang++) fails to update the address of the exit_entry label after putting all the .os together.

We'll try to manually patch the address in the KEXT binary and see if it fixes the problem. But eventually, we need to understand the interface between NASM and the Mac linker and come up with a proper fix.

raphaelning · 2018-07-17T07:34:38Z

The manual address/offset patching method works, so this vmx_get_rip issue is probably the last bug we need to fix :-)

I inspected vmx_ops.o with otool and nm, but did not spot anything abnormal...

If we compare the original (vmcs.{asm, c}) and current implementations of this assembly routine, there are two main differences:

i. The old code uses the lea instruction instead of mov.
ii. The old code declares exit_entry as a global label (MASM EXIT_ENTRY:: rather than EXIT_ENTRY:, GAS EXIT_ENTRY: rather than something like 1b:).

Changing exit_entry to a global label doesn't seem to help. I'm now trying to figure out how to make lea work...

raphaelning · 2018-07-17T09:25:47Z

core/vmx_ops.asm

+%endif
+
+function vmx_get_rip, 0
+    mov reg_ret, asm_vmxrun.exit_entry


=> lea reg_ret, [rel asm_vmxrun.exit_entry]

This is enough to fix the macOS kernel panic! (A lot of credit goes to @junxiaoc )

Basically, the use of lea with the REL keyword causes the address to be computed using RIP-relative addressing. The disassembly looks like this:

0000000000004a62 48 8d 05 7a ff ff ff leaq asm_vmxrun.exit_entry(%rip), %rax

Although rel is specific to 64-bit mode, it seems to be just ignored for the Win32 build, and this lea approach works across all platforms we support. Nevertheless, for readability (especially since mov is more intuitive than lea), we could save the mov approach for 32-bit.

Please feel free to incorporate
haxm-PR64-mac-kernel-panic-fix.zip into your 3rd commit.

@raphaelning @junxiaoc Thank you so much for debugging+fixing the issue, and also for your patience with this PR. :-)

I'll add the patch and squash everything into the 3rd commit (it will be big, but as mentioned earlier it's convenient to avoid breaking git-bisect).

Signed-off-by: Alexandro Sanchez Bach <[email protected]>

raphaelning

Sounds good! We're excited to land this PR :-)

AlexAltea · 2018-07-17T09:56:20Z

All done!

AlexAltea force-pushed the cleanup branch from d4f2973 to 9cbd306 Compare June 19, 2018 01:39

AlexAltea force-pushed the cleanup branch from 9cbd306 to 82fe894 Compare June 19, 2018 10:23

raphaelning suggested changes Jun 20, 2018

View reviewed changes

AlexAltea force-pushed the cleanup branch 2 times, most recently from f2b32d1 to 77ff4b8 Compare June 20, 2018 09:45

AlexAltea force-pushed the cleanup branch from 77ff4b8 to 4f68ef6 Compare June 20, 2018 10:17

AlexAltea force-pushed the cleanup branch 4 times, most recently from 54bf8c3 to b67284a Compare June 22, 2018 10:29

raphaelning reviewed Jun 25, 2018

View reviewed changes

raphaelning suggested changes Jun 26, 2018

View reviewed changes

raphaelning reviewed Jun 29, 2018

View reviewed changes

AlexAltea force-pushed the cleanup branch 4 times, most recently from 400d196 to d555074 Compare July 5, 2018 01:51

AlexAltea force-pushed the cleanup branch from d555074 to b346cc4 Compare July 5, 2018 02:15

raphaelning suggested changes Jul 5, 2018

View reviewed changes

AlexAltea force-pushed the cleanup branch from 48f34c9 to 818fafe Compare July 6, 2018 22:19

AlexAltea added 2 commits July 7, 2018 00:30

Renamed exception vectors and VMX exit reasons

e74a4ec

Using designated initializers

6f693d4

AlexAltea force-pushed the cleanup branch from 818fafe to 10597c3 Compare July 6, 2018 22:32

raphaelning reviewed Jul 9, 2018

View reviewed changes

AlexAltea force-pushed the cleanup branch from 10597c3 to 438db3c Compare July 9, 2018 23:25

raphaelning suggested changes Jul 11, 2018

View reviewed changes

AlexAltea force-pushed the cleanup branch from 438db3c to 6bd9b40 Compare July 11, 2018 09:38

AlexAltea force-pushed the cleanup branch from 6bd9b40 to 4eca5b6 Compare July 11, 2018 10:00

raphaelning suggested changes Jul 11, 2018

View reviewed changes

AlexAltea force-pushed the cleanup branch 2 times, most recently from 9f9da56 to 62a7eb7 Compare July 14, 2018 11:14

raphaelning suggested changes Jul 16, 2018

View reviewed changes

raphaelning suggested changes Jul 17, 2018

View reviewed changes

Ported platform-specific assembly sources to NASM

5ea877d

Signed-off-by: Alexandro Sanchez Bach <[email protected]>

AlexAltea force-pushed the cleanup branch from 5c7cf25 to 5ea877d Compare July 17, 2018 09:55

raphaelning approved these changes Jul 17, 2018

View reviewed changes

raphaelning merged commit fd8de55 into intel:master Jul 17, 2018

AlexAltea deleted the cleanup branch July 19, 2018 14:07

Codebase cleanup and assembly refactoring #64

Codebase cleanup and assembly refactoring #64

Uh oh!

Conversation

AlexAltea commented Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

raphaelning commented Jun 19, 2018

Uh oh!

raphaelning left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexAltea commented Jun 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

raphaelning commented Jun 20, 2018

Uh oh!

raphaelning left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

AlexAltea commented Jun 19, 2018 •

edited

Loading

AlexAltea commented Jun 20, 2018 •

edited

Loading

AlexAltea commented Jul 2, 2018 •

edited

Loading

AlexAltea commented Jul 5, 2018 •

edited

Loading