* [PATCH v6 1/7] Check "load IA32_PAT" VM-exit control on vmentry
2019-04-11 19:18 [PATCH v6 0/7] KVM: nVMX Add IA32_PAT consistency checks Sean Christopherson
@ 2019-04-11 19:18 ` Sean Christopherson
2019-04-11 19:18 ` [PATCH v6 2/7] Check "load IA32_PAT" VM-entry " Sean Christopherson
` (5 subsequent siblings)
6 siblings, 0 replies; 14+ messages in thread
From: Sean Christopherson @ 2019-04-11 19:18 UTC (permalink / raw)
To: Paolo Bonzini, Radim Krčmář
Cc: kvm, Krish Sadhukhan, Karl Heubaum, Sean Christopherson, Jim Mattson
From: Krish Sadhukhan <krish.sadhukhan@oracle.com>
According to section "Checks on Host Control Registers and MSRs" in Intel
SDM vol 3C, the following check is performed on vmentry:
If the "load IA32_PAT" VM-exit control is 1, the value of the field
for the IA32_PAT MSR must be one that could be written by WRMSR
without fault at CPL 0. Specifically, each of the 8 bytes in the
field must have one of the values 0 (UC), 1 (WC), 4 (WT), 5 (WP),
6 (WB), or 7 (UC-).
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>
Reviewed-by: Jim Mattson <jmattson@google.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
arch/x86/kvm/vmx/nested.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 153e539c29c9..5133e0cf6eb9 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2590,6 +2590,10 @@ static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
is_noncanonical_address(vmcs12->host_ia32_sysenter_eip, vcpu))
return -EINVAL;
+ if ((vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PAT) &&
+ !kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, vmcs12->host_ia32_pat))
+ return -EINVAL;
+
/*
* If the load IA32_EFER VM-exit control is 1, bits reserved in the
* IA32_EFER MSR must be 0 in the field for that register. In addition,
--
2.21.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v6 2/7] Check "load IA32_PAT" VM-entry control on vmentry
2019-04-11 19:18 [PATCH v6 0/7] KVM: nVMX Add IA32_PAT consistency checks Sean Christopherson
2019-04-11 19:18 ` [PATCH v6 1/7] Check "load IA32_PAT" VM-exit control on vmentry Sean Christopherson
@ 2019-04-11 19:18 ` Sean Christopherson
2019-04-11 19:18 ` [PATCH v6 3/7] KVM: nVMX: Move guest non-reg state checks to VM-Exit path Sean Christopherson
` (4 subsequent siblings)
6 siblings, 0 replies; 14+ messages in thread
From: Sean Christopherson @ 2019-04-11 19:18 UTC (permalink / raw)
To: Paolo Bonzini, Radim Krčmář
Cc: kvm, Krish Sadhukhan, Karl Heubaum, Sean Christopherson, Jim Mattson
From: Krish Sadhukhan <krish.sadhukhan@oracle.com>
According to section "Checking and Loading Guest State" in Intel SDM vol
3C, the following check is performed on vmentry:
If the "load IA32_PAT" VM-entry control is 1, the value of the field
for the IA32_PAT MSR must be one that could be written by WRMSR
without fault at CPL 0. Specifically, each of the 8 bytes in the
field must have one of the values 0 (UC), 1 (WC), 4 (WT), 5 (WP),
6 (WB), or 7 (UC-).
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>
Reviewed-by: Jim Mattson <jmattson@google.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
arch/x86/kvm/vmx/nested.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 5133e0cf6eb9..f487aae3233e 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2680,6 +2680,10 @@ static int nested_vmx_check_vmentry_postreqs(struct kvm_vcpu *vcpu,
!nested_guest_cr4_valid(vcpu, vmcs12->guest_cr4))
return 1;
+ if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PAT) &&
+ !kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, vmcs12->guest_ia32_pat))
+ return 1;
+
if (nested_vmx_check_vmcs_link_ptr(vcpu, vmcs12)) {
*exit_qual = ENTRY_FAIL_VMCS_LINK_PTR;
return 1;
--
2.21.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v6 3/7] KVM: nVMX: Move guest non-reg state checks to VM-Exit path
2019-04-11 19:18 [PATCH v6 0/7] KVM: nVMX Add IA32_PAT consistency checks Sean Christopherson
2019-04-11 19:18 ` [PATCH v6 1/7] Check "load IA32_PAT" VM-exit control on vmentry Sean Christopherson
2019-04-11 19:18 ` [PATCH v6 2/7] Check "load IA32_PAT" VM-entry " Sean Christopherson
@ 2019-04-11 19:18 ` Sean Christopherson
2019-04-11 21:00 ` Krish Sadhukhan
2019-04-11 19:18 ` [PATCH v6 4/7] KVM: nVMX: Rename and split top-level consistency checks to match SDM Sean Christopherson
` (3 subsequent siblings)
6 siblings, 1 reply; 14+ messages in thread
From: Sean Christopherson @ 2019-04-11 19:18 UTC (permalink / raw)
To: Paolo Bonzini, Radim Krčmář
Cc: kvm, Krish Sadhukhan, Karl Heubaum, Sean Christopherson, Jim Mattson
Per Intel's SDM, volume 3, section Checking and Loading Guest State:
Because the checking and the loading occur concurrently, a failure may
be discovered only after some state has been loaded. For this reason,
the logical processor responds to such failures by loading state from
the host-state area, as it would for a VM exit.
In other words, a failed non-register state consistency check results in
a VM-Exit, not VM-Fail. Moving the non-reg state checks also paves the
way for renaming nested_vmx_check_vmentry_postreqs() to align with the
SDM, i.e. nested_vmx_check_vmentry_guest_state().
Fixes: 26539bd0e446a ("KVM: nVMX: check vmcs12 for valid activity state")
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
arch/x86/kvm/vmx/nested.c | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index f487aae3233e..fe1323ab6894 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2612,18 +2612,6 @@ static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
return 0;
}
-/*
- * Checks related to Guest Non-register State
- */
-static int nested_check_guest_non_reg_state(struct vmcs12 *vmcs12)
-{
- if (vmcs12->guest_activity_state != GUEST_ACTIVITY_ACTIVE &&
- vmcs12->guest_activity_state != GUEST_ACTIVITY_HLT)
- return -EINVAL;
-
- return 0;
-}
-
static int nested_vmx_check_vmentry_prereqs(struct kvm_vcpu *vcpu,
struct vmcs12 *vmcs12)
{
@@ -2635,9 +2623,6 @@ static int nested_vmx_check_vmentry_prereqs(struct kvm_vcpu *vcpu,
if (nested_check_host_control_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;
}
@@ -2668,6 +2653,18 @@ static int nested_vmx_check_vmcs_link_ptr(struct kvm_vcpu *vcpu,
return r;
}
+/*
+ * Checks related to Guest Non-register State
+ */
+static int nested_check_guest_non_reg_state(struct vmcs12 *vmcs12)
+{
+ if (vmcs12->guest_activity_state != GUEST_ACTIVITY_ACTIVE &&
+ vmcs12->guest_activity_state != GUEST_ACTIVITY_HLT)
+ return -EINVAL;
+
+ return 0;
+}
+
static int nested_vmx_check_vmentry_postreqs(struct kvm_vcpu *vcpu,
struct vmcs12 *vmcs12,
u32 *exit_qual)
@@ -2713,6 +2710,9 @@ static int nested_vmx_check_vmentry_postreqs(struct kvm_vcpu *vcpu,
(vmcs12->guest_bndcfgs & MSR_IA32_BNDCFGS_RSVD)))
return 1;
+ if (nested_check_guest_non_reg_state(vmcs12))
+ return 1;
+
return 0;
}
--
2.21.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v6 3/7] KVM: nVMX: Move guest non-reg state checks to VM-Exit path
2019-04-11 19:18 ` [PATCH v6 3/7] KVM: nVMX: Move guest non-reg state checks to VM-Exit path Sean Christopherson
@ 2019-04-11 21:00 ` Krish Sadhukhan
0 siblings, 0 replies; 14+ messages in thread
From: Krish Sadhukhan @ 2019-04-11 21:00 UTC (permalink / raw)
To: Sean Christopherson, Paolo Bonzini, Radim Krčmář
Cc: kvm, Karl Heubaum, Jim Mattson
On 04/11/2019 12:18 PM, Sean Christopherson wrote:
> Per Intel's SDM, volume 3, section Checking and Loading Guest State:
>
> Because the checking and the loading occur concurrently, a failure may
> be discovered only after some state has been loaded. For this reason,
> the logical processor responds to such failures by loading state from
> the host-state area, as it would for a VM exit.
>
> In other words, a failed non-register state consistency check results in
> a VM-Exit, not VM-Fail. Moving the non-reg state checks also paves the
> way for renaming nested_vmx_check_vmentry_postreqs() to align with the
> SDM, i.e. nested_vmx_check_vmentry_guest_state().
>
> Fixes: 26539bd0e446a ("KVM: nVMX: check vmcs12 for valid activity state")
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
> arch/x86/kvm/vmx/nested.c | 30 +++++++++++++++---------------
> 1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index f487aae3233e..fe1323ab6894 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2612,18 +2612,6 @@ static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
> return 0;
> }
>
> -/*
> - * Checks related to Guest Non-register State
> - */
> -static int nested_check_guest_non_reg_state(struct vmcs12 *vmcs12)
> -{
> - if (vmcs12->guest_activity_state != GUEST_ACTIVITY_ACTIVE &&
> - vmcs12->guest_activity_state != GUEST_ACTIVITY_HLT)
> - return -EINVAL;
> -
> - return 0;
> -}
> -
> static int nested_vmx_check_vmentry_prereqs(struct kvm_vcpu *vcpu,
> struct vmcs12 *vmcs12)
> {
> @@ -2635,9 +2623,6 @@ static int nested_vmx_check_vmentry_prereqs(struct kvm_vcpu *vcpu,
> if (nested_check_host_control_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;
> }
>
> @@ -2668,6 +2653,18 @@ static int nested_vmx_check_vmcs_link_ptr(struct kvm_vcpu *vcpu,
> return r;
> }
>
> +/*
> + * Checks related to Guest Non-register State
> + */
> +static int nested_check_guest_non_reg_state(struct vmcs12 *vmcs12)
> +{
> + if (vmcs12->guest_activity_state != GUEST_ACTIVITY_ACTIVE &&
> + vmcs12->guest_activity_state != GUEST_ACTIVITY_HLT)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> static int nested_vmx_check_vmentry_postreqs(struct kvm_vcpu *vcpu,
> struct vmcs12 *vmcs12,
> u32 *exit_qual)
> @@ -2713,6 +2710,9 @@ static int nested_vmx_check_vmentry_postreqs(struct kvm_vcpu *vcpu,
> (vmcs12->guest_bndcfgs & MSR_IA32_BNDCFGS_RSVD)))
> return 1;
>
> + if (nested_check_guest_non_reg_state(vmcs12))
> + return 1;
> +
> return 0;
> }
>
Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v6 4/7] KVM: nVMX: Rename and split top-level consistency checks to match SDM
2019-04-11 19:18 [PATCH v6 0/7] KVM: nVMX Add IA32_PAT consistency checks Sean Christopherson
` (2 preceding siblings ...)
2019-04-11 19:18 ` [PATCH v6 3/7] KVM: nVMX: Move guest non-reg state checks to VM-Exit path Sean Christopherson
@ 2019-04-11 19:18 ` Sean Christopherson
2019-04-11 21:23 ` Krish Sadhukhan
2019-04-11 19:18 ` [PATCH v6 5/7] KVM: nVMX: Set VM-{Fail,Exit} failure info via params, not return val Sean Christopherson
` (2 subsequent siblings)
6 siblings, 1 reply; 14+ messages in thread
From: Sean Christopherson @ 2019-04-11 19:18 UTC (permalink / raw)
To: Paolo Bonzini, Radim Krčmář
Cc: kvm, Krish Sadhukhan, Karl Heubaum, Sean Christopherson, Jim Mattson
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;
--
2.21.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v6 4/7] KVM: nVMX: Rename and split top-level consistency checks to match SDM
2019-04-11 19:18 ` [PATCH v6 4/7] KVM: nVMX: Rename and split top-level consistency checks to match SDM Sean Christopherson
@ 2019-04-11 21:23 ` Krish Sadhukhan
0 siblings, 0 replies; 14+ messages in thread
From: Krish Sadhukhan @ 2019-04-11 21:23 UTC (permalink / raw)
To: Sean Christopherson, Paolo Bonzini, Radim Krčmář
Cc: kvm, Karl Heubaum, Jim Mattson
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>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v6 5/7] KVM: nVMX: Set VM-{Fail,Exit} failure info via params, not return val
2019-04-11 19:18 [PATCH v6 0/7] KVM: nVMX Add IA32_PAT consistency checks Sean Christopherson
` (3 preceding siblings ...)
2019-04-11 19:18 ` [PATCH v6 4/7] KVM: nVMX: Rename and split top-level consistency checks to match SDM Sean Christopherson
@ 2019-04-11 19:18 ` Sean Christopherson
2019-04-11 21:56 ` Krish Sadhukhan
2019-04-12 8:30 ` Paolo Bonzini
2019-04-11 19:18 ` [PATCH v6 6/7] KVM: nVMX: Collapse nested_check_host_control_regs() into its caller Sean Christopherson
2019-04-11 19:18 ` [PATCH v6 7/7] KVM: nVMX: Return -EINVAL when signaling failure in VM-Entry helpers Sean Christopherson
6 siblings, 2 replies; 14+ messages in thread
From: Sean Christopherson @ 2019-04-11 19:18 UTC (permalink / raw)
To: Paolo Bonzini, Radim Krčmář
Cc: kvm, Krish Sadhukhan, Karl Heubaum, Sean Christopherson, Jim Mattson
Convert all top-level nested VM-Enter consistency check functions to
use explicit parameters to pass failure information to the caller.
Using an explicit parameter achieves several goals:
- Provides consistent prototypes for all functions.
- Self-documents the net effect of failure, e.g. without the explicit
parameter it may not be obvious that nested_vmx_check_guest_state()
leads to a VM-Exit.
- Does not give the false impression that failure information is
always consumed and/or relevant, e.g. vmx_set_nested_state() only
cares whether or not the checks were successful.
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
arch/x86/kvm/vmx/nested.c | 61 ++++++++++++++++++++++-----------------
1 file changed, 35 insertions(+), 26 deletions(-)
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index b22605d5ee9e..16cff40456ee 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -817,7 +817,8 @@ static int nested_vmx_store_msr_check(struct kvm_vcpu *vcpu,
* Load guest's/host's msr at nested entry/exit.
* return 0 for success, entry index for failure.
*/
-static u32 nested_vmx_load_msr(struct kvm_vcpu *vcpu, u64 gpa, u32 count)
+static int nested_vmx_load_msr(struct kvm_vcpu *vcpu, u64 gpa, u32 count,
+ u32 *exit_reason, u32 *exit_qual)
{
u32 i;
struct vmx_msr_entry e;
@@ -849,7 +850,9 @@ static u32 nested_vmx_load_msr(struct kvm_vcpu *vcpu, u64 gpa, u32 count)
}
return 0;
fail:
- return i + 1;
+ *exit_reason = EXIT_REASON_MSR_LOAD_FAIL;
+ *exit_qual = i + 1;
+ return -EINVAL;
}
static int nested_vmx_store_msr(struct kvm_vcpu *vcpu, u64 gpa, u32 count)
@@ -2574,12 +2577,15 @@ static int nested_check_vm_entry_controls(struct kvm_vcpu *vcpu,
}
static int nested_vmx_check_controls(struct kvm_vcpu *vcpu,
- struct vmcs12 *vmcs12)
+ struct vmcs12 *vmcs12,
+ u32 *vm_instruction_error)
{
+ *vm_instruction_error = VMXERR_ENTRY_INVALID_CONTROL_FIELD;
+
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 -EINVAL;
return 0;
}
@@ -2624,10 +2630,13 @@ static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
}
static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
- struct vmcs12 *vmcs12)
+ struct vmcs12 *vmcs12,
+ u32 *vm_instruction_error)
{
+ *vm_instruction_error = VMXERR_ENTRY_INVALID_HOST_STATE_FIELD;
+
if (nested_check_host_control_regs(vcpu, vmcs12))
- return VMXERR_ENTRY_INVALID_HOST_STATE_FIELD;
+ return -EINVAL;
return 0;
}
@@ -2673,10 +2682,12 @@ static int nested_check_guest_non_reg_state(struct vmcs12 *vmcs12)
static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
struct vmcs12 *vmcs12,
+ u32 *exit_reason,
u32 *exit_qual)
{
bool ia32e;
+ *exit_reason = EXIT_REASON_INVALID_STATE;
*exit_qual = ENTRY_FAIL_DEFAULT;
if (!nested_guest_cr0_valid(vcpu, vmcs12->guest_cr0) ||
@@ -2965,7 +2976,7 @@ int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu, bool from_vmentry)
struct vcpu_vmx *vmx = to_vmx(vcpu);
struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
bool evaluate_pending_interrupts;
- u32 exit_reason = EXIT_REASON_INVALID_STATE;
+ u32 exit_reason;
u32 exit_qual;
evaluate_pending_interrupts = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL) &
@@ -2991,7 +3002,8 @@ int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu, bool from_vmentry)
return -1;
}
- if (nested_vmx_check_guest_state(vcpu, vmcs12, &exit_qual))
+ if (nested_vmx_check_guest_state(vcpu, vmcs12,
+ &exit_reason, &exit_qual))
goto vmentry_fail_vmexit;
}
@@ -3003,11 +3015,9 @@ int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu, bool from_vmentry)
goto vmentry_fail_vmexit_guest_mode;
if (from_vmentry) {
- exit_reason = EXIT_REASON_MSR_LOAD_FAIL;
- exit_qual = nested_vmx_load_msr(vcpu,
- vmcs12->vm_entry_msr_load_addr,
- vmcs12->vm_entry_msr_load_count);
- if (exit_qual)
+ if (nested_vmx_load_msr(vcpu, vmcs12->vm_entry_msr_load_addr,
+ vmcs12->vm_entry_msr_load_count,
+ &exit_reason, &exit_qual))
goto vmentry_fail_vmexit_guest_mode;
} else {
/*
@@ -3087,6 +3097,7 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch)
struct vmcs12 *vmcs12;
struct vcpu_vmx *vmx = to_vmx(vcpu);
u32 interrupt_shadow = vmx_get_interrupt_shadow(vcpu);
+ u32 vm_instruction_error;
int ret;
if (!nested_vmx_check_permission(vcpu))
@@ -3136,13 +3147,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_controls(vcpu, vmcs12);
- if (ret)
- return nested_vmx_failValid(vcpu, ret);
+ if (nested_vmx_check_controls(vcpu, vmcs12, &vm_instruction_error))
+ return nested_vmx_failValid(vcpu, vm_instruction_error);
- ret = nested_vmx_check_host_state(vcpu, vmcs12);
- if (ret)
- return nested_vmx_failValid(vcpu, ret);
+ if (nested_vmx_check_host_state(vcpu, vmcs12, &vm_instruction_error))
+ return nested_vmx_failValid(vcpu, vm_instruction_error);
/*
* We're finally done with prerequisite checking, and can start with
@@ -3594,7 +3603,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
struct vmcs12 *vmcs12)
{
struct kvm_segment seg;
- u32 entry_failure_code;
+ u32 ign;
if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_EFER)
vcpu->arch.efer = vmcs12->host_ia32_efer;
@@ -3629,7 +3638,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
* Only PDPTE load can fail as the value of cr3 was checked on entry and
* couldn't have changed.
*/
- if (nested_vmx_load_cr3(vcpu, vmcs12->host_cr3, false, &entry_failure_code))
+ if (nested_vmx_load_cr3(vcpu, vmcs12->host_cr3, false, &ign))
nested_vmx_abort(vcpu, VMX_ABORT_LOAD_HOST_PDPTE_FAIL);
if (!enable_ept)
@@ -3727,7 +3736,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
vmx_update_msr_bitmap(vcpu);
if (nested_vmx_load_msr(vcpu, vmcs12->vm_exit_msr_load_addr,
- vmcs12->vm_exit_msr_load_count))
+ vmcs12->vm_exit_msr_load_count, &ign, &ign))
nested_vmx_abort(vcpu, VMX_ABORT_LOAD_HOST_MSR_FAIL);
}
@@ -5352,7 +5361,7 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
{
struct vcpu_vmx *vmx = to_vmx(vcpu);
struct vmcs12 *vmcs12;
- u32 exit_qual;
+ u32 ign;
int ret;
if (kvm_state->format != 0)
@@ -5465,9 +5474,9 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
return -EINVAL;
}
- if (nested_vmx_check_controls(vcpu, vmcs12) ||
- nested_vmx_check_host_state(vcpu, vmcs12) ||
- nested_vmx_check_guest_state(vcpu, vmcs12, &exit_qual))
+ if (nested_vmx_check_controls(vcpu, vmcs12, &ign) ||
+ nested_vmx_check_host_state(vcpu, vmcs12, &ign) ||
+ nested_vmx_check_guest_state(vcpu, vmcs12, &ign, &ign))
return -EINVAL;
vmx->nested.dirty_vmcs12 = true;
--
2.21.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v6 5/7] KVM: nVMX: Set VM-{Fail,Exit} failure info via params, not return val
2019-04-11 19:18 ` [PATCH v6 5/7] KVM: nVMX: Set VM-{Fail,Exit} failure info via params, not return val Sean Christopherson
@ 2019-04-11 21:56 ` Krish Sadhukhan
2019-04-12 8:30 ` Paolo Bonzini
1 sibling, 0 replies; 14+ messages in thread
From: Krish Sadhukhan @ 2019-04-11 21:56 UTC (permalink / raw)
To: Sean Christopherson, Paolo Bonzini, Radim Krčmář
Cc: kvm, Karl Heubaum, Jim Mattson
On 04/11/2019 12:18 PM, Sean Christopherson wrote:
> Convert all top-level nested VM-Enter consistency check functions to
> use explicit parameters to pass failure information to the caller.
> Using an explicit parameter achieves several goals:
>
> - Provides consistent prototypes for all functions.
> - Self-documents the net effect of failure, e.g. without the explicit
> parameter it may not be obvious that nested_vmx_check_guest_state()
> leads to a VM-Exit.
> - Does not give the false impression that failure information is
> always consumed and/or relevant, e.g. vmx_set_nested_state() only
> cares whether or not the checks were successful.
>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
> arch/x86/kvm/vmx/nested.c | 61 ++++++++++++++++++++++-----------------
> 1 file changed, 35 insertions(+), 26 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index b22605d5ee9e..16cff40456ee 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -817,7 +817,8 @@ static int nested_vmx_store_msr_check(struct kvm_vcpu *vcpu,
> * Load guest's/host's msr at nested entry/exit.
> * return 0 for success, entry index for failure.
> */
> -static u32 nested_vmx_load_msr(struct kvm_vcpu *vcpu, u64 gpa, u32 count)
> +static int nested_vmx_load_msr(struct kvm_vcpu *vcpu, u64 gpa, u32 count,
> + u32 *exit_reason, u32 *exit_qual)
> {
> u32 i;
> struct vmx_msr_entry e;
> @@ -849,7 +850,9 @@ static u32 nested_vmx_load_msr(struct kvm_vcpu *vcpu, u64 gpa, u32 count)
> }
> return 0;
> fail:
> - return i + 1;
> + *exit_reason = EXIT_REASON_MSR_LOAD_FAIL;
> + *exit_qual = i + 1;
> + return -EINVAL;
> }
>
> static int nested_vmx_store_msr(struct kvm_vcpu *vcpu, u64 gpa, u32 count)
> @@ -2574,12 +2577,15 @@ static int nested_check_vm_entry_controls(struct kvm_vcpu *vcpu,
> }
>
> static int nested_vmx_check_controls(struct kvm_vcpu *vcpu,
> - struct vmcs12 *vmcs12)
> + struct vmcs12 *vmcs12,
> + u32 *vm_instruction_error)
> {
> + *vm_instruction_error = VMXERR_ENTRY_INVALID_CONTROL_FIELD;
Although, all of the callee functions generate the same error code,
isn't it cleaner to set the error in the callee themselves where the
actual checking is done ?
> +
> 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 -EINVAL;
>
> return 0;
> }
> @@ -2624,10 +2630,13 @@ static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
> }
>
> static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
> - struct vmcs12 *vmcs12)
> + struct vmcs12 *vmcs12,
> + u32 *vm_instruction_error)
> {
> + *vm_instruction_error = VMXERR_ENTRY_INVALID_HOST_STATE_FIELD;
> +
> if (nested_check_host_control_regs(vcpu, vmcs12))
> - return VMXERR_ENTRY_INVALID_HOST_STATE_FIELD;
> + return -EINVAL;
>
> return 0;
> }
> @@ -2673,10 +2682,12 @@ static int nested_check_guest_non_reg_state(struct vmcs12 *vmcs12)
>
> static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
> struct vmcs12 *vmcs12,
> + u32 *exit_reason,
> u32 *exit_qual)
> {
> bool ia32e;
>
> + *exit_reason = EXIT_REASON_INVALID_STATE;
Shouldn't we also reflect VMX_EXIT_REASONS_FAILED_VMENTRY in 'exit_reason' ?
> *exit_qual = ENTRY_FAIL_DEFAULT;
>
> if (!nested_guest_cr0_valid(vcpu, vmcs12->guest_cr0) ||
> @@ -2965,7 +2976,7 @@ int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu, bool from_vmentry)
> struct vcpu_vmx *vmx = to_vmx(vcpu);
> struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
> bool evaluate_pending_interrupts;
> - u32 exit_reason = EXIT_REASON_INVALID_STATE;
> + u32 exit_reason;
> u32 exit_qual;
>
> evaluate_pending_interrupts = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL) &
> @@ -2991,7 +3002,8 @@ int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu, bool from_vmentry)
> return -1;
> }
>
> - if (nested_vmx_check_guest_state(vcpu, vmcs12, &exit_qual))
> + if (nested_vmx_check_guest_state(vcpu, vmcs12,
> + &exit_reason, &exit_qual))
> goto vmentry_fail_vmexit;
> }
>
> @@ -3003,11 +3015,9 @@ int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu, bool from_vmentry)
> goto vmentry_fail_vmexit_guest_mode;
>
> if (from_vmentry) {
> - exit_reason = EXIT_REASON_MSR_LOAD_FAIL;
> - exit_qual = nested_vmx_load_msr(vcpu,
> - vmcs12->vm_entry_msr_load_addr,
> - vmcs12->vm_entry_msr_load_count);
> - if (exit_qual)
> + if (nested_vmx_load_msr(vcpu, vmcs12->vm_entry_msr_load_addr,
> + vmcs12->vm_entry_msr_load_count,
> + &exit_reason, &exit_qual))
> goto vmentry_fail_vmexit_guest_mode;
> } else {
> /*
> @@ -3087,6 +3097,7 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch)
> struct vmcs12 *vmcs12;
> struct vcpu_vmx *vmx = to_vmx(vcpu);
> u32 interrupt_shadow = vmx_get_interrupt_shadow(vcpu);
> + u32 vm_instruction_error;
> int ret;
>
> if (!nested_vmx_check_permission(vcpu))
> @@ -3136,13 +3147,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_controls(vcpu, vmcs12);
> - if (ret)
> - return nested_vmx_failValid(vcpu, ret);
> + if (nested_vmx_check_controls(vcpu, vmcs12, &vm_instruction_error))
> + return nested_vmx_failValid(vcpu, vm_instruction_error);
>
> - ret = nested_vmx_check_host_state(vcpu, vmcs12);
> - if (ret)
> - return nested_vmx_failValid(vcpu, ret);
> + if (nested_vmx_check_host_state(vcpu, vmcs12, &vm_instruction_error))
> + return nested_vmx_failValid(vcpu, vm_instruction_error);
>
> /*
> * We're finally done with prerequisite checking, and can start with
> @@ -3594,7 +3603,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
> struct vmcs12 *vmcs12)
> {
> struct kvm_segment seg;
> - u32 entry_failure_code;
> + u32 ign;
>
> if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_EFER)
> vcpu->arch.efer = vmcs12->host_ia32_efer;
> @@ -3629,7 +3638,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
> * Only PDPTE load can fail as the value of cr3 was checked on entry and
> * couldn't have changed.
> */
> - if (nested_vmx_load_cr3(vcpu, vmcs12->host_cr3, false, &entry_failure_code))
> + if (nested_vmx_load_cr3(vcpu, vmcs12->host_cr3, false, &ign))
> nested_vmx_abort(vcpu, VMX_ABORT_LOAD_HOST_PDPTE_FAIL);
>
> if (!enable_ept)
> @@ -3727,7 +3736,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
> vmx_update_msr_bitmap(vcpu);
>
> if (nested_vmx_load_msr(vcpu, vmcs12->vm_exit_msr_load_addr,
> - vmcs12->vm_exit_msr_load_count))
> + vmcs12->vm_exit_msr_load_count, &ign, &ign))
> nested_vmx_abort(vcpu, VMX_ABORT_LOAD_HOST_MSR_FAIL);
> }
>
> @@ -5352,7 +5361,7 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
> {
> struct vcpu_vmx *vmx = to_vmx(vcpu);
> struct vmcs12 *vmcs12;
> - u32 exit_qual;
> + u32 ign;
> int ret;
>
> if (kvm_state->format != 0)
> @@ -5465,9 +5474,9 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
> return -EINVAL;
> }
>
> - if (nested_vmx_check_controls(vcpu, vmcs12) ||
> - nested_vmx_check_host_state(vcpu, vmcs12) ||
> - nested_vmx_check_guest_state(vcpu, vmcs12, &exit_qual))
> + if (nested_vmx_check_controls(vcpu, vmcs12, &ign) ||
> + nested_vmx_check_host_state(vcpu, vmcs12, &ign) ||
> + nested_vmx_check_guest_state(vcpu, vmcs12, &ign, &ign))
> return -EINVAL;
>
> vmx->nested.dirty_vmcs12 = true;
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 5/7] KVM: nVMX: Set VM-{Fail,Exit} failure info via params, not return val
2019-04-11 19:18 ` [PATCH v6 5/7] KVM: nVMX: Set VM-{Fail,Exit} failure info via params, not return val Sean Christopherson
2019-04-11 21:56 ` Krish Sadhukhan
@ 2019-04-12 8:30 ` Paolo Bonzini
2019-04-12 19:12 ` Sean Christopherson
1 sibling, 1 reply; 14+ messages in thread
From: Paolo Bonzini @ 2019-04-12 8:30 UTC (permalink / raw)
To: Sean Christopherson, Radim Krčmář
Cc: kvm, Krish Sadhukhan, Karl Heubaum, Jim Mattson
On 11/04/19 21:18, Sean Christopherson wrote:
> Convert all top-level nested VM-Enter consistency check functions to
> use explicit parameters to pass failure information to the caller.
> Using an explicit parameter achieves several goals:
>
> - Provides consistent prototypes for all functions.
> - Self-documents the net effect of failure, e.g. without the explicit
> parameter it may not be obvious that nested_vmx_check_guest_state()
> leads to a VM-Exit.
> - Does not give the false impression that failure information is
> always consumed and/or relevant, e.g. vmx_set_nested_state() only
> cares whether or not the checks were successful.
>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
I think we have to agree to disagree on this one. :)
I'd rather have something like this (untested) replacing patches 5 and 6:
-------------- 8< -----------------
From: Paolo Bonzini <pbonzini@redhat.com>
Subject: [PATCH] KVM: nVMX: Return -EINVAL when signaling failure in pre-VM-Entry helpers
Convert all top-level nested VM-Enter consistency check functions to
return 0/-EINVAL instead of failure codes, since now they can only
ever return one failure code.
This also does not give the false impression that failure information is
always consumed and/or relevant, e.g. vmx_set_nested_state() only
cares whether or not the checks were successful.
nested_check_host_control_regs() can also now be inlined into its caller,
nested_vmx_check_host_state, since the two have effectively become the
same function.
Based on a patch by Sean Christopherson.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index f58de538e6d3..c75edccdd3bc 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2595,16 +2595,13 @@ static int nested_vmx_check_controls(struct kvm_vcpu *vcpu,
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 -EINVAL;
return 0;
}
-/*
- * Checks related to Host Control Registers and MSRs
- */
-static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
- struct vmcs12 *vmcs12)
+static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
+ struct vmcs12 *vmcs12)
{
bool ia32e;
@@ -2639,15 +2636,6 @@ static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
return 0;
}
-static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
- struct vmcs12 *vmcs12)
-{
- if (nested_check_host_control_regs(vcpu, vmcs12))
- return VMXERR_ENTRY_INVALID_HOST_STATE_FIELD;
-
- return 0;
-}
-
static int nested_vmx_check_vmcs_link_ptr(struct kvm_vcpu *vcpu,
struct vmcs12 *vmcs12)
{
@@ -3152,13 +3140,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_controls(vcpu, vmcs12);
- if (ret)
- return nested_vmx_failValid(vcpu, ret);
+ if (nested_vmx_check_controls(vcpu, vmcs12))
+ return nested_vmx_failValid(vcpu, VMXERR_ENTRY_INVALID_CONTROL_FIELD);
- ret = nested_vmx_check_host_state(vcpu, vmcs12);
- if (ret)
- return nested_vmx_failValid(vcpu, ret);
+ if (nested_vmx_check_host_state(vcpu, vmcs12))
+ return nested_vmx_failValid(vcpu, VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
/*
* We're finally done with prerequisite checking, and can start with
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v6 5/7] KVM: nVMX: Set VM-{Fail,Exit} failure info via params, not return val
2019-04-12 8:30 ` Paolo Bonzini
@ 2019-04-12 19:12 ` Sean Christopherson
0 siblings, 0 replies; 14+ messages in thread
From: Sean Christopherson @ 2019-04-12 19:12 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Radim Krčmář,
kvm, Krish Sadhukhan, Karl Heubaum, Jim Mattson
On Fri, Apr 12, 2019 at 10:30:51AM +0200, Paolo Bonzini wrote:
> On 11/04/19 21:18, Sean Christopherson wrote:
> > Convert all top-level nested VM-Enter consistency check functions to
> > use explicit parameters to pass failure information to the caller.
> > Using an explicit parameter achieves several goals:
> >
> > - Provides consistent prototypes for all functions.
> > - Self-documents the net effect of failure, e.g. without the explicit
> > parameter it may not be obvious that nested_vmx_check_guest_state()
> > leads to a VM-Exit.
> > - Does not give the false impression that failure information is
> > always consumed and/or relevant, e.g. vmx_set_nested_state() only
> > cares whether or not the checks were successful.
> >
> > Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > ---
>
> I think we have to agree to disagree on this one. :)
Aaargh! The dreaded Maintainer Hammer of Doom! LGTM ;-)
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v6 6/7] KVM: nVMX: Collapse nested_check_host_control_regs() into its caller
2019-04-11 19:18 [PATCH v6 0/7] KVM: nVMX Add IA32_PAT consistency checks Sean Christopherson
` (4 preceding siblings ...)
2019-04-11 19:18 ` [PATCH v6 5/7] KVM: nVMX: Set VM-{Fail,Exit} failure info via params, not return val Sean Christopherson
@ 2019-04-11 19:18 ` Sean Christopherson
2019-04-11 22:02 ` Krish Sadhukhan
2019-04-11 19:18 ` [PATCH v6 7/7] KVM: nVMX: Return -EINVAL when signaling failure in VM-Entry helpers Sean Christopherson
6 siblings, 1 reply; 14+ messages in thread
From: Sean Christopherson @ 2019-04-11 19:18 UTC (permalink / raw)
To: Paolo Bonzini, Radim Krčmář
Cc: kvm, Krish Sadhukhan, Karl Heubaum, Sean Christopherson, Jim Mattson
...now that nested_vmx_check_host_state() returns -EINVAL to indicate
failure, i.e. moving the checks doesn't require changing their return
values to be 'VMXERR_ENTRY_INVALID_HOST_STATE_FIELD'. This eliminates
a misleading function name since nested_check_host_control_regs() was
checking MSRs as well as control registers.
Alternatively, nested_check_host_control_regs() could be split up,
e.g. into _control_regs(), _debug_regs() and _msrs(), but that's a
bit gratutitous at this time given that the entire function is 30 LoC.
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
arch/x86/kvm/vmx/nested.c | 26 ++++++--------------------
1 file changed, 6 insertions(+), 20 deletions(-)
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 16cff40456ee..98afbe7c15a1 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2590,13 +2590,11 @@ static int nested_vmx_check_controls(struct kvm_vcpu *vcpu,
return 0;
}
-/*
- * Checks related to Host Control Registers and MSRs
- */
-static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
- struct vmcs12 *vmcs12)
+static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
+ struct vmcs12 *vmcs12,
+ u32 *vm_instruction_error)
{
- bool ia32e;
+ *vm_instruction_error = VMXERR_ENTRY_INVALID_HOST_STATE_FIELD;
if (!nested_host_cr0_valid(vcpu, vmcs12->host_cr0) ||
!nested_host_cr4_valid(vcpu, vmcs12->host_cr4) ||
@@ -2618,8 +2616,8 @@ static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
* the host address-space size VM-exit control.
*/
if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_EFER) {
- ia32e = (vmcs12->vm_exit_controls &
- VM_EXIT_HOST_ADDR_SPACE_SIZE) != 0;
+ bool ia32e = (vmcs12->vm_exit_controls &
+ VM_EXIT_HOST_ADDR_SPACE_SIZE) != 0;
if (!kvm_valid_efer(vcpu, vmcs12->host_ia32_efer) ||
ia32e != !!(vmcs12->host_ia32_efer & EFER_LMA) ||
ia32e != !!(vmcs12->host_ia32_efer & EFER_LME))
@@ -2629,18 +2627,6 @@ static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
return 0;
}
-static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
- struct vmcs12 *vmcs12,
- u32 *vm_instruction_error)
-{
- *vm_instruction_error = VMXERR_ENTRY_INVALID_HOST_STATE_FIELD;
-
- if (nested_check_host_control_regs(vcpu, vmcs12))
- return -EINVAL;
-
- return 0;
-}
-
static int nested_vmx_check_vmcs_link_ptr(struct kvm_vcpu *vcpu,
struct vmcs12 *vmcs12)
{
--
2.21.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v6 6/7] KVM: nVMX: Collapse nested_check_host_control_regs() into its caller
2019-04-11 19:18 ` [PATCH v6 6/7] KVM: nVMX: Collapse nested_check_host_control_regs() into its caller Sean Christopherson
@ 2019-04-11 22:02 ` Krish Sadhukhan
0 siblings, 0 replies; 14+ messages in thread
From: Krish Sadhukhan @ 2019-04-11 22:02 UTC (permalink / raw)
To: Sean Christopherson, Paolo Bonzini, Radim Krčmář
Cc: kvm, Karl Heubaum, Jim Mattson
On 04/11/2019 12:18 PM, Sean Christopherson wrote:
> ...now that nested_vmx_check_host_state() returns -EINVAL to indicate
> failure, i.e. moving the checks doesn't require changing their return
> values to be 'VMXERR_ENTRY_INVALID_HOST_STATE_FIELD'. This eliminates
> a misleading function name since nested_check_host_control_regs() was
> checking MSRs as well as control registers.
>
> Alternatively, nested_check_host_control_regs() could be split up,
> e.g. into _control_regs(), _debug_regs() and _msrs(), but that's a
> bit gratutitous at this time given that the entire function is 30 LoC.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
> arch/x86/kvm/vmx/nested.c | 26 ++++++--------------------
> 1 file changed, 6 insertions(+), 20 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 16cff40456ee..98afbe7c15a1 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2590,13 +2590,11 @@ static int nested_vmx_check_controls(struct kvm_vcpu *vcpu,
> return 0;
> }
>
> -/*
> - * Checks related to Host Control Registers and MSRs
> - */
> -static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
> - struct vmcs12 *vmcs12)
> +static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
> + struct vmcs12 *vmcs12,
> + u32 *vm_instruction_error)
> {
> - bool ia32e;
> + *vm_instruction_error = VMXERR_ENTRY_INVALID_HOST_STATE_FIELD;
>
> if (!nested_host_cr0_valid(vcpu, vmcs12->host_cr0) ||
> !nested_host_cr4_valid(vcpu, vmcs12->host_cr4) ||
> @@ -2618,8 +2616,8 @@ static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
> * the host address-space size VM-exit control.
> */
> if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_EFER) {
> - ia32e = (vmcs12->vm_exit_controls &
> - VM_EXIT_HOST_ADDR_SPACE_SIZE) != 0;
> + bool ia32e = (vmcs12->vm_exit_controls &
> + VM_EXIT_HOST_ADDR_SPACE_SIZE) != 0;
> if (!kvm_valid_efer(vcpu, vmcs12->host_ia32_efer) ||
> ia32e != !!(vmcs12->host_ia32_efer & EFER_LMA) ||
> ia32e != !!(vmcs12->host_ia32_efer & EFER_LME))
> @@ -2629,18 +2627,6 @@ static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
> return 0;
> }
>
> -static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
> - struct vmcs12 *vmcs12,
> - u32 *vm_instruction_error)
> -{
> - *vm_instruction_error = VMXERR_ENTRY_INVALID_HOST_STATE_FIELD;
> -
> - if (nested_check_host_control_regs(vcpu, vmcs12))
> - return -EINVAL;
> -
> - return 0;
> -}
> -
> static int nested_vmx_check_vmcs_link_ptr(struct kvm_vcpu *vcpu,
> struct vmcs12 *vmcs12)
> {
Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v6 7/7] KVM: nVMX: Return -EINVAL when signaling failure in VM-Entry helpers
2019-04-11 19:18 [PATCH v6 0/7] KVM: nVMX Add IA32_PAT consistency checks Sean Christopherson
` (5 preceding siblings ...)
2019-04-11 19:18 ` [PATCH v6 6/7] KVM: nVMX: Collapse nested_check_host_control_regs() into its caller Sean Christopherson
@ 2019-04-11 19:18 ` Sean Christopherson
6 siblings, 0 replies; 14+ messages in thread
From: Sean Christopherson @ 2019-04-11 19:18 UTC (permalink / raw)
To: Paolo Bonzini, Radim Krčmář
Cc: kvm, Krish Sadhukhan, Karl Heubaum, Sean Christopherson, Jim Mattson
Most, but not all, helpers that are related to emulating consistency
checks for nested VM-Entry return -EINVAL when a check fails. Convert
the holdouts to have consistency throughout and to make it clear that
the functions are signaling pass/fail as opposed to "resume guest" vs.
"exit to userspace".
Opportunistically fix bad indentation in nested_vmx_check_guest_state().
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
arch/x86/kvm/vmx/nested.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 98afbe7c15a1..b8b74a73e00b 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -917,7 +917,7 @@ static int nested_vmx_load_cr3(struct kvm_vcpu *vcpu, unsigned long cr3, bool ne
if (cr3 != kvm_read_cr3(vcpu) || (!nested_ept && pdptrs_changed(vcpu))) {
if (!nested_cr3_valid(vcpu, cr3)) {
*entry_failure_code = ENTRY_FAIL_DEFAULT;
- return 1;
+ return -EINVAL;
}
/*
@@ -928,7 +928,7 @@ static int nested_vmx_load_cr3(struct kvm_vcpu *vcpu, unsigned long cr3, bool ne
!nested_ept) {
if (!load_pdptrs(vcpu, vcpu->arch.walk_mmu, cr3)) {
*entry_failure_code = ENTRY_FAIL_PDPTE;
- return 1;
+ return -EINVAL;
}
}
}
@@ -2360,13 +2360,13 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
*/
if (vmx->emulation_required) {
*entry_failure_code = ENTRY_FAIL_DEFAULT;
- return 1;
+ return -EINVAL;
}
/* Shadow page tables on either EPT or shadow page tables. */
if (nested_vmx_load_cr3(vcpu, vmcs12->guest_cr3, nested_cpu_has_ept(vmcs12),
entry_failure_code))
- return 1;
+ return -EINVAL;
if (!enable_ept)
vcpu->arch.walk_mmu->inject_page_fault = vmx_inject_page_fault_nested;
@@ -2678,15 +2678,15 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
if (!nested_guest_cr0_valid(vcpu, vmcs12->guest_cr0) ||
!nested_guest_cr4_valid(vcpu, vmcs12->guest_cr4))
- return 1;
+ return -EINVAL;
if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PAT) &&
!kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, vmcs12->guest_ia32_pat))
- return 1;
+ return -EINVAL;
if (nested_vmx_check_vmcs_link_ptr(vcpu, vmcs12)) {
*exit_qual = ENTRY_FAIL_VMCS_LINK_PTR;
- return 1;
+ return -EINVAL;
}
/*
@@ -2705,16 +2705,16 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
ia32e != !!(vmcs12->guest_ia32_efer & EFER_LMA) ||
((vmcs12->guest_cr0 & X86_CR0_PG) &&
ia32e != !!(vmcs12->guest_ia32_efer & EFER_LME)))
- return 1;
+ return -EINVAL;
}
if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS) &&
- (is_noncanonical_address(vmcs12->guest_bndcfgs & PAGE_MASK, vcpu) ||
- (vmcs12->guest_bndcfgs & MSR_IA32_BNDCFGS_RSVD)))
- return 1;
+ (is_noncanonical_address(vmcs12->guest_bndcfgs & PAGE_MASK, vcpu) ||
+ (vmcs12->guest_bndcfgs & MSR_IA32_BNDCFGS_RSVD)))
+ return -EINVAL;
if (nested_check_guest_non_reg_state(vmcs12))
- return 1;
+ return -EINVAL;
return 0;
}
--
2.21.0
^ permalink raw reply related [flat|nested] 14+ messages in thread