Commit d28bc9dd25ce reversed the order of two lines which initialize cr0,
allowing the current (old) cr0 value to mess up vcpu initialization.
This was observed in the checks for cr0 X86_CR0_WP bit in the context of
kvm_mmu_reset_context(). Besides, setting vcpu->arch.cr0 after vmx_set_cr0()
is completely redundant. Change the order back to ensure proper vcpu
initialization.
The combination of booting with ovmf firmware when guest vcpus > 1 and kvm's
ept=N option being set results in a VM-entry failure. This patch fixes that.
Signed-off-by: Bruce Rogers <[email protected]>
---
arch/x86/kvm/vmx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index ee1c8a9..ab4a387 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -5046,8 +5046,8 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
vmcs_write16(VIRTUAL_PROCESSOR_ID, vmx->vpid);
cr0 = X86_CR0_NW | X86_CR0_CD | X86_CR0_ET;
- vmx_set_cr0(vcpu, cr0); /* enter rmode */
vmx->vcpu.arch.cr0 = cr0;
+ vmx_set_cr0(vcpu, cr0); /* enter rmode */
vmx_set_cr4(vcpu, 0);
vmx_set_efer(vcpu, 0);
vmx_fpu_activate(vcpu);
--
1.9.0
On Fri, Apr 22, 2016 at 12:56:03PM -0600, Bruce Rogers wrote:
> Commit d28bc9dd25ce reversed the order of two lines which initialize cr0,
> allowing the current (old) cr0 value to mess up vcpu initialization.
> This was observed in the checks for cr0 X86_CR0_WP bit in the context of
> kvm_mmu_reset_context(). Besides, setting vcpu->arch.cr0 after vmx_set_cr0()
> is completely redundant. Change the order back to ensure proper vcpu
> initialization.
>
> The combination of booting with ovmf firmware when guest vcpus > 1 and kvm's
> ept=N option being set results in a VM-entry failure. This patch fixes that.
>
> Signed-off-by: Bruce Rogers <[email protected]>
> ---
> arch/x86/kvm/vmx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
<formletter>
This is not the correct way to submit patches for inclusion in the
stable kernel tree. Please read Documentation/stable_kernel_rules.txt
for how to do this properly.
</formletter>
2016-04-22 12:56-0600, Bruce Rogers:
> Commit d28bc9dd25ce reversed the order of two lines which initialize cr0,
> allowing the current (old) cr0 value to mess up vcpu initialization.
> This was observed in the checks for cr0 X86_CR0_WP bit in the context of
> kvm_mmu_reset_context(). Besides, setting vcpu->arch.cr0 after vmx_set_cr0()
> is completely redundant. Change the order back to ensure proper vcpu
> initialization.
>
> The combination of booting with ovmf firmware when guest vcpus > 1 and kvm's
> ept=N option being set results in a VM-entry failure. This patch fixes that.
Greg pointed out missing,
Cc: [email protected]
when [email protected] was Cc'd. Adding
Fixes: d28bc9dd25ce ("KVM: x86: INIT and reset sequences are different")
would be nice too (even when it is redundant).
> Signed-off-by: Bruce Rogers <[email protected]>
> ---
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> @@ -5046,8 +5046,8 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
> cr0 = X86_CR0_NW | X86_CR0_CD | X86_CR0_ET;
> - vmx_set_cr0(vcpu, cr0); /* enter rmode */
> vmx->vcpu.arch.cr0 = cr0;
> + vmx_set_cr0(vcpu, cr0); /* enter rmode */
So vmx_set_cr0() has a code that depends on vmx->vcpu.arch.cr0 being
already set the to new value. Do you know what function is it?
I think we better set vmx->vcpu.arch.cr0 early in vmx_set_cr0().
Or do other callsites somehow depend on the old cr0 value?
Thanks.
>>> On 4/28/2016 at 01:08 PM, Radim Kr*m?* <[email protected]> wrote:
> 2016-04-22 12:56-0600, Bruce Rogers:
>> Commit d28bc9dd25ce reversed the order of two lines which initialize cr0,
>> allowing the current (old) cr0 value to mess up vcpu initialization.
>> This was observed in the checks for cr0 X86_CR0_WP bit in the context of
>> kvm_mmu_reset_context(). Besides, setting vcpu->arch.cr0 after vmx_set_cr0()
>> is completely redundant. Change the order back to ensure proper vcpu
>> initialization.
>>
>> The combination of booting with ovmf firmware when guest vcpus > 1 and kvm's
>> ept=N option being set results in a VM-entry failure. This patch fixes that.
>
> Greg pointed out missing,
> Cc: [email protected]
> when [email protected] was Cc'd. Adding
> Fixes: d28bc9dd25ce ("KVM: x86: INIT and reset sequences are different")
> would be nice too (even when it is redundant).
>
>> Signed-off-by: Bruce Rogers <[email protected]>
>> ---
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> @@ -5046,8 +5046,8 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool
> init_event)
>> cr0 = X86_CR0_NW | X86_CR0_CD | X86_CR0_ET;
>> - vmx_set_cr0(vcpu, cr0); /* enter rmode */
>> vmx->vcpu.arch.cr0 = cr0;
>> + vmx_set_cr0(vcpu, cr0); /* enter rmode */
>
> So vmx_set_cr0() has a code that depends on vmx->vcpu.arch.cr0 being
> already set the to new value. Do you know what function is it?
The one which I partly referred to above is the following:
vmx_set_cr0() ->
enter_rmode() ->
kvm_mmu_reset_context() ->
init_kvm_softmmu() ->
kvm_init_shadow_mmu() ->
is_write_protected()
which uses the CR0 WP bit.
There may be other problematic references. I haven't tried to do an
exhaustive search.
>
> I think we better set vmx->vcpu.arch.cr0 early in vmx_set_cr0().
> Or do other callsites somehow depend on the old cr0 value?
You may be right that that is a better overall fix. I was simply trying
to undo the erroneous lines in the commit which broke things, partly
to have a patch better suited for applying to stable releases.
I'll send a v2 shortly with your suggested additions to the patch header.
Thanks,
Bruce