Re: [PATCH 3/3] KVM: selftests: Add test for race in kvm_recalculate_apic_map()

[Sean Christopherson <seanjc@google.com>]

On Thu, May 25, 2023, Michal Luczaj wrote:
> Keep switching between LAPIC_MODE_X2APIC and LAPIC_MODE_DISABLED during
> APIC map construction.
> 
> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> ---
>  tools/testing/selftests/kvm/Makefile          |   1 +
>  .../kvm/x86_64/recalc_apic_map_race.c         | 110 ++++++++++++++++++
>  2 files changed, 111 insertions(+)
>  create mode 100644 tools/testing/selftests/kvm/x86_64/recalc_apic_map_race.c
> 
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index 7a5ff646e7e7..c9b8f16fb23f 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -116,6 +116,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/sev_migrate_tests
>  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_race
>  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_apic_map_race.c b/tools/testing/selftests/kvm/x86_64/recalc_apic_map_race.c
> new file mode 100644
> index 000000000000..1122df858623
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86_64/recalc_apic_map_race.c
> @@ -0,0 +1,110 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * recalc_apic_map_race
> + *
> + * Test for a race condition in kvm_recalculate_apic_map().
> + */
> +
> +#include <sys/ioctl.h>
> +#include <pthread.h>
> +#include <time.h>
> +
> +#include "processor.h"
> +#include "test_util.h"
> +#include "kvm_util.h"
> +#include "apic.h"
> +
> +#define TIMEOUT			5	/* seconds */
> +#define STUFFING		100
> +
> +#define LAPIC_MODE_DISABLED	0
> +#define LAPIC_MODE_X2APIC	(MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE)
> +#define MAX_XAPIC_ID		0xff
> +
> +static int add_vcpu(struct kvm_vm *vm, long id)
> +{
> +	int vcpu = __vm_ioctl(vm, KVM_CREATE_VCPU, (void *)id);
> +
> +	static struct {
> +		struct kvm_cpuid2 head;
> +		struct kvm_cpuid_entry2 entry;
> +	} cpuid = {
> +		.head.nent = 1,
> +		/* X86_FEATURE_X2APIC */
> +		.entry = {
> +			.function = 0x1,
> +			.index = 0,
> +			.ecx = 1UL << 21
> +		}
> +	};
> +
> +	ASSERT_EQ(ioctl(vcpu, KVM_SET_CPUID2, &cpuid.head), 0);
> +
> +	return vcpu;
> +}
> +
> +static void set_apicbase(int vcpu, u64 mode)
> +{
> +	struct {
> +		struct kvm_msrs head;
> +		struct kvm_msr_entry entry;
> +	} msr = {
> +		.head.nmsrs = 1,
> +		.entry = {
> +			.index = MSR_IA32_APICBASE,
> +			.data = mode
> +		}
> +	};
> +
> +	ASSERT_EQ(ioctl(vcpu, KVM_SET_MSRS, &msr.head), msr.head.nmsrs);
> +}
> +
> +static void *race(void *arg)
> +{
> +	struct kvm_lapic_state state = {};
> +	int vcpu = *(int *)arg;
> +
> +	while (1) {
> +		/* Trigger kvm_recalculate_apic_map(). */
> +		ioctl(vcpu, KVM_SET_LAPIC, &state);
> +		pthread_testcancel();
> +	}
> +
> +	return NULL;
> +}
> +
> +int main(void)
> +{
> +	int vcpu0, vcpuN, i;
> +	struct kvm_vm *vm;
> +	pthread_t thread;
> +	time_t t;
> +
> +	vm = vm_create_barebones();
> +	vm_create_irqchip(vm);

All of these open coded ioctl() calls is unnecessary.  Ditto for the fancy
stuffing, which through trial and error I discovered is done to avoid having
vCPUs with aliased xAPIC IDs, which would cause KVM to bail before triggering
the bug.  It's much easier to just create the max number of vCPUs and enable
x2APIC on all of them.

This can all be distilled down to:

// SPDX-License-Identifier: GPL-2.0-only
/*
 * recalc_apic_map_race
 *
 * Test for a race condition in kvm_recalculate_apic_map().
 */

#include <sys/ioctl.h>
#include <pthread.h>
#include <time.h>

#include "processor.h"
#include "test_util.h"
#include "kvm_util.h"
#include "apic.h"

#define TIMEOUT		5	/* seconds */

#define LAPIC_DISABLED	0
#define LAPIC_X2APIC	(MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE)
#define MAX_XAPIC_ID	0xff

static void *race(void *arg)
{
	struct kvm_lapic_state lapic = {};
	struct kvm_vcpu *vcpu = arg;

	while (1) {
		/* Trigger kvm_recalculate_apic_map(). */
		vcpu_ioctl(vcpu, KVM_SET_LAPIC, &lapic);
		pthread_testcancel();
	}

	return NULL;
}

int main(void)
{
	struct kvm_vcpu *vcpus[KVM_MAX_VCPUS];
	struct kvm_vcpu *vcpuN;
	struct kvm_vm *vm;
	pthread_t thread;
	time_t t;
	int i;

	kvm_static_assert(KVM_MAX_VCPUS > MAX_XAPIC_ID);

	/*
	 * Creating the max number of vCPUs supported by selftests so that KVM
	 * has decent amount of work to do when recalculating the map, i.e. to
	 * make the problematic window large enough to hit.
	 */
	vm = vm_create_with_vcpus(KVM_MAX_VCPUS, NULL, vcpus);

	/*
	 * Enable x2APIC on all vCPUs so that KVM doesn't bail from the recalc
	 * due to vCPUs having aliased xAPIC IDs (truncated to 8 bits).
	 */
	for (i = 0; i < KVM_MAX_VCPUS; i++)
		vcpu_set_msr(vcpus[i], MSR_IA32_APICBASE, LAPIC_X2APIC);

	ASSERT_EQ(pthread_create(&thread, NULL, race, vcpus[0]), 0);

	vcpuN = vcpus[KVM_MAX_VCPUS - 1];
	for (t = time(NULL) + TIMEOUT; time(NULL) < t;) {
		vcpu_set_msr(vcpuN, MSR_IA32_APICBASE, LAPIC_X2APIC);
		vcpu_set_msr(vcpuN, MSR_IA32_APICBASE, LAPIC_DISABLED);
	}

	ASSERT_EQ(pthread_cancel(thread), 0);
	ASSERT_EQ(pthread_join(thread, NULL), 0);

	kvm_vm_release(vm);

	return 0;
}