Change in coreboot[master]: RFC: soc/amd/picasso: Enable paging for boot

[Raul Rangel (Code Review)]

Hello Furquan Shaikh,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/coreboot/+/42889

to review the following change.

Change subject: RFC: soc/amd/picasso: Enable paging for boot
......................................................................

RFC: soc/amd/picasso: Enable paging for boot

This is WIP because I think we should dynamically map the 0-4KiB page
when we need to write to that range. Otherwise dereferencing a null
pointer is valid.

BUG=b:159081993
TEST=Boot trembyle to the OS.

Signed-off-by: Furquan Shaikh 
Signed-off-by: Raul E Rangel 
Change-Id: I5d027a71cd32f6240fa004d813513b84a1ecfd26
---
M src/arch/x86/ebda.c
M src/arch/x86/tables.c
M src/device/oprom/realmode/x86.c
M src/soc/amd/picasso/Makefile.inc
M src/soc/amd/picasso/bootblock/bootblock.c
A src/soc/amd/picasso/page_map.txt
6 files changed, 65 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/42889/1

diff --git a/src/arch/x86/ebda.c b/src/arch/x86/ebda.c
index ea2d9d3..9c54359 100644
--- a/src/arch/x86/ebda.c
+++ b/src/arch/x86/ebda.c
@@ -5,6 +5,7 @@
 #include <arch/ebda.h>
 #include <commonlib/endian.h>
 #include <console/console.h>
+#include <cpu/x86/pae.h>

static void *get_ebda_start(void)
 {
@@ -29,7 +30,13 @@
    ebda_kb = ebda_size >> 10;
    ebda = get_ebda_start();

-	/* clear BIOS DATA AREA */
+	/*
+	 * We disable paging because the BDA is located between 0 - 4KiB.
+	 * We don't want to map a page in this region because otherwise
+	 * 0 will become a valid memory address
+	 */
+	paging_disable_pae();
+
    zero_n(X86_BDA_BASE, X86_BDA_SIZE);

/* Avoid unaligned write16() since it's undefined behavior */
@@ -39,6 +46,8 @@
    /* Set up EBDA */
    zero_n(ebda, ebda_size);
    write_le16(ebda, ebda_kb);
+
+	paging_enable_pae();
 }

void setup_default_ebda(void)
diff --git a/src/arch/x86/tables.c b/src/arch/x86/tables.c
index 492674c..3e2f19a 100644
--- a/src/arch/x86/tables.c
+++ b/src/arch/x86/tables.c
@@ -11,6 +11,7 @@
 #include <string.h>
 #include <cbmem.h>
 #include <smbios.h>
+#include <cpu/x86/pae.h>

static unsigned long write_pirq_table(unsigned long rom_table_end)
 {
@@ -200,8 +201,17 @@
    if (CONFIG(GENERATE_SMBIOS_TABLES))
    	rom_table_end = write_smbios_table(rom_table_end);

+	/*
+	 * We disable paging because the forwarding table is located
+	 * between 0 - 4KiB. We don't want to map a page in this region because
+	 * otherwise 0 will become a valid memory address
+	 */
+	paging_disable_pae();
+
    sz = write_coreboot_forwarding_table(forwarding_table, coreboot_table);

+	paging_enable_pae();
+
    forwarding_table += sz;
    /* Align up to page boundary for historical consistency. */
    forwarding_table = ALIGN_UP(forwarding_table, 4*KiB);
diff --git a/src/device/oprom/realmode/x86.c b/src/device/oprom/realmode/x86.c
index 5215934..27bdf27 100644
--- a/src/device/oprom/realmode/x86.c
+++ b/src/device/oprom/realmode/x86.c
@@ -12,6 +12,7 @@
 #include <pc80/i8254.h>
 #include <string.h>
 #include <vbe.h>
+#include <cpu/x86/pae.h>

/* we use x86emu's register file representation */
 #include <x86emu/regs.h>
@@ -303,6 +304,8 @@
 {
    printk(BIOS_DEBUG, "VBE: Getting information about VESA mode %04x\n",
    	mi->video_mode);
+
+	paging_disable_pae();
    char *buffer = PTR_TO_REAL_MODE(__realmode_buffer);
    u16 buffer_seg = (((unsigned long)buffer) >> 4) & 0xff00;
    u16 buffer_adr = ((unsigned long)buffer) & 0xffff;
@@ -312,6 +315,9 @@
    	die("\nError: In %s function\n", __func__);
    memcpy(mi->mode_info_block, buffer, sizeof(mi->mode_info_block));
    mode_info_valid = 1;
+
+	paging_enable_pae();
+
    return 0;
 }

@@ -322,10 +328,12 @@
    mi->video_mode |= (1 << 14);
    // request clearing of framebuffer
    mi->video_mode &= ~(1 << 15);
+	paging_disable_pae();
    X86_EAX = realmode_interrupt(0x10, VESA_SET_MODE, mi->video_mode,
    		0x0000, 0x0000, 0x0000, 0x0000);
    if (vbe_check_for_failure(X86_AH))
    	die("\nError: In %s function\n", __func__);
+	paging_enable_pae();
    return 0;
 }

@@ -356,10 +364,13 @@
 void vbe_textmode_console(void)
 {
    delay(2);
+
+	paging_disable_pae();
    X86_EAX = realmode_interrupt(0x10, 0x0003, 0x0000, 0x0000,
    			0x0000, 0x0000, 0x0000);
    if (vbe_check_for_failure(X86_AH))
    	die("\nError: In %s function\n", __func__);
+	paging_enable_pae();
 }

int fill_lb_framebuffer(struct lb_framebuffer *framebuffer)
@@ -409,6 +420,13 @@
    /* Set up C interrupt handlers */
    setup_interrupt_handlers();

+	/*
+	 * We disable paging because the IDT and Option ROM are located between
+	 * 0 - 4KiB. We don't want to map a page in this region because
+	 * otherwise 0 will become a valid memory address.
+	 */
+	paging_disable_pae();
+
    /* Set up real-mode IDT */
    setup_realmode_idt();

@@ -419,6 +437,9 @@
    /* TODO ES:DI Pointer to System BIOS PnP Installation Check Structure */
    /* Option ROM entry point is at OPROM start + 3 */
    realmode_call(addr + 0x0003, num_dev, 0xffff, 0x0000, 0xffff, 0x0, 0x0);
+
+	paging_enable_pae();
+
    printk(BIOS_DEBUG, "... Option ROM returned.\n");

#if CONFIG(FRAMEBUFFER_SET_VESA_MODE)
diff --git a/src/soc/amd/picasso/Makefile.inc b/src/soc/amd/picasso/Makefile.inc
index 9d3bbd8..5b0be65 100644
--- a/src/soc/amd/picasso/Makefile.inc
+++ b/src/soc/amd/picasso/Makefile.inc
@@ -445,6 +445,17 @@

cpu_microcode_bins += $(wildcard 3rdparty/amd_blobs/picasso/PSP/UcodePatch_*.bin)

+$(obj)/pt.c $(obj)/pdpt.c: src/soc/amd/picasso/page_map.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.c --pt_output_c_file=$(obj)/pt.c
+
+cbfs-files-y += pt
+pt-file := $(obj)/pt.c:struct
+pt-type := raw
+
+cbfs-files-y += pdpt
+pdpt-file := $(obj)/pdpt.c:struct
+pdpt-type := raw
+
 $(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

diff --git a/src/soc/amd/picasso/bootblock/bootblock.c b/src/soc/amd/picasso/bootblock/bootblock.c
index ec30c19..b82f99f 100644
--- a/src/soc/amd/picasso/bootblock/bootblock.c
+++ b/src/soc/amd/picasso/bootblock/bootblock.c
@@ -112,6 +112,9 @@
    if (acpi_is_wakeup_s3()) {
    	printk(BIOS_DEBUG, "Enabling S3 page tables\n");
    	paging_enable_for_car("pdpt_s3", "pt_s3");
+	} else {
+		printk(BIOS_DEBUG, "Enabling page tables\n");
+		paging_enable_for_car("pdpt", "pt");
    }
 }

diff --git a/src/soc/amd/picasso/page_map.txt b/src/soc/amd/picasso/page_map.txt
new file mode 100644
index 0000000..0fd224b
--- /dev/null
+++ b/src/soc/amd/picasso/page_map.txt
@@ -0,0 +1,10 @@
+0xd0000000, 0x100000000, UC, NX # All of MMIO
+0xff000000, 0x100000000, WP, # memory-mapped SPI
+0xcbd0b000, 0xd0000000, WB, # RAM for CBMEM and FSP
+0x30000000, 0x3023f000, WB, # RAM for payload
+0x100000, 0x3BD000, WB, # RAM for lower memory usage
+0xc0000, 0x100000, WB, # RAM for option ROM + EBDA [0xf6000 - 0xf6400] + IDT [0x0xff065 + 0xff84d + 0xff841 + 0xfec59 + 0xfe739 + 0xff859 + 0xfe82e + 0xfefd2 + 0xffe6e]
+0xa0000, 0xc0000, UC, NX # VGA
+0x30000, 0x40000, WB, # RAM for SIPI
+0x1000, 0x2000, WB, # RAM for IDT stub code [0x1000 - 0x1900]
+# 0x0, 0x1000, WB, # RAM for BDA [0x400 - 0x600] + CB forwarding table [0x500 - 0x1000]

[Raul Rangel (Code Review)]

Hello Patrick Georgi, Martin Roth, Furquan Shaikh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/42889

to look at the new patch set (#2).

Change subject: RFC: soc/amd/picasso: Enable paging for boot
......................................................................

RFC: soc/amd/picasso: Enable paging for boot

This is WIP because I think we should dynamically map the 0-4KiB page
when we need to write to that range. Otherwise dereferencing a null
pointer is valid.

BUG=b:159081993
TEST=Boot trembyle to the OS.

Signed-off-by: Furquan Shaikh 
Signed-off-by: Raul E Rangel 
Change-Id: I5d027a71cd32f6240fa004d813513b84a1ecfd26
---
M src/arch/x86/ebda.c
M src/arch/x86/tables.c
M src/device/oprom/realmode/x86.c
M src/soc/amd/picasso/Makefile.inc
M src/soc/amd/picasso/bootblock/bootblock.c
A src/soc/amd/picasso/page_map.txt
6 files changed, 65 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/42889/2

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42889 )

Change subject: RFC: soc/amd/picasso: Enable paging for boot
......................................................................

Patch Set 2:

(3 comments)

https://review.coreboot.org/c/coreboot/+/42889/2/src/arch/x86/tables.c 
File src/arch/x86/tables.c:

https://review.coreboot.org/c/coreboot/+/42889/2/src/arch/x86/tables.c@213 
PS2, Line 213: paging_enable_pae
is this ok with nothing written to cr3?

https://review.coreboot.org/c/coreboot/+/42889/2/src/device/oprom/realmode/x... 
File src/device/oprom/realmode/x86.c:

https://review.coreboot.org/c/coreboot/+/42889/2/src/device/oprom/realmode/x... 
PS2, Line 308: paging_disable_pae();
Can this be done in a different patchset?

https://review.coreboot.org/c/coreboot/+/42889/2/src/soc/amd/picasso/page_ma... 
File src/soc/amd/picasso/page_map.txt:

https://review.coreboot.org/c/coreboot/+/42889/2/src/soc/amd/picasso/page_ma... 
PS2, Line 3: 0xcbd0b000, 0xd0000000, WB, # RAM for CBMEM and FSP
should this not be determined at runtime?

[Raul Rangel (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Furquan Shaikh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/42889

to look at the new patch set (#3).

Change subject: RFC: soc/amd/picasso: Enable paging for boot
......................................................................

RFC: soc/amd/picasso: Enable paging for boot

This is WIP because I think we should dynamically map the 0-4KiB page
when we need to write to that range. Otherwise dereferencing a null
pointer is valid.

BUG=b:159081993
TEST=Boot trembyle to the OS.

Signed-off-by: Furquan Shaikh 
Signed-off-by: Raul E Rangel 
Change-Id: I5d027a71cd32f6240fa004d813513b84a1ecfd26
---
M src/arch/x86/ebda.c
M src/arch/x86/tables.c
M src/device/oprom/realmode/x86.c
M src/soc/amd/picasso/Makefile.inc
M src/soc/amd/picasso/bootblock/bootblock.c
A src/soc/amd/picasso/page_map.txt
6 files changed, 65 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/42889/3

[M] Change in coreboot[master]: RFC: soc/amd/picasso: Enable paging for boot

[Martin L Roth (Code Review)]

Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/42889?usp=email )

Change subject: RFC: soc/amd/picasso: Enable paging for boot
......................................................................

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.