Re: [kvm-unit-tests PATCH v3 8/8] x86: VMX: Add tests for nested "load IA32_PERF_GLOBAL_CTRL"

[Krish Sadhukhan <krish.sadhukhan@oracle.com>]

On 09/04/2019 05:35 PM, Oliver Upton wrote:
> On Wed, Sep 04, 2019 at 05:13:17PM -0700, Krish Sadhukhan wrote:
>>
>> On 09/03/2019 02:58 PM, Oliver Upton wrote:
>>> Tests to verify that KVM performs the correct checks on Host/Guest state
>>> at VM-entry, as described in SDM 26.3.1.1 "Checks on Guest Control
>>> Registers, Debug Registers, and MSRs" and SDM 26.2.2 "Checks on Host
>>> Control Registers and MSRs".
>>>
>>> Test that KVM does the following:
>>>
>>>       If the "load IA32_PERF_GLOBAL_CTRL" VM-entry control is 1, the
>>>       reserved bits of the IA32_PERF_GLOBAL_CTRL MSR must be 0 in the
>>>       GUEST_IA32_PERF_GLOBAL_CTRL VMCS field. Otherwise, the VM-entry
>>>       should fail with an exit reason of "VM-entry failure due to invalid
>>>       guest state" (33). On a successful VM-entry, the correct value
>>>       should be observed when the nested VM performs an RDMSR on
>>>       IA32_PERF_GLOBAL_CTRL.
>>>
>>>       If the "load IA32_PERF_GLOBAL_CTRL" VM-exit control is 1, the
>>>       reserved bits of the IA32_PERF_GLOBAL_CTRL MSR must be 0 in the
>>>       HOST_IA32_PERF_GLOBAL_CTRL VMCS field. Otherwise, the VM-entry
>>>       should fail with a VM-instruction error of "VM entry with invalid
>>>       host-state field(s)" (8). On a successful VM-exit, the correct value
>>>       should be observed when L1 performs an RDMSR on
>>>       IA32_PERF_GLOBAL_CTRL.
>>>
>>> Suggested-by: Jim Mattson <jmattson@google.com>
>>> Co-developed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>>> Signed-off-by: Oliver Upton <oupton@google.com>
>>> ---
>>>    x86/vmx_tests.c | 199 +++++++++++++++++++++++++++++++++++++++++++++++-
>>>    1 file changed, 197 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
>>> index b72a27583793..73c46eba6be9 100644
>>> --- a/x86/vmx_tests.c
>>> +++ b/x86/vmx_tests.c
>>> @@ -5033,7 +5033,7 @@ static void guest_state_test_main(void)
>>>    			break;
>>>    		if (data->enabled) {
>>> -			obs = rdmsr(obs);
>>> +			obs = rdmsr(data->msr);
>>>    			report("Guest state is 0x%lx (expected 0x%lx)",
>>>    			       data->exp == obs, obs, data->exp);
>>>    		}
>>> @@ -6854,6 +6854,200 @@ static void test_host_efer(void)
>>>    	test_efer(HOST_EFER, "HOST_EFER", EXI_CONTROLS, EXI_LOAD_EFER);
>>>    }
>>> +union cpuid10_eax {
>>> +	struct {
>>> +		unsigned int version_id:8;
>>> +		unsigned int num_counters:8;
>> It's better to name it num_gpc or num_counters_gp.
> Ack to all style comments that follow. I brought these structs over from
> kvm, but I'll align the style with what is suggested for kvm-unit-tests.
>
>>> +		unsigned int bit_width:8;
>>> +		unsigned int mask_length:8;
>>> +	} split;
>>> +	unsigned int full;
>>> +};
>>> +
>>> +union cpuid10_edx {
>>> +	struct {
>>> +		unsigned int num_counters_fixed:5;
>>> +		unsigned int bit_width_fixed:8;
>>> +		unsigned int reserved:19;
>>> +	} split;
>>> +	unsigned int full;
>>> +};
>>> +
>> In kvm-unit-test source,  the naming of variables, functions etc. seems to
>> be based on the hex value of the CPUID leaf that they represent. For
>> example, cpuid_7_ebx, check_cpuid_80000001_edx etc. Isn't it better to name
>> these structures something like cpuidA_eax and cpuidA_edx ?
>>
>>> +static bool valid_pgc(u64 val)
>>> +{
>>> +	struct cpuid id;
>>> +	union cpuid10_eax eax;
>>> +	union cpuid10_edx edx;
>>> +	u64 mask;
>>> +
>>> +	id = cpuid(0xA);
>>> +	eax.full = id.a;
>>> +	edx.full = id.d;
>>> +	mask = ~(((1ull << eax.split.num_counters) - 1) |
>>> +		(((1ull << edx.split.num_counters_fixed) - 1) << 32));
>>> +
>>> +	return !(val & mask);
>>> +}
>>> +
>>> +static void test_pgc_vmlaunch(u32 xerror, bool xfail, bool host)
>>> +{
>>> +	u32 inst_err;
>>> +	u64 guest_rip, inst_len, obs;
>>> +	bool success;
>>> +	struct load_state_test_data *data = &load_state_test_data;
>>> +
>>> +	if (host) {
>>> +		success = vmlaunch_succeeds();
>>> +		obs = rdmsr(data->msr);
>>> +		if (data->enabled && success)
>>> +			report("Host state is 0x%lx (expected 0x%lx)",
>>> +			       data->exp == obs, obs, data->exp);
>>> +	} else {
>>> +		if (xfail)
>>> +			enter_guest_with_invalid_guest_state();
>>> +		else
>>> +			enter_guest();
>>> +		success = VMX_VMCALL == (vmcs_read(EXI_REASON) & 0xff);
>>> +		guest_rip = vmcs_read(GUEST_RIP);
>>> +		inst_len = vmcs_read(EXI_INST_LEN);
>>> +		if (success)
>>> +			vmcs_write(GUEST_RIP, guest_rip + inst_len);
>> Is it possible to re-use the existing report_guest_state_test() here and the
>> below code ?
> Ah, I missed this in my cursory search. Better to reuse what's already
> here :)
>
>>> +	}
>>> +	if (!success) {
>>> +		inst_err = vmcs_read(VMX_INST_ERROR);
>>> +		report("vmlaunch failed, VMX Inst Error is %d (expected %d)",
>>> +		       xerror == inst_err, inst_err, xerror);
>>> +	} else {
>>> +		report("vmlaunch succeeded", success != xfail);
>>> +	}
>>> +}
>>> +
>>> +/*
>>> + * test_load_pgc is a generic function for testing the
>>> + * "load IA32_PERF_GLOBAL_CTRL" VM-{entry,exit} control. This test function
>>> + * will test the provided ctrl_val disabled and enabled.
>>> + *
>>> + * @nr - VMCS field number corresponding to the Host/Guest state field
>>> + * @name - Name of the above VMCS field for printing in test report
>>> + * @ctrl_nr - VMCS field number corresponding to the VM-{entry,exit} control
>>> + * @ctrl_val - Bit to set on the ctrl field.
>>> + */
>>> +static void test_load_pgc(u32 nr, const char *name, u32 ctrl_nr,
>>> +			  const char *ctrl_name, u64 ctrl_val)
>>> +{
>>> +	u64 ctrl_saved = vmcs_read(ctrl_nr);
>>> +	u64 pgc_saved = vmcs_read(nr);
>>> +	u64 i, val;
>>> +	bool host = nr == HOST_PERF_GLOBAL_CTRL;
>>> +	struct load_state_test_data *data = &load_state_test_data;
>>> +
>>> +	data->msr = MSR_CORE_PERF_GLOBAL_CTRL;
>>> +	msr_bmp_init();
>>> +	if (!host) {
>>> +		vmx_set_test_stage(1);
>>> +		test_set_guest(guest_state_test_main);
>>> +	}
>>> +	vmcs_write(ctrl_nr, ctrl_saved & ~ctrl_val);
>>> +	data->enabled = false;
>>> +	report_prefix_pushf("\"load IA32_PERF_GLOBAL_CTRL\"=0 on %s",
>>> +			    ctrl_name);
>>> +	for (i = 0; i < 64; i++) {
>>> +		val = 1ull << i;
>>> +		vmcs_write(nr, val);
>>> +		report_prefix_pushf("%s = 0x%lx", name, val);
>>> +		/*
>>> +		 * If the "load IA32_PERF_GLOBAL_CTRL" bit is 0 then
>>> +		 * the {HOST,GUEST}_IA32_PERF_GLOBAL_CTRL field is ignored,
>>> +		 * thus setting reserved bits in this field does not cause
>>> +		 * vmlaunch to fail.
>>> +		 */
>> This comment is really not required as it's obvious that there's no effect
>> on VM-entry if the control bit is not set.
> I'll drop this.
>
>>> +		test_pgc_vmlaunch(0, false, host);
>>> +		report_prefix_pop();
>>> +	}
>>> +	report_prefix_pop();
>>> +
>>> +	vmcs_write(ctrl_nr, ctrl_saved | ctrl_val);
>>> +	data->enabled = true;
>>> +	report_prefix_pushf("\"load IA32_PERF_GLOBAL_CTRL\"=1 on %s",
>>> +			    ctrl_name);
>>> +	for (i = 0; i < 64; i++) {
>>> +		val = 1ull << i;
>>> +		data->exp = val;
>>> +		vmcs_write(nr, val);
>>> +		report_prefix_pushf("%s = 0x%lx", name, val);
>>> +		if (valid_pgc(val)) {
>>> +			test_pgc_vmlaunch(0, false, host);
>>> +		} else {
>>> +			/*
>>> +			 * [SDM 30.4]
>>> +			 *
>>> +			 * Invalid host state fields result in an VM
>>> +			 * instruction error with error number 8
>>> +			 * (VMXERR_ENTRY_INVALID_HOST_STATE_FIELD)
>>> +			 */
>>> +			if (host) {
>>> +				test_pgc_vmlaunch(
>>> +					VMXERR_ENTRY_INVALID_HOST_STATE_FIELD,
>>> +					true, host);
>>> +			/*
>>> +			 * [SDM 26.1]
>>> +			 *
>>> +			 * If a VM-Entry fails according to one of
>>> +			 * the guest-state checks, the exit reason on the VMCS
>>> +			 * will be set to reason number 33 (VMX_FAIL_STATE)
>> Again, these comments are probably not necessary.
> I wanted to capture the nuance between host/guest, since the control
> flow seems unnecessarily complicated on first glance. If they're
> distracting, I'll drop these comments as well.

The only reason for this comment is other guest state tests currently do 
not have such elaborate comments.
But I am fine either way.

>
>>> +			 */
>>> +			} else {
>>> +				test_pgc_vmlaunch(
>>> +					0,
>>> +					true, host);
>>> +				TEST_ASSERT_EQ(
>>> +					VMX_ENTRY_FAILURE | VMX_FAIL_STATE,
>>> +					vmcs_read(EXI_REASON));
>>> +			}
>>> +		}
>>> +		report_prefix_pop();
>>> +	}
>>> +
>>> +	report_prefix_pop();
>>> +
>>> +	if (nr == GUEST_PERF_GLOBAL_CTRL) {
>>> +		/*
>>> +		 * Let the guest finish execution
>>> +		 */
>>> +		vmx_set_test_stage(2);
>>> +		vmcs_write(ctrl_nr, ctrl_saved);
>>> +		vmcs_write(nr, pgc_saved);
>>> +		enter_guest();
>>> +	}
>>> +
>>> +	vmcs_write(ctrl_nr, ctrl_saved);
>>> +	vmcs_write(nr, pgc_saved);
>>> +}
>>> +
>>> +static void test_load_host_pgc(void)
>>> +{
>>> +	if (!(ctrl_exit_rev.clr & EXI_LOAD_PERF)) {
>>> +		printf("\"load IA32_PERF_GLOBAL_CTRL\" "
>>> +		       "exit control not supported\n");
>>> +		return;
>>> +	}
>>> +
>>> +	test_load_pgc(HOST_PERF_GLOBAL_CTRL, "HOST_PERF_GLOBAL_CTRL",
>>> +		      EXI_CONTROLS, "EXI_CONTROLS", EXI_LOAD_PERF);
>>> +}
>>> +
>>> +
>>> +static void test_load_guest_pgc(void)
>>> +{
>>> +	if (!(ctrl_enter_rev.clr & ENT_LOAD_PERF)) {
>>> +		printf("\"load IA32_PERF_GLOBAL_CTRL\" "
>>> +		       "entry control not supported\n");
>>> +	}
>>> +
>>> +	test_load_pgc(GUEST_PERF_GLOBAL_CTRL, "GUEST_PERF_GLOBAL_CTRL",
>>> +		      ENT_CONTROLS, "ENT_CONTROLS", ENT_LOAD_PERF);
>>> +}
>>> +
>>>    /*
>>>     * PAT values higher than 8 are uninteresting since they're likely lumped
>>>     * in with "8". We only test values above 8 one bit at a time,
>>> @@ -7147,6 +7341,7 @@ static void vmx_host_state_area_test(void)
>>>    	test_sysenter_field(HOST_SYSENTER_EIP, "HOST_SYSENTER_EIP");
>>>    	test_host_efer();
>>> +	test_load_host_pgc();
>> I would rename it to test_load_host_perf_global_ctrl to avoid confusion
>> between "performance counter" and "VMCS control field" though the name is
>> bit long.
> Sure thing.
>
>>>    	test_load_host_pat();
>>>    	test_host_segment_regs();
>>>    	test_host_desc_tables();
>>> @@ -8587,7 +8782,6 @@ static int invalid_msr_entry_failure(struct vmentry_failure *failure)
>>>    	return VMX_TEST_VMEXIT;
>>>    }
>>> -
>>>    #define TEST(name) { #name, .v2 = name }
>>>    /* name/init/guest_main/exit_handler/syscall_handler/guest_regs */
>>> @@ -8637,6 +8831,7 @@ struct vmx_test vmx_tests[] = {
>>>    	TEST(vmx_host_state_area_test),
>>>    	TEST(vmx_guest_state_area_test),
>>>    	TEST(vmentry_movss_shadow_test),
>>> +	TEST(test_load_guest_pgc),
>> Same comment as above.
>> The other comment I have is, why not put this inside of
>> 'vmx_guest_state_area_test' because this VMCS field belongs to Guest State
>> Area only.
> This is actually a point of discussion. Are we going to want every guest
> state test to set up its guest each time? As the code stands currently,
> we can only set an L2 guest once, or the test will fail out.
>
> This was less invasive, but I think the better solution would be to set
> up the guest once from 'vmx_guest_state_area_test', let the test
> functions run, then clean it up. This would require also moving the
> guest setup tidbits out of test_pat(). Would you agree this is the
> better route?

As you said, the current implementation allows a guest executable to be 
set only once for each of the entries in 'vmx_tests' array. Hence 
sub-tests can not setup their own guest.

Now, the sub-tests (including future additions) within 
'vmx_guest_state_area_test' are complete and independent tests on their 
own. The only reason for grouping them under 'vmx_guest_state_area_test' 
is to organize them in a nice hierarchical fashion.

May be, a better solution is to remove the restriction in 
test_set_guest() and let each test/sub-test set it's own guest. This 
will also mean that test_set_guest() should set GUEST_RIP as well.

>
>>>    	/* APICv tests */
>>>    	TEST(vmx_eoi_bitmap_ioapic_scan_test),
>>>    	TEST(vmx_hlt_with_rvi_test),
> Thanks for the review, Krish. I'll address style fixes where noted, only
> pending question is on the guest state test.
>
> --
> Thanks,
> Oliver