Re: [kvm-unit-tests PATCH v3 7/8] x86: VMX: Make guest_state_test_main() check state from nested VM

[Oliver Upton <oupton@google.com>]

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