Hi Conor,
On 02/08/2023 13:12, Conor Dooley wrote:
Add an erratum for versions [v0.8 to v1.3) of OpenSBI which fail to add
the "no-map" property to the reserved memory nodes for the regions it
has protected using PMPs.
Our existing fix sweeping hibernation under the carpet by marking it
NONPORTABLE is insufficient as there are other ways to generate
accesses to these reserved memory regions, as Petr discovered [1]
while testing crash kernels & kdump.
Intercede during the boot process when the afflicted versions of OpenSBI
are present & set the "no-map" property in all "mmode_resv" nodes before
the kernel does its reserved memory region initialisation.
Reported-by: Song Shuai <suagrfillet@xxxxxxxxx>
Link: https://lore.kernel.org/all/CAAYs2=gQvkhTeioMmqRDVGjdtNF_vhB+vm_1dHJxPNi75YDQ_Q@xxxxxxxxxxxxxx/
Reported-by: JeeHeng Sia <jeeheng.sia@xxxxxxxxxxxxxxxx>
Link: https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/ITXwaKfA6z8
Reported-by: Petr Tesarik <petrtesarik@xxxxxxxxxxxxxxx>
Closes: https://lore.kernel.org/linux-riscv/76ff0f51-d6c1-580d-f943-061e93073306@xxxxxxxxxxxxxxx/ [1]
CC: stable@xxxxxxxxxxxxxxx
Signed-off-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
---
arch/riscv/include/asm/sbi.h | 5 +++++
arch/riscv/kernel/sbi.c | 42 +++++++++++++++++++++++++++++++++++-
arch/riscv/mm/init.c | 3 +++
3 files changed, 49 insertions(+), 1 deletion(-)
diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
index 5b4a1bf5f439..5360f3476278 100644
--- a/arch/riscv/include/asm/sbi.h
+++ b/arch/riscv/include/asm/sbi.h
@@ -252,6 +252,9 @@ enum sbi_pmu_ctr_type {
#define SBI_ERR_ALREADY_STARTED -7
#define SBI_ERR_ALREADY_STOPPED -8
+/* SBI implementation IDs */
+#define SBI_IMP_OPENSBI 1
+
extern unsigned long sbi_spec_version;
struct sbiret {
long error;
@@ -259,6 +262,8 @@ struct sbiret {
};
void sbi_init(void);
+void sbi_apply_reserved_mem_erratum(void *dtb_va);
+
struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0,
unsigned long arg1, unsigned long arg2,
unsigned long arg3, unsigned long arg4,
diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
index c672c8ba9a2a..aeb27263fa53 100644
--- a/arch/riscv/kernel/sbi.c
+++ b/arch/riscv/kernel/sbi.c
@@ -5,8 +5,10 @@
* Copyright (c) 2020 Western Digital Corporation or its affiliates.
*/
+#include <linux/acpi.h>
#include <linux/bits.h>
#include <linux/init.h>
+#include <linux/libfdt.h>
#include <linux/pm.h>
#include <linux/reboot.h>
#include <asm/sbi.h>
@@ -583,6 +585,40 @@ long sbi_get_mimpid(void)
}
EXPORT_SYMBOL_GPL(sbi_get_mimpid);
+static long sbi_firmware_id;
+static long sbi_firmware_version;
+
+/*
+ * For devicetrees patched by OpenSBI a "mmode_resv" node is added to cover
+ * the region OpenSBI has protected by means of a PMP. Some versions of OpenSBI,
+ * [v0.8 to v1.3), omitted the "no-map" property, but this trips up hibernation
+ * among other things.
+ */
+void __init sbi_apply_reserved_mem_erratum(void *dtb_pa)
+{
+ int child, reserved_mem;
+
+ if (sbi_firmware_id != SBI_IMP_OPENSBI)
+ return;
+
+ if (!acpi_disabled)
+ return;
+
+ if (sbi_firmware_version >= 0x10003 || sbi_firmware_version < 0x8)
+ return;
+
+ reserved_mem = fdt_path_offset((void *)dtb_pa, "/reserved-memory");
+ if (reserved_mem < 0)
+ return;
+
+ fdt_for_each_subnode(child, (void *)dtb_pa, reserved_mem) {
+ const char *name = fdt_get_name((void *)dtb_pa, child, NULL);
+
+ if (!strncmp(name, "mmode_resv", 10))
I would check that name != NULL before strncmp.
+ fdt_setprop((void *)dtb_pa, child, "no-map", NULL, 0);
+ }
+};
+
void __init sbi_init(void)
{
int ret;
@@ -596,8 +632,12 @@ void __init sbi_init(void)
sbi_major_version(), sbi_minor_version());
if (!sbi_spec_is_0_1()) {
+ sbi_firmware_id = sbi_get_firmware_id();
+ sbi_firmware_version = sbi_get_firmware_version();
+
pr_info("SBI implementation ID=0x%lx Version=0x%lx\n",
- sbi_get_firmware_id(), sbi_get_firmware_version());
+ sbi_firmware_id, sbi_firmware_version);
+
if (sbi_probe_extension(SBI_EXT_TIME)) {
__sbi_set_timer = __sbi_set_timer_v02;
pr_info("SBI TIME extension detected\n");
diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
index 70fb31960b63..cb16bfdeacdb 100644
--- a/arch/riscv/mm/init.c
+++ b/arch/riscv/mm/init.c
@@ -29,6 +29,7 @@
#include <asm/tlbflush.h>
#include <asm/sections.h>
#include <asm/soc.h>
+#include <asm/sbi.h>
#include <asm/io.h>
#include <asm/ptdump.h>
#include <asm/numa.h>
@@ -253,6 +254,8 @@ static void __init setup_bootmem(void)
* in the device tree, otherwise the allocation could end up in a
* reserved region.
*/
+
+ sbi_apply_reserved_mem_erratum(dtb_early_va);
early_init_fdt_scan_reserved_mem();
/*
Otherwise the patch looks good to me:
You can add:
Reviewed-by: Alexandre Ghiti <alexghiti@xxxxxxxxxxxx>
I just have one question though (that we discussed privately already):
should we fix openSBI in the kernel? If yes, what makes a bug worth
being fixed in the kernel?
Thanks,
