Hello Furquan Shaikh,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/42888
to review the following change.
Change subject: soc/amd/picasso: Enable paging on S3 resume ......................................................................
soc/amd/picasso: Enable paging on S3 resume
This way we can prevent the BSP from accidentally stomping on OS memory.
BUG=b:159081993 TEST=S3 suspend and resume to OS. See paging enabled message. Ran suspend_stress_test for 20+ cycles.
Signed-off-by: Furquan Shaikh furquan@google.com Signed-off-by: Raul E Rangel rrangel@google.com Change-Id: I58fed6f297831b3cdc2758a5e7610388171d84d3 --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/bootblock/bootblock.c M src/soc/amd/picasso/chip.c M src/soc/amd/picasso/memlayout.ld A src/soc/amd/picasso/page_map_s3.txt 6 files changed, 64 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/42888/1
diff --git a/src/soc/amd/picasso/Kconfig b/src/soc/amd/picasso/Kconfig index 9860546..f9c653a 100644 --- a/src/soc/amd/picasso/Kconfig +++ b/src/soc/amd/picasso/Kconfig @@ -174,6 +174,12 @@ Sets the size of DRAM allocation for verstage in linker script if running as a separate stage on x86.
+config PAGE_TABLE_ADDR + hex + default 0x2B0000 + help + Sets the address in DRAM where page tables should be loaded. + config RAMBASE hex default 0x10000000 diff --git a/src/soc/amd/picasso/Makefile.inc b/src/soc/amd/picasso/Makefile.inc index 39269e9..9d3bbd8 100644 --- a/src/soc/amd/picasso/Makefile.inc +++ b/src/soc/amd/picasso/Makefile.inc @@ -445,4 +445,15 @@
cpu_microcode_bins += $(wildcard 3rdparty/amd_blobs/picasso/PSP/UcodePatch_*.bin)
+$(obj)/pt_s3.c $(obj)/pdpt_s3.c: src/soc/amd/picasso/page_map_s3.txt $(DOTCONFIG) util/x86/x86_page_tables.go + go run util/x86/x86_page_tables.go --iomap_file=$< --metadata_base_address=$(CONFIG_PAGE_TABLE_ADDR) --pdpt_output_c_file=$(obj)/pdpt_s3.c --pt_output_c_file=$(obj)/pt_s3.c + +cbfs-files-y += pt_s3 +pt_s3-file := $(obj)/pt_s3.c:struct +pt_s3-type := raw + +cbfs-files-y += pdpt_s3 +pdpt_s3-file := $(obj)/pdpt_s3.c:struct +pdpt_s3-type := raw + endif # ($(CONFIG_SOC_AMD_PICASSO),y) diff --git a/src/soc/amd/picasso/bootblock/bootblock.c b/src/soc/amd/picasso/bootblock/bootblock.c index a3935cc..ec30c19 100644 --- a/src/soc/amd/picasso/bootblock/bootblock.c +++ b/src/soc/amd/picasso/bootblock/bootblock.c @@ -4,6 +4,7 @@ #include <symbols.h> #include <bootblock_common.h> #include <console/console.h> +#include <cpu/x86/pae.h> #include <cpu/x86/cache.h> #include <cpu/x86/msr.h> #include <cpu/amd/msr.h> @@ -104,6 +105,17 @@ wrmsr(S3_RESUME_EIP_MSR, s3_resume_entry); }
+static void enable_paging(void) +{ + paging_set_nxe(1); + paging_set_default_pat(); + if (acpi_is_wakeup_s3()) { + printk(BIOS_DEBUG, "Enabling S3 page tables\n"); + paging_enable_for_car("pdpt_s3", "pt_s3"); + } +} + + asmlinkage void bootblock_c_entry(uint64_t base_timestamp) { set_caching(); @@ -123,5 +135,7 @@ u32 val = cpuid_eax(1); printk(BIOS_DEBUG, "Family_Model: %08x\n", val);
+ enable_paging(); + fch_early_init(); } diff --git a/src/soc/amd/picasso/chip.c b/src/soc/amd/picasso/chip.c index 2e5fae5..2abe54e 100644 --- a/src/soc/amd/picasso/chip.c +++ b/src/soc/amd/picasso/chip.c @@ -1,6 +1,8 @@ /* SPDX-License-Identifier: GPL-2.0-only */
+#include <bootstate.h> #include <console/console.h> +#include <cpu/x86/pae.h> #include <device/device.h> #include <device/pci.h> #include <drivers/i2c/designware/dw_i2c.h> @@ -173,9 +175,30 @@ southbridge_final(chip_info); }
+static void picasso_disable_paging(void *unused) +{ + printk(BIOS_INFO, "%s: Disable Paging\n", __func__); + paging_disable_pae(); +} + +static void picasso_os_entry(void *unused) +{ + picasso_disable_paging(NULL); +} + struct chip_operations soc_amd_picasso_ops = { CHIP_NAME("AMD Picasso SOC") .enable_dev = enable_dev, .init = soc_init, .final = soc_final }; + +BOOT_STATE_INIT_ENTRY( + BS_PAYLOAD_BOOT, + BS_ON_ENTRY, + picasso_disable_paging, NULL); + +BOOT_STATE_INIT_ENTRY( + BS_OS_RESUME, + BS_ON_ENTRY, + picasso_os_entry, NULL); diff --git a/src/soc/amd/picasso/memlayout.ld b/src/soc/amd/picasso/memlayout.ld index 6f43ba1..27c72a2 100644 --- a/src/soc/amd/picasso/memlayout.ld +++ b/src/soc/amd/picasso/memlayout.ld @@ -89,6 +89,9 @@ VERSTAGE(CONFIG_VERSTAGE_ADDR, CONFIG_VERSTAGE_SIZE) #endif
+ REGION(pagetables, CONFIG_PAGE_TABLE_ADDR, 4096 * 12, 8) + REGION(pdpt, ., 32, 32) + EARLY_RESERVED_DRAM_END(.)
RAMSTAGE(CONFIG_RAMBASE, 8M) diff --git a/src/soc/amd/picasso/page_map_s3.txt b/src/soc/amd/picasso/page_map_s3.txt new file mode 100644 index 0000000..a03796f --- /dev/null +++ b/src/soc/amd/picasso/page_map_s3.txt @@ -0,0 +1,7 @@ +0xd0000000, 0x100000000, UC, NX # All of MMIO +0xff000000, 0x100000000, WP, # memory-mapped SPI +0xcbd0b000, 0xd0000000, WB, # RAM for CBMEM and FSP +0x100000, 0x3BD000, WB, # RAM for lower memory usage +0xFF000, 0x100000, WB, # RAM for FSP AP Reset Vectors [0xFF000 - 0xFF1A4] +0xE0000, 0x100000, WB, # ACPI Reset Vector search area +0x30000, 0x40000, WB, # RAM for SIPI
Hello Patrick Georgi, Martin Roth, Furquan Shaikh, Julius Werner, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42888
to look at the new patch set (#2).
Change subject: soc/amd/picasso: Enable paging on S3 resume ......................................................................
soc/amd/picasso: Enable paging on S3 resume
This way we can prevent the BSP from accidentally stomping on OS memory.
BUG=b:159081993 TEST=S3 suspend and resume to OS. See paging enabled message. Ran suspend_stress_test for 20+ cycles.
Signed-off-by: Furquan Shaikh furquan@google.com Signed-off-by: Raul E Rangel rrangel@google.com Change-Id: I58fed6f297831b3cdc2758a5e7610388171d84d3 --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/bootblock/bootblock.c M src/soc/amd/picasso/chip.c M src/soc/amd/picasso/memlayout.ld A src/soc/amd/picasso/page_map_s3.txt 6 files changed, 67 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/42888/2
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42888 )
Change subject: soc/amd/picasso: Enable paging on S3 resume ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42888/2/src/soc/amd/picasso/page_ma... File src/soc/amd/picasso/page_map_s3.txt:
https://review.coreboot.org/c/coreboot/+/42888/2/src/soc/amd/picasso/page_ma... PS2, Line 3: 0xcbd0b000, 0xd0000000, WB, # RAM for CBMEM and FSP : 0x100000, 0x2BD000, WB, # RAM for lower memory usage I think ideally these would be runtime mapped.
Raul Rangel has removed Patrick Rudolph from this change. ( https://review.coreboot.org/c/coreboot/+/42888 )
Change subject: soc/amd/picasso: Enable paging on S3 resume ......................................................................
Removed reviewer Patrick Rudolph.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42888 )
Change subject: soc/amd/picasso: Enable paging on S3 resume ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42888/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42888/2//COMMIT_MSG@9 PS2, Line 9: This way we can prevent the BSP from accidentally stomping on OS memory. How is this Picasso specific? or is it an AGESA issue?
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Furquan Shaikh, Patrick Rudolph, Julius Werner, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42888
to look at the new patch set (#3).
Change subject: soc/amd/picasso: Enable paging on S3 resume ......................................................................
soc/amd/picasso: Enable paging on S3 resume
This way we can prevent the BSP from accidentally stomping on OS memory.
BUG=b:159081993 TEST=S3 suspend and resume to OS. See paging enabled message. Ran suspend_stress_test for 20+ cycles.
Signed-off-by: Furquan Shaikh furquan@google.com Signed-off-by: Raul E Rangel rrangel@google.com Change-Id: I58fed6f297831b3cdc2758a5e7610388171d84d3 --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/bootblock/bootblock.c M src/soc/amd/picasso/chip.c M src/soc/amd/picasso/memlayout.ld A src/soc/amd/picasso/page_map_s3.txt 6 files changed, 62 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/42888/3
Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/42888?usp=email )
Change subject: soc/amd/picasso: Enable paging on S3 resume ......................................................................
Abandoned
This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.
-
Martin L Roth (Code Review) -
Nico Huber (Code Review)
-
Raul Rangel (Code Review)