[PATCH v3 0/2] mm/memblock: Add "reserve_mem" to reserved named memory at boot up

[PATCH v3 0/2] mm/memblock: Add "reserve_mem" to reserved named memory at boot up

[Steven Rostedt]

Reserve unspecified location of physical memory from kernel command line

Background:

In ChromeOS, we have 1 MB of pstore ramoops reserved so that we can extract
dmesg output and some other information when a crash happens in the field.
(This is only done when the user selects "Allow Google to collect data for
 improving the system"). But there are cases when there's a bug that
requires more data to be retrieved to figure out what is happening. We would
like to increase the pstore size, either temporarily, or maybe even
permanently. The pstore on these devices are at a fixed location in RAM (as
the RAM is not cleared on soft reboots nor crashes). The location is chosen
by the BIOS (coreboot) and passed to the kernel via ACPI tables on x86.
There's a driver that queries for this to initialize the pstore for
ChromeOS:

  See drivers/platform/chrome/chromeos_pstore.c

Problem:

The problem is that, even though there's a process to change the kernel on
these systems, and is done regularly to install updates, the firmware is
updated much less frequently. Choosing the place in RAM also takes special
care, and may be in a different address for different boards. Updating the
size via firmware is a large effort and not something that many are willing
to do for a temporary pstore size change.

Requirement:

Need a way to reserve memory that will be at a consistent location for
every boot, if the kernel and system are the same. Does not need to work
if rebooting to a different kernel, or if the system can change the
memory layout between boots.

The reserved memory can not be an hard coded address, as the same kernel /
command line needs to run on several different machines. The picked memory
reservation just needs to be the same for a given machine, but may be
different for different machines.

Solution:

The solution I have come up with is to introduce a new "reserve_mem=" kernel
command line. This parameter takes the following format:

  reserve_mem=nn:align:label

Where nn is the size of memory to reserve, the align is the alignment of
that memory, and label is the way for other sub-systems to find that memory.
This way the kernel command line could have:

  reserve_mem=12M:4096:oops   ramoops.mem_name=oops

At boot up, the kernel will search for 12 megabytes in usable memory regions
with an alignment of 4096. It will start at the highest regions and work its
way down (for those old devices that want access to lower address DMA). When
it finds a region, it will save it off in a small table and mark it with the
"oops" label. Then the pstore ramoops sub-system could ask for that memory
and location, and it will map itself there.

This prototype allows for 8 different mappings (which may be overkill, 4 is
probably plenty) with 16 byte size to store the label.

I have tested this and it works for us to solve the above problem. We can
update the kernel and command line and increase the size of pstore without
needing to update the firmware, or knowing every memory layout of each
board. I only tested this locally, it has not been tested in the field.

Changes since v2: https://lore.kernel.org/all/20240606150143.876469296@goodmis.org/

- Fixed typo of "reserver"

- Added EXPORT_SYMBOL_GPL() for reserve_mem_find_by_name()

- Removed "built-in" from module description that was changed from v1.


Changes since v1: https://lore.kernel.org/all/20240603233330.801075898@goodmis.org/

- Updated the change log of the first patch as well as added an entry
  into kernel-parameters.txt about how reserve_mem is for soft reboots
  and may not be reliable. 


Steven Rostedt (Google) (2):
      mm/memblock: Add "reserve_mem" to reserved named memory at boot up
      pstore/ramoops: Add ramoops.mem_name= command line option

----
 Documentation/admin-guide/kernel-parameters.txt | 20 +++++
 fs/pstore/ram.c                                 | 14 ++++
 include/linux/mm.h                              |  2 +
 mm/memblock.c                                   | 98 +++++++++++++++++++++++++
 4 files changed, 134 insertions(+)

[PATCH v3 1/2] mm/memblock: Add "reserve_mem" to reserved named memory at boot up

[Steven Rostedt]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

In order to allow for requesting a memory region that can be used for
things like pstore on multiple machines where the memory layout is not the
same, add a new option to the kernel command line called "reserve_mem".

The format is:  reserve_mem=nn:align:name

Where it will find nn amount of memory at the given alignment of align.
The name field is to allow another subsystem to retrieve where the memory
was found. For example:

  reserve_mem=12M:4096:oops ramoops.mem_name=oops

Where ramoops.mem_name will tell ramoops that memory was reserved for it
via the reserve_mem option and it can find it by calling:

  if (reserve_mem_find_by_name("oops", &start, &size)) {
	// start holds the start address and size holds the size given

This is typically used for systems that do not wipe the RAM, and this
command line will try to reserve the same physical memory on soft reboots.
Note, it is not guaranteed to be the same location. For example, if KASLR
places the kernel at the location of where the RAM reservation was from a
previous boot, the new reservation will be at a different location.  Any
subsystem using this feature must add a way to verify that the contents of
the physical memory is from a previous boot, as there may be cases where
the memory will not be located at the same location.

Not all systems may work either. There could be bit flips if the reboot
goes through the BIOS. Using kexec to reboot the machine is likely to
have better results in such cases.

Link: https://lore.kernel.org/all/ZjJVnZUX3NZiGW6q@kernel.org/

Suggested-by: Mike Rapoport <rppt@kernel.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
Changes since v2: https://lore.kernel.org/linux-trace-kernel/20240606150316.751642266@goodmis.org

- Fixed typo "reserver"

- Added EXPORT_SYMBOL_GPL() for reserve_mem_find_by_name()

 .../admin-guide/kernel-parameters.txt         | 20 ++++
 include/linux/mm.h                            |  2 +
 mm/memblock.c                                 | 98 +++++++++++++++++++
 3 files changed, 120 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index b600df82669d..ce7de8136f2f 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -5710,6 +5710,26 @@
 			them.  If <base> is less than 0x10000, the region
 			is assumed to be I/O ports; otherwise it is memory.
 
+	reserve_mem=	[RAM]
+			Format: nn[KNG]:<align>:<label>
+			Reserve physical memory and label it with a name that
+			other subsystems can use to access it. This is typically
+			used for systems that do not wipe the RAM, and this command
+			line will try to reserve the same physical memory on
+			soft reboots. Note, it is not guaranteed to be the same
+			location. For example, if KASLR places the kernel at the
+			location of where the RAM reservation was from a previous
+			boot, the new reservation will be at a different location.
+			Any subsystem using this feature must add a way to verify
+			that the contents of the physical memory is from a previous
+			boot, as there may be cases where the memory will not be
+			located at the same location.
+
+			The format is size:align:label for example, to request
+			12 megabytes of 4096 alignment for ramoops:
+
+			reserve_mem=12M:4096:oops ramoops.mem_name=oops
+
 	reservetop=	[X86-32,EARLY]
 			Format: nn[KMG]
 			Reserves a hole at the top of the kernel virtual
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 9849dfda44d4..b4455cc02f2c 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -4263,4 +4263,6 @@ static inline bool pfn_is_unaccepted_memory(unsigned long pfn)
 void vma_pgtable_walk_begin(struct vm_area_struct *vma);
 void vma_pgtable_walk_end(struct vm_area_struct *vma);
 
+int reserve_mem_find_by_name(const char *name, unsigned long *start, unsigned long *size);
+
 #endif /* _LINUX_MM_H */
diff --git a/mm/memblock.c b/mm/memblock.c
index d09136e040d3..60a2161de351 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -2244,6 +2244,104 @@ void __init memblock_free_all(void)
 	totalram_pages_add(pages);
 }
 
+/* Keep a table to reserve named memory */
+#define RESERVE_MEM_MAX_ENTRIES		8
+#define RESERVE_MEM_NAME_SIZE		16
+struct reserve_mem_table {
+	char			name[RESERVE_MEM_NAME_SIZE];
+	unsigned long		start;
+	unsigned long		size;
+};
+static struct reserve_mem_table reserved_mem_table[RESERVE_MEM_MAX_ENTRIES];
+static int reserved_mem_count;
+
+/* Add wildcard region with a lookup name */
+static int __init reserved_mem_add(unsigned long start, unsigned long size,
+				   const char *name)
+{
+	struct reserve_mem_table *map;
+
+	if (!name || !name[0] || strlen(name) >= RESERVE_MEM_NAME_SIZE)
+		return -EINVAL;
+
+	if (reserved_mem_count >= RESERVE_MEM_MAX_ENTRIES)
+		return -1;
+
+	map = &reserved_mem_table[reserved_mem_count++];
+	map->start = start;
+	map->size = size;
+	strscpy(map->name, name);
+	return 0;
+}
+
+/**
+ * reserve_mem_find_by_name - Find reserved memory region with a given name
+ * @name: The name that is attached to a reserved memory region
+ * @start: If found, holds the start address
+ * @size: If found, holds the size of the address.
+ *
+ * Returns: 1 if found or 0 if not found.
+ */
+int reserve_mem_find_by_name(const char *name, unsigned long *start, unsigned long *size)
+{
+	struct reserve_mem_table *map;
+	int i;
+
+	for (i = 0; i < reserved_mem_count; i++) {
+		map = &reserved_mem_table[i];
+		if (!map->size)
+			continue;
+		if (strcmp(name, map->name) == 0) {
+			*start = map->start;
+			*size = map->size;
+			return 1;
+		}
+	}
+	return 0;
+}
+EXPORT_SYMBOL_GPL(reserve_mem_find_by_name);
+
+/*
+ * Parse early_reserve_mem=nn:align:name
+ */
+static int __init reserve_mem(char *p)
+{
+	phys_addr_t start, size, align;
+	char *oldp;
+	int err;
+
+	if (!p)
+		return -EINVAL;
+
+	oldp = p;
+	size = memparse(p, &p);
+	if (p == oldp)
+		return -EINVAL;
+
+	if (*p != ':')
+		return -EINVAL;
+
+	align = memparse(p+1, &p);
+	if (*p != ':')
+		return -EINVAL;
+
+	start = memblock_phys_alloc(size, align);
+	if (!start)
+		return -ENOMEM;
+
+	p++;
+	err = reserved_mem_add(start, size, p);
+	if (err) {
+		memblock_phys_free(start, size);
+		return err;
+	}
+
+	p += strlen(p);
+
+	return *p == '\0' ? 0: -EINVAL;
+}
+__setup("reserve_mem=", reserve_mem);
+
 #if defined(CONFIG_DEBUG_FS) && defined(CONFIG_ARCH_KEEP_MEMBLOCK)
 static const char * const flagname[] = {
 	[ilog2(MEMBLOCK_HOTPLUG)] = "HOTPLUG",
-- 
2.43.0

[PATCH v3 2/2] pstore/ramoops: Add ramoops.mem_name= command line option

[Steven Rostedt]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

Add a method to find a region specified by reserve_mem=nn:align:name for
ramoops. Adding a kernel command line parameter:

  reserve_mem=12M:4096:oops ramoops.mem_name=oops

Will use the size and location defined by the memmap parameter where it
finds the memory and labels it "oops". The "oops" in the ramoops option
is used to search for it.

This allows for arbitrary RAM to be used for ramoops if it is known that
the memory is not cleared on kernel crashes or soft reboots.

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
Changes since v2: https://lore.kernel.org/linux-trace-kernel/20240606150316.916395285@goodmis.org

- Removed "built-in" from module description that was changed from v1.

 fs/pstore/ram.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index b1a455f42e93..0653b76c70fe 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -50,6 +50,10 @@ module_param_hw(mem_address, ullong, other, 0400);
 MODULE_PARM_DESC(mem_address,
 		"start of reserved RAM used to store oops/panic logs");
 
+static char *mem_name;
+module_param_named(mem_name, mem_name, charp, 0400);
+MODULE_PARM_DESC(mem_name, "name of kernel param that holds addr");
+
 static ulong mem_size;
 module_param(mem_size, ulong, 0400);
 MODULE_PARM_DESC(mem_size,
@@ -914,6 +918,16 @@ static void __init ramoops_register_dummy(void)
 {
 	struct ramoops_platform_data pdata;
 
+	if (mem_name) {
+		unsigned long start;
+		unsigned long size;
+
+		if (reserve_mem_find_by_name(mem_name, &start, &size)) {
+			mem_address = start;
+			mem_size = size;
+		}
+	}
+
 	/*
 	 * Prepare a dummy platform data structure to carry the module
 	 * parameters. If mem_size isn't set, then there are no module
-- 
2.43.0

Re: [PATCH v3 2/2] pstore/ramoops: Add ramoops.mem_name= command line option

[Kees Cook]

On Tue, Jun 11, 2024 at 10:49:13AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> Add a method to find a region specified by reserve_mem=nn:align:name for
> ramoops. Adding a kernel command line parameter:
> 
>   reserve_mem=12M:4096:oops ramoops.mem_name=oops
> 
> Will use the size and location defined by the memmap parameter where it
> finds the memory and labels it "oops". The "oops" in the ramoops option
> is used to search for it.
> 
> This allows for arbitrary RAM to be used for ramoops if it is known that
> the memory is not cleared on kernel crashes or soft reboots.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

Acked-by: Kees Cook <kees@kernel.org>

Let me know if this should go via the pstore tree, if you'd rather carry
it?

-- 
Kees Cook

Re: [PATCH v3 2/2] pstore/ramoops: Add ramoops.mem_name= command line option

[Steven Rostedt]

On Wed, 12 Jun 2024 11:45:57 -0700
Kees Cook <kees@kernel.org> wrote:

> On Tue, Jun 11, 2024 at 10:49:13AM -0400, Steven Rostedt wrote:
> > From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> > 
> > Add a method to find a region specified by reserve_mem=nn:align:name for
> > ramoops. Adding a kernel command line parameter:
> > 
> >   reserve_mem=12M:4096:oops ramoops.mem_name=oops
> > 
> > Will use the size and location defined by the memmap parameter where it
> > finds the memory and labels it "oops". The "oops" in the ramoops option
> > is used to search for it.
> > 
> > This allows for arbitrary RAM to be used for ramoops if it is known that
> > the memory is not cleared on kernel crashes or soft reboots.
> > 
> > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>  
> 
> Acked-by: Kees Cook <kees@kernel.org>
> 
> Let me know if this should go via the pstore tree, if you'd rather carry
> it?
> 

I'm going to send out another version to cover some more comments that
were made, and perhaps it's best if Mike Rapoport takes it through his
tree.

Thanks!

-- Steve

Re: [PATCH v3 2/2] pstore/ramoops: Add ramoops.mem_name= command line option

[Mike Rapoport]

On Wed, Jun 12, 2024 at 02:52:28PM -0400, Steven Rostedt wrote:
> On Wed, 12 Jun 2024 11:45:57 -0700
> Kees Cook <kees@kernel.org> wrote:
> 
> > On Tue, Jun 11, 2024 at 10:49:13AM -0400, Steven Rostedt wrote:
> > > From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> > > 
> > > Add a method to find a region specified by reserve_mem=nn:align:name for
> > > ramoops. Adding a kernel command line parameter:
> > > 
> > >   reserve_mem=12M:4096:oops ramoops.mem_name=oops
> > > 
> > > Will use the size and location defined by the memmap parameter where it
> > > finds the memory and labels it "oops". The "oops" in the ramoops option
> > > is used to search for it.
> > > 
> > > This allows for arbitrary RAM to be used for ramoops if it is known that
> > > the memory is not cleared on kernel crashes or soft reboots.
> > > 
> > > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>  
> > 
> > Acked-by: Kees Cook <kees@kernel.org>
> > 
> > Let me know if this should go via the pstore tree, if you'd rather carry
> > it?
> > 
> 
> I'm going to send out another version to cover some more comments that
> were made, and perhaps it's best if Mike Rapoport takes it through his
> tree.

I've added one more comment to v5, with that fixed I can take this.
 
> Thanks!
> 
> -- Steve

-- 
Sincerely yours,
Mike.

Re: [PATCH v3 2/2] pstore/ramoops: Add ramoops.mem_name= command line option

[Ard Biesheuvel]

On Thu, 13 Jun 2024 at 08:04, Mike Rapoport <rppt@kernel.org> wrote:
>
> On Wed, Jun 12, 2024 at 02:52:28PM -0400, Steven Rostedt wrote:
> > On Wed, 12 Jun 2024 11:45:57 -0700
> > Kees Cook <kees@kernel.org> wrote:
> >
> > > On Tue, Jun 11, 2024 at 10:49:13AM -0400, Steven Rostedt wrote:
> > > > From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> > > >
> > > > Add a method to find a region specified by reserve_mem=nn:align:name for
> > > > ramoops. Adding a kernel command line parameter:
> > > >
> > > >   reserve_mem=12M:4096:oops ramoops.mem_name=oops
> > > >
> > > > Will use the size and location defined by the memmap parameter where it
> > > > finds the memory and labels it "oops". The "oops" in the ramoops option
> > > > is used to search for it.
> > > >
> > > > This allows for arbitrary RAM to be used for ramoops if it is known that
> > > > the memory is not cleared on kernel crashes or soft reboots.
> > > >
> > > > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> > >
> > > Acked-by: Kees Cook <kees@kernel.org>
> > >
> > > Let me know if this should go via the pstore tree, if you'd rather carry
> > > it?
> > >
> >
> > I'm going to send out another version to cover some more comments that
> > were made, and perhaps it's best if Mike Rapoport takes it through his
> > tree.
>
> I've added one more comment to v5, with that fixed I can take this.
>

So how is this supposed to work wrt to the rigid 'no user visible
regressions' rule, given that this whole thing is a best effort thing
to begin with. This needs at least a huge disclaimer that this rule
does not apply, and if this works today, there is no guarantee that it
will keep working on newer kernels. Otherwise, you will be making the
job of the people who work on the boot code significantly more
difficult. And even then, I wonder whether Linus and #regzcop are
going to honour such a disclaimer.

So this belongs downstream, unless some guarantees can be provided
that this functionality is exempt from the usual regression policies.

Re: [PATCH v3 2/2] pstore/ramoops: Add ramoops.mem_name= command line option

[Steven Rostedt]

On Thu, 13 Jun 2024 08:11:48 +0200
Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > I've added one more comment to v5, with that fixed I can take this.
> >  
> 
> So how is this supposed to work wrt to the rigid 'no user visible
> regressions' rule, given that this whole thing is a best effort thing

This has nothing to do with user space. The kernel command line has
broken in the past. If you update the kernel, you can update the
command line. There's no "no user visible regressions" rule. It's
"Don't break user space". This has nothing to do with user space.

> to begin with. This needs at least a huge disclaimer that this rule
> does not apply, and if this works today, there is no guarantee that it
> will keep working on newer kernels. Otherwise, you will be making the
> job of the people who work on the boot code significantly more
> difficult. And even then, I wonder whether Linus and #regzcop are
> going to honour such a disclaimer.

Again, this has nothing to do with user space. The rule Linus talks
about is breaking user space. This is about kernel debugging. Something
*completely different*!

> 
> So this belongs downstream, unless some guarantees can be provided
> that this functionality is exempt from the usual regression policies.

I disagree. kexec/kdump also has the same issues.


-- Steve

Re: [PATCH v3 2/2] pstore/ramoops: Add ramoops.mem_name= command line option

[Ard Biesheuvel]

On Thu, 13 Jun 2024 at 15:26, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Thu, 13 Jun 2024 08:11:48 +0200
> Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > I've added one more comment to v5, with that fixed I can take this.
> > >
> >
> > So how is this supposed to work wrt to the rigid 'no user visible
> > regressions' rule, given that this whole thing is a best effort thing
>
> This has nothing to do with user space. The kernel command line has
> broken in the past. If you update the kernel, you can update the
> command line. There's no "no user visible regressions" rule. It's
> "Don't break user space". This has nothing to do with user space.
>
> > to begin with. This needs at least a huge disclaimer that this rule
> > does not apply, and if this works today, there is no guarantee that it
> > will keep working on newer kernels. Otherwise, you will be making the
> > job of the people who work on the boot code significantly more
> > difficult. And even then, I wonder whether Linus and #regzcop are
> > going to honour such a disclaimer.
>
> Again, this has nothing to do with user space. The rule Linus talks
> about is breaking user space. This is about kernel debugging. Something
> *completely different*!
>
> >
> > So this belongs downstream, unless some guarantees can be provided
> > that this functionality is exempt from the usual regression policies.
>
> I disagree. kexec/kdump also has the same issues.
>

Fair enough. As long as it is documented that there is no guarantee
that this will keep working over a kernel upgrade, then I have no
objections.

Re: [PATCH v3 2/2] pstore/ramoops: Add ramoops.mem_name= command line option

[Steven Rostedt]

On Thu, 13 Jun 2024 16:06:47 +0200
Ard Biesheuvel <ardb@kernel.org> wrote:

> Fair enough. As long as it is documented that there is no guarantee
> that this will keep working over a kernel upgrade, then I have no
> objections.

Thanks,

I'll send out v6 and even stress that this may not work for all kernel
versions.

-- Steve

Re: [PATCH v3 2/2] pstore/ramoops: Add ramoops.mem_name= command line option

[Kees Cook]

On June 13, 2024 7:06:47 AM PDT, Ard Biesheuvel <ardb@kernel.org> wrote:
>On Thu, 13 Jun 2024 at 15:26, Steven Rostedt <rostedt@goodmis.org> wrote:
>>
>> On Thu, 13 Jun 2024 08:11:48 +0200
>> Ard Biesheuvel <ardb@kernel.org> wrote:
>> > >
>> > > I've added one more comment to v5, with that fixed I can take this.
>> > >
>> >
>> > So how is this supposed to work wrt to the rigid 'no user visible
>> > regressions' rule, given that this whole thing is a best effort thing
>>
>> This has nothing to do with user space. The kernel command line has
>> broken in the past. If you update the kernel, you can update the
>> command line. There's no "no user visible regressions" rule. It's
>> "Don't break user space". This has nothing to do with user space.
>>
>> > to begin with. This needs at least a huge disclaimer that this rule
>> > does not apply, and if this works today, there is no guarantee that it
>> > will keep working on newer kernels. Otherwise, you will be making the
>> > job of the people who work on the boot code significantly more
>> > difficult. And even then, I wonder whether Linus and #regzcop are
>> > going to honour such a disclaimer.
>>
>> Again, this has nothing to do with user space. The rule Linus talks
>> about is breaking user space. This is about kernel debugging. Something
>> *completely different*!
>>
>> >
>> > So this belongs downstream, unless some guarantees can be provided
>> > that this functionality is exempt from the usual regression policies.
>>
>> I disagree. kexec/kdump also has the same issues.
>>
>
>Fair enough. As long as it is documented that there is no guarantee
>that this will keep working over a kernel upgrade, then I have no
>objections.

Yeah, I should better document this for pstore as a whole, but I've already made the call that cross-kernel-versison operation is best effort.

-Kees

-- 
Kees Cook