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?
> > +
> > 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
