Nico Huber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31319
Change subject: cpu/x86/mtrr: Fix shift into sign-bit in C ......................................................................
cpu/x86/mtrr: Fix shift into sign-bit in C
In C, the calculations are done using an `int` type. When shifting left in _FROM_4G_TOP(), we shift the result into its sign bit which is undefined behaviour in C. We cannot cast to an unsigned here and neither use an integer suffix if we want to stay most portable and compatible to assemblers. So we have to treat C and assembly inde- pendently.
Alternatively, we could let the preprocessor do the job. But only with a lot of `if` and `elif` as we can't let it evaluate arith- metic expressions directly.
Change-Id: I67ce5873e1ba35468022ebab1a3a67669d282e1e Signed-off-by: Nico Huber nico.h@gmx.de --- M src/arch/x86/postcar_loader.c M src/cpu/intel/car/non-evict/cache_as_ram.S M src/cpu/intel/car/p4-netburst/cache_as_ram.S M src/cpu/intel/fsp_model_406dx/bootblock.c M src/cpu/intel/haswell/bootblock.c M src/cpu/intel/haswell/romstage.c M src/cpu/intel/model_2065x/bootblock.c M src/cpu/intel/model_206ax/bootblock.c M src/drivers/intel/fsp1_0/cache_as_ram.inc M src/include/cpu/x86/mtrr.h M src/northbridge/intel/i440bx/ram_calc.c M src/soc/intel/baytrail/romstage/romstage.c M src/soc/intel/broadwell/bootblock/cpu.c M src/soc/intel/broadwell/romstage/romstage.c M src/soc/intel/fsp_baytrail/bootblock/bootblock.c 15 files changed, 20 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/31319/1
diff --git a/src/arch/x86/postcar_loader.c b/src/arch/x86/postcar_loader.c index 141e8d2..e53b15b 100644 --- a/src/arch/x86/postcar_loader.c +++ b/src/arch/x86/postcar_loader.c @@ -107,7 +107,7 @@ { if (!IS_ENABLED(CONFIG_BOOT_DEVICE_MEMORY_MAPPED)) return; - postcar_frame_add_mtrr(pcf, CACHE_ROM_BASE, CACHE_ROM_SIZE, type); + postcar_frame_add_mtrr(pcf, CACHE_ROM_BASE_C, CACHE_ROM_SIZE, type); }
void *postcar_commit_mtrrs(struct postcar_frame *pcf) diff --git a/src/cpu/intel/car/non-evict/cache_as_ram.S b/src/cpu/intel/car/non-evict/cache_as_ram.S index 7788a2d..6378f68 100644 --- a/src/cpu/intel/car/non-evict/cache_as_ram.S +++ b/src/cpu/intel/car/non-evict/cache_as_ram.S @@ -118,7 +118,7 @@ /* Enable cache for our code in Flash because we do XIP here */ movl $MTRR_PHYS_BASE(1), %ecx xorl %edx, %edx - movl $(CACHE_ROM_BASE | MTRR_TYPE_WRPROT), %eax + movl $(CACHE_ROM_BASE_ASM | MTRR_TYPE_WRPROT), %eax wrmsr
movl $MTRR_PHYS_MASK(1), %ecx diff --git a/src/cpu/intel/car/p4-netburst/cache_as_ram.S b/src/cpu/intel/car/p4-netburst/cache_as_ram.S index 9d50582..7324619 100644 --- a/src/cpu/intel/car/p4-netburst/cache_as_ram.S +++ b/src/cpu/intel/car/p4-netburst/cache_as_ram.S @@ -304,7 +304,7 @@ /* Cache the whole rom to fetch microcode updates */ movl $MTRR_PHYS_BASE(1), %ecx xorl %edx, %edx - movl $(CACHE_ROM_BASE | MTRR_TYPE_WRPROT), %eax + movl $(CACHE_ROM_BASE_ASM | MTRR_TYPE_WRPROT), %eax wrmsr
movl $MTRR_PHYS_MASK(1), %ecx diff --git a/src/cpu/intel/fsp_model_406dx/bootblock.c b/src/cpu/intel/fsp_model_406dx/bootblock.c index 13c5d0f..0a388ed 100644 --- a/src/cpu/intel/fsp_model_406dx/bootblock.c +++ b/src/cpu/intel/fsp_model_406dx/bootblock.c @@ -61,7 +61,7 @@ msr_t msr;
disable_cache(); - set_var_mtrr(1, CACHE_ROM_BASE, CACHE_ROM_SIZE, MTRR_TYPE_WRPROT); + set_var_mtrr(1, CACHE_ROM_BASE_C, CACHE_ROM_SIZE, MTRR_TYPE_WRPROT); enable_cache();
/* Enable Variable MTRRs */ diff --git a/src/cpu/intel/haswell/bootblock.c b/src/cpu/intel/haswell/bootblock.c index 3a306b5..48edc21 100644 --- a/src/cpu/intel/haswell/bootblock.c +++ b/src/cpu/intel/haswell/bootblock.c @@ -50,7 +50,7 @@ msr_t msr;
disable_cache(); - set_var_mtrr(1, CACHE_ROM_BASE, CACHE_ROM_SIZE, MTRR_TYPE_WRPROT); + set_var_mtrr(1, CACHE_ROM_BASE_C, CACHE_ROM_SIZE, MTRR_TYPE_WRPROT); enable_cache();
/* Enable Variable MTRRs */ diff --git a/src/cpu/intel/haswell/romstage.c b/src/cpu/intel/haswell/romstage.c index ff729d6..9b9f156 100644 --- a/src/cpu/intel/haswell/romstage.c +++ b/src/cpu/intel/haswell/romstage.c @@ -55,7 +55,7 @@ if (postcar_frame_init(&pcf, ROMSTAGE_RAM_STACK_SIZE)) die("Unable to initialize postcar frame.\n"); /* Cache the ROM as WP just below 4GiB. */ - postcar_frame_add_mtrr(&pcf, CACHE_ROM_BASE, CACHE_ROM_SIZE, + postcar_frame_add_mtrr(&pcf, CACHE_ROM_BASE_C, CACHE_ROM_SIZE, MTRR_TYPE_WRPROT);
/* Cache RAM as WB from 0 -> CACHE_TMP_RAMTOP. */ diff --git a/src/cpu/intel/model_2065x/bootblock.c b/src/cpu/intel/model_2065x/bootblock.c index ed528d1..b017c50 100644 --- a/src/cpu/intel/model_2065x/bootblock.c +++ b/src/cpu/intel/model_2065x/bootblock.c @@ -50,7 +50,7 @@ msr_t msr;
disable_cache(); - set_var_mtrr(1, CACHE_ROM_BASE, CACHE_ROM_SIZE, MTRR_TYPE_WRPROT); + set_var_mtrr(1, CACHE_ROM_BASE_C, CACHE_ROM_SIZE, MTRR_TYPE_WRPROT); enable_cache();
/* Enable Variable MTRRs */ diff --git a/src/cpu/intel/model_206ax/bootblock.c b/src/cpu/intel/model_206ax/bootblock.c index 670b097..9cf51be 100644 --- a/src/cpu/intel/model_206ax/bootblock.c +++ b/src/cpu/intel/model_206ax/bootblock.c @@ -52,7 +52,7 @@ msr_t msr;
disable_cache(); - set_var_mtrr(1, CACHE_ROM_BASE, CACHE_ROM_SIZE, MTRR_TYPE_WRPROT); + set_var_mtrr(1, CACHE_ROM_BASE_C, CACHE_ROM_SIZE, MTRR_TYPE_WRPROT); enable_cache();
/* Enable Variable MTRRs */ diff --git a/src/drivers/intel/fsp1_0/cache_as_ram.inc b/src/drivers/intel/fsp1_0/cache_as_ram.inc index 8ac3595..3477374 100644 --- a/src/drivers/intel/fsp1_0/cache_as_ram.inc +++ b/src/drivers/intel/fsp1_0/cache_as_ram.inc @@ -112,8 +112,8 @@ CAR_init_params: .long dummy_microcode .long 0 - .long CACHE_ROM_BASE /* Firmware Location */ - .long CACHE_ROM_SIZE /* Total Firmware Length */ + .long CACHE_ROM_BASE_ASM /* Firmware Location */ + .long CACHE_ROM_SIZE /* Total Firmware Length */
CAR_init_stack: .long CAR_init_done diff --git a/src/include/cpu/x86/mtrr.h b/src/include/cpu/x86/mtrr.h index eb7d78d..f1e4ecf 100644 --- a/src/include/cpu/x86/mtrr.h +++ b/src/include/cpu/x86/mtrr.h @@ -150,7 +150,7 @@ #define _ALIGN_DOWN_POW2(x) ((x) & ~_POW2_MASK(x))
/* Calculate `4GiB - x` (e.g. absolute address for offset from 4GiB) */ -#define _FROM_4G_TOP(x) (((1 << 20) - ((x) >> 12)) << 12) +#define _FROM_4G_TOP(one, x) (((one << 20) - ((x) >> 12)) << 12)
/* At the end of romstage, low RAM 0..CACHE_TM_RAMTOP may be set * as write-back cacheable to speed up ramstage decompression. @@ -164,7 +164,7 @@
/* For ROM caching, generally, try to use the next power of 2. */ #define OPTIMAL_CACHE_ROM_SIZE _ALIGN_UP_POW2(CONFIG_ROM_SIZE) -#define OPTIMAL_CACHE_ROM_BASE _FROM_4G_TOP(OPTIMAL_CACHE_ROM_SIZE) +#define OPTIMAL_CACHE_ROM_BASE _FROM_4G_TOP(1, OPTIMAL_CACHE_ROM_SIZE) #if (OPTIMAL_CACHE_ROM_SIZE < CONFIG_ROM_SIZE) || \ (OPTIMAL_CACHE_ROM_SIZE >= (2 * CONFIG_ROM_SIZE)) # error "Optimal CACHE_ROM_SIZE can't be derived, _POW2_MASK needs refinement." @@ -176,7 +176,7 @@ to get a power of 2 again, for a single MTRR. */ #define CAR_END (CONFIG_DCACHE_RAM_BASE + CONFIG_DCACHE_RAM_SIZE) #if CAR_END > OPTIMAL_CACHE_ROM_BASE -# define CAR_CACHE_ROM_SIZE _ALIGN_DOWN_POW2(_FROM_4G_TOP(CAR_END)) +# define CAR_CACHE_ROM_SIZE _ALIGN_DOWN_POW2(_FROM_4G_TOP(1, CAR_END)) #else # define CAR_CACHE_ROM_SIZE OPTIMAL_CACHE_ROM_SIZE #endif @@ -192,6 +192,7 @@ # define CACHE_ROM_SIZE CAR_CACHE_ROM_SIZE #endif
-#define CACHE_ROM_BASE _FROM_4G_TOP(CACHE_ROM_SIZE) +#define CACHE_ROM_BASE_ASM _FROM_4G_TOP(1, CACHE_ROM_SIZE) +#define CACHE_ROM_BASE_C _FROM_4G_TOP(1u, CACHE_ROM_SIZE)
#endif /* CPU_X86_MTRR_H */ diff --git a/src/northbridge/intel/i440bx/ram_calc.c b/src/northbridge/intel/i440bx/ram_calc.c index 3362d93..50e0a0d 100644 --- a/src/northbridge/intel/i440bx/ram_calc.c +++ b/src/northbridge/intel/i440bx/ram_calc.c @@ -81,7 +81,7 @@ die("Unable to initialize postcar frame.\n");
/* Cache the ROM as WP just below 4GiB. */ - postcar_frame_add_mtrr(&pcf, CACHE_ROM_BASE, CACHE_ROM_SIZE, + postcar_frame_add_mtrr(&pcf, CACHE_ROM_BASE_C, CACHE_ROM_SIZE, MTRR_TYPE_WRPROT);
/* Cache RAM as WB from 0 -> CACHE_TMP_RAMTOP. */ diff --git a/src/soc/intel/baytrail/romstage/romstage.c b/src/soc/intel/baytrail/romstage/romstage.c index 4b49906..a2bf938 100644 --- a/src/soc/intel/baytrail/romstage/romstage.c +++ b/src/soc/intel/baytrail/romstage/romstage.c @@ -249,7 +249,7 @@ if (postcar_frame_init(&pcf, ROMSTAGE_RAM_STACK_SIZE)) die("Unable to initialize postcar frame.\n"); /* Cache the ROM as WP just below 4GiB. */ - postcar_frame_add_mtrr(&pcf, CACHE_ROM_BASE, CACHE_ROM_SIZE, + postcar_frame_add_mtrr(&pcf, CACHE_ROM_BASE_C, CACHE_ROM_SIZE, MTRR_TYPE_WRPROT);
/* Cache RAM as WB from 0 -> CACHE_TMP_RAMTOP. */ diff --git a/src/soc/intel/broadwell/bootblock/cpu.c b/src/soc/intel/broadwell/bootblock/cpu.c index 7508bc2..2d56f32 100644 --- a/src/soc/intel/broadwell/bootblock/cpu.c +++ b/src/soc/intel/broadwell/bootblock/cpu.c @@ -42,7 +42,7 @@
disable_cache(); /* Why only top 4MiB ? */ - set_var_mtrr(1, CACHE_ROM_BASE, CACHE_ROM_SIZE, MTRR_TYPE_WRPROT); + set_var_mtrr(1, CACHE_ROM_BASE_C, CACHE_ROM_SIZE, MTRR_TYPE_WRPROT); enable_cache();
/* Enable Variable MTRRs */ diff --git a/src/soc/intel/broadwell/romstage/romstage.c b/src/soc/intel/broadwell/romstage/romstage.c index b89d948..add0b47 100644 --- a/src/soc/intel/broadwell/romstage/romstage.c +++ b/src/soc/intel/broadwell/romstage/romstage.c @@ -47,7 +47,7 @@ if (postcar_frame_init(&pcf, ROMSTAGE_RAM_STACK_SIZE)) die("Unable to initialize postcar frame.\n"); /* Cache the ROM as WP just below 4GiB. */ - postcar_frame_add_mtrr(&pcf, CACHE_ROM_BASE, CACHE_ROM_SIZE, + postcar_frame_add_mtrr(&pcf, CACHE_ROM_BASE_C, CACHE_ROM_SIZE, MTRR_TYPE_WRPROT);
/* Cache RAM as WB from 0 -> CACHE_TMP_RAMTOP. */ diff --git a/src/soc/intel/fsp_baytrail/bootblock/bootblock.c b/src/soc/intel/fsp_baytrail/bootblock/bootblock.c index 8ce0a1d..2b44804 100644 --- a/src/soc/intel/fsp_baytrail/bootblock/bootblock.c +++ b/src/soc/intel/fsp_baytrail/bootblock/bootblock.c @@ -70,7 +70,7 @@ msr_t msr;
disable_cache(); - set_var_mtrr(1, CACHE_ROM_BASE, CACHE_ROM_SIZE, MTRR_TYPE_WRPROT); + set_var_mtrr(1, CACHE_ROM_BASE_C, CACHE_ROM_SIZE, MTRR_TYPE_WRPROT); enable_cache();
/* Enable Variable MTRRs */
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31319 )
Change subject: cpu/x86/mtrr: Fix shift into sign-bit in C ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31319/1/src/include/cpu/x86/mtrr.h File src/include/cpu/x86/mtrr.h:
https://review.coreboot.org/#/c/31319/1/src/include/cpu/x86/mtrr.h@196 PS1, Line 196: #define CACHE_ROM_BASE_C _FROM_4G_TOP(1u, CACHE_ROM_SIZE) Can you use #if defined(__ASSEMBLER__) here to get different define for same name CACHE_ROM_BASE ?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31319 )
Change subject: cpu/x86/mtrr: Fix shift into sign-bit in C ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31319/1/src/include/cpu/x86/mtrr.h File src/include/cpu/x86/mtrr.h:
https://review.coreboot.org/#/c/31319/1/src/include/cpu/x86/mtrr.h@196 PS1, Line 196: #define CACHE_ROM_BASE_C _FROM_4G_TOP(1u, CACHE_ROM_SIZE)
Can you use #if defined(__ASSEMBLER__) here to get different define for same name CACHE_ROM_BASE ?
Yeah, should have done. I was a little absent-minded it seems...
Nico Huber has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/31319 )
Change subject: cpu/x86/mtrr: Fix shift into sign-bit in C ......................................................................
Abandoned
there are better ways...
-
Kyösti Mälkki (Code Review) -
Nico Huber (Code Review)