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@xxxxxxxxxx>
> ---
> .../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);
}
