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@xxxxxxxxx>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> ---
> 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
