Received-SPF: pass (google.com: domain of linux-kernel+bounces-28971-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <6cb0b6bb-9ef9-a7c9-0f59-fbd760aac644@quicinc.com>
Date: Wed, 17 Jan 2024 18:07:46 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH v7 08/12] pstore/ram: Add dynamic ramoops region support
 through commandline
Content-Language: en-US
To: Randy Dunlap <rdunlap@infradead.org>, <corbet@lwn.net>,
        <andersson@kernel.org>, <konrad.dybcio@linaro.org>,
        <robh+dt@kernel.org>, <krzysztof.kozlowski+dt@linaro.org>,
        <conor+dt@kernel.org>, <keescook@chromium.org>, <tony.luck@intel.com>,
        <gpiccoli@igalia.com>, <mathieu.poirier@linaro.org>, <vigneshr@ti.com>,
        <nm@ti.com>, <matthias.bgg@gmail.com>, <kgene@kernel.org>,
        <alim.akhtar@samsung.com>, <bmasney@redhat.com>
CC: <linux-doc@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20240109153200.12848-1-quic_mojha@quicinc.com>
 <20240109153200.12848-9-quic_mojha@quicinc.com>
 <e48c1e84-b27e-4109-b80d-084ba8a399ff@infradead.org>
From: Mukesh Ojha <quic_mojha@quicinc.com>
In-Reply-To: <e48c1e84-b27e-4109-b80d-084ba8a399ff@infradead.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 1/10/2024 4:03 AM, Randy Dunlap wrote:
> 
> 
> On 1/9/24 07:31, 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
> 
>                                                  its
> 
>> 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=<size>' and fill up reserved resource structure
>> and export the structure, so that it can be read by ramoops driver.
>>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>> ---
>>   Documentation/admin-guide/ramoops.rst | 23 +++++++++-
>>   fs/pstore/Kconfig                     | 15 ++++++
>>   fs/pstore/ram.c                       | 66 +++++++++++++++++++++++++--
>>   include/linux/pstore_ram.h            |  5 ++
>>   init/main.c                           |  2 +
>>   5 files changed, 107 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/admin-guide/ramoops.rst b/Documentation/admin-guide/ramoops.rst
>> index e9f85142182d..517b00981e99 100644
>> --- a/Documentation/admin-guide/ramoops.rst
>> +++ b/Documentation/admin-guide/ramoops.rst
>> @@ -33,6 +33,13 @@ memory are implementation defined, and won't work on many ARMs such as omaps.
>>   Setting ``mem_type=2`` attempts to treat the memory region as normal memory,
>>   which enables full cache on it. This can improve the performance.
>>   
>> +Ramoops supports its memory to be allocated dynamically during early boot
>> +for plaforms that does not have support for warm boot i.e., no assurance
> 
>         platforms that do not have
> 
> 
>> +of Ram content will be preserved across boot and for these platform
> 
>     that RAM content                             and for these platforms
> 
>> +giving static Ramoops memory is not necessary as it has separate backend
>> +mechanism to retrieve ramoops content on system failure. More about
>> +how to enable Dynamic ramoops in ``Setting the parameters`` A.b section.
>> +
>>   The memory area is divided into ``record_size`` chunks (also rounded down to
>>   power of two) and each kmesg dump writes a ``record_size`` chunk of
>>   information.
>> @@ -59,7 +66,7 @@ Setting the parameters
>>   
>>   Setting the ramoops parameters can be done in several different manners:
>>   
>> - A. Use the module parameters (which have the names of the variables described
>> + A.a  Use the module parameters (which have the names of the variables described
>>    as before). For quick debugging, you can also reserve parts of memory during
>>    boot and then use the reserved memory for ramoops. For example, assuming a
>>    machine with > 128 MB of memory, the following kernel command line will tell
>> @@ -68,6 +75,20 @@ Setting the ramoops parameters can be done in several different manners:
>>   
>>   	mem=128M ramoops.mem_address=0x8000000 ramoops.ecc=1
>>   
>> + A.b  Ramoops memory can be also be dynamically reserve by Kernel and in such
> 
>                                                     reserved
> 
>> + scenario ``mem_address`` i.e, Ramoops base address can be anywhere in the RAM
> 
>                               i.e.,
> 
>> + instead of being fixed and predefined. A separate command line option
>> + ``dyn_ramoops_size=<size>`` and kernel config CONFIG_PSTORE_DYNAMIC_RAMOOPS
>> + is provided to facilitate Dynamic Ramoops memory reservation during early boot.
> 
>      are provided
> 
>> + The command line option and the config should only be used in the presence of
>> + separate backend which knows how to recover Dynamic Ramoops region otherwise
>> + regular ramoops functionality will be impacted.
>> + ``mem_size`` should not be used if Dynamic Ramoops support is requested and if
>> + both are given ``mem_size`` value is overwritten with ``dyn_ramoops_size`` value
>> + i.e, Dynamic Ramoops takes precedence::
> 
>      i.e.,
> 
>> +
>> +	dyn_ramoops_size=2M ramoops.console_size=2097152
>> +
>>    B. Use Device Tree bindings, as described in
>>    ``Documentation/devicetree/bindings/reserved-memory/ramoops.yaml``.
>>    For example::
>> diff --git a/fs/pstore/Kconfig b/fs/pstore/Kconfig
>> index 3acc38600cd1..2f2bb483fd85 100644
>> --- a/fs/pstore/Kconfig
>> +++ b/fs/pstore/Kconfig
>> @@ -81,6 +81,21 @@ config PSTORE_RAM
>>   
>>   	  For more information, see Documentation/admin-guide/ramoops.rst.
>>   
>> +config PSTORE_DYNAMIC_RAMOOPS
>> +	bool "Reserve ramoops region dynamically"
>> +	select PSTORE_RAM
>> +	help
>> +	  This enables the dynamic reservation of ramoops region for a special case
>> +	  where there is no requirement to access the logs from pstorefs on next boot
> 
> 	                                                                         boot;
> 
>> +	  instead there is separate backend mechanism like minidump present which has
>> +	  awareness about the dynamic ramoops region and can recover the logs. This is
>> +	  enabled via command line parameter dyn_ramoops_size=<size> and should not be
>> +	  used in absence of separate backend which knows how to recover this dynamic
>> +	  region.
>> +
>> +	  Note whenever this config is selected ramoops driver will be build statically
> 
> 	                                                               built
> 
>> +	  into kernel.
>> +
>>   config PSTORE_ZONE
>>   	tristate
>>   	depends on PSTORE
>> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
>> index 88b34fdbf759..795a8300631e 100644
>> --- a/fs/pstore/ram.c
>> +++ b/fs/pstore/ram.c
>> @@ -20,6 +20,7 @@
>>   #include <linux/compiler.h>
>>   #include <linux/of.h>
>>   #include <linux/of_address.h>
>> +#include <linux/memblock.h>
>>   #include <linux/mm.h>
>>   
>>   #include "internal.h"
>> @@ -103,6 +104,59 @@ struct ramoops_context {
>>   };
>>   
>>   static struct platform_device *dummy;
>> +static struct resource dyn_ramoops_res = {
>> +	.name  = "ramoops",
>> +	.start = 0,
>> +	.end   = 0,
>> +	.flags = IORESOURCE_BUSY | IORESOURCE_SYSTEM_RAM,
>> +	.desc  = IORES_DESC_NONE,
>> +};
>> +static int dyn_ramoops_size;
>> +
>> +#ifdef CONFIG_PSTORE_DYNAMIC_RAMOOPS
>> +static int __init parse_dyn_ramoops_size(char *p)
>> +{
>> +	char *tmp;
>> +
>> +	dyn_ramoops_size = memparse(p, &tmp);
>> +	if (p == tmp) {
>> +		pr_err("ramoops: memory size expected\n");
>> +		dyn_ramoops_size = 0;
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +early_param("dyn_ramoops_size", parse_dyn_ramoops_size);
>> +
>> +/*
>> + * setup_dynamic_ramoops() - reserves memory for dynamic ramoops
>> + *
>> + * This enable dynamic reserve memory support for ramoops through
> 
>             enables

Thanks for the review, Noted all the points for the next version.

-Mukesh
> 
>> + * command line.
>> + */
> 
>