On 09/04/2019 05:49 PM, Oliver Upton wrote:
On Wed, Sep 04, 2019 at 05:25:40PM -0700, Krish Sadhukhan wrote:
On 09/03/2019 02:58 PM, Oliver Upton wrote:
The current tests for guest state do not yet check the validity of
loaded state from within the nested VM. Introduce the
load_state_test_data struct to share data with the nested VM.
Signed-off-by: Oliver Upton <oupton@xxxxxxxxxx>
---
x86/vmx_tests.c | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index f035f24a771a..b72a27583793 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -5017,13 +5017,28 @@ static void test_entry_msr_load(void)
test_vmx_valid_controls(false);
}
+static struct load_state_test_data {
+ u32 msr;
+ u64 exp;
+ bool enabled;
+} load_state_test_data;
A better name is probably 'loaded_state_test_data' as you are checking the
validity of the loaded MSR in the guest.
Other usages of structs for data sharing follow the previous naming
convention, but I slightly missed the mark with that as well. Other
structs seem to use the same prefix that the associated tests have (e.g.
ept_access_test_data corresponds to ept_access_test_*). To best match
that pattern, I should instead name it "vmx_state_area_test_data" (since
its used for both guest/host test data anyway.
That isn't to say there is a better pattern we could follow for naming
this! Which do you think is better?
'vmx_state_area_test_data' sounds fine to me. Thanks !
+
static void guest_state_test_main(void)
{
+ u64 obs;
+ struct load_state_test_data *data = &load_state_test_data;
+
while (1) {
- if (vmx_get_test_stage() != 2)
- vmcall();
- else
+ if (vmx_get_test_stage() == 2)
break;
+
+ if (data->enabled) {
+ obs = rdmsr(obs);
Although you fixed it in the next patch, why not use 'data->msr' in place
of 'obs' as the parameter to rdmsr() in this patch only ?
Ugh, I mucked this up when reworking before sending out. 'data->msr'
should have appeared in this patch. I'll fix this.
+ report("Guest state is 0x%lx (expected 0x%lx)",
+ data->exp == obs, obs, data->exp);
+ }
+
+ vmcall();
}
asm volatile("fnop");
@@ -6854,7 +6869,9 @@ static void test_pat(u32 field, const char * field_name, u32 ctrl_field,
u64 i, val;
u32 j;
int error;
+ struct load_state_test_data *data = &load_state_test_data;
+ data->enabled = false;
vmcs_clear_bits(ctrl_field, ctrl_bit);
if (field == GUEST_PAT) {
vmx_set_test_stage(1);
Thanks for the review, Krish. Looks like a typo I didn't rework into
this patch correctly, please let me know what you think on the other
comment.
--
Thanks,
Oliver
