On 02/05/2019 03:30 PM, Sean Christopherson wrote:
On Tue, Feb 05, 2019 at 05:34:26PM -0500, Krish Sadhukhan wrote:
The function 'test_vmx_controls' has thus far been used to test the VMX
controls and has the expected error code (returned by KVM) hard-coded in it.
When vmlaunch fails due to a bad Host State Area, the error code returned by
KVM is different. So rename this function and add a new parameter so that it
can also be used for testing the Host State Area during vmlaunch of L2 guests.
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@xxxxxxxxxx>
---
x86/vmx_tests.c | 164 +++++++++++++++++++++++++-----------------------
1 file changed, 86 insertions(+), 78 deletions(-)
diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 9a3cdee..b69a7d9 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -3285,7 +3285,7 @@ success:
/*
* Try to launch the current VMCS.
*/
-static void test_vmx_controls(bool controls_valid, bool xfail)
+static void test_vmlaunch(bool controls_valid, bool xfail, u32 error_expected)
{
bool success = vmlaunch_succeeds();
u32 vmx_inst_err;
@@ -3295,8 +3295,8 @@ static void test_vmx_controls(bool controls_valid, bool xfail)
if (!success) {
vmx_inst_err = vmcs_read(VMX_INST_ERROR);
report("VMX inst error is %d (actual %d)",
- vmx_inst_err == VMXERR_ENTRY_INVALID_CONTROL_FIELD,
- VMXERR_ENTRY_INVALID_CONTROL_FIELD, vmx_inst_err);
+ vmx_inst_err == error_expected, error_expected,
+ vmx_inst_err);
}
That's a lot of call sites to change just to reuse a few lines of code.
And the xfail stuff isn't needed for the new host state checks. What
about moving the VMX_INST_ERROR logic to a helper and reusing that?
From 01c9fe2e0d4e0b4593334396a0a880f5adfd3fb5 Mon Sep 17 00:00:00 2001
From: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
Date: Tue, 5 Feb 2019 15:25:24 -0800
Subject: [PATCH] KVM: nVMX: Add helper to check VMX_INST_ERROR after failed
vmlaunch
...so that the code can be reused for failure due to reasons other than
invalid controls, e.g. invalid host state.
Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
---
x86/vmx_tests.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 28bab81..97c2163 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -3285,22 +3285,26 @@ success:
return true;
}
+static void check_vmx_instr_err(u32 expected)
+{
+ u32 vmx_inst_err;
+
+ vmx_inst_err = vmcs_read(VMX_INST_ERROR);
+ report("VMX inst error is %d (actual %d)",
+ vmx_inst_err == expected, expected, vmx_inst_err);
+}
+
/*
* Try to launch the current VMCS.
*/
static void test_vmx_controls(bool controls_valid, bool xfail)
{
bool success = vmlaunch_succeeds();
- u32 vmx_inst_err;
report_xfail("vmlaunch %s", xfail, success == controls_valid,
controls_valid ? "succeeds" : "fails");
- if (!success) {
- vmx_inst_err = vmcs_read(VMX_INST_ERROR);
- report("VMX inst error is %d (actual %d)",
- vmx_inst_err == VMXERR_ENTRY_INVALID_CONTROL_FIELD,
- VMXERR_ENTRY_INVALID_CONTROL_FIELD, vmx_inst_err);
- }
+ if (!success)
+ check_vmx_instr_err(VMXERR_ENTRY_INVALID_CONTROL_FIELD);
}
/*
First of all, 'test_vmx_controls' seems a misnomer to me. All that this
function does is vmlaunch'es the current VMCS and checks the error. From
that perspective, something like 'do_vmlaunch' seems a more appropriate
name to me. Also, we need to encapsulate the vmlaunch call and the error
checking within a function such that the function can be reused for any
VMCS field and not just the VMX control fields.
With respect to your suggested changes in patch# 4, the following lines
of code will have to be duplicated each time we test a Host State Area
field (or any other non-VMX-control field):
success = vmlaunch_succeeds();
report("vmlaunch %s", !success, "fails");
if (!success)
check_vmx_instr_err(VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
Even though the call sites need to be changed, how about having two
different versions of 'do_vmlaunch' - one for 'xfail' cases and the
other for regular cases - such that any of these 'do_vmlaunch' functions
can be used for any type of VMCS field and not just the VMX controls ?
diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index ee0c9ff..336d0df 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -3282,22 +3282,43 @@ success:
return true;
}
+static void check_vmx_instr_err(u32 expected)
+{
+ u32 vmx_inst_err;
+
+ vmx_inst_err = vmcs_read(VMX_INST_ERROR);
+ report("VMX inst error is %d (actual %d)",
+ vmx_inst_err == expected, expected, vmx_inst_err);
+}
+
/*
* Try to launch the current VMCS.
*/
-static void test_vmx_controls(bool controls_valid, bool xfail)
+static void do_vmlaunch(bool controls_valid, u32 error_expected)
+{
+ bool success = vmlaunch_succeeds();
+ u32 vmx_inst_err;
+
+ report("vmlaunch %s", success == controls_valid,
+ controls_valid ? "succeeds" : "fails");
+ if (!success)
+ check_vmx_instr_err(error_expected);
+}
+
+
+/*
+ * Try to launch the current VMCS.
+ */
+static void do_vmlaunch_xfail(bool controls_valid, u32 error_expected,
+ bool xfail)
{
bool success = vmlaunch_succeeds();
u32 vmx_inst_err;
report_xfail("vmlaunch %s", xfail, success == controls_valid,
controls_valid ? "succeeds" : "fails");
- if (!success) {
- vmx_inst_err = vmcs_read(VMX_INST_ERROR);
- report("VMX inst error is %d (actual %d)",
- vmx_inst_err == VMXERR_ENTRY_INVALID_CONTROL_FIELD,
- VMXERR_ENTRY_INVALID_CONTROL_FIELD, vmx_inst_err);
- }
+ if (!success)
+ check_vmx_instr_err(error_expected);
}
