Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36382 )
Change subject: cpu/x86/mtrr: Add function to set MTRR with CR0.CD set ......................................................................
cpu/x86/mtrr: Add function to set MTRR with CR0.CD set
MTRRs should only be changed when CR0.CD is set. In a CAR environment this is a rather fragile thing to do. This is why it is best to implement this in assembly.
Change-Id: I4ff59d35ade125f60ed0002a386f41fd8ad54073 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/x86/mtrr/Makefile.inc M src/cpu/x86/mtrr/earlymtrr.c A src/cpu/x86/mtrr/set_mtrr.S M src/include/cpu/x86/mtrr.h 4 files changed, 88 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/36382/1
diff --git a/src/cpu/x86/mtrr/Makefile.inc b/src/cpu/x86/mtrr/Makefile.inc index caa6e9c..4052c52 100644 --- a/src/cpu/x86/mtrr/Makefile.inc +++ b/src/cpu/x86/mtrr/Makefile.inc @@ -3,6 +3,9 @@ romstage-y += earlymtrr.c bootblock-y += earlymtrr.c
+bootblock-y += set_mtrr.S +verstage-y += set_mtrr.S + bootblock-y += debug.c romstage-y += debug.c postcar-y += debug.c diff --git a/src/cpu/x86/mtrr/earlymtrr.c b/src/cpu/x86/mtrr/earlymtrr.c index 02dfbdc..2f6a64d 100644 --- a/src/cpu/x86/mtrr/earlymtrr.c +++ b/src/cpu/x86/mtrr/earlymtrr.c @@ -52,3 +52,17 @@ maskm.hi = (1 << (cpu_phys_address_size() - 32)) - 1; wrmsr(MTRR_PHYS_MASK(reg), maskm); } + +void set_var_mtrr_uncached( + unsigned int reg, unsigned int base, unsigned int size, + unsigned int type) +{ + /* Bit Bit 32-35 of MTRRphysMask should be set to 1 */ + /* FIXME: It only support 4G less range */ + msr_t basem, maskm; + basem.lo = base | type; + basem.hi = 0; + maskm.lo = ~(size - 1) | MTRR_PHYS_MASK_VALID; + maskm.hi = (1 << (cpu_phys_address_size() - 32)) - 1; + set_var_mtrr_uncached_asm(reg, basem, maskm); +} diff --git a/src/cpu/x86/mtrr/set_mtrr.S b/src/cpu/x86/mtrr/set_mtrr.S new file mode 100644 index 0000000..43c19f7 --- /dev/null +++ b/src/cpu/x86/mtrr/set_mtrr.S @@ -0,0 +1,67 @@ +/* + * This file is part of the coreboot project. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ +#include <cpu/x86/mtrr.h> +#include <cpu/x86/cache.h> +#include <cpu/x86/post_code.h> +.global set_var_mtrr_uncached_asm + /* ARG0: mtrr_num + * ARG1: mtrr_base.lo + * ARG2: mtrr_base.hi + * ARG3: mtrr_mask.lo + * ARG4: mtrr_mask.hi + */ +.code32 +set_var_mtrr_uncached_asm: + /* Callee saved registers */ + pushl %ebp + movl %esp, %ebp + addl $8, %ebp + pushl %ebx + pushl %edx + pushl %esi + pushl %edi + + movl 0(%ebp), %ecx + movl 4(%ebp), %ebx + movl 8(%ebp), %edx + movl 12(%ebp), %esi + movl 16(%ebp), %edi + + /* Disable caching before setting MTRR */ + movl %cr0, %eax + orl $CR0_CacheDisable, %eax + movl %eax, %cr0 + + /* MTRR_PHYS_BASE */ + imul $2, %ecx, %ecx + addl $MTRR_PHYS_BASE(0), %ecx + movl %ebx, %eax + wrmsr + /* MTRR_PHYS_MASK */ + addl $1, %ecx + movl %esi, %eax + movl %edi, %edx + wrmsr + + /* Enable cache again. */ + movl %cr0, %eax + andl $(~(CR0_CacheDisable | CR0_NoWriteThrough)), %eax + movl %eax, %cr0 + + /* Restore Callee saved registers */ + popl %edi + popl %esi + popl %edx + popl %ebx + popl %ebp + ret diff --git a/src/include/cpu/x86/mtrr.h b/src/include/cpu/x86/mtrr.h index 29256c8..103388f 100644 --- a/src/include/cpu/x86/mtrr.h +++ b/src/include/cpu/x86/mtrr.h @@ -107,6 +107,10 @@ void set_var_mtrr(unsigned int reg, unsigned int base, unsigned int size, unsigned int type); int get_free_var_mtrr(void); +void set_var_mtrr_uncached(unsigned int reg, unsigned int base, + unsigned int size, unsigned int type); +__attribute__((cdecl)) void set_var_mtrr_uncached_asm(unsigned int reg, + msr_t base, msr_t mask);
asmlinkage void display_mtrrs(void);
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36382 )
Change subject: cpu/x86/mtrr: Add function to set MTRR with CR0.CD set ......................................................................
Patch Set 1:
(6 comments)
https://review.coreboot.org/c/coreboot/+/36382/1/src/cpu/x86/mtrr/earlymtrr.... File src/cpu/x86/mtrr/earlymtrr.c:
https://review.coreboot.org/c/coreboot/+/36382/1/src/cpu/x86/mtrr/earlymtrr.... PS1, Line 55: Please comment on the purpose of the function.
https://review.coreboot.org/c/coreboot/+/36382/1/src/cpu/x86/mtrr/earlymtrr.... PS1, Line 56: uncached Not convinced by the name, but I guess it doesn't matter that much if you add a comment. set_var_mtrr_during_car()?
https://review.coreboot.org/c/coreboot/+/36382/1/src/cpu/x86/mtrr/earlymtrr.... PS1, Line 60: Bit s/Bit Bit/Bits/ ;)
https://review.coreboot.org/c/coreboot/+/36382/1/src/cpu/x86/mtrr/earlymtrr.... PS1, Line 61: support support*s*
https://review.coreboot.org/c/coreboot/+/36382/1/src/cpu/x86/mtrr/set_mtrr.S File src/cpu/x86/mtrr/set_mtrr.S:
https://review.coreboot.org/c/coreboot/+/36382/1/src/cpu/x86/mtrr/set_mtrr.S... PS1, Line 29: pushl %ebx : pushl %edx : pushl %esi : pushl %edi : : movl 0(%ebp), %ecx : movl 4(%ebp), %ebx : movl 8(%ebp), %edx : movl 12(%ebp), %esi : movl 16(%ebp), %edi Lots of boilerplate to ensure that we don't access memory while CD is set. It's worth a comment that this is a requirement, IMHO.
https://review.coreboot.org/c/coreboot/+/36382/1/src/cpu/x86/mtrr/set_mtrr.S... PS1, Line 56: /* Enable cache again. */ What if it wasn't enabled before? Sure, we shouldn't have called it. That's why I think we should make more clear when it should be used.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36382 )
Change subject: cpu/x86/mtrr: Add function to set MTRR with CR0.CD set ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/36382/1/src/cpu/x86/mtrr/Makefile.i... File src/cpu/x86/mtrr/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36382/1/src/cpu/x86/mtrr/Makefile.i... PS1, Line 7: verstage-y += set_mtrr.S Does this file always work for 64-bit?
https://review.coreboot.org/c/coreboot/+/36382/1/src/cpu/x86/mtrr/set_mtrr.S File src/cpu/x86/mtrr/set_mtrr.S:
https://review.coreboot.org/c/coreboot/+/36382/1/src/cpu/x86/mtrr/set_mtrr.S... PS1, Line 32: pushl %edi pusha ?
https://review.coreboot.org/c/coreboot/+/36382/1/src/include/cpu/x86/mtrr.h File src/include/cpu/x86/mtrr.h:
https://review.coreboot.org/c/coreboot/+/36382/1/src/include/cpu/x86/mtrr.h@... PS1, Line 112: __attribute__((cdecl)) asmlinkage exists for this purpose
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36382 )
Change subject: cpu/x86/mtrr: Add function to set MTRR with CR0.CD set ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36382/1/src/cpu/x86/mtrr/Makefile.i... File src/cpu/x86/mtrr/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36382/1/src/cpu/x86/mtrr/Makefile.i... PS1, Line 7: verstage-y += set_mtrr.S
Does this file always work for 64-bit?
I think it should work and compile fine with 64-bit.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36382 )
Change subject: cpu/x86/mtrr: Add function to set MTRR with CR0.CD set ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36382/1/src/cpu/x86/mtrr/Makefile.i... File src/cpu/x86/mtrr/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36382/1/src/cpu/x86/mtrr/Makefile.i... PS1, Line 7: verstage-y += set_mtrr.S
Does this file always work for 64-bit? […]
but I think from your other patch that the ABI this function is implementing will not work. Are you assuming the caller in 64-bit mode will work w/ this assembly?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36382 )
Change subject: cpu/x86/mtrr: Add function to set MTRR with CR0.CD set ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36382/1/src/cpu/x86/mtrr/Makefile.i... File src/cpu/x86/mtrr/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36382/1/src/cpu/x86/mtrr/Makefile.i... PS1, Line 7: verstage-y += set_mtrr.S
but I think from your other patch that the ABI this function is implementing will not work. Are you assuming the caller in 64-bit mode will work w/ this assembly?
Oh right... asmlinkage should do the trick?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36382 )
Change subject: cpu/x86/mtrr: Add function to set MTRR with CR0.CD set ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36382/1/src/cpu/x86/mtrr/Makefile.i... File src/cpu/x86/mtrr/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36382/1/src/cpu/x86/mtrr/Makefile.i... PS1, Line 7: verstage-y += set_mtrr.S
but I think from your other patch that the ABI this function is implementing will not work. Are you assuming the caller in 64-bit mode will work w/ this assembly?
Oh right... asmlinkage should do the trick?
Even with asmlinkage the sysv 64bit ABI is used with arguments on %rdi,%rsi and %rdx. Do we really want to insist on doing this with CR0.CD set? It works fine without on all CPUs this code currently targets (Intel core and core2). It might be useful for amdfam10 where L2 cache used for WP-IO does L2 cache eviction but from amdfam15 onward L2 WP-IO does not cause L2 eviction (anymore) according to BKDG. We are about to drop amdfam10 support, so..
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36382 )
Change subject: cpu/x86/mtrr: Add function to set MTRR with CR0.CD set ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36382/1/src/cpu/x86/mtrr/Makefile.i... File src/cpu/x86/mtrr/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36382/1/src/cpu/x86/mtrr/Makefile.i... PS1, Line 7: verstage-y += set_mtrr.S
but I think from your other patch that the ABI this function is implementing will not work. […]
I never insisted. From my point of view, you're the experts :)
Won't fam10 be dropped after the imminent release?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36382 )
Change subject: cpu/x86/mtrr: Add function to set MTRR with CR0.CD set ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36382/1/src/cpu/x86/mtrr/Makefile.i... File src/cpu/x86/mtrr/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36382/1/src/cpu/x86/mtrr/Makefile.i... PS1, Line 7: verstage-y += set_mtrr.S
I never insisted. From my point of view, you're the experts :)
Won't fam10 be dropped after the imminent release?
I suspect so.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36382 )
Change subject: cpu/x86/mtrr: Add function to set MTRR with CR0.CD set ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36382/1/src/cpu/x86/mtrr/Makefile.i... File src/cpu/x86/mtrr/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36382/1/src/cpu/x86/mtrr/Makefile.i... PS1, Line 7: verstage-y += set_mtrr.S
I never insisted. From my point of view, you're the experts :) […]
I don't think setting CR0.CD is necessary, and it actually can complicate matters. If people want this we'll just need to add more guarding w.r.t. x86-64 support vs x86-32 support. We'd likely need multiple instances.
Arthur Heymans has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/36382 )
Change subject: cpu/x86/mtrr: Add function to set MTRR with CR0.CD set ......................................................................
Abandoned
-
Aaron Durbin (Code Review) -
Arthur Heymans (Code Review)
-
Nico Huber (Code Review)