Re: [kvm-unit-tests PATCH 4/5] Add a test for writing canonical values to various msrs and fields

[Sean Christopherson <seanjc@google.com>]

On Fri, Sep 06, 2024, Maxim Levitsky wrote:
> +static void test_register_write(const char *register_name, u64 test_register,
> +				bool force_emulation, u64 test_value,
> +				bool expect_success)
> +{
> +	u64 old_value, expected_value;
> +	bool success, test_passed = false;
> +	int test_mode = (force_emulation ? SET_REGISTER_MODE_FEP : SET_REGISTER_MODE_SAFE);
> +
> +	old_value = get_test_register_value(test_register);
> +	expected_value = expect_success ? test_value : old_value;
> +
> +	/*
> +	 * TODO: Successful write to the MSR_GS_BASE corrupts it,
> +	 * and that breaks the wrmsr_safe macro.
> +	 */
> +	if ((test_register >> 32) == MSR_GS_BASE && expect_success)
> +		test_mode = SET_REGISTER_MODE_UNSAFE;
> +
> +	/* Write the test value*/
> +	success =  set_test_register_value(test_register, test_mode, test_value);
> +
> +	if (success != expect_success) {
> +		report(false,

This can be report_fail().  But that's a moot point because skipping the cleanup
on unexpected *success* leaves registers in a bad state and crashes the test.

> +		       "Write of test register %s with value %lx unexpectedly %s",
> +		       register_name, test_value,
> +		       (success ? "succeeded" : "failed"));
> +		goto exit;
> +	}
> +
> +	/*
> +	 * Check that the value was really written.
> +	 * Don't test TR and LDTR, because it's not possible to read them
> +	 * directly.
> +	 */
> +
> +	if (test_register != TEST_REGISTER_TR_BASE &&
> +	    test_register != TEST_REGISTER_LDT_BASE) {
> +		u64 new_value = get_test_register_value(test_register);
> +
> +		if (new_value != expected_value) {
> +			report(false,

Same thing here (on both fronts).

> +			       "Register %s wasn't set to %lx as expected (actual value %lx)",
> +			       register_name, expected_value, new_value);
> +			goto exit;
> +		}
> +	}
> +
> +	/*
> +	 * Restore the old value directly without safety wrapper,
> +	 * to avoid test crashes related to temporary clobbered GDT/IDT/etc bases.
> +	 */
> +
> +	set_test_register_value(test_register, SET_REGISTER_MODE_UNSAFE, old_value);
> +	test_passed = true;
> +exit:
> +	report(test_passed, "Tested setting %s to 0x%lx value - %s", register_name,
> +	       test_value, success ? "success" : "failure");
> +}

...

> +#define TEST_REGISTER(register_name, force_emulation) \
> +		      test_register(#register_name, register_name, force_emulation)

Rather than print the entire name, concatenate TEST_REGISTER with the register
name, e.g. GDTR_BASE, so that the outputs are simply GDTR_BASE.

> +	/*
> +	 * SYSENTER ESP/EIP MSRs have canonical checks only on Intel,
> +	 * because only on Intel these instructions were extended to 64 bit.
> +	 *
> +	 * TODO: KVM emulation however ignores canonical checks for these MSRs
> +	 * even on Intel, to support cross-vendor migration.
> +	 *
> +	 * Thus only run the check on bare metal.

Sadly, KVM's behavior also applies for nested VMX, i.e. when running the test
nested, there is no bare metal.  AFAIK, there's no easy way to detect a nested
run from within the test; it would have to be fed in from the test runner.

For now, I'll make the whole thing a TODO so that people don't have to re-debug
why the test fails when run in a VM (and because testing bare metal isn't all
that interesting).

> +static void do_test(void)
> +{
> +	printf("\n");
> +	printf("Running the test without emulation:\n");
> +	__do_test(false);

Do the printf()s in the inner helper, it's easy to pivot on @forced_emulation.

> +	printf("\n");
> +
> +	if (is_fep_available()) {
> +		printf("Running the test with forced emulation:\n");
> +		__do_test(true);
> +	} else

Needs curly braces (moot point if the printf() is moved).

> +		report_skip("force emulation prefix not enabled - skipping");
> +}
> +
> +int main(int ac, char **av)
> +{
> +	/* set dummy LDTR pointer */
> +	set_gdt_entry(FIRST_SPARE_SEL, 0xffaabb, 0xffff, 0x82, 0);
> +	lldt(FIRST_SPARE_SEL);
> +
> +	do_test();
> +
> +	printf("\n");
> +
> +	if (this_cpu_has(X86_FEATURE_LA57)) {
> +		printf("Switching to 5 level paging mode and rerunning the test...\n");
> +		setup_5level_page_table();
> +		do_test();
> +	} else

Curly braces.

> +		report_skip("Skipping the test in 5-level paging mode - not supported on the host");
> +
> +	return report_summary();
> +}
> -- 
> 2.26.3
>