Re: [kvm-unit-tests PATCH] x86: Prevent APIC base address from changing in test_enable_x2apic()

[Sean Christopherson <sean.j.christopherson@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 1707 bytes --]

On Mon, Apr 15, 2019 at 11:31:00AM -0700, nadav.amit@gmail.com wrote:
> From: Nadav Amit <nadav.amit@gmail.com>
> 
> 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@gmail.com>
> ---
>  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
> 

[-- Attachment #2: 0001-x86-apic-Preserve-APIC-base-and-BSP-bits-during-x2AP.patch --]
[-- Type: text/x-diff, Size: 2541 bytes --]

From 0905302c23f20f67483b281e33f1930135087e3a Mon Sep 17 00:00:00 2001
From: Sean Christopherson <sean.j.christopherson@intel.com>
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@intel.com>
---
 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