[PATCH v3 1/2] KVM: X86: Processor States following Reset or INIT

[PATCH v3 1/2] KVM: X86: Processor States following Reset or INIT

[Wanpeng Li]

From: Wanpeng Li <wanpeng.li@hotmail.com>

- XCR0 is reset to 1 by RESET but not INIT
- XSS is zeroed by both RESET and INIT
- BNDCFGU, BND0-BND3, BNDCFGS are zeroed by both RESET and INIT

This patch does this according to SDM.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Jim Mattson <jmattson@google.com>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
v2 -> v3:
 * fix null pointer deference
 * fix patch description
v1 -> v2:
 * XCR0 is not zeroed by INIT
 * XSS, BNDCFGU, BND0-BND3, BNDCFGS are zeroed by both RESET and INIT

 arch/x86/kvm/vmx.c |  2 ++
 arch/x86/kvm/x86.c | 15 +++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 244e366..ab0f16e 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -5581,6 +5581,8 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 	vmcs_write32(GUEST_ACTIVITY_STATE, GUEST_ACTIVITY_ACTIVE);
 	vmcs_write32(GUEST_INTERRUPTIBILITY_INFO, 0);
 	vmcs_writel(GUEST_PENDING_DBG_EXCEPTIONS, 0);
+	if (kvm_mpx_supported())
+		vmcs_write64(GUEST_BNDCFGS, 0);
 
 	setup_msrs(vmx);
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b0d2915..b1c1755 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7804,18 +7804,33 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 	kvm_async_pf_hash_reset(vcpu);
 	vcpu->arch.apf.halted = false;
 
+	if (kvm_mpx_supported()) {
+		void *mpx_state_buffer;
+
+		mpx_state_buffer = get_xsave_addr(&vcpu->arch.guest_fpu.state.xsave, XFEATURE_MASK_BNDREGS);
+		if (mpx_state_buffer)
+			memset(mpx_state_buffer, 0, sizeof(struct mpx_bndreg_state));
+		mpx_state_buffer = get_xsave_addr(&vcpu->arch.guest_fpu.state.xsave, XFEATURE_MASK_BNDCSR);
+		if (mpx_state_buffer)
+			memset(mpx_state_buffer, 0, sizeof(u64));
+	}
+
 	if (!init_event) {
 		kvm_pmu_reset(vcpu);
 		vcpu->arch.smbase = 0x30000;
 
 		vcpu->arch.msr_platform_info = MSR_PLATFORM_INFO_CPUID_FAULT;
 		vcpu->arch.msr_misc_features_enables = 0;
+
+		vcpu->arch.xcr0 = XFEATURE_MASK_FP;
 	}
 
 	memset(vcpu->arch.regs, 0, sizeof(vcpu->arch.regs));
 	vcpu->arch.regs_avail = ~0;
 	vcpu->arch.regs_dirty = ~0;
 
+	vcpu->arch.ia32_xss = 0;
+
 	kvm_x86_ops->vcpu_reset(vcpu, init_event);
 }
 
-- 
2.7.4

[PATCH v3 2/2] KVM: VMX: Don't expose unrestricted_guest is enabled if ept is disabled

[Wanpeng Li]

From: Wanpeng Li <wanpeng.li@hotmail.com>

SDM mentioned:

 "If either the “unrestricted guest” VM-execution control or the “mode-based
  execute control for EPT” VM- execution control is 1, the “enable EPT”
  VM-execution control must also be 1."

However, we can still observe unrestricted_guest is Y after inserting the kvm-intel.ko
w/ ept=N. It depends on later starts a guest in order that the function
vmx_compute_secondary_exec_control() can be executed, then both the module parameter
and exec control fields will be amended.

This patch fixes it by amending module parameter immediately during vmcs data setup.

Reviewed-by: Jim Mattson <jmattson@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Jim Mattson <jmattson@google.com>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
v1 -> v2: 
 * reduce to just "enable_ept = 0"

 arch/x86/kvm/vmx.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index ab0f16e..dedff59 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -6730,16 +6730,13 @@ static __init int hardware_setup(void)
 	if (!cpu_has_vmx_ept() ||
 	    !cpu_has_vmx_ept_4levels() ||
 	    !cpu_has_vmx_ept_mt_wb() ||
-	    !cpu_has_vmx_invept_global()) {
+	    !cpu_has_vmx_invept_global())
 		enable_ept = 0;
-		enable_unrestricted_guest = 0;
-		enable_ept_ad_bits = 0;
-	}
 
 	if (!cpu_has_vmx_ept_ad_bits() || !enable_ept)
 		enable_ept_ad_bits = 0;
 
-	if (!cpu_has_vmx_unrestricted_guest())
+	if (!cpu_has_vmx_unrestricted_guest() || !enable_ept)
 		enable_unrestricted_guest = 0;
 
 	if (!cpu_has_vmx_flexpriority())
-- 
2.7.4

Re: [PATCH v3 2/2] KVM: VMX: Don't expose unrestricted_guest is enabled if ept is disabled

[Jim Mattson]

Reviewed-by: Jim Mattson <jmattson@google.com>

On Mon, Oct 9, 2017 at 3:51 PM, Wanpeng Li <kernellwp@gmail.com> wrote:
> From: Wanpeng Li <wanpeng.li@hotmail.com>
>
> SDM mentioned:
>
>  "If either the “unrestricted guest” VM-execution control or the “mode-based
>   execute control for EPT” VM- execution control is 1, the “enable EPT”
>   VM-execution control must also be 1."
>
> However, we can still observe unrestricted_guest is Y after inserting the kvm-intel.ko
> w/ ept=N. It depends on later starts a guest in order that the function
> vmx_compute_secondary_exec_control() can be executed, then both the module parameter
> and exec control fields will be amended.
>
> This patch fixes it by amending module parameter immediately during vmcs data setup.
>
> Reviewed-by: Jim Mattson <jmattson@google.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Cc: Jim Mattson <jmattson@google.com>
> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> ---
> v1 -> v2:
>  * reduce to just "enable_ept = 0"
>
>  arch/x86/kvm/vmx.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index ab0f16e..dedff59 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -6730,16 +6730,13 @@ static __init int hardware_setup(void)
>         if (!cpu_has_vmx_ept() ||
>             !cpu_has_vmx_ept_4levels() ||
>             !cpu_has_vmx_ept_mt_wb() ||
> -           !cpu_has_vmx_invept_global()) {
> +           !cpu_has_vmx_invept_global())
>                 enable_ept = 0;
> -               enable_unrestricted_guest = 0;
> -               enable_ept_ad_bits = 0;
> -       }
>
>         if (!cpu_has_vmx_ept_ad_bits() || !enable_ept)
>                 enable_ept_ad_bits = 0;
>
> -       if (!cpu_has_vmx_unrestricted_guest())
> +       if (!cpu_has_vmx_unrestricted_guest() || !enable_ept)
>                 enable_unrestricted_guest = 0;
>
>         if (!cpu_has_vmx_flexpriority())
> --
> 2.7.4
>

Re: [PATCH v3 1/2] KVM: X86: Processor States following Reset or INIT

[Radim Krčmář]

2017-10-09 15:51-0700, Wanpeng Li:
> From: Wanpeng Li <wanpeng.li@hotmail.com>
> 
> - XCR0 is reset to 1 by RESET but not INIT
> - XSS is zeroed by both RESET and INIT
> - BNDCFGU, BND0-BND3, BNDCFGS are zeroed by both RESET and INIT
> 
> This patch does this according to SDM.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Cc: Jim Mattson <jmattson@google.com>
> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> ---
> v2 -> v3:
>  * fix null pointer deference
>  * fix patch description
> v1 -> v2:
>  * XCR0 is not zeroed by INIT
>  * XSS, BNDCFGU, BND0-BND3, BNDCFGS are zeroed by both RESET and INIT
> 
>  arch/x86/kvm/vmx.c |  2 ++
>  arch/x86/kvm/x86.c | 15 +++++++++++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> @@ -7804,18 +7804,33 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>  	kvm_async_pf_hash_reset(vcpu);
>  	vcpu->arch.apf.halted = false;
>  
> +	if (kvm_mpx_supported()) {
> +		void *mpx_state_buffer;
> +
> +		mpx_state_buffer = get_xsave_addr(&vcpu->arch.guest_fpu.state.xsave, XFEATURE_MASK_BNDREGS);
> +		if (mpx_state_buffer)
> +			memset(mpx_state_buffer, 0, sizeof(struct mpx_bndreg_state));

I think we should call kvm_put_guest_fpu() before doing this.
The register might be loaded in CPU and XSAVE instruction from
vcpu_put() would overwrite any changes we did.

> +		mpx_state_buffer = get_xsave_addr(&vcpu->arch.guest_fpu.state.xsave, XFEATURE_MASK_BNDCSR);
> +		if (mpx_state_buffer)
> +			memset(mpx_state_buffer, 0, sizeof(u64));

XFEATURE_MASK_BNDCSR is actually

  struct mpx_bndcsr {
  	u64 bndcfgu;
  	u64 bndstatus;
  } __packed;

So clearing two u64 would be correct, thanks.

Re: [PATCH v3 1/2] KVM: X86: Processor States following Reset or INIT

[Wanpeng Li]

2017-10-11 2:55 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>:
> 2017-10-09 15:51-0700, Wanpeng Li:
>> From: Wanpeng Li <wanpeng.li@hotmail.com>
>>
>> - XCR0 is reset to 1 by RESET but not INIT
>> - XSS is zeroed by both RESET and INIT
>> - BNDCFGU, BND0-BND3, BNDCFGS are zeroed by both RESET and INIT
>>
>> This patch does this according to SDM.
>>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Radim Krčmář <rkrcmar@redhat.com>
>> Cc: Jim Mattson <jmattson@google.com>
>> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
>> ---
>> v2 -> v3:
>>  * fix null pointer deference
>>  * fix patch description
>> v1 -> v2:
>>  * XCR0 is not zeroed by INIT
>>  * XSS, BNDCFGU, BND0-BND3, BNDCFGS are zeroed by both RESET and INIT
>>
>>  arch/x86/kvm/vmx.c |  2 ++
>>  arch/x86/kvm/x86.c | 15 +++++++++++++++
>>  2 files changed, 17 insertions(+)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> @@ -7804,18 +7804,33 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>>       kvm_async_pf_hash_reset(vcpu);
>>       vcpu->arch.apf.halted = false;
>>
>> +     if (kvm_mpx_supported()) {
>> +             void *mpx_state_buffer;
>> +
>> +             mpx_state_buffer = get_xsave_addr(&vcpu->arch.guest_fpu.state.xsave, XFEATURE_MASK_BNDREGS);
>> +             if (mpx_state_buffer)
>> +                     memset(mpx_state_buffer, 0, sizeof(struct mpx_bndreg_state));
>
> I think we should call kvm_put_guest_fpu() before doing this.
> The register might be loaded in CPU and XSAVE instruction from
> vcpu_put() would overwrite any changes we did.

Except in vCPU creation path, vcpu_put() is called in the
destroy/error path, so I think it doesn't matter to overwrite changes
we did.

>
>> +             mpx_state_buffer = get_xsave_addr(&vcpu->arch.guest_fpu.state.xsave, XFEATURE_MASK_BNDCSR);
>> +             if (mpx_state_buffer)
>> +                     memset(mpx_state_buffer, 0, sizeof(u64));
>
> XFEATURE_MASK_BNDCSR is actually
>
>   struct mpx_bndcsr {
>         u64 bndcfgu;
>         u64 bndstatus;
>   } __packed;
>
> So clearing two u64 would be correct, thanks.

SDM section 9.1.1 just mentioned BNDCFGU will be cleared after
RESET/INIT, but not mentioned BNDSTATUS.

Regards,
Wanpeng Li

Re: [PATCH v3 1/2] KVM: X86: Processor States following Reset or INIT

[Radim Krčmář]

2017-10-11 08:39+0800, Wanpeng Li:
> 2017-10-11 2:55 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>:
> > 2017-10-09 15:51-0700, Wanpeng Li:
> >> From: Wanpeng Li <wanpeng.li@hotmail.com>
> >>
> >> - XCR0 is reset to 1 by RESET but not INIT
> >> - XSS is zeroed by both RESET and INIT
> >> - BNDCFGU, BND0-BND3, BNDCFGS are zeroed by both RESET and INIT
> >>
> >> This patch does this according to SDM.
> >>
> >> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >> Cc: Radim Krčmář <rkrcmar@redhat.com>
> >> Cc: Jim Mattson <jmattson@google.com>
> >> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> >> ---
> >> v2 -> v3:
> >>  * fix null pointer deference
> >>  * fix patch description
> >> v1 -> v2:
> >>  * XCR0 is not zeroed by INIT
> >>  * XSS, BNDCFGU, BND0-BND3, BNDCFGS are zeroed by both RESET and INIT
> >>
> >>  arch/x86/kvm/vmx.c |  2 ++
> >>  arch/x86/kvm/x86.c | 15 +++++++++++++++
> >>  2 files changed, 17 insertions(+)
> >>
> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >> @@ -7804,18 +7804,33 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
> >>       kvm_async_pf_hash_reset(vcpu);
> >>       vcpu->arch.apf.halted = false;
> >>
> >> +     if (kvm_mpx_supported()) {
> >> +             void *mpx_state_buffer;
> >> +
> >> +             mpx_state_buffer = get_xsave_addr(&vcpu->arch.guest_fpu.state.xsave, XFEATURE_MASK_BNDREGS);
> >> +             if (mpx_state_buffer)
> >> +                     memset(mpx_state_buffer, 0, sizeof(struct mpx_bndreg_state));
> >
> > I think we should call kvm_put_guest_fpu() before doing this.
> > The register might be loaded in CPU and XSAVE instruction from
> > vcpu_put() would overwrite any changes we did.
> 
> Except in vCPU creation path, vcpu_put() is called in the
> destroy/error path, so I think it doesn't matter to overwrite changes
> we did.

Yeah, creation doesn't have FPU loaded and destroy doesn't matter in any
case, but we also have the INIT path from kvm_apic_accept_events() that
can be called with loaded FPU and does not let userspace fix the state.

> >> +             mpx_state_buffer = get_xsave_addr(&vcpu->arch.guest_fpu.state.xsave, XFEATURE_MASK_BNDCSR);
> >> +             if (mpx_state_buffer)
> >> +                     memset(mpx_state_buffer, 0, sizeof(u64));
> >
> > XFEATURE_MASK_BNDCSR is actually
> >
> >   struct mpx_bndcsr {
> >         u64 bndcfgu;
> >         u64 bndstatus;
> >   } __packed;
> >
> > So clearing two u64 would be correct, thanks.
> 
> SDM section 9.1.1 just mentioned BNDCFGU will be cleared after
> RESET/INIT, but not mentioned BNDSTATUS.

That section conspires ;) I found a mention in 17.3.3 Configuration and
Status Registers:

  RESET or INIT# will set BNDCFGx and BNDSTATUS registers to zero.

Re: [PATCH v3 1/2] KVM: X86: Processor States following Reset or INIT

[Wanpeng Li]

2017-10-11 19:36 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>:
> 2017-10-11 08:39+0800, Wanpeng Li:
>> 2017-10-11 2:55 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>:
>> > 2017-10-09 15:51-0700, Wanpeng Li:
>> >> From: Wanpeng Li <wanpeng.li@hotmail.com>
>> >>
>> >> - XCR0 is reset to 1 by RESET but not INIT
>> >> - XSS is zeroed by both RESET and INIT
>> >> - BNDCFGU, BND0-BND3, BNDCFGS are zeroed by both RESET and INIT
>> >>
>> >> This patch does this according to SDM.
>> >>
>> >> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> >> Cc: Radim Krčmář <rkrcmar@redhat.com>
>> >> Cc: Jim Mattson <jmattson@google.com>
>> >> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
>> >> ---
>> >> v2 -> v3:
>> >>  * fix null pointer deference
>> >>  * fix patch description
>> >> v1 -> v2:
>> >>  * XCR0 is not zeroed by INIT
>> >>  * XSS, BNDCFGU, BND0-BND3, BNDCFGS are zeroed by both RESET and INIT
>> >>
>> >>  arch/x86/kvm/vmx.c |  2 ++
>> >>  arch/x86/kvm/x86.c | 15 +++++++++++++++
>> >>  2 files changed, 17 insertions(+)
>> >>
>> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> >> @@ -7804,18 +7804,33 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>> >>       kvm_async_pf_hash_reset(vcpu);
>> >>       vcpu->arch.apf.halted = false;
>> >>
>> >> +     if (kvm_mpx_supported()) {
>> >> +             void *mpx_state_buffer;
>> >> +
>> >> +             mpx_state_buffer = get_xsave_addr(&vcpu->arch.guest_fpu.state.xsave, XFEATURE_MASK_BNDREGS);
>> >> +             if (mpx_state_buffer)
>> >> +                     memset(mpx_state_buffer, 0, sizeof(struct mpx_bndreg_state));
>> >
>> > I think we should call kvm_put_guest_fpu() before doing this.
>> > The register might be loaded in CPU and XSAVE instruction from
>> > vcpu_put() would overwrite any changes we did.
>>
>> Except in vCPU creation path, vcpu_put() is called in the
>> destroy/error path, so I think it doesn't matter to overwrite changes
>> we did.
>
> Yeah, creation doesn't have FPU loaded and destroy doesn't matter in any
> case, but we also have the INIT path from kvm_apic_accept_events() that
> can be called with loaded FPU and does not let userspace fix the state.

Thanks for pointing out this. :)

>
>> >> +             mpx_state_buffer = get_xsave_addr(&vcpu->arch.guest_fpu.state.xsave, XFEATURE_MASK_BNDCSR);
>> >> +             if (mpx_state_buffer)
>> >> +                     memset(mpx_state_buffer, 0, sizeof(u64));
>> >
>> > XFEATURE_MASK_BNDCSR is actually
>> >
>> >   struct mpx_bndcsr {
>> >         u64 bndcfgu;
>> >         u64 bndstatus;
>> >   } __packed;
>> >
>> > So clearing two u64 would be correct, thanks.
>>
>> SDM section 9.1.1 just mentioned BNDCFGU will be cleared after
>> RESET/INIT, but not mentioned BNDSTATUS.
>
> That section conspires ;) I found a mention in 17.3.3 Configuration and
> Status Registers:
>
>   RESET or INIT# will set BNDCFGx and BNDSTATUS registers to zero.

Agreed.

Regards,
Wanpeng Li