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

Generate #UD exception for unsupported instructions which cause vm-exits #247

Merged
merged 1 commit into from
Dec 9, 2019

Conversation

nevilad
Copy link
Contributor

@nevilad nevilad commented Nov 21, 2019

Generate #UD exception for unsupported instructions which cause vm-exits, instead of terminating the guest.

@HaxmCI HaxmCI added CI:Build Pass CI:Build Pass CI:Mac Test Pass CI:Mac Test Pass labels Nov 21, 2019
@coxuintel
Copy link
Contributor

In what scenario do this happen?

@nevilad
Copy link
Contributor Author

nevilad commented Nov 22, 2019

When some application calls GETSEC, INVD, VMCALL, VMCLEAR, VMLAUNCH, VMPTRLD, VMPTRST, VMREAD, VMRESUME, VMWRITE, VMXOFF, VMXON or XSETBV instruction.
See Intel SDM 3c 25.1.2 Instructions That Cause VM Exits Unconditionally. VMREAD and VMWRITE cause vm-exits conditionally and these conditions are met in hax.

@coxuintel
Copy link
Contributor

coxuintel commented Nov 25, 2019

Yes technically it's absolutely correct, SDM is the bible. What I'm concern is a "real" test case.
As you know the No.1 client of HAXM is Android Emulator, and with the help of many enthusiastic experts like you, HAXM can support other host and guest OS. However since it's still a SW product, we'll need the actual scenario and related test cases when some new feature added, similarly a detailed reproducing step is required if it's a bug. Then when we change the code, our full cycle test could cover the change, if missing, we expand the test coverage, to try our best avoid any possible regression.
Back to this PR, do you have an existing test that may cover this?

@nevilad
Copy link
Contributor Author

nevilad commented Nov 25, 2019

I started a Windows 7 guest, then manually ran an application which calls some of these instructions and observed the application crash on #ud exception (before this patch the result was VM crash). The tests should do the same - wait till OS loads, then simply call these instructions. Since they result in an #ud exception, there is no need to worry about they arguments correctness. For example a test only runs a vmcall instruction and checks that it received #ud exception and the virtual machine is still working and didn't crash. I have code snippets of the tests for Visual Studio and Windows.

@coxuintel
Copy link
Contributor

Interesting. Sounds like you are testing some nested virtualization stuff. We tested Windows 7 before but it's not covered in regular full test. You have tested you patch and it resolves the issue you met right? Seems VMX_EXIT_VMREAD&VMX_EXIT_VMWRITE shouldn't be there.

[VMX_EXIT_VMLAUNCH] = exit_unsupported_instruction,
[VMX_EXIT_VMPTRLD] = exit_unsupported_instruction,
[VMX_EXIT_VMPTRST] = exit_unsupported_instruction,
[VMX_EXIT_VMREAD] = exit_unsupported_instruction,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why VMREAD/WMWRITE registered to unconditional exit handler?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cpuid handling in hax tells the guest that it does not support VT. So execution of any instruction in this subset should generate an #UD. Intel SDM says that exits of these instructions are conditionally, the conditions in hax lead to exits. I tested these instructions and they exited.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it makes the code more clear by adding comments after each of these vmx_exit_* item, like already did in the enum, tells that some are due to unconditional exit while other are conditional according to the SDM 3C 25.1.2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comments.

…its,

instead of terminating the guest.

Signed-off-by: Alexey Romko <nevilad@yahoo.com>
@nevilad nevilad force-pushed the ud_unsupported_instr branch from fa7f038 to fdcdf05 Compare December 5, 2019 19:24
Copy link
Contributor

@coxuintel coxuintel left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@coxuintel coxuintel merged commit 8ca428f into intel:master Dec 9, 2019
@nevilad nevilad deleted the ud_unsupported_instr branch December 9, 2019 08:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI:Build Pass CI:Build Pass CI:Mac Test Pass CI:Mac Test Pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants