On 10/03/2016 09:27, Xiao Guangrong wrote:
>>
>
>> + if (!enable_ept) {
>> + guest_efer |= EFER_NX;
>> + ignore_bits |= EFER_NX;
>
> Update ignore_bits is not necessary i think.
More precisely, ignore_bits is only needed if guest EFER.NX=0 and we're
not in this CR0.WP=1/CR4.SMEP=0 situation. In theory you could have
guest EFER.NX=1 and host EFER.NX=0.
This is what I came up with (plus some comments :)):
u64 guest_efer = vmx->vcpu.arch.efer;
u64 ignore_bits = 0;
if (!enable_ept) {
if (boot_cpu_has(X86_FEATURE_SMEP))
guest_efer |= EFER_NX;
else if (!(guest_efer & EFER_NX))
ignore_bits |= EFER_NX;
}
>> - guest_efer = vmx->vcpu.arch.efer;
>> if (!(guest_efer & EFER_LMA))
>> guest_efer &= ~EFER_LME;
>> if (guest_efer != host_efer)
>> add_atomic_switch_msr(vmx, MSR_EFER,
>> guest_efer, host_efer);
>
> So, why not set EFER_NX (if !ept) just in this branch to make the fix
> more simpler?
I didn't like having
guest_efer = vmx->vcpu.arch.efer;
...
if (!enable_ept)
guest_efer |= EFER_NX;
guest_efer &= ~ignore_bits;
guest_efer |= host_efer & ignore_bits;
...
if (...) {
guest_efer = vmx->vcpu.arch.efer;
if (!enable_ept)
guest_efer |= EFER_NX;
...
}
My patch is bigger but the resulting code is smaller and easier to follow:
guest_efer = vmx->vcpu.arch.efer;
if (!enable_ept)
guest_efer |= EFER_NX;
...
if (...) {
...
} else {
guest_efer &= ~ignore_bits;
guest_efer |= host_efer & ignore_bits;
}
Paolo
