diff mbox series

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

Message ID 20190819214650.41991-3-nikita.leshchenko@oracle.com (mailing list archive)
State New, archived
Headers show
Series KVM: nVMX: Improve HLT activity support | expand

Commit Message

Nikita Leshenko Aug. 19, 2019, 9:46 p.m. UTC 
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(+)

Comments

Liran Alon Aug. 19, 2019, 10:44 p.m. UTC | #1 
> 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
>
Sean Christopherson Aug. 19, 2019, 11:35 p.m. UTC | #2 
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
>
diff mbox series

Patch

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,