[kvm-unit-tests PATCH] x86: apic: Preserve APIC base and BSP bits during x2APIC tests

[kvm-unit-tests PATCH] x86: apic: Preserve APIC base and BSP bits during x2APIC tests

[Sean Christopherson]

...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");
-- 
2.21.0

Re: [kvm-unit-tests PATCH] x86: apic: Preserve APIC base and BSP bits during x2APIC tests

[Paolo Bonzini]

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