Re: [PATCH v3] KVM: selftests: Add a test for gPAT handling in L2

[Sean Christopherson <seanjc@google.com>]

On Thu, May 21, 2026, Yosry Ahmed wrote:
> +static void l1_guest_code_invalid_gpat(struct svm_test_data *svm)
> +{
> +	unsigned long l2_guest_stack[L2_GUEST_STACK_SIZE];
> +	struct vmcb *vmcb = svm->vmcb;
> +
> +	/* VMRUN should fail without running L2 */
> +	generic_svm_setup(svm, NULL, &l2_guest_stack[L2_GUEST_STACK_SIZE]);
> +
> +	vmcb->save.g_pat = INVALID_PAT_VALUE;
> +	run_guest(vmcb, svm->vmcb_gpa);
> +
> +	GUEST_ASSERT_EQ(vmcb->control.exit_code, SVM_EXIT_ERR);
> +	GUEST_DONE();
> +}
> +
> +static void run_test(const char *test_name, void *guest_code, bool do_save_restore)
> +{
> +	struct kvm_x86_state *state;
> +	struct kvm_vcpu *vcpu;
> +	struct kvm_vm *vm;
> +	struct ucall uc;
> +	gva_t svm_gva;
> +
> +	pr_info("Testing: %s\n", test_name);
> +
> +	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
> +	vm_enable_cap(vm, KVM_CAP_DISABLE_QUIRKS2,
> +		      KVM_X86_QUIRK_NESTED_SVM_SHARED_PAT);
> +
> +	if (npt_enabled) {
> +		vm_enable_npt(vm);
> +		tdp_identity_map_default_memslots(vm);
> +	}
> +
> +	vcpu_alloc_svm(vm, &svm_gva);
> +	vcpu_args_set(vcpu, 1, svm_gva);
> +	sync_global_to_guest(vm, npt_enabled);
> +	sync_global_to_guest(vm, nr_iterations);
> +
> +	for (;;) {
> +		vcpu_run(vcpu);
> +		TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO);
> +
> +		switch (get_ucall(vcpu, &uc)) {
> +		case UCALL_ABORT:
> +			REPORT_GUEST_ASSERT(uc);
> +			/* NOT REACHED */
> +		case UCALL_SYNC:
> +			if (do_save_restore) {
> +				pr_info("  Save/restore at sync point %ld\n",
> +					uc.args[1]);

Meh, this is noise to me.  Maybe it's marginally useful if there's a failure, but
I don't see how it can possibly provide enough information to have to avoid
hacking on the test.

> +				state = vcpu_save_state(vcpu);
> +				kvm_vm_release(vm);
> +				vcpu = vm_recreate_with_one_vcpu(vm);
> +				vm_enable_cap(vm, KVM_CAP_DISABLE_QUIRKS2,
> +					      KVM_X86_QUIRK_NESTED_SVM_SHARED_PAT);
> +				vcpu_load_state(vcpu, state);
> +				kvm_x86_state_cleanup(state);
> +			}
> +			break;
> +		case UCALL_DONE:
> +			pr_info("  PASSED\n");

This is useless and noisy.  The test runs in ~.25 seconds, and most of that is
the printks.  If there's a failure, I'll see.  Otherwise it's a pretty safe
assumption the test passed.

> +			kvm_vm_free(vm);
> +			return;
> +		default:
> +			TEST_FAIL("Unknown ucall %lu", uc.cmd);
> +		}
> +	}
> +}
> +
> +#define NPT_DISABLED 0
> +#define NPT_ENABLED 1
> +
> +#define NO_SAVE_RESTORE 0
> +#define DO_SAVE_RESTORE 1

Eww.

> +
> +#define NESTED_PAT_TEST(test_name, guest_code, npt, sr, iter)	\
> +({								\

Unless you need to "return" a value, do-while (0) is preferred over ({}).  There's
a technical reason, but I can't remember it off the top of my head.

> +	npt_enabled = npt;					\
> +	nr_iterations = iter;					\
> +	run_test(test_name, guest_code, sr);			\

LOL, all of this, and the one thing that _doesn't_ use macro shenanigans is
string concatenation.

> +})
> +
> +int main(int argc, char *argv[])
> +{
> +	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_SVM));
> +	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_NPT));
> +	TEST_REQUIRE(kvm_has_cap(KVM_CAP_NESTED_STATE));
> +	TEST_REQUIRE(kvm_check_cap(KVM_CAP_DISABLE_QUIRKS2) &
> +		     KVM_X86_QUIRK_NESTED_SVM_SHARED_PAT);
> +
> +	NESTED_PAT_TEST("Invalid gPAT", l1_guest_code_invalid_gpat,
> +			NPT_ENABLED, NO_SAVE_RESTORE, 1);
> +
> +	NESTED_PAT_TEST("Nested NPT disabled", l1_guest_code,
> +			NPT_DISABLED, NO_SAVE_RESTORE, 1);
> +
> +	NESTED_PAT_TEST("Nested NPT enabled", l1_guest_code,
> +			NPT_ENABLED, NO_SAVE_RESTORE, 1);
> +
> +	NESTED_PAT_TEST("Save/Restore", l1_guest_code,
> +			NPT_ENABLED, DO_SAVE_RESTORE, 1);
> +
> +	NESTED_PAT_TEST("Multiple VM-Entries", l1_guest_code,
> +			NPT_ENABLED, NO_SAVE_RESTORE, 10);

I appreciated trying to avoid true/false literals, but this is harder to read.
At least "true" and "false" are visually different, {N,D}O_SAVE_RESTORE are one
char, and NPT_{DIS,EN}ABLED are two.

And explicitly defining the number of iterations and whether or not to do save/restore
is silly.  It requires _more_ code to provide _less_ coverage.  Just run the combos
for npt_enabled=false.

If we're going to use macro shenanigans, let's actually use them!  I've got this
working, I'll post a v4.

#define gpat_test(test_name, guest_code, npt_input)			\
do {									\
	npt_input;							\
									\
	pr_info("Testing: " test_name "\n");				\
	run_test(guest_code, false, 1);					\
									\
	if (guest_code == l1_guest_code) {				\
		pr_info("Testing: " test_name " Save/Restore\n");	\
		run_test(guest_code, true, 1);				\
									\
		pr_info("Testing: " test_name " Multiple VMRUNs\n");	\
		run_test(guest_code, false, 10);			\
	}								\
} while (0)

int main(int argc, char *argv[])
{
	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_SVM));
	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_NPT));
	TEST_REQUIRE(kvm_has_cap(KVM_CAP_NESTED_STATE));
	TEST_REQUIRE(kvm_check_cap(KVM_CAP_DISABLE_QUIRKS2) &
		     KVM_X86_QUIRK_NESTED_SVM_SHARED_PAT);

	gpat_test("Invalid gPAT", l1_guest_code_invalid_gpat, npt_enabled = true);
	gpat_test("Nested NPT disabled", l1_guest_code, npt_enabled = false);
	gpat_test("Nested NPT enabled", l1_guest_code, npt_enabled = true);

	return 0;
}

> +
> +	return 0;
> +}
> 
> base-commit: 4f256d5770febb9d61f9b57a4c79c491bf4987f1
> -- 
> 2.54.0.794.g4f17f83d09-goog
>