Skip to content
This repository was archived by the owner on Jan 28, 2023. It is now read-only.

Conversation

@AlexAltea
Copy link
Contributor

@AlexAltea AlexAltea commented Nov 6, 2018

As discussed in #108 (comment), there's few improvements that seemed to me a step in the right direction.
Let me know your thoughts on the matter.

Changes

  • Refactored VMCS dumping code: The previous implementation of dump_vmcs obtained the VMCS component name by hashing its value with a gperf-generated perfect hash function at vmcs_names.c. These compact hashes were used as indices in a manually-initialized table of names. This results in lots of boilerplate code, manual initialization/deinitialization and a dependency in gperf.

    This new approach uses a switch/case to dispatch the component names and compactifies the component encoding with the macro HASH to "encourage" the compiler to implement the switch/case via jump tables. The approach works in GCC, Clang, MSVC (check Godbolt snippet). Advantages:

    • Less code.
    • No external dependencies.
    • No memory allocated at runtime.
    • Better performance: No need to query an extra table to compute the perfect hash.
    • Perfect hashing tested at build-time: If the hash macro maps two components to the same value, the file won't compile.
  • Centralized all dumping-related functions in dump.c (renamed from dump_vmcs.c). Not sure if this is the best location, but it probably is better than having inlined snippets (that are either commented out or disabled via preprocessor). More functions could be added to this file, e.g. dumping segment selectors.

  • Created name.c to handle all enum-to-string conversions via functions prefixed with name_. For now I've handled just VMX error and exit codes (aside from the previous VMCS components), but more codes could be added there if required.

  • Improved old logging messages: It looks as if certain log messages (in particular hax_debug/hax_log ones), had been written long time ago and thus do not fully comply with most of the codebase. Changes done:

    • Removed unnecessary "HAX: " prefix from certain calls to log functions. This was most likely a leftover from older HAXM revisions, since nowadays log functions add automatically the corresponding prefix.
    • Promoted some hax_debug/hax_log to hax_error in the case of actual errors, e.g. in cpu_vmentry_failed.

@raphaelning Is there any reason to preserve hax_log functions as-is instead of promoting them directly to hax_info? At the moment they are macro'ed to hax_info: Was this intentional, e.g. to change the macro during as a "debugging hack during development", or can we just remove the macro?

If you think we could get rid of this macro, I can amend the last commit and force-push the change.

The previous implementation of `dump_vmcs` obtained the VMCS component name by hashing its value with a gperf-generated perfect hash function at vmcs_names.c. These compact hashes were used as indices in a manually-initialized table of names. This results in lots of boilerplate code, manual initialization/deinitialization and a dependency in gperf.

This new approach uses a switch/case to dispatch the component names and compactifies the component encoding with the macro `HASH` to encourage the compiler to implement the switch/case via jump tables. The approach works in GCC, Clang, MSVC. Advantages:

- Less code.
- No external dependencies.
- No memory allocated at runtime.
- Better performance: No need to query an extra table to compute the perfect hash.
- Perfect hashing tested at build-time: If the hash macro maps two components to the same value, the file won't compile.

Signed-off-by: Alexandro Sanchez Bach <asanchez@kryptoslogic.com>
- Renamed `dump_vcms.*` to `dump.*` and moved other dump-related functions/snippets there.

- Removed unnecessary references to haxm-windows headers inside the haxm-core project.

Signed-off-by: Alexandro Sanchez Bach <asanchez@kryptoslogic.com>
Signed-off-by: Alexandro Sanchez Bach <asanchez@kryptoslogic.com>
- Removed unnecessary "HAX: " prefix from certain calls to log functions. This was most likely a leftover from older HAXM revisions, since nowadays log functions add automatically the corresponding prefix.

- Promoted some hax_debug/hax_log to hax_error in the case of actual errors, e.g. in `cpu_vmentry_failed`.

Signed-off-by: Alexandro Sanchez Bach <asanchez@kryptoslogic.com>
Copy link
Contributor

@raphaelning raphaelning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is a great PR! I wasn't familiar with the jump table technique, but I was glad to learn something new :-) The Godbolt link is really helpful!

Centralized all dumping-related functions in dump.c (renamed from dump_vmcs.c). Not sure if this is the best location, but it probably is better than having inlined snippets (that are either commented out or disabled via preprocessor).

Agreed. The new name sounds good to me.

@raphaelning Is there any reason to preserve hax_log functions as-is instead of promoting them directly to hax_info? At the moment they are macro'ed to hax_info: Was this intentional, e.g. to change the macro during as a "debugging hack during development", or can we just remove the macro?

I have no idea about this redundant macro, and I never use it myself. I agree that it should be removed so we can use hax_info, etc. consistently across the codebase.

@AlexAltea
Copy link
Contributor Author

All done!

- Some functions used hax_log, which was macro'ed to hax_info. Most likely a leftover from older HAXM revisions. This has been removed and replaced with hax_info directly.

- Fixed some incorrect format specifiers and improved documentation of `name_vmcs_component`.

Signed-off-by: Alexandro Sanchez Bach <asanchez@kryptoslogic.com>
Copy link
Contributor

@raphaelning raphaelning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@wcwang wcwang merged commit e13a10e into intel:master Nov 7, 2018
@AlexAltea AlexAltea deleted the dump branch November 7, 2018 10:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants