-
Notifications
You must be signed in to change notification settings - Fork 880
Fixes #29. Ensure cr0/cr4 fixed bit settings prior vmxon. #49
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot! I've made quite a few comments, but I wouldn't ask you to address all of them, since most of them are out of the scope of this patch ("refactoring") and can be addressed later. I'd pick out the one at cpu.c:723
as the only required change.
If you don't have time to improve this patch, let me know if it would be easier if we took over and updated the PR directly.
@@ -636,11 +719,12 @@ uint32 load_vmcs(struct vcpu_t *vcpu, preempt_flag *flags) | |||
} | |||
} | |||
#endif | |||
check_vmxon_coherency(cpu_data); | |||
restore_host_cr(cpu_data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we should only restore the CRs when fatal == true
. In the !fatal
case, we should just act as if __vmxon()
were successful (even though it failed because another Mac VMM had enabled VMX for this CPU).
|
||
smp_mb(); | ||
|
||
err = __vmxoff(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's an idea to avoid exposing restore_host_cr()
without moving all __vmxon()
error handling to vmxroot_enter()
:
Basically, we ensure that it's safe to call vmxroot_leave()
at all times:
err = vmxroot_enter();
// Do something
// Always call vmxroot_leave(), even if vmxroot_enter() failed
err = vmxroot_leave();
For this to work, we just need to make sure we do not call __vmxoff()
unless we know VMX is already on for the current CPU, or even better, unless it was HAXM that turned VMX on for the current CPU. I think this is exactly what the VMXON_HAX
flag is for, so we can check (cpu_data->vmm_flag & VMXON_HAX)
, assuming vmxroot_enter()
always sets this flag after a successful __vmxon()
. Of course, we also need to clear the flag after __vmxoff()
.
struct per_cpu_data; | ||
extern vmx_error_t vmxroot_enter(struct per_cpu_data *cpu_data); | ||
extern vmx_error_t vmxroot_leave(struct per_cpu_data *cpu_data); | ||
extern void restore_host_cr(struct per_cpu_data *cpu_data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- nit: Is there a good reason to use
extern
in function declarations? It has no effect, and people would expect the function to be defined elsewhere (in a.c
file). - To me,
restore_host_cr()
is too low-level to be exposed as part of the interface. If it has to be exposed, how about using the same prefix for consistency, e.g.vmxroot_restore_host_cr()
? Ideally,vmxroot_enter()
should take care of error handling, so its caller doesn't have to.
ia32_wrmsr(IA32_FEATURE_CONTROL, | ||
fc_msr | FC_LOCKED | FC_VMXON_OUTSMX); | ||
|
||
return __vmxon(hax_page_pa(cpu_data->vmxon_page)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I said in another comment, it would be nice if we handled__vmxon()
errors in vmxroot_enter()
and did not expose restore_host_cr()
. In fact, there are pieces of logic currently implemented in load_vmcs()
that should eventually be moved here:
- Updating the global variables for debugging, e.g.
log_vmxon_xxx
. - Setting the
VMXON_HAX
bit ofcpu_data->vmm_flag
if__vmxon()
was successful. - Handling the Mac-specific
!fatal
case. - Handling the
fatal
case.
Making all these changes would probably be too ambitious for this refactoring effort. But maybe we could get 1 and 2 done first?
* Restore host control registers value before entering vmxroot | ||
* operations. Must be smp_func_call() safe. | ||
*/ | ||
void restore_host_cr(struct per_cpu_data *cpu_data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this function static
when we remove its declaration from vmx.h
.
void restore_host_cr(struct per_cpu_data *cpu_data) | ||
{ | ||
if (cpu_data->vmm_flag & VMXON_RESTORE_CR0) | ||
set_cr0(cpu_data->host_cr0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also clear the VMXON_RESTORE_CR0
bit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, no debate :)
set_cr0(cpu_data->host_cr0); | ||
|
||
if (cpu_data->vmm_flag & VMXON_RESTORE_CR4) | ||
set_cr4(cpu_data->host_cr4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also clear the VMXON_RESTORE_CR4
bit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one too
uint64 cr0, cr0_f0, cr0_f1; | ||
uint64 cr4, cr4_f0, cr4_f1; | ||
|
||
smp_mb(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really familiar with memory barriers, and I'm curious why you use it here. Because of this line, smp_mb()
is called twice for both load_vmcs()
and invept_smpfunc()
after the refactoring, but only once in the original code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only compiler stuff optimization to prevent memory reordering operations ... Please refer to https://yarchive.net/comp/linux/memory_barriers.html, if you want to know more about it. You don't have to see it as a real function call.
In this situation we have to be safe toward smp function calls (like it's the case in HAXM code when you meet xxx_smpfunc()). Even if the 2 functions use to be called along side, if one day someone calls them appart ... it may fail.
As for all of your previous comments, i tried to keep the philosophy of the existing code without rewriting everything actually.
{ | ||
vmx_error_t err; | ||
|
||
smp_mb(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here (see my comment on line 574).
@@ -502,7 +502,15 @@ uint8 is_vmcs_loaded(struct vcpu_t *vcpu) | |||
|
|||
int debug_vmcs_count = 0; | |||
|
|||
void restore_host_cr4_vmxe(struct per_cpu_data *cpu_data); | |||
void check_vmxon_coherency(struct per_cpu_data *cpu_data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this function static
? Actually, a better idea is to merge this function into vmxroot_leave()
(after modifying vmxroot_leave()
as I propose below).
Hello, I'm really sorry for the answer delay. I had no more time slot to work on the subject :( and i think it will be difficult by the end of the year. I'll answer point by point to your comments, but please feel free to do what ever you think it's best with the PR. I did it a little bit in a hurry, to help, but based on your comments it tends to become a full-time dev job :) and i totally agree some part would have to be completely rewritten. |
There is coexistence issue in MacOS when some Mac VMMs are running at the same time. invept might not be invoked due to coexistence issue. This change is to fix the issue that invept is not invoked. It also refactors the existed logic for VMXON/VMXOFF into two functions. The function names come from PR #49.
Thanks, Stephane. I guess we should have kept this PR simple and left the refactoring to another PR. Now we have created #69 for the latter. When that is merged, it should be easy to fix #29. But I understand that you may not have time to rebase your original patch and resubmit it. We can do that later and will give you credit in the commit message. |
563eb1b
to
6b942e3
Compare
b73a231
to
da1b8ec
Compare
Hi,
Sorry for the delay. Holidays :) It fixes the vmxon issue #29 with code refactoring as discussed in PR46 (you can now ignore).