Re: [PATCH 00/11] KVM: fix issues with stale control fields

[Paolo Bonzini <pbonzini@redhat.com>]

> Fix two bugs where the guest could do stupid things on purpose to
> cause problems in the host.
>
> Patches 1-5 cover cases where actions done through VMCB control fields
> have to be redone if VMRUN fails.  In particular, failed VMRUNs can
> cause pending TLB flushes to be dropped.
>
> Patch 6 fixes a case where eVMCS execution controls can cause the
> host to use a stale MSR permission bitmap.  Patches 7-11 are tests
> for nested x2APIC; don't run them on an unpatched kernel.

In addition to what was reported by Sashiko, the test does not pass on
SVM.  Fixed as follows:

diff --git a/tools/testing/selftests/kvm/x86/nested_x2apic_test.c b/tools/testing/selftests/kvm/x86/nested_x2apic_test.c
index eb89d5bfc0fe..ce204ce29a9c 100644
--- a/tools/testing/selftests/kvm/x86/nested_x2apic_test.c
+++ b/tools/testing/selftests/kvm/x86/nested_x2apic_test.c
@@ -48,6 +48,7 @@ static void l1_svm_code(struct svm_test_data *svm)
 	GUEST_ASSERT_EQ(ctrl->exit_code, SVM_EXIT_CPUID);

 	stgi();
+	x2apic_write_reg(APIC_TASKPRI, 0);
 }

 static void l1_vmx_code(struct vmx_pages *vmx, struct hyperv_test_pages *hv_pages)
@@ -80,14 +81,12 @@ static void l1_vmx_code(struct vmx_pages *vmx, struct hyperv_test_pages *hv_page
 	control |= CPU_BASED_USE_MSR_BITMAPS | CPU_BASED_TPR_SHADOW;
 	GUEST_ASSERT_EQ(vmwrite(CPU_BASED_VM_EXEC_CONTROL, control), 0);

-	if (control & CPU_BASED_ACTIVATE_SECONDARY_CONTROLS) {
-		control = vmreadz(SECONDARY_VM_EXEC_CONTROL);
-		control |= SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE |
-			   SECONDARY_EXEC_APIC_REGISTER_VIRT |
-			   SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY;
-		control &= (rdmsr(MSR_IA32_VMX_PROCBASED_CTLS2) >> 32);
-		GUEST_ASSERT_EQ(vmwrite(SECONDARY_VM_EXEC_CONTROL, control), 0);
-	}
+	control = vmreadz(SECONDARY_VM_EXEC_CONTROL);
+	control |= SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE |
+		   SECONDARY_EXEC_APIC_REGISTER_VIRT |
+		   SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY;
+	control &= (rdmsr(MSR_IA32_VMX_PROCBASED_CTLS2) >> 32);
+	GUEST_ASSERT_EQ(vmwrite(SECONDARY_VM_EXEC_CONTROL, control), 0);

 	GUEST_ASSERT(!vmlaunch());
 	GUEST_ASSERT_EQ(vmreadz(VM_EXIT_REASON), EXIT_REASON_CPUID);
@@ -103,7 +102,7 @@ static void l1_vmx_code_part2(void)
 	control &= ~CPU_BASED_TPR_SHADOW;
 	GUEST_ASSERT_EQ(vmwrite(CPU_BASED_VM_EXEC_CONTROL, control), 0);

-	control = vmread(SECONDARY_VM_EXEC_CONTROL, &control);
+	control = vmreadz(SECONDARY_VM_EXEC_CONTROL);
 	control &= ~(SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE |
 			SECONDARY_EXEC_APIC_REGISTER_VIRT |
 			SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
@@ -154,21 +153,23 @@ static void l1_guest_code(void *test_data, void *hv_pages)

 	l1_test_x2apic_intercepts();

-	if (this_cpu_has(X86_FEATURE_VMX))
+	if (this_cpu_has(X86_FEATURE_VMX)) {
 		l1_vmx_code_part2();
-
-	l1_test_x2apic_intercepts();
+		l1_test_x2apic_intercepts();
+	}

 	GUEST_DONE();
 }

-static void __test_x2apic_intercepts(bool use_evmcs)
+static void test_x2apic_intercepts(bool with_inhibit_apicv, bool use_evmcs)
 {
 	gva_t nested_test_data_gva, hv_pages_gva = 0;
 	struct kvm_vcpu *vcpu;
 	struct kvm_vm *vm;
 	struct ucall uc;

+	inhibit_apicv = with_inhibit_apicv;
+
 	vm = vm_create_with_one_vcpu(&vcpu, l1_guest_code);
 	vm_install_exception_handler(vm, POSTED_INTR_VECTOR, guest_irq_handler);
 	vm_install_exception_handler(vm, POSTED_INTR_WAKEUP_VECTOR, guest_irq_handler);
@@ -207,29 +208,15 @@ static void __test_x2apic_intercepts(bool use_evmcs)
 	kvm_vm_free(vm);
 }

-#define _test_x2apic_intercepts(inhibit_apic_setting)	\
-do {							\
-							\
-	inhibit_apic_setting;				\
-							\
-	__test_x2apic_intercepts(use_evmcs);		\
-} while (0)
-
-#define test_x2apic_intercepts(use_evmcs_setting)	\
-do {							\
-	bool use_evmcs_setting;				\
-							\
-	_test_x2apic_intercepts(inhibit_apicv = true);	\
-	_test_x2apic_intercepts(inhibit_apicv = false);	\
-} while (0)
-
-
 int main(int argc, char *argv[])
 {
 	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_SVM) || kvm_cpu_has(X86_FEATURE_VMX));

-	test_x2apic_intercepts(use_evmcs = false);
+	test_x2apic_intercepts(true, false);
+	test_x2apic_intercepts(false, false);

-	if (kvm_has_cap(KVM_CAP_HYPERV_ENLIGHTENED_VMCS))
-		test_x2apic_intercepts(use_evmcs = true);
+	if (kvm_has_cap(KVM_CAP_HYPERV_ENLIGHTENED_VMCS)) {
+		test_x2apic_intercepts(true, true);
+		test_x2apic_intercepts(false, true);
+	}
 }

Paolo