[coreboot-gerrit] [XS] Change in coreboot[master]: cpu/x86/cache: Call wbinvd only once CR0.CD is set

[Subrata Banik (Code Review)]

Subrata Banik has submitted this change. ( https://review.coreboot.org/c/coreboot/+/75474?usp=email )

Change subject: cpu/x86/cache: Call wbinvd only once CR0.CD is set
......................................................................

cpu/x86/cache: Call wbinvd only once CR0.CD is set

This patch removes the wbinvd call preceding CR0.CD setting in
disable_cache() to improve the boot time performances. According to
some experimental measurements, the wbinvd execution takes between 1.6
up and 6 milliseconds to complete so it is preferable to call it only
when necessary.

According to Intel Software Developer Manual Vol 3.A - 12.5.3
Preventing Caching section there is no need to flush and invalidate
the cache before settings CR0.CD. The documented sequence consists in
setting CR0.CD and then call wbinvd.

We also could not find any extra requirements in the AMD64
Architecture Programmer’s Manual - Volume 2 - Memory System chapter.

This extra wbinvd in coreboot disable_cache() function does not seem
documented and looking into the history of the project got us all the
way back to original commit 8ca8d7665d67 ("- Initial checkin of the
freebios2 tree") from April 2003.

Even the original disable_cache() implementation (see below) is a bit
curious as the comment list two actions:
1. Disable cache cover by line 74, 75 and 77
2. Write back the cache and flush TLB - Line 78
But it does not provide any explanation for the wbinvd call line 76.

68  static inline void disable_cache(void)
    69  {
    70          unsigned int tmp;
    71          /* Disable cache */
    72          /* Write back the cache and flush TLB */
    73          asm volatile (
    74                  "movl  %%cr0, %0\n\t"
    75                  "orl  $0x40000000, %0\n\t"
    76                  "wbinvd\n\t"
    77                  "movl  %0, %%cr0\n\t"
    78                  "wbinvd\n\t"
    79                  :"=r" (tmp)
    80                  ::"memory");
    81  }

BUG=b/260455826
TEST=Successful boot on Skolas and Rex board

Change-Id: I08c6486dc93c4d70cadc22a760d1b7e536e85bfa
Signed-off-by: Jeremy Compostella 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/75474
Tested-by: build bot (Jenkins) 
Reviewed-by: Subrata Banik 
Reviewed-by: Himanshu Sahdev 
---
M src/include/cpu/x86/cache.h
1 file changed, 0 insertions(+), 1 deletion(-)

Approvals:
  Subrata Banik: Looks good to me, approved
  Himanshu Sahdev: Looks good to me, but someone else must approve
  build bot (Jenkins): Verified

diff --git a/src/include/cpu/x86/cache.h b/src/include/cpu/x86/cache.h
index d35c4e7..4143d97 100644
--- a/src/include/cpu/x86/cache.h
+++ b/src/include/cpu/x86/cache.h
@@ -57,7 +57,6 @@
    CRx_TYPE cr0;
    cr0 = read_cr0();
    cr0 |= CR0_CD;
-	wbinvd();
    write_cr0(cr0);
    wbinvd();
 }