[RFC PATCH 0/1] fdt: Mark "/reserved-memory" nodes as nosave if !reusable

[RFC PATCH 0/1] fdt: Mark "/reserved-memory" nodes as nosave if !reusable

[Alexandre Ghiti]

In the RISC-V kernel, the firmware does not mark the region it uses as
"no-map" so that the kernel can avoid having holes in the linear mapping
and then use larger pages.

But with the recent support of hibernation on RISC-V, we noticed that
the hibernation process was trying to access those firmware regions
which are protected from *all* accesses (using PMP). So the hibernation
process fails.

We still don't have any "specification" regarding the naming of those
firmware regions, so we can't currently target those specific regions
(they are described as subnodes of "/reserved-memory").

I think we should actually mark all those "/reserved-memory" regions as
"nosave" since they should not be used by the kernel (see the
documentation pointed in the patch), hence the RFC since I may be
missing something here (legacy, other usage...etc).

Please let me know what you think!

Alexandre Ghiti (1):
  fdt: Mark "/reserved-memory" nodes as nosave if !reusable

 arch/riscv/kernel/setup.c |  2 +
 drivers/of/fdt.c          | 77 +++++++++++++++++++++++++++++++++++++++
 include/linux/of_fdt.h    |  1 +
 3 files changed, 80 insertions(+)

-- 
2.39.2


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[RFC PATCH 1/1] fdt: Mark "/reserved-memory" nodes as nosave if !reusable

[Alexandre Ghiti]

The hibernation process will access those reserved memory regions if
they are part of the linear mapping, but as described in
devicetree/bindings/reserved-memory/reserved-memory.yaml,
"/reserved-memory" nodes should not be used as normal memory, unless they
are marked as reusable which means the kernel can access it at some point.

Otherwise those regions are only used by drivers which should do what's
necessary when the hibernation process is started, or they can contain
the firmware reserved memory regions which should not be accessed at all.

Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
---
 arch/riscv/kernel/setup.c |  2 +
 drivers/of/fdt.c          | 77 +++++++++++++++++++++++++++++++++++++++
 include/linux/of_fdt.h    |  1 +
 3 files changed, 80 insertions(+)

diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
index 36b026057503..642f1035b5ce 100644
--- a/arch/riscv/kernel/setup.c
+++ b/arch/riscv/kernel/setup.c
@@ -299,6 +299,8 @@ void __init setup_arch(char **cmdline_p)
 	if (IS_ENABLED(CONFIG_RISCV_ISA_ZICBOM) &&
 	    riscv_isa_extension_available(NULL, ZICBOM))
 		riscv_noncoherent_supported();
+
+	early_init_fdt_nosave_reserved_mem();
 }
 
 static int __init topology_init(void)
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index bf502ba8da95..863de7e6b10c 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -26,6 +26,7 @@
 #include <linux/serial_core.h>
 #include <linux/sysfs.h>
 #include <linux/random.h>
+#include <linux/suspend.h>
 
 #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
 #include <asm/page.h>
@@ -494,6 +495,43 @@ static int __init early_init_dt_reserve_memory(phys_addr_t base,
 	return memblock_reserve(base, size);
 }
 
+/*
+ * __reserved_mem_nosave_reg() - Make all memory described in 'reg' property as
+ * nosave, unless it is "reusable".
+ */
+static void __init __reserved_mem_nosave_reg(unsigned long node,
+					     const char *uname)
+{
+	int t_len = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
+	phys_addr_t base, size;
+	int len;
+	const __be32 *prop;
+	bool reusable;
+
+	prop = of_get_flat_dt_prop(node, "reg", &len);
+	if (!prop)
+		return;
+
+	if (len && len % t_len != 0) {
+		pr_err("Reserved memory: invalid reg property in '%s', skipping node.\n",
+		       uname);
+		return;
+	}
+
+	reusable = of_get_flat_dt_prop(node, "reusable", NULL) != NULL;
+
+	while (len >= t_len) {
+		base = dt_mem_next_cell(dt_root_addr_cells, &prop);
+		size = dt_mem_next_cell(dt_root_size_cells, &prop);
+
+		if (size && !reusable)
+			register_nosave_region(phys_to_pfn(base),
+					       phys_to_pfn(base + size));
+
+		len -= t_len;
+	}
+}
+
 /*
  * __reserved_mem_reserve_reg() - reserve all memory described in 'reg' property
  */
@@ -596,6 +634,38 @@ static int __init fdt_scan_reserved_mem(void)
 	return 0;
 }
 
+/*
+ * fdt_nosave_reserved_mem() - scan a single FDT node to mark reserved memory
+ * as nosave.
+ */
+static int __init fdt_nosave_reserved_mem(void)
+{
+	int node, child;
+	const void *fdt = initial_boot_params;
+
+	node = fdt_path_offset(fdt, "/reserved-memory");
+	if (node < 0)
+		return -ENODEV;
+
+	if (__reserved_mem_check_root(node) != 0) {
+		pr_err("Reserved memory: unsupported node format, ignoring\n");
+		return -EINVAL;
+	}
+
+	fdt_for_each_subnode(child, fdt, node) {
+		const char *uname;
+
+		if (!of_fdt_device_is_available(fdt, child))
+			continue;
+
+		uname = fdt_get_name(fdt, child, NULL);
+
+		__reserved_mem_nosave_reg(child, uname);
+	}
+
+	return 0;
+}
+
 /*
  * fdt_reserve_elfcorehdr() - reserves memory for elf core header
  *
@@ -649,6 +719,13 @@ void __init early_init_fdt_scan_reserved_mem(void)
 	fdt_init_reserved_mem();
 }
 
+void __init early_init_fdt_nosave_reserved_mem(void)
+{
+#ifdef CONFIG_HIBERNATION
+	fdt_nosave_reserved_mem();
+#endif
+}
+
 /**
  * early_init_fdt_reserve_self() - reserve the memory used by the FDT blob
  */
diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
index d69ad5bb1eb1..55eb5a0f7305 100644
--- a/include/linux/of_fdt.h
+++ b/include/linux/of_fdt.h
@@ -63,6 +63,7 @@ extern int early_init_dt_scan_memory(void);
 extern void early_init_dt_check_for_usable_mem_range(void);
 extern int early_init_dt_scan_chosen_stdout(void);
 extern void early_init_fdt_scan_reserved_mem(void);
+extern void early_init_fdt_nosave_reserved_mem(void);
 extern void early_init_fdt_reserve_self(void);
 extern void early_init_dt_add_memory_arch(u64 base, u64 size);
 extern u64 dt_mem_next_cell(int s, const __be32 **cellp);
-- 
2.39.2


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC PATCH 1/1] fdt: Mark "/reserved-memory" nodes as nosave if !reusable

[Rob Herring]

On Tue, May 30, 2023 at 10:04:25AM +0200, Alexandre Ghiti wrote:
> The hibernation process will access those reserved memory regions if
> they are part of the linear mapping, but as described in
> devicetree/bindings/reserved-memory/reserved-memory.yaml,
> "/reserved-memory" nodes should not be used as normal memory, unless they
> are marked as reusable which means the kernel can access it at some point.
> 
> Otherwise those regions are only used by drivers which should do what's
> necessary when the hibernation process is started, or they can contain
> the firmware reserved memory regions which should not be accessed at all.

Hibernation is only one case. Speculative accesses could also occur. I 
think some of the memory debugging stuff will walk memory as well. If 
something can't be accessed, it better have 'no-map'.

> 
> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> ---
>  arch/riscv/kernel/setup.c |  2 +

How is this specific to Risc-V? Hint, it's not.

>  drivers/of/fdt.c          | 77 +++++++++++++++++++++++++++++++++++++++
>  include/linux/of_fdt.h    |  1 +
>  3 files changed, 80 insertions(+)
> 
> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> index 36b026057503..642f1035b5ce 100644
> --- a/arch/riscv/kernel/setup.c
> +++ b/arch/riscv/kernel/setup.c
> @@ -299,6 +299,8 @@ void __init setup_arch(char **cmdline_p)
>  	if (IS_ENABLED(CONFIG_RISCV_ISA_ZICBOM) &&
>  	    riscv_isa_extension_available(NULL, ZICBOM))
>  		riscv_noncoherent_supported();
> +
> +	early_init_fdt_nosave_reserved_mem();
>  }
>  
>  static int __init topology_init(void)
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index bf502ba8da95..863de7e6b10c 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -26,6 +26,7 @@
>  #include <linux/serial_core.h>
>  #include <linux/sysfs.h>
>  #include <linux/random.h>
> +#include <linux/suspend.h>
>  
>  #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
>  #include <asm/page.h>
> @@ -494,6 +495,43 @@ static int __init early_init_dt_reserve_memory(phys_addr_t base,
>  	return memblock_reserve(base, size);
>  }
>  
> +/*
> + * __reserved_mem_nosave_reg() - Make all memory described in 'reg' property as
> + * nosave, unless it is "reusable".
> + */
> +static void __init __reserved_mem_nosave_reg(unsigned long node,
> +					     const char *uname)
> +{
> +	int t_len = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
> +	phys_addr_t base, size;
> +	int len;
> +	const __be32 *prop;
> +	bool reusable;
> +
> +	prop = of_get_flat_dt_prop(node, "reg", &len);
> +	if (!prop)
> +		return;
> +
> +	if (len && len % t_len != 0) {
> +		pr_err("Reserved memory: invalid reg property in '%s', skipping node.\n",
> +		       uname);
> +		return;
> +	}
> +
> +	reusable = of_get_flat_dt_prop(node, "reusable", NULL) != NULL;
> +
> +	while (len >= t_len) {
> +		base = dt_mem_next_cell(dt_root_addr_cells, &prop);
> +		size = dt_mem_next_cell(dt_root_size_cells, &prop);
> +
> +		if (size && !reusable)
> +			register_nosave_region(phys_to_pfn(base),
> +					       phys_to_pfn(base + size));
> +
> +		len -= t_len;
> +	}
> +}
> +
>  /*
>   * __reserved_mem_reserve_reg() - reserve all memory described in 'reg' property
>   */
> @@ -596,6 +634,38 @@ static int __init fdt_scan_reserved_mem(void)
>  	return 0;
>  }
>  
> +/*
> + * fdt_nosave_reserved_mem() - scan a single FDT node to mark reserved memory
> + * as nosave.
> + */
> +static int __init fdt_nosave_reserved_mem(void)
> +{
> +	int node, child;
> +	const void *fdt = initial_boot_params;
> +
> +	node = fdt_path_offset(fdt, "/reserved-memory");
> +	if (node < 0)
> +		return -ENODEV;
> +
> +	if (__reserved_mem_check_root(node) != 0) {
> +		pr_err("Reserved memory: unsupported node format, ignoring\n");
> +		return -EINVAL;
> +	}
> +
> +	fdt_for_each_subnode(child, fdt, node) {
> +		const char *uname;
> +
> +		if (!of_fdt_device_is_available(fdt, child))
> +			continue;
> +
> +		uname = fdt_get_name(fdt, child, NULL);
> +
> +		__reserved_mem_nosave_reg(child, uname);
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * fdt_reserve_elfcorehdr() - reserves memory for elf core header
>   *
> @@ -649,6 +719,13 @@ void __init early_init_fdt_scan_reserved_mem(void)
>  	fdt_init_reserved_mem();
>  }
>  
> +void __init early_init_fdt_nosave_reserved_mem(void)
> +{
> +#ifdef CONFIG_HIBERNATION
> +	fdt_nosave_reserved_mem();
> +#endif
> +}
> +
>  /**
>   * early_init_fdt_reserve_self() - reserve the memory used by the FDT blob
>   */
> diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
> index d69ad5bb1eb1..55eb5a0f7305 100644
> --- a/include/linux/of_fdt.h
> +++ b/include/linux/of_fdt.h
> @@ -63,6 +63,7 @@ extern int early_init_dt_scan_memory(void);
>  extern void early_init_dt_check_for_usable_mem_range(void);
>  extern int early_init_dt_scan_chosen_stdout(void);
>  extern void early_init_fdt_scan_reserved_mem(void);
> +extern void early_init_fdt_nosave_reserved_mem(void);
>  extern void early_init_fdt_reserve_self(void);
>  extern void early_init_dt_add_memory_arch(u64 base, u64 size);
>  extern u64 dt_mem_next_cell(int s, const __be32 **cellp);
> -- 
> 2.39.2
> 

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv