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

[Paolo Bonzini <pbonzini@redhat.com>]

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