| Message ID | 20230109130605.2013555-3-eesposit@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | eb98192576315d3f4c6c990d589ab398e7091782 |
| Headers | show |
| Series | xapic: make sure x2APIC -> xapic transition correctly | expand |
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); }
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(struct kvm_vcpu *vcpu, bool x2apic_enabled, + int expected_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); + +} + +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)); + 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); + + /* 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; + 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); vm = vm_create_with_one_vcpu(&x.vcpu, x2apic_guest_code); test_icr(&x);
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(+)