| Message ID | 20190813131303.137684-1-nikita.leshchenko@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: nVMX: Check that HLT activity state is supported | expand |
On Tue, Aug 13, 2019 at 04:13:03PM +0300, Nikita Leshenko wrote: > Fail VM entry if GUEST_ACTIVITY_HLT is unsupported. According to "SDM A.6 - > Miscellaneous Data", VM entry should fail if the HLT activity is not marked as > supported on IA32_VMX_MISC MSR. > > Usermode might disable GUEST_ACTIVITY_HLT support in the vCPU with > vmx_restore_vmx_misc(). Before this commit VM entries would have succeeded > anyway. Is there a use case for disabling GUEST_ACTIVITY_HLT? Or can we simply disallow writes to IA32_VMX_MISC that disable GUEST_ACTIVITY_HLT? To disable GUEST_ACTIVITY_HLT, userspace also has to make CPU_BASED_HLT_EXITING a "must be 1" control, otherwise KVM will be presenting a bogus model to L1. The bad model is visible to L1 if CPU_BASED_HLT_EXITING is set by L0, i.e. KVM is running without kvm_hlt_in_guest(), and cleared by L1. In that case, a HLT from L2 will be handled in L0. L0 will set the state to KVM_MP_STATE_HALTED and report to L1 (on a nested VM-Exit, e.g. INTR), that the activity state is GUEST_ACTIVITY_HLT, which from L1's perspective doesn't exist. > Reviewed-by: Liran Alon <liran.alon@oracle.com> > Reviewed-by: Joao Martins <joao.m.martins@oracle.com> > Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com> > --- > arch/x86/kvm/vmx/nested.c | 16 ++++++++++++---- > arch/x86/kvm/vmx/nested.h | 5 +++++ > 2 files changed, 17 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > index 46af3a5e9209..3165e2f7992f 100644 > --- a/arch/x86/kvm/vmx/nested.c > +++ b/arch/x86/kvm/vmx/nested.c > @@ -2656,11 +2656,19 @@ static int nested_vmx_check_vmcs_link_ptr(struct kvm_vcpu *vcpu, > /* > * Checks related to Guest Non-register State > */ > -static int nested_check_guest_non_reg_state(struct vmcs12 *vmcs12) > +static int nested_check_guest_non_reg_state(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) > { > - if (vmcs12->guest_activity_state != GUEST_ACTIVITY_ACTIVE && > - vmcs12->guest_activity_state != GUEST_ACTIVITY_HLT) > + switch (vmcs12->guest_activity_state) { > + case GUEST_ACTIVITY_ACTIVE: > + /* Always supported */ > + break; > + case GUEST_ACTIVITY_HLT: > + if (!nested_cpu_has_activity_state_hlt(vcpu)) > + return -EINVAL; > + break; > + default: > return -EINVAL; > + } > > return 0; > } > @@ -2710,7 +2718,7 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu, > (vmcs12->guest_bndcfgs & MSR_IA32_BNDCFGS_RSVD))) > return -EINVAL; > > - if (nested_check_guest_non_reg_state(vmcs12)) > + if (nested_check_guest_non_reg_state(vcpu, vmcs12)) > return -EINVAL; > > return 0; > diff --git a/arch/x86/kvm/vmx/nested.h b/arch/x86/kvm/vmx/nested.h > index e847ff1019a2..4a294d3ff820 100644 > --- a/arch/x86/kvm/vmx/nested.h > +++ b/arch/x86/kvm/vmx/nested.h > @@ -123,6 +123,11 @@ static inline bool nested_cpu_has_zero_length_injection(struct kvm_vcpu *vcpu) > return to_vmx(vcpu)->nested.msrs.misc_low & VMX_MISC_ZERO_LEN_INS; > } > > +static inline bool nested_cpu_has_activity_state_hlt(struct kvm_vcpu *vcpu) > +{ > + return to_vmx(vcpu)->nested.msrs.misc_low & VMX_MISC_ACTIVITY_HLT; > +} > + > static inline bool nested_cpu_supports_monitor_trap_flag(struct kvm_vcpu *vcpu) > { > return to_vmx(vcpu)->nested.msrs.procbased_ctls_high & > -- > 2.20.1 >
> On 13 Aug 2019, at 17:35, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > On Tue, Aug 13, 2019 at 04:13:03PM +0300, Nikita Leshenko wrote: >> Fail VM entry if GUEST_ACTIVITY_HLT is unsupported. According to "SDM A.6 - >> Miscellaneous Data", VM entry should fail if the HLT activity is not marked as >> supported on IA32_VMX_MISC MSR. >> >> Usermode might disable GUEST_ACTIVITY_HLT support in the vCPU with >> vmx_restore_vmx_misc(). Before this commit VM entries would have succeeded >> anyway. > > Is there a use case for disabling GUEST_ACTIVITY_HLT? Or can we simply > disallow writes to IA32_VMX_MISC that disable GUEST_ACTIVITY_HLT? I don't have a specific case for disabling it. Disallowing turning it off in IA32_VMX_MISC is also possible. Because of what you said below I will submit a different patch that does so. Nikita > > To disable GUEST_ACTIVITY_HLT, userspace also has to make > CPU_BASED_HLT_EXITING a "must be 1" control, otherwise KVM will be > presenting a bogus model to L1. > > The bad model is visible to L1 if CPU_BASED_HLT_EXITING is set by L0, > i.e. KVM is running without kvm_hlt_in_guest(), and cleared by L1. In > that case, a HLT from L2 will be handled in L0. L0 will set the state to > KVM_MP_STATE_HALTED and report to L1 (on a nested VM-Exit, e.g. INTR), > that the activity state is GUEST_ACTIVITY_HLT, which from L1's perspective > doesn't exist. > >> Reviewed-by: Liran Alon <liran.alon@oracle.com> >> Reviewed-by: Joao Martins <joao.m.martins@oracle.com> >> Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com> >> --- >> arch/x86/kvm/vmx/nested.c | 16 ++++++++++++---- >> arch/x86/kvm/vmx/nested.h | 5 +++++ >> 2 files changed, 17 insertions(+), 4 deletions(-) >> >> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c >> index 46af3a5e9209..3165e2f7992f 100644 >> --- a/arch/x86/kvm/vmx/nested.c >> +++ b/arch/x86/kvm/vmx/nested.c >> @@ -2656,11 +2656,19 @@ static int nested_vmx_check_vmcs_link_ptr(struct kvm_vcpu *vcpu, >> /* >> * Checks related to Guest Non-register State >> */ >> -static int nested_check_guest_non_reg_state(struct vmcs12 *vmcs12) >> +static int nested_check_guest_non_reg_state(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) >> { >> - if (vmcs12->guest_activity_state != GUEST_ACTIVITY_ACTIVE && >> - vmcs12->guest_activity_state != GUEST_ACTIVITY_HLT) >> + switch (vmcs12->guest_activity_state) { >> + case GUEST_ACTIVITY_ACTIVE: >> + /* Always supported */ >> + break; >> + case GUEST_ACTIVITY_HLT: >> + if (!nested_cpu_has_activity_state_hlt(vcpu)) >> + return -EINVAL; >> + break; >> + default: >> return -EINVAL; >> + } >> >> return 0; >> } >> @@ -2710,7 +2718,7 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu, >> (vmcs12->guest_bndcfgs & MSR_IA32_BNDCFGS_RSVD))) >> return -EINVAL; >> >> - if (nested_check_guest_non_reg_state(vmcs12)) >> + if (nested_check_guest_non_reg_state(vcpu, vmcs12)) >> return -EINVAL; >> >> return 0; >> diff --git a/arch/x86/kvm/vmx/nested.h b/arch/x86/kvm/vmx/nested.h >> index e847ff1019a2..4a294d3ff820 100644 >> --- a/arch/x86/kvm/vmx/nested.h >> +++ b/arch/x86/kvm/vmx/nested.h >> @@ -123,6 +123,11 @@ static inline bool nested_cpu_has_zero_length_injection(struct kvm_vcpu *vcpu) >> return to_vmx(vcpu)->nested.msrs.misc_low & VMX_MISC_ZERO_LEN_INS; >> } >> >> +static inline bool nested_cpu_has_activity_state_hlt(struct kvm_vcpu *vcpu) >> +{ >> + return to_vmx(vcpu)->nested.msrs.misc_low & VMX_MISC_ACTIVITY_HLT; >> +} >> + >> static inline bool nested_cpu_supports_monitor_trap_flag(struct kvm_vcpu *vcpu) >> { >> return to_vmx(vcpu)->nested.msrs.procbased_ctls_high & >> -- >> 2.20.1 >>
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index 46af3a5e9209..3165e2f7992f 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -2656,11 +2656,19 @@ static int nested_vmx_check_vmcs_link_ptr(struct kvm_vcpu *vcpu, /* * Checks related to Guest Non-register State */ -static int nested_check_guest_non_reg_state(struct vmcs12 *vmcs12) +static int nested_check_guest_non_reg_state(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) { - if (vmcs12->guest_activity_state != GUEST_ACTIVITY_ACTIVE && - vmcs12->guest_activity_state != GUEST_ACTIVITY_HLT) + switch (vmcs12->guest_activity_state) { + case GUEST_ACTIVITY_ACTIVE: + /* Always supported */ + break; + case GUEST_ACTIVITY_HLT: + if (!nested_cpu_has_activity_state_hlt(vcpu)) + return -EINVAL; + break; + default: return -EINVAL; + } return 0; } @@ -2710,7 +2718,7 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu, (vmcs12->guest_bndcfgs & MSR_IA32_BNDCFGS_RSVD))) return -EINVAL; - if (nested_check_guest_non_reg_state(vmcs12)) + if (nested_check_guest_non_reg_state(vcpu, vmcs12)) return -EINVAL; return 0; diff --git a/arch/x86/kvm/vmx/nested.h b/arch/x86/kvm/vmx/nested.h index e847ff1019a2..4a294d3ff820 100644 --- a/arch/x86/kvm/vmx/nested.h +++ b/arch/x86/kvm/vmx/nested.h @@ -123,6 +123,11 @@ static inline bool nested_cpu_has_zero_length_injection(struct kvm_vcpu *vcpu) return to_vmx(vcpu)->nested.msrs.misc_low & VMX_MISC_ZERO_LEN_INS; } +static inline bool nested_cpu_has_activity_state_hlt(struct kvm_vcpu *vcpu) +{ + return to_vmx(vcpu)->nested.msrs.misc_low & VMX_MISC_ACTIVITY_HLT; +} + static inline bool nested_cpu_supports_monitor_trap_flag(struct kvm_vcpu *vcpu) { return to_vmx(vcpu)->nested.msrs.procbased_ctls_high &