Change in coreboot[master]: WIP: riscv/mb/qemu: fix DRAM probing

[Philipp Hug (Code Review)]

Philipp Hug has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36486 )


Change subject: WIP: riscv/mb/qemu: fix DRAM probing
......................................................................

WIP: riscv/mb/qemu: fix DRAM probing

Current version of qemu raise an exception when accessing invalid memory.
Modify the probing code to temporary redirect the exception handler like on
ARM platform.

TEST=qemu detects RAM size correctly
Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Signed-off-by: Philipp Hug <[e-mail]>
---
M src/arch/riscv/Makefile.inc
M src/arch/riscv/include/arch/exception.h
A src/arch/riscv/ramdetect.c
M src/arch/riscv/trap_handler.c
M src/arch/riscv/trap_util.S
M src/lib/ramdetect.c
6 files changed, 57 insertions(+), 3 deletions(-)



  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/36486/1

diff --git a/src/arch/riscv/Makefile.inc b/src/arch/riscv/Makefile.inc
index 0039fab..7465a8f 100644
--- a/src/arch/riscv/Makefile.inc
+++ b/src/arch/riscv/Makefile.inc
@@ -101,6 +101,7 @@
 romstage-y += stages.c
 romstage-y += misc.c
 romstage-$(ARCH_RISCV_PMP) += pmp.c
+romstage-y += ramdetect.c
 romstage-y += smp.c
 romstage-y += \
 	$(top)/src/lib/memchr.c \
@@ -142,6 +143,7 @@
 ramstage-y += virtual_memory.c
 ramstage-y += stages.c
 ramstage-y += misc.c
+ramstage-y += ramdetect.c
 ramstage-y += smp.c
 ramstage-y += boot.c
 ramstage-y += tables.c
diff --git a/src/arch/riscv/include/arch/exception.h b/src/arch/riscv/include/arch/exception.h
index 6fbbdf0..cdca582 100644
--- a/src/arch/riscv/include/arch/exception.h
+++ b/src/arch/riscv/include/arch/exception.h
@@ -53,7 +53,7 @@
 }
 
 void redirect_trap(void);
-void trap_handler(trapframe *tf);
+void default_trap_handler(trapframe *tf);
 void handle_supervisor_call(trapframe *tf);
 void handle_misaligned(trapframe *tf);
 
diff --git a/src/arch/riscv/ramdetect.c b/src/arch/riscv/ramdetect.c
new file mode 100644
index 0000000..47153ab
--- /dev/null
+++ b/src/arch/riscv/ramdetect.c
@@ -0,0 +1,46 @@
+/*
+ * This file is part of the coreboot project.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include <arch/exception.h>
+#include <types.h>
+#include <console/console.h>
+#include <device/mmio.h>
+#include <ramdetect.h>
+#include <arch/smp/spinlock.h>
+
+static enum {
+	ABORT_CHECKER_NOT_TRIGGERED,
+	ABORT_CHECKER_TRIGGERED,
+} abort_state = ABORT_CHECKER_NOT_TRIGGERED;
+
+extern void(*trap_handler)(trapframe *tf);
+
+#define insn_size 4
+static void ramcheck_trap_handler(trapframe *tf)
+{
+	printk(BIOS_DEBUG, "TRAP 0x%lx!!!\n", tf->epc);
+	abort_state = ABORT_CHECKER_TRIGGERED;
+
+	/*
+	 * skip read instruction.
+	 * currenctly hardcoded to 32bit instruction size
+	 */
+	write_csr(mepc, read_csr(mepc) + insn_size);
+}
+
+int probe_mb(const uintptr_t dram_start, const uintptr_t size)
+{
+	uintptr_t addr = dram_start + (size * MiB) - sizeof(uint32_t);
+	void *ptr = (void *)addr;
+
+	abort_state = ABORT_CHECKER_NOT_TRIGGERED;
+	trap_handler = ramcheck_trap_handler;
+	barrier();
+	read32(ptr);
+	trap_handler = default_trap_handler;
+	barrier();
+	return abort_state == ABORT_CHECKER_NOT_TRIGGERED;
+}
diff --git a/src/arch/riscv/trap_handler.c b/src/arch/riscv/trap_handler.c
index 6b39fab..1d51268 100644
--- a/src/arch/riscv/trap_handler.c
+++ b/src/arch/riscv/trap_handler.c
@@ -118,7 +118,10 @@
 		break;
 	}
 }
-void trap_handler(trapframe *tf)
+
+void(*trap_handler)(trapframe *tf) = default_trap_handler;
+
+void default_trap_handler(trapframe *tf)
 {
 	write_csr(mscratch, tf);
 	if (tf->cause & 0x8000000000000000ULL) {
diff --git a/src/arch/riscv/trap_util.S b/src/arch/riscv/trap_util.S
index 67e917c..93f5afd 100644
--- a/src/arch/riscv/trap_util.S
+++ b/src/arch/riscv/trap_util.S
@@ -120,7 +120,8 @@
 	save_tf
 
 	mv	a0, sp
-	jal	trap_handler
+	ld	t0, trap_handler
+	jalr	t0
 
 	restore_regs
 	addi	sp, sp, MENTRY_FRAME_SIZE
diff --git a/src/lib/ramdetect.c b/src/lib/ramdetect.c
index 2c83092..eaf6190 100644
--- a/src/lib/ramdetect.c
+++ b/src/lib/ramdetect.c
@@ -58,6 +58,8 @@
 	if (saved_result)
 		return saved_result;
 
+	printk(BIOS_DEBUG, "RAMDETECT: Starting\n");
+
 	/* Find the MSB + 1. */
 	size_t tmp = probe_size;
 	do {

-- 
To view, visit https://review.coreboot.org/c/coreboot/+/36486
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Gerrit-Change-Number: 36486
Gerrit-PatchSet: 1
Gerrit-Owner: Philipp Hug <[e-mail]>
Gerrit-MessageType: newchange

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36486 )

Change subject: WIP: riscv/mb/qemu: fix DRAM probing
......................................................................


Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/36486/1/src/arch/riscv/ramdetect.c 
File src/arch/riscv/ramdetect.c:

https://review.coreboot.org/c/coreboot/+/36486/1/src/arch/riscv/ramdetect.c@... 
PS1, Line 19: extern void(*trap_handler)(trapframe *tf);
missing space after return type


https://review.coreboot.org/c/coreboot/+/36486/1/src/arch/riscv/trap_handler... 
File src/arch/riscv/trap_handler.c:

https://review.coreboot.org/c/coreboot/+/36486/1/src/arch/riscv/trap_handler... 
PS1, Line 122: void(*trap_handler)(trapframe *tf) = default_trap_handler;
missing space after return type



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/36486
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Gerrit-Change-Number: 36486
Gerrit-PatchSet: 1
Gerrit-Owner: Philipp Hug <[e-mail]>
Gerrit-Reviewer: Martin Roth <[e-mail]>
Gerrit-Reviewer: Patrick Georgi <[e-mail]>
Gerrit-Reviewer: Philipp Hug <[e-mail]>
Gerrit-Reviewer: ron minnich <[e-mail]>
Gerrit-CC: build bot (Jenkins) <[e-mail]>
Gerrit-Comment-Date: Wed, 30 Oct 2019 16:38:15 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[Philipp Hug (Code Review)]

Hello ron minnich, Patrick Georgi, Martin Roth, 

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

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

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

Change subject: WIP: riscv/mb/qemu: fix DRAM probing
......................................................................

WIP: riscv/mb/qemu: fix DRAM probing

Current version of qemu raise an exception when accessing invalid memory.
Modify the probing code to temporary redirect the exception handler like on
ARM platform.

TEST=qemu detects RAM size correctly
Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Signed-off-by: Philipp Hug <[e-mail]>
---
M src/arch/riscv/Makefile.inc
M src/arch/riscv/include/arch/exception.h
A src/arch/riscv/ramdetect.c
M src/arch/riscv/trap_handler.c
M src/arch/riscv/trap_util.S
M src/lib/ramdetect.c
6 files changed, 57 insertions(+), 3 deletions(-)


  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/36486/2
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/36486
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Gerrit-Change-Number: 36486
Gerrit-PatchSet: 2
Gerrit-Owner: Philipp Hug <[e-mail]>
Gerrit-Reviewer: Martin Roth <[e-mail]>
Gerrit-Reviewer: Patrick Georgi <[e-mail]>
Gerrit-Reviewer: Philipp Hug <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Reviewer: ron minnich <[e-mail]>
Gerrit-MessageType: newpatchset

[Marc Karasek (Code Review)]

Marc Karasek has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36486 )

Change subject: WIP: riscv/mb/qemu: fix DRAM probing
......................................................................


Patch Set 2: Code-Review-1

This still relies on the Kconfig value matching what is on the cmdline for qemu

https://review.coreboot.org/c/coreboot/+/38904 allows you to specify the DDR size so 
at least you can match what the qemu line with the coreboot code.  Not dynamic but better than nothing


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/36486
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Gerrit-Change-Number: 36486
Gerrit-PatchSet: 2
Gerrit-Owner: Philipp Hug <[e-mail]>
Gerrit-Reviewer: Marc Karasek <[e-mail]>
Gerrit-Reviewer: Martin Roth <[e-mail]>
Gerrit-Reviewer: Patrick Georgi <[e-mail]>
Gerrit-Reviewer: Philipp Hug <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Reviewer: ron minnich <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Fri, 14 Feb 2020 19:31:12 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Marc Karasek (Code Review)]

Marc Karasek has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36486 )

Change subject: WIP: riscv/mb/qemu: fix DRAM probing
......................................................................


Patch Set 2:

How did you get this to compile?  There is no trap_handler.o in romstage and if you add it it fails to build.  handle_sbi is undefined..


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/36486
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Gerrit-Change-Number: 36486
Gerrit-PatchSet: 2
Gerrit-Owner: Philipp Hug <[e-mail]>
Gerrit-Reviewer: Marc Karasek <[e-mail]>
Gerrit-Reviewer: Martin Roth <[e-mail]>
Gerrit-Reviewer: Patrick Georgi <[e-mail]>
Gerrit-Reviewer: Philipp Hug <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Reviewer: ron minnich <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Fri, 14 Feb 2020 22:24:18 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[Marc Karasek (Code Review)]

Marc Karasek has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36486 )

Change subject: WIP: riscv/mb/qemu: fix DRAM probing
......................................................................


Patch Set 2:

We do not need to scan when using qemu.  The ddr size is known and can be a fixed value.

See
https://review.coreboot.org/c/coreboot/+/38904

This makes much more sense to have an option to compile coreboot for qemu-riscv with
the value you want to use then specify this (or a smaller value) on the cmdline. 
No need to muck with exception handlers, etc..


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/36486
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Gerrit-Change-Number: 36486
Gerrit-PatchSet: 2
Gerrit-Owner: Philipp Hug <[e-mail]>
Gerrit-Reviewer: Marc Karasek <[e-mail]>
Gerrit-Reviewer: Martin Roth <[e-mail]>
Gerrit-Reviewer: Patrick Georgi <[e-mail]>
Gerrit-Reviewer: Philipp Hug <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Reviewer: ron minnich <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Mon, 17 Feb 2020 14:51:17 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36486 )

Change subject: WIP: riscv/mb/qemu: fix DRAM probing
......................................................................


Patch Set 2:

It looks like there's a bug in qemu as mepc seems to have no influence on pc on mret. @Philipp any idea what could be wrong?


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/36486
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Gerrit-Change-Number: 36486
Gerrit-PatchSet: 2
Gerrit-Owner: Philipp Hug <[e-mail]>
Gerrit-Reviewer: Marc Karasek <[e-mail]>
Gerrit-Reviewer: Martin Roth <[e-mail]>
Gerrit-Reviewer: Patrick Georgi <[e-mail]>
Gerrit-Reviewer: Philipp Hug <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Reviewer: ron minnich <[e-mail]>
Gerrit-CC: Patrick Rudolph <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Tue, 18 Feb 2020 07:58:01 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[Arthur Heymans (Code Review)]

Attention is currently required from: Philipp Hug.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36486 )

Change subject: WIP: riscv/mb/qemu: fix DRAM probing
......................................................................


Patch Set 2:

(1 comment)

Patchset:

PS2: 
I tried rebasing this on master but it now fails to update the trap handler. With the same compiler this still works. I'm at a loss here... Do you have any idea?



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/36486
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Gerrit-Change-Number: 36486
Gerrit-PatchSet: 2
Gerrit-Owner: Philipp Hug <[e-mail]>
Gerrit-Reviewer: Marc Karasek <[e-mail]>
Gerrit-Reviewer: Martin L Roth <[e-mail]>
Gerrit-Reviewer: Philipp Hug <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Reviewer: ron minnich <[e-mail]>
Gerrit-CC: Arthur Heymans <[e-mail]>
Gerrit-CC: Patrick Rudolph <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Attention: Philipp Hug <[e-mail]>
Gerrit-Comment-Date: Fri, 13 May 2022 12:16:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[Arthur Heymans (Code Review)]

Attention is currently required from: Philipp Hug.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36486 )

Change subject: WIP: riscv/mb/qemu: fix DRAM probing
......................................................................


Patch Set 2:

(1 comment)

Patchset:

PS2:
> I tried rebasing this on master but it now fails to update the trap handler. With the same compiler this still works. I'm at a loss here... Do you have any idea?

nvm. It looks like the exception handler does not get installed in romstage, so it's using the bootblock one...



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/36486
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Gerrit-Change-Number: 36486
Gerrit-PatchSet: 2
Gerrit-Owner: Philipp Hug <[e-mail]>
Gerrit-Reviewer: Marc Karasek <[e-mail]>
Gerrit-Reviewer: Martin L Roth <[e-mail]>
Gerrit-Reviewer: Philipp Hug <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Reviewer: ron minnich <[e-mail]>
Gerrit-CC: Arthur Heymans <[e-mail]>
Gerrit-CC: Patrick Rudolph <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Attention: Philipp Hug <[e-mail]>
Gerrit-Comment-Date: Fri, 13 May 2022 12:19:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Arthur Heymans <[e-mail]>
Gerrit-MessageType: comment

[M] Change in coreboot[master]: riscv/mb/qemu: fix DRAM probing

[Arthur Heymans (Code Review)]

Attention is currently required from: Philipp Hug, Arthur Heymans.

Arthur Heymans has uploaded a new patch set (#3) to the change originally created by Philipp Hug. ( https://review.coreboot.org/c/coreboot/+/36486 )

Change subject: riscv/mb/qemu: fix DRAM probing
......................................................................

riscv/mb/qemu: fix DRAM probing

Current version of qemu raise an exception when accessing invalid memory.
Modify the probing code to temporary redirect the exception handler like on
ARM platform.

TEST=qemu detects RAM size correctly
Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Signed-off-by: Philipp Hug <[e-mail]>
Signed-off-by: Arthur Heymans <[e-mail]>
---
M src/arch/riscv/Makefile.inc
M src/arch/riscv/include/arch/exception.h
A src/arch/riscv/ramdetect.c
M src/arch/riscv/trap_handler.c
M src/arch/riscv/trap_util.S
M src/lib/imd_cbmem.c
M src/lib/ramdetect.c
7 files changed, 75 insertions(+), 4 deletions(-)


  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/36486/3
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/36486
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Gerrit-Change-Number: 36486
Gerrit-PatchSet: 3
Gerrit-Owner: Philipp Hug <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Marc Karasek <[e-mail]>
Gerrit-Reviewer: Martin L Roth <[e-mail]>
Gerrit-Reviewer: Philipp Hug <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Reviewer: ron minnich <[e-mail]>
Gerrit-CC: Arthur Heymans <[e-mail]>
Gerrit-CC: Patrick Rudolph <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Attention: Philipp Hug <[e-mail]>
Gerrit-Attention: Arthur Heymans <[e-mail]>
Gerrit-MessageType: newpatchset

[M] Change in coreboot[master]: riscv/mb/qemu: fix DRAM probing

[build bot (Jenkins) (Code Review)]

Attention is currently required from: Philipp Hug, Arthur Heymans.

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36486 )

Change subject: riscv/mb/qemu: fix DRAM probing
......................................................................


Patch Set 3:

(5 comments)

Commit Message:

Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-161259):
https://review.coreboot.org/c/coreboot/+/36486/comment/992a9596_c916d1b2 
PS3, Line 8: 
Possible unwrapped commit description (prefer a maximum 72 chars per line)


Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-161259):
https://review.coreboot.org/c/coreboot/+/36486/comment/fc078581_033e0937 
PS3, Line 9: Current version of qemu raise an exception when accessing invalid memory.
Possible unwrapped commit description (prefer a maximum 72 chars per line)


File src/arch/riscv/ramdetect.c:

Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-161259):
https://review.coreboot.org/c/coreboot/+/36486/comment/3068a977_0967d5e8 
PS3, Line 19: extern void(*trap_handler)(trapframe *tf);
missing space after return type


Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-161259):
https://review.coreboot.org/c/coreboot/+/36486/comment/9c1e1bd2_ee9d0878 
PS3, Line 44: 	printk(BIOS_DEBUG, "%lx is %s DRAM\n", dram_start + size * MiB, abort_state == ABORT_CHECKER_NOT_TRIGGERED ? "" : "not");
line length of 129 exceeds 96 columns


File src/arch/riscv/trap_handler.c:

Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-161259):
https://review.coreboot.org/c/coreboot/+/36486/comment/3a3ad6d2_7f9dadc9 
PS3, Line 112: void(*trap_handler)(trapframe *tf) = default_trap_handler;
missing space after return type



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/36486
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Gerrit-Change-Number: 36486
Gerrit-PatchSet: 3
Gerrit-Owner: Philipp Hug <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Marc Karasek <[e-mail]>
Gerrit-Reviewer: Martin L Roth <[e-mail]>
Gerrit-Reviewer: Philipp Hug <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Reviewer: ron minnich <[e-mail]>
Gerrit-CC: Arthur Heymans <[e-mail]>
Gerrit-CC: Patrick Rudolph <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Attention: Philipp Hug <[e-mail]>
Gerrit-Attention: Arthur Heymans <[e-mail]>
Gerrit-Comment-Date: Tue, 25 Oct 2022 19:19:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[M] Change in coreboot[main]: riscv/mb/qemu: fix DRAM probing

[Philipp Hug (Code Review)]

Attention is currently required from: Arthur Heymans, Philipp Hug.

Hello Arthur Heymans, Marc Karasek, Martin L Roth, build bot (Jenkins), ron minnich, 

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

    https://review.coreboot.org/c/coreboot/+/36486?usp=email

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


Change subject: riscv/mb/qemu: fix DRAM probing
......................................................................

riscv/mb/qemu: fix DRAM probing

Current version of qemu raise an exception when accessing invalid memory.
Modify the probing code to temporary redirect the exception handler like on
ARM platform.

TEST=qemu detects RAM size correctly
Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Signed-off-by: Philipp Hug <[e-mail]>
Signed-off-by: Arthur Heymans <[e-mail]>
---
M src/arch/riscv/Makefile.mk
M src/arch/riscv/include/arch/exception.h
A src/arch/riscv/ramdetect.c
M src/arch/riscv/trap_handler.c
M src/arch/riscv/trap_util.S
M src/lib/imd_cbmem.c
M src/lib/ramdetect.c
7 files changed, 59 insertions(+), 4 deletions(-)


  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/36486/5
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/36486?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Gerrit-Change-Number: 36486
Gerrit-PatchSet: 5
Gerrit-Owner: Philipp Hug <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Marc Karasek <[e-mail]>
Gerrit-Reviewer: Martin L Roth <[e-mail]>
Gerrit-Reviewer: Philipp Hug <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Reviewer: ron minnich <[e-mail]>
Gerrit-CC: Arthur Heymans <[e-mail]>
Gerrit-CC: Patrick Rudolph <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-CC: Stefan Reinauer <[e-mail]>
Gerrit-Attention: Philipp Hug <[e-mail]>
Gerrit-Attention: Arthur Heymans <[e-mail]>
Gerrit-MessageType: newpatchset

[M] Change in coreboot[main]: riscv/mb/qemu: fix DRAM probing

[Philipp Hug (Code Review)]

Attention is currently required from: Arthur Heymans, Philipp Hug.

Hello Arthur Heymans, Marc Karasek, Martin L Roth, build bot (Jenkins), ron minnich, 

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

    https://review.coreboot.org/c/coreboot/+/36486?usp=email

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


Change subject: riscv/mb/qemu: fix DRAM probing
......................................................................

riscv/mb/qemu: fix DRAM probing

Current version of qemu raise an exception when accessing invalid memory.
Modify the probing code to temporary redirect the exception handler like on
ARM platform.
Also move saving of the stack frame out to trap_util.S to have all at the same
place for a future rewrite.

TEST=boots to ramstage
Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Signed-off-by: Philipp Hug <[e-mail]>
Signed-off-by: Arthur Heymans <[e-mail]>
---
M src/arch/riscv/Makefile.mk
M src/arch/riscv/include/arch/exception.h
A src/arch/riscv/ramdetect.c
M src/arch/riscv/trap_handler.c
M src/arch/riscv/trap_util.S
M src/lib/imd_cbmem.c
M src/lib/ramdetect.c
7 files changed, 63 insertions(+), 5 deletions(-)


  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/36486/7
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/36486?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Gerrit-Change-Number: 36486
Gerrit-PatchSet: 7
Gerrit-Owner: Philipp Hug <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Marc Karasek <[e-mail]>
Gerrit-Reviewer: Martin L Roth <[e-mail]>
Gerrit-Reviewer: Philipp Hug <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Reviewer: ron minnich <[e-mail]>
Gerrit-CC: Arthur Heymans <[e-mail]>
Gerrit-CC: Patrick Rudolph <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-CC: Stefan Reinauer <[e-mail]>
Gerrit-Attention: Philipp Hug <[e-mail]>
Gerrit-Attention: Arthur Heymans <[e-mail]>
Gerrit-MessageType: newpatchset

[M] Change in coreboot[main]: riscv/mb/qemu: fix DRAM probing

[Philipp Hug (Code Review)]

Attention is currently required from: Arthur Heymans, Philipp Hug.

Hello Arthur Heymans, Marc Karasek, Martin L Roth, build bot (Jenkins), ron minnich, 

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

    https://review.coreboot.org/c/coreboot/+/36486?usp=email

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


Change subject: riscv/mb/qemu: fix DRAM probing
......................................................................

riscv/mb/qemu: fix DRAM probing

Current version of qemu raise an exception when accessing invalid memory.
Modify the probing code to temporary redirect the exception handler like on
ARM platform.
Also move saving of the stack frame out to trap_util.S to have all at the same
place for a future rewrite.

TEST=boots to ramstage
Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Signed-off-by: Philipp Hug <[e-mail]>
Signed-off-by: Arthur Heymans <[e-mail]>
---
M src/arch/riscv/Makefile.mk
M src/arch/riscv/include/arch/exception.h
A src/arch/riscv/ramdetect.c
M src/arch/riscv/trap_handler.c
M src/arch/riscv/trap_util.S
M src/lib/imd_cbmem.c
M src/lib/ramdetect.c
7 files changed, 63 insertions(+), 5 deletions(-)


  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/36486/9
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/36486?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Gerrit-Change-Number: 36486
Gerrit-PatchSet: 9
Gerrit-Owner: Philipp Hug <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Marc Karasek <[e-mail]>
Gerrit-Reviewer: Martin L Roth <[e-mail]>
Gerrit-Reviewer: Philipp Hug <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Reviewer: ron minnich <[e-mail]>
Gerrit-CC: Arthur Heymans <[e-mail]>
Gerrit-CC: Patrick Rudolph <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-CC: Stefan Reinauer <[e-mail]>
Gerrit-Attention: Philipp Hug <[e-mail]>
Gerrit-Attention: Arthur Heymans <[e-mail]>
Gerrit-MessageType: newpatchset

[M] Change in coreboot[main]: riscv/mb/qemu: fix DRAM probing

[Philipp Hug (Code Review)]

Attention is currently required from: Arthur Heymans, Arthur Heymans.

Philipp Hug has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36486?usp=email )

Change subject: riscv/mb/qemu: fix DRAM probing
......................................................................


Patch Set 9:

(1 comment)

Patchset:

PS2:
> > I tried rebasing this on master but it now fails to update the trap handler. […]
> > is this still an issue?

[M] Change in coreboot[main]: riscv/mb/qemu: fix DRAM probing

[ron minnich (Code Review)]

Attention is currently required from: Arthur Heymans, Arthur Heymans, Philipp Hug.

ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36486?usp=email )

Change subject: riscv/mb/qemu: fix DRAM probing
......................................................................


Patch Set 9:

(4 comments)

Patchset:

PS9: 
I had a few questions.


File src/arch/riscv/ramdetect.c:

https://review.coreboot.org/c/coreboot/+/36486/comment/31d456c6_f61d5590 :
PS9, Line 21: #define insn_size 4
Is this true in an era of compact instruction extension? I no longer know.
Should insn_size be define somewhere in <arch/...>?


https://review.coreboot.org/c/coreboot/+/36486/comment/19ace76c_4b7b7f97 :
PS9, Line 29: 	 */
Can we ensure we never build with compact instrutions enabled? I'd be fine with that.


https://review.coreboot.org/c/coreboot/+/36486/comment/f2207e23_717c9969 :
PS9, Line 35: 	uintptr_t addr = dram_start + (size * MiB) - sizeof(uint32_t);
Should we make sure dram_start is 32-bit aligned, in case of someone doing something silly? it would be a pain to take an alignment trap here. Or should the probe be for read8, just to be sure?



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/36486?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Gerrit-Change-Number: 36486
Gerrit-PatchSet: 9
Gerrit-Owner: Philipp Hug <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Marc Karasek <[e-mail]>
Gerrit-Reviewer: Martin L Roth <[e-mail]>
Gerrit-Reviewer: Philipp Hug <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Reviewer: ron minnich <[e-mail]>
Gerrit-CC: Arthur Heymans <[e-mail]>
Gerrit-CC: Patrick Rudolph <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-CC: Stefan Reinauer <[e-mail]>
Gerrit-CC: ron minnich
Gerrit-Attention: Arthur Heymans <[e-mail]>
Gerrit-Attention: Philipp Hug <[e-mail]>
Gerrit-Attention: Arthur Heymans <[e-mail]>
Gerrit-Comment-Date: Sat, 03 Feb 2024 19:00:53 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[M] Change in coreboot[main]: riscv/mb/qemu: fix DRAM probing

[Philipp Hug (Code Review)]

Attention is currently required from: Arthur Heymans, Arthur Heymans, ron minnich.

Philipp Hug has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36486?usp=email )

Change subject: riscv/mb/qemu: fix DRAM probing
......................................................................


Patch Set 9:

(3 comments)

File src/arch/riscv/ramdetect.c:

https://review.coreboot.org/c/coreboot/+/36486/comment/8adfa408_a6320919 :
PS9, Line 21: #define insn_size 4
> Is this true in an era of compact instruction extension? I no longer know. […]
> we can't. to be really sure we'd have to read the instruction from memory. so this is a hack that works as long as we have 4 byte instructions.

https://review.coreboot.org/c/coreboot/+/36486/comment/07dcef89_da93a493 :
PS9, Line 29: 	 */
> Can we ensure we never build with compact instrutions enabled? I'd be fine with that.
> see above.

https://review.coreboot.org/c/coreboot/+/36486/comment/d22342f7_1cc2e61b :
PS9, Line 35: 	uintptr_t addr = dram_start + (size * MiB) - sizeof(uint32_t);
> Should we make sure dram_start is 32-bit aligned, in case of someone doing something silly? it would […]
> dram_start is coming from the linker file and we assume that memory is usually aligned, if not I'd rather have an exception during probing, I think.

[M] Change in coreboot[main]: riscv/mb/qemu: fix DRAM probing

[ron minnich (Code Review)]

Attention is currently required from: Arthur Heymans, Arthur Heymans, Philipp Hug, ron minnich.

ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36486?usp=email )

Change subject: riscv/mb/qemu: fix DRAM probing
......................................................................


Patch Set 9:

(3 comments)

File src/arch/riscv/ramdetect.c:

https://review.coreboot.org/c/coreboot/+/36486/comment/d9997f14_5282dd10 :
PS9, Line 21: #define insn_size 4
> we can't. to be really sure we'd have to read the instruction from memory. […]
> Acknowledged

https://review.coreboot.org/c/coreboot/+/36486/comment/1ee489bb_9c0ad95e :
PS9, Line 29: 	 */
> see above.
> Acknowledged

https://review.coreboot.org/c/coreboot/+/36486/comment/9d0ceb5e_573ddc61 :
PS9, Line 35: 	uintptr_t addr = dram_start + (size * MiB) - sizeof(uint32_t);
> dram_start is coming from the linker file and we assume that memory is usually aligned, if not I'd r […]
> Acknowledged

[M] Change in coreboot[main]: riscv/mb/qemu: fix DRAM probing

[Philipp Hug (Code Review)]

Attention is currently required from: Arthur Heymans, Arthur Heymans, Philipp Hug, ron minnich.

Hello Arthur Heymans, Marc Karasek, Martin L Roth, build bot (Jenkins), ron minnich, 

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

    https://review.coreboot.org/c/coreboot/+/36486?usp=email

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


Change subject: riscv/mb/qemu: fix DRAM probing
......................................................................

riscv/mb/qemu: fix DRAM probing

Current version of qemu raise an exception when accessing invalid memory.
Modify the probing code to temporary redirect the exception handler like on
ARM platform.
Also move saving of the stack frame out to trap_util.S to have all at the same
place for a future rewrite.

TEST=boots to ramstage
Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Signed-off-by: Philipp Hug <[e-mail]>
Signed-off-by: Arthur Heymans <[e-mail]>
clang-formatted-by: Your Name
---
M src/arch/riscv/Makefile.mk
M src/arch/riscv/include/arch/exception.h
A src/arch/riscv/ramdetect.c
M src/arch/riscv/trap_handler.c
M src/arch/riscv/trap_util.S
M src/lib/imd_cbmem.c
M src/lib/ramdetect.c
7 files changed, 80 insertions(+), 5 deletions(-)


  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/36486/10
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/36486?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Gerrit-Change-Number: 36486
Gerrit-PatchSet: 10
Gerrit-Owner: Philipp Hug <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Marc Karasek <[e-mail]>
Gerrit-Reviewer: Martin L Roth <[e-mail]>
Gerrit-Reviewer: Philipp Hug <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Reviewer: ron minnich <[e-mail]>
Gerrit-CC: Arthur Heymans <[e-mail]>
Gerrit-CC: Patrick Rudolph <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-CC: Stefan Reinauer <[e-mail]>
Gerrit-CC: ron minnich
Gerrit-Attention: Arthur Heymans <[e-mail]>
Gerrit-Attention: Philipp Hug <[e-mail]>
Gerrit-Attention: ron minnich
Gerrit-Attention: Arthur Heymans <[e-mail]>
Gerrit-MessageType: newpatchset

[M] Change in coreboot[main]: riscv/mb/qemu: fix DRAM probing

[Philipp Hug (Code Review)]

Attention is currently required from: Arthur Heymans, Arthur Heymans, Philipp Hug, ron minnich.

Hello Arthur Heymans, Marc Karasek, Martin L Roth, build bot (Jenkins), ron minnich, 

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

    https://review.coreboot.org/c/coreboot/+/36486?usp=email

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


Change subject: riscv/mb/qemu: fix DRAM probing
......................................................................

riscv/mb/qemu: fix DRAM probing

Current version of qemu raise an exception when accessing invalid memory.
Modify the probing code to temporary redirect the exception handler like on
ARM platform.
Also move saving of the stack frame out to trap_util.S to have all at the same
place for a future rewrite.

TEST=boots to ramstage
Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Signed-off-by: Philipp Hug <[e-mail]>
Signed-off-by: Arthur Heymans <[e-mail]>
---
M src/arch/riscv/Makefile.mk
M src/arch/riscv/include/arch/exception.h
A src/arch/riscv/ramdetect.c
M src/arch/riscv/trap_handler.c
M src/arch/riscv/trap_util.S
M src/lib/imd_cbmem.c
M src/lib/ramdetect.c
7 files changed, 95 insertions(+), 19 deletions(-)


  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/36486/11
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/36486?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Gerrit-Change-Number: 36486
Gerrit-PatchSet: 11
Gerrit-Owner: Philipp Hug <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Marc Karasek <[e-mail]>
Gerrit-Reviewer: Martin L Roth <[e-mail]>
Gerrit-Reviewer: Philipp Hug <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Reviewer: ron minnich <[e-mail]>
Gerrit-CC: Arthur Heymans <[e-mail]>
Gerrit-CC: Patrick Rudolph <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-CC: Stefan Reinauer <[e-mail]>
Gerrit-CC: ron minnich
Gerrit-Attention: Arthur Heymans <[e-mail]>
Gerrit-Attention: Philipp Hug <[e-mail]>
Gerrit-Attention: ron minnich
Gerrit-Attention: Arthur Heymans <[e-mail]>
Gerrit-MessageType: newpatchset

[M] Change in coreboot[main]: riscv/mb/qemu: fix DRAM probing

[Philipp Hug (Code Review)]

Attention is currently required from: Arthur Heymans, Arthur Heymans, Philipp Hug, ron minnich.

Hello Arthur Heymans, Marc Karasek, Martin L Roth, build bot (Jenkins), ron minnich, 

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

    https://review.coreboot.org/c/coreboot/+/36486?usp=email

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


Change subject: riscv/mb/qemu: fix DRAM probing
......................................................................

riscv/mb/qemu: fix DRAM probing

Current version of qemu raise an exception when accessing invalid memory.
Modify the probing code to temporary redirect the exception handler like on
ARM platform.
Also move saving of the stack frame out to trap_util.S to have all at the same
place for a future rewrite.

TEST=boots to ramstage
Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Signed-off-by: Philipp Hug <[e-mail]>
Signed-off-by: Arthur Heymans <[e-mail]>
---
M src/arch/riscv/Makefile.mk
M src/arch/riscv/include/arch/exception.h
A src/arch/riscv/ramdetect.c
M src/arch/riscv/trap_handler.c
M src/arch/riscv/trap_util.S
M src/lib/imd_cbmem.c
6 files changed, 93 insertions(+), 19 deletions(-)


  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/36486/12
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/36486?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Gerrit-Change-Number: 36486
Gerrit-PatchSet: 12
Gerrit-Owner: Philipp Hug <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Marc Karasek <[e-mail]>
Gerrit-Reviewer: Martin L Roth <[e-mail]>
Gerrit-Reviewer: Philipp Hug <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Reviewer: ron minnich <[e-mail]>
Gerrit-CC: Arthur Heymans <[e-mail]>
Gerrit-CC: Patrick Rudolph <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-CC: Stefan Reinauer <[e-mail]>
Gerrit-CC: ron minnich
Gerrit-Attention: Arthur Heymans <[e-mail]>
Gerrit-Attention: Philipp Hug <[e-mail]>
Gerrit-Attention: ron minnich
Gerrit-Attention: Arthur Heymans <[e-mail]>
Gerrit-MessageType: newpatchset

[M] Change in coreboot[main]: riscv/mb/qemu: fix DRAM probing

[Philipp Hug (Code Review)]

Attention is currently required from: Arthur Heymans, Arthur Heymans, ron minnich, ron minnich.

Philipp Hug has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36486?usp=email )

Change subject: riscv/mb/qemu: fix DRAM probing
......................................................................


Patch Set 12:

(1 comment)

File src/arch/riscv/ramdetect.c:

https://review.coreboot.org/c/coreboot/+/36486/comment/c5b05751_4989643f :
PS9, Line 21: #define insn_size 4
> Acknowledged
> added code to handle compress instructions as well

[M] Change in coreboot[main]: riscv/mb/qemu: fix DRAM probing

[Philipp Hug (Code Review)]

Attention is currently required from: Arthur Heymans, Arthur Heymans, Maximilian Brune, ron minnich, ron minnich.

Hello Arthur Heymans, Marc Karasek, Martin L Roth, Maximilian Brune, build bot (Jenkins), ron minnich, 

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

    https://review.coreboot.org/c/coreboot/+/36486?usp=email

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


Change subject: riscv/mb/qemu: fix DRAM probing
......................................................................

riscv/mb/qemu: fix DRAM probing

Current version of qemu raise an exception when accessing invalid memory.
Modify the probing code to temporary redirect the exception handler like on
ARM platform.
Also move saving of the stack frame out to trap_util.S to have all at the same
place for a future rewrite.

TEST=boots to ramstage
Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Signed-off-by: Philipp Hug <[e-mail]>
Signed-off-by: Arthur Heymans <[e-mail]>
---
M src/arch/riscv/Makefile.mk
M src/arch/riscv/include/arch/exception.h
A src/arch/riscv/ramdetect.c
M src/arch/riscv/trap_handler.c
M src/arch/riscv/trap_util.S
M src/lib/imd_cbmem.c
6 files changed, 93 insertions(+), 19 deletions(-)


  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/36486/14
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/36486?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Gerrit-Change-Number: 36486
Gerrit-PatchSet: 14
Gerrit-Owner: Philipp Hug <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Marc Karasek <[e-mail]>
Gerrit-Reviewer: Martin L Roth <[e-mail]>
Gerrit-Reviewer: Maximilian Brune <[e-mail]>
Gerrit-Reviewer: Philipp Hug <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Reviewer: ron minnich <[e-mail]>
Gerrit-CC: Arthur Heymans <[e-mail]>
Gerrit-CC: Patrick Rudolph <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-CC: Stefan Reinauer <[e-mail]>
Gerrit-CC: ron minnich
Gerrit-Attention: Arthur Heymans <[e-mail]>
Gerrit-Attention: ron minnich
Gerrit-Attention: Maximilian Brune <[e-mail]>
Gerrit-Attention: Arthur Heymans <[e-mail]>
Gerrit-Attention: ron minnich <[e-mail]>
Gerrit-MessageType: newpatchset

[M] Change in coreboot[main]: riscv/mb/qemu: fix DRAM probing

[Philipp Hug (Code Review)]

Attention is currently required from: Arthur Heymans, Arthur Heymans, Maximilian Brune, ron minnich, ron minnich.

Hello Arthur Heymans, Marc Karasek, Martin L Roth, Maximilian Brune, build bot (Jenkins), ron minnich, 

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

    https://review.coreboot.org/c/coreboot/+/36486?usp=email

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

The following approvals got outdated and were removed:
Verified+1 by build bot (Jenkins)


Change subject: riscv/mb/qemu: fix DRAM probing
......................................................................

riscv/mb/qemu: fix DRAM probing

Current version of qemu raise an exception when accessing invalid memory.
Modify the probing code to temporary redirect the exception handler like on
ARM platform.
Also move saving of the stack frame out to trap_util.S to have all at the same
place for a future rewrite.

TEST=boots to ramstage
Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Signed-off-by: Philipp Hug <[e-mail]>
Signed-off-by: Arthur Heymans <[e-mail]>
---
M src/arch/riscv/Makefile.mk
M src/arch/riscv/include/arch/exception.h
A src/arch/riscv/ramdetect.c
M src/arch/riscv/trap_handler.c
M src/arch/riscv/trap_util.S
5 files changed, 90 insertions(+), 16 deletions(-)


  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/36486/15
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/36486?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Gerrit-Change-Number: 36486
Gerrit-PatchSet: 15
Gerrit-Owner: Philipp Hug <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Marc Karasek <[e-mail]>
Gerrit-Reviewer: Martin L Roth <[e-mail]>
Gerrit-Reviewer: Maximilian Brune <[e-mail]>
Gerrit-Reviewer: Philipp Hug <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Reviewer: ron minnich <[e-mail]>
Gerrit-CC: Arthur Heymans <[e-mail]>
Gerrit-CC: Patrick Rudolph <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-CC: Stefan Reinauer <[e-mail]>
Gerrit-CC: ron minnich
Gerrit-Attention: Arthur Heymans <[e-mail]>
Gerrit-Attention: ron minnich
Gerrit-Attention: Maximilian Brune <[e-mail]>
Gerrit-Attention: Arthur Heymans <[e-mail]>
Gerrit-Attention: ron minnich <[e-mail]>
Gerrit-MessageType: newpatchset

[M] Change in coreboot[main]: riscv/mb/qemu: fix DRAM probing

[ron minnich (Code Review)]

Attention is currently required from: Arthur Heymans, Arthur Heymans, Maximilian Brune, Philipp Hug, ron minnich.

ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36486?usp=email )

Change subject: riscv/mb/qemu: fix DRAM probing
......................................................................


Patch Set 15:

(1 comment)

Patchset:

PS15: 
I'm sure it's me, but if I Download this via a pull, it won't even build. I am using the same toolchain I use to build a working unmatched coreboot.

/home/rminnich/coreboot/util/crossgcc/xgcc/bin/riscv64-elf-ld.bfd: build/bootblock/arch/riscv/romstage.o: in function `.L0 ':
/home/rminnich/coreboot/src/arch/riscv/romstage.S:13: multiple definition of `_start'; build/bootblock/arch/riscv/bootblock.o:/home/rminnich/coreboot/src/arch/riscv/bootblock.S:23: first defined here
/home/rminnich/coreboot/util/crossgcc/xgcc/bin/riscv64-elf-ld.bfd: build/bootblock/mainboard/emulation/qemu-riscv/romstage.o: in function `main':
/home/rminnich/coreboot/src/mainboard/emulation/qemu-riscv/romstage.c:8: multiple definition of `main'; build/bootblock/lib/bootblock.o:/home/rminnich/coreboot/src/lib/bootblock.c:77: first defined here
/home/rminnich/coreboot/util/crossgcc/xgcc/bin/riscv64-elf-ld.bfd: warning: build/cbfs/fallback/bootblock.debug has a LOAD segment with RWX permissions
/home/rminnich/coreboot/util/crossgcc/xgcc/bin/riscv64-elf-ld.bfd: build/bootblock/lib/prog_loaders.o: in function `run_romstage':
/home/rminnich/coreboot/src/lib/prog_loaders.c:23:(.text.run_romstage+0x8): undefined reference to `romstage_main'
make: *** [src/arch/riscv/Makefile.mk:81: build/cbfs/fallback/bootblock.debug] Error 1

I'm a bit lost here.



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/36486?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Gerrit-Change-Number: 36486
Gerrit-PatchSet: 15
Gerrit-Owner: Philipp Hug <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Marc Karasek <[e-mail]>
Gerrit-Reviewer: Martin L Roth <[e-mail]>
Gerrit-Reviewer: Maximilian Brune <[e-mail]>
Gerrit-Reviewer: Philipp Hug <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Reviewer: ron minnich <[e-mail]>
Gerrit-CC: Arthur Heymans <[e-mail]>
Gerrit-CC: Patrick Rudolph <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-CC: Stefan Reinauer <[e-mail]>
Gerrit-CC: ron minnich
Gerrit-Attention: Arthur Heymans <[e-mail]>
Gerrit-Attention: Philipp Hug <[e-mail]>
Gerrit-Attention: ron minnich
Gerrit-Attention: Maximilian Brune <[e-mail]>
Gerrit-Attention: Arthur Heymans <[e-mail]>
Gerrit-Comment-Date: Thu, 15 Feb 2024 18:58:26 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[M] Change in coreboot[main]: riscv/mb/qemu: fix DRAM probing

[ron minnich (Code Review)]

Attention is currently required from: Arthur Heymans, Arthur Heymans, Maximilian Brune, Philipp Hug, ron minnich.

ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36486?usp=email )

Change subject: riscv/mb/qemu: fix DRAM probing
......................................................................


Patch Set 15: Code-Review+2

(1 comment)

Patchset:

PS15: 
It was my mistake
You MUST have this for now
CONFIG_SEPARATE_ROMSTAGE=y
verified it works.



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/36486?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Gerrit-Change-Number: 36486
Gerrit-PatchSet: 15
Gerrit-Owner: Philipp Hug <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Marc Karasek <[e-mail]>
Gerrit-Reviewer: Martin L Roth <[e-mail]>
Gerrit-Reviewer: Maximilian Brune <[e-mail]>
Gerrit-Reviewer: Philipp Hug <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Reviewer: ron minnich <[e-mail]>
Gerrit-CC: Arthur Heymans <[e-mail]>
Gerrit-CC: Patrick Rudolph <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-CC: Stefan Reinauer <[e-mail]>
Gerrit-CC: ron minnich
Gerrit-Attention: Arthur Heymans <[e-mail]>
Gerrit-Attention: Philipp Hug <[e-mail]>
Gerrit-Attention: ron minnich
Gerrit-Attention: Maximilian Brune <[e-mail]>
Gerrit-Attention: Arthur Heymans <[e-mail]>
Gerrit-Comment-Date: Thu, 15 Feb 2024 22:49:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[M] Change in coreboot[main]: riscv/mb/qemu: fix DRAM probing

[ron minnich (Code Review)]

Attention is currently required from: Arthur Heymans, Arthur Heymans, Maximilian Brune, Philipp Hug, ron minnich.

ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36486?usp=email )

Change subject: riscv/mb/qemu: fix DRAM probing
......................................................................


Patch Set 15:

(1 comment)

Patchset:

PS15:
> I'm sure it's me, but if I Download this via a pull, it won't even build. […]
> Done

[M] Change in coreboot[main]: riscv/mb/qemu: fix DRAM probing

[Arthur Heymans (Code Review)]

Attention is currently required from: Arthur Heymans, Maximilian Brune, Philipp Hug, ron minnich.

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36486?usp=email )

Change subject: riscv/mb/qemu: fix DRAM probing
......................................................................


Patch Set 15:

(1 comment)

Patchset:

PS2:
> is this still an issue?
> Nope. Done

[M] Change in coreboot[main]: riscv/mb/qemu: fix DRAM probing

[Felix Singer (Code Review)]

Attention is currently required from: Arthur Heymans, Maximilian Brune, Philipp Hug, ron minnich.

Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36486?usp=email )

The change is no longer submittable: All-Comments-Resolved is unsatisfied now.

Change subject: riscv/mb/qemu: fix DRAM probing
......................................................................


Patch Set 15:

(1 comment)

File src/arch/riscv/ramdetect.c:

https://review.coreboot.org/c/coreboot/+/36486/comment/d606119e_6c0ca84a :
PS15, Line 1: /*
            :  * This file is part of the coreboot project.
            :  *
            :  * SPDX-License-Identifier: GPL-2.0-or-later
            :  */
```
/* SPDX-License-Identifier: GPL-2.0-or-later */
```



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/36486?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Gerrit-Change-Number: 36486
Gerrit-PatchSet: 15
Gerrit-Owner: Philipp Hug <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Marc Karasek <[e-mail]>
Gerrit-Reviewer: Martin L Roth <[e-mail]>
Gerrit-Reviewer: Maximilian Brune <[e-mail]>
Gerrit-Reviewer: Philipp Hug <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Reviewer: ron minnich <[e-mail]>
Gerrit-CC: Arthur Heymans <[e-mail]>
Gerrit-CC: Felix Singer <[e-mail]>
Gerrit-CC: Patrick Rudolph <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-CC: Stefan Reinauer <[e-mail]>
Gerrit-CC: ron minnich
Gerrit-Attention: Arthur Heymans <[e-mail]>
Gerrit-Attention: Philipp Hug <[e-mail]>
Gerrit-Attention: ron minnich
Gerrit-Attention: Maximilian Brune <[e-mail]>
Gerrit-Comment-Date: Wed, 28 Feb 2024 23:01:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[M] Change in coreboot[main]: riscv/mb/qemu: fix DRAM probing

[Felix Singer (Code Review)]

Attention is currently required from: Arthur Heymans, Maximilian Brune, Philipp Hug, ron minnich.

Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36486?usp=email )

Change subject: riscv/mb/qemu: fix DRAM probing
......................................................................


Patch Set 15:

(1 comment)

Commit Message:

https://review.coreboot.org/c/coreboot/+/36486/comment/bd79c644_6678660d :
PS15, Line 8:
> `Possible unwrapped commit description (prefer a maximum 72 chars per line)`

Please fix.



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/36486?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Gerrit-Change-Number: 36486
Gerrit-PatchSet: 15
Gerrit-Owner: Philipp Hug <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Marc Karasek <[e-mail]>
Gerrit-Reviewer: Martin L Roth <[e-mail]>
Gerrit-Reviewer: Maximilian Brune <[e-mail]>
Gerrit-Reviewer: Philipp Hug <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Reviewer: ron minnich <[e-mail]>
Gerrit-CC: Arthur Heymans <[e-mail]>
Gerrit-CC: Felix Singer <[e-mail]>
Gerrit-CC: Patrick Rudolph <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-CC: Stefan Reinauer <[e-mail]>
Gerrit-CC: ron minnich
Gerrit-Attention: Arthur Heymans <[e-mail]>
Gerrit-Attention: Philipp Hug <[e-mail]>
Gerrit-Attention: ron minnich
Gerrit-Attention: Maximilian Brune <[e-mail]>
Gerrit-Comment-Date: Thu, 29 Feb 2024 13:03:25 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[M] Change in coreboot[main]: riscv/mb/qemu: fix DRAM probing

[Maximilian Brune (Code Review)]

Attention is currently required from: Arthur Heymans, Philipp Hug, ron minnich.

Maximilian Brune has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36486?usp=email )

Change subject: riscv/mb/qemu: fix DRAM probing
......................................................................


Patch Set 15:

(1 comment)

File src/arch/riscv/ramdetect.c:

https://review.coreboot.org/c/coreboot/+/36486/comment/ee8dd363_869af3b0 :
PS15, Line 24: 	uint16_t ins = mprv_read_mxr_u16((uint16_t *)vaddr);
Why use `mprv`? This should always be executed in machine mode.



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/36486?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Gerrit-Change-Number: 36486
Gerrit-PatchSet: 15
Gerrit-Owner: Philipp Hug <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Marc Karasek <[e-mail]>
Gerrit-Reviewer: Martin L Roth <[e-mail]>
Gerrit-Reviewer: Maximilian Brune <[e-mail]>
Gerrit-Reviewer: Philipp Hug <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Reviewer: ron minnich <[e-mail]>
Gerrit-CC: Arthur Heymans <[e-mail]>
Gerrit-CC: Felix Singer <[e-mail]>
Gerrit-CC: Patrick Rudolph <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-CC: Stefan Reinauer <[e-mail]>
Gerrit-CC: ron minnich
Gerrit-Attention: Arthur Heymans <[e-mail]>
Gerrit-Attention: Philipp Hug <[e-mail]>
Gerrit-Attention: ron minnich
Gerrit-Comment-Date: Fri, 01 Mar 2024 02:51:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[M] Change in coreboot[main]: riscv/mb/qemu: fix DRAM probing

[Philipp Hug (Code Review)]

Attention is currently required from: Arthur Heymans, Philipp Hug, ron minnich, ron minnich.

Hello Arthur Heymans, Marc Karasek, Martin L Roth, Maximilian Brune, build bot (Jenkins), ron minnich, 

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

    https://review.coreboot.org/c/coreboot/+/36486?usp=email

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

The following approvals got outdated and were removed:
Code-Review+2 by ron minnich, Verified+1 by build bot (Jenkins)


Change subject: riscv/mb/qemu: fix DRAM probing
......................................................................

riscv/mb/qemu: fix DRAM probing

Current version of qemu raise an exception when accessing invalid
memory.  Modify the probing code to temporary redirect the exception
handler like on ARM platform.
Also move saving of the stack frame out to trap_util.S to have all at
the same place for a future rewrite.

TEST=boots to ramstage
Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Signed-off-by: Philipp Hug <[e-mail]>
Signed-off-by: Arthur Heymans <[e-mail]>
---
M src/arch/riscv/Makefile.mk
M src/arch/riscv/include/arch/exception.h
A src/arch/riscv/ramdetect.c
M src/arch/riscv/trap_handler.c
M src/arch/riscv/trap_util.S
M src/lib/ramdetect.c
M src/mainboard/emulation/qemu-riscv/Kconfig
7 files changed, 93 insertions(+), 23 deletions(-)


  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/36486/16
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/36486?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Gerrit-Change-Number: 36486
Gerrit-PatchSet: 16
Gerrit-Owner: Philipp Hug <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Marc Karasek <[e-mail]>
Gerrit-Reviewer: Martin L Roth <[e-mail]>
Gerrit-Reviewer: Maximilian Brune <[e-mail]>
Gerrit-Reviewer: Philipp Hug <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Reviewer: ron minnich <[e-mail]>
Gerrit-CC: Arthur Heymans <[e-mail]>
Gerrit-CC: Felix Singer <[e-mail]>
Gerrit-CC: Patrick Rudolph <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-CC: Stefan Reinauer <[e-mail]>
Gerrit-CC: ron minnich
Gerrit-Attention: Arthur Heymans <[e-mail]>
Gerrit-Attention: Philipp Hug <[e-mail]>
Gerrit-Attention: ron minnich
Gerrit-Attention: ron minnich <[e-mail]>
Gerrit-MessageType: newpatchset

[M] Change in coreboot[main]: riscv/mb/qemu: fix DRAM probing

[Philipp Hug (Code Review)]

Attention is currently required from: Arthur Heymans, Philipp Hug, ron minnich, ron minnich.

Hello Arthur Heymans, Marc Karasek, Martin L Roth, Maximilian Brune, build bot (Jenkins), ron minnich, 

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

    https://review.coreboot.org/c/coreboot/+/36486?usp=email

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


Change subject: riscv/mb/qemu: fix DRAM probing
......................................................................

riscv/mb/qemu: fix DRAM probing

Current version of qemu raise an exception when accessing invalid
memory.  Modify the probing code to temporary redirect the exception
handler like on ARM platform.
Also move saving of the stack frame out to trap_util.S to have all at
the same place for a future rewrite.

TEST=boots to ramstage
Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Signed-off-by: Philipp Hug <[e-mail]>
Signed-off-by: Arthur Heymans <[e-mail]>
---
M src/arch/riscv/Makefile.mk
M src/arch/riscv/include/arch/exception.h
A src/arch/riscv/ramdetect.c
M src/arch/riscv/trap_handler.c
M src/arch/riscv/trap_util.S
M src/lib/ramdetect.c
M src/mainboard/emulation/qemu-riscv/Kconfig
7 files changed, 93 insertions(+), 23 deletions(-)


  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/36486/17
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/36486?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Gerrit-Change-Number: 36486
Gerrit-PatchSet: 17
Gerrit-Owner: Philipp Hug <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Marc Karasek <[e-mail]>
Gerrit-Reviewer: Martin L Roth <[e-mail]>
Gerrit-Reviewer: Maximilian Brune <[e-mail]>
Gerrit-Reviewer: Philipp Hug <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Reviewer: ron minnich <[e-mail]>
Gerrit-CC: Arthur Heymans <[e-mail]>
Gerrit-CC: Felix Singer <[e-mail]>
Gerrit-CC: Patrick Rudolph <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-CC: Stefan Reinauer <[e-mail]>
Gerrit-CC: ron minnich
Gerrit-Attention: Arthur Heymans <[e-mail]>
Gerrit-Attention: Philipp Hug <[e-mail]>
Gerrit-Attention: ron minnich
Gerrit-Attention: ron minnich <[e-mail]>
Gerrit-MessageType: newpatchset

[M] Change in coreboot[main]: riscv/mb/qemu: fix DRAM probing

[Philipp Hug (Code Review)]

Attention is currently required from: Arthur Heymans, Felix Singer, Maximilian Brune, ron minnich, ron minnich.

Philipp Hug has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36486?usp=email )

Change subject: riscv/mb/qemu: fix DRAM probing
......................................................................


Patch Set 17:

(3 comments)

Commit Message:

https://review.coreboot.org/c/coreboot/+/36486/comment/7784be6e_741f5e79 :
PS15, Line 8:
> > `Possible unwrapped commit description (prefer a maximum 72 chars per line)` […]
> > Done

File src/arch/riscv/ramdetect.c:

https://review.coreboot.org/c/coreboot/+/36486/comment/a6a8087d_c12be64f :
PS15, Line 1: /*
            :  * This file is part of the coreboot project.
            :  *
            :  * SPDX-License-Identifier: GPL-2.0-or-later
            :  */
> ``` […]

Done


https://review.coreboot.org/c/coreboot/+/36486/comment/7962c0cc_a7ef5fe1 :
PS15, Line 24: 	uint16_t ins = mprv_read_mxr_u16((uint16_t *)vaddr);
> Why use `mprv`? This should always be executed in machine mode.
> dropped

[M] Change in coreboot[main]: riscv/mb/qemu: fix DRAM probing

[ron minnich (Code Review)]

Attention is currently required from: Arthur Heymans, Felix Singer, Maximilian Brune, Philipp Hug, ron minnich.

ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36486?usp=email )

Change subject: riscv/mb/qemu: fix DRAM probing
......................................................................


Patch Set 17: Code-Review+2


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/36486?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Gerrit-Change-Number: 36486
Gerrit-PatchSet: 17
Gerrit-Owner: Philipp Hug <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Marc Karasek <[e-mail]>
Gerrit-Reviewer: Martin L Roth <[e-mail]>
Gerrit-Reviewer: Maximilian Brune <[e-mail]>
Gerrit-Reviewer: Philipp Hug <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Reviewer: ron minnich <[e-mail]>
Gerrit-CC: Arthur Heymans <[e-mail]>
Gerrit-CC: Felix Singer <[e-mail]>
Gerrit-CC: Patrick Rudolph <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-CC: ron minnich
Gerrit-Attention: Felix Singer <[e-mail]>
Gerrit-Attention: Arthur Heymans <[e-mail]>
Gerrit-Attention: Philipp Hug <[e-mail]>
Gerrit-Attention: ron minnich
Gerrit-Attention: Maximilian Brune <[e-mail]>
Gerrit-Comment-Date: Sat, 02 Mar 2024 22:24:44 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[M] Change in coreboot[main]: riscv/mb/qemu: fix DRAM probing

[Arthur Heymans (Code Review)]

Attention is currently required from: Arthur Heymans, Felix Singer, Maximilian Brune, Philipp Hug, ron minnich.

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36486?usp=email )

Change subject: riscv/mb/qemu: fix DRAM probing
......................................................................


Patch Set 17: Code-Review+2


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/36486?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Gerrit-Change-Number: 36486
Gerrit-PatchSet: 17
Gerrit-Owner: Philipp Hug <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Marc Karasek <[e-mail]>
Gerrit-Reviewer: Martin L Roth <[e-mail]>
Gerrit-Reviewer: Maximilian Brune <[e-mail]>
Gerrit-Reviewer: Philipp Hug <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Reviewer: ron minnich <[e-mail]>
Gerrit-CC: Arthur Heymans <[e-mail]>
Gerrit-CC: Felix Singer <[e-mail]>
Gerrit-CC: Patrick Rudolph <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-CC: ron minnich
Gerrit-Attention: Felix Singer <[e-mail]>
Gerrit-Attention: Arthur Heymans <[e-mail]>
Gerrit-Attention: Philipp Hug <[e-mail]>
Gerrit-Attention: ron minnich
Gerrit-Attention: Maximilian Brune <[e-mail]>
Gerrit-Comment-Date: Mon, 04 Mar 2024 10:48:33 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[M] Change in coreboot[main]: riscv/mb/qemu: fix DRAM probing

[ron minnich (Code Review)]

ron minnich has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36486?usp=email )

Change subject: riscv/mb/qemu: fix DRAM probing
......................................................................

riscv/mb/qemu: fix DRAM probing

Current version of qemu raise an exception when accessing invalid
memory.  Modify the probing code to temporary redirect the exception
handler like on ARM platform.
Also move saving of the stack frame out to trap_util.S to have all at
the same place for a future rewrite.

TEST=boots to ramstage
Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Signed-off-by: Philipp Hug <[e-mail]>
Signed-off-by: Arthur Heymans <[e-mail]>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/36486
Reviewed-by: ron minnich <[e-mail]>
Tested-by: build bot (Jenkins) <[e-mail]>
---
M src/arch/riscv/Makefile.mk
M src/arch/riscv/include/arch/exception.h
A src/arch/riscv/ramdetect.c
M src/arch/riscv/trap_handler.c
M src/arch/riscv/trap_util.S
M src/lib/ramdetect.c
M src/mainboard/emulation/qemu-riscv/Kconfig
7 files changed, 93 insertions(+), 23 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  ron minnich: Looks good to me, approved
  Arthur Heymans: Looks good to me, approved




diff --git a/src/arch/riscv/Makefile.mk b/src/arch/riscv/Makefile.mk
index 1267195..0dbfd2a 100644
--- a/src/arch/riscv/Makefile.mk
+++ b/src/arch/riscv/Makefile.mk
@@ -97,6 +97,7 @@
 ifeq ($(CONFIG_ARCH_ROMSTAGE_RISCV),y)
 
 romstage-$(CONFIG_SEPARATE_ROMSTAGE) += romstage.S
+romstage-y += ramdetect.c
 
 # Build the romstage
 
@@ -120,6 +121,7 @@
 
 ramstage-y =
 ramstage-y += ramstage.S
+ramstage-y += ramdetect.c
 ramstage-y += tables.c
 ramstage-y += payload.c
 ramstage-y += fit_payload.c
diff --git a/src/arch/riscv/include/arch/exception.h b/src/arch/riscv/include/arch/exception.h
index 208cc81..9339437 100644
--- a/src/arch/riscv/include/arch/exception.h
+++ b/src/arch/riscv/include/arch/exception.h
@@ -26,7 +26,7 @@
 }
 
 void redirect_trap(void);
-void trap_handler(trapframe *tf);
+void default_trap_handler(trapframe *tf);
 void handle_supervisor_call(trapframe *tf);
 void handle_misaligned(trapframe *tf);
 
diff --git a/src/arch/riscv/ramdetect.c b/src/arch/riscv/ramdetect.c
new file mode 100644
index 0000000..3382435
--- /dev/null
+++ b/src/arch/riscv/ramdetect.c
@@ -0,0 +1,61 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#include <arch/exception.h>
+#include <types.h>
+#include <console/console.h>
+#include <device/mmio.h>
+#include <ramdetect.h>
+#include <arch/smp/spinlock.h>
+#include <vm.h>
+
+static enum {
+	ABORT_CHECKER_NOT_TRIGGERED,
+	ABORT_CHECKER_TRIGGERED,
+} abort_state = ABORT_CHECKER_NOT_TRIGGERED;
+
+extern void (*trap_handler)(trapframe *tf);
+
+static int get_instruction_len(uintptr_t addr)
+{
+	uint16_t ins = read16p(addr);
+
+	/*
+	 * 16-bit or 32-bit instructions supported
+	 */
+	if ((ins & 0x3) != 3) {
+		return 2;
+	} else if ((ins & 0x1f) != 0x1f) {
+		return 4;
+	}
+
+	die("Not a 16bit or 32bit instruction 0x%x\n", ins);
+}
+
+static void ramcheck_trap_handler(trapframe *tf)
+{
+	abort_state = ABORT_CHECKER_TRIGGERED;
+
+	/*
+	 * skip read instruction.
+	 */
+	int insn_size = get_instruction_len(tf->epc);
+
+	write_csr(mepc, read_csr(mepc) + insn_size);
+}
+
+int probe_mb(const uintptr_t dram_start, const uintptr_t size)
+{
+	uintptr_t addr = dram_start + (size * MiB) - sizeof(uint32_t);
+	void *ptr = (void *)addr;
+
+	abort_state = ABORT_CHECKER_NOT_TRIGGERED;
+	trap_handler = ramcheck_trap_handler;
+	barrier();
+	read32(ptr);
+	trap_handler = default_trap_handler;
+	barrier();
+	printk(BIOS_DEBUG, "%lx is %s DRAM\n", dram_start + size * MiB,
+	       abort_state == ABORT_CHECKER_NOT_TRIGGERED ? "" : "not");
+
+	return abort_state == ABORT_CHECKER_NOT_TRIGGERED;
+}
diff --git a/src/arch/riscv/trap_handler.c b/src/arch/riscv/trap_handler.c
index fbc6ae4..4cbccc5 100644
--- a/src/arch/riscv/trap_handler.c
+++ b/src/arch/riscv/trap_handler.c
@@ -33,10 +33,14 @@
 static const char *mstatus_to_previous_mode(uintptr_t ms)
 {
 	switch (ms & MSTATUS_MPP) {
-	case 0x00000000: return "user";
-	case 0x00000800: return "supervisor";
-	case 0x00001000: return "hypervisor";
-	case 0x00001800: return "machine";
+	case 0x00000000:
+		return "user";
+	case 0x00000800:
+		return "supervisor";
+	case 0x00001000:
+		return "hypervisor";
+	case 0x00001800:
+		return "machine";
 	}
 
 	return "unknown";
@@ -52,16 +56,13 @@
 	printk(BIOS_DEBUG, "\n");
 
 	if (tf->cause < ARRAY_SIZE(exception_names))
-		printk(BIOS_DEBUG, "Exception:          %s\n",
-				exception_names[tf->cause]);
+		printk(BIOS_DEBUG, "Exception:          %s\n", exception_names[tf->cause]);
 	else
-		printk(BIOS_DEBUG, "Trap:               Unknown cause %p\n",
-				(void *)tf->cause);
+		printk(BIOS_DEBUG, "Trap:               Unknown cause %p\n", (void *)tf->cause);
 
 	previous_mode = mstatus_to_previous_mode(read_csr(mstatus));
 	printk(BIOS_DEBUG, "Hart ID:            %d\n", hart_id);
-	printk(BIOS_DEBUG, "Previous mode:      %s%s\n",
-			previous_mode, mprv? " (MPRV)":"");
+	printk(BIOS_DEBUG, "Previous mode:      %s%s\n", previous_mode, mprv ? " (MPRV)" : "");
 	printk(BIOS_DEBUG, "Bad instruction pc: %p\n", (void *)tf->epc);
 	printk(BIOS_DEBUG, "Bad address:        %p\n", (void *)tf->badvaddr);
 	printk(BIOS_DEBUG, "Stored ra:          %p\n", (void *)tf->gpr[1]);
@@ -101,16 +102,17 @@
 		break;
 	default:
 		printk(BIOS_EMERG, "======================================\n");
-		printk(BIOS_EMERG, "coreboot: Unknown machine interrupt: 0x%llx\n",
-		       cause);
+		printk(BIOS_EMERG, "coreboot: Unknown machine interrupt: 0x%llx\n", cause);
 		printk(BIOS_EMERG, "======================================\n");
 		print_trap_information(tf);
 		break;
 	}
 }
-void trap_handler(trapframe *tf)
+
+void (*trap_handler)(trapframe *tf) = default_trap_handler;
+
+void default_trap_handler(trapframe *tf)
 {
-	write_csr(mscratch, tf);
 	if (tf->cause & 0x8000000000000000ULL) {
 		interrupt_handler(tf);
 		return;
diff --git a/src/arch/riscv/trap_util.S b/src/arch/riscv/trap_util.S
index c5691c5..d7b1250 100644
--- a/src/arch/riscv/trap_util.S
+++ b/src/arch/riscv/trap_util.S
@@ -121,7 +121,12 @@
 
   save_tf
   move  a0,sp
-  jal trap_handler
+
+  # store pointer to stack frame (moved out from trap_handler)
+  csrw  mscratch, sp
+
+  LOAD	t0, trap_handler
+  jalr	t0
 
 trap_return:
 	csrr	a0, mscratch
diff --git a/src/lib/ramdetect.c b/src/lib/ramdetect.c
index 9a29d0f..cef3d57 100644
--- a/src/lib/ramdetect.c
+++ b/src/lib/ramdetect.c
@@ -11,15 +11,12 @@
 int __weak probe_mb(const uintptr_t dram_start, const uintptr_t size)
 {
 	uintptr_t addr = dram_start + (size * MiB) - sizeof(uint32_t);
-	static const uint32_t patterns[] = {
-		0x55aa55aa,
-		0x12345678
-	};
-	void *ptr = (void *) addr;
+	static const uint32_t patterns[] = {0x55aa55aa, 0x12345678};
+	void *ptr = (void *)addr;
 	size_t i;
 
 	/* Don't accidentally clobber oneself. */
-	if (OVERLAP(addr, addr + sizeof(uint32_t), (uintptr_t)_program, (uintptr_t) _eprogram))
+	if (OVERLAP(addr, addr + sizeof(uint32_t), (uintptr_t)_program, (uintptr_t)_eprogram))
 		return 1;
 
 	uint32_t old = read32(ptr);
diff --git a/src/mainboard/emulation/qemu-riscv/Kconfig b/src/mainboard/emulation/qemu-riscv/Kconfig
index d915d68..af4416d 100644
--- a/src/mainboard/emulation/qemu-riscv/Kconfig
+++ b/src/mainboard/emulation/qemu-riscv/Kconfig
@@ -45,7 +45,10 @@
 
 config DRAM_SIZE_MB
 	int
-	default 32768
+	default 16383
+	help
+	  Qemu maps MMIO at ALIGN_UP(top_of_mem, 16 * GiB)
+	  To avoid confusing the dram probing algorithm, avoid large dram sizes (16G - 1m)
 
 config OPENSBI_PLATFORM
 	string

-- 
To view, visit https://review.coreboot.org/c/coreboot/+/36486?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I25860f688c7546714f6fdbce8c8f96da6400813c
Gerrit-Change-Number: 36486
Gerrit-PatchSet: 18
Gerrit-Owner: Philipp Hug <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Marc Karasek <[e-mail]>
Gerrit-Reviewer: Martin L Roth <[e-mail]>
Gerrit-Reviewer: Maximilian Brune <[e-mail]>
Gerrit-Reviewer: Philipp Hug <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Reviewer: ron minnich <[e-mail]>
Gerrit-CC: Arthur Heymans <[e-mail]>
Gerrit-CC: Felix Singer <[e-mail]>
Gerrit-CC: Patrick Rudolph <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-CC: ron minnich
Gerrit-MessageType: merged