Re: [PATCH v5 08/17] arm64: mm: Add dynamic ramoops region support through command line

[Pavan Kondeti]

On Sun, Sep 10, 2023 at 01:46:09AM +0530, Mukesh Ojha wrote:
> The reserved memory region for ramoops is assumed to be at a fixed
> and known location when read from the devicetree. This may not be
> required for something like Qualcomm's minidump which is interested
> in knowing addresses of ramoops region but it does not put hard
> requirement of address being fixed as most of it's SoC does not
> support warm reset and does not use pstorefs at all instead it has
> firmware way of collecting ramoops region if it gets to know the
> address and register it with apss minidump table which is sitting
> in shared memory region in DDR and firmware will have access to
> these table during reset and collects it on crash of SoC.
> 
> So, add the support of reserving ramoops region to be dynamically
> allocated early during boot if it is request through command line
> via 'dyn_ramoops_size=' and fill up reserved resource structure and
> export the structure, so that it can be read by ramoops driver.
> 

This needs to be documented at
Documentation/admin-guide/kernel-parameters.txt

> Signed-off-by: Mukesh Ojha <quic_mojha@xxxxxxxxxxx>
> ---
>  arch/arm64/mm/init.c       | 94 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pstore_ram.h |  2 +
>  2 files changed, 96 insertions(+)
> 
> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> index d31c3a9290c5..14d7086758bf 100644
> --- a/arch/arm64/mm/init.c
> +++ b/arch/arm64/mm/init.c
> @@ -31,6 +31,7 @@
>  #include <linux/hugetlb.h>
>  #include <linux/acpi_iort.h>
>  #include <linux/kmemleak.h>
> +#include <linux/pstore_ram.h>
>  
>  #include <asm/boot.h>
>  #include <asm/fixmap.h>
> @@ -73,6 +74,93 @@ phys_addr_t __ro_after_init arm64_dma_phys_limit;
>  
>  #define DEFAULT_CRASH_KERNEL_LOW_SIZE	(128UL << 20)
>  
> +#define RAMOOPS_ADDR_HIGH_MAX		(PHYS_MASK + 1)
> +
> +/* Location of the reserved area for the dynamic ramoops */
> +struct resource dyn_ramoops_res = {
> +	.name  = "ramoops",
> +	.start = 0,
> +	.end   = 0,
> +	.flags = IORESOURCE_BUSY | IORESOURCE_SYSTEM_RAM,
> +	.desc  = IORES_DESC_NONE,
> +};
> +EXPORT_SYMBOL(dyn_ramoops_res);

Use EXPORT_SYMBOL_GPL.

> +
> +static int __init parse_dynamic_ramoops(char *cmdline, unsigned long long *size)
> +{
> +	const char *name = "dyn_ramoops_size=";
> +	char *p = NULL;
> +	char *q = NULL;
> +	char *tmp;
> +
> +	if (!cmdline)
> +		return -ENOENT;
> +
> +	/* Check for "dyn_ramoops_size" and use the later if there are more */
> +	p = strstr(cmdline, name);
> +	while (p) {
> +		q = p;
> +		p = strchr(p, ' ');
> +		if (!p)
> +			break;
> +
> +		p = strstr(p + 1, name);
> +	}
> +
> +	if (!q) {
> +		pr_err("ramoops: No entry found for %s\n", name);
> +		return -ENOENT;
> +	}
> +
> +	p = q + strlen(name);
> +	*size = memparse(p, &tmp);
> +	if (p == tmp) {
> +		pr_err("ramoops: memory value expected\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int __init parse_dyn_ramoops_size_dummy(char *arg)
> +{
> +	return 0;
> +}
> +early_param("dyn_ramoops_size", parse_dyn_ramoops_size_dummy);
> +

Any reason why we can't parse and cache the size in early param handler
it self?

> +/*
> + * reserve_dynamic_ramoops() - reserves memory for dynamic ramoops
> + *
> + * This enable dynamic reserve memory support for ramoops through
> + * command line.
> + */
> +static void __init reserve_dynamic_ramoops(void)
> +{
> +	char *cmdline = boot_command_line;
> +	unsigned long long ramoops_base;
> +	unsigned long long ramoops_size;
> +
> +	if (!IS_ENABLED(CONFIG_PSTORE_RAM))
> +		return;
> +

Should not most part of this patch be under CONFIG_PSTORE_RAM?

> +	if (parse_dynamic_ramoops(cmdline, &ramoops_size))
> +		return;
> +
> +	ramoops_base = memblock_phys_alloc_range(ramoops_size, SMP_CACHE_BYTES,
> +						 0, RAMOOPS_ADDR_HIGH_MAX);

It may be appropriate to use one of MEMBLOCK_ALLOC_xxx flags for the end
marker.

> +	if (!ramoops_base) {
> +		pr_err("cannot allocate ramoops dynamic memory (size:0x%llx).\n",
> +			ramoops_size);
> +		return;
> +	}
> +
> +	kmemleak_ignore_phys(ramoops_base);

Looks like you need MEMBLOCK_ALLOC_NOLEAKTRACE

> +
> +	dyn_ramoops_res.start = ramoops_base;
> +	dyn_ramoops_res.end = ramoops_base + ramoops_size - 1;
> +	insert_resource(&iomem_resource, &dyn_ramoops_res);
> +}
> +
>  static int __init reserve_crashkernel_low(unsigned long long low_size)
>  {
>  	unsigned long long low_base;
> @@ -456,6 +544,12 @@ void __init bootmem_init(void)
>  	 */
>  	reserve_crashkernel();
>  
> +	/*
> +	 * Reserving ramoops region resource dynamically in case it is
> +	 * requested from command line.
> +	 */
> +	reserve_dynamic_ramoops();
> +
>  	memblock_dump_all();
>  }
>  
> diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
> index 9d65ff94e216..07d700b7649d 100644
> --- a/include/linux/pstore_ram.h
> +++ b/include/linux/pstore_ram.h
> @@ -10,6 +10,8 @@
>  
>  #include <linux/pstore.h>
>  
> +extern struct resource dyn_ramoops_res;
> +

What about other architectures?

>  struct persistent_ram_ecc_info {
>  	int block_size;
>  	int ecc_size;
> -- 
> 2.7.4
>