Date:   Thu, 2 Feb 2023 00:37:23 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Emanuele Giuseppe Esposito <eesposit@redhat.com>
Cc:     kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        "H. Peter Anvin" <hpa@zytor.com>, Shuah Khan <shuah@kernel.org>,
        Gautam Menghani <gautammenghani201@gmail.com>,
        Zeng Guang <guang.zeng@intel.com>,
        Krish Sadhukhan <krish.sadhukhan@oracle.com>,
        Jim Mattson <jmattson@google.com>,
        linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org
Subject: Re: [RFC PATCH 2/2] KVM: selftests: APIC_ID must be correctly
 updated when disabling x2apic
Message-ID: <Y9sFw0PkuR5EPm4l@google.com>
References: <20230109130605.2013555-1-eesposit@redhat.com>
 <20230109130605.2013555-3-eesposit@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230109130605.2013555-3-eesposit@redhat.com>
Precedence: bulk

On Mon, Jan 09, 2023, Emanuele Giuseppe Esposito wrote:
> Make sure the APIC_ID is correctly shifted in the right bit positions
> when disabling x2APIC via KVM_SET_MSRS.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  .../selftests/kvm/x86_64/xapic_state_test.c   | 64 +++++++++++++++++++
>  1 file changed, 64 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/x86_64/xapic_state_test.c b/tools/testing/selftests/kvm/x86_64/xapic_state_test.c
> index d7d37dae3eeb..6ebda7162a25 100644
> --- a/tools/testing/selftests/kvm/x86_64/xapic_state_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/xapic_state_test.c
> @@ -132,6 +132,62 @@ static void test_icr(struct xapic_vcpu *x)
>  	__test_icr(x, -1ull & ~APIC_DM_FIXED_MASK);
>  }
>  
> +static void _test_lapic_id

There's no need for the underscore since this is "lapic" vs. "apic", though I would
prefer to name them both "apic" and go with double underscores, i.e. __test_apic_id().

> (struct kvm_vcpu *vcpu, bool x2apic_enabled,

Pass the entire apic_base value to avoid magic booleans, and then that also lets
this helper do the vcpu_set_msr().

> +			   int expected_id)

There's no need to pass the expected APIC ID, it can be derived from vcpu->id.

> +{
> +	struct kvm_lapic_state xapic;
> +
> +	vcpu_ioctl(vcpu, KVM_GET_LAPIC, &xapic);
> +	if (x2apic_enabled)
> +		ASSERT_EQ(xapic.regs[APIC_ID], expected_id);
> +	else
> +		ASSERT_EQ(xapic.regs[0x23], expected_id);

Oof.  It's gross (we need more helpers), but the APIC_ID should be read as a 32-bit
value, both to fully validate x2APIC and to check that KVM doesn't leave bits set
in the reserved portion of APIC_ID when in xAPIC mode.

	apic_id = *((u32 *)&xapic.regs[APIC_ID]);

And then shift the expected ID instead of the actual ID so that it's more obvious
what went wrong on failure, e.g. generate errors like

	APIC_ID not set back to xAPIC format; wanted = 1000000, got = 1

versus just seeing '0' from the high byte.

> +}
> +
> +static void test_apic_id(struct kvm_vcpu *vcpu, int id)
> +{
> +	int ret;
> +	struct {
> +		struct kvm_msrs info;
> +		struct kvm_msr_entry entries[1];
> +	} msr_data = {
> +		.info.nmsrs = 1,
> +		.entries[0].index = MSR_IA32_APICBASE,
> +	};
> +
> +	/* vcpu is initialized with xAPIC enabled */
> +	ret = __vcpu_ioctl(vcpu, KVM_GET_MSRS, &msr_data.info);
> +	TEST_ASSERT(ret == 1, __KVM_IOCTL_ERROR("__vcpu_ioctl", ret));

Use vcpu_get_msr().

> +	ASSERT_EQ(msr_data.entries[0].data & MSR_IA32_APICBASE_ENABLE,
> +		  MSR_IA32_APICBASE_ENABLE);

This is hard to read.  I get annoyed with TEST_ASSERT() requiring a message, but
in this case using ASSERT_EQ() to avoid the message is a net negative (I blinked
a few times to figure out what it was asserting).

		TEST_ASSERT(apic_base & MSR_IA32_APICBASE_ENABLE,
			    "APIC not in ENABLED state at vCPU RESET");
		TEST_ASSERT(!(apic_base & X2APIC_ENABLE),
			    "APIC not in xAPIC mode at vCPU RESET");

> +	ASSERT_EQ(msr_data.entries[0].data & X2APIC_ENABLE, 0);
> +	_test_lapic_id(vcpu, false, id);
> +
> +	/* enable x2APIC */
> +	msr_data.entries[0].data |= X2APIC_ENABLE;
> +	ret = __vcpu_ioctl(vcpu, KVM_SET_MSRS, &msr_data.info);
> +	TEST_ASSERT(ret == 1, __KVM_IOCTL_ERROR("__vcpu_ioctl", ret));
> +	ASSERT_EQ(msr_data.entries[0].data & MSR_IA32_APICBASE_ENABLE,
> +		  MSR_IA32_APICBASE_ENABLE);
> +	ASSERT_EQ(msr_data.entries[0].data & X2APIC_ENABLE, X2APIC_ENABLE);
> +	_test_lapic_id(vcpu, true, id);
> +
> +	/*
> +	 * Check that disabling x2APIC correctly updates the APIC ID to the
> +	 * xAPIC format.
> +	 */
> +	msr_data.entries[0].data ^= X2APIC_ENABLE;

XOR works, but it obfuscates the code.  AND ~, or just use the original value.

> +	ret = __vcpu_ioctl(vcpu, KVM_SET_MSRS, &msr_data.info);
> +	TEST_ASSERT(ret == 1, __KVM_IOCTL_ERROR("__vcpu_ioctl", ret));
> +	ASSERT_EQ(msr_data.entries[0].data & MSR_IA32_APICBASE_ENABLE,
> +		  MSR_IA32_APICBASE_ENABLE);
> +	ASSERT_EQ(msr_data.entries[0].data & X2APIC_ENABLE, 0);
> +	_test_lapic_id(vcpu, false, id);
> +}
> +
> +#define NCPUS 3
> +
>  int main(int argc, char *argv[])
>  {
>  	struct xapic_vcpu x = {
> @@ -139,6 +195,14 @@ int main(int argc, char *argv[])
>  		.is_x2apic = true,
>  	};
>  	struct kvm_vm *vm;
> +	struct kvm_vcpu *vcpus[NCPUS] = { 0 };
> +	int i;
> +
> +	vm = vm_create_with_vcpus(NCPUS, NULL, vcpus);
> +	vm_enable_cap(vm, KVM_CAP_X2APIC_API, KVM_X2APIC_API_USE_32BIT_IDS);
> +	for (i = 0; i < NCPUS; i++)
> +		test_apic_id(vcpus[i], i);
> +	kvm_vm_free(vm);

I would prefer to put this in the helper, test_apic_id(), so that there isn't
confusion between the number of vCPUs for that sub-test and the existing tests.

This is what I ended up with:

static void __test_apic_id(struct kvm_vcpu *vcpu, uint64_t apic_base)
{
	uint32_t apic_id, expected;
	struct kvm_lapic_state xapic;

	vcpu_set_msr(vcpu, MSR_IA32_APICBASE, apic_base);

	vcpu_ioctl(vcpu, KVM_GET_LAPIC, &xapic);

	expected = apic_base & X2APIC_ENABLE ? vcpu->id : vcpu->id << 24;
	apic_id = *((u32 *)&xapic.regs[APIC_ID]);

	TEST_ASSERT(apic_id == expected,
		    "APIC_ID not set back to %s format; wanted = %x, got = %x",
		    (apic_base & X2APIC_ENABLE) ? "x2APIC" : "xAPIC",
		    expected, apic_id);
}

/*
 * Verify that KVM switches the APIC_ID between xAPIC and x2APIC when userspace
 * stuffs MSR_IA32_APICBASE.  Setting the APIC_ID when x2APIC is enabled and
 * when the APIC transitions for DISABLED to ENABLED is architectural behavior
 * (on Intel), whereas the x2APIC => xAPIC transition behavior is KVM ABI since
 * attempted to transition from x2APIC to xAPIC without disabling the APIC is
 * architecturally disallowed.
 */
static void test_apic_id(void)
{
	const uint32_t NR_VCPUS = 3;
	struct kvm_vcpu *vcpus[NR_VCPUS];
	uint64_t apic_base;
	struct kvm_vm *vm;
	int i;

	vm = vm_create_with_vcpus(NR_VCPUS, NULL, vcpus);
	vm_enable_cap(vm, KVM_CAP_X2APIC_API, KVM_X2APIC_API_USE_32BIT_IDS);

	for (i = 0; i < NR_VCPUS; i++) {
		apic_base = vcpu_get_msr(vcpus[i], MSR_IA32_APICBASE);

		TEST_ASSERT(apic_base & MSR_IA32_APICBASE_ENABLE,
			    "APIC not in ENABLED state at vCPU RESET");
		TEST_ASSERT(!(apic_base & X2APIC_ENABLE),
			    "APIC not in xAPIC mode at vCPU RESET");

		__test_apic_id(vcpus[i], apic_base);
		__test_apic_id(vcpus[i], apic_base | X2APIC_ENABLE);
		__test_apic_id(vcpus[i], apic_base);
	}

	kvm_vm_free(vm);
}