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); }