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

emulate: save/restore host flags in fastop_dispatch #216

Merged
merged 1 commit into from
Jul 9, 2019

Conversation

jarveson
Copy link
Contributor

@jarveson jarveson commented Jun 21, 2019

fastop_dispatch isn't saving/restoring host flags before overwriting with guest flags, which can cause some interesting issues. This fixes that by just pushing and poping host flags before/after executing fastop instruction.

@wayne-ma
Copy link
Contributor

OK to verify

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

coxuintel commented Jul 4, 2019

emmm I'm investigating another interesting issue in PR#204 recently and finally came to the same root cause as described in this PR, that FLAGS are corrupted after fastop.
I have some comments to the change, and I'll also add @AlexAltea to take a look.

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.

In generally, it's better to add PUSHF/POPF at begin/end and adjust stack_arg offset accordingly.

@@ -210,11 +210,13 @@ fastop_dispatch:
mov reg_dst, [ebx]
mov reg_src1, [esi]
mov reg_src2, [edi]
pushf
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll suggest move PUSHF to beginning, after push ebp. And move POPF to end, before pop ebp. Then change stack_arg(index) by adding another 0x4 offset. No need to change call stack_arg() so that the fastop handler and remaining arguments are still start from index 0.

@@ -237,11 +239,13 @@ fastop_dispatch:
mov reg_dst, [rsi]
mov reg_src1, [r10]
mov reg_src2, [r11]
pushf
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto. Move this PUSHF to beginning and adjust stack_arg() offset. Move POPF to end.

@@ -262,11 +266,13 @@ fastop_dispatch:
mov reg_dst, [r11]
mov reg_src1, [r8]
mov reg_src2, [r9]
pushf
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

@coxuintel coxuintel mentioned this pull request Jul 4, 2019
@AlexAltea
Copy link
Contributor

Looks good to me! Thanks for the patch @jarveson, I also agree with the new pushf/popf placement suggested by @coxuintel.

@jarveson
Copy link
Contributor Author

jarveson commented Jul 4, 2019

sure, so something like this then?

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.

The new change looks good to me. I'll merge this PR.

@coxuintel
Copy link
Contributor

@jarveson would you mind update the commit message in your own branch? I'll merge it.

fastop_dispatch isn't saving/restoring host flags before overwriting with guest flags, which can cause some interesting issues. This fixes that by just pushing and poping host flags before/after executing fastop instruction.

Signed-off-by: Jake Arveson <jarveson@gmail.com>
@jarveson
Copy link
Contributor Author

jarveson commented Jul 8, 2019

like this? otherwise im not fully sure what you mean

@coxuintel
Copy link
Contributor

like this? otherwise im not fully sure what you mean

Exactly. Unlike PR comments, detailed commit msg in original commit will remains in git repo which helps future development.

@coxuintel coxuintel merged commit de01cba into intel:master Jul 9, 2019
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.

5 participants