<p>Subrata Banik has uploaded this change for <strong>review</strong>.</p><p><a href="https://review.coreboot.org/26288">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">arch/x86: Align cbmem with CONFIG_STACK_SIZE<br><br>As per c_start.S implementation logic, struct cpu_info pointer<br>will be present at 8 bytes below stack_top(_estack).<br><br>Getting stack_base and stack_end using checkstack(_estack, 0)<br>function and comparing struct cpu_info *ci pointer address with<br>different STACK_SIZE.<br><br>STACK_SIZE=8K (0x2000)<br><br>stack_base = 0x85ccc000<br>stack_top = 0x85cce000<br><br>(struct cpu_info) ci = 0x85ccdff8<br><br>STACK_SIZE=16K (0x4000)<br><br>stack_base = 0x85cca000<br>stack_top = 0x85cce000<br><br>(struct cpu_info) ci = 0x85ccfff8<br><br>Due to wrong alignment issue, cpu_index variable is getting random/wrong<br>value.<br><br>BUG=b:79562868<br>TEST=Verify cpu_index is returning correct cpu_index of BSP and AP<br>with different STACK_SIZE 4K/8K/16K/32K on KBL and CNL platform.<br><br>Change-Id: Ifdb44a8a9902c9b3b393d01c1e288ac34f5e5e8e<br>Signed-off-by: Subrata Banik <subrata.banik@intel.com><br>---<br>M src/Kconfig<br>M src/arch/x86/c_start.S<br>M src/arch/x86/include/arch/cpu.h<br>M src/cpu/x86/mp_init.c<br>M src/include/cbmem.h<br>M src/lib/ext_stage_cache.c<br>M src/lib/imd.c<br>M src/lib/memrange.c<br>M src/lib/rmodule.c<br>9 files changed, 27 insertions(+), 17 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/26288/1</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/src/Kconfig b/src/Kconfig</span><br><span>index 99a704d..4bf76b9 100644</span><br><span>--- a/src/Kconfig</span><br><span>+++ b/src/Kconfig</span><br><span>@@ -395,6 +395,11 @@</span><br><span> default 0x1000 if ARCH_X86</span><br><span> default 0x0</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+config STACK_ALIGNMENT_SIZE</span><br><span style="color: hsl(120, 100%, 40%);">+ hex</span><br><span style="color: hsl(120, 100%, 40%);">+ default STACK_SIZE if ARCH_X86</span><br><span style="color: hsl(120, 100%, 40%);">+ default 0x1000</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> config MAX_CPUS</span><br><span> int</span><br><span> default 1</span><br><span>diff --git a/src/arch/x86/c_start.S b/src/arch/x86/c_start.S</span><br><span>index 6426ef3..b9573ba 100644</span><br><span>--- a/src/arch/x86/c_start.S</span><br><span>+++ b/src/arch/x86/c_start.S</span><br><span>@@ -19,7 +19,7 @@</span><br><span> .global _stack</span><br><span> .global _estack</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-.align CONFIG_STACK_SIZE</span><br><span style="color: hsl(120, 100%, 40%);">+.align CONFIG_STACK_ALIGNMENT_SIZE</span><br><span> _stack:</span><br><span> .space CONFIG_MAX_CPUS*CONFIG_STACK_SIZE</span><br><span> _estack:</span><br><span>diff --git a/src/arch/x86/include/arch/cpu.h b/src/arch/x86/include/arch/cpu.h</span><br><span>index 5d44aae..7d0f182 100644</span><br><span>--- a/src/arch/x86/include/arch/cpu.h</span><br><span>+++ b/src/arch/x86/include/arch/cpu.h</span><br><span>@@ -206,8 +206,8 @@</span><br><span> "orl %2, %0 "</span><br><span> #endif</span><br><span> : "=r" (ci)</span><br><span style="color: hsl(0, 100%, 40%);">- : "0" (~(CONFIG_STACK_SIZE - 1)),</span><br><span style="color: hsl(0, 100%, 40%);">- "r" (CONFIG_STACK_SIZE - sizeof(struct cpu_info))</span><br><span style="color: hsl(120, 100%, 40%);">+ : "0" (~(CONFIG_STACK_ALIGNMENT_SIZE - 1)),</span><br><span style="color: hsl(120, 100%, 40%);">+ "r" (CONFIG_STACK_ALIGNMENT_SIZE - sizeof(struct cpu_info))</span><br><span> );</span><br><span> return ci;</span><br><span> }</span><br><span>diff --git a/src/cpu/x86/mp_init.c b/src/cpu/x86/mp_init.c</span><br><span>index ef576ec..8689904 100644</span><br><span>--- a/src/cpu/x86/mp_init.c</span><br><span>+++ b/src/cpu/x86/mp_init.c</span><br><span>@@ -317,8 +317,9 @@</span><br><span> return ap_count;</span><br><span> }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- if (rmodule_load_alignment(&sipi_mod) != 4096) {</span><br><span style="color: hsl(0, 100%, 40%);">- printk(BIOS_CRIT, "SIPI module load alignment(%d) != 4096.\n",</span><br><span style="color: hsl(120, 100%, 40%);">+ if (rmodule_load_alignment(&sipi_mod) != CONFIG_STACK_ALIGNMENT_SIZE) {</span><br><span style="color: hsl(120, 100%, 40%);">+ printk(BIOS_CRIT, "SIPI module load alignment(%d) != "</span><br><span style="color: hsl(120, 100%, 40%);">+ "CONFIG_STACK_ALIGNMENT_SIZE.\n",</span><br><span> rmodule_load_alignment(&sipi_mod));</span><br><span> return ap_count;</span><br><span> }</span><br><span>diff --git a/src/include/cbmem.h b/src/include/cbmem.h</span><br><span>index 007bc54..c1eae80 100644</span><br><span>--- a/src/include/cbmem.h</span><br><span>+++ b/src/include/cbmem.h</span><br><span>@@ -38,7 +38,7 @@</span><br><span> * may be removed, but the last one added is the only that can be removed.</span><br><span> */</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-#define DYN_CBMEM_ALIGN_SIZE (4096)</span><br><span style="color: hsl(120, 100%, 40%);">+#define DYN_CBMEM_ALIGN_SIZE CONFIG_STACK_ALIGNMENT_SIZE</span><br><span> #define CBMEM_ROOT_SIZE DYN_CBMEM_ALIGN_SIZE</span><br><span> </span><br><span> /* The root region is at least DYN_CBMEM_ALIGN_SIZE . */</span><br><span>diff --git a/src/lib/ext_stage_cache.c b/src/lib/ext_stage_cache.c</span><br><span>index c3d4aee..5614b0f 100644</span><br><span>--- a/src/lib/ext_stage_cache.c</span><br><span>+++ b/src/lib/ext_stage_cache.c</span><br><span>@@ -40,7 +40,8 @@</span><br><span> imd_handle_init(imd, (void *)(size + (uintptr_t)base));</span><br><span> </span><br><span> printk(BIOS_DEBUG, "External stage cache:\n");</span><br><span style="color: hsl(0, 100%, 40%);">- imd_create_tiered_empty(imd, 4096, 4096, 1024, 32);</span><br><span style="color: hsl(120, 100%, 40%);">+ imd_create_tiered_empty(imd, CONFIG_STACK_ALIGNMENT_SIZE,</span><br><span style="color: hsl(120, 100%, 40%);">+ CONFIG_STACK_ALIGNMENT_SIZE, 1024, 32);</span><br><span> if (imd_limit_size(imd, size))</span><br><span> printk(BIOS_DEBUG, "Could not limit stage cache size.\n");</span><br><span> }</span><br><span>diff --git a/src/lib/imd.c b/src/lib/imd.c</span><br><span>index d17cc81..2be5573 100644</span><br><span>--- a/src/lib/imd.c</span><br><span>+++ b/src/lib/imd.c</span><br><span>@@ -26,7 +26,7 @@</span><br><span> static const uint32_t IMD_ROOT_PTR_MAGIC = 0xc0389481;</span><br><span> static const uint32_t IMD_ENTRY_MAGIC = ~0xc0389481;</span><br><span> static const uint32_t SMALL_REGION_ID = CBMEM_ID_IMD_SMALL;</span><br><span style="color: hsl(0, 100%, 40%);">-static const size_t LIMIT_ALIGN = 4096;</span><br><span style="color: hsl(120, 100%, 40%);">+static const size_t LIMIT_ALIGN = CONFIG_STACK_ALIGNMENT_SIZE;</span><br><span> </span><br><span> /* In-memory data structures. */</span><br><span> struct imd_root_pointer {</span><br><span>diff --git a/src/lib/memrange.c b/src/lib/memrange.c</span><br><span>index 96d7524..2adc478 100644</span><br><span>--- a/src/lib/memrange.c</span><br><span>+++ b/src/lib/memrange.c</span><br><span>@@ -228,12 +228,14 @@</span><br><span> if (size == 0)</span><br><span> return;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- /* The addresses are aligned to 4096 bytes: the begin address is</span><br><span style="color: hsl(0, 100%, 40%);">- * aligned down while the end address is aligned up to be conservative</span><br><span style="color: hsl(0, 100%, 40%);">- * about the full range covered. */</span><br><span style="color: hsl(0, 100%, 40%);">- begin = ALIGN_DOWN(base, 4096);</span><br><span style="color: hsl(120, 100%, 40%);">+ /*</span><br><span style="color: hsl(120, 100%, 40%);">+ * The addresses are aligned to CONFIG_STACK_ALIGNMENT_SIZE bytes:</span><br><span style="color: hsl(120, 100%, 40%);">+ * the begin address is aligned down while the end address is aligned</span><br><span style="color: hsl(120, 100%, 40%);">+ * up to be conservative about the full range covered.</span><br><span style="color: hsl(120, 100%, 40%);">+ */</span><br><span style="color: hsl(120, 100%, 40%);">+ begin = ALIGN_DOWN(base, CONFIG_STACK_ALIGNMENT_SIZE);</span><br><span> end = begin + size + (base - begin);</span><br><span style="color: hsl(0, 100%, 40%);">- end = ALIGN_UP(end, 4096) - 1;</span><br><span style="color: hsl(120, 100%, 40%);">+ end = ALIGN_UP(end, CONFIG_STACK_ALIGNMENT_SIZE) - 1;</span><br><span> action(ranges, begin, end, tag);</span><br><span> }</span><br><span> </span><br><span>diff --git a/src/lib/rmodule.c b/src/lib/rmodule.c</span><br><span>index 66d5120..68939a6 100644</span><br><span>--- a/src/lib/rmodule.c</span><br><span>+++ b/src/lib/rmodule.c</span><br><span>@@ -177,7 +177,7 @@</span><br><span> * The base address where the program is loaded needs to be a multiple</span><br><span> * of the program's starting link address. That way all data alignment</span><br><span> * in the program is preserved. Default to 4KiB. */</span><br><span style="color: hsl(0, 100%, 40%);">- return 4096;</span><br><span style="color: hsl(120, 100%, 40%);">+ return CONFIG_STACK_ALIGNMENT_SIZE;</span><br><span> }</span><br><span> </span><br><span> int rmodule_load(void *base, struct rmodule *module)</span><br><span>@@ -210,8 +210,8 @@</span><br><span> if (region_alignment & (region_alignment - 1))</span><br><span> BUG();</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- if (region_alignment < 4096)</span><br><span style="color: hsl(0, 100%, 40%);">- region_alignment = 4096;</span><br><span style="color: hsl(120, 100%, 40%);">+ if (region_alignment < CONFIG_STACK_ALIGNMENT_SIZE)</span><br><span style="color: hsl(120, 100%, 40%);">+ region_alignment = CONFIG_STACK_ALIGNMENT_SIZE;</span><br><span> </span><br><span> /* Sanity check rmodule_header size. The code below assumes it is less</span><br><span> * than the minimum alignment required. */</span><br><span>@@ -228,7 +228,8 @@</span><br><span> * to place the rmodule so that the program falls on the aligned</span><br><span> * address with the header just before it. Therefore, we need at least</span><br><span> * a page to account for the size of the header. */</span><br><span style="color: hsl(0, 100%, 40%);">- *region_size = ALIGN(rmodule_size + region_alignment, 4096);</span><br><span style="color: hsl(120, 100%, 40%);">+ *region_size = ALIGN(rmodule_size + region_alignment,</span><br><span style="color: hsl(120, 100%, 40%);">+ CONFIG_STACK_ALIGNMENT_SIZE);</span><br><span> /* The program starts immediately after the header. However,</span><br><span> * it needs to be aligned to a 4KiB boundary. Therefore, adjust the</span><br><span> * program location so that the program lands on a page boundary. The</span><br><span></span><br></pre><p>To view, visit <a href="https://review.coreboot.org/26288">change 26288</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://review.coreboot.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://review.coreboot.org/26288"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: coreboot </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: newchange </div>
<div style="display:none"> Gerrit-Change-Id: Ifdb44a8a9902c9b3b393d01c1e288ac34f5e5e8e </div>
<div style="display:none"> Gerrit-Change-Number: 26288 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> </div>