[PATCH v2] KVM: x86: Reset RFLAGS state following processor init/reset

[PATCH v2] KVM: x86: Reset RFLAGS state following processor init/reset

[Wanpeng Li]

Reference SDM Volume 1 3.4.3:

Following initialization of the processor (either by asserting the 
RESET pin or the INIT pin), the state of the EFLAGS register is 
00000002H.

However, the eflags fixed bit is not set and other bits are also not 
cleared during the init/reset in kvm.

This patch reset eflags register to 00000002H following initialization 
of the processor.

Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
v1 -> v2:
 * use vmcs_writel

 arch/x86/kvm/vmx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index b680c2e..1a95ef7 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -4935,6 +4935,7 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 	vmx_set_efer(vcpu, 0);
 	vmx_fpu_activate(vcpu);
 	update_exception_bitmap(vcpu);
+	vmcs_writel(GUEST_RFLAGS, X86_EFLAGS_FIXED);
 
 	vpid_sync_context(vmx->vpid);
 }
-- 
1.9.1

Re: [PATCH v2] KVM: x86: Reset RFLAGS state following processor init/reset

[Paolo Bonzini]

On 03/11/2015 12:40, Wanpeng Li wrote:
> Reference SDM Volume 1 3.4.3:
> 
> Following initialization of the processor (either by asserting the 
> RESET pin or the INIT pin), the state of the EFLAGS register is 
> 00000002H.
> 
> However, the eflags fixed bit is not set and other bits are also not 
> cleared during the init/reset in kvm.
> 
> This patch reset eflags register to 00000002H following initialization 
> of the processor.
> 
> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> ---
> v1 -> v2:
>  * use vmcs_writel
> 
>  arch/x86/kvm/vmx.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index b680c2e..1a95ef7 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -4935,6 +4935,7 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>  	vmx_set_efer(vcpu, 0);
>  	vmx_fpu_activate(vcpu);
>  	update_exception_bitmap(vcpu);
> +	vmcs_writel(GUEST_RFLAGS, X86_EFLAGS_FIXED);
>  
>  	vpid_sync_context(vmx->vpid);
>  }
> 

No, this is doing exactly the same thing that is already done elsewhere
in vmx_vcpu_reset (which Nadav pointed out to you).  So it's not just a
pointless addition with no effect at all; it's wrong, because it
introduces duplication.

Please answer this question: is there a bug or not?

If yes, then using kvm_set_rflags as in v1 is the right thing.  However,
you have to remove the _existing_ vmcs_writel call in vmx_vcpu_reset.
Also, if there is a bug you have to explain it in the commit message and
provide a testcase.  By the way, I am still waiting for the VPID test cases.

If no, then this is a cleanup, we can still do the change but you have
to explain this in the commit message.

Paolo