[PATCH 0/2] KVM: nVMX: Improve HLT activity support

[PATCH 0/2] KVM: nVMX: Improve HLT activity support

[Nikita Leshenko]

These patches improve various aspects of nested HLT activity state support. The
first patch prevents usermode from turning off the feature bit in VMX_MISC MSR
and the second patch adds additional activity state related checks on VMCS12 as
required by SDM.

These patches were tested to not cause regressions in kvm-unit-tests.

Nikita

[PATCH 1/2] KVM: nVMX: Always indicate HLT activity support in VMX_MISC MSR

[Nikita Leshenko]

Before this commit, userspace could disable the GUEST_ACTIVITY_HLT bit in
VMX_MISC yet KVM would happily accept GUEST_ACTIVITY_HLT activity state in
VMCS12. We can fix it by either failing VM entries with HLT activity state when
it's not supported or by disallowing clearing this bit.

The latter is preferable. If we go with the former, 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.

Don't fail writes that disable GUEST_ACTIVITY_HLT to maintain backwards
compatibility.

Reviewed-by: Liran Alon <liran.alon@oracle.com>
Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
---
 arch/x86/kvm/vmx/nested.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 46af3a5e9209..24734946ec75 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -1102,6 +1102,14 @@ static int vmx_restore_vmx_misc(struct vcpu_vmx *vmx, u64 data)
 	if (vmx_misc_mseg_revid(data) != vmx_misc_mseg_revid(vmx_misc))
 		return -EINVAL;
 
+	/*
+	 * We always support HLT activity state. In the past it was possible to
+	 * turn HLT bit off (without actually turning off HLT activity state
+	 * support) so we don't fail vmx_restore_vmx_misc if this bit is turned
+	 * off.
+	 */
+	data |= VMX_MISC_ACTIVITY_HLT;
+
 	vmx->nested.msrs.misc_low = data;
 	vmx->nested.msrs.misc_high = data >> 32;
 
-- 
2.20.1

[PATCH 2/2] KVM: nVMX: Check guest activity state on vmentry of nested guests

[Nikita Leshenko]

The checks are written in the same order and structure as they appear in "SDM
26.3.1.5 - Checks on Guest Non-Register State", to ease verification.

Reviewed-by: Liran Alon <liran.alon@oracle.com>
Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
---
 arch/x86/kvm/vmx/nested.c | 24 ++++++++++++++++++++++++
 arch/x86/kvm/vmx/vmcs.h   | 13 +++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 24734946ec75..e2ee217f8ffe 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2666,10 +2666,34 @@ static int nested_vmx_check_vmcs_link_ptr(struct kvm_vcpu *vcpu,
  */
 static int nested_check_guest_non_reg_state(struct vmcs12 *vmcs12)
 {
+	/* Activity state must contain supported value */
 	if (vmcs12->guest_activity_state != GUEST_ACTIVITY_ACTIVE &&
 	    vmcs12->guest_activity_state != GUEST_ACTIVITY_HLT)
 		return -EINVAL;
 
+	/* Must not be HLT if SS DPL is not 0 */
+	if (VMX_AR_DPL(vmcs12->guest_ss_ar_bytes) != 0 &&
+	    vmcs12->guest_activity_state == GUEST_ACTIVITY_HLT)
+		return -EINVAL;
+
+	/* Must be active if blocking by MOV-SS or STI */
+	if ((vmcs12->guest_interruptibility_info &
+	    (GUEST_INTR_STATE_MOV_SS | GUEST_INTR_STATE_STI)) &&
+	    vmcs12->guest_activity_state != GUEST_ACTIVITY_ACTIVE)
+		return -EINVAL;
+
+	/* In HLT, only some interruptions are allowed */
+	if (vmcs12->vm_entry_intr_info_field & INTR_INFO_VALID_MASK &&
+	    vmcs12->guest_activity_state == GUEST_ACTIVITY_HLT) {
+		u32 intr_info = vmcs12->vm_entry_intr_info_field;
+		if (!is_ext_interrupt(intr_info) &&
+		    !is_nmi(intr_info) &&
+		    !is_debug(intr_info) &&
+		    !is_machine_check(intr_info) &&
+		    !is_mtf(intr_info))
+		    return -EINVAL;
+	}
+
 	return 0;
 }
 
diff --git a/arch/x86/kvm/vmx/vmcs.h b/arch/x86/kvm/vmx/vmcs.h
index cb6079f8a227..c5577c40b19d 100644
--- a/arch/x86/kvm/vmx/vmcs.h
+++ b/arch/x86/kvm/vmx/vmcs.h
@@ -102,6 +102,13 @@ static inline bool is_machine_check(u32 intr_info)
 		(INTR_TYPE_HARD_EXCEPTION | MC_VECTOR | INTR_INFO_VALID_MASK);
 }
 
+static inline bool is_mtf(u32 intr_info)
+{
+	return (intr_info & (INTR_INFO_INTR_TYPE_MASK | INTR_INFO_VECTOR_MASK |
+			     INTR_INFO_VALID_MASK)) ==
+		(INTR_TYPE_OTHER_EVENT | 0 | INTR_INFO_VALID_MASK);
+}
+
 /* Undocumented: icebp/int1 */
 static inline bool is_icebp(u32 intr_info)
 {
@@ -115,6 +122,12 @@ static inline bool is_nmi(u32 intr_info)
 		== (INTR_TYPE_NMI_INTR | INTR_INFO_VALID_MASK);
 }
 
+static inline bool is_ext_interrupt(u32 intr_info)
+{
+	return (intr_info & (INTR_INFO_INTR_TYPE_MASK | INTR_INFO_VALID_MASK))
+		== (INTR_TYPE_EXT_INTR | INTR_INFO_VALID_MASK);
+}
+
 enum vmcs_field_width {
 	VMCS_FIELD_WIDTH_U16 = 0,
 	VMCS_FIELD_WIDTH_U64 = 1,
-- 
2.20.1

Re: [PATCH 1/2] KVM: nVMX: Always indicate HLT activity support in VMX_MISC MSR

[Sean Christopherson]

On Tue, Aug 20, 2019 at 12:46:49AM +0300, Nikita Leshenko wrote:
> Before this commit, userspace could disable the GUEST_ACTIVITY_HLT bit in
> VMX_MISC yet KVM would happily accept GUEST_ACTIVITY_HLT activity state in
> VMCS12. We can fix it by either failing VM entries with HLT activity state when
> it's not supported or by disallowing clearing this bit.
> 
> The latter is preferable. If we go with the former, 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.
> 
> Don't fail writes that disable GUEST_ACTIVITY_HLT to maintain backwards
> compatibility.

Paolo, do we actually need to maintain backwards compatibility in this
case?  This seems like a good candidate for "fix the bug and see who yells".

> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 46af3a5e9209..24734946ec75 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -1102,6 +1102,14 @@ static int vmx_restore_vmx_misc(struct vcpu_vmx *vmx, u64 data)
>  	if (vmx_misc_mseg_revid(data) != vmx_misc_mseg_revid(vmx_misc))
>  		return -EINVAL;
>  
> +	/*
> +	 * We always support HLT activity state. In the past it was possible to
> +	 * turn HLT bit off (without actually turning off HLT activity state
> +	 * support) so we don't fail vmx_restore_vmx_misc if this bit is turned
> +	 * off.
> +	 */
> +	data |= VMX_MISC_ACTIVITY_HLT;
> +
>  	vmx->nested.msrs.misc_low = data;
>  	vmx->nested.msrs.misc_high = data >> 32;
>  
> -- 
> 2.20.1
> 

Re: [PATCH 2/2] KVM: nVMX: Check guest activity state on vmentry of nested guests

[Liran Alon]

> On 20 Aug 2019, at 0:46, Nikita Leshenko <nikita.leshchenko@oracle.com> wrote:
> 
> The checks are written in the same order and structure as they appear in "SDM
> 26.3.1.5 - Checks on Guest Non-Register State", to ease verification.
> 
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> ---
> arch/x86/kvm/vmx/nested.c | 24 ++++++++++++++++++++++++
> arch/x86/kvm/vmx/vmcs.h   | 13 +++++++++++++
> 2 files changed, 37 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 24734946ec75..e2ee217f8ffe 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2666,10 +2666,34 @@ static int nested_vmx_check_vmcs_link_ptr(struct kvm_vcpu *vcpu,
>  */
> static int nested_check_guest_non_reg_state(struct vmcs12 *vmcs12)
> {
> +	/* Activity state must contain supported value */
> 	if (vmcs12->guest_activity_state != GUEST_ACTIVITY_ACTIVE &&
> 	    vmcs12->guest_activity_state != GUEST_ACTIVITY_HLT)
> 		return -EINVAL;
> 
> +	/* Must not be HLT if SS DPL is not 0 */
> +	if (VMX_AR_DPL(vmcs12->guest_ss_ar_bytes) != 0 &&
> +	    vmcs12->guest_activity_state == GUEST_ACTIVITY_HLT)
> +		return -EINVAL;
> +
> +	/* Must be active if blocking by MOV-SS or STI */
> +	if ((vmcs12->guest_interruptibility_info &
> +	    (GUEST_INTR_STATE_MOV_SS | GUEST_INTR_STATE_STI)) &&
> +	    vmcs12->guest_activity_state != GUEST_ACTIVITY_ACTIVE)
> +		return -EINVAL;
> +
> +	/* In HLT, only some interruptions are allowed */
> +	if (vmcs12->vm_entry_intr_info_field & INTR_INFO_VALID_MASK &&
> +	    vmcs12->guest_activity_state == GUEST_ACTIVITY_HLT) {
> +		u32 intr_info = vmcs12->vm_entry_intr_info_field;
> +		if (!is_ext_interrupt(intr_info) &&
> +		    !is_nmi(intr_info) &&
> +		    !is_debug(intr_info) &&
> +		    !is_machine_check(intr_info) &&
> +		    !is_mtf(intr_info))
> +		    return -EINVAL;
> +	}
> +
> 	return 0;
> }
> 
> diff --git a/arch/x86/kvm/vmx/vmcs.h b/arch/x86/kvm/vmx/vmcs.h
> index cb6079f8a227..c5577c40b19d 100644
> --- a/arch/x86/kvm/vmx/vmcs.h
> +++ b/arch/x86/kvm/vmx/vmcs.h
> @@ -102,6 +102,13 @@ static inline bool is_machine_check(u32 intr_info)
> 		(INTR_TYPE_HARD_EXCEPTION | MC_VECTOR | INTR_INFO_VALID_MASK);
> }
> 
> +static inline bool is_mtf(u32 intr_info)
> +{
> +	return (intr_info & (INTR_INFO_INTR_TYPE_MASK | INTR_INFO_VECTOR_MASK |
> +			     INTR_INFO_VALID_MASK)) ==
> +		(INTR_TYPE_OTHER_EVENT | 0 | INTR_INFO_VALID_MASK);
> +}
> +
> /* Undocumented: icebp/int1 */
> static inline bool is_icebp(u32 intr_info)
> {
> @@ -115,6 +122,12 @@ static inline bool is_nmi(u32 intr_info)
> 		== (INTR_TYPE_NMI_INTR | INTR_INFO_VALID_MASK);
> }
> 
> +static inline bool is_ext_interrupt(u32 intr_info)
> +{
> +	return (intr_info & (INTR_INFO_INTR_TYPE_MASK | INTR_INFO_VALID_MASK))
> +		== (INTR_TYPE_EXT_INTR | INTR_INFO_VALID_MASK);
> +}

is_external_intr() already exists. You should just use that.

> +
> enum vmcs_field_width {
> 	VMCS_FIELD_WIDTH_U16 = 0,
> 	VMCS_FIELD_WIDTH_U64 = 1,
> -- 
> 2.20.1
> 

Re: [PATCH 2/2] KVM: nVMX: Check guest activity state on vmentry of nested guests

[Sean Christopherson]

[-- Attachment #1: Type: text/plain, Size: 3925 bytes --]

On Tue, Aug 20, 2019 at 12:46:50AM +0300, Nikita Leshenko wrote:
> The checks are written in the same order and structure as they appear in "SDM
> 26.3.1.5 - Checks on Guest Non-Register State", to ease verification.
> 
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 24 ++++++++++++++++++++++++
>  arch/x86/kvm/vmx/vmcs.h   | 13 +++++++++++++
>  2 files changed, 37 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 24734946ec75..e2ee217f8ffe 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2666,10 +2666,34 @@ static int nested_vmx_check_vmcs_link_ptr(struct kvm_vcpu *vcpu,
>   */
>  static int nested_check_guest_non_reg_state(struct vmcs12 *vmcs12)
>  {
> +	/* Activity state must contain supported value */
>  	if (vmcs12->guest_activity_state != GUEST_ACTIVITY_ACTIVE &&
>  	    vmcs12->guest_activity_state != GUEST_ACTIVITY_HLT)
>  		return -EINVAL;
>  
> +	/* Must not be HLT if SS DPL is not 0 */
> +	if (VMX_AR_DPL(vmcs12->guest_ss_ar_bytes) != 0 &&
> +	    vmcs12->guest_activity_state == GUEST_ACTIVITY_HLT)
> +		return -EINVAL;
> +
> +	/* Must be active if blocking by MOV-SS or STI */
> +	if ((vmcs12->guest_interruptibility_info &
> +	    (GUEST_INTR_STATE_MOV_SS | GUEST_INTR_STATE_STI)) &&
> +	    vmcs12->guest_activity_state != GUEST_ACTIVITY_ACTIVE)

IMO, following the SDM verbatim doesn't help readability in this case.
E.g. filtering out ACTIVE state right away cuts down on the amount of
code and helps the reader focus on the unique aspects of each check.

	if (vmcs12->guest_activity_state == GUEST_ACTIVITY_ACTIVE)
		return 0;

> +		return -EINVAL;
> +
> +	/* In HLT, only some interruptions are allowed */
> +	if (vmcs12->vm_entry_intr_info_field & INTR_INFO_VALID_MASK &&
> +	    vmcs12->guest_activity_state == GUEST_ACTIVITY_HLT) {
> +		u32 intr_info = vmcs12->vm_entry_intr_info_field;
> +		if (!is_ext_interrupt(intr_info) &&
> +		    !is_nmi(intr_info) &&
> +		    !is_debug(intr_info) &&
> +		    !is_machine_check(intr_info) &&
> +		    !is_mtf(intr_info))
> +		    return -EINVAL;

Bad indentation.  scripts/checkpatch.pl will catch this.  It'll also
complain about the changelog running past 75 chars and about "Missing a
blank line after declarations" for u32 intr_info, both of which are
easy nits to fix.

> +	}
> +
>  	return 0;
>  }
>  
> diff --git a/arch/x86/kvm/vmx/vmcs.h b/arch/x86/kvm/vmx/vmcs.h
> index cb6079f8a227..c5577c40b19d 100644
> --- a/arch/x86/kvm/vmx/vmcs.h
> +++ b/arch/x86/kvm/vmx/vmcs.h
> @@ -102,6 +102,13 @@ static inline bool is_machine_check(u32 intr_info)
>  		(INTR_TYPE_HARD_EXCEPTION | MC_VECTOR | INTR_INFO_VALID_MASK);
>  }
>  
> +static inline bool is_mtf(u32 intr_info)

We may want to call this is_pending_mtf().  MTF has a dedicated VM-Exit
type and doesn't populate VM_EXIT_INTRO_INFO, while all of the other
helpers are valid for both VM-Enter and VM-Exit.

> +{
> +	return (intr_info & (INTR_INFO_INTR_TYPE_MASK | INTR_INFO_VECTOR_MASK |
> +			     INTR_INFO_VALID_MASK)) ==
> +		(INTR_TYPE_OTHER_EVENT | 0 | INTR_INFO_VALID_MASK);

This reminded me a patch I've been meaning to write.  Any objection to
carrying the attached patch as cleanup and reworking this code accordingly?

> +}
> +
>  /* Undocumented: icebp/int1 */
>  static inline bool is_icebp(u32 intr_info)
>  {
> @@ -115,6 +122,12 @@ static inline bool is_nmi(u32 intr_info)
>  		== (INTR_TYPE_NMI_INTR | INTR_INFO_VALID_MASK);
>  }
>  
> +static inline bool is_ext_interrupt(u32 intr_info)
> +{
> +	return (intr_info & (INTR_INFO_INTR_TYPE_MASK | INTR_INFO_VALID_MASK))
> +		== (INTR_TYPE_EXT_INTR | INTR_INFO_VALID_MASK);
> +}
> +
>  enum vmcs_field_width {
>  	VMCS_FIELD_WIDTH_U16 = 0,
>  	VMCS_FIELD_WIDTH_U64 = 1,
> -- 
> 2.20.1
> 

[-- Attachment #2: 0001-KVM-VMX-Add-helpers-to-identify-interrupt-type-from-.patch --]
[-- Type: text/x-diff, Size: 2823 bytes --]

From 588a85e32e3d2ed71712a21557ba9baf0e3aa25c Mon Sep 17 00:00:00 2001
From: Sean Christopherson <sean.j.christopherson@intel.com>
Date: Mon, 19 Aug 2019 16:14:01 -0700
Subject: [PATCH] KVM: VMX: Add helpers to identify interrupt type from
 intr_info

Add is_intr_type() and is_intr_type_n() to consolidate the boilerplate
code for querying a specific type of interrupt given an encoded value
from VMCS.VM_{ENTER,EXIT}_INTR_INFO, with and without an associated
vector respectively.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx/vmcs.h | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmcs.h b/arch/x86/kvm/vmx/vmcs.h
index 481ad879197b..dcf21cca160b 100644
--- a/arch/x86/kvm/vmx/vmcs.h
+++ b/arch/x86/kvm/vmx/vmcs.h
@@ -72,11 +72,22 @@ struct loaded_vmcs {
 	struct vmcs_controls_shadow controls_shadow;
 };
 
+static inline bool is_intr_type(u32 intr_info, u32 type)
+{
+	return (intr_info & (INTR_INFO_VALID_MASK | INTR_INFO_INTR_TYPE_MASK))
+		== (INTR_INFO_VALID_MASK | type);
+}
+
+static inline bool is_intr_type_n(u32 intr_info, u32 type, u8 vector)
+{
+	return (intr_info & (INTR_INFO_VALID_MASK | INTR_INFO_INTR_TYPE_MASK |
+			     INTR_INFO_VECTOR_MASK)) ==
+		(INTR_INFO_VALID_MASK | type | vector);
+}
+
 static inline bool is_exception_n(u32 intr_info, u8 vector)
 {
-	return (intr_info & (INTR_INFO_INTR_TYPE_MASK | INTR_INFO_VECTOR_MASK |
-			     INTR_INFO_VALID_MASK)) ==
-		(INTR_TYPE_HARD_EXCEPTION | vector | INTR_INFO_VALID_MASK);
+	return is_intr_type_n(intr_info, INTR_TYPE_HARD_EXCEPTION, vector);
 }
 
 static inline bool is_debug(u32 intr_info)
@@ -106,28 +117,23 @@ static inline bool is_gp_fault(u32 intr_info)
 
 static inline bool is_machine_check(u32 intr_info)
 {
-	return (intr_info & (INTR_INFO_INTR_TYPE_MASK | INTR_INFO_VECTOR_MASK |
-			     INTR_INFO_VALID_MASK)) ==
-		(INTR_TYPE_HARD_EXCEPTION | MC_VECTOR | INTR_INFO_VALID_MASK);
+	return is_exception_n(intr_info, MC_VECTOR);
 }
 
 /* Undocumented: icebp/int1 */
 static inline bool is_icebp(u32 intr_info)
 {
-	return (intr_info & (INTR_INFO_INTR_TYPE_MASK | INTR_INFO_VALID_MASK))
-		== (INTR_TYPE_PRIV_SW_EXCEPTION | INTR_INFO_VALID_MASK);
+	return is_intr_type(intr_info, INTR_TYPE_PRIV_SW_EXCEPTION);
 }
 
 static inline bool is_nmi(u32 intr_info)
 {
-	return (intr_info & (INTR_INFO_INTR_TYPE_MASK | INTR_INFO_VALID_MASK))
-		== (INTR_TYPE_NMI_INTR | INTR_INFO_VALID_MASK);
+	return is_intr_type(intr_info, INTR_TYPE_NMI_INTR);
 }
 
 static inline bool is_external_intr(u32 intr_info)
 {
-	return (intr_info & (INTR_INFO_VALID_MASK | INTR_INFO_INTR_TYPE_MASK))
-		== (INTR_INFO_VALID_MASK | INTR_TYPE_EXT_INTR);
+	return is_intr_type(intr_info, INTR_TYPE_EXT_INTR);
 }
 
 enum vmcs_field_width {
-- 
2.22.0

Re: [PATCH 1/2] KVM: nVMX: Always indicate HLT activity support in VMX_MISC MSR

[Jim Mattson]

On Mon, Aug 19, 2019 at 3:11 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Tue, Aug 20, 2019 at 12:46:49AM +0300, Nikita Leshenko wrote:
> > Before this commit, userspace could disable the GUEST_ACTIVITY_HLT bit in
> > VMX_MISC yet KVM would happily accept GUEST_ACTIVITY_HLT activity state in
> > VMCS12. We can fix it by either failing VM entries with HLT activity state when
> > it's not supported or by disallowing clearing this bit.
> >
> > The latter is preferable. If we go with the former, 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.
> >
> > Don't fail writes that disable GUEST_ACTIVITY_HLT to maintain backwards
> > compatibility.
>
> Paolo, do we actually need to maintain backwards compatibility in this
> case?  This seems like a good candidate for "fix the bug and see who yells".

Google's userspace clears bit 6. Please don't fail that write!

Re: [PATCH 1/2] KVM: nVMX: Always indicate HLT activity support in VMX_MISC MSR

[Sean Christopherson]

On Wed, Aug 21, 2019 at 01:59:20PM -0700, Jim Mattson wrote:
> On Mon, Aug 19, 2019 at 3:11 PM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> >
> > On Tue, Aug 20, 2019 at 12:46:49AM +0300, Nikita Leshenko wrote:
> > > Before this commit, userspace could disable the GUEST_ACTIVITY_HLT bit in
> > > VMX_MISC yet KVM would happily accept GUEST_ACTIVITY_HLT activity state in
> > > VMCS12. We can fix it by either failing VM entries with HLT activity state when
> > > it's not supported or by disallowing clearing this bit.
> > >
> > > The latter is preferable. If we go with the former, 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.
> > >
> > > Don't fail writes that disable GUEST_ACTIVITY_HLT to maintain backwards
> > > compatibility.
> >
> > Paolo, do we actually need to maintain backwards compatibility in this
> > case?  This seems like a good candidate for "fix the bug and see who yells".
> 
> Google's userspace clears bit 6. Please don't fail that write!

Booooo.

Requiring CPU_BASED_HLT_EXITING to be forced to 1 is probably off the
table since the bits are in two separate MSRs, i.e. we run into a
chicken-and-egg scenario.

Silently forcing GUEST_ACTIVITY_HLT seems wrong, especially if userspace
has also forced CPU_BASED_HLT_EXITING, e.g. KVM would be letting L1 do
something userspace explicitly asked KVM to prevent.

Generally speaking, KVM lets userspace do dumb things so long as it
doesn't break the kernel, so, what if we change sync_vmcs02_to_vmcs12()
to propagate GUEST_ACTIVITY_HLT to vmcs12 if and only if the activity
state exists?  That might be better than causing a nested VM-Enter to
fail?  I'm also a-ok doing nothing and fulling putting the onus on
userspace to get the config correct.

Re: [PATCH 1/2] KVM: nVMX: Always indicate HLT activity support in VMX_MISC MSR

[Jim Mattson]

On Wed, Aug 21, 2019 at 3:22 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Wed, Aug 21, 2019 at 01:59:20PM -0700, Jim Mattson wrote:
> > On Mon, Aug 19, 2019 at 3:11 PM Sean Christopherson
> > <sean.j.christopherson@intel.com> wrote:
> > >
> > > On Tue, Aug 20, 2019 at 12:46:49AM +0300, Nikita Leshenko wrote:
> > > > Before this commit, userspace could disable the GUEST_ACTIVITY_HLT bit in
> > > > VMX_MISC yet KVM would happily accept GUEST_ACTIVITY_HLT activity state in
> > > > VMCS12. We can fix it by either failing VM entries with HLT activity state when
> > > > it's not supported or by disallowing clearing this bit.
> > > >
> > > > The latter is preferable. If we go with the former, 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.
> > > >
> > > > Don't fail writes that disable GUEST_ACTIVITY_HLT to maintain backwards
> > > > compatibility.
> > >
> > > Paolo, do we actually need to maintain backwards compatibility in this
> > > case?  This seems like a good candidate for "fix the bug and see who yells".
> >
> > Google's userspace clears bit 6. Please don't fail that write!
>
> Booooo.
>

Supporting activity state HLT is on our list of things to do, but I'm
not convinced that kvm actually handles it properly yet. For
instance...

What happens if L1 launches L2 into activity state HLT with a
zero-valued VMX preemption timer? (Maybe this is fixed now?)
What happens if "monitor trap flag" is set and "HLT exiting" is clear
in the vmcs12, and immediately on VM-entry, L2 executes HLT? (Yes,
this is a special case of MTF being broken when L0 emulates an L2
instruction.)

I'm sure there are other interesting scenarios that haven't been validated.

Re: [PATCH 1/2] KVM: nVMX: Always indicate HLT activity support in VMX_MISC MSR

[Sean Christopherson]

On Wed, Aug 21, 2019 at 04:01:49PM -0700, Jim Mattson wrote:
> On Wed, Aug 21, 2019 at 3:22 PM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> >
> > On Wed, Aug 21, 2019 at 01:59:20PM -0700, Jim Mattson wrote:
> > > On Mon, Aug 19, 2019 at 3:11 PM Sean Christopherson
> > > <sean.j.christopherson@intel.com> wrote:
> > > >
> > > > On Tue, Aug 20, 2019 at 12:46:49AM +0300, Nikita Leshenko wrote:
> > > > > Before this commit, userspace could disable the GUEST_ACTIVITY_HLT bit in
> > > > > VMX_MISC yet KVM would happily accept GUEST_ACTIVITY_HLT activity state in
> > > > > VMCS12. We can fix it by either failing VM entries with HLT activity state when
> > > > > it's not supported or by disallowing clearing this bit.
> > > > >
> > > > > The latter is preferable. If we go with the former, 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.
> > > > >
> > > > > Don't fail writes that disable GUEST_ACTIVITY_HLT to maintain backwards
> > > > > compatibility.
> > > >
> > > > Paolo, do we actually need to maintain backwards compatibility in this
> > > > case?  This seems like a good candidate for "fix the bug and see who yells".
> > >
> > > Google's userspace clears bit 6. Please don't fail that write!
> >
> > Booooo.
> >
> 
> Supporting activity state HLT is on our list of things to do, but I'm
> not convinced that kvm actually handles it properly yet. For
> instance...

I fully understand why you'd want to hide it from L1, I was just bummed
that we couldn't go with a quick and dirty fix :-)

> What happens if L1 launches L2 into activity state HLT with a
> zero-valued VMX preemption timer? (Maybe this is fixed now?)

I think that one got fixed in vmx_start_preemption_timer().

> What happens if "monitor trap flag" is set and "HLT exiting" is clear
> in the vmcs12, and immediately on VM-entry, L2 executes HLT? (Yes,
> this is a special case of MTF being broken when L0 emulates an L2
> instruction.)
> 
> I'm sure there are other interesting scenarios that haven't been validated.

Re: [PATCH 1/2] KVM: nVMX: Always indicate HLT activity support in VMX_MISC MSR

[Jim Mattson]

On Mon, Aug 19, 2019 at 2:47 PM Nikita Leshenko
<nikita.leshchenko@oracle.com> wrote:
>
> Before this commit, userspace could disable the GUEST_ACTIVITY_HLT bit in
> VMX_MISC yet KVM would happily accept GUEST_ACTIVITY_HLT activity state in
> VMCS12. We can fix it by either failing VM entries with HLT activity state when
> it's not supported or by disallowing clearing this bit.
>
> The latter is preferable. If we go with the former, 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.
>
> Don't fail writes that disable GUEST_ACTIVITY_HLT to maintain backwards
> compatibility.
>
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 46af3a5e9209..24734946ec75 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -1102,6 +1102,14 @@ static int vmx_restore_vmx_misc(struct vcpu_vmx *vmx, u64 data)
>         if (vmx_misc_mseg_revid(data) != vmx_misc_mseg_revid(vmx_misc))
>                 return -EINVAL;
>
> +       /*
> +        * We always support HLT activity state. In the past it was possible to
> +        * turn HLT bit off (without actually turning off HLT activity state
> +        * support) so we don't fail vmx_restore_vmx_misc if this bit is turned
> +        * off.
> +        */
> +       data |= VMX_MISC_ACTIVITY_HLT;
> +
>         vmx->nested.msrs.misc_low = data;
>         vmx->nested.msrs.misc_high = data >> 32;
>

This change breaks live migration to an upgraded kernel, since it
doesn't allow the IA32_VMX_MISC MSR to be restored to its original
value. I think this warrants a quirk.

Re: [PATCH 1/2] KVM: nVMX: Always indicate HLT activity support in VMX_MISC MSR

[Nikita Leshenko]

> On 21 Aug 2019, at 23:59, Jim Mattson <jmattson@google.com> wrote:
> 
> On Mon, Aug 19, 2019 at 3:11 PM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
>> 
>> On Tue, Aug 20, 2019 at 12:46:49AM +0300, Nikita Leshenko wrote:
>>> Before this commit, userspace could disable the GUEST_ACTIVITY_HLT bit in
>>> VMX_MISC yet KVM would happily accept GUEST_ACTIVITY_HLT activity state in
>>> VMCS12. We can fix it by either failing VM entries with HLT activity state when
>>> it's not supported or by disallowing clearing this bit.
>>> 
>>> The latter is preferable. If we go with the former, 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.
>>> 
>>> Don't fail writes that disable GUEST_ACTIVITY_HLT to maintain backwards
>>> compatibility.
>> 
>> Paolo, do we actually need to maintain backwards compatibility in this
>> case?  This seems like a good candidate for "fix the bug and see who yells".
> 
> Google's userspace clears bit 6. Please don't fail that write!
What happens if the guest tries to use HLT activity state regardless of the bit?
Currently in KVM the guest will succeed, since there are no checks on VM entry for
that. I previously submitted a patch[1] that adds a check for that, what do you think
about it?

[1] https://patchwork.kernel.org/patch/11092209/

Nikita