[kvm-unit-tests PATCH] x86: Disable cache before relocating local APIC

[kvm-unit-tests PATCH] x86: Disable cache before relocating local APIC

[nadav.amit]

From: Nadav Amit <nadav.amit@gmail.com>

According to the SDM, during initialization, the BSP "Switches to
protected mode and ensures that the APIC address space is mapped to the
strong uncacheable (UC) memory type."

This requirement is not followed when the tests relocate the APIC. Set
the cache-disable flag while the APIC base is reprogrammed. According
to the SDM, the MTRRs should be modified as well, but it seems somewhat
complicated to do that and probably unnecessary.

Cc: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
---
 lib/x86/processor.h | 1 +
 x86/apic.c          | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/lib/x86/processor.h b/lib/x86/processor.h
index 916e67d..59137da 100644
--- a/lib/x86/processor.h
+++ b/lib/x86/processor.h
@@ -29,6 +29,7 @@
 #define X86_CR0_TS     0x00000008
 #define X86_CR0_WP     0x00010000
 #define X86_CR0_AM     0x00040000
+#define X86_CR0_CD     0x40000000
 #define X86_CR0_PG     0x80000000
 #define X86_CR3_PCID_MASK 0x00000fff
 #define X86_CR4_TSD    0x00000004
diff --git a/x86/apic.c b/x86/apic.c
index de5990c..de4a181 100644
--- a/x86/apic.c
+++ b/x86/apic.c
@@ -165,8 +165,13 @@ static void test_apicbase(void)
 {
     u64 orig_apicbase = rdmsr(MSR_IA32_APICBASE);
     u32 lvr = apic_read(APIC_LVR);
+    u64 cr0 = read_cr0();
     u64 value;
 
+    /* Disable caching to prevent the APIC from being cacheable */
+    write_cr0(cr0 | X86_CR0_CD);
+    asm volatile ("wbinvd" ::: "memory");
+
     wrmsr(MSR_IA32_APICBASE, orig_apicbase & ~(APIC_EN | APIC_EXTD));
     wrmsr(MSR_IA32_APICBASE, ALTERNATE_APIC_BASE | APIC_BSP | APIC_EN);
 
@@ -186,6 +191,8 @@ static void test_apicbase(void)
     wrmsr(MSR_IA32_APICBASE, orig_apicbase);
     apic_write(APIC_SPIV, 0x1ff);
 
+    write_cr0(cr0);
+
     report_prefix_pop();
 }
 
-- 
2.17.1

Re: [kvm-unit-tests PATCH] x86: Disable cache before relocating local APIC

[Sean Christopherson]

On Tue, Apr 30, 2019 at 05:27:01AM -0700, nadav.amit@gmail.com wrote:
> From: Nadav Amit <nadav.amit@gmail.com>
> 
> According to the SDM, during initialization, the BSP "Switches to
> protected mode and ensures that the APIC address space is mapped to the
> strong uncacheable (UC) memory type."
> 
> This requirement is not followed when the tests relocate the APIC. Set
> the cache-disable flag while the APIC base is reprogrammed. According
> to the SDM, the MTRRs should be modified as well, but it seems somewhat
> complicated to do that and probably unnecessary.

Alternatively, what about defining ALTERNATE_APIC_BASE to a sane value
that is guaranteed to be UC (assuming BIOS isn't being mean)?  Maybe a
use well-known address, e.g. 0xfed40000 from the TPM, so as to avoid any
other unwanted side effects.

> Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
> ---
>  lib/x86/processor.h | 1 +
>  x86/apic.c          | 7 +++++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/lib/x86/processor.h b/lib/x86/processor.h
> index 916e67d..59137da 100644
> --- a/lib/x86/processor.h
> +++ b/lib/x86/processor.h
> @@ -29,6 +29,7 @@
>  #define X86_CR0_TS     0x00000008
>  #define X86_CR0_WP     0x00010000
>  #define X86_CR0_AM     0x00040000
> +#define X86_CR0_CD     0x40000000
>  #define X86_CR0_PG     0x80000000
>  #define X86_CR3_PCID_MASK 0x00000fff
>  #define X86_CR4_TSD    0x00000004
> diff --git a/x86/apic.c b/x86/apic.c
> index de5990c..de4a181 100644
> --- a/x86/apic.c
> +++ b/x86/apic.c
> @@ -165,8 +165,13 @@ static void test_apicbase(void)
>  {
>      u64 orig_apicbase = rdmsr(MSR_IA32_APICBASE);
>      u32 lvr = apic_read(APIC_LVR);
> +    u64 cr0 = read_cr0();
>      u64 value;
>  
> +    /* Disable caching to prevent the APIC from being cacheable */
> +    write_cr0(cr0 | X86_CR0_CD);
> +    asm volatile ("wbinvd" ::: "memory");
> +
>      wrmsr(MSR_IA32_APICBASE, orig_apicbase & ~(APIC_EN | APIC_EXTD));
>      wrmsr(MSR_IA32_APICBASE, ALTERNATE_APIC_BASE | APIC_BSP | APIC_EN);
>  
> @@ -186,6 +191,8 @@ static void test_apicbase(void)
>      wrmsr(MSR_IA32_APICBASE, orig_apicbase);
>      apic_write(APIC_SPIV, 0x1ff);
>  
> +    write_cr0(cr0);
> +
>      report_prefix_pop();
>  }
>  
> -- 
> 2.17.1
> 

Re: [kvm-unit-tests PATCH] x86: Disable cache before relocating local APIC

[Nadav Amit]

> On Apr 30, 2019, at 1:27 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> 
> On Tue, Apr 30, 2019 at 05:27:01AM -0700, nadav.amit@gmail.com wrote:
>> From: Nadav Amit <nadav.amit@gmail.com>
>> 
>> According to the SDM, during initialization, the BSP "Switches to
>> protected mode and ensures that the APIC address space is mapped to the
>> strong uncacheable (UC) memory type."
>> 
>> This requirement is not followed when the tests relocate the APIC. Set
>> the cache-disable flag while the APIC base is reprogrammed. According
>> to the SDM, the MTRRs should be modified as well, but it seems somewhat
>> complicated to do that and probably unnecessary.
> 
> Alternatively, what about defining ALTERNATE_APIC_BASE to a sane value
> that is guaranteed to be UC (assuming BIOS isn't being mean)?  Maybe a
> use well-known address, e.g. 0xfed40000 from the TPM, so as to avoid any
> other unwanted side effects.

Cool. This solution is much better than mine and it works for me. I’ll send
a different patch instead.