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

From: Krish Sadhukhan <krish.sadhukhan@oracle.com>
To: Oliver Upton <oupton@google.com>
Cc: kvm@vger.kernel.org, "Paolo Bonzini" <pbonzini@redhat.com>,
	"Radim Krčmář" <rkrcmar@redhat.com>,
	"Jim Mattson" <jmattson@google.com>,
	"Peter Shier" <pshier@google.com>,
	"Sean Christopherson" <sean.j.christopherson@intel.com>
Subject: Re: [kvm-unit-tests PATCH v3 7/8] x86: VMX: Make guest_state_test_main() check state from nested VM
Date: Thu, 5 Sep 2019 18:07:29 -0700	[thread overview]
Message-ID: <133d4228-7a95-3957-b494-ce9d027d00a9@oracle.com> (raw)
In-Reply-To: <20190905004919.GB107023@google.com>

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