| Message ID | 20190903215801.183193-8-oupton@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: VMX: Add full nested support for IA32_PERF_GLOBAL_CTRL | expand |
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@google.com> > --- > 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. > + > 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 ? > + 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);
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@google.com> > > --- > > 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? > > + > > 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
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@google.com> >>> --- >>> 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
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; + 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); + 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);
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@google.com> --- x86/vmx_tests.c | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-)