diff mbox series

[v2,6/6] nVMX x86: Re-organize the code in nested_check_vmentry_prereqs(), related to Guest Non-Register State

Message ID 20181115054554.4831-7-krish.sadhukhan@oracle.com (mailing list archive)
State New, archived
Headers show
Series [v2,1/6] nVMX x86: Re-name check_vmentry_pre[post]reqs to nested_check_vmentry_pre[post]reqs | expand

Commit Message

Krish Sadhukhan Nov. 15, 2018, 5:45 a.m. UTC 
Separate out the checks in nested_check_vmentry_prereqs(), that are related
to Guest Non-Register State, to a separate function. The re-organized code is
easier for readability, enhancement and maintenance.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com>
Reviewed-by: Mark Kanda <mark.kanda@oracle.com>
---
 arch/x86/kvm/vmx.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

Comments

Sean Christopherson Nov. 15, 2018, 6:08 p.m. UTC | #1 
On Thu, Nov 15, 2018 at 12:45:54AM -0500, Krish Sadhukhan wrote:
> Separate out the checks in nested_check_vmentry_prereqs(), that are related
> to Guest Non-Register State, to a separate function. The re-organized code is
> easier for readability, enhancement and maintenance.
> 
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com>
> Reviewed-by: Mark Kanda <mark.kanda@oracle.com>
> ---
>  arch/x86/kvm/vmx.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 93e49aa..d60c529 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -12531,12 +12531,21 @@ static int nested_check_host_ctl_regs(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs
>  	return 0;
>  }
>  
> -static int nested_check_vmentry_prereqs(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
> +/*
> + * Checks related to Guest Non-register State
> + */
> +static int nested_check_guest_non_reg_state(struct vmcs12 *vmcs12)
>  {
> +	/* Failed "Guest Activity State" checks cause VMExit */

If you're going to add a comment it should either be a "TODO" or explain
that KVM diverges from the SDM for an unknown reason, or better yet find
someone that can explain why KVM diverges :).  My guess is it's
intentional as KVM is preventing WFS and Shutdown states, which also
diverges from the SDM.

Stating that the checks cause VMExits but not actually signalling a VMExit
makes it look like you introduced the bug.

>  	if (vmcs12->guest_activity_state != GUEST_ACTIVITY_ACTIVE &&
>  	    vmcs12->guest_activity_state != GUEST_ACTIVITY_HLT)
> -		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> +		return -EINVAL;
> +
> +	return 0;
> +}
>  
> +static int nested_check_vmentry_prereqs(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
> +{
>  	if (nested_check_vm_execution_ctls(vcpu, vmcs12) ||
>  	    nested_check_vm_exit_ctls(vcpu, vmcs12) ||
>  	    nested_check_vm_entry_ctls(vcpu, vmcs12))
> @@ -12545,6 +12554,9 @@ static int nested_check_vmentry_prereqs(struct kvm_vcpu *vcpu, struct vmcs12 *vm
>  	if (nested_check_host_ctl_regs(vcpu, vmcs12))
>  		return VMXERR_ENTRY_INVALID_HOST_STATE_FIELD;
>  
> +	if (nested_check_guest_non_reg_state(vmcs12))
> +		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> +
>  	return 0;
>  }
>  
> -- 
> 2.9.5
>
Jim Mattson Nov. 15, 2018, 6:24 p.m. UTC | #2 
On Thu, Nov 15, 2018 at 10:08 AM, Sean Christopherson
<sean.j.christopherson@intel.com> wrote:

> If you're going to add a comment it should either be a "TODO" or explain
> that KVM diverges from the SDM for an unknown reason, or better yet find
> someone that can explain why KVM diverges :).  My guess is it's
> intentional as KVM is preventing WFS and Shutdown states, which also
> diverges from the SDM.

The SDM allows for the CPU to enumerate the supported non-active guest
activity states in the IA32_VMX_MISC MSR. KVM does not currently
enumerate support for shutdown or WFS. The code for the VM-entry check
shouldn't implicitly know this, though, particularly since userspace
can mask off supported VT-x features by modifying the default VMX
capability MSRs. This code should check whether the
vmcs12->guest_activity_state is either GUEST_ACTIVITY_ACTIVE or one of
the supported states enumerated by vmx->nested.msrs.misc_low (bits 6,
7, and 8).
diff mbox series

Patch

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 93e49aa..d60c529 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -12531,12 +12531,21 @@  static int nested_check_host_ctl_regs(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs
 	return 0;
 }
 
-static int nested_check_vmentry_prereqs(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
+/*
+ * Checks related to Guest Non-register State
+ */
+static int nested_check_guest_non_reg_state(struct vmcs12 *vmcs12)
 {
+	/* Failed "Guest Activity State" checks cause VMExit */
 	if (vmcs12->guest_activity_state != GUEST_ACTIVITY_ACTIVE &&
 	    vmcs12->guest_activity_state != GUEST_ACTIVITY_HLT)
-		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
+		return -EINVAL;
+
+	return 0;
+}
 
+static int nested_check_vmentry_prereqs(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
+{
 	if (nested_check_vm_execution_ctls(vcpu, vmcs12) ||
 	    nested_check_vm_exit_ctls(vcpu, vmcs12) ||
 	    nested_check_vm_entry_ctls(vcpu, vmcs12))
@@ -12545,6 +12554,9 @@  static int nested_check_vmentry_prereqs(struct kvm_vcpu *vcpu, struct vmcs12 *vm
 	if (nested_check_host_ctl_regs(vcpu, vmcs12))
 		return VMXERR_ENTRY_INVALID_HOST_STATE_FIELD;
 
+	if (nested_check_guest_non_reg_state(vmcs12))
+		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
+
 	return 0;
 }