Re: [PATCH v6 4/7] KVM: nVMX: Rename and split top-level consistency checks to match SDM

[Krish Sadhukhan <krish.sadhukhan@oracle.com>]

On 04/11/2019 12:18 PM, Sean Christopherson wrote:
> Rename the top-level consistency check functions to (loosely) align with
> the SDM.  Historically, KVM has used the terms "prereq" and "postreq" to
> differentiate between consistency checks that lead to VM-Fail and those
> that lead to VM-Exit.  The terms are vague and potentially misleading,
> e.g. "postreq" might be interpreted as occurring after VM-Entry.
>
> Note, while the SDM lumps controls and host state into a single section,
> "Checks on VMX Controls and Host-State Area", split them into separate
> top-level functions as the two categories of checks result in different
> VM instruction errors.  This split will allow for additional cleanup.
>
> Note #2, "vmentry" is intentionally dropped from the new function names
> to avoid confusion with nested_check_vm_entry_controls(), and to keep
> the length of the functions names somewhat manageable.
>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>   arch/x86/kvm/vmx/nested.c | 39 +++++++++++++++++++++++++--------------
>   1 file changed, 25 insertions(+), 14 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index fe1323ab6894..b22605d5ee9e 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2573,6 +2573,17 @@ static int nested_check_vm_entry_controls(struct kvm_vcpu *vcpu,
>   	return 0;
>   }
>   
> +static int nested_vmx_check_controls(struct kvm_vcpu *vcpu,
> +				     struct vmcs12 *vmcs12)
> +{
> +	if (nested_check_vm_execution_controls(vcpu, vmcs12) ||
> +	    nested_check_vm_exit_controls(vcpu, vmcs12) ||
> +	    nested_check_vm_entry_controls(vcpu, vmcs12))
> +		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> +
> +	return 0;
> +}
> +
>   /*
>    * Checks related to Host Control Registers and MSRs
>    */
> @@ -2612,14 +2623,9 @@ static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
>   	return 0;
>   }
>   
> -static int nested_vmx_check_vmentry_prereqs(struct kvm_vcpu *vcpu,
> -					    struct vmcs12 *vmcs12)
> +static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
> +				       struct vmcs12 *vmcs12)
>   {
> -	if (nested_check_vm_execution_controls(vcpu, vmcs12) ||
> -	    nested_check_vm_exit_controls(vcpu, vmcs12) ||
> -	    nested_check_vm_entry_controls(vcpu, vmcs12))
> -		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> -
>   	if (nested_check_host_control_regs(vcpu, vmcs12))
>   		return VMXERR_ENTRY_INVALID_HOST_STATE_FIELD;
>   
> @@ -2665,9 +2671,9 @@ static int nested_check_guest_non_reg_state(struct vmcs12 *vmcs12)
>   	return 0;
>   }
>   
> -static int nested_vmx_check_vmentry_postreqs(struct kvm_vcpu *vcpu,
> -					     struct vmcs12 *vmcs12,
> -					     u32 *exit_qual)
> +static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
> +					struct vmcs12 *vmcs12,
> +					u32 *exit_qual)
>   {
>   	bool ia32e;
>   
> @@ -2985,7 +2991,7 @@ int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu, bool from_vmentry)
>   			return -1;
>   		}
>   
> -		if (nested_vmx_check_vmentry_postreqs(vcpu, vmcs12, &exit_qual))
> +		if (nested_vmx_check_guest_state(vcpu, vmcs12, &exit_qual))
>   			goto vmentry_fail_vmexit;
>   	}
>   
> @@ -3130,7 +3136,11 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch)
>   			launch ? VMXERR_VMLAUNCH_NONCLEAR_VMCS
>   			       : VMXERR_VMRESUME_NONLAUNCHED_VMCS);
>   
> -	ret = nested_vmx_check_vmentry_prereqs(vcpu, vmcs12);
> +	ret = nested_vmx_check_controls(vcpu, vmcs12);
> +	if (ret)
> +		return nested_vmx_failValid(vcpu, ret);
> +
> +	ret = nested_vmx_check_host_state(vcpu, vmcs12);
>   	if (ret)
>   		return nested_vmx_failValid(vcpu, ret);
>   
> @@ -5455,8 +5465,9 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
>   			return -EINVAL;
>   	}
>   
> -	if (nested_vmx_check_vmentry_prereqs(vcpu, vmcs12) ||
> -	    nested_vmx_check_vmentry_postreqs(vcpu, vmcs12, &exit_qual))
> +	if (nested_vmx_check_controls(vcpu, vmcs12) ||
> +	    nested_vmx_check_host_state(vcpu, vmcs12) ||
> +	    nested_vmx_check_guest_state(vcpu, vmcs12, &exit_qual))
>   		return -EINVAL;
>   
>   	vmx->nested.dirty_vmcs12 = true;

It's probably a good idea to add a one-line comment above each of the 
top-level check functions.

Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>