Re: [PATCH 1/2] KVM: x86: Make x2APIC ID 100% readonly

[Michal Luczaj <mhal@rbox.co>]

On 8/2/24 22:29, Sean Christopherson wrote:
> [...]
> Making the x2APIC ID fully readonly fixes a WARN in KVM's optimized map
> calculation, which expects the LDR to align with the x2APIC ID.
> 
>   WARNING: CPU: 2 PID: 958 at arch/x86/kvm/lapic.c:331 kvm_recalculate_apic_map+0x609/0xa00 [kvm]
>   CPU: 2 PID: 958 Comm: recalc_apic_map Not tainted 6.4.0-rc3-vanilla+ #35
>   Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Arch Linux 1.16.2-1-1 04/01/2014
>   RIP: 0010:kvm_recalculate_apic_map+0x609/0xa00 [kvm]
>   Call Trace:
>    <TASK>
>    kvm_apic_set_state+0x1cf/0x5b0 [kvm]
>    kvm_arch_vcpu_ioctl+0x1806/0x2100 [kvm]
>    kvm_vcpu_ioctl+0x663/0x8a0 [kvm]
>    __x64_sys_ioctl+0xb8/0xf0
>    do_syscall_64+0x56/0x80
>    entry_SYSCALL_64_after_hwframe+0x46/0xb0
>   RIP: 0033:0x7fade8b9dd6f

Isn't this WARN_ON_ONCE() inherently racy, though? With your patch applied,
it can still be hit by juggling the APIC modes.

[   53.882945] WARNING: CPU: 13 PID: 1181 at arch/x86/kvm/lapic.c:355 kvm_recalculate_apic_map+0x335/0x650 [kvm]
[   53.883007] CPU: 13 UID: 1000 PID: 1181 Comm: recalc_logical_ Not tainted 6.11.0-rc1nokasan+ #18
[   53.883009] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Arch Linux 1.16.3-1-1 04/01/2014
[   53.883010] RIP: 0010:kvm_recalculate_apic_map+0x335/0x650 [kvm]
[   53.883057] Call Trace:
[   53.883058]  <TASK>
[   53.883169]  kvm_apic_set_state+0x105/0x3d0 [kvm]
[   53.883201]  kvm_arch_vcpu_ioctl+0xf09/0x19c0 [kvm]
[   53.883285]  kvm_vcpu_ioctl+0x6cc/0x920 [kvm]
[   53.883310]  __x64_sys_ioctl+0x90/0xd0
[   53.883313]  do_syscall_64+0x93/0x180
[   53.883623]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
[   53.883625] RIP: 0033:0x7fd90fee0d2d

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 48d32c5aa3eb..3344f1478230 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -129,6 +129,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/amx_test
 TEST_GEN_PROGS_x86_64 += x86_64/max_vcpuid_cap_test
 TEST_GEN_PROGS_x86_64 += x86_64/triple_fault_event_test
 TEST_GEN_PROGS_x86_64 += x86_64/recalc_apic_map_test
+TEST_GEN_PROGS_x86_64 += x86_64/recalc_logical_map_warn
 TEST_GEN_PROGS_x86_64 += access_tracking_perf_test
 TEST_GEN_PROGS_x86_64 += demand_paging_test
 TEST_GEN_PROGS_x86_64 += dirty_log_test
diff --git a/tools/testing/selftests/kvm/x86_64/recalc_logical_map_warn.c b/tools/testing/selftests/kvm/x86_64/recalc_logical_map_warn.c
new file mode 100644
index 000000000000..ad3ae0433230
--- /dev/null
+++ b/tools/testing/selftests/kvm/x86_64/recalc_logical_map_warn.c
@@ -0,0 +1,49 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * WARN_ON_ONCE(ldr != kvm_apic_calc_x2apic_ldr(kvm_x2apic_id(apic)))
+ * in arch/x86/kvm/lapic.c:kvm_recalculate_logical_map()
+ */
+
+#include <pthread.h>
+
+#include "processor.h"
+#include "kvm_util.h"
+#include "apic.h"
+
+#define LAPIC_XAPIC	MSR_IA32_APICBASE_ENABLE
+#define LAPIC_X2APIC	(MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE)
+
+static void *race(void *arg)
+{
+	struct kvm_lapic_state state = {};
+	struct kvm_vcpu *vcpu0 = arg;
+
+	/* Trigger kvm_recalculate_apic_map(). */
+	for (;;)
+		__vcpu_ioctl(vcpu0, KVM_SET_LAPIC, &state);
+
+	return NULL;
+}
+
+int main(void)
+{
+	struct kvm_vcpu *vcpus[2];
+	struct kvm_vm *vm;
+	pthread_t thread;
+
+	vm = vm_create_with_vcpus(2, NULL, vcpus);
+
+	vcpu_set_msr(vcpus[1], MSR_IA32_APICBASE, LAPIC_X2APIC);
+	vcpu_set_msr(vcpus[1], APIC_BASE_MSR + (APIC_SPIV >> 4), APIC_SPIV_APIC_ENABLED);
+
+	TEST_ASSERT_EQ(pthread_create(&thread, NULL, race, vcpus[0]), 0);
+
+	for (;;) {
+		_vcpu_set_msr(vcpus[1], MSR_IA32_APICBASE, LAPIC_XAPIC);
+		_vcpu_set_msr(vcpus[1], MSR_IA32_APICBASE, LAPIC_X2APIC);
+	}
+
+	kvm_vm_free(vm);
+
+	return 0;
+}