Re: [PATCH] KVM: x86: fix ordering of cr0 initialization code in vmx_cpu_reset
>>> On 4/28/2016 at 01:08 PM, Radim Kr*má* 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: sta...@vger.kernel.org
> when sta...@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
>> ---
>> 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
Re: [PATCH] KVM: x86: fix ordering of cr0 initialization code in vmx_cpu_reset
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: sta...@vger.kernel.org
when sta...@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
> ---
> 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
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 > --- > arch/x86/kvm/vmx.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) 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.
[PATCH] KVM: x86: fix ordering of cr0 initialization code in vmx_cpu_reset
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 --- 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
