Codebase cleanup and portability refactoring #110

AlexAltea · 2018-10-19T02:28:43Z

This patch allows for easier portability of HAXM to other platforms (see #108).
Builds successfully under Windows (32-bit and 64-bit configurations) and MacOS.

Changes

Main changes are:

Normalized casing: s/ASSERT/assert, s/TRUE/true, s/FALSE/false, since it was inconsistent (both uppercase and lowercase variants were used across the codebase).
Normalized integer types: The codebase mixes (u)intN_t and (u)intN. This commit picks the former variant and consistently use it. Unnecessary typedef's to the latter variant are removed.
Fixed missing declarations: Defined helper functions and macros to fix errors caused by undeclared non-standard functions, e.g.: min, max.
Generalized page-related constants: Platform-specific headers, e.g. hax_windows.h defined page-related constants such as page_size and page_shift that are identical in all systems since they are unique to the x86 architecture. They were moved tohax_types.h.
Custom arch/compiler/platform macros: Created unique HAX_ARCH_*, HAX_COMPILER_*, HAX_PLATFORM_* macros: The goal is avoiding dependance in compiler/platform-specific macros throughout the codebase by creating custom macros declared at a single location.
Fixed function prototypes: Changing function() to function(void), fixing -Wstrict-prototypes.

Full information in the messages of the individual commits.

AlexAltea · 2018-10-19T02:38:23Z

I think I've found a bug in core/memory.c:332-344:

#if defined(__MACH__)
#ifdef __x86_64__
            hva = (uint64_t)pmem->kva + (cur_va - pmem->uva);
#else
            hva = (uint64_t)(uint32_t)pmem->kva + (cur_va - pmem->uva);
#endif
#else   // __MACH
#if defined(_WIN64)
            hva = (uint64_t)pmem->kva + (cur_va - pmem->uva);
#else
            hva = 0;
#endif
#endif

Note how Windows (32-bit) sets hva to 0, while ironically MacOS (32-bit! which doesn't even exist) sets it to a non-zero value. Probably these two lines should be swapped. This probably hasn't affected end users since this only triggers in the absence of CONFIG_HAX_EPT2.

If it's a bug, I could easily fix it with this (which show the usefulness of the new HAX_ARCH* macros):

#ifdef HAX_ARCH_X86_64
            hva = (uint64_t)pmem->kva + (cur_va - pmem->uva);
#else
            hva = (uint64_t)(uint32_t)pmem->kva + (cur_va - pmem->uva);
#endif

- Replaced ASSERT-with-assert, TRUE-with-true, FALSE-with-false, since they were inconsistent (both uppercase and lowercase variants were used across the codebase). - Removed uneccesary macros for uppercase variants. Signed-off-by: Alexandro Sanchez Bach <[email protected]>

Changing `function()` to `function(void)`, fixing `-Wstrict-prototypes`. Signed-off-by: Alexandro Sanchez Bach <[email protected]>

The codebase mixes (u)intN_t and (u)intN. This commit picks the former variant and consistently use it. Unnecessary typedef's to the latter variant are removed. Regex: "int([0-9]+)(?![_\d])" -> "int$1_t" (Mostly accurate). Signed-off-by: Alexandro Sanchez Bach <[email protected]>

Platform-specific headers, e.g. hax_windows.h, defined page-related constants such as page_size and page_shift that are identical in all systems since they are unique to the x86 architecture. This commit moves the definitions to hax_types.h, and: - Makes these macros uppercase (as per coding convention). - Adds the `HAX_` prefix (as per coding convention). This is also necessary to prevent clashes with kernel headers. These macros are used in all of `core`. Platform-specific code is still allowed to use platform-specific macros like the ones defined in the WDK. Signed-off-by: Alexandro Sanchez Bach <[email protected]>

- Replaced `1 << PAGE_SHIFT` with `PAGE_SIZE`. - Added macros for undeclared non-standard functions: `min`, `max`. Signed-off-by: Alexandro Sanchez Bach <[email protected]>

- Moved component_index_t definition from hax_types.h to vmx.h, since it's not a type consumed outside core. - Created unique `HAX_ARCH_*`, `HAX_COMPILER_*`, `HAX_PLATFORM_*` macros: The goal is avoiding dependance in compiler/platform-specific macros throughout the codebase by creating custom macros declared at a single location. - Changed some (wrongly assigned as Windows/MacOS-specific) macros into MSVC/Clang-specific macros, e.g. ALIGNED(x) and PACKED. Signed-off-by: Alexandro Sanchez Bach <[email protected]>

raphaelning · 2018-10-19T04:23:56Z

I think I've found a bug in core/memory.c:332-344:

Thanks! Indeed there's a bug that might affect Win32 hosts in the !CONFIG_HAX_EPT2 case (now obsolete), although it's not really in the code you quoted. According to git blame, this bug was introduced long before the CONFIG_HAX_EPT2 change, but somehow it didn't really cause any big problem for our Win32 users...

Anyway, hva = 0 seems to be intended for Win32 due to a limitation on the amount of physical RAM that can be mapped into kernel space. There used to be the following logic at line 348:

#if (defined(__MACH__) || defined(_WIN64))
            if (!hax_core_set_p2m(vm, gpfn, hpfn, hva, rom))
#else
           if (!hax_core_set_p2m(vm, gpfn, hpfn, hva, rom, cur_va))
#endif

But the #else case got deleted by accident.

BTW, HAXM used to support 32-bit Mac.

raphaelning

Regex: "int([0-9]+)(?![_\d])" -> "int$1_t" (Mostly accurate).

Does "mostly" imply that there were exceptions? Just curious.

I'm done with the review. Other than a few picky comments, everything looks good. Thanks!

include/hax_types.h

raphaelning · 2018-10-19T06:37:22Z

include/hax_types.h

+// MSVC
+#ifdef _MSC_VER
+#define HAX_COMPILER_MSVC
+#define PACKED


Apparently MSVC doesn't have a simple equivalent for PACKED. Could you add a FIXME comment for this?

include/hax_types.h

- Throwing compile-time errors if no valid architecture/compiler/platform is found. - Replacing platform-specific macros with the new HAX_ARCH_*/HAX_COMPILER_*/HAX_PLATFORM_* macros. Signed-off-by: Alexandro Sanchez Bach <[email protected]>

AlexAltea · 2018-10-19T10:16:32Z

All fixes applied!

Regex: "int([0-9]+)(?![_\d])" -> "int$1_t" (Mostly accurate).

Does "mostly" imply that there were exceptions? Just curious.

Yeah. The regex rule correctly replaced everything, but it accidentally hit SInt32 since the Visual Studio editor does case-insensitive replacements by default (even in regex mode). Toggling case sensitivity fixes it. Additionally, the rule isn't limited to just intN and uintN but will also replace foointN to foointN_t.

That note in the commit message is a mere reminder of what I did, and also a reminder to double-check if used ever again. :-)

raphaelning

Thanks!

include/hax_types.h

raphaelning · 2018-10-19T10:56:46Z

core/include/vmx.h

        }

-#elif defined(__MACH__)
+#elif defined(HAX_PLATFORM_DARWIN)


I guess this is probably to workaround a compiler issue (MSVC vs. Clang), because the Mac code differs from the Windows code only in an extra pair of parentheses. But since we've finished testing the PR, you can address this in a later patch when you add Linux support.

AlexAltea force-pushed the refactor branch from 1713f3c to 84fd5cc Compare October 19, 2018 03:20

AlexAltea added 6 commits October 18, 2018 20:25

Fixed function prototypes

dc84ce0

Changing `function()` to `function(void)`, fixing `-Wstrict-prototypes`. Signed-off-by: Alexandro Sanchez Bach <[email protected]>

Fixed minor typos and compilation issues

a82e494

- Replaced `1 << PAGE_SHIFT` with `PAGE_SIZE`. - Added macros for undeclared non-standard functions: `min`, `max`. Signed-off-by: Alexandro Sanchez Bach <[email protected]>

AlexAltea force-pushed the refactor branch 3 times, most recently from 0d07ac8 to a479c8a Compare October 19, 2018 03:38

raphaelning reviewed Oct 19, 2018

View reviewed changes

Improved robustness/consistency of new macros

4cef403

- Throwing compile-time errors if no valid architecture/compiler/platform is found. - Replacing platform-specific macros with the new HAX_ARCH_*/HAX_COMPILER_*/HAX_PLATFORM_* macros. Signed-off-by: Alexandro Sanchez Bach <[email protected]>

AlexAltea force-pushed the refactor branch from 2c4be93 to 4cef403 Compare October 19, 2018 10:07

raphaelning approved these changes Oct 19, 2018

View reviewed changes

wcwang merged commit 245bdd6 into intel:master Oct 19, 2018

AlexAltea deleted the refactor branch October 19, 2018 12:20

raphaelning mentioned this pull request Oct 25, 2018

Restructuring codebase #113

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Codebase cleanup and portability refactoring #110

Codebase cleanup and portability refactoring #110

Uh oh!

AlexAltea commented Oct 19, 2018 •

edited

Loading

Uh oh!

AlexAltea commented Oct 19, 2018

Uh oh!

raphaelning commented Oct 19, 2018

Uh oh!

raphaelning left a comment

Uh oh!

Uh oh!

raphaelning Oct 19, 2018

Uh oh!

Uh oh!

Uh oh!

AlexAltea commented Oct 19, 2018 •

edited

Loading

Uh oh!

raphaelning left a comment

Uh oh!

Uh oh!

raphaelning Oct 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Codebase cleanup and portability refactoring #110

Codebase cleanup and portability refactoring #110

Uh oh!

Conversation

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

Uh oh!

Changes

Uh oh!

AlexAltea commented Oct 19, 2018

Uh oh!

raphaelning commented Oct 19, 2018

Uh oh!

raphaelning left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

raphaelning Oct 19, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

raphaelning left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

raphaelning Oct 19, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AlexAltea commented Oct 19, 2018 •

edited

Loading

AlexAltea commented Oct 19, 2018 •

edited

Loading