Change in ...coreboot[master]: soc/ucb/riscv: Protect CBFS from payload loader

[Patrick Rudolph (Code Review)]

Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33426

Change subject: soc/ucb/riscv: Protect CBFS from payload loader
......................................................................

soc/ucb/riscv: Protect CBFS from payload loader

The payload loader doesn't know about CBFS in DRAM.
Mark the region as RAMSTAGE to make sure the payload won't
overwrite the CBFS while decompressing.

Change-Id: I36a18cb727f660ac9e77df413026627ea160c1e1
Signed-off-by: Patrick Rudolph 
---
M src/soc/ucb/riscv/Makefile.inc
A src/soc/ucb/riscv/tables.c
2 files changed, 24 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/33426/1

diff --git a/src/soc/ucb/riscv/Makefile.inc b/src/soc/ucb/riscv/Makefile.inc
index ef03642..ba2a246 100644
--- a/src/soc/ucb/riscv/Makefile.inc
+++ b/src/soc/ucb/riscv/Makefile.inc
@@ -7,5 +7,6 @@

ramstage-y += cbmem.c
 ramstage-y += ipi.c
+ramstage-y += tables.c

endif
diff --git a/src/soc/ucb/riscv/tables.c b/src/soc/ucb/riscv/tables.c
new file mode 100644
index 0000000..749cb0c
--- /dev/null
+++ b/src/soc/ucb/riscv/tables.c
@@ -0,0 +1,23 @@
+/*
+ * This file is part of the coreboot project.
+ *
+ * Copyright (C) 2019 9elements Agency GmbH 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <bootmem.h>
+
+void bootmem_platform_add_ranges(void)
+{
+	// Protect CBFS in DRAM
+	bootmem_add_range((uintptr_t)0x80000000,
+			  CONFIG_COREBOOT_ROMSIZE_KB * 1024, BM_MEM_RAMSTAGE);
+}

[Philipp Hug (Code Review)]

Philipp Hug has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33426 )

Change subject: soc/ucb/riscv: Protect CBFS from payload loader
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/33426/1/src/soc/ucb/riscv/tables.c 
File src/soc/ucb/riscv/tables.c:

https://review.coreboot.org/#/c/33426/1/src/soc/ucb/riscv/tables.c@21 
PS1, Line 21: 	bootmem_add_range((uintptr_t)0x80000000,
Use a constant for DRAM address

[Patrick Rudolph (Code Review)]

Hello ron minnich, build bot (Jenkins), Philipp Hug, Xiang Wang,

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

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

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

Change subject: soc/ucb/riscv: Protect CBFS from payload loader
......................................................................

soc/ucb/riscv: Protect CBFS from payload loader

The payload loader doesn't know about CBFS in DRAM.
Mark the region as RAMSTAGE to make sure the payload won't
overwrite the CBFS while decompressing.

Change-Id: I36a18cb727f660ac9e77df413026627ea160c1e1
Signed-off-by: Patrick Rudolph 
---
M src/soc/ucb/riscv/Makefile.inc
A src/soc/ucb/riscv/tables.c
2 files changed, 24 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/33426/2

[Patrick Rudolph (Code Review)]

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

Change subject: soc/ucb/riscv: Protect CBFS from payload loader
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/33426/1/src/soc/ucb/riscv/tables.c 
File src/soc/ucb/riscv/tables.c:

https://review.coreboot.org/#/c/33426/1/src/soc/ucb/riscv/tables.c@21 
PS1, Line 21: 	bootmem_add_range((uintptr_t)0x80000000,
> Use a constant for DRAM address

Used the _dram symbol, which will be set by the linker script to the correct address

[Philipp Hug (Code Review)]

Philipp Hug has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33426 )

Change subject: soc/ucb/riscv: Protect CBFS from payload loader
......................................................................

Patch Set 2:

This change assumes that coreboot is always loaded from RAM in qemu which is only a workaround because qemu doesn't support -bios coreboot.rom for the virt machine yet.

[Patrick Rudolph (Code Review)]

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

Change subject: soc/ucb/riscv: Protect CBFS from payload loader
......................................................................

Patch Set 2:
> Patch Set 2:
>
> This change assumes that coreboot is always loaded from RAM in qemu which is only a workaround because qemu doesn't support -bios coreboot.rom for the virt machine yet.

Yes. Should it moved to src/mainboard/emulation/qemu-riscv/ instead?
The "virt" machine will probably never support -bios, as it's only for booting a kernel, not for emulating a full SoC.

Change in coreboot[master]: mb/emulation/qemu-riscv: Protect CBFS from payload loader

[Patrick Rudolph (Code Review)]

Hello ron minnich, build bot (Jenkins), Philipp Hug, Xiang Wang,

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

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

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

Change subject: mb/emulation/qemu-riscv: Protect CBFS from payload loader
......................................................................

mb/emulation/qemu-riscv: Protect CBFS from payload loader

The virt machine is special as it doesn't emulate flash and it puts
the coreboot.rom at start of DRAM. The payload loader doesn't know
about CBFS in DRAM and overwrites the CBFS while decompressing
payloads, resulting in undefined behaviour.

Mark the region as SRAM to make sure the payload won't
overwrite the CBFS while decompressing.
As payload is always decompressed to DRAM, it wouldn't touch
SRAM memory regions.

Change-Id: I36a18cb727f660ac9e77df413026627ea160c1e1
Signed-off-by: Patrick Rudolph 
---
M src/mainboard/emulation/qemu-riscv/memlayout.ld
M src/mainboard/emulation/qemu-riscv/rom_media.c
2 files changed, 13 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/33426/3

Change in coreboot[master]: mb/emulation/qemu-riscv: Protect CBFS from payload loader

[Philipp Deppenwiese (Code Review)]

Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33426 )

Change subject: mb/emulation/qemu-riscv: Protect CBFS from payload loader
......................................................................

Patch Set 3: Code-Review+2

Change in coreboot[master]: mb/emulation/qemu-riscv: Protect CBFS from payload loader

[Patrick Rudolph (Code Review)]

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

Change subject: mb/emulation/qemu-riscv: Protect CBFS from payload loader
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/33426/1/src/soc/ucb/riscv/tables.c 
File src/soc/ucb/riscv/tables.c:

https://review.coreboot.org/c/coreboot/+/33426/1/src/soc/ucb/riscv/tables.c@... 
PS1, Line 21: 	bootmem_add_range((uintptr_t)0x80000000,
> Used the _dram symbol, which will be set by the linker script to the correct address

Removed that code.

Change in coreboot[master]: mb/emulation/qemu-riscv: Protect CBFS from payload loader

[Patrick Rudolph (Code Review)]

Patrick Rudolph has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33426 )

Change subject: mb/emulation/qemu-riscv: Protect CBFS from payload loader
......................................................................

mb/emulation/qemu-riscv: Protect CBFS from payload loader

The virt machine is special as it doesn't emulate flash and it puts
the coreboot.rom at start of DRAM. The payload loader doesn't know
about CBFS in DRAM and overwrites the CBFS while decompressing
payloads, resulting in undefined behaviour.

Mark the region as SRAM to make sure the payload won't
overwrite the CBFS while decompressing.
As payload is always decompressed to DRAM, it wouldn't touch
SRAM memory regions.

Change-Id: I36a18cb727f660ac9e77df413026627ea160c1e1
Signed-off-by: Patrick Rudolph 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/33426
Tested-by: build bot (Jenkins) 
Reviewed-by: Philipp Deppenwiese 
---
M src/mainboard/emulation/qemu-riscv/memlayout.ld
M src/mainboard/emulation/qemu-riscv/rom_media.c
2 files changed, 13 insertions(+), 6 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Philipp Deppenwiese: Looks good to me, approved

diff --git a/src/mainboard/emulation/qemu-riscv/memlayout.ld b/src/mainboard/emulation/qemu-riscv/memlayout.ld
index 7f8ec3d..2166d23 100644
--- a/src/mainboard/emulation/qemu-riscv/memlayout.ld
+++ b/src/mainboard/emulation/qemu-riscv/memlayout.ld
@@ -17,16 +17,23 @@
 #include <arch/header.ld>
 #include <mainboard/addressmap.h>

-//Stages start after CBFS in DRAM
+// Stages start after CBFS in DRAM
 #define STAGES_START (QEMU_VIRT_DRAM + CONFIG_ROM_SIZE)

SECTIONS
 {
-	DRAM_START(QEMU_VIRT_DRAM)
+	// the virt target doesn't emulate flash and just puts the CBFS into DRAM.
+	// fake SRAM where CBFS resides. It's only done for better integration.
+	SRAM_START(QEMU_VIRT_DRAM)
    BOOTBLOCK(QEMU_VIRT_DRAM, 64K)
    // CBFS goes here
-	STACK(STAGES_START, 4K)
-	ROMSTAGE(STAGES_START + 64K, 128K)
-	PRERAM_CBMEM_CONSOLE(STAGES_START + 192K, 8K)
+	SRAM_END(STAGES_START)
+	DRAM_START(STAGES_START)
+
+#if ENV_ROMSTAGE
+	ROMSTAGE(STAGES_START, 128K)
+#endif
+	PRERAM_CBMEM_CONSOLE(STAGES_START + 128K, 8K)
    RAMSTAGE(STAGES_START + 200K, 16M)
+	STACK(STAGES_START + 200K + 16M, 4K)
 }
diff --git a/src/mainboard/emulation/qemu-riscv/rom_media.c b/src/mainboard/emulation/qemu-riscv/rom_media.c
index bac1984..79e5ca8 100644
--- a/src/mainboard/emulation/qemu-riscv/rom_media.c
+++ b/src/mainboard/emulation/qemu-riscv/rom_media.c
@@ -19,7 +19,7 @@
 /* This assumes that the CBFS resides at start of dram, which is true for the
  * default configuration. */
 static const struct mem_region_device boot_dev =
-	MEM_REGION_DEV_RO_INIT(_dram, CONFIG_ROM_SIZE);
+	MEM_REGION_DEV_RO_INIT(_sram, CONFIG_ROM_SIZE);

const struct region_device *boot_device_ro(void)
 {