diff mbox series

[1/7] KVM: nVMX: Prepend "nested_" to check_vmentry_{pre,post}reqs()

Message ID 20181130190438.13591-2-krish.sadhukhan@oracle.com (mailing list archive)
State New, archived
Headers show
Series [1/7] KVM: nVMX: Prepend "nested_" to check_vmentry_{pre,post}reqs() | expand

Commit Message

Krish Sadhukhan Nov. 30, 2018, 7:04 p.m. UTC 
.. as they are used only in nested context.

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

Comments

Jim Mattson Dec. 3, 2018, 7:18 p.m. UTC | #1 
On Fri, Nov 30, 2018 at 11:28 AM Krish Sadhukhan
<krish.sadhukhan@oracle.com> wrote:
>
> .. as they are used only in nested context.
>
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com>
> Reviewed-by: Mark Kanda <mark.kanda@oracle.com>

I'm not sure what you mean by "nested context." That sounds to me like
"while running L2." However, these functions are only used while
running L1. There are many, many functions that fall into the category
of "L1 execution related to VMX emulation" that don't have a "nested"
prefix (e.g. the entire prepare_vmcs02* family). I don't think this
additional prefix actually helps to clarify anything.
Krish Sadhukhan Dec. 3, 2018, 8:41 p.m. UTC | #2 
On 12/03/2018 11:18 AM, Jim Mattson wrote:
> On Fri, Nov 30, 2018 at 11:28 AM Krish Sadhukhan
> <krish.sadhukhan@oracle.com> wrote:
>> .. as they are used only in nested context.
>>
>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>> Reviewed-by: Liran Alon <liran.alon@oracle.com>
>> Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com>
>> Reviewed-by: Mark Kanda <mark.kanda@oracle.com>
> I'm not sure what you mean by "nested context." That sounds to me like
> "while running L2." However, these functions are only used while
> running L1. There are many, many functions that fall into the category
> of "L1 execution related to VMX emulation" that don't have a "nested"
> prefix (e.g. the entire prepare_vmcs02* family). I don't think this
> additional prefix actually helps to clarify anything.

I meant, "functions called for executing or for preparing to execute the 
nested guest".   prepare_vmcs02* family of functions identify themselves 
with a "vmcsXX" in their name while check_vmentry_{pre|post}reqs do not 
have any identification. It's just for better readability of the code.
Sean Christopherson Dec. 4, 2018, 5:13 p.m. UTC | #3 
On Fri, Nov 30, 2018 at 02:04:32PM -0500, Krish Sadhukhan wrote:
> .. as they are used only in nested context.
> 
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com>
> Reviewed-by: Mark Kanda <mark.kanda@oracle.com>
> ---
>  arch/x86/kvm/vmx.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 02edd99..3a74a4c 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -13003,7 +13003,8 @@ static int nested_vmx_check_nmi_controls(struct vmcs12 *vmcs12)
>  	return 0;
>  }
>  
> -static int check_vmentry_prereqs(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
> +static int nested_check_vmentry_prereqs(struct kvm_vcpu *vcpu,
> +					struct vmcs12 *vmcs12)

I think the prefix should be "nested_vmx_" to be consistent with (most)
other cases where we prepend nested_.  It's helpful to clarify between
e.g. nested_cpu...() and nested_vmx...().  For me it gives me a frame of
reference, e.g. "oh, this code is querying a property of the nested CPU"
vs "oh, this code implements some nested VMX behavior".

Sorry for not thinking about this earlier, I feel like I'm serving death
by a thousand cuts...
diff mbox series

Patch

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 02edd99..3a74a4c 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -13003,7 +13003,8 @@  static int nested_vmx_check_nmi_controls(struct vmcs12 *vmcs12)
 	return 0;
 }
 
-static int check_vmentry_prereqs(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
+static int nested_check_vmentry_prereqs(struct kvm_vcpu *vcpu,
+					struct vmcs12 *vmcs12)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	bool ia32e;
@@ -13185,8 +13186,8 @@  static int nested_vmx_check_vmcs_link_ptr(struct kvm_vcpu *vcpu,
 	return r;
 }
 
-static int check_vmentry_postreqs(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
-				  u32 *exit_qual)
+static int nested_check_vmentry_postreqs(struct kvm_vcpu *vcpu,
+					 struct vmcs12 *vmcs12, u32 *exit_qual)
 {
 	bool ia32e;
 
@@ -13379,7 +13380,7 @@  static int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu,
 			return -1;
 		}
 
-		if (check_vmentry_postreqs(vcpu, vmcs12, &exit_qual))
+		if (nested_check_vmentry_postreqs(vcpu, vmcs12, &exit_qual))
 			goto vmentry_fail_vmexit;
 	}
 
@@ -13515,7 +13516,7 @@  static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch)
 			launch ? VMXERR_VMLAUNCH_NONCLEAR_VMCS
 			       : VMXERR_VMRESUME_NONLAUNCHED_VMCS);
 
-	ret = check_vmentry_prereqs(vcpu, vmcs12);
+	ret = nested_check_vmentry_prereqs(vcpu, vmcs12);
 	if (ret)
 		return nested_vmx_failValid(vcpu, ret);
 
@@ -14980,8 +14981,8 @@  static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
 			return -EINVAL;
 	}
 
-	if (check_vmentry_prereqs(vcpu, vmcs12) ||
-	    check_vmentry_postreqs(vcpu, vmcs12, &exit_qual))
+	if (nested_check_vmentry_prereqs(vcpu, vmcs12) ||
+	    nested_check_vmentry_postreqs(vcpu, vmcs12, &exit_qual))
 		return -EINVAL;
 
 	vmx->nested.dirty_vmcs12 = true;