| Message ID | 20190415200034.6839-1-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests] x86: apic: Preserve APIC base and BSP bits during x2APIC tests | expand |
On 15/04/19 22:00, Sean Christopherson wrote: > ...and reset to xAPIC mode unless x2APIC was already enabled. Make the > "APIC disabled to APIC enabled" a reported subtest so that it's > (slightly) more obviously that the vCPU is transitioning back to legacy > xAPIC mode and that the APIC was disabled via a previous subtest, i.e. > won't fault due to an illegal transition. > > Note, reset_apic() is overkill since the vCPU is already in legacy xAPIC > mode, i.e. the test really just needs "apic_ops = &xapic_ops;". But two > MSR accesses are negligible in the grand scheme and using reset_apic() is > less reliant on the exact flow of the test. > > Reported-by: Nadav Amit <nadav.amit@gmail.com> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > x86/apic.c | 30 +++++++++++++++++++++--------- > 1 file changed, 21 insertions(+), 9 deletions(-) > > diff --git a/x86/apic.c b/x86/apic.c > index 51744cf..7f711c0 100644 > --- a/x86/apic.c > +++ b/x86/apic.c > @@ -69,32 +69,44 @@ static void do_write_apicbase(void *data) > > static bool test_write_apicbase_exception(u64 data) > { > - data |= APIC_DEFAULT_PHYS_BASE | APIC_BSP; > return test_for_exception(GP_VECTOR, do_write_apicbase, &data); > } > > static void test_enable_x2apic(void) > { > + u64 orig_apicbase = rdmsr(MSR_IA32_APICBASE); > + u64 apicbase; > + > if (enable_x2apic()) { > printf("x2apic enabled\n"); > > + apicbase = orig_apicbase & ~(APIC_EN | APIC_EXTD); > report("x2apic enabled to invalid state", > - test_write_apicbase_exception(APIC_EXTD)); > + test_write_apicbase_exception(apicbase | APIC_EXTD)); > report("x2apic enabled to apic enabled", > - test_write_apicbase_exception(APIC_EN)); > + test_write_apicbase_exception(apicbase | APIC_EN)); > > report("x2apic enabled to disabled state", > - !test_write_apicbase_exception(0)); > + !test_write_apicbase_exception(apicbase | 0)); > report("disabled to invalid state", > - test_write_apicbase_exception(APIC_EXTD)); > + test_write_apicbase_exception(apicbase | APIC_EXTD)); > report("disabled to x2apic enabled", > - test_write_apicbase_exception(APIC_EN | APIC_EXTD)); > + test_write_apicbase_exception(apicbase | APIC_EN | APIC_EXTD)); > > - wrmsr(MSR_IA32_APICBASE, APIC_EN); > + report("apic disabled to apic enabled", > + !test_write_apicbase_exception(apicbase | APIC_EN)); > report("apic enabled to invalid state", > - test_write_apicbase_exception(APIC_EXTD)); > + test_write_apicbase_exception(apicbase | APIC_EXTD)); > > - wrmsr(MSR_IA32_APICBASE, APIC_EN | APIC_EXTD); > + if (orig_apicbase & APIC_EXTD) > + enable_x2apic(); > + else > + reset_apic(); > + > + /* > + * Disabling the APIC resets various APIC registers, restore them to > + * their desired values. > + */ > apic_write(APIC_SPIV, 0x1ff); > } else { > printf("x2apic not detected\n"); > Queued, thanks. Paolo
diff --git a/x86/apic.c b/x86/apic.c index 51744cf..7f711c0 100644 --- a/x86/apic.c +++ b/x86/apic.c @@ -69,32 +69,44 @@ static void do_write_apicbase(void *data) static bool test_write_apicbase_exception(u64 data) { - data |= APIC_DEFAULT_PHYS_BASE | APIC_BSP; return test_for_exception(GP_VECTOR, do_write_apicbase, &data); } static void test_enable_x2apic(void) { + u64 orig_apicbase = rdmsr(MSR_IA32_APICBASE); + u64 apicbase; + if (enable_x2apic()) { printf("x2apic enabled\n"); + apicbase = orig_apicbase & ~(APIC_EN | APIC_EXTD); report("x2apic enabled to invalid state", - test_write_apicbase_exception(APIC_EXTD)); + test_write_apicbase_exception(apicbase | APIC_EXTD)); report("x2apic enabled to apic enabled", - test_write_apicbase_exception(APIC_EN)); + test_write_apicbase_exception(apicbase | APIC_EN)); report("x2apic enabled to disabled state", - !test_write_apicbase_exception(0)); + !test_write_apicbase_exception(apicbase | 0)); report("disabled to invalid state", - test_write_apicbase_exception(APIC_EXTD)); + test_write_apicbase_exception(apicbase | APIC_EXTD)); report("disabled to x2apic enabled", - test_write_apicbase_exception(APIC_EN | APIC_EXTD)); + test_write_apicbase_exception(apicbase | APIC_EN | APIC_EXTD)); - wrmsr(MSR_IA32_APICBASE, APIC_EN); + report("apic disabled to apic enabled", + !test_write_apicbase_exception(apicbase | APIC_EN)); report("apic enabled to invalid state", - test_write_apicbase_exception(APIC_EXTD)); + test_write_apicbase_exception(apicbase | APIC_EXTD)); - wrmsr(MSR_IA32_APICBASE, APIC_EN | APIC_EXTD); + if (orig_apicbase & APIC_EXTD) + enable_x2apic(); + else + reset_apic(); + + /* + * Disabling the APIC resets various APIC registers, restore them to + * their desired values. + */ apic_write(APIC_SPIV, 0x1ff); } else { printf("x2apic not detected\n");
...and reset to xAPIC mode unless x2APIC was already enabled. Make the "APIC disabled to APIC enabled" a reported subtest so that it's (slightly) more obviously that the vCPU is transitioning back to legacy xAPIC mode and that the APIC was disabled via a previous subtest, i.e. won't fault due to an illegal transition. Note, reset_apic() is overkill since the vCPU is already in legacy xAPIC mode, i.e. the test really just needs "apic_ops = &xapic_ops;". But two MSR accesses are negligible in the grand scheme and using reset_apic() is less reliant on the exact flow of the test. Reported-by: Nadav Amit <nadav.amit@gmail.com> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- x86/apic.c | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-)