On Mon, Apr 15, 2019 at 11:31:00AM -0700, nadav.amit@xxxxxxxxx wrote:
> From: Nadav Amit <nadav.amit@xxxxxxxxx>
>
> test_enable_x2apic() unintentionally changes the APIC base address to
> zero and resets the BSP indication. This actually causes the local APIC
> to overlap the IDT area, which is wrong. Prevent it from happening by
> keeping the APIC base address and BSP-bit as it was before.
>
> Signed-off-by: Nadav Amit <nadav.amit@xxxxxxxxx>
> ---
> x86/apic.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/x86/apic.c b/x86/apic.c
> index 51744cf..0849f87 100644
> --- a/x86/apic.c
> +++ b/x86/apic.c
> @@ -90,11 +90,11 @@ static void test_enable_x2apic(void)
> report("disabled to x2apic enabled",
> test_write_apicbase_exception(APIC_EN | APIC_EXTD));
>
> - wrmsr(MSR_IA32_APICBASE, APIC_EN);
> + wrmsr(MSR_IA32_APICBASE, APIC_EN | APIC_DEFAULT_PHYS_BASE | APIC_BSP);
> report("apic enabled to invalid state",
> test_write_apicbase_exception(APIC_EXTD));
>
> - wrmsr(MSR_IA32_APICBASE, APIC_EN | APIC_EXTD);
> + wrmsr(MSR_IA32_APICBASE, APIC_EN | APIC_EXTD | APIC_DEFAULT_PHYS_BASE | APIC_BSP);
It probably doesn't matter since AFAIK kvm-unit-tests always uses the
default base, but preserving the current base+BSP would be preferred.
The #GP tests get away with using APIC_DEFAULT_PHYS_BASE because the
WRMSR will never succeed, but even that is poor form. And the test
should also reset to xAPIC when it's done.
I think the attached patch covers everything.
> apic_write(APIC_SPIV, 0x1ff);
> } else {
> printf("x2apic not detected\n");
> --
> 2.17.1
>
>From 0905302c23f20f67483b281e33f1930135087e3a Mon Sep 17 00:00:00 2001
From: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
Date: Mon, 15 Apr 2019 11:53:00 -0700
Subject: [PATCH] x86: apic: Preserve APIC base and BSP bits during x2APIC
tests
...and reset to xAPIC mode unless x2APIC was already enabled.
Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
---
x86/apic.c | 24 +++++++++++++++---------
1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/x86/apic.c b/x86/apic.c
index 51744cf..02b6128 100644
--- a/x86/apic.c
+++ b/x86/apic.c
@@ -69,33 +69,39 @@ 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);
+ wrmsr(MSR_IA32_APICBASE, 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);
+ wrmsr(MSR_IA32_APICBASE, apicbase | APIC_EN | APIC_EXTD);
apic_write(APIC_SPIV, 0x1ff);
+
+ if (!(orig_apicbase & APIC_EXTD))
+ reset_apic();
} else {
printf("x2apic not detected\n");
--
2.21.0
