[PATCH] KVM: x86: fix ordering of cr0 initialization code in vmx_cpu_reset

[PATCH] KVM: x86: fix ordering of cr0 initialization code in vmx_cpu_reset

[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.

Signed-off-by: Bruce Rogers <brogers@suse.com>
---
 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

Re: [PATCH] KVM: x86: fix ordering of cr0 initialization code in vmx_cpu_reset

[Greg KH]

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 <brogers@suse.com>
> ---
>  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>

Re: [PATCH] KVM: x86: fix ordering of cr0 initialization code in vmx_cpu_reset

[Radim Krčmář]

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: stable@vger.kernel.org
when stable@vger.kernel.org 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 <brogers@suse.com>
> ---
> 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.

Re: [PATCH] KVM: x86: fix ordering of cr0 initialization code in vmx_cpu_reset

[Bruce Rogers]

>>> On 4/28/2016 at 01:08 PM, Radim Kr*mￜ* <rkrcmar@redhat.com> 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: stable@vger.kernel.org
> when stable@vger.kernel.org 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 <brogers@suse.com>
>> ---
>> 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