Rocky Phagura has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: SMM module loader version 2 ......................................................................
cpu/x86/smm: SMM module loader version 2
Xeon-SP a Skylake Scalable Processor can have 36 CPU threads(18 cores). Current coreboot SMM is unable to handle more than ~32 CPU threads. This patch introduces a version 2 of the SMM module loader which addresses this problem. Having two versions of the SMM module loader prevents any issues to current projects. Future Xeon-SP products will be using this version of the SMM loader. Subsequent patches will enable board specifc functionality for Xeon-SP.
The reason for moving to version 2 is the state save area begins to encroach upon the smi handling code when more than 32 CPU threads are in the system. This can cause system hangs, reboots, etc. The second change is related to staggered entry points with simple near jumps. In the current loader, near jumps will not work because the CPU is jumping within the same code segment. In version 2, "far" address jumps are necessary therefore protected mode must is enabled first. The SMM layout and how the CPUs are staggered are documented in the code.
By making the modifications above, this allows the smm module loader to expand easily as more CPU threads are added.
TEST=build for Tiogapass platform under OCP mainboard. Enable the following in Kconfig. select CPU_INTEL_COMMON_SMM select SOC_INTEL_COMMON_BLOCK_SMM select SMM_TSEG select HAVE_SMI_HANDLER select ACPI_INTEL_HARDWARE_SLEEP_VALUES
Debug console will show all 36 cores relocated. Further tested by generating SMI's to port 0xb2 using XDP/ITP HW debugger and ensured all cores entering and exiting SMM properly. In addition, booted to Linux 5.4 kernel and observed no issues during mp init.
Change-Id: I00a23a5f2a46110536c344254868390dbb71854c Signed-off-by: Rocky Phagura rphagura@fb.com --- M src/cpu/x86/Kconfig M src/cpu/x86/smm/Makefile.inc A src/cpu/x86/smm/smm_module_loaderv2.c M src/include/cpu/x86/smm.h 4 files changed, 678 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/43684/1
diff --git a/src/cpu/x86/Kconfig b/src/cpu/x86/Kconfig index 5394cd0..b3a16bc 100644 --- a/src/cpu/x86/Kconfig +++ b/src/cpu/x86/Kconfig @@ -121,6 +121,14 @@
endif
+config X86_SMM_LOADER_VERSION2 + bool + default n + depends on HAVE_SMI_HANDLER + help + This option enables SMM module loader that works with server + platforms which may contain more than 32 CPU threads. + config SMM_LAPIC_REMAP_MITIGATION bool default y if NORTHBRIDGE_INTEL_I945 diff --git a/src/cpu/x86/smm/Makefile.inc b/src/cpu/x86/smm/Makefile.inc index dbe567a..1273a6c 100644 --- a/src/cpu/x86/smm/Makefile.inc +++ b/src/cpu/x86/smm/Makefile.inc @@ -1,6 +1,10 @@ ## SPDX-License-Identifier: GPL-2.0-only
+ifeq ($(CONFIG_X86_SMM_LOADER_VERSION2),y) +ramstage-y += smm_module_loaderv2.c +else ramstage-y += smm_module_loader.c +endif ramstage-y += smi_trigger.c
ifeq ($(CONFIG_ARCH_RAMSTAGE_X86_32),y) diff --git a/src/cpu/x86/smm/smm_module_loaderv2.c b/src/cpu/x86/smm/smm_module_loaderv2.c new file mode 100644 index 0000000..d1ccec0 --- /dev/null +++ b/src/cpu/x86/smm/smm_module_loaderv2.c @@ -0,0 +1,648 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <string.h> +#include <rmodule.h> +#include <cpu/x86/smm.h> +#include <commonlib/helpers.h> +#include <console/console.h> +#include <security/intel/stm/SmmStm.h> +test +#define FXSAVE_SIZE 512 +#define SMM_CODE_SEGMENT_SIZE 0x10000 +/* FXSAVE area during relocation. While it may not be strictly needed the + SMM stub code relies on the FXSAVE area being non-zero to enable SSE + instructions within SMM mode. */ +static uint8_t fxsave_area_relocation[CONFIG_MAX_CPUS][FXSAVE_SIZE] +__attribute__((aligned(16))); + +/* + * Components that make up the SMRAM: + * 1. Save state - the total save state memory used + * 2. Stack - stacks for the CPUs in the SMM handler + * 3. Stub - SMM stub code for calling into handler + * 4. Handler - C-based SMM handler. + * + * The components are assumed to consist of one consecutive region. + */ + +/* These parameters are used by the SMM stub code. A pointer to the params + * is also passed to the C-base handler. */ +struct smm_stub_params { + u32 stack_size; + u32 stack_top; + u32 c_handler; + u32 c_handler_arg; + u32 fxsave_area; + u32 fxsave_area_size; + struct smm_runtime runtime; +} __packed; + +/* + * The stub is the entry point that sets up protected mode and stacks for each + * CPU. It then calls into the SMM handler module. It is encoded as an rmodule. + */ +extern unsigned char _binary_smmstub_start[]; + +/* Per CPU minimum stack size. */ +#define SMM_MINIMUM_STACK_SIZE 32 + +struct cpu_smm_info { + uint8_t active; + unsigned int smbase; + unsigned int entry; + unsigned int ss_start; + unsigned int code_start; + unsigned int code_end; +}; +struct cpu_smm_info cpus[CONFIG_MAX_CPUS] = { 0 }; + +/* + * This method creates a map of all the CPU entry points, save state locations + * and the beginning and end of code segments for each CPU. This map is used + * during relocation to properly align as many CPUs that can fit into the SMRAM + * region. For more information on how SMRAM works, refer to the latest Intel + * developer's manuals (volume 3, chapter 34). SMRAM is divided up into the + * following regions: + * +-----------------+ Top of SMRAM + * | | <- MSEG, FXSAVE + * +-----------------+ + * | common | + * | smi handler | 64K + * | | + * +-----------------+ + * | CPU 0 code seg | + * +-----------------+ + * | CPU 1 code seg | + * +-----------------+ + * | CPU x code seg | + * +-----------------+ + * | | + * | | + * +-----------------+ + * | stacks | + * +-----------------+ <- START of SMRAM + * + * The code below checks when a code segment is full and begins placing the remainder + * CPUs in the lower segments. The entry point for each CPU is smbase + 8000 + * and save state is smbase + 0x8000 + (0x8000 - state save size). Save state + * area grows downward into the CPUs entry point. Therefore staggering too many + * CPUs in one 32K block will corrupt CPU0's entry code as the save states move + * downward. + * input : smbase of first CPU (all other CPUs + * will go below this address) + * input : num_cpus in the system. The map will + * be created from 0 to num_cpus. + */ +static int smm_create_map(unsigned int smbase, unsigned int num_cpus, + const struct smm_loader_params *params) +{ + unsigned int i; + struct rmodule smm_stub; + unsigned int ss_size = params->per_cpu_save_state_size, stub_size; + unsigned int smm_entry_offset = params->smm_main_entry_offset; + unsigned int seg_count = 0, segments = 0, available; + unsigned int cpus_in_segment = 0; + unsigned int base = smbase; + + if (rmodule_parse(&_binary_smmstub_start, &smm_stub)) + printk(BIOS_ERR, "%s : unable to get SMM module size\n", __func__); + + stub_size = rmodule_memory_size(&smm_stub); + /* How many CPUs can fit into one segment? */ + available = 0xFFFF - smm_entry_offset - ss_size - stub_size; + if (available > 0) { + cpus_in_segment = available/ss_size; + /* minimum segments needed will always be 1*/ + segments = num_cpus / cpus_in_segment + 1; + printk(BIOS_DEBUG, + "%s: cpus in one segment %d\n", __func__, cpus_in_segment); + printk(BIOS_DEBUG, + "%s: min # of segments needed %d\n", __func__, segments); + } else { + printk(BIOS_ERR, + "%s: not enough space in SMM to setup all CPUs\n", __func__); + return -1; + } + + if (sizeof(cpus)/sizeof(struct cpu_smm_info) < cpus_in_segment * segments) { + printk(BIOS_ERR, + "%s: more CPUs than originally configured for\n", __func__); + return -1; + } + + for (i = 0; i < num_cpus; i++) { + cpus[i].smbase = base; + cpus[i].entry = base + smm_entry_offset; + cpus[i].ss_start = cpus[i].entry + (smm_entry_offset - ss_size); + cpus[i].code_start = cpus[i].entry; + cpus[i].code_end = cpus[i].entry + stub_size; + cpus[i].active = 1; + base -= ss_size; + seg_count++; + if (seg_count >= cpus_in_segment) { + base -= smm_entry_offset; + seg_count = 0; + } + } + + if (CONFIG_DEFAULT_CONSOLE_LOGLEVEL >= BIOS_DEBUG) { + seg_count = 0; + for (i = 0; i < num_cpus; i++) { + printk(BIOS_DEBUG, "CPU 0x%x\n", i); + printk(BIOS_DEBUG, + " smbase 0x%x entry 0x%x\n", + cpus[i].smbase, cpus[i].entry); + printk(BIOS_DEBUG, + " ss_start 0x%x code_end 0x%x\n", + cpus[i].ss_start, cpus[i].code_end); + seg_count++; + if (seg_count >= cpus_in_segment) { + printk(BIOS_DEBUG, + "-------------NEW CODE SEGMENT --------------\n"); + seg_count = 0; + } + } + } + return 1; +} + +/* + * This method expects the smm relocation map to be complete. + * This method does not read any HW registers, it simply uses a + * map that was created during SMM setup. + * input: cpu_num - cpu number which is used as an index into the + * map to return the smbase + */ +int smm_get_cpu_smbase(unsigned int cpu_num) +{ + if (cpu_num < CONFIG_MAX_CPUS) { + if (cpus[cpu_num].active) + return cpus[cpu_num].smbase; + } + return 0; +} + +/* + * This method assumes that at least 1 CPU has been set up from + * which it will place other CPUs below its smbase ensuring that + * save state does not clobber the first CPUs init code segment. The init + * code which is the smm stub code is the same for all CPUs. They enter + * smm, setup stacks (based on their apic id), enter protected mode + * and then jump to the common smi handler. The stack is allocated + * at the beginning of smram (aka tseg base, not smbase). The stack + * pointer for each CPU is calculated by using its apic id + * (code is in smm_stub.s) + * Each entry point will now have the same stub code which, sets up the CPU + * stack, enters protected mode and then jumps to the smi handler. It is + * important to enter protected mode before the jump because the "jump to + * address" might be larger than the 20bit address supported by real mode. + * SMI entry right now is in real mode. + * input: smbase - this is the smbase of the first cpu not the smbase + * where tseg starts (aka smram_start). All CPUs code segment + * and stack will be below this point except for the common + * SMI handler which is one segment above + * input: num_cpus - number of cpus that need relocation including + * the first CPU (though its code is already loaded) + * input: top of stack (stacks work downward by default in Intel HW) + * output: return -1, if runtime smi code could not be installed. In + * this case SMM will not work and any SMI's generated will + * cause a CPU shutdown or general protection fault becuause + * the appropriate smi handling code was not installed + */ +static void smm_place_entry_code(unsigned int smbase, unsigned int num_cpus, + unsigned int stack_top, const struct smm_loader_params *params) +{ + unsigned int i; + unsigned int size; + if (smm_create_map(smbase, num_cpus, params)) { + /* + * Ensure there was enough space and the last CPUs smbase + * did not encroach upon the stack. Stack top is smram start + * + size of stack. + */ + if (cpus[num_cpus].active) { + if (cpus[num_cpus - 1].smbase + + params->smm_main_entry_offset < stack_top) { + printk(BIOS_ERR, + "%s : need more stack space, stack encroachment\n", + __func__); + printk(BIOS_ERR, "%s : smbase %x, stack_top %x\n", + __func__, cpus[num_cpus].smbase, stack_top); + return; + } + } + } else { + printk(BIOS_ERR, "%s : unable to place smm entry code\n", __func__); + return; + } + + printk(BIOS_INFO, "%s : smbase %x, stack_top %x\n", + __func__, cpus[num_cpus-1].smbase, stack_top); + + /* start at 1, the first CPU stub code is already there */ + size = cpus[0].code_end - cpus[0].code_start; + for (i = 1; i < num_cpus; i++) { + memcpy((int *)cpus[i].code_start, (int *)cpus[0].code_start, size); + printk(BIOS_DEBUG, + "SMM Module: placing smm entry code at 0x%x, cpu # 0x%x\n", + cpus[i].code_start, i); + printk(BIOS_DEBUG, "%s copying from 0x%x to 0x%x 0x%x bytes\n", + __func__, cpus[0].code_start, cpus[i].code_start, size); + } + return; +} + +/* + * Place stacks in base -> base + size region, but ensure the stacks don't + * overlap the staggered entry points. + */ +static void *smm_stub_place_stacks(char *base, size_t size, + struct smm_loader_params *params) +{ + size_t total_stack_size; + char *stacks_top; + + /* If stack space is requested assume the space lives in the lower + * half of SMRAM. */ + total_stack_size = params->per_cpu_stack_size * + params->num_concurrent_stacks; + printk(BIOS_DEBUG, "%s cpus: %x : stack space: needed -> %x\n", + __func__, (int)params->num_concurrent_stacks, + (int)total_stack_size); + printk(BIOS_DEBUG, " available -> %x : per_cpu_stack_size : %x\n", + (int)size, (int)params->per_cpu_stack_size); + + /* There has to be at least one stack user. */ + if (params->num_concurrent_stacks < 1) + return NULL; + + /* Total stack size cannot fit. */ + if (total_stack_size > size) + return NULL; + + /* Stacks extend down to SMBASE */ + stacks_top = &base[total_stack_size]; + printk(BIOS_DEBUG, "%s : exit, stack_top %x\n", __func__, (int)stacks_top); + + return stacks_top; +} + +/* + * Place the staggered entry points for each CPU. The entry points are + * staggered by the per CPU SMM save state size extending down from + * SMM_ENTRY_OFFSET. + */ +static void smm_stub_place_staggered_entry_points(char *base, + const struct smm_loader_params *params, const struct rmodule *smm_stub) +{ + size_t stub_entry_offset; + + stub_entry_offset = rmodule_entry_offset(smm_stub); + /* Each CPU now has its own stub code, which enters protected mode, + * sets up the stack, and then jumps to common SMI handler + */ + if (params->num_concurrent_save_states > 1 || stub_entry_offset != 0) { + smm_place_entry_code((int)base, params->num_concurrent_save_states, + (int)params->stack_top, params); + } +} + +/* + * The stub setup code assumes it is completely contained within the + * default SMRAM size (0x10000) for the default SMI handler (entry at + * 0x30000), but no assumption should be made for the permanent SMI handler. + * The placement of CPU entry points for permanent handler are determined + * by the number of CPUs in the system and the amount of SMRAM. + * There are potentially 3 regions to place + * within the default SMRAM size: + * 1. Save state areas + * 2. Stub code + * 3. Stack areas + * + * The save state and stack areas are treated as contiguous for the number of + * concurrent areas requested. The save state always lives at the top of the + * the CPUS smbase (and the entry point is at offset 0x8000. This allows only a certain + * number of CPUs with staggered entry points until the save state area comes + * down far enough to overwrite/corrupt the entry code (stub code). Therefore, + * an SMM map is created to avoid this corruption, see smm_create_map above. + * This module setup code works for the default SMM handler setup and the + * permanent SMM handler. + */ +static int smm_module_setup_stub(void *smbase, size_t smm_size, + struct smm_loader_params *params, + void *fxsave_area) +{ + size_t total_save_state_size; + size_t smm_stub_size; + size_t stub_entry_offset; + char *smm_stub_loc; + void *stacks_top; + size_t size; + char *base; + size_t i; + struct smm_stub_params *stub_params; + struct rmodule smm_stub; + unsigned int total_size_all; + base = smbase; + size = smm_size; + + /* The number of concurrent stacks cannot exceed CONFIG_MAX_CPUS. */ + if (params->num_concurrent_stacks > CONFIG_MAX_CPUS) { + printk(BIOS_ERR, "%s : not enough stacks\n", __func__); + return -1; + } + + /* Fail if can't parse the smm stub rmodule. */ + if (rmodule_parse(&_binary_smmstub_start, &smm_stub)) { + printk(BIOS_ERR, "%s : unable to parse smm stub\n", __func__); + return -1; + } + + /* Adjust remaining size to account for save state. */ + total_save_state_size = params->per_cpu_save_state_size * + params->num_concurrent_save_states; + if (total_save_state_size > size) { + printk(BIOS_ERR, + "%s:more state save space needed:need -> %x:available->%x\n", + __func__, (int)total_save_state_size, (int)size); + return -1; + } + + size -= total_save_state_size; + + /* The save state size encroached over the first SMM entry point. */ + if (size <= params->smm_main_entry_offset) { + printk(BIOS_ERR, "%s:encroachment over SMM entry point\n", __func__); + printk(BIOS_ERR, "%s:state save size: %x : smm_entry_offset -> %x\n", + __func__, (int)size, params->smm_main_entry_offset); + return -1; + } + + /* Need a minimum stack size and alignment. */ + if (params->per_cpu_stack_size <= SMM_MINIMUM_STACK_SIZE || + (params->per_cpu_stack_size & 3) != 0) { + printk(BIOS_ERR, "%s : need minimum stack size\n", __func__); + return -1; + } + + smm_stub_loc = NULL; + smm_stub_size = rmodule_memory_size(&smm_stub); + stub_entry_offset = rmodule_entry_offset(&smm_stub); + + /* Put the stub at at the main entry point */ + smm_stub_loc = &base[params->smm_main_entry_offset]; + + /* Stub is too big to fit. */ + if (smm_stub_size > (size - params->smm_main_entry_offset)) { + printk(BIOS_ERR, "%s : stub is too big to fit\n", __func__); + return -1; + } + + /* The stacks, if requested, live in the lower half of SMRAM space + * for default handler, but for relocated handler it lives at the beginning + * of SMRAM which is TSEG base + */ + size = params->num_concurrent_stacks*params->per_cpu_stack_size; + stacks_top = smm_stub_place_stacks((char *)params->smram_start, size, params); + if (stacks_top == NULL) { + printk(BIOS_ERR, "%s : not enough space for stacks\n", __func__); + printk(BIOS_ERR, "%s : ....need -> %x : available -> %x\n", __func__, + (int)base, (int)size); + return -1; + } + params->stack_top = stacks_top; + /* Load the stub. */ + if (rmodule_load(smm_stub_loc, &smm_stub)) { + printk(BIOS_ERR, "%s : load module failed\n", __func__); + return -1; + } + + smm_stub_place_staggered_entry_points(base, params, &smm_stub); + + /* Setup the parameters for the stub code. */ + stub_params = rmodule_parameters(&smm_stub); + stub_params->stack_top = (uintptr_t)stacks_top; + stub_params->stack_size = params->per_cpu_stack_size; + stub_params->c_handler = (uintptr_t)params->handler; + stub_params->c_handler_arg = (uintptr_t)params->handler_arg; + stub_params->fxsave_area = (uintptr_t)fxsave_area; + stub_params->fxsave_area_size = FXSAVE_SIZE; + stub_params->runtime.smbase = (uintptr_t)smbase; + stub_params->runtime.smm_size = smm_size; + stub_params->runtime.save_state_size = params->per_cpu_save_state_size; + stub_params->runtime.num_cpus = params->num_concurrent_stacks; + + printk(BIOS_DEBUG, "%s : stack_end = 0x%x\n", + __func__, (int)stub_params->runtime.smbase); + printk(BIOS_DEBUG, + "%s : stack_top = 0x%x\n", __func__, (int)stub_params->stack_top); + printk(BIOS_DEBUG, "%s : stack_size = 0x%x\n", + __func__, (int)stub_params->stack_size); + printk(BIOS_DEBUG, "%s : runtime.smbase = 0x%x\n", + __func__, (int)stub_params->runtime.smbase); + printk(BIOS_DEBUG, "%s : runtime.start32_offset = 0x%x\n", __func__, + (int)stub_params->runtime.start32_offset); + printk(BIOS_DEBUG, "%s : runtime.smm_size = 0x%x\n", + __func__, (int)smm_size); + printk(BIOS_DEBUG, "%s : per_cpu_save_state_size = 0x%x\n", + __func__, (int)stub_params->runtime.save_state_size); + printk(BIOS_DEBUG, "%s : num_cpus = 0x%x\n", __func__, + (int)stub_params->runtime.num_cpus); + printk(BIOS_DEBUG, "%s : total_save_state_size = 0x%x\n", + __func__, (int)(stub_params->runtime.save_state_size* + stub_params->runtime.num_cpus)); + total_size_all = (int)stub_params->stack_size + + (int)(stub_params->runtime.save_state_size* + stub_params->runtime.num_cpus); + printk(BIOS_DEBUG, "%s : total_size_all = 0x%x\n", __func__, + total_size_all); + + /* Initialize the APIC id to CPU number table to be 1:1 */ + for (i = 0; i < params->num_concurrent_stacks; i++) + stub_params->runtime.apic_id_to_cpu[i] = i; + + /* Allow the initiator to manipulate SMM stub parameters. */ + params->runtime = &stub_params->runtime; + + printk(BIOS_DEBUG, "SMM Module: stub loaded at %p. Will call %p(%p)\n", + smm_stub_loc, params->handler, params->handler_arg); + return 0; +} + +/* + * smm_setup_relocation_handler assumes the callback is already loaded in + * memory. i.e. Another SMM module isn't chained to the stub. The other + * assumption is that the stub will be entered from the default SMRAM + * location: 0x30000 -> 0x40000. + */ +int smm_setup_relocation_handler(struct smm_loader_params *params) +{ + void *smram = (void *)(SMM_DEFAULT_BASE); + printk(BIOS_SPEW, "%s: enter\n", __func__); + /* There can't be more than 1 concurrent save state for the relocation + * handler because all CPUs default to 0x30000 as SMBASE. */ + if (params->num_concurrent_save_states > 1) + return -1; + + /* A handler has to be defined to call for relocation. */ + if (params->handler == NULL) + return -1; + + /* Since the relocation handler always uses stack, adjust the number + * of concurrent stack users to be CONFIG_MAX_CPUS. */ + if (params->num_concurrent_stacks == 0) + params->num_concurrent_stacks = CONFIG_MAX_CPUS; + + params->smm_main_entry_offset = SMM_ENTRY_OFFSET; + params->smram_start = SMM_DEFAULT_BASE; + params->smram_end = SMM_DEFAULT_BASE + SMM_DEFAULT_SIZE; + return smm_module_setup_stub(smram, SMM_DEFAULT_SIZE, + params, fxsave_area_relocation); + printk(BIOS_SPEW, "%s: exit\n", __func__); +} + +/* + *The SMM module is placed within the provided region in the following + * manner: + * +-----------------+ <- smram + size + * | BIOS resource | + * | list (STM) | + * +-----------------+ + * | fxsave area | + * +-----------------+ + * | smi handler | + * | ... | + * +-----------------+ <- cpu0 + * | stub code | <- cpu1 + * | stub code | <- cpu2 + * | stub code | <- cpu3, etc + * | | + * | | + * | | + * | stacks | + * +-----------------+ <- smram start + + * It should be noted that this algorithm will not work for + * SMM_DEFAULT_SIZE SMRAM regions such as the A segment. This algorithm + * expects a region large enough to encompass the handler and stacks + * as well as the SMM_DEFAULT_SIZE. + */ +int smm_load_module(void *smram, size_t size, struct smm_loader_params *params) +{ + struct rmodule smm_mod; + size_t total_stack_size; + size_t handler_size; + size_t module_alignment; + size_t alignment_size; + size_t fxsave_size; + void *fxsave_area; + size_t total_size = 0; + char *base; + + if (size <= SMM_DEFAULT_SIZE) + return -1; + + /* Load main SMI handler at the top of SMRAM + * everything else will go below + */ + base = smram; + base += size; + params->smram_start = (uintptr_t)smram; + params->smram_end = params->smram_start + size; + params->smm_main_entry_offset = SMM_ENTRY_OFFSET; + + /* Fail if can't parse the smm rmodule. */ + if (rmodule_parse(&_binary_smm_start, &smm_mod)) + return -1; + + /* Clear SMM region */ + if (CONFIG(DEBUG_SMI)) + memset(smram, 0xcd, size); + + total_stack_size = params->per_cpu_stack_size * + params->num_concurrent_stacks; + total_size += total_stack_size; + /* Stacks are the base of SMRAM */ + params->stack_top = smram + total_stack_size; + + /* MSEG starts at the top of SMRAM and works down */ + if (CONFIG(STM)) { + base -= CONFIG_MSEG_SIZE + CONFIG_BIOS_RESOURCE_LIST_SIZE; + total_size += CONFIG_MSEG_SIZE + CONFIG_BIOS_RESOURCE_LIST_SIZE; + } + + /* FXSAVE goes below MSEG */ + if (CONFIG(SSE)) { + fxsave_size = FXSAVE_SIZE * params->num_concurrent_stacks; + fxsave_area = base - fxsave_size; + base -= fxsave_size; + total_size += fxsave_size; + } else { + fxsave_size = 0; + fxsave_area = NULL; + } + + + handler_size = rmodule_memory_size(&smm_mod); + base -= handler_size; + total_size += handler_size; + module_alignment = rmodule_load_alignment(&smm_mod); + alignment_size = module_alignment - + ((uintptr_t)base % module_alignment); + if (alignment_size != module_alignment) { + handler_size += alignment_size; + base += alignment_size; + } + + printk(BIOS_DEBUG, + "%s : total_smm_space_needed 0x%x, available -> 0x%x\n", + __func__, (int)total_size, (int)size); + + /* Does the required amount of memory exceed the SMRAM region size? */ + if (total_size > size) { + printk(BIOS_ERR, + "%s : need more SMRAM\n", __func__); + return -1; + } + if (handler_size > SMM_CODE_SEGMENT_SIZE) { + printk(BIOS_ERR, "%s : increase SMM_CODE_SEGMENT_SIZE: handler_size = 0x%0x\n", + __func__, (int)handler_size); + return -1; + } + + if (rmodule_load(base, &smm_mod)) + return -1; + + params->handler = rmodule_entry(&smm_mod); + params->handler_arg = rmodule_parameters(&smm_mod); + + printk(BIOS_DEBUG, "%s : smram_start: 0x%x\n", + __func__, (int)smram); + printk(BIOS_DEBUG, "%s : smram_end: 0x%x\n", + __func__, (int)smram + (int)size); + printk(BIOS_DEBUG, "%s : stack_top: 0x%x\n", + __func__, (int)params->stack_top); + printk(BIOS_DEBUG, "%s : handler start 0x%x\n", + __func__, (int)params->handler); + printk(BIOS_DEBUG, "%s : handler_size 0x%x\n", + __func__, (int)handler_size); + printk(BIOS_DEBUG, "%s : handler_arg 0x%x\n", + __func__, (int)params->handler_arg); + printk(BIOS_DEBUG, "%s : fxsave_area 0x%x\n", + __func__, (int)fxsave_area); + printk(BIOS_DEBUG, "%s : fxsave_size 0x%x\n", + __func__, (int)fxsave_size); + printk(BIOS_DEBUG, "%s : CONFIG_MSEG_SIZE 0x%x\n", + __func__, (int)CONFIG_MSEG_SIZE); + printk(BIOS_DEBUG, "%s : CONFIG_BIOS_RESOURCE_LIST_SIZE 0x%x\n", + __func__, (int)CONFIG_BIOS_RESOURCE_LIST_SIZE); + + /* CPU 0 smbase goes first, all other CPUs + * will be staggered below + */ + base -= SMM_CODE_SEGMENT_SIZE; + printk(BIOS_DEBUG, "%s : cpu0 entry: 0x%x\n", + __func__, (int)base); + params->smm_entry = (uintptr_t)base + params->smm_main_entry_offset; + return smm_module_setup_stub(base, size, params, fxsave_area); +} diff --git a/src/include/cpu/x86/smm.h b/src/include/cpu/x86/smm.h index a3101e5..7838223 100644 --- a/src/include/cpu/x86/smm.h +++ b/src/include/cpu/x86/smm.h @@ -128,6 +128,12 @@ * into this field so the code doing the loading can manipulate the * runtime's assumptions. e.g. updating the APIC id to CPU map to * handle sparse APIC id space. + * The following parameters are only used when X86_SMM_LOADER_VERSION2 is enabled. + * - smm_entry - entry address of first CPU thread, all others will be tiled + * below this address. + * - smm_main_entry_offset - default entry offset (e.g 0x8000) + * - smram_start - smaram starting address + * - smram_end - smram ending address */ struct smm_loader_params { void *stack_top; @@ -141,12 +147,24 @@ void *handler_arg;
struct smm_runtime *runtime; + + /* The following are only used by X86_SMM_LOADER_VERSION2 */ + #if CONFIG(X86_SMM_LOADER_VERSION2) + unsigned int smm_entry; + unsigned int smm_main_entry_offset; + unsigned int smram_start; + unsigned int smram_end; + #endif };
/* Both of these return 0 on success, < 0 on failure. */ int smm_setup_relocation_handler(struct smm_loader_params *params); int smm_load_module(void *smram, size_t size, struct smm_loader_params *params);
+#if CONFIG(X86_SMM_LOADER_VERSION2) +int smm_get_cpu_smbase(unsigned int cpu_num); +#endif + /* Backup and restore default SMM region. */ void *backup_default_smm_area(void); void restore_default_smm_area(void *smm_save_area);
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: SMM module loader version 2 ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43684/1/src/cpu/x86/smm/smm_module_... File src/cpu/x86/smm/smm_module_loaderv2.c:
https://review.coreboot.org/c/coreboot/+/43684/1/src/cpu/x86/smm/smm_module_... PS1, Line 253: } void function return statements are not generally useful
Rocky Phagura has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: SMM module loader version 2 ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43684/1/src/cpu/x86/smm/smm_module_... File src/cpu/x86/smm/smm_module_loaderv2.c:
https://review.coreboot.org/c/coreboot/+/43684/1/src/cpu/x86/smm/smm_module_... PS1, Line 253: }
void function return statements are not generally useful
Done
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Patrick Rudolph, Jonathan Zhang, Eugene Myers, David Hendricks, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43684
to look at the new patch set (#2).
Change subject: cpu/x86/smm: SMM module loader version 2 ......................................................................
cpu/x86/smm: SMM module loader version 2
Xeon-SP a Skylake Scalable Processor can have 36 CPU threads(18 cores). Current coreboot SMM is unable to handle more than ~32 CPU threads. This patch introduces a version 2 of the SMM module loader which addresses this problem. Having two versions of the SMM module loader prevents any issues to current projects. Future Xeon-SP products will be using this version of the SMM loader. Subsequent patches will enable board specifc functionality for Xeon-SP.
The reason for moving to version 2 is the state save area begins to encroach upon the smi handling code when more than 32 CPU threads are in the system. This can cause system hangs, reboots, etc. The second change is related to staggered entry points with simple near jumps. In the current loader, near jumps will not work because the CPU is jumping within the same code segment. In version 2, "far" address jumps are necessary therefore protected mode must is enabled first. The SMM layout and how the CPUs are staggered are documented in the code.
By making the modifications above, this allows the smm module loader to expand easily as more CPU threads are added.
TEST=build for Tiogapass platform under OCP mainboard. Enable the following in Kconfig. select CPU_INTEL_COMMON_SMM select SOC_INTEL_COMMON_BLOCK_SMM select SMM_TSEG select HAVE_SMI_HANDLER select ACPI_INTEL_HARDWARE_SLEEP_VALUES
Debug console will show all 36 cores relocated. Further tested by generating SMI's to port 0xb2 using XDP/ITP HW debugger and ensured all cores entering and exiting SMM properly. In addition, booted to Linux 5.4 kernel and observed no issues during mp init.
Change-Id: I00a23a5f2a46110536c344254868390dbb71854c Signed-off-by: Rocky Phagura rphagura@fb.com --- M src/cpu/x86/Kconfig M src/cpu/x86/smm/Makefile.inc A src/cpu/x86/smm/smm_module_loaderv2.c M src/include/cpu/x86/smm.h 4 files changed, 678 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/43684/2
Rocky Phagura has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: SMM module loader version 2 ......................................................................
Patch Set 2:
Incorporated feedback from David from another review thread. The old SMM patch will be abandoned soon (https://review.coreboot.org/c/coreboot/+/41829) and for now has been moved to WIP. Please provide feedback on this patch as necessary. I've incorporated all the feedback from the old one (if you provided feedback on the old patch, its already been fixed here).
Thank you for taking the time to review and provide feedback.
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: SMM module loader version 2 ......................................................................
Patch Set 2:
Stefan, you wrote the original SMM loading, I think, so adding you here. Aaron, Furquan, any opinions?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: SMM module loader version 2 ......................................................................
Patch Set 2: Code-Review+1
(12 comments)
https://review.coreboot.org/c/coreboot/+/43684/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43684/2//COMMIT_MSG@9 PS2, Line 9: Xeon-SP a Skylake Scalable Processor can have 36 CPU threads(18 cores). Current nit: Please wrap these lines at 72 characters
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/Kconfig File src/cpu/x86/Kconfig:
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/Kconfig@125 PS2, Line 125: bool Could we please add a prompt here so that one can easily enable it?
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... File src/cpu/x86/smm/smm_module_loaderv2.c:
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 15: uint8_t #include <stdint.h>
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 51: unsigned int Are these memory addresses? If so, I'd use uintptr_t
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 108: printk(BIOS_ERR, "%s: unable to get SMM module size\n", __func__); Can we continue after this error?
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 112: 0xFFFF What does this magic number mean?
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 176: int Shouldn't this be an uintptr_t as well?
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 213: unsigned int uintptr_t
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 226: params->smm_main_entry_offset < stack_top) { I'd indent this part differently so that it is clear it's a continuation (e.g. add a tab)
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 231: __func__, cpus[num_cpus].smbase, stack_top); one more tab
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 604: "%s: need more SMRAM\n", __func__); add another tab
https://review.coreboot.org/c/coreboot/+/43684/2/src/include/cpu/x86/smm.h File src/include/cpu/x86/smm.h:
https://review.coreboot.org/c/coreboot/+/43684/2/src/include/cpu/x86/smm.h@1... PS2, Line 152: #if CONFIG(X86_SMM_LOADER_VERSION2) nit: I would not indent preprocessor
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: SMM module loader version 2 ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... File src/cpu/x86/smm/smm_module_loaderv2.c:
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 247: BIOS`_DEBUG stray `
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: SMM module loader version 2 ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/Kconfig File src/cpu/x86/Kconfig:
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/Kconfig@129 PS2, Line 129: server nit: maybe deemphasize the "server" mentions a bit? There are few non-server designs out there with more than 32 threads right now, but that may change over the next few years and nothing in here is strictly "server stuff".
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... File src/cpu/x86/smm/smm_module_loaderv2.c:
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 123: not enough space in SMM to setup all CPUs So how would a firmware developer start to solve this, or is SMM just not available on designs with too many threads?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: SMM module loader version 2 ......................................................................
Patch Set 2:
I tested this on Lenovo X230 (CPU: Intel(R) Core(TM) i5-3320M CPU @ 2.60GHz) and Asrock B85M Pro4 (CPU: Intel(R) Core(TM) i7-4770S CPU @ 3.10GHz): both locked up inside `src/cpu/x86/smm/smi_trigger.c` function `apm_control()`.
Log of the Asrock B85M Pro4: https://paste.flashrom.org/view.php?id=3349
Rocky Phagura has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: SMM module loader version 2 ......................................................................
Patch Set 2:
Patch Set 2:
I tested this on Lenovo X230 (CPU: Intel(R) Core(TM) i5-3320M CPU @ 2.60GHz) and Asrock B85M Pro4 (CPU: Intel(R) Core(TM) i7-4770S CPU @ 3.10GHz): both locked up inside `src/cpu/x86/smm/smi_trigger.c` function `apm_control()`.
Log of the Asrock B85M Pro4: https://paste.flashrom.org/view.php?id=3349
Angel, can you please set MAX_CPUS to a 8 or more in Kconfig? In server mp_init code we error out if MAX_CPUS is less than what's found in the system, but that's not happening here and hence we are getting this error in the log: smm_create_map: more CPUs than originally configured for
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: SMM module loader version 2 ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2:
I tested this on Lenovo X230 (CPU: Intel(R) Core(TM) i5-3320M CPU @ 2.60GHz) and Asrock B85M Pro4 (CPU: Intel(R) Core(TM) i7-4770S CPU @ 3.10GHz): both locked up inside `src/cpu/x86/smm/smi_trigger.c` function `apm_control()`.
Log of the Asrock B85M Pro4: https://paste.flashrom.org/view.php?id=3349
Angel, can you please set MAX_CPUS to a 8 or more in Kconfig? In server mp_init code we error out if MAX_CPUS is less than what's found in the system, but that's not happening here and hence we are getting this error in the log: smm_create_map: more CPUs than originally configured for
Weird... I have MAX_CPUS=8 when building for the Asrock B85M Pro4 and the i7-4770S is a quad-core with HyperThreading, so it has 8 threads... Should be enough. I wonder where `cpus in one segment 30` comes from.
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: SMM module loader version 2 ......................................................................
Patch Set 2:
Rocky, could you please add an entry to Documentation/releases/coreboot-4.13-relnotes.md for this new capability?
Hello build bot (Jenkins), Patrick Georgi, Furquan Shaikh, Martin Roth, Patrick Rudolph, Jonathan Zhang, Eugene Myers, Stefan Reinauer, David Hendricks, Angel Pons, Aaron Durbin, Eugene Myers,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43684
to look at the new patch set (#3).
Change subject: cpu/x86/smm: SMM module loader version 2 ......................................................................
cpu/x86/smm: SMM module loader version 2
Xeon-SP a Skylake Scalable Processor can have 36 CPU threads(18 cores). Current coreboot SMM is unable to handle more than ~32 CPU threads. This patch introduces a version 2 of the SMM module loader which addresses this problem. Having two versions of the SMM module loader prevents any issues to current projects. Future Xeon-SP products will be using this version of the SMM loader. Subsequent patches will enable board specifc functionality for Xeon-SP.
The reason for moving to version 2 is the state save area begins to encroach upon the smi handling code when more than 32 CPU threads are in the system. This can cause system hangs, reboots, etc. The second change is related to staggered entry points with simple near jumps. In the current loader, near jumps will not work because the CPU is jumping within the same code segment. In version 2, "far" address jumps are necessary therefore protected mode must is enabled first. The SMM layout and how the CPUs are staggered are documented in the code.
By making the modifications above, this allows the smm module loader to expand easily as more CPU threads are added.
TEST=build for Tiogapass platform under OCP mainboard. Enable the following in Kconfig. select CPU_INTEL_COMMON_SMM select SOC_INTEL_COMMON_BLOCK_SMM select SMM_TSEG select HAVE_SMI_HANDLER select ACPI_INTEL_HARDWARE_SLEEP_VALUES
Debug console will show all 36 cores relocated. Further tested by generating SMI's to port 0xb2 using XDP/ITP HW debugger and ensured all cores entering and exiting SMM properly. In addition, booted to Linux 5.4 kernel and observed no issues during mp init.
Change-Id: I00a23a5f2a46110536c344254868390dbb71854c Signed-off-by: Rocky Phagura rphagura@fb.com --- M src/cpu/x86/Kconfig M src/cpu/x86/smm/Makefile.inc A src/cpu/x86/smm/smm_module_loaderv2.c M src/include/cpu/x86/smm.h 4 files changed, 684 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/43684/3
Rocky Phagura has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: SMM module loader version 2 ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2:
Patch Set 2:
I tested this on Lenovo X230 (CPU: Intel(R) Core(TM) i5-3320M CPU @ 2.60GHz) and Asrock B85M Pro4 (CPU: Intel(R) Core(TM) i7-4770S CPU @ 3.10GHz): both locked up inside `src/cpu/x86/smm/smi_trigger.c` function `apm_control()`.
Log of the Asrock B85M Pro4: https://paste.flashrom.org/view.php?id=3349
Angel, can you please set MAX_CPUS to a 8 or more in Kconfig? In server mp_init code we error out if MAX_CPUS is less than what's found in the system, but that's not happening here and hence we are getting this error in the log: smm_create_map: more CPUs than originally configured for
Weird... I have MAX_CPUS=8 when building for the Asrock B85M Pro4 and the i7-4770S is a quad-core with HyperThreading, so it has 8 threads... Should be enough. I wonder where `cpus in one segment 30` comes from.
Angel, I think I found the issue. I've uploaded a new patch. The changes are in smm_loaderv2.c only. It provides more debug information and solves the issue with error you were seeing related to: smm_create_map: more CPUs than originally configured for. I tested this on my system by using various CPU threads and its working. Please try with these changes. Thank you for your help!
Rocky Phagura has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: SMM module loader version 2 ......................................................................
Patch Set 3:
Patch Set 2:
Rocky, could you please add an entry to Documentation/releases/coreboot-4.13-relnotes.md for this new capability?
Patrick, This is a good idea. I will add this to the documentation. Thx
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: SMM module loader version 2 ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... File src/cpu/x86/smm/smm_module_loaderv2.c:
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 247: BIOS`_DEBUG
stray `
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: SMM module loader version 2 ......................................................................
Patch Set 3: Code-Review+1
Patch Set 2:
Patch Set 2:
Patch Set 2:
Patch Set 2:
I tested this on Lenovo X230 (CPU: Intel(R) Core(TM) i5-3320M CPU @ 2.60GHz) and Asrock B85M Pro4 (CPU: Intel(R) Core(TM) i7-4770S CPU @ 3.10GHz): both locked up inside `src/cpu/x86/smm/smi_trigger.c` function `apm_control()`.
Log of the Asrock B85M Pro4: https://paste.flashrom.org/view.php?id=3349
Angel, can you please set MAX_CPUS to a 8 or more in Kconfig? In server mp_init code we error out if MAX_CPUS is less than what's found in the system, but that's not happening here and hence we are getting this error in the log: smm_create_map: more CPUs than originally configured for
Weird... I have MAX_CPUS=8 when building for the Asrock B85M Pro4 and the i7-4770S is a quad-core with HyperThreading, so it has 8 threads... Should be enough. I wonder where `cpus in one segment 30` comes from.
Angel, I think I found the issue. I've uploaded a new patch. The changes are in smm_loaderv2.c only. It provides more debug information and solves the issue with error you were seeing related to: smm_create_map: more CPUs than originally configured for. I tested this on my system by using various CPU threads and its working. Please try with these changes. Thank you for your help!
I tried again, I ended up with the same problem. I think I found out why, though.
In `src/cpu/x86/mp_init.c`, function `smm_do_relocation`, there's a calculation that needs to match that of the SMM module loader. In my case, it didn't, and that's why I was getting hangs whenever SMM was called. Adding this offset worked for me on Haswell (but it's not a good idea to hardcode it):
perm_smbase += (0x7faea000 - 0x7f800000);
The first value is the location of the relocated SMBASE for CPU #0. The second value is smram_start.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: SMM module loader version 2 ......................................................................
Patch Set 3:
I've pushed the fixups here: CB:44174
I've asked around on IRC, and looks like there's people who don't think having yet another version of the SMM module loader is a good idea. IMHO, now that I more or less understand how it works, we might as well replace the current loader with this.
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: SMM module loader version 2 ......................................................................
Patch Set 3:
Patch Set 3:
I've pushed the fixups here: CB:44174
I've asked around on IRC, and looks like there's people who don't think having yet another version of the SMM module loader is a good idea. IMHO, now that I more or less understand how it works, we might as well replace the current loader with this.
The idea was to have them in parallel for a short while so we can more easily debug issues and transition chipsets one by one. Even mid-term we should only have one loader.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: SMM module loader version 2 ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 3:
I've pushed the fixups here: CB:44174
I've asked around on IRC, and looks like there's people who don't think having yet another version of the SMM module loader is a good idea. IMHO, now that I more or less understand how it works, we might as well replace the current loader with this.
The idea was to have them in parallel for a short while so we can more easily debug issues and transition chipsets one by one. Even mid-term we should only have one loader.
Arthur tested this on Lenovo X60: http://ix.io/2tcy
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: SMM module loader version 2 ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43684/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43684/3//COMMIT_MSG@7 PS3, Line 7: cpu/x86/smm: SMM module loader version 2 Please make it a statement by adding a verb (in imperative mood).
Rocky Phagura has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: SMM module loader version 2 ......................................................................
Patch Set 3:
Patch Set 3: Code-Review+1
Patch Set 2:
Patch Set 2:
Patch Set 2:
Patch Set 2:
I tested this on Lenovo X230 (CPU: Intel(R) Core(TM) i5-3320M CPU @ 2.60GHz) and Asrock B85M Pro4 (CPU: Intel(R) Core(TM) i7-4770S CPU @ 3.10GHz): both locked up inside `src/cpu/x86/smm/smi_trigger.c` function `apm_control()`.
Log of the Asrock B85M Pro4: https://paste.flashrom.org/view.php?id=3349
Angel, can you please set MAX_CPUS to a 8 or more in Kconfig? In server mp_init code we error out if MAX_CPUS is less than what's found in the system, but that's not happening here and hence we are getting this error in the log: smm_create_map: more CPUs than originally configured for
Weird... I have MAX_CPUS=8 when building for the Asrock B85M Pro4 and the i7-4770S is a quad-core with HyperThreading, so it has 8 threads... Should be enough. I wonder where `cpus in one segment 30` comes from.
Angel, I think I found the issue. I've uploaded a new patch. The changes are in smm_loaderv2.c only. It provides more debug information and solves the issue with error you were seeing related to: smm_create_map: more CPUs than originally configured for. I tested this on my system by using various CPU threads and its working. Please try with these changes. Thank you for your help!
I tried again, I ended up with the same problem. I think I found out why, though.
In `src/cpu/x86/mp_init.c`, function `smm_do_relocation`, there's a calculation that needs to match that of the SMM module loader. In my case, it didn't, and that's why I was getting hangs whenever SMM was called. Adding this offset worked for me on Haswell (but it's not a good idea to hardcode it):
perm_smbase += (0x7faea000 - 0x7f800000);
The first value is the location of the relocated SMBASE for CPU #0. The second value is smram_start.
Angel, I completely understand the problem you are having. Yes, you are right. In mp_init.c, there is some code that is needed. Here is what I have in my mp_init.c, it was in my original patch but not in loader version 2, hence why are you seeing the problems. The code below needs to go in smm_do_relocation.
/* * The permanent handler runs with all cpus concurrently. Get the location * of the SMBASE for this CPU since the loader already created a map of all * CPU threads and where each entry point will reside in the SMRAM region */ perm_smbase = smm_get_cpu_smbase(cpu); mp_state.perm_smbase = perm_smbase; if (perm_smbase <= 0) { printk(BIOS_ERR, "%s : error, smbase 0x%x not found for this cpu 0x%x\n", __func__, (int)perm_smbase, cpu); return; } mp_state.ops.relocation_handler(cpu, curr_smbase, perm_smbase);
So I will add this snippet into mp_init.c and update this patch if you agree.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: SMM module loader version 2 ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 3: Code-Review+1
I tried again, I ended up with the same problem. I think I found out why, though.
In `src/cpu/x86/mp_init.c`, function `smm_do_relocation`, there's a calculation that needs to match that of the SMM module loader. In my case, it didn't, and that's why I was getting hangs whenever SMM was called. Adding this offset worked for me on Haswell (but it's not a good idea to hardcode it):
perm_smbase += (0x7faea000 - 0x7f800000);
The first value is the location of the relocated SMBASE for CPU #0. The second value is smram_start.
Angel, I completely understand the problem you are having. Yes, you are right. In mp_init.c, there is some code that is needed. Here is what I have in my mp_init.c, it was in my original patch but not in loader version 2, hence why are you seeing the problems. The code below needs to go in smm_do_relocation.
/* * The permanent handler runs with all cpus concurrently. Get the location * of the SMBASE for this CPU since the loader already created a map of all * CPU threads and where each entry point will reside in the SMRAM region */ perm_smbase = smm_get_cpu_smbase(cpu); mp_state.perm_smbase = perm_smbase; if (perm_smbase <= 0) { printk(BIOS_ERR, "%s : error, smbase 0x%x not found for this cpu 0x%x\n", __func__, (int)perm_smbase, cpu); return; } mp_state.ops.relocation_handler(cpu, curr_smbase, perm_smbase);
So I will add this snippet into mp_init.c and update this patch if you agree.
Sure. You can also grab what I've done on CB:44174 which would work for both smm module loaders. Also, David and I left some comments on other patchsets, which would be good to take care of as well.
Rocky Phagura has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: Introduce SMM module loader version 2 ......................................................................
Patch Set 4:
(1 comment)
Angel, I incorporated your changes from the other patch. I've added one more change in mp_init.c. There are 2 SMM regions: 1. Default Region at 30000h. During this relocation phase only so many CPUs can relocate at the same time otherwise the state save will corrupt the entry code. Therefore, you will notice I added code to handle this case which means CPUs will relocate one at a time. This will add a slight delay to the boot time (hopefully its negligible). Unfortunately there is nothing we can do because of the SMRAM HW limitations.
https://review.coreboot.org/c/coreboot/+/43684/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43684/3//COMMIT_MSG@7 PS3, Line 7: cpu/x86/smm: SMM module loader version 2
Please make it a statement by adding a verb (in imperative mood).
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: Introduce SMM module loader version 2 ......................................................................
Patch Set 4: Code-Review+1
(15 comments)
Patch Set 4:
(1 comment)
Angel, I incorporated your changes from the other patch. I've added one more change in mp_init.c. There are 2 SMM regions:
- Default Region at 30000h. During this relocation phase only so many CPUs can relocate at the same time otherwise the state save will corrupt the entry code. Therefore, you will notice I added code to handle this case which means CPUs will relocate one at a time. This will add a slight delay to the boot time (hopefully its negligible). Unfortunately there is nothing we can do because of the SMRAM HW limitations.
IMHO, looks good. I've bumped all comments from older patchsets, most of which are not done yet.
Another thing: to make sure this code gets build-tested, I would add `CONFIG_X86_SMM_LOADER_VERSION2=y` to `configs/config.asrock_b85m_pro4.debug_smmstore_hotplug_gcov_ubsan_em100`. This is a config file I've added to build-test several options that were previously not build-tested.
https://review.coreboot.org/c/coreboot/+/43684/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43684/2//COMMIT_MSG@9 PS2, Line 9: Xeon-SP a Skylake Scalable Processor can have 36 CPU threads(18 cores). Current
nit: Please wrap these lines at 72 characters
*poke*
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/Kconfig File src/cpu/x86/Kconfig:
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/Kconfig@125 PS2, Line 125: bool
Could we please add a prompt here so that one can easily enable it?
No need
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/Kconfig@129 PS2, Line 129: server
nit: maybe deemphasize the "server" mentions a bit? There are few non-server designs out there with […]
Agreed, this works even on non-server stuff
https://review.coreboot.org/c/coreboot/+/43684/4/src/cpu/x86/mp_init.c File src/cpu/x86/mp_init.c:
https://review.coreboot.org/c/coreboot/+/43684/4/src/cpu/x86/mp_init.c@734 PS4, Line 734: __func__, cpu); Fits on the previous line (line length limit is 96 characters)
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... File src/cpu/x86/smm/smm_module_loaderv2.c:
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 15: uint8_t
#include <stdint. […]
*poke*
Or, if you use `bool` and `size_t`, just #include <types.h>
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 51: unsigned int
Are these memory addresses? If so, I'd use uintptr_t
*poke*
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 108: printk(BIOS_ERR, "%s: unable to get SMM module size\n", __func__);
Can we continue after this error?
*poke*
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 112: 0xFFFF
What does this magic number mean?
*poke*
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 123: not enough space in SMM to setup all CPUs
So how would a firmware developer start to solve this, or is SMM just not available on designs with […]
*poke*
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 176: int
Shouldn't this be an uintptr_t as well?
Done
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 213: unsigned int
uintptr_t
*poke*
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 226: params->smm_main_entry_offset < stack_top) {
I'd indent this part differently so that it is clear it's a continuation (e.g. […]
*poke*
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 231: __func__, cpus[num_cpus].smbase, stack_top);
one more tab
*poke*
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 604: "%s: need more SMRAM\n", __func__);
add another tab
*poke*
Or reflow, it fits in a single line
https://review.coreboot.org/c/coreboot/+/43684/2/src/include/cpu/x86/smm.h File src/include/cpu/x86/smm.h:
https://review.coreboot.org/c/coreboot/+/43684/2/src/include/cpu/x86/smm.h@1... PS2, Line 152: #if CONFIG(X86_SMM_LOADER_VERSION2)
nit: I would not indent preprocessor
*poke*
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: Introduce SMM module loader version 2 ......................................................................
Patch Set 4: Code-Review+1
Also, David and I left some comments on other patchsets, which would be good to take care of as well.
Mine were mostly just about unnecessary casts, but Rocky took care of those in this version.
Hello build bot (Jenkins), Patrick Georgi, Furquan Shaikh, Martin Roth, Patrick Rudolph, Jonathan Zhang, Eugene Myers, David Hendricks, Stefan Reinauer, Angel Pons, Aaron Durbin, Eugene Myers,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43684
to look at the new patch set (#5).
Change subject: cpu/x86/smm: Introduce SMM module loader version 2 ......................................................................
cpu/x86/smm: Introduce SMM module loader version 2
Xeon-SP a Skylake Scalable Processor can have 36 CPU threads(18 cores). Current coreboot SMM is unable to handle more than ~32 CPU threads. This patch introduces a version 2 of the SMM module loader which addresses this problem. Having two versions of the SMM module loader prevents any issues to current projects. Future Xeon-SP products will be using this version of the SMM loader. Subsequent patches will enable board specifc functionality for Xeon-SP.
The reason for moving to version 2 is the state save area begins to encroach upon the smi handling code when more than 32 CPU threads are in the system. This can cause system hangs, reboots, etc. The second change is related to staggered entry points with simple near jumps. In the current loader, near jumps will not work because the CPU is jumping within the same code segment. In version 2, "far" address jumps are necessary therefore protected mode must is enabled first. The SMM layout and how the CPUs are staggered are documented in the code.
By making the modifications above, this allows the smm module loader to expand easily as more CPU threads are added.
TEST=build for Tiogapass platform under OCP mainboard. Enable the following in Kconfig. select CPU_INTEL_COMMON_SMM select SOC_INTEL_COMMON_BLOCK_SMM select SMM_TSEG select HAVE_SMI_HANDLER select ACPI_INTEL_HARDWARE_SLEEP_VALUES
Debug console will show all 36 cores relocated. Further tested by generating SMI's to port 0xb2 using XDP/ITP HW debugger and ensured all cores entering and exiting SMM properly. In addition, booted to Linux 5.4 kernel and observed no issues during mp init.
Change-Id: I00a23a5f2a46110536c344254868390dbb71854c Signed-off-by: Rocky Phagura rphagura@fb.com --- M Documentation/releases/coreboot-4.13-relnotes.md M src/cpu/x86/Kconfig M src/cpu/x86/mp_init.c M src/cpu/x86/smm/Makefile.inc A src/cpu/x86/smm/smm_module_loaderv2.c M src/include/cpu/x86/smm.h 6 files changed, 725 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/43684/5
Rocky Phagura has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: Introduce SMM module loader version 2 ......................................................................
Patch Set 5:
(13 comments)
Incorporated feedback and uploaded the changes. Also updated Documentation/releases/coreboot-4.13.relnotes.md.
Thank you for testing and providing feedback everybody!
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/Kconfig File src/cpu/x86/Kconfig:
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/Kconfig@125 PS2, Line 125: bool
No need
So no changes that I need to make here?
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/Kconfig@129 PS2, Line 129: server
Agreed, this works even on non-server stuff
Done
https://review.coreboot.org/c/coreboot/+/43684/4/src/cpu/x86/mp_init.c File src/cpu/x86/mp_init.c:
https://review.coreboot.org/c/coreboot/+/43684/4/src/cpu/x86/mp_init.c@734 PS4, Line 734: __func__, cpu);
Fits on the previous line (line length limit is 96 characters)
Done
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... File src/cpu/x86/smm/smm_module_loaderv2.c:
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 15: uint8_t
*poke* […]
This is same as previous loader, so I didn't change anything. I left it as is.
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 51: unsigned int
*poke*
done
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 108: printk(BIOS_ERR, "%s: unable to get SMM module size\n", __func__);
*poke*
No, we should not continue. Good catch. I've made the fix by adding a return 0 there.
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 112: 0xFFFF
*poke*
Yes, this can be confusing. It means 64K segment. I've now added a clarification in the above comment line.
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 123: not enough space in SMM to setup all CPUs
*poke*
A developer would have to read the Intel Software Developer's Manuals and the datasheet of the specific SOC to determine how much SMRAM is supported. This is based on various features available in the HW.
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 213: unsigned int
*poke*
Done
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 226: params->smm_main_entry_offset < stack_top) {
*poke*
Done
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 231: __func__, cpus[num_cpus].smbase, stack_top);
*poke*
Done
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 604: "%s: need more SMRAM\n", __func__);
*poke* […]
Done
https://review.coreboot.org/c/coreboot/+/43684/2/src/include/cpu/x86/smm.h File src/include/cpu/x86/smm.h:
https://review.coreboot.org/c/coreboot/+/43684/2/src/include/cpu/x86/smm.h@1... PS2, Line 152: #if CONFIG(X86_SMM_LOADER_VERSION2)
*poke*
Done
Eugene Myers has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: Introduce SMM module loader version 2 ......................................................................
Patch Set 5: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... File src/cpu/x86/smm/smm_module_loaderv2.c:
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 123: not enough space in SMM to setup all CPUs
A developer would have to read the Intel Software Developer's Manuals and the datasheet of the speci […]
From reading the statements on line 112, 114 and 116, the error message in line 123 is somewhat misleading. 'available' means the space left over within a given 64K where other cpus' stack/stub can be placed. If 'available' < 0, then there is a problem in the stub_size and/or ss_size as the combination of the two is simply too large even in the case of a single cpu. if '0 < available < ss_size', then cpus_in_segment should be one as you have already accounted one of the CPUs and 'available' is the space that you can fit other CPUs.
So, line 123 should inform the developer that his stub_size and ss_size (as entry_offset is normally hardware dependent) are too large for the segment.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: Introduce SMM module loader version 2 ......................................................................
Patch Set 5: Code-Review+1
(18 comments)
https://review.coreboot.org/c/coreboot/+/43684/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43684/2//COMMIT_MSG@9 PS2, Line 9: Xeon-SP a Skylake Scalable Processor can have 36 CPU threads(18 cores). Current
*poke*
Done
https://review.coreboot.org/c/coreboot/+/43684/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43684/5//COMMIT_MSG@9 PS5, Line 9: ( nit: space before `(`
https://review.coreboot.org/c/coreboot/+/43684/5//COMMIT_MSG@9 PS5, Line 9: a no `a`
https://review.coreboot.org/c/coreboot/+/43684/5//COMMIT_MSG@15 PS5, Line 15: specifc typo: specific
https://review.coreboot.org/c/coreboot/+/43684/5//COMMIT_MSG@18 PS5, Line 18: smi nit: SMI
https://review.coreboot.org/c/coreboot/+/43684/5//COMMIT_MSG@23 PS5, Line 23: must is `must` or `is`? Pick one of them, but not both 😄
https://review.coreboot.org/c/coreboot/+/43684/5//COMMIT_MSG@26 PS5, Line 26: smm nit: SMM
https://review.coreboot.org/c/coreboot/+/43684/5/Documentation/releases/core... File Documentation/releases/coreboot-4.13-relnotes.md:
https://review.coreboot.org/c/coreboot/+/43684/5/Documentation/releases/core... PS5, Line 44: trailing whitespace
https://review.coreboot.org/c/coreboot/+/43684/5/Documentation/releases/core... PS5, Line 44: an extensive : number I'd mention numbers here, e.g.: `more than 32 CPU threads`
https://review.coreboot.org/c/coreboot/+/43684/5/Documentation/releases/core... PS5, Line 50: trailing whitespace
https://review.coreboot.org/c/coreboot/+/43684/5/src/cpu/x86/mp_init.c File src/cpu/x86/mp_init.c:
https://review.coreboot.org/c/coreboot/+/43684/5/src/cpu/x86/mp_init.c@775 PS5, Line 775: cpus = 1; Instead of serializing relocation, could we use a smaller number instead?
int cpus = MIN(num_cpus, 24);
https://review.coreboot.org/c/coreboot/+/43684/5/src/cpu/x86/mp_init.c@782 PS5, Line 782: cpus This changes the number of concurrent save states for the !X86_SMM_LOADER_VERSION2 case.
https://review.coreboot.org/c/coreboot/+/43684/5/src/cpu/x86/mp_init.c@804 PS5, Line 804: CPUS nit: CPUs
https://review.coreboot.org/c/coreboot/+/43684/5/src/cpu/x86/mp_init.c@806 PS5, Line 806: cpus nit: CPUs
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... File src/cpu/x86/smm/smm_module_loaderv2.c:
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 51: unsigned int
done
Done
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 108: printk(BIOS_ERR, "%s: unable to get SMM module size\n", __func__);
No, we should not continue. Good catch. I've made the fix by adding a return 0 there.
Done
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 112: 0xFFFF
Yes, this can be confusing. It means 64K segment. […]
Ack, thank you
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 226: params->smm_main_entry_offset < stack_top) {
Done
Ack
Rocky Phagura has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: Introduce SMM module loader version 2 ......................................................................
Patch Set 5:
(14 comments)
https://review.coreboot.org/c/coreboot/+/43684/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43684/5//COMMIT_MSG@9 PS5, Line 9: a
no `a`
Done
https://review.coreboot.org/c/coreboot/+/43684/5//COMMIT_MSG@9 PS5, Line 9: (
nit: space before `(`
Done
https://review.coreboot.org/c/coreboot/+/43684/5//COMMIT_MSG@15 PS5, Line 15: specifc
typo: specific
Done
https://review.coreboot.org/c/coreboot/+/43684/5//COMMIT_MSG@18 PS5, Line 18: smi
nit: SMI
Done
https://review.coreboot.org/c/coreboot/+/43684/5//COMMIT_MSG@23 PS5, Line 23: must is
`must` or `is`? Pick one of them, but not both 😄
Done
https://review.coreboot.org/c/coreboot/+/43684/5//COMMIT_MSG@26 PS5, Line 26: smm
nit: SMM
Done
https://review.coreboot.org/c/coreboot/+/43684/5/Documentation/releases/core... File Documentation/releases/coreboot-4.13-relnotes.md:
https://review.coreboot.org/c/coreboot/+/43684/5/Documentation/releases/core... PS5, Line 44:
trailing whitespace
Done
https://review.coreboot.org/c/coreboot/+/43684/5/Documentation/releases/core... PS5, Line 44: an extensive : number
I'd mention numbers here, e.g. […]
Done
https://review.coreboot.org/c/coreboot/+/43684/5/Documentation/releases/core... PS5, Line 50:
trailing whitespace
Done
https://review.coreboot.org/c/coreboot/+/43684/5/src/cpu/x86/mp_init.c File src/cpu/x86/mp_init.c:
https://review.coreboot.org/c/coreboot/+/43684/5/src/cpu/x86/mp_init.c@775 PS5, Line 775: cpus = 1;
Instead of serializing relocation, could we use a smaller number instead? […]
This would be a performance and optimization setting. SMM is already very difficult to debug. How about we save this for a future patch?
https://review.coreboot.org/c/coreboot/+/43684/5/src/cpu/x86/mp_init.c@782 PS5, Line 782: cpus
This changes the number of concurrent save states for the !X86_SMM_LOADER_VERSION2 case.
It shouldn't unless I missed something. On line 794: int cpus = num_cpus;
will take care of it for the older loader version.
https://review.coreboot.org/c/coreboot/+/43684/5/src/cpu/x86/mp_init.c@804 PS5, Line 804: CPUS
nit: CPUs
Done
https://review.coreboot.org/c/coreboot/+/43684/5/src/cpu/x86/mp_init.c@806 PS5, Line 806: cpus
nit: CPUs
Done
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... File src/cpu/x86/smm/smm_module_loaderv2.c:
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 123: not enough space in SMM to setup all CPUs
From reading the statements on line 112, 114 and 116, the error message in line 123 is somewhat misl […]
Done. Clarification added.
Hello build bot (Jenkins), Patrick Georgi, Furquan Shaikh, Martin Roth, Patrick Rudolph, Eugene Myers, Jonathan Zhang, David Hendricks, Stefan Reinauer, Angel Pons, Aaron Durbin, Eugene Myers,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43684
to look at the new patch set (#6).
Change subject: cpu/x86/smm: Introduce SMM module loader version 2 ......................................................................
cpu/x86/smm: Introduce SMM module loader version 2
Xeon-SP Skylake Scalable Processor can have 36 CPU threads (18 cores). Current coreboot SMM is unable to handle more than ~32 CPU threads. This patch introduces a version 2 of the SMM module loader which addresses this problem. Having two versions of the SMM module loader prevents any issues to current projects. Future Xeon-SP products will be using this version of the SMM loader. Subsequent patches will enable board specific functionality for Xeon-SP.
The reason for moving to version 2 is the state save area begins to encroach upon the SMI handling code when more than 32 CPU threads are in the system. This can cause system hangs, reboots, etc. The second change is related to staggered entry points with simple near jumps. In the current loader, near jumps will not work because the CPU is jumping within the same code segment. In version 2, "far" address jumps are necessary therefore protected mode must be enabled first. The SMM layout and how the CPUs are staggered are documented in the code.
By making the modifications above, this allows the smm module loader to expand easily as more CPU threads are added.
TEST=build for Tiogapass platform under OCP mainboard. Enable the following in Kconfig. select CPU_INTEL_COMMON_SMM select SOC_INTEL_COMMON_BLOCK_SMM select SMM_TSEG select HAVE_SMI_HANDLER select ACPI_INTEL_HARDWARE_SLEEP_VALUES
Debug console will show all 36 cores relocated. Further tested by generating SMI's to port 0xb2 using XDP/ITP HW debugger and ensured all cores entering and exiting SMM properly. In addition, booted to Linux 5.4 kernel and observed no issues during mp init.
Change-Id: I00a23a5f2a46110536c344254868390dbb71854c Signed-off-by: Rocky Phagura rphagura@fb.com --- M Documentation/releases/coreboot-4.13-relnotes.md M src/cpu/x86/Kconfig M src/cpu/x86/mp_init.c M src/cpu/x86/smm/Makefile.inc A src/cpu/x86/smm/smm_module_loaderv2.c M src/include/cpu/x86/smm.h 6 files changed, 726 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/43684/6
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: Introduce SMM module loader version 2 ......................................................................
Patch Set 6: Code-Review+1
(3 comments)
https://review.coreboot.org/c/coreboot/+/43684/5/src/cpu/x86/mp_init.c File src/cpu/x86/mp_init.c:
https://review.coreboot.org/c/coreboot/+/43684/5/src/cpu/x86/mp_init.c@775 PS5, Line 775: cpus = 1;
This would be a performance and optimization setting. SMM is already very difficult to debug. […]
Sounds good
https://review.coreboot.org/c/coreboot/+/43684/5/src/cpu/x86/mp_init.c@782 PS5, Line 782: cpus
It shouldn't unless I missed something. On line 794: […]
The original setting is the following:
.num_concurrent_save_states = 1,
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... File src/cpu/x86/smm/smm_module_loaderv2.c:
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 15: uint8_t
This is same as previous loader, so I didn't change anything. I left it as is.
What I mean is that since you're using `uint8_t` and friends, you should include <stdint.h> in this file
Rocky Phagura has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: Introduce SMM module loader version 2 ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43684/5/src/cpu/x86/mp_init.c File src/cpu/x86/mp_init.c:
https://review.coreboot.org/c/coreboot/+/43684/5/src/cpu/x86/mp_init.c@782 PS5, Line 782: cpus
The original setting is the following: […]
In the original SMM loader, this is set to num_cpus. In my first patch of SMM version loader 2, I set this to 1. As we discussed, for platforms with less than 32 CPU threads, they can all relocate at the same time without problems. So now I added an #if condition and if its loader version 2 and only then do we want to relocate each CPU one by one. So this helps with backward comptability. Let me know if you need more info on this. Here is a snippet from the original smm module loader.
struct smm_loader_params smm_params = { .per_cpu_stack_size = CONFIG_SMM_STUB_STACK_SIZE, .num_concurrent_stacks = num_cpus, .per_cpu_save_state_size = save_state_size, .num_concurrent_save_states = 1, .handler = smm_do_relocation, };
Hello build bot (Jenkins), Patrick Georgi, Furquan Shaikh, Martin Roth, Patrick Rudolph, Eugene Myers, Jonathan Zhang, David Hendricks, Stefan Reinauer, Angel Pons, Aaron Durbin, Eugene Myers,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43684
to look at the new patch set (#7).
Change subject: cpu/x86/smm: Introduce SMM module loader version 2 ......................................................................
cpu/x86/smm: Introduce SMM module loader version 2
Xeon-SP Skylake Scalable Processor can have 36 CPU threads (18 cores). Current coreboot SMM is unable to handle more than ~32 CPU threads. This patch introduces a version 2 of the SMM module loader which addresses this problem. Having two versions of the SMM module loader prevents any issues to current projects. Future Xeon-SP products will be using this version of the SMM loader. Subsequent patches will enable board specific functionality for Xeon-SP.
The reason for moving to version 2 is the state save area begins to encroach upon the SMI handling code when more than 32 CPU threads are in the system. This can cause system hangs, reboots, etc. The second change is related to staggered entry points with simple near jumps. In the current loader, near jumps will not work because the CPU is jumping within the same code segment. In version 2, "far" address jumps are necessary therefore protected mode must be enabled first. The SMM layout and how the CPUs are staggered are documented in the code.
By making the modifications above, this allows the smm module loader to expand easily as more CPU threads are added.
TEST=build for Tiogapass platform under OCP mainboard. Enable the following in Kconfig. select CPU_INTEL_COMMON_SMM select SOC_INTEL_COMMON_BLOCK_SMM select SMM_TSEG select HAVE_SMI_HANDLER select ACPI_INTEL_HARDWARE_SLEEP_VALUES
Debug console will show all 36 cores relocated. Further tested by generating SMI's to port 0xb2 using XDP/ITP HW debugger and ensured all cores entering and exiting SMM properly. In addition, booted to Linux 5.4 kernel and observed no issues during mp init.
Change-Id: I00a23a5f2a46110536c344254868390dbb71854c Signed-off-by: Rocky Phagura rphagura@fb.com --- M Documentation/releases/coreboot-4.13-relnotes.md M src/cpu/x86/Kconfig M src/cpu/x86/mp_init.c M src/cpu/x86/smm/Makefile.inc A src/cpu/x86/smm/smm_module_loaderv2.c M src/include/cpu/x86/smm.h 6 files changed, 727 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/43684/7
Rocky Phagura has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: Introduce SMM module loader version 2 ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... File src/cpu/x86/smm/smm_module_loaderv2.c:
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 15: uint8_t
What I mean is that since you're using `uint8_t` and friends, you should include <stdint. […]
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: Introduce SMM module loader version 2 ......................................................................
Patch Set 7: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/43684/5/src/cpu/x86/mp_init.c File src/cpu/x86/mp_init.c:
https://review.coreboot.org/c/coreboot/+/43684/5/src/cpu/x86/mp_init.c@782 PS5, Line 782: cpus
In the original SMM loader, this is set to num_cpus. […]
I see that. What I see is that `num_concurrent_save_states` was 1, but now it will be `num_cpus` for the original allocator configuration.
I think we might be talking past each other. In short, what I'd like is to change this:
.num_concurrent_save_states = cpus,
Back to what it originally was:
.num_concurrent_save_states = 1,
Rocky Phagura has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: Introduce SMM module loader version 2 ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43684/5/src/cpu/x86/mp_init.c File src/cpu/x86/mp_init.c:
https://review.coreboot.org/c/coreboot/+/43684/5/src/cpu/x86/mp_init.c@782 PS5, Line 782: cpus
I see that. […]
Done...
Hello build bot (Jenkins), Patrick Georgi, Furquan Shaikh, Martin Roth, Patrick Rudolph, Eugene Myers, Jonathan Zhang, David Hendricks, Stefan Reinauer, Angel Pons, Aaron Durbin, Eugene Myers,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43684
to look at the new patch set (#8).
Change subject: cpu/x86/smm: Introduce SMM module loader version 2 ......................................................................
cpu/x86/smm: Introduce SMM module loader version 2
Xeon-SP Skylake Scalable Processor can have 36 CPU threads (18 cores). Current coreboot SMM is unable to handle more than ~32 CPU threads. This patch introduces a version 2 of the SMM module loader which addresses this problem. Having two versions of the SMM module loader prevents any issues to current projects. Future Xeon-SP products will be using this version of the SMM loader. Subsequent patches will enable board specific functionality for Xeon-SP.
The reason for moving to version 2 is the state save area begins to encroach upon the SMI handling code when more than 32 CPU threads are in the system. This can cause system hangs, reboots, etc. The second change is related to staggered entry points with simple near jumps. In the current loader, near jumps will not work because the CPU is jumping within the same code segment. In version 2, "far" address jumps are necessary therefore protected mode must be enabled first. The SMM layout and how the CPUs are staggered are documented in the code.
By making the modifications above, this allows the smm module loader to expand easily as more CPU threads are added.
TEST=build for Tiogapass platform under OCP mainboard. Enable the following in Kconfig. select CPU_INTEL_COMMON_SMM select SOC_INTEL_COMMON_BLOCK_SMM select SMM_TSEG select HAVE_SMI_HANDLER select ACPI_INTEL_HARDWARE_SLEEP_VALUES
Debug console will show all 36 cores relocated. Further tested by generating SMI's to port 0xb2 using XDP/ITP HW debugger and ensured all cores entering and exiting SMM properly. In addition, booted to Linux 5.4 kernel and observed no issues during mp init.
Change-Id: I00a23a5f2a46110536c344254868390dbb71854c Signed-off-by: Rocky Phagura rphagura@fb.com --- M Documentation/releases/coreboot-4.13-relnotes.md M src/cpu/x86/Kconfig M src/cpu/x86/mp_init.c M src/cpu/x86/smm/Makefile.inc A src/cpu/x86/smm/smm_module_loaderv2.c M src/include/cpu/x86/smm.h 6 files changed, 727 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/43684/8
Hello build bot (Jenkins), Patrick Georgi, Furquan Shaikh, Martin Roth, Patrick Rudolph, Eugene Myers, Jonathan Zhang, David Hendricks, Stefan Reinauer, Angel Pons, Aaron Durbin, Eugene Myers,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43684
to look at the new patch set (#9).
Change subject: cpu/x86/smm: Introduce SMM module loader version 2 ......................................................................
cpu/x86/smm: Introduce SMM module loader version 2
Xeon-SP Skylake Scalable Processor can have 36 CPU threads (18 cores). Current coreboot SMM is unable to handle more than ~32 CPU threads. This patch introduces a version 2 of the SMM module loader which addresses this problem. Having two versions of the SMM module loader prevents any issues to current projects. Future Xeon-SP products will be using this version of the SMM loader. Subsequent patches will enable board specific functionality for Xeon-SP.
The reason for moving to version 2 is the state save area begins to encroach upon the SMI handling code when more than 32 CPU threads are in the system. This can cause system hangs, reboots, etc. The second change is related to staggered entry points with simple near jumps. In the current loader, near jumps will not work because the CPU is jumping within the same code segment. In version 2, "far" address jumps are necessary therefore protected mode must be enabled first. The SMM layout and how the CPUs are staggered are documented in the code.
By making the modifications above, this allows the smm module loader to expand easily as more CPU threads are added.
TEST=build for Tiogapass platform under OCP mainboard. Enable the following in Kconfig. select CPU_INTEL_COMMON_SMM select SOC_INTEL_COMMON_BLOCK_SMM select SMM_TSEG select HAVE_SMI_HANDLER select ACPI_INTEL_HARDWARE_SLEEP_VALUES
Debug console will show all 36 cores relocated. Further tested by generating SMI's to port 0xb2 using XDP/ITP HW debugger and ensured all cores entering and exiting SMM properly. In addition, booted to Linux 5.4 kernel and observed no issues during mp init.
Change-Id: I00a23a5f2a46110536c344254868390dbb71854c Signed-off-by: Rocky Phagura rphagura@fb.com --- M Documentation/releases/coreboot-4.13-relnotes.md M src/cpu/x86/Kconfig M src/cpu/x86/mp_init.c M src/cpu/x86/smm/Makefile.inc A src/cpu/x86/smm/smm_module_loaderv2.c M src/include/cpu/x86/smm.h 6 files changed, 726 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/43684/9
Hello build bot (Jenkins), Patrick Georgi, Furquan Shaikh, Martin Roth, Patrick Rudolph, Eugene Myers, Jonathan Zhang, David Hendricks, Stefan Reinauer, Angel Pons, Aaron Durbin, Eugene Myers,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43684
to look at the new patch set (#10).
Change subject: cpu/x86/smm: Introduce SMM module loader version 2 ......................................................................
cpu/x86/smm: Introduce SMM module loader version 2
Xeon-SP Skylake Scalable Processor can have 36 CPU threads (18 cores). Current coreboot SMM is unable to handle more than ~32 CPU threads. This patch introduces a version 2 of the SMM module loader which addresses this problem. Having two versions of the SMM module loader prevents any issues to current projects. Future Xeon-SP products will be using this version of the SMM loader. Subsequent patches will enable board specific functionality for Xeon-SP.
The reason for moving to version 2 is the state save area begins to encroach upon the SMI handling code when more than 32 CPU threads are in the system. This can cause system hangs, reboots, etc. The second change is related to staggered entry points with simple near jumps. In the current loader, near jumps will not work because the CPU is jumping within the same code segment. In version 2, "far" address jumps are necessary therefore protected mode must be enabled first. The SMM layout and how the CPUs are staggered are documented in the code.
By making the modifications above, this allows the smm module loader to expand easily as more CPU threads are added.
TEST=build for Tiogapass platform under OCP mainboard. Enable the following in Kconfig. select CPU_INTEL_COMMON_SMM select SOC_INTEL_COMMON_BLOCK_SMM select SMM_TSEG select HAVE_SMI_HANDLER select ACPI_INTEL_HARDWARE_SLEEP_VALUES
Debug console will show all 36 cores relocated. Further tested by generating SMI's to port 0xb2 using XDP/ITP HW debugger and ensured all cores entering and exiting SMM properly. In addition, booted to Linux 5.4 kernel and observed no issues during mp init.
Change-Id: I00a23a5f2a46110536c344254868390dbb71854c Signed-off-by: Rocky Phagura rphagura@fb.com --- M Documentation/releases/coreboot-4.13-relnotes.md M src/cpu/x86/Kconfig M src/cpu/x86/mp_init.c M src/cpu/x86/smm/Makefile.inc A src/cpu/x86/smm/smm_module_loaderv2.c M src/include/cpu/x86/smm.h 6 files changed, 726 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/43684/10
Rocky Phagura has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: Introduce SMM module loader version 2 ......................................................................
Patch Set 10:
Not sure, why its failing but then showing SUCCESS.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: Introduce SMM module loader version 2 ......................................................................
Patch Set 10: Code-Review+1
Patch Set 10:
Not sure, why its failing but then showing SUCCESS.
It says ABORTED, which happens when you upload a new patchset before it gets build-tested.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: Introduce SMM module loader version 2 ......................................................................
Patch Set 10:
(1 comment)
I'll give it a try on Asrock B85M Pro4 now just in case.
https://review.coreboot.org/c/coreboot/+/43684/5/src/cpu/x86/mp_init.c File src/cpu/x86/mp_init.c:
https://review.coreboot.org/c/coreboot/+/43684/5/src/cpu/x86/mp_init.c@782 PS5, Line 782: cpus
Done...
Thank you!
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: Introduce SMM module loader version 2 ......................................................................
Patch Set 10: Code-Review+2
Boots!
Rocky Phagura has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: Introduce SMM module loader version 2 ......................................................................
Patch Set 10:
Awesome. Thank you for your help and everyone else who provided feedback!
David Hendricks has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: Introduce SMM module loader version 2 ......................................................................
cpu/x86/smm: Introduce SMM module loader version 2
Xeon-SP Skylake Scalable Processor can have 36 CPU threads (18 cores). Current coreboot SMM is unable to handle more than ~32 CPU threads. This patch introduces a version 2 of the SMM module loader which addresses this problem. Having two versions of the SMM module loader prevents any issues to current projects. Future Xeon-SP products will be using this version of the SMM loader. Subsequent patches will enable board specific functionality for Xeon-SP.
The reason for moving to version 2 is the state save area begins to encroach upon the SMI handling code when more than 32 CPU threads are in the system. This can cause system hangs, reboots, etc. The second change is related to staggered entry points with simple near jumps. In the current loader, near jumps will not work because the CPU is jumping within the same code segment. In version 2, "far" address jumps are necessary therefore protected mode must be enabled first. The SMM layout and how the CPUs are staggered are documented in the code.
By making the modifications above, this allows the smm module loader to expand easily as more CPU threads are added.
TEST=build for Tiogapass platform under OCP mainboard. Enable the following in Kconfig. select CPU_INTEL_COMMON_SMM select SOC_INTEL_COMMON_BLOCK_SMM select SMM_TSEG select HAVE_SMI_HANDLER select ACPI_INTEL_HARDWARE_SLEEP_VALUES
Debug console will show all 36 cores relocated. Further tested by generating SMI's to port 0xb2 using XDP/ITP HW debugger and ensured all cores entering and exiting SMM properly. In addition, booted to Linux 5.4 kernel and observed no issues during mp init.
Change-Id: I00a23a5f2a46110536c344254868390dbb71854c Signed-off-by: Rocky Phagura rphagura@fb.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/43684 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M Documentation/releases/coreboot-4.13-relnotes.md M src/cpu/x86/Kconfig M src/cpu/x86/mp_init.c M src/cpu/x86/smm/Makefile.inc A src/cpu/x86/smm/smm_module_loaderv2.c M src/include/cpu/x86/smm.h 6 files changed, 726 insertions(+), 8 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/Documentation/releases/coreboot-4.13-relnotes.md b/Documentation/releases/coreboot-4.13-relnotes.md index 2910867..dcc8bf4 100644 --- a/Documentation/releases/coreboot-4.13-relnotes.md +++ b/Documentation/releases/coreboot-4.13-relnotes.md @@ -39,4 +39,14 @@ the platforms. More details about the tools are added in [README.md](https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/uti...).
+### New version of SMM loader + +A new version of the SMM loader which accomodates platforms with over 32 CPU +CPU threads. The existing version of SMM loader uses a 64K code/data +segment and only a limited number of CPU threads can fit into one segment +(because of save state, STM, other features, etc). This loader extends beyond +the 64K segment to accomodate additional CPUs and in theory allows as many +CPU threads as possible limited only by SMRAM space and not by 64K. By default +this loader version is disabled. Please see cpu/x86/Kconfig for more info. + ### Add significant changes here diff --git a/src/cpu/x86/Kconfig b/src/cpu/x86/Kconfig index 5394cd0..b3a16bc 100644 --- a/src/cpu/x86/Kconfig +++ b/src/cpu/x86/Kconfig @@ -121,6 +121,14 @@
endif
+config X86_SMM_LOADER_VERSION2 + bool + default n + depends on HAVE_SMI_HANDLER + help + This option enables SMM module loader that works with server + platforms which may contain more than 32 CPU threads. + config SMM_LAPIC_REMAP_MITIGATION bool default y if NORTHBRIDGE_INTEL_I945 diff --git a/src/cpu/x86/mp_init.c b/src/cpu/x86/mp_init.c index caed8f4..5807831 100644 --- a/src/cpu/x86/mp_init.c +++ b/src/cpu/x86/mp_init.c @@ -726,12 +726,21 @@ * the location of the new SMBASE. If using SMM modules then this * calculation needs to match that of the module loader. */ +#if CONFIG(X86_SMM_LOADER_VERSION2) + perm_smbase = smm_get_cpu_smbase(cpu); + mp_state.perm_smbase = perm_smbase; + if (!perm_smbase) { + printk(BIOS_ERR, "%s: bad SMBASE for CPU %d\n", __func__, cpu); + return; + } +#else perm_smbase = mp_state.perm_smbase; perm_smbase -= cpu * runtime->save_state_size; - - printk(BIOS_DEBUG, "New SMBASE 0x%08lx\n", perm_smbase); +#endif
/* Setup code checks this callback for validity. */ + printk(BIOS_INFO, "%s : curr_smbase 0x%x perm_smbase 0x%x, cpu = %d\n", + __func__, (int)curr_smbase, (int)perm_smbase, cpu); mp_state.ops.relocation_handler(cpu, curr_smbase, perm_smbase);
if (CONFIG(STM)) { @@ -758,9 +767,17 @@
static int install_relocation_handler(int num_cpus, size_t save_state_size) { + int cpus = num_cpus; +#if CONFIG(X86_SMM_LOADER_VERSION2) + /* Default SMRAM size is not big enough to concurrently + * handle relocation for more than ~32 CPU threads + * therefore, relocate 1 by 1. */ + cpus = 1; +#endif + struct smm_loader_params smm_params = { .per_cpu_stack_size = CONFIG_SMM_STUB_STACK_SIZE, - .num_concurrent_stacks = num_cpus, + .num_concurrent_stacks = cpus, .per_cpu_save_state_size = save_state_size, .num_concurrent_save_states = 1, .handler = smm_do_relocation, @@ -770,9 +787,10 @@ if (mp_state.ops.adjust_smm_params != NULL) mp_state.ops.adjust_smm_params(&smm_params, 0);
- if (smm_setup_relocation_handler(&smm_params)) + if (smm_setup_relocation_handler(&smm_params)) { + printk(BIOS_ERR, "%s: smm setup failed\n", __func__); return -1; - + } adjust_smm_apic_id_map(&smm_params);
return 0; @@ -781,8 +799,13 @@ static int install_permanent_handler(int num_cpus, uintptr_t smbase, size_t smsize, size_t save_state_size) { - /* There are num_cpus concurrent stacks and num_cpus concurrent save - * state areas. Lastly, set the stack size to 1KiB. */ + /* + * All the CPUs will relocate to permanaent handler now. Set parameters + * needed for all CPUs. The placement of each CPUs entry point is + * determined by the loader. This code simply provides the beginning of + * SMRAM region, the number of CPUs who will use the handler, the stack + * size and save state size for each CPU. + */ struct smm_loader_params smm_params = { .per_cpu_stack_size = CONFIG_SMM_MODULE_STACK_SIZE, .num_concurrent_stacks = num_cpus, @@ -794,7 +817,7 @@ if (mp_state.ops.adjust_smm_params != NULL) mp_state.ops.adjust_smm_params(&smm_params, 1);
- printk(BIOS_DEBUG, "Installing SMM handler to 0x%08lx\n", smbase); + printk(BIOS_DEBUG, "Installing permanent SMM handler to 0x%08lx\n", smbase);
if (smm_load_module((void *)smbase, smsize, &smm_params)) return -1; diff --git a/src/cpu/x86/smm/Makefile.inc b/src/cpu/x86/smm/Makefile.inc index dbe567a..1273a6c 100644 --- a/src/cpu/x86/smm/Makefile.inc +++ b/src/cpu/x86/smm/Makefile.inc @@ -1,6 +1,10 @@ ## SPDX-License-Identifier: GPL-2.0-only
+ifeq ($(CONFIG_X86_SMM_LOADER_VERSION2),y) +ramstage-y += smm_module_loaderv2.c +else ramstage-y += smm_module_loader.c +endif ramstage-y += smi_trigger.c
ifeq ($(CONFIG_ARCH_RAMSTAGE_X86_32),y) diff --git a/src/cpu/x86/smm/smm_module_loaderv2.c b/src/cpu/x86/smm/smm_module_loaderv2.c new file mode 100644 index 0000000..10cc628 --- /dev/null +++ b/src/cpu/x86/smm/smm_module_loaderv2.c @@ -0,0 +1,655 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <stdint.h> +#include <string.h> +#include <rmodule.h> +#include <cpu/x86/smm.h> +#include <commonlib/helpers.h> +#include <console/console.h> +#include <security/intel/stm/SmmStm.h> + +#define FXSAVE_SIZE 512 +#define SMM_CODE_SEGMENT_SIZE 0x10000 +/* FXSAVE area during relocation. While it may not be strictly needed the + SMM stub code relies on the FXSAVE area being non-zero to enable SSE + instructions within SMM mode. */ +static uint8_t fxsave_area_relocation[CONFIG_MAX_CPUS][FXSAVE_SIZE] +__attribute__((aligned(16))); + +/* + * Components that make up the SMRAM: + * 1. Save state - the total save state memory used + * 2. Stack - stacks for the CPUs in the SMM handler + * 3. Stub - SMM stub code for calling into handler + * 4. Handler - C-based SMM handler. + * + * The components are assumed to consist of one consecutive region. + */ + +/* These parameters are used by the SMM stub code. A pointer to the params + * is also passed to the C-base handler. */ +struct smm_stub_params { + u32 stack_size; + u32 stack_top; + u32 c_handler; + u32 c_handler_arg; + u32 fxsave_area; + u32 fxsave_area_size; + struct smm_runtime runtime; +} __packed; + +/* + * The stub is the entry point that sets up protected mode and stacks for each + * CPU. It then calls into the SMM handler module. It is encoded as an rmodule. + */ +extern unsigned char _binary_smmstub_start[]; + +/* Per CPU minimum stack size. */ +#define SMM_MINIMUM_STACK_SIZE 32 + +struct cpu_smm_info { + uint8_t active; + uintptr_t smbase; + uintptr_t entry; + uintptr_t ss_start; + uintptr_t code_start; + uintptr_t code_end; +}; +struct cpu_smm_info cpus[CONFIG_MAX_CPUS] = { 0 }; + +/* + * This method creates a map of all the CPU entry points, save state locations + * and the beginning and end of code segments for each CPU. This map is used + * during relocation to properly align as many CPUs that can fit into the SMRAM + * region. For more information on how SMRAM works, refer to the latest Intel + * developer's manuals (volume 3, chapter 34). SMRAM is divided up into the + * following regions: + * +-----------------+ Top of SMRAM + * | | <- MSEG, FXSAVE + * +-----------------+ + * | common | + * | smi handler | 64K + * | | + * +-----------------+ + * | CPU 0 code seg | + * +-----------------+ + * | CPU 1 code seg | + * +-----------------+ + * | CPU x code seg | + * +-----------------+ + * | | + * | | + * +-----------------+ + * | stacks | + * +-----------------+ <- START of SMRAM + * + * The code below checks when a code segment is full and begins placing the remainder + * CPUs in the lower segments. The entry point for each CPU is smbase + 0x8000 + * and save state is smbase + 0x8000 + (0x8000 - state save size). Save state + * area grows downward into the CPUs entry point. Therefore staggering too many + * CPUs in one 32K block will corrupt CPU0's entry code as the save states move + * downward. + * input : smbase of first CPU (all other CPUs + * will go below this address) + * input : num_cpus in the system. The map will + * be created from 0 to num_cpus. + */ +static int smm_create_map(uintptr_t smbase, unsigned int num_cpus, + const struct smm_loader_params *params) +{ + unsigned int i; + struct rmodule smm_stub; + unsigned int ss_size = params->per_cpu_save_state_size, stub_size; + unsigned int smm_entry_offset = params->smm_main_entry_offset; + unsigned int seg_count = 0, segments = 0, available; + unsigned int cpus_in_segment = 0; + unsigned int base = smbase; + + if (rmodule_parse(&_binary_smmstub_start, &smm_stub)) { + printk(BIOS_ERR, "%s: unable to get SMM module size\n", __func__); + return 0; + } + + stub_size = rmodule_memory_size(&smm_stub); + /* How many CPUs can fit into one 64K segment? */ + available = 0xFFFF - smm_entry_offset - ss_size - stub_size; + if (available > 0) { + cpus_in_segment = available / ss_size; + /* minimum segments needed will always be 1 */ + segments = num_cpus / cpus_in_segment + 1; + printk(BIOS_DEBUG, + "%s: cpus allowed in one segment %d\n", __func__, cpus_in_segment); + printk(BIOS_DEBUG, + "%s: min # of segments needed %d\n", __func__, segments); + } else { + printk(BIOS_ERR, "%s: not enough space in SMM to setup all CPUs\n", __func__); + printk(BIOS_ERR, " save state & stub size need to be reduced\n"); + printk(BIOS_ERR, " or increase SMRAM size\n"); + return 0; + } + + if (sizeof(cpus) / sizeof(struct cpu_smm_info) < num_cpus) { + printk(BIOS_ERR, + "%s: increase MAX_CPUS in Kconfig\n", __func__); + return 0; + } + + for (i = 0; i < num_cpus; i++) { + cpus[i].smbase = base; + cpus[i].entry = base + smm_entry_offset; + cpus[i].ss_start = cpus[i].entry + (smm_entry_offset - ss_size); + cpus[i].code_start = cpus[i].entry; + cpus[i].code_end = cpus[i].entry + stub_size; + cpus[i].active = 1; + base -= ss_size; + seg_count++; + if (seg_count >= cpus_in_segment) { + base -= smm_entry_offset; + seg_count = 0; + } + } + + if (CONFIG_DEFAULT_CONSOLE_LOGLEVEL >= BIOS_DEBUG) { + seg_count = 0; + for (i = 0; i < num_cpus; i++) { + printk(BIOS_DEBUG, "CPU 0x%x\n", i); + printk(BIOS_DEBUG, + " smbase %zx entry %zx\n", + cpus[i].smbase, cpus[i].entry); + printk(BIOS_DEBUG, + " ss_start %zx code_end %zx\n", + cpus[i].ss_start, cpus[i].code_end); + seg_count++; + if (seg_count >= cpus_in_segment) { + printk(BIOS_DEBUG, + "-------------NEW CODE SEGMENT --------------\n"); + seg_count = 0; + } + } + } + return 1; +} + +/* + * This method expects the smm relocation map to be complete. + * This method does not read any HW registers, it simply uses a + * map that was created during SMM setup. + * input: cpu_num - cpu number which is used as an index into the + * map to return the smbase + */ +u32 smm_get_cpu_smbase(unsigned int cpu_num) +{ + if (cpu_num < CONFIG_MAX_CPUS) { + if (cpus[cpu_num].active) + return cpus[cpu_num].smbase; + } + return 0; +} + +/* + * This method assumes that at least 1 CPU has been set up from + * which it will place other CPUs below its smbase ensuring that + * save state does not clobber the first CPUs init code segment. The init + * code which is the smm stub code is the same for all CPUs. They enter + * smm, setup stacks (based on their apic id), enter protected mode + * and then jump to the common smi handler. The stack is allocated + * at the beginning of smram (aka tseg base, not smbase). The stack + * pointer for each CPU is calculated by using its apic id + * (code is in smm_stub.s) + * Each entry point will now have the same stub code which, sets up the CPU + * stack, enters protected mode and then jumps to the smi handler. It is + * important to enter protected mode before the jump because the "jump to + * address" might be larger than the 20bit address supported by real mode. + * SMI entry right now is in real mode. + * input: smbase - this is the smbase of the first cpu not the smbase + * where tseg starts (aka smram_start). All CPUs code segment + * and stack will be below this point except for the common + * SMI handler which is one segment above + * input: num_cpus - number of cpus that need relocation including + * the first CPU (though its code is already loaded) + * input: top of stack (stacks work downward by default in Intel HW) + * output: return -1, if runtime smi code could not be installed. In + * this case SMM will not work and any SMI's generated will + * cause a CPU shutdown or general protection fault because + * the appropriate smi handling code was not installed + */ + +static int smm_place_entry_code(uintptr_t smbase, unsigned int num_cpus, + unsigned int stack_top, const struct smm_loader_params *params) +{ + unsigned int i; + unsigned int size; + if (smm_create_map(smbase, num_cpus, params)) { + /* + * Ensure there was enough space and the last CPUs smbase + * did not encroach upon the stack. Stack top is smram start + * + size of stack. + */ + if (cpus[num_cpus].active) { + if (cpus[num_cpus - 1].smbase + + params->smm_main_entry_offset < stack_top) { + printk(BIOS_ERR, "%s: stack encroachment\n", __func__); + printk(BIOS_ERR, "%s: smbase %zx, stack_top %x\n", + __func__, cpus[num_cpus].smbase, stack_top); + return 0; + } + } + } else { + printk(BIOS_ERR, "%s: unable to place smm entry code\n", __func__); + return 0; + } + + printk(BIOS_INFO, "%s: smbase %zx, stack_top %x\n", + __func__, cpus[num_cpus-1].smbase, stack_top); + + /* start at 1, the first CPU stub code is already there */ + size = cpus[0].code_end - cpus[0].code_start; + for (i = 1; i < num_cpus; i++) { + memcpy((int *)cpus[i].code_start, (int *)cpus[0].code_start, size); + printk(BIOS_DEBUG, + "SMM Module: placing smm entry code at %zx, cpu # 0x%x\n", + cpus[i].code_start, i); + printk(BIOS_DEBUG, "%s: copying from %zx to %zx 0x%x bytes\n", + __func__, cpus[0].code_start, cpus[i].code_start, size); + } + return 1; +} + +/* + * Place stacks in base -> base + size region, but ensure the stacks don't + * overlap the staggered entry points. + */ +static void *smm_stub_place_stacks(char *base, size_t size, + struct smm_loader_params *params) +{ + size_t total_stack_size; + char *stacks_top; + + /* If stack space is requested assume the space lives in the lower + * half of SMRAM. */ + total_stack_size = params->per_cpu_stack_size * + params->num_concurrent_stacks; + printk(BIOS_DEBUG, "%s: cpus: %zx : stack space: needed -> %zx\n", + __func__, params->num_concurrent_stacks, + total_stack_size); + printk(BIOS_DEBUG, " available -> %zx : per_cpu_stack_size : %zx\n", + size, params->per_cpu_stack_size); + + /* There has to be at least one stack user. */ + if (params->num_concurrent_stacks < 1) + return NULL; + + /* Total stack size cannot fit. */ + if (total_stack_size > size) + return NULL; + + /* Stacks extend down to SMBASE */ + stacks_top = &base[total_stack_size]; + printk(BIOS_DEBUG, "%s: exit, stack_top %p\n", __func__, stacks_top); + + return stacks_top; +} + +/* + * Place the staggered entry points for each CPU. The entry points are + * staggered by the per CPU SMM save state size extending down from + * SMM_ENTRY_OFFSET. + */ +static int smm_stub_place_staggered_entry_points(char *base, + const struct smm_loader_params *params, const struct rmodule *smm_stub) +{ + size_t stub_entry_offset; + int rc = 1; + stub_entry_offset = rmodule_entry_offset(smm_stub); + /* Each CPU now has its own stub code, which enters protected mode, + * sets up the stack, and then jumps to common SMI handler + */ + if (params->num_concurrent_save_states > 1 || stub_entry_offset != 0) { + rc = smm_place_entry_code((unsigned int)base, + params->num_concurrent_save_states, + (unsigned int)params->stack_top, params); + } + return rc; +} + +/* + * The stub setup code assumes it is completely contained within the + * default SMRAM size (0x10000) for the default SMI handler (entry at + * 0x30000), but no assumption should be made for the permanent SMI handler. + * The placement of CPU entry points for permanent handler are determined + * by the number of CPUs in the system and the amount of SMRAM. + * There are potentially 3 regions to place + * within the default SMRAM size: + * 1. Save state areas + * 2. Stub code + * 3. Stack areas + * + * The save state and smm stack are treated as contiguous for the number of + * concurrent areas requested. The save state always lives at the top of the + * the CPUS smbase (and the entry point is at offset 0x8000). This allows only a certain + * number of CPUs with staggered entry points until the save state area comes + * down far enough to overwrite/corrupt the entry code (stub code). Therefore, + * an SMM map is created to avoid this corruption, see smm_create_map() above. + * This module setup code works for the default (0x30000) SMM handler setup and the + * permanent SMM handler. + */ +static int smm_module_setup_stub(void *smbase, size_t smm_size, + struct smm_loader_params *params, + void *fxsave_area) +{ + size_t total_save_state_size; + size_t smm_stub_size; + size_t stub_entry_offset; + char *smm_stub_loc; + void *stacks_top; + size_t size; + char *base; + size_t i; + struct smm_stub_params *stub_params; + struct rmodule smm_stub; + unsigned int total_size_all; + base = smbase; + size = smm_size; + + /* The number of concurrent stacks cannot exceed CONFIG_MAX_CPUS. */ + if (params->num_concurrent_stacks > CONFIG_MAX_CPUS) { + printk(BIOS_ERR, "%s: not enough stacks\n", __func__); + return -1; + } + + /* Fail if can't parse the smm stub rmodule. */ + if (rmodule_parse(&_binary_smmstub_start, &smm_stub)) { + printk(BIOS_ERR, "%s: unable to parse smm stub\n", __func__); + return -1; + } + + /* Adjust remaining size to account for save state. */ + total_save_state_size = params->per_cpu_save_state_size * + params->num_concurrent_save_states; + if (total_save_state_size > size) { + printk(BIOS_ERR, + "%s: more state save space needed:need -> %zx:available->%zx\n", + __func__, total_save_state_size, size); + return -1; + } + + size -= total_save_state_size; + + /* The save state size encroached over the first SMM entry point. */ + if (size <= params->smm_main_entry_offset) { + printk(BIOS_ERR, "%s: encroachment over SMM entry point\n", __func__); + printk(BIOS_ERR, "%s: state save size: %zx : smm_entry_offset -> %x\n", + __func__, size, params->smm_main_entry_offset); + return -1; + } + + /* Need a minimum stack size and alignment. */ + if (params->per_cpu_stack_size <= SMM_MINIMUM_STACK_SIZE || + (params->per_cpu_stack_size & 3) != 0) { + printk(BIOS_ERR, "%s: need minimum stack size\n", __func__); + return -1; + } + + smm_stub_loc = NULL; + smm_stub_size = rmodule_memory_size(&smm_stub); + stub_entry_offset = rmodule_entry_offset(&smm_stub); + + /* Put the stub at the main entry point */ + smm_stub_loc = &base[params->smm_main_entry_offset]; + + /* Stub is too big to fit. */ + if (smm_stub_size > (size - params->smm_main_entry_offset)) { + printk(BIOS_ERR, "%s: stub is too big to fit\n", __func__); + return -1; + } + + /* The stacks, if requested, live in the lower half of SMRAM space + * for default handler, but for relocated handler it lives at the beginning + * of SMRAM which is TSEG base + */ + size = params->num_concurrent_stacks * params->per_cpu_stack_size; + stacks_top = smm_stub_place_stacks((char *)params->smram_start, size, params); + if (stacks_top == NULL) { + printk(BIOS_ERR, "%s: not enough space for stacks\n", __func__); + printk(BIOS_ERR, "%s: ....need -> %p : available -> %zx\n", __func__, + base, size); + return -1; + } + params->stack_top = stacks_top; + /* Load the stub. */ + if (rmodule_load(smm_stub_loc, &smm_stub)) { + printk(BIOS_ERR, "%s: load module failed\n", __func__); + return -1; + } + + if (!smm_stub_place_staggered_entry_points(base, params, &smm_stub)) { + printk(BIOS_ERR, "%s: staggered entry points failed\n", __func__); + return -1; + } + + /* Setup the parameters for the stub code. */ + stub_params = rmodule_parameters(&smm_stub); + stub_params->stack_top = (uintptr_t)stacks_top; + stub_params->stack_size = params->per_cpu_stack_size; + stub_params->c_handler = (uintptr_t)params->handler; + stub_params->c_handler_arg = (uintptr_t)params->handler_arg; + stub_params->fxsave_area = (uintptr_t)fxsave_area; + stub_params->fxsave_area_size = FXSAVE_SIZE; + stub_params->runtime.smbase = (uintptr_t)smbase; + stub_params->runtime.smm_size = smm_size; + stub_params->runtime.save_state_size = params->per_cpu_save_state_size; + stub_params->runtime.num_cpus = params->num_concurrent_stacks; + + printk(BIOS_DEBUG, "%s: stack_end = 0x%x\n", + __func__, stub_params->runtime.smbase); + printk(BIOS_DEBUG, + "%s: stack_top = 0x%x\n", __func__, stub_params->stack_top); + printk(BIOS_DEBUG, "%s: stack_size = 0x%x\n", + __func__, stub_params->stack_size); + printk(BIOS_DEBUG, "%s: runtime.smbase = 0x%x\n", + __func__, stub_params->runtime.smbase); + printk(BIOS_DEBUG, "%s: runtime.start32_offset = 0x%x\n", __func__, + stub_params->runtime.start32_offset); + printk(BIOS_DEBUG, "%s: runtime.smm_size = 0x%zx\n", + __func__, smm_size); + printk(BIOS_DEBUG, "%s: per_cpu_save_state_size = 0x%x\n", + __func__, stub_params->runtime.save_state_size); + printk(BIOS_DEBUG, "%s: num_cpus = 0x%x\n", __func__, + stub_params->runtime.num_cpus); + printk(BIOS_DEBUG, "%s: total_save_state_size = 0x%x\n", + __func__, (stub_params->runtime.save_state_size * + stub_params->runtime.num_cpus)); + total_size_all = stub_params->stack_size + + (stub_params->runtime.save_state_size * + stub_params->runtime.num_cpus); + printk(BIOS_DEBUG, "%s: total_size_all = 0x%x\n", __func__, + total_size_all); + + /* Initialize the APIC id to CPU number table to be 1:1 */ + for (i = 0; i < params->num_concurrent_stacks; i++) + stub_params->runtime.apic_id_to_cpu[i] = i; + + /* Allow the initiator to manipulate SMM stub parameters. */ + params->runtime = &stub_params->runtime; + + printk(BIOS_DEBUG, "SMM Module: stub loaded at %p. Will call %p(%p)\n", + smm_stub_loc, params->handler, params->handler_arg); + return 0; +} + +/* + * smm_setup_relocation_handler assumes the callback is already loaded in + * memory. i.e. Another SMM module isn't chained to the stub. The other + * assumption is that the stub will be entered from the default SMRAM + * location: 0x30000 -> 0x40000. + */ +int smm_setup_relocation_handler(struct smm_loader_params *params) +{ + void *smram = (void *)(SMM_DEFAULT_BASE); + printk(BIOS_SPEW, "%s: enter\n", __func__); + /* There can't be more than 1 concurrent save state for the relocation + * handler because all CPUs default to 0x30000 as SMBASE. */ + if (params->num_concurrent_save_states > 1) + return -1; + + /* A handler has to be defined to call for relocation. */ + if (params->handler == NULL) + return -1; + + /* Since the relocation handler always uses stack, adjust the number + * of concurrent stack users to be CONFIG_MAX_CPUS. */ + if (params->num_concurrent_stacks == 0) + params->num_concurrent_stacks = CONFIG_MAX_CPUS; + + params->smm_main_entry_offset = SMM_ENTRY_OFFSET; + params->smram_start = SMM_DEFAULT_BASE; + params->smram_end = SMM_DEFAULT_BASE + SMM_DEFAULT_SIZE; + return smm_module_setup_stub(smram, SMM_DEFAULT_SIZE, + params, fxsave_area_relocation); + printk(BIOS_SPEW, "%s: exit\n", __func__); +} + +/* + *The SMM module is placed within the provided region in the following + * manner: + * +-----------------+ <- smram + size + * | BIOS resource | + * | list (STM) | + * +-----------------+ + * | fxsave area | + * +-----------------+ + * | smi handler | + * | ... | + * +-----------------+ <- cpu0 + * | stub code | <- cpu1 + * | stub code | <- cpu2 + * | stub code | <- cpu3, etc + * | | + * | | + * | | + * | stacks | + * +-----------------+ <- smram start + + * It should be noted that this algorithm will not work for + * SMM_DEFAULT_SIZE SMRAM regions such as the A segment. This algorithm + * expects a region large enough to encompass the handler and stacks + * as well as the SMM_DEFAULT_SIZE. + */ +int smm_load_module(void *smram, size_t size, struct smm_loader_params *params) +{ + struct rmodule smm_mod; + size_t total_stack_size; + size_t handler_size; + size_t module_alignment; + size_t alignment_size; + size_t fxsave_size; + void *fxsave_area; + size_t total_size = 0; + char *base; + + if (size <= SMM_DEFAULT_SIZE) + return -1; + + /* Load main SMI handler at the top of SMRAM + * everything else will go below + */ + base = smram; + base += size; + params->smram_start = (uintptr_t)smram; + params->smram_end = params->smram_start + size; + params->smm_main_entry_offset = SMM_ENTRY_OFFSET; + + /* Fail if can't parse the smm rmodule. */ + if (rmodule_parse(&_binary_smm_start, &smm_mod)) + return -1; + + /* Clear SMM region */ + if (CONFIG(DEBUG_SMI)) + memset(smram, 0xcd, size); + + total_stack_size = params->per_cpu_stack_size * + params->num_concurrent_stacks; + total_size += total_stack_size; + /* Stacks are the base of SMRAM */ + params->stack_top = smram + total_stack_size; + + /* MSEG starts at the top of SMRAM and works down */ + if (CONFIG(STM)) { + base -= CONFIG_MSEG_SIZE + CONFIG_BIOS_RESOURCE_LIST_SIZE; + total_size += CONFIG_MSEG_SIZE + CONFIG_BIOS_RESOURCE_LIST_SIZE; + } + + /* FXSAVE goes below MSEG */ + if (CONFIG(SSE)) { + fxsave_size = FXSAVE_SIZE * params->num_concurrent_stacks; + fxsave_area = base - fxsave_size; + base -= fxsave_size; + total_size += fxsave_size; + } else { + fxsave_size = 0; + fxsave_area = NULL; + } + + + handler_size = rmodule_memory_size(&smm_mod); + base -= handler_size; + total_size += handler_size; + module_alignment = rmodule_load_alignment(&smm_mod); + alignment_size = module_alignment - + ((uintptr_t)base % module_alignment); + if (alignment_size != module_alignment) { + handler_size += alignment_size; + base += alignment_size; + } + + printk(BIOS_DEBUG, + "%s: total_smm_space_needed %zx, available -> %zx\n", + __func__, total_size, size); + + /* Does the required amount of memory exceed the SMRAM region size? */ + if (total_size > size) { + printk(BIOS_ERR, "%s: need more SMRAM\n", __func__); + return -1; + } + if (handler_size > SMM_CODE_SEGMENT_SIZE) { + printk(BIOS_ERR, "%s: increase SMM_CODE_SEGMENT_SIZE: handler_size = %zx\n", + __func__, handler_size); + return -1; + } + + if (rmodule_load(base, &smm_mod)) + return -1; + + params->handler = rmodule_entry(&smm_mod); + params->handler_arg = rmodule_parameters(&smm_mod); + + printk(BIOS_DEBUG, "%s: smram_start: 0x%p\n", + __func__, smram); + printk(BIOS_DEBUG, "%s: smram_end: %p\n", + __func__, smram + size); + printk(BIOS_DEBUG, "%s: stack_top: %p\n", + __func__, params->stack_top); + printk(BIOS_DEBUG, "%s: handler start %p\n", + __func__, params->handler); + printk(BIOS_DEBUG, "%s: handler_size %zx\n", + __func__, handler_size); + printk(BIOS_DEBUG, "%s: handler_arg %p\n", + __func__, params->handler_arg); + printk(BIOS_DEBUG, "%s: fxsave_area %p\n", + __func__, fxsave_area); + printk(BIOS_DEBUG, "%s: fxsave_size %zx\n", + __func__, fxsave_size); + printk(BIOS_DEBUG, "%s: CONFIG_MSEG_SIZE 0x%x\n", + __func__, CONFIG_MSEG_SIZE); + printk(BIOS_DEBUG, "%s: CONFIG_BIOS_RESOURCE_LIST_SIZE 0x%x\n", + __func__, CONFIG_BIOS_RESOURCE_LIST_SIZE); + + /* CPU 0 smbase goes first, all other CPUs + * will be staggered below + */ + base -= SMM_CODE_SEGMENT_SIZE; + printk(BIOS_DEBUG, "%s: cpu0 entry: %p\n", + __func__, base); + params->smm_entry = (uintptr_t)base + params->smm_main_entry_offset; + return smm_module_setup_stub(base, size, params, fxsave_area); +} diff --git a/src/include/cpu/x86/smm.h b/src/include/cpu/x86/smm.h index a3101e5..db63e8b 100644 --- a/src/include/cpu/x86/smm.h +++ b/src/include/cpu/x86/smm.h @@ -128,6 +128,12 @@ * into this field so the code doing the loading can manipulate the * runtime's assumptions. e.g. updating the APIC id to CPU map to * handle sparse APIC id space. + * The following parameters are only used when X86_SMM_LOADER_VERSION2 is enabled. + * - smm_entry - entry address of first CPU thread, all others will be tiled + * below this address. + * - smm_main_entry_offset - default entry offset (e.g 0x8000) + * - smram_start - smaram starting address + * - smram_end - smram ending address */ struct smm_loader_params { void *stack_top; @@ -141,12 +147,24 @@ void *handler_arg;
struct smm_runtime *runtime; + + /* The following are only used by X86_SMM_LOADER_VERSION2 */ +#if CONFIG(X86_SMM_LOADER_VERSION2) + unsigned int smm_entry; + unsigned int smm_main_entry_offset; + unsigned int smram_start; + unsigned int smram_end; +#endif };
/* Both of these return 0 on success, < 0 on failure. */ int smm_setup_relocation_handler(struct smm_loader_params *params); int smm_load_module(void *smram, size_t size, struct smm_loader_params *params);
+#if CONFIG(X86_SMM_LOADER_VERSION2) +u32 smm_get_cpu_smbase(unsigned int cpu_num); +#endif + /* Backup and restore default SMM region. */ void *backup_default_smm_area(void); void restore_default_smm_area(void *smm_save_area);
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: Introduce SMM module loader version 2 ......................................................................
Patch Set 11:
(1 comment)
Patchset:
PS11: GCC 10.2 from Debian sid/unstable shows the errors below.
``` CC ramstage/cpu/x86/smm/smm_module_loader.o src/cpu/x86/smm/smm_module_loader.c: In function 'smm_create_map': src/cpu/x86/smm/smm_module_loader.c:146:19: error: format '%zx' expects argument of type 'size_t', but argument 3 has type 'uintptr_t' {aka 'long unsigned int'} [-Werror=format=] 146 | " smbase %zx entry %zx\n", | ~~^ | | | unsigned int | %lx 147 | cpus[i].smbase, cpus[i].entry); | ~~~~~~~~~~~~~~ | | | uintptr_t {aka long unsigned int} src/cpu/x86/smm/smm_module_loader.c:146:30: error: format '%zx' expects argument of type 'size_t', but argument 4 has type 'uintptr_t' {aka 'long unsigned int'} [-Werror=format=] 146 | " smbase %zx entry %zx\n", | ~~^ | | | unsigned int | %lx 147 | cpus[i].smbase, cpus[i].entry); | ~~~~~~~~~~~~~ | | | uintptr_t {aka long unsigned int} src/cpu/x86/smm/smm_module_loader.c:149:28: error: format '%zx' expects argument of type 'size_t', but argument 3 has type 'uintptr_t' {aka 'long unsigned int'} [-Werror=format=] 149 | " ss_start %zx code_end %zx\n", | ~~^ | | | unsigned int | %lx 150 | cpus[i].ss_start, cpus[i].code_end); | ~~~~~~~~~~~~~~~~ | | | uintptr_t {aka long unsigned int} src/cpu/x86/smm/smm_module_loader.c:149:42: error: format '%zx' expects argument of type 'size_t', but argument 4 has type 'uintptr_t' {aka 'long unsigned int'} [-Werror=format=] 149 | " ss_start %zx code_end %zx\n", | ~~^ | | | unsigned int | %lx 150 | cpus[i].ss_start, cpus[i].code_end); | ~~~~~~~~~~~~~~~~ | | | uintptr_t {aka long unsigned int} src/cpu/x86/smm/smm_module_loader.c: In function 'smm_place_entry_code': src/cpu/x86/smm/smm_module_loader.c:220:36: error: format '%zx' expects argument of type 'size_t', but argument 4 has type 'uintptr_t' {aka 'long unsigned int'} [-Werror=format=] 220 | printk(BIOS_ERR, "%s: smbase %zx, stack_top %lx\n", | ~~^ | | | unsigned int | %lx 221 | __func__, cpus[num_cpus].smbase, stack_top); | ~~~~~~~~~~~~~~~~~~~~~ | | | uintptr_t {aka long unsigned int} src/cpu/x86/smm/smm_module_loader.c:226:34: error: format '%zx' expects argument of type 'size_t', but argument 4 has type 'uintptr_t' {aka 'long unsigned int'} [-Werror=format=] 226 | printk(BIOS_INFO, "%s: smbase %zx, stack_top %lx\n", | ~~^ | | | unsigned int | %lx 227 | __func__, cpus[num_cpus-1].smbase, stack_top); | ~~~~~~~~~~~~~~~~~~~~~~~ | | | uintptr_t {aka long unsigned int} src/cpu/x86/smm/smm_module_loader.c:234:45: error: format '%zx' expects argument of type 'size_t', but argument 3 has type 'uintptr_t' {aka 'long unsigned int'} [-Werror=format=] 234 | "SMM Module: placing smm entry code at %zx, cpu # 0x%x\n", | ~~^ | | | unsigned int | %lx 235 | cpus[i].code_start, i); | ~~~~~~~~~~~~~~~~~~ | | | uintptr_t {aka long unsigned int} src/cpu/x86/smm/smm_module_loader.c:236:42: error: format '%zx' expects argument of type 'size_t', but argument 4 has type 'uintptr_t' {aka 'long unsigned int'} [-Werror=format=] 236 | printk(BIOS_DEBUG, "%s: copying from %zx to %zx 0x%x bytes\n", | ~~^ | | | unsigned int | %lx 237 | __func__, cpus[0].code_start, cpus[i].code_start, size); | ~~~~~~~~~~~~~~~~~~ | | | uintptr_t {aka long unsigned int} src/cpu/x86/smm/smm_module_loader.c:236:49: error: format '%zx' expects argument of type 'size_t', but argument 5 has type 'uintptr_t' {aka 'long unsigned int'} [-Werror=format=] 236 | printk(BIOS_DEBUG, "%s: copying from %zx to %zx 0x%x bytes\n", | ~~^ | | | unsigned int | %lx 237 | __func__, cpus[0].code_start, cpus[i].code_start, size); | ~~~~~~~~~~~~~~~~~~ | | | uintptr_t {aka long unsigned int} src/cpu/x86/smm/smm_module_loader.c: In function 'smm_module_setup_stub': src/cpu/x86/smm/smm_module_loader.c:359:70: error: format '%lx' expects argument of type 'long unsigned int', but argument 5 has type 'unsigned int' [-Werror=format=] 359 | printk(BIOS_ERR, "%s: state save size: %zx : smm_entry_offset -> %lx\n", | ~~^ | | | long unsigned int | %x src/cpu/x86/smm/smm_module_loader.c:414:42: error: format '%lx' expects argument of type 'long unsigned int', but argument 4 has type 'u32' {aka 'unsigned int'} [-Werror=format=] 414 | printk(BIOS_DEBUG, "%s: stack_end = 0x%lx\n", | ~~^ | | | long unsigned int | %x 415 | __func__, stub_params->stack_top - total_stack_size); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | | | u32 {aka unsigned int} CC ramstage/cpu/x86/pae/pgtbl.o CC ramstage/cpu/x86/mtrr/debug.o CC ramstage/cpu/x86/mtrr/mtrr.o cc1: all warnings being treated as errors ```
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: Introduce SMM module loader version 2 ......................................................................
Patch Set 11:
(1 comment)
Patchset:
PS11:
GCC 10.2 from Debian sid/unstable shows the errors below. […]
1. https://review.coreboot.org/c/coreboot/+/54341 2. https://review.coreboot.org/c/coreboot/+/54342 3. https://review.coreboot.org/c/coreboot/+/54343
Attention is currently required from: Rocky Phagura. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: Introduce SMM module loader version 2 ......................................................................
Patch Set 11:
(1 comment)
File src/cpu/x86/smm/smm_module_loaderv2.c:
https://review.coreboot.org/c/coreboot/+/43684/comment/84cbcb22_3314ca80 PS11, Line 425: if (!smm_stub_place_staggered_entry_points(base, params, &smm_stub)) { : printk(BIOS_ERR, "%s: staggered entry points failed\n", __func__); : return -1; : } : : /* Setup the parameters for the stub code. */ : stub_params = rmodule_parameters(&smm_stub); : stub_params->stack_top = (uintptr_t)stacks_top; : stub_params->stack_size = params->per_cpu_stack_size; : stub_params->c_handler = (uintptr_t)params->handler; : stub_params->c_handler_arg = (uintptr_t)params->handler_arg; : stub_params->fxsave_area = (uintptr_t)fxsave_area; : stub_params->fxsave_area_size = FXSAVE_SIZE; : stub_params->runtime.smbase = (uintptr_t)smbase; : stub_params->runtime.smm_size = smm_size; : stub_params->runtime.save_state_size = params->per_cpu_save_state_size; : stub_params->runtime.num_cpus = params->num_concurrent_stacks; hmmmfff this is bad! this only sets the c_handler on the BSP entry. On all the APs SMIs result in jumps to whatever c_handler is, likely 0. This means the OS kernel can likely highjack SMM!