| Age | Commit message (Collapse) | Author |
|---|
|
The current region_end() implementation is susceptible to overflow
if the region is at the end of the addressable space. A common case
with the memory-mapped flash of x86 directly below the 32-bit limit.
Note: This patch also changes console output to inclusive limits.
IMO, to the better.
Change-Id: Ic4bd6eced638745b7e845504da74542e4220554a
Signed-off-by: Nico Huber <nico.h@gmx.de>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/79946
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Julius Werner <jwerner@chromium.org>
|
|
We introduce two new functions to create region objects. They allow us
to check for integer overflows (region_create_untrusted()) or assert
their absence (region_create()).
This fixes potential overflows in region_overlap() checks in SMI
handlers, where we would wrongfully report MMIO as *not* overlapping
SMRAM.
Also, two cases of strtol() in parse_region() (cbfstool), where the
results were implicitly converted to `size_t`, are replaced with the
unsigned strtoul().
FIT payload support is left out, as it doesn't use the region API
(only the struct).
Change-Id: I4ae3e6274c981c9ab4fb1263c2a72fa68ef1c32b
Ticket: https://ticket.coreboot.org/issues/522
Found-by: Vadim Zaliva <lord@digamma.ai>
Signed-off-by: Nico Huber <nico.h@gmx.de>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/79905
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Felix Held <felix-coreboot@felixheld.de>
|
|
<stdio.h> header is used for input/output operations (such as printf,
scanf, fopen, etc.). Although some input/output functions can manipulate
strings, they do not need to directly include <string.h> because they
are declared independently.
Change-Id: Ibe2a4ff6f68843a6d99cfdfe182cf2dd922802aa
Signed-off-by: Elyes Haouas <ehaouas@noos.fr>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/82665
Reviewed-by: Yidi Lin <yidilin@google.com>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
|
|
<string.h> is supposed to provide <stdarg.h> and <stdio.h>
Change-Id: I021ba535ba5ec683021c4dfc41ac18d9cebbcfd2
Signed-off-by: Elyes Haouas <ehaouas@noos.fr>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/81853
Reviewed-by: Eric Lai <ericllai@google.com>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
|
|
This removes the runtime SMI call to set up the communication buffer
for SMMSTORE in favor of setting this buffer up during the installation
of the smihandler.
The reason is that it's less code in the handler and a time costly SMI
is also avoided in ramstage.
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Change-Id: I94dce77711f37f87033530f5ae48cb850a39341b
Reviewed-on: https://review.coreboot.org/c/coreboot/+/79738
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Patrick Rudolph <patrick.rudolph@9elements.com>
|
|
<types.h> is supposed to provide <stdint.h>.
Change-Id: Ia68a0dc8fba4a48401e213ebb8356e32f0a019ab
Signed-off-by: Elyes Haouas <ehaouas@noos.fr>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/81633
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Arthur Heymans <arthur@aheymans.xyz>
|
|
This data is used by smm_region_overlaps_handler(). Callers use this
helper to determine if it's safe to read/write to memory buffers taken
from untrusted input.
coreboot SMI handlers must not be confused into writing over any SMRAM
subregion, which includes the TSEG_STAGE_CACHE and chipset-specific area
(sometimes, IED), not just the handlers.
If stage cache writes were permitted, this could compromise the
integrity of the S3 resume path.
The consequences to overwriting the chipset-specific area are undefined.
Change-Id: Ibd9ed34fcfd77a4236b5cf122747a6718ce9c91f
Signed-off-by: Benjamin Doron <benjamin.doron@9elements.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/80703
Reviewed-by: Shuo Liu <shuo.liu@intel.com>
Reviewed-by: Patrick Rudolph <patrick.rudolph@9elements.com>
Reviewed-by: Jérémy Compostella <jeremy.compostella@intel.com>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Arthur Heymans <arthur@aheymans.xyz>
|
|
Relying on page tables being in RO flash is not safe in every setup,
therefore set up some page tables in SMRAM that the permanent smihandler
can use.
Tested on QEMU.
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Change-Id: Icb3086abd577b9abb9966dd910a264a873ace4ed
Reviewed-on: https://review.coreboot.org/c/coreboot/+/80336
Reviewed-by: Patrick Rudolph <patrick.rudolph@9elements.com>
Reviewed-by: Benjamin Doron <benjamin.doron00@gmail.com>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
|
|
To allow for more flexibility like generating page tables at runtime or
page tables that are part of the ramstage, add a parameter to
sipi_vector.S and smm_stub.S so that APs use the same page tables as the
BSP during their initialization.
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Change-Id: I1250ea6f63c65228178ee66e06d988dadfcc2a37
Reviewed-on: https://review.coreboot.org/c/coreboot/+/80335
Reviewed-by: Patrick Rudolph <patrick.rudolph@9elements.com>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Jérémy Compostella <jeremy.compostella@intel.com>
|
|
No rmodule was using heap.
Change-Id: I0bc049a5231dabbec1c962a99ef875eddcc4ac6e
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/80733
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Julius Werner <jwerner@chromium.org>
|
|
This makes it easier to reuse, e.g. if you want to do it twice in one
assembly file.
Change-Id: Ida861338004187e4e714be41e17c8447fa4cf935
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/79261
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Felix Held <felix-coreboot@felixheld.de>
|
|
When a device with no resource is passed it will keep overwriting
the current slot. Remove the conditional and allow a PCI device
to not have any resources.
This is particular useful for the next commits that makes use
of the PCI resource store to pass UBOX devices to SMM that allow
to lock-down SMM from within an SMI handler. Those devices do
not have any resources and cannot be hardcoded in SMM as their
PCI segment group and bus number varies depending on socket
count, CPU discovery and configuration.
Change-Id: I1a1b5944c97da5be6b9794c653b5159683f492e5
Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/80246
Reviewed-by: Arthur Heymans <arthur@aheymans.xyz>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Martin L Roth <gaumless@gmail.com>
|
|
Allow SMM to verify the list of provided PCI devices by comparing
the device and vendor ID for each PCI device.
Change-Id: I7086fa450fcb117ef8767c199c30462c1ab1e1b6
Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/80245
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Arthur Heymans <arthur@aheymans.xyz>
|
|
The .inc suffix is confusing to various tools as it's not specific to
Makefiles. This means that editors don't recognize the files, and don't
open them with highlighting and any other specific editor functionality.
This issue is also seen in the release notes generation script where
Makefiles get renamed before running cloc.
Signed-off-by: Martin Roth <gaumless@gmail.com>
Change-Id: I552d487978906f5ea74c3d0d85373fe5b2de3f38
Reviewed-on: https://review.coreboot.org/c/coreboot/+/80068
Reviewed-by: Michael Niewöhner <foss@mniewoehner.de>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Felix Singer <service+coreboot-gerrit@felixsinger.de>
Reviewed-by: Maximilian Brune <maximilian.brune@9elements.com>
|
|
Always use the high-level API region_offset() and region_sz()
functions. This excludes the internal `region.c` code as well
as unit tests. FIT payload support was also skipped, as it
seems it never tried to use the API and would need a bigger
overhaul.
Change-Id: Iaae116a1ab2da3b2ea2a5ebcd0c300b238582834
Signed-off-by: Nico Huber <nico.h@gmx.de>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/79904
Reviewed-by: Arthur Heymans <arthur@aheymans.xyz>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Felix Singer <service+coreboot-gerrit@felixsinger.de>
|
|
When the SMI transfer monitor (STM) is configured, get_save_state
returns an incorrect pointer to the cpu save state because the size
(rounded up to 0x100) of the processor System Management Mode (SMM)
descriptor needs to be subtracted out in this case.
This patch addresses the issue identified in CB:76601, which means
that SMMSTOREv2 now works with the STM.
Thanks to Jeremy Compostella for suggesting this version of the patch.
Resolves: https://ticket.coreboot.org/issues/511
Change-Id: I0233c6d13bdffb3853845ac6ef25c066deaab747
Signed-off-by: Eugene D. Myers <edmyers@cyberpackventures.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/78889
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Arthur Heymans <arthur@aheymans.xyz>
|
|
The EFER MSR is in the SMM save state and RSM properly restores it.
Returning to 32bit mode was only done so that fxsave was done in the
same mode as fxrstor, but this is no longer done.
See commit 1efca4d570 (cpu/x86/smm: Drop fxsave/fxrstor logic)
TESTED on qemu: the smihandler works fine.
Change-Id: Ie0e9584afd1f08f51ca57da5c4350042699f130d
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/68895
Reviewed-by: Paul Menzel <paulepanter@mailbox.org>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Elyes Haouas <ehaouas@noos.fr>
|
|
Since we now explicitly compile both ramstage and smihandler code
without floating point operations and associated registers we don't need
to save/restore floating point registers.
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Change-Id: I180b9781bf5849111501ae8e9806554a7851c0da
Reviewed-on: https://review.coreboot.org/c/coreboot/+/75317
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Elyes Haouas <ehaouas@noos.fr>
|
|
The comment got stale because a few elements from the struct got
dropped.
Change-Id: I83469e24dfab82b9182accb549960dd06d81e02f
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/68894
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Kyösti Mälkki <kyosti.malkki@gmail.com>
|
|
%ebp is used for the stack frame on which the fxrstor address is pushed.
entry64.inc does not trash it so that's fine.
Change-Id: If027437dccac9ad507ceb534c6aae77ea43bdfda
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/68896
Reviewed-by: Nico Huber <nico.h@gmx.de>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Maximilian Brune <maximilian.brune@9elements.com>
Reviewed-by: Kyösti Mälkki <kyosti.malkki@gmail.com>
|
|
Don't pass the stub params to the mp_init code.
Change-Id: I070bc00ae5e5bceb6c5b90ea833cc057dd41f6cc
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/64802
Reviewed-by: Patrick Rudolph <siro@das-labor.org>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
|
|
In the current design the relocatable parameters are used to know the
offset of the 32bit startpoint. This requires back and forward
interaction between the stub, the loader and the mp init code. This
makes the code hard to read.
This is static information known at buildtime, so a better way to deal
with this is to generate a header that contains this offset.
Change-Id: Ic01badd2af11a6e1dbc27c8e928916fedf104b5b
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/64625
Reviewed-by: Patrick Rudolph <siro@das-labor.org>
Reviewed-by: Maximilian Brune <maximilian.brune@9elements.com>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
|
|
In certain cases data within protected memmory areas like SMRAM could
be leaked or modified if an attacker remaps PCI BARs to point within
that area. Add support to the existing SMM runtime to allow storing
PCI resources in SMRAM and then later retrieving them.
BRANCH=guybrush
BUG=b:186792595
TEST=builds
Signed-off-by: Robert Zieba <robertzieba@google.com>
Change-Id: I23fb1e935dd1b89f1cc5c834cc2025f0fe5fda37
Reviewed-on: https://review.coreboot.org/c/coreboot/+/67931
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Karthik Ramasubramanian <kramasub@google.com>
|
|
Add a Kconfig RUNTIME_CONFIGURABLE_SMM_LOGLEVEL that enables
mainboard to override mainboard_set_smm_log_level for SMM log level.
This can let SMM have different log level than other stages for
more flexibility.
Another reason is that getting certain data that requires searching
from flash VPD or CMOS is not very ideal to be done in SMM, so in this
change the value can be passed via the member variable in struct
smm_runtime and be referenced directly in SMM.
One example is that mainboard can get the desired SMM log level from
VPD/CMOS, and pass SMM console log level via the variable and in SMM
it can be referenced in get_console_loglevel() override function
directly.
Tested=On OCP Delta Lake, verified SMM log level can be overridden.
Change-Id: I81722a4f1bf75ec942cc06e403ad702dfe938e71
Signed-off-by: Johnny Lin <johnny_lin@wiwynn.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/49460
Reviewed-by: David Hendricks <david.hendricks@gmail.com>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Jonathan Zhang <jonzhang@fb.com>
|
|
There are four requirements for the SMI to hit a printk()
this commit now removes.
Build must have DEBUG_SMI=y, otherwise any printk() is a no-op
inside SMM.
ASL must have a TRAP() with argument 0x99 or 0x32 for SMIF value.
Platform needs to have IO Trap #3 enabled at IO 0x800.
The SMI monitor must call io_trap_handler for IO Trap #3.
At the moment, only getac/p470 would meet the above criteria
with TRAP(0x32) in its DSDT _INI method. The ASL ignores any
return value of TRAP() calls made.
A mainboard IO trap handler should have precedence over
a southbridge IO trap handler. At the moment we seem to have
no cases of the latter to support, so remove the latter.
Change-Id: I3a3298c8d9814db8464fbf7444c6e0e6ac6ac008
Signed-off-by: Kyösti Mälkki <kyosti.malkki@gmail.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/70365
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
Reviewed-by: Arthur Heymans <arthur@aheymans.xyz>
|
|
We have equivalent southbridge_smi_set_eos().
Change-Id: I03a48f0ec9efac2a220aa4ca502a5f504d78c585
Signed-off-by: Kyösti Mälkki <kyosti.malkki@gmail.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/69668
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Arthur Heymans <arthur@aheymans.xyz>
|
|
Change-Id: Idca56583c1c8dc41ad11d915ec3e8be781fb4e48
Signed-off-by: Kyösti Mälkki <kyosti.malkki@gmail.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/69665
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Arthur Heymans <arthur@aheymans.xyz>
|
|
This code was never tested with SSE enabled. Now qemu enables it and
FX_SAVE encroaches on the save states. Without SSE enabled the handler
just happened to be aligned downwards enough to have the save states
fit. With SSE enabled that's not the case. The proper fix is to give the
code setting up stubs the right base address, which is the same as for
the TSEG codepath.
Change-Id: I45355efb274c6ddd09a6fb57743d2f6a5b53d209
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/69233
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Kyösti Mälkki <kyosti.malkki@gmail.com>
|
|
This codepath is deprecated after the 4.18 release.
Change-Id: I7e90f457f3979781d06323ef1350d5fb05a6be43
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/69121
Reviewed-by: Elyes Haouas <ehaouas@noos.fr>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
|
|
This platform use the LEGACY_SMP_INIT which is to be deprecated after
release 4.18.
Change-Id: I18eb1c1ccad16980a4e57318dec411b82c45b25a
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/69116
Reviewed-by: Elyes Haouas <ehaouas@noos.fr>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
|
|
This board use the LEGACY_SMP_INIT which is to be deprecated after
release 4.18.
Change-Id: Idf37ade31ddb55697df1a65062c092a0a485e175
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/69114
Reviewed-by: Elyes Haouas <ehaouas@noos.fr>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
|
|
Signed-off-by: Elyes Haouas <ehaouas@noos.fr>
Change-Id: I01c6651079333686cb0eb68e89e56d7907868124
Reviewed-on: https://review.coreboot.org/c/coreboot/+/68204
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Martin Roth <martin.roth@amd.corp-partner.google.com>
|
|
Signed-off-by: Elyes Haouas <ehaouas@noos.fr>
Change-Id: I36c54e62797e67c1732f8deaf8843daf35610e22
Reviewed-on: https://review.coreboot.org/c/coreboot/+/68032
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Martin L Roth <gaumless@gmail.com>
|
|
Replace nodeid() function in cpu/x86/smm/smihandler.c with calling
lapicid() from include/cpu/x86/lapic.h.
TEST=Timeless build for lenovo/g505s which includes this file in the
build results in identical firmware image.
Signed-off-by: Felix Held <felix-coreboot@felixheld.de>
Change-Id: I336ca9888e24e4d6f10a81cc4f3760c9d7c8f4bc
Reviewed-on: https://review.coreboot.org/c/coreboot/+/67777
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
Reviewed-by: Martin Roth <martin.roth@amd.corp-partner.google.com>
|
|
Instead of redefining the register address in smihandler.c, use the
existing definitions from include/cpu/x86/lapic_def.h.
TEST=Timeless build for lenovo/g505s which includes this file in the
build results in identical firmware image.
Signed-off-by: Felix Held <felix-coreboot@felixheld.de>
Change-Id: Id22f9b5ce53c7bced6bbcc3f5026d4c793b34f78
Reviewed-on: https://review.coreboot.org/c/coreboot/+/67776
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
|
|
CB:63475 inadvertently disabled the STM by moving its load point
off of the MSEG boundry, which is a hardware requirement. In
addition, the BIOS resource list cannot be located within the
MSEG. This patch fixes the issue by moving the STM load point
to the MSEG boundry and placing the bios resource list just below
the MSEG where the STM setup functions can find it.
Fixes: commit 5747f6c (cpu/x86/smm_module_loader.c Rewrite setup)
Signed-off-by: Eugene Myers <edmyers@tycho.nsa.gov>
Change-Id: I7359939063bb1a172fcb701551c099edebfbedd5
Reviewed-on: https://review.coreboot.org/c/coreboot/+/67665
Reviewed-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-by: Eugene Myers <cedarhouse1@comcast.net>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
|
|
This fixes following errors when building GA-945GCM-S2L with clang 14.0.5.
CC ramstage/cpu/x86/smm/smm_module_loader.o
src/cpu/x86/smm/smm_module_loader.c:180:10: error: format specifies type 'unsigned long' but the argument has type 'size_t' (aka 'unsigned int') [-Werror,-Wformat]
region_offset(&cpus[i].stub_code), i);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/cpu/x86/smm/smm_module_loader.c:184:20: error: format specifies type 'unsigned long' but the argument has type 'size_t' (aka 'unsigned int') [-Werror,-Wformat]
__func__, region_offset(&cpus[0].stub_code),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/cpu/x86/smm/smm_module_loader.c:185:10: error: format specifies type 'unsigned long' but the argument has type 'size_t' (aka 'unsigned int') [-Werror,-Wformat]
region_offset(&cpus[i].stub_code), size);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/cpu/x86/smm/smm_module_loader.c:349:52: error: format specifies type 'unsigned long' but the argument has type 'size_t' (aka 'unsigned int') [-Werror,-Wformat]
printk(BIOS_DEBUG, "%-12s [0x%lx-0x%lx]\n", name, region_offset(®ion),
~~~ ^~~~~~~~~~~~~~~~~~~~~~
%zx
src/cpu/x86/smm/smm_module_loader.c:350:9: error: format specifies type 'unsigned long' but the argument has type 'size_t' (aka 'unsigned int') [-Werror,-Wformat]
region_end(®ion));
Signed-off-by: Elyes Haouas <ehaouas@noos.fr>
Change-Id: I59f20aacf91cb50fb194a84082a643b34c6c1ae5
Reviewed-on: https://review.coreboot.org/c/coreboot/+/65154
Reviewed-by: Arthur Heymans <arthur@aheymans.xyz>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
|
|
We use a region later on so we might as well use a region from the
start. This simplifies the computations too.
Change-Id: Iffa36ccb89c36401d3856b24364216e83ca35f91
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/64609
Reviewed-by: Kyösti Mälkki <kyosti.malkki@gmail.com>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Lean Sheng Tan <sheng.tan@9elements.com>
|
|
This allows for some runtime checks on all SMM elements and removes
the need for manual checks.
We can drop completely separate codepaths on SMM_TSEG & SMM_ASEG as the
only difference is where permanent handler gets placed.
TESTED on prodrive/hermes and qemu with SSM_ASEG with 4 cores & SMM_TSEG
with 128 cores. This code figured out quite some problems with
overlapping regions so I think this is the right approach.
Change-Id: Ib7e2e3ae16c223ecfd8d5bce6ff6c17c53496925
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/63602
Reviewed-by: Kyösti Mälkki <kyosti.malkki@gmail.com>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Lean Sheng Tan <sheng.tan@9elements.com>
|
|
Some logging is superfluous and logging that code is being copied is
'SPEW' level.
Change-Id: I84d49a394cc53d78f1e1d3936502ac16810daf9f
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/63481
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Kyösti Mälkki <kyosti.malkki@gmail.com>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
|
|
Checking if the stack encroaches on the entry points is done in other
parts of the code.
Change-Id: I275d5dda9c69cc89608450ae27dd5dbd581e3595
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/63480
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Kyösti Mälkki <kyosti.malkki@gmail.com>
|
|
There is NULL dereference in adjust_apic_id_map() and updating
apic_id_to_cpu[] array within SMM stub fails.
Initial apic_id_to_cpu[] array may have worked for platforms
where APIC IDs are consecutive.
Change-Id: Ie59a731bfc883f8a47048b2ceacc66f44aa5b68c
Signed-off-by: Kyösti Mälkki <kyosti.malkki@gmail.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/64798
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Uwe Poeche <uwe.poeche@siemens.com>
Reviewed-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-by: Lean Sheng Tan <sheng.tan@9elements.com>
Reviewed-by: Sean Rhodes <sean@starlabs.systems>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
|
|
Now that the save state size is handled properly inside the smm_loader
there is no reason to make that distinction in the mp_init code anymore.
Change-Id: Ia0002a33b6d0f792d8d78cf625fd7e830e3e50fc
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/63479
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Martin L Roth <gaumless@tutanota.com>
|
|
This code was hard to read as it did too much and had a lot of state
to keep track of.
It also looks like the staggered entry points were first copied and
only later the parameters of the first stub were filled in. This
means that only the BSP stub is actually jumping to the permanent
smihandler. On the APs the stub would jump to wherever c_handler
happens to point to, which is likely 0. This effectively means that on
APs it's likely easy to have arbitrary code execution in SMM which is a
security problem.
Change-Id: I42ef9d6a30f3039f25e2cde975086a1365ca4182
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/63478
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Martin L Roth <gaumless@tutanota.com>
|
|
We don't want to keep track of the real smm size all the time.
As a bonus now ss_start is now really the start of the save state
instead of top - MAX(stub_size, save state size).
Change-Id: I0981022e6c0df110d4a342ff06b1a3332911e2b7
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/63477
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Martin L Roth <gaumless@tutanota.com>
|
|
This code is much easier to read if one does not have to keep track of
mutable variables.
This also fixes the alignment code on the TSEG smihandler setup code.
It was aligning the code upwards instead of downwards which would cause
it to encroach a part of the save state.
Change-Id: I310a232ced2ab15064bff99a39a26f745239f6b9
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/63475
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Werner Zeh <werner.zeh@siemens.com>
Reviewed-by: Martin L Roth <gaumless@tutanota.com>
|
|
This is a duplicate of code_start.
Change-Id: I38e8905e3ed940fb34280c939d6f2f1fce8480a7
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/63476
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Kyösti Mälkki <kyosti.malkki@gmail.com>
Reviewed-by: Felix Held <felix-coreboot@felixheld.de>
Reviewed-by: Lean Sheng Tan <sheng.tan@9elements.com>
|
|
This code was very hard to read so rewrite it using as few mutable local
variables as possible.
Tested on qemu with 128 cores.
Change-Id: I7a455ba45a1c92533a8ecfd1aeecf34b4a63e409
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/63474
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Martin L Roth <gaumless@tutanota.com>
|
|
Currently no smihandler uses heap.
coreboot's heap manager also is quite limited in what it will
free (only the latest alloc). This makes it a bad idea to use it inside
the smihandler, as depending on the alloc usage the heap might actually
be full at some point, breaking the smihandler.
This also reduces the ramstage by 448 bytes on google/vilboz.
Change-Id: I70cd822be17c1efe13c94a9dbd2e1038808b9c56
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/64521
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Kyösti Mälkki <kyosti.malkki@gmail.com>
Reviewed-by: Felix Held <felix-coreboot@felixheld.de>
Reviewed-by: Nico Huber <nico.h@gmx.de>
|
|
The sinkhole exploit exists in placing the lapic base such that it
messes with GDT. This can be mitigated by checking the lapic MSR
against the current program counter.
Change-Id: I49927c4f4218552b732bac8aae551d845ad7f079
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/37289
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
|
|
There is no reason to do this in a separate loop.
Change-Id: I7fe9f1004597602147aae72f4b754395b6b527cf
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/63473
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Elyes Haouas <ehaouas@noos.fr>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
|
|
This change provides hooks for the SoC so it can perform any
initialization and cleanup in the SMM handler.
For example, if we have a UART enabled firmware with DEBUG_SMI, the UART
controller could have been powered off by the OS. In this case we need
to power on the UART when entering SMM, and then power it off before we
exit. If the OS had the UART enabled when entering SMM, we should
snapshot the UART register state, and restore it on exit. Otherwise we
risk clearing some interrupt enable bits.
BUG=b:221231786, b:217968734
TEST=Build test guybrush
Signed-off-by: Raul E Rangel <rrangel@chromium.org>
Change-Id: I946619cd62a974a98c575a92943b43ea639fc329
Reviewed-on: https://review.coreboot.org/c/coreboot/+/62500
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Karthik Ramasubramanian <kramasub@google.com>
|
|
This change will allow the SMI handler to write to the cbmem console
buffer. Normally SMIs can only be debugged using some kind of serial
port (UART). By storing the SMI logs into cbmem we can debug SMIs using
`cbmem -1`. Now that these logs are available to the OS we could also
verify there were no errors in the SMI handler.
Since SMM can write to all of DRAM, we can't trust any pointers
provided by cbmem after the OS has booted. For this reason we store the
cbmem console pointer as part of the SMM runtime parameters. The cbmem
console is implemented as a circular buffer so it will never write
outside of this area.
BUG=b:221231786
TEST=Boot non-serial FW with DEBUG_SMI and verified SMI messages are
visible when running `cbmem -1`. Perform a suspend/resume cycle and
verify new SMI events are written to the cbmem console log.
Signed-off-by: Raul E Rangel <rrangel@chromium.org>
Change-Id: Ia1e310a12ca2f54210ccfaee58807cb808cfff79
Reviewed-on: https://review.coreboot.org/c/coreboot/+/62355
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Karthik Ramasubramanian <kramasub@google.com>
|
|
This will allow to migrate all platform to the parallel_mp init code
and drop the old lapic_init code.
Change-Id: If499e21a8dc7fca18bd5990f833170d0fc21e10c
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Signed-off-by: Kyösti Mälkki <kyosti.malkki@gmail.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/58700
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
|
|
This reverts commit ceaf959678905f44a54a116f37bd15acab5d4608.
The AMD Picasso SoC doesn't support x2APIC and neither advertises the
presence of its support via bit 21 in EAX of CPUID leaf 1 nor has the
bit 10 in the APIC base address MSR 0x1b set, but it does have 0xd CPUID
leaves, so just checking for the presence of that CPUID leaf isn't
sufficient to be sure that EDX of the CPUID leaf 0xb will contain a
valid APIC ID.
In the case of Picasso EDX of the CPUID leaf 0xb returns 0 for all cores
which causes coreboot to get stuck somewhere at the end of MP init.
I'm not 100% sure if we should additionally check bit 21 in EAX of CPUID
function 1 is set instead of adding back the is_x2apic_mode check.
TEST=Mandolin with a Picasso SoC boots again.
Signed-off-by: Felix Held <felix-coreboot@felixheld.de>
Change-Id: If1e3c55ce2d048b14c08e06bb79810179a87993d
Reviewed-on: https://review.coreboot.org/c/coreboot/+/61776
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Raul Rangel <rrangel@chromium.org>
Reviewed-by: Tim Wawrzynczak <twawrzynczak@chromium.org>
Reviewed-by: Christian Walter <christian.walter@9elements.com>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
Reviewed-by: Kyösti Mälkki <kyosti.malkki@gmail.com>
|
|
Now that the console system itself will clearly differentiate loglevels,
it is no longer necessary to explicitly add "ERROR: " in front of every
BIOS_ERR message to help it stand out more (and allow automated tooling
to grep for it). Removing all these extra .rodata characters should save
us a nice little amount of binary size.
This patch was created by running
find src/ -type f -exec perl -0777 -pi -e 's/printk\(\s*BIOS_ERR,\s*"ERROR: /printk\(BIOS_ERR, "/gi' '{}' ';'
and doing some cursory review/cleanup on the result. Then doing the same
thing for BIOS_WARN with
's/printk\(\s*BIOS_WARNING,\s*"WARN(ING)?: /printk\(BIOS_WARNING, "/gi'
Signed-off-by: Julius Werner <jwerner@chromium.org>
Change-Id: I3d0573acb23d2df53db6813cb1a5fc31b5357db8
Reviewed-on: https://review.coreboot.org/c/coreboot/+/61309
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: HAOUAS Elyes <ehaouas@noos.fr>
Reviewed-by: Lance Zhao
Reviewed-by: Jason Glenesk <jason.glenesk@gmail.com>
|
|
A lot of soc code requires a definition of apm_control, which
smm/smi_trigger.c provided for !HAVE_SMI_HANDLER, but is not added as
a build target.
Fixes building Q35 without smihandler.
Change-Id: Ie57819b3d169311371a1caca83c9b0c796b46048
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/59913
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
Reviewed-by: Kyösti Mälkki <kyosti.malkki@gmail.com>
Reviewed-by: HAOUAS Elyes <ehaouas@noos.fr>
|
|
This is just the amount of cpus so rename it for simplicity.
Change-Id: Ib2156136894eeda4a29e8e694480abe06da62959
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/58699
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: HAOUAS Elyes <ehaouas@noos.fr>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
|
|
Both the relocation handler and the permanent handler use the same
stacks, so things can be simplified.
Change-Id: I7bdca775550e8280757a6c5a5150a0d638d5fc2d
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/58698
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: HAOUAS Elyes <ehaouas@noos.fr>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
|
|
Even when we're not in X2APIC mode, the information in CPUID
leaf 0xb will be valid if that leaf is implemented on the CPU.
Change-Id: I0f1f46fe5091ebeab6dfb4c7e151150cf495d0cb
Signed-off-by: Kyösti Mälkki <kyosti.malkki@gmail.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/58386
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Tim Wawrzynczak <twawrzynczak@chromium.org>
Reviewed-by: Wonkyu Kim <wonkyu.kim@intel.com>
Reviewed-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
|
|
Change-Id: I85750282ab274f52bc176a1ac151ef2f9e0dd15d
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/58697
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: HAOUAS Elyes <ehaouas@noos.fr>
|
|
Change-Id: I7fa1f9402b177a036f08bf99c98a6191c35fa0b5
Signed-off-by: Elyes HAOUAS <ehaouas@noos.fr>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/61371
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Jason Glenesk <jason.glenesk@gmail.com>
Reviewed-by: Felix Held <felix-coreboot@felixheld.de>
|
|
Now that cpu_info() is no longer used by COOP_MULTITASKING, we no
longer need to set up cpu_info in SMM. When using CPU_INFO_V2, if
something does manage to call cpu_info() while executing in SMM mode,
the %gs segment is disabled, so it will generate an exception.
BUG=b:179699789
TEST=Boot guybrush to OS with threads enabled
Signed-off-by: Raul E Rangel <rrangel@chromium.org>
Change-Id: Id64f32cc63082880a92dab6deb473431b2238cd0
Reviewed-on: https://review.coreboot.org/c/coreboot/+/58204
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Karthik Ramasubramanian <kramasub@google.com>
Reviewed-by: Paul Menzel <paulepanter@mailbox.org>
|
|
There is currently a fundamental flaw in the current cpu_info()
implementation. It assumes that current stack is CONFIG_STACK_SIZE
aligned. This assumption breaks down when performing SMM relocation.
The first step in performing SMM relocation is changing the SMBASE. This
is accomplished by installing the smmstub at 0x00038000, which is the
default SMM entry point. The stub is configured to set up a new stack
with the size of 1 KiB (CONFIG_SMM_STUB_STACK_SIZE), and an entry point
of smm_do_relocation located in RAMSTAGE RAM.
This means that when smm_do_relocation is executed, it is running in SMM
with a different sized stack. When cpu_info() gets called it will be
using CONFIG_STACK_SIZE to calculate the location of the cpu_info
struct. This results in reading random memory. Since cpu_info() has to
run in multiple environments, we can't use a compile time constant to
locate the cpu_info struct.
This CL introduces a new way of locating cpu_info. It uses a per-cpu
segment descriptor that points to a per-cpu segment that is allocated on
the stack. By using a segment descriptor to point to the per-cpu data,
we no longer need to calculate the location of the cpu_info struct. This
has the following advantages:
* Stacks no longer need to be CONFIG_STACK_SIZE aligned.
* Accessing an unconfigured segment will result in an exception. This
ensures no one can call cpu_info() from an unsupported environment.
* Segment selectors are cleared when entering SMM and restored when
leaving SMM.
* There is a 1:1 mapping between cpu and cpu_info. When using
COOP_MULTITASKING, a new cpu_info is currently allocated at the top of
each thread's stack. This no longer needs to happen.
This CL guards most of the code with CONFIG(CPU_INFO_V2). I did this so
reviewers can feel more comfortable knowing most of the CL is a no-op. I
would eventually like to remove most of the guards though.
This CL does not touch the LEGACY_SMP_INIT code path. I don't have any
way of testing it.
The %gs segment was chosen over the %fs segment because it's what the
linux kernel uses for per-cpu data in x86_64 mode.
BUG=b:194391185, b:179699789
TEST=Boot guybrush with CPU_INFO_V2 and verify BSP and APs have correct
%gs segment. Verify cpu_info looks sane. Verify booting to the OS
works correctly with COOP_MULTITASKING enabled.
Signed-off-by: Raul E Rangel <rrangel@chromium.org>
Change-Id: I79dce9597cb784acb39a96897fb3c2f2973bfd98
Reviewed-on: https://review.coreboot.org/c/coreboot/+/57627
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Eric Peers <epeers@google.com>
Reviewed-by: Karthik Ramasubramanian <kramasub@google.com>
|
|
The %fs and %gs segment are typically used to implement thread local
storage or cpu local storage. We don't currently use these in coreboot,
so there is no reason to map them. By setting the segment index to 0,
it disables the segment. If an instruction tries to read from one of
these segments an exception will be raised.
The end goal is to make cpu_info() use the %gs segment. This will remove
the stack alignment requirements and fix smm_do_relocation.
BUG=b:194391185, b:179699789
TEST=Boot guybrush to OS
Signed-off-by: Raul E Rangel <rrangel@chromium.org>
Change-Id: Iaa376e562acc6bd1dfffb7a23bdec82aa474c1d5
Reviewed-on: https://review.coreboot.org/c/coreboot/+/57860
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Eric Peers <epeers@google.com>
Reviewed-by: Arthur Heymans <arthur@aheymans.xyz>
|
|
Tested on Intel Sandybridge x86_64 and x86_32.
Change-Id: I152483d24af0512c0ee4fbbe8931b7312e487ac6
Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/44867
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
|
|
This commit adds a method called `mainboard_smi_finalize` which provides
a mechanism for a mainboard to execute some code as part of the finalize
method in the SMM stage before SoC does its finalization.
BUG=b:191189275
BRANCH=None
TEST=Implement `mainboard_smi_finalize` on lalala and verify that the
code executes in SMM.
Signed-off-by: Aseda Aboagye <aaboagye@google.com>
Change-Id: If1ee63431e3c2a5831a4656c3a361229acff3f42
Reviewed-on: https://review.coreboot.org/c/coreboot/+/55649
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Karthik Ramasubramanian <kramasub@google.com>
|
|
This fixes a hard to debug hang that could occur in any stage, but in
the end it follows simple rules and is easy to fix.
In long mode the 32bit displacement addressing used on 'mov' and 'lea'
instructions is sign-extended. Those instructions can be found using
readelf on the stage and searching for relocation type R_X86_64_32S.
The sign extension is no issue when either running in protected mode or
the code module and thus the address is below 2GiB. If the address is
greater than 2GiB, as usually the case for code in TSEG, the higher
address bits [64:32] are all set to 1 and the effective address is
pointing to memory not paged. Accessing this memory will cause a page
fault, which isn't handled either.
To prevent such problems
- disable R_AMD64_32S relocations in rmodtool
- add comment explaining why it's not allowed
- use the pseudo op movabs, which doesn't use 32bit displacement addressing
- Print a useful error message if such a reloc is present in the code
Fixes a crash in TSEG and when in long mode seen on Intel Sandybridge.
Change-Id: Ia5f5a9cde7c325f67b12e3a8e9a76283cc3870a3
Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/55448
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Arthur Heymans <arthur@aheymans.xyz>
|
|
Note that there are assumptions about LAPIC MMIO location
in both AMD and Intel sources in coreboot proper.
Change-Id: I2c668f5f9b93d170351c00d77d003c230900e0b4
Signed-off-by: Kyösti Mälkki <kyosti.malkki@gmail.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/55194
Reviewed-by: Felix Held <felix-coreboot@felixheld.de>
Reviewed-by: Arthur Heymans <arthur@aheymans.xyz>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
|
|
The 64-bit compiler x86_64-linux-gnu-gcc-10 aborts the build with the
format warning below:
CC ramstage/cpu/x86/smm/smm_module_loader.o
src/cpu/x86/smm/smm_module_loader.c:415:42: error: format '%lx' expects argument of type 'long unsigned int', but argument 4 has type 'u32' {aka 'unsigned int'} [-Werror=format=]
415 | printk(BIOS_DEBUG, "%s: stack_end = 0x%lx\n",
| ~~^
| |
| long unsigned int
| %x
416 | __func__, stub_params->stack_top - total_stack_size);
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| |
| u32 {aka unsigned int}
The size of `size_t` differs between i386-elf (32-bit) and
x86_64-elf/x86_64-linux-gnu (64-bit).
Unfortunately, coreboot hardcodes
src/include/inttypes.h:#define PRIx32 "x"
so `PRIx32` cannot be used.
There use `z` as length modifier, as size_t should be always big enough
to hold the value.
Found-by: x86_64-linux-gnu-gcc-10 (Debian 10.2.1-6) 10.2.1 20210110
Fixes: afb7a814 ("cpu/x86/smm: Introduce SMM module loader version 2")
Change-Id: Ib504bc5e5b19f62d4702b7f485522a2ee3d26685
Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/54343
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Felix Held <felix-coreboot@felixheld.de>
|
|
The 64-bit compiler x86_64-linux-gnu-gcc-10 aborts the build with the
format warning below:
CC ramstage/cpu/x86/smm/smm_module_loader.o
src/cpu/x86/smm/smm_module_loader.c: In function 'smm_module_setup_stub':
src/cpu/x86/smm/smm_module_loader.c:360:70: error: format '%lx' expects argument of type 'long unsigned int', but argument 5 has type 'unsigned int' [-Werror=format=]
360 | printk(BIOS_ERR, "%s: state save size: %zx : smm_entry_offset -> %lx\n",
| ~~^
| |
| long unsigned int
| %x
As `size_t` is defined as `long unsigned int` in i386-elf (32-bit), the
length modifier `l` matches there. With x86_64-elf/x86_64-linux-gnu
(64-bit) and `-m32` `size_t` is defined as `unsigned int` resulting in a
type mismatch. So, use the correct length modifier `z` for the type
`size_t`.
Found-by: x86_64-linux-gnu-gcc-10 (Debian 10.2.1-6) 10.2.1 20210110
Fixes: afb7a814 ("cpu/x86/smm: Introduce SMM module loader version 2")
Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
Change-Id: I4172e0f4dc40437250da89b7720a5c1e5fbab709
Reviewed-on: https://review.coreboot.org/c/coreboot/+/54342
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Werner Zeh <werner.zeh@siemens.com>
|
|
The 64-bit compiler x86_64-linux-gnu-gcc-10 aborts the build with the
format warning 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}
In coreboot `uintptr_t` is defined in `src/include/stdint.h`:
typedef unsigned long uintptr_t;
As `size_t` is defined as `long unsigned int` in i386-elf (32-bit), the
length modifier `z` matches there. With x86_64-elf/x86_64-linux-gnu
(64-bit) and `-m32` `size_t` is defined as `unsigned int` resulting in a
type mismatch. Normally, `PRIxPTR` would need to be used as a length
modifier, but as coreboot always defines `uintptr_t` to `unsigned long`
(and in `src/include/inttypes.h` also defines `PRIxPTR` as `"lx"`), use
the length modifier `l` to make the code more readable.
Found-by: x86_64-linux-gnu-gcc-10 (Debian 10.2.1-6) 10.2.1 20210110
Fixes: afb7a814 ("cpu/x86/smm: Introduce SMM module loader version 2")
Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
Change-Id: I32bff397c8a033fe34390e6c1a7dfe773707a4e8
Reviewed-on: https://review.coreboot.org/c/coreboot/+/54341
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Werner Zeh <werner.zeh@siemens.com>
|
|
Change-Id: I28f262078cf7f5ec4ed707639e845710a8cc56ea
Signed-off-by: Patrick Georgi <pgeorgi@google.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/53926
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Kyösti Mälkki <kyosti.malkki@gmail.com>
Reviewed-by: Paul Menzel <paulepanter@mailbox.org>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
|
|
Fix booting issues on google/kahlee introduced by CB:51723.
Update use inital apic id in smm_stub.S to support xapic mode error.
Check more bits(LAPIC_BASE_MSR BIT10 and BIT11) for x2apic mode.
TEST=Boot to OS and check apicid, debug log for CPUIDs
cpuid_ebx(1), cpuid_ext(0xb, 0), cpuid_edx(0xb) etc
Signed-off-by: Wonkyu Kim <wonkyu.kim@intel.com>
Change-Id: Ia28f60a077182c3753f6ba9fbdd141f951d39b37
Reviewed-on: https://review.coreboot.org/c/coreboot/+/52696
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Patrick Georgi <pgeorgi@google.com>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
|
|
This patch removes a call to console_init() and debug print message since
the code is not thread safe. This prevents system hangs (soft hangs)
while in SMM if user drops in a new SOC with more cores or another
socket or as a result of bad configuration. Console is already
initialized after the lock has been acquired so this does not affect any
other functionality.
Tested on DeltaLake mainboard with SMM enabled and 52 CPU threads.
Change-Id: I7e8af35d1cde78b327144b6a9da528ae7870e874
Signed-off-by: Rocky Phagura <rphagura@fb.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/52518
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Nico Huber <nico.h@gmx.de>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
|
|
Coverity detects the control flow UNREACHABLE issue for the printk
usage. This change adds rc to keep the smm_module_setup_stub function
call and returns rc after printk usage.
Found-by: Coverity CID 1452602
TEST=None
Signed-off-by: John Zhao <john.zhao@intel.com>
Change-Id: Ie3b90a8197c3b84c5a1dbca8a9ef566bef35c9ab
Reviewed-on: https://review.coreboot.org/c/coreboot/+/52574
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
|
|
As v1 was dropped, rename v2.
Change-Id: I4dd51804e9391284c7624c42ad8180a14b1a4c84
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/51528
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
|
|
Change-Id: I536a104428ae86e82977f2510b9e76715398b442
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/51187
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
|
|
Use the same stack location during relocation as for the permanent
handler.
When the number of CPUs is too large the stacks during relocation
don't fit inside the default SMRAM segment at 0x30000. Currently the
code would just let the CPU stack base grow downwards outside of the
default SMM segment which would corrupt lower memory if S3 is
implemented.
Also update the comment on smm_module_setup_stub().
Change-Id: I6a0a890e8b1c2408301564c22772032cfee4d296
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/51186
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
|
|
Implement x2apic mode as existing code only supports apic mode.
Use info from LAPIC_BASE_MSR (LAPIC_BASE_MSR_X2APIC_MODE) to check
if apic mode or x2apic mode and implement x2apic mode according to
x2apic specfication.
Reference:
https://software.intel.com/content/www/us/en/develop/download/intel-64-architecture-x2apic-specification.html
BUG=None
BRANCH=None
TEST=boot to OS and check apic mode
cat /proc/cpuinfo | grep "apicid"
ex) can see apicid bigger than 255
apicid : 256
apicid : 260
Signed-off-by: Wonkyu Kim <wonkyu.kim@intel.com>
Change-Id: I0bb729b0521fb9dc38b7981014755daeaf9ca817
Reviewed-on: https://review.coreboot.org/c/coreboot/+/51723
Reviewed-by: Ravishankar Sarawadi <ravishankar.sarawadi@intel.com>
Reviewed-by: Jamie Ryu <jamie.m.ryu@intel.com>
Reviewed-by: Patrick Georgi <pgeorgi@google.com>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
|
|
This fixes an issue introduced in
commit ad0116c0327f575f0af184a2f4861848a49a0e2a
cpu/x86/smm_loaderv2: Remove unused variables
It removed one variable that was needed to set the SMM start address
that is used to set the SMM stack location.
Change-Id: Iddf9f204db54f0d97a90bb423b65db2f7625217f
Signed-off-by: Marc Jones <marcjones@sysproconsulting.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/51721
Reviewed-by: Jay Talbott <JayTalbott@sysproconsulting.com>
Reviewed-by: Arthur Heymans <arthur@aheymans.xyz>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
|
|
The argument provided to the function was always the same as the one
computed inside the function so drop the argument.
Change-Id: I14abf400dce1bd9b03e401b6619a0500a650fa0e
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/51527
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
|
|
The permanent handler module argument 'save_state_size' now holds the
meaning of the real save state size which is then substracted from the
CPUs save state 'top' to get the save state base.
TESTED with qemu Q35 on x86_64 where the stub size exceeds the AMD64
save state size.
Change-Id: I55d7611a17b6d0a39aee1c56318539232a9bb781
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/50770
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
|
|
Remove variables that are either constants or are just assigned but
not used.
Change-Id: I5d291a3464f30fc5d9f4b7233bde575010275973
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/50784
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
|
|
Change-Id: I6648d0710bc0ba71cfbaaf4db7a8c1f33bbc9b35
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/51183
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
|
|
With the smm_module_loaderv2 the save state map is not linear so copy
a map from ramstage into the smihandler.
TESTED on QEMU q35: Both SMMLOADER V1 and V2 handle save states properly.
Change-Id: I31c57b59559ad4ee98500d83969424e5345881ee
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/50769
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
|
|
Move out smm_create_map as this was not run if concurrent_save_states
is 1. The cpus struct array is used in the smm_get_cpu_smbase()
callback so it is necessary to create this.
TEST: run qemu/q35 with -smp 1 (or no -smp argument)
Change-Id: I07a98bbc9ff6dce548171ee6cd0c303db94087aa
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/50783
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
|
|
The parameters that the permanent handler requires are pushed directly
to the permanent handlers relocatable module params.
The paremeters that the relocation handler requires are not passed on
via arguments but are copied inside the ramstage. This is ok as the
relocation handler calls into ramstage.
Change-Id: Ice311d05e2eb0e95122312511d83683d7f0dee58
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/50767
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Patrick Georgi <pgeorgi@google.com>
|
|
struct smm_loader_params is a struct that is passed around in the
ramstage code to set up either the relocation handler or the permanent
handler. At the moment no parameters in the stub 'smm_runtime' are
referenced so it can be dropped. The purpose is to drop the
smm_runtime struct from the stub as it is already located in the
permanent handler.
Change-Id: I09c1b649b5991f55b5ccf57f22e4a3ad4c9e4f03
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/50766
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
|
|
Keep a copy of start32_offset into ramstage to avoid needing to pass
arguments, calling from assembly. Doing this in C code is better than
assembly.
Change-Id: Iac04358e377026f45293bbee03e30d792df407fd
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/50765
Reviewed-by: Eugene Myers <cedarhouse1@comcast.net>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
|
|
Instead of passing on parameters from the stub to the permanent
handler, add them directly to the permanent handler.
The parameters in the stub will be removed in a later patch.
Change-Id: Ib3bde78dd9e0c02dd1d86e03665fa9c65e3d07eb
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/50764
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
|
|
This is only consumed by the stub and not by the relocation handler or
the permanent handler, so move it out of the runtime struct.
Change-Id: I01ed0a412c23c8a82d88408be058a27e55d0dc4d
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/50762
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
|
|
Change-Id: I15b433483c36cce04816e8895789997d91702484
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/51530
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Patrick Georgi <pgeorgi@google.com>
|
|
These stub params need to be synced with the code in smm_stub.S and
are consumed by both the smmloader and smmloader_v2. So it is better
to have the definition located in one place.
Change-Id: Ide3e0cb6dea3359fa9ae660eab627499832817c9
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/50761
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
|
|
Generated with a variant of
https://coccinelle.gitlabpages.inria.fr/website/rules/array.cocci
Change-Id: I083704fd48faeb6c67bba3367fbcfe554a9f7c66
Signed-off-by: Patrick Georgi <pgeorgi@google.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/50594
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
Reviewed-by: HAOUAS Elyes <ehaouas@noos.fr>
Reviewed-by: Kyösti Mälkki <kyosti.malkki@gmail.com>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
|
|
Change-Id: I7a3a1b63c0ef14b1e24ecce2df66f7970e5eb669
Signed-off-by: Kyösti Mälkki <kyosti.malkki@gmail.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/49892
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
|
|
Unify the debug messages on raised SMIs.
Change-Id: I34eeb41d929bfb18730ac821a63bde95ef9a0b3e
Signed-off-by: Kyösti Mälkki <kyosti.malkki@gmail.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/49248
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
|
|
Change-Id: I712fca09b1618017412a3d91f81627ec876f2894
Signed-off-by: Elyes HAOUAS <ehaouas@noos.fr>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/49511
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Jacob Garber <jgarber1@ualberta.ca>
|
|
Change-Id: I9971069803a7cd1b9be0ac0cfa410b6e1fdc3eeb
Signed-off-by: Kyösti Mälkki <kyosti.malkki@gmail.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/49342
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
|
|
This change uses append operation (+=) instead of assignment (:=) for
smm-c-deps to ensure that any earlier assignment is not
overwritten.
Change-Id: Ic1d62b414cfe3f61ee2b80b026b7338faa186904
Signed-off-by: Furquan Shaikh <furquan@google.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/49208
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-by: Tim Wawrzynczak <twawrzynczak@chromium.org>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
Reviewed-by: Karthik Ramasubramanian <kramasub@google.com>
|
