[RESEND PATCH v2 1/2] pstore: Add mem_type property DT parsing support

[RESEND PATCH v2 1/2] pstore: Add mem_type property DT parsing support

[Mukesh Ojha]

There could be a sceanario where we define some region
in normal memory and use them store to logs which is later
retrieved by bootloader during warm reset.

In this scenario, we wanted to treat this memory as normal
cacheable memory instead of default behaviour which
is an overhead. Making it cacheable could improve
performance.

This commit gives control to change mem_type from Device
tree, and also documents the value for normal memory.

Signed-off-by: Mukesh Ojha <mojha@codeaurora.org>
---
Changes in v2:
 - if-else converted to switch case
 - updated MODULE_PARM_DESC with new memory type.
 - default setting is still intact.

 Documentation/admin-guide/ramoops.rst |  4 +++-
 fs/pstore/ram.c                       |  3 ++-
 fs/pstore/ram_core.c                  | 18 ++++++++++++++++--
 3 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/Documentation/admin-guide/ramoops.rst b/Documentation/admin-guide/ramoops.rst
index b0a1ae7..8f107d8 100644
--- a/Documentation/admin-guide/ramoops.rst
+++ b/Documentation/admin-guide/ramoops.rst
@@ -3,7 +3,7 @@ Ramoops oops/panic logger
 
 Sergiu Iordache <sergiu@chromium.org>
 
-Updated: 17 November 2011
+Updated: 10 Feb 2021
 
 Introduction
 ------------
@@ -30,6 +30,8 @@ mapping to pgprot_writecombine. Setting ``mem_type=1`` attempts to use
 depends on atomic operations. At least on ARM, pgprot_noncached causes the
 memory to be mapped strongly ordered, and atomic operations on strongly ordered
 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.
 
 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
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index ca6d8a8..af4ca6a4 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -56,7 +56,7 @@ MODULE_PARM_DESC(mem_size,
 static unsigned int mem_type;
 module_param(mem_type, uint, 0400);
 MODULE_PARM_DESC(mem_type,
-		"set to 1 to try to use unbuffered memory (default 0)");
+		"set to 1 to use unbuffered memory, 2 for cached memory (default 0)");
 
 static int ramoops_max_reason = -1;
 module_param_named(max_reason, ramoops_max_reason, int, 0400);
@@ -666,6 +666,7 @@ static int ramoops_parse_dt(struct platform_device *pdev,
 		field = value;						\
 	}
 
+	parse_u32("mem-type", pdata->record_size, pdata->mem_type);
 	parse_u32("record-size", pdata->record_size, 0);
 	parse_u32("console-size", pdata->console_size, 0);
 	parse_u32("ftrace-size", pdata->ftrace_size, 0);
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index aa8e0b6..0da012f 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -396,6 +396,10 @@ void persistent_ram_zap(struct persistent_ram_zone *prz)
 	persistent_ram_update_header_ecc(prz);
 }
 
+#define MEM_TYPE_WCOMBINE	0
+#define MEM_TYPE_NONCACHED	1
+#define MEM_TYPE_NORMAL		2
+
 static void *persistent_ram_vmap(phys_addr_t start, size_t size,
 		unsigned int memtype)
 {
@@ -409,10 +413,20 @@ static void *persistent_ram_vmap(phys_addr_t start, size_t size,
 	page_start = start - offset_in_page(start);
 	page_count = DIV_ROUND_UP(size + offset_in_page(start), PAGE_SIZE);
 
-	if (memtype)
+	switch (memtype) {
+	case MEM_TYPE_NORMAL:
+		prot = PAGE_KERNEL;
+		break;
+	case MEM_TYPE_NONCACHED:
 		prot = pgprot_noncached(PAGE_KERNEL);
-	else
+		break;
+	case MEM_TYPE_WCOMBINE:
 		prot = pgprot_writecombine(PAGE_KERNEL);
+		break;
+	default:
+		pr_err("invalid memory type\n");
+		return NULL;
+	}
 
 	pages = kmalloc_array(page_count, sizeof(struct page *), GFP_KERNEL);
 	if (!pages) {
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

[RESEND PATCH v2 2/2] pstore: Add buffer start check during init

[Mukesh Ojha]

From: Huang Yiwei <hyiwei@codeaurora.org>

In a scenario of panic, when we use DRAM to store log instead
of persistant storage and during warm reset when we copy these
data outside of ram. Missing check on prz->start(write position)
can cause crash because it can be any value and can point outside
the mapped region. So add the start check to avoid.

Signed-off-by: Huang Yiwei <hyiwei@codeaurora.org>
Signed-off-by: Mukesh Ojha <mojha@codeaurora.org>
---
change in v2:
 - this is on top of first patchset.

 fs/pstore/ram_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index 0da012f..a15748a 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -514,7 +514,7 @@ static int persistent_ram_post_init(struct persistent_ram_zone *prz, u32 sig,
 	sig ^= PERSISTENT_RAM_SIG;
 
 	if (prz->buffer->sig == sig) {
-		if (buffer_size(prz) == 0) {
+		if (buffer_size(prz) == 0 && buffer_start(prz) == 0) {
 			pr_debug("found existing empty buffer\n");
 			return 0;
 		}
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [RESEND PATCH v2 1/2] pstore: Add mem_type property DT parsing support

[Mukesh Ojha]

Hi Kees,

i have updated the patch based on your last comments.
please review.

Thanks,
Mukesh

On 2/25/2021 9:30 PM, Mukesh Ojha wrote:
> There could be a sceanario where we define some region
> in normal memory and use them store to logs which is later
> retrieved by bootloader during warm reset.
>
> In this scenario, we wanted to treat this memory as normal
> cacheable memory instead of default behaviour which
> is an overhead. Making it cacheable could improve
> performance.
>
> This commit gives control to change mem_type from Device
> tree, and also documents the value for normal memory.
>
> Signed-off-by: Mukesh Ojha <mojha@codeaurora.org>
> ---
> Changes in v2:
>   - if-else converted to switch case
>   - updated MODULE_PARM_DESC with new memory type.
>   - default setting is still intact.
>
>   Documentation/admin-guide/ramoops.rst |  4 +++-
>   fs/pstore/ram.c                       |  3 ++-
>   fs/pstore/ram_core.c                  | 18 ++++++++++++++++--
>   3 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/admin-guide/ramoops.rst b/Documentation/admin-guide/ramoops.rst
> index b0a1ae7..8f107d8 100644
> --- a/Documentation/admin-guide/ramoops.rst
> +++ b/Documentation/admin-guide/ramoops.rst
> @@ -3,7 +3,7 @@ Ramoops oops/panic logger
>   
>   Sergiu Iordache <sergiu@chromium.org>
>   
> -Updated: 17 November 2011
> +Updated: 10 Feb 2021
>   
>   Introduction
>   ------------
> @@ -30,6 +30,8 @@ mapping to pgprot_writecombine. Setting ``mem_type=1`` attempts to use
>   depends on atomic operations. At least on ARM, pgprot_noncached causes the
>   memory to be mapped strongly ordered, and atomic operations on strongly ordered
>   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.
>   
>   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
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index ca6d8a8..af4ca6a4 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -56,7 +56,7 @@ MODULE_PARM_DESC(mem_size,
>   static unsigned int mem_type;
>   module_param(mem_type, uint, 0400);
>   MODULE_PARM_DESC(mem_type,
> -		"set to 1 to try to use unbuffered memory (default 0)");
> +		"set to 1 to use unbuffered memory, 2 for cached memory (default 0)");
>   
>   static int ramoops_max_reason = -1;
>   module_param_named(max_reason, ramoops_max_reason, int, 0400);
> @@ -666,6 +666,7 @@ static int ramoops_parse_dt(struct platform_device *pdev,
>   		field = value;						\
>   	}
>   
> +	parse_u32("mem-type", pdata->record_size, pdata->mem_type);
>   	parse_u32("record-size", pdata->record_size, 0);
>   	parse_u32("console-size", pdata->console_size, 0);
>   	parse_u32("ftrace-size", pdata->ftrace_size, 0);
> diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
> index aa8e0b6..0da012f 100644
> --- a/fs/pstore/ram_core.c
> +++ b/fs/pstore/ram_core.c
> @@ -396,6 +396,10 @@ void persistent_ram_zap(struct persistent_ram_zone *prz)
>   	persistent_ram_update_header_ecc(prz);
>   }
>   
> +#define MEM_TYPE_WCOMBINE	0
> +#define MEM_TYPE_NONCACHED	1
> +#define MEM_TYPE_NORMAL		2
> +
>   static void *persistent_ram_vmap(phys_addr_t start, size_t size,
>   		unsigned int memtype)
>   {
> @@ -409,10 +413,20 @@ static void *persistent_ram_vmap(phys_addr_t start, size_t size,
>   	page_start = start - offset_in_page(start);
>   	page_count = DIV_ROUND_UP(size + offset_in_page(start), PAGE_SIZE);
>   
> -	if (memtype)
> +	switch (memtype) {
> +	case MEM_TYPE_NORMAL:
> +		prot = PAGE_KERNEL;
> +		break;
> +	case MEM_TYPE_NONCACHED:
>   		prot = pgprot_noncached(PAGE_KERNEL);
> -	else
> +		break;
> +	case MEM_TYPE_WCOMBINE:
>   		prot = pgprot_writecombine(PAGE_KERNEL);
> +		break;
> +	default:
> +		pr_err("invalid memory type\n");
> +		return NULL;
> +	}
>   
>   	pages = kmalloc_array(page_count, sizeof(struct page *), GFP_KERNEL);
>   	if (!pages) {

Re: [RESEND PATCH v2 1/2] pstore: Add mem_type property DT parsing support

[Mukesh Ojha]

Hi All,

can you please review this ?

Thanks,

Mukesh

On 3/2/2021 1:59 PM, Mukesh Ojha wrote:
> Hi Kees,
>
> i have updated the patch based on your last comments.
> please review.
>
> Thanks,
> Mukesh
>
> On 2/25/2021 9:30 PM, Mukesh Ojha wrote:
>> There could be a sceanario where we define some region
>> in normal memory and use them store to logs which is later
>> retrieved by bootloader during warm reset.
>>
>> In this scenario, we wanted to treat this memory as normal
>> cacheable memory instead of default behaviour which
>> is an overhead. Making it cacheable could improve
>> performance.
>>
>> This commit gives control to change mem_type from Device
>> tree, and also documents the value for normal memory.
>>
>> Signed-off-by: Mukesh Ojha <mojha@codeaurora.org>
>> ---
>> Changes in v2:
>>   - if-else converted to switch case
>>   - updated MODULE_PARM_DESC with new memory type.
>>   - default setting is still intact.
>>
>>   Documentation/admin-guide/ramoops.rst |  4 +++-
>>   fs/pstore/ram.c                       |  3 ++-
>>   fs/pstore/ram_core.c                  | 18 ++++++++++++++++--
>>   3 files changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/admin-guide/ramoops.rst 
>> b/Documentation/admin-guide/ramoops.rst
>> index b0a1ae7..8f107d8 100644
>> --- a/Documentation/admin-guide/ramoops.rst
>> +++ b/Documentation/admin-guide/ramoops.rst
>> @@ -3,7 +3,7 @@ Ramoops oops/panic logger
>>     Sergiu Iordache <sergiu@chromium.org>
>>   -Updated: 17 November 2011
>> +Updated: 10 Feb 2021
>>     Introduction
>>   ------------
>> @@ -30,6 +30,8 @@ mapping to pgprot_writecombine. Setting 
>> ``mem_type=1`` attempts to use
>>   depends on atomic operations. At least on ARM, pgprot_noncached 
>> causes the
>>   memory to be mapped strongly ordered, and atomic operations on 
>> strongly ordered
>>   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.
>>     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
>> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
>> index ca6d8a8..af4ca6a4 100644
>> --- a/fs/pstore/ram.c
>> +++ b/fs/pstore/ram.c
>> @@ -56,7 +56,7 @@ MODULE_PARM_DESC(mem_size,
>>   static unsigned int mem_type;
>>   module_param(mem_type, uint, 0400);
>>   MODULE_PARM_DESC(mem_type,
>> -        "set to 1 to try to use unbuffered memory (default 0)");
>> +        "set to 1 to use unbuffered memory, 2 for cached memory 
>> (default 0)");
>>     static int ramoops_max_reason = -1;
>>   module_param_named(max_reason, ramoops_max_reason, int, 0400);
>> @@ -666,6 +666,7 @@ static int ramoops_parse_dt(struct 
>> platform_device *pdev,
>>           field = value;                        \
>>       }
>>   +    parse_u32("mem-type", pdata->record_size, pdata->mem_type);
>>       parse_u32("record-size", pdata->record_size, 0);
>>       parse_u32("console-size", pdata->console_size, 0);
>>       parse_u32("ftrace-size", pdata->ftrace_size, 0);
>> diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
>> index aa8e0b6..0da012f 100644
>> --- a/fs/pstore/ram_core.c
>> +++ b/fs/pstore/ram_core.c
>> @@ -396,6 +396,10 @@ void persistent_ram_zap(struct 
>> persistent_ram_zone *prz)
>>       persistent_ram_update_header_ecc(prz);
>>   }
>>   +#define MEM_TYPE_WCOMBINE    0
>> +#define MEM_TYPE_NONCACHED    1
>> +#define MEM_TYPE_NORMAL        2
>> +
>>   static void *persistent_ram_vmap(phys_addr_t start, size_t size,
>>           unsigned int memtype)
>>   {
>> @@ -409,10 +413,20 @@ static void *persistent_ram_vmap(phys_addr_t 
>> start, size_t size,
>>       page_start = start - offset_in_page(start);
>>       page_count = DIV_ROUND_UP(size + offset_in_page(start), 
>> PAGE_SIZE);
>>   -    if (memtype)
>> +    switch (memtype) {
>> +    case MEM_TYPE_NORMAL:
>> +        prot = PAGE_KERNEL;
>> +        break;
>> +    case MEM_TYPE_NONCACHED:
>>           prot = pgprot_noncached(PAGE_KERNEL);
>> -    else
>> +        break;
>> +    case MEM_TYPE_WCOMBINE:
>>           prot = pgprot_writecombine(PAGE_KERNEL);
>> +        break;
>> +    default:
>> +        pr_err("invalid memory type\n");
>> +        return NULL;
>> +    }
>>         pages = kmalloc_array(page_count, sizeof(struct page *), 
>> GFP_KERNEL);
>>       if (!pages) {

Re: [RESEND PATCH v2 2/2] pstore: Add buffer start check during init

[Kees Cook]

On Thu, Feb 25, 2021 at 09:30:17PM +0530, Mukesh Ojha wrote:
> From: Huang Yiwei <hyiwei@codeaurora.org>
> 
> In a scenario of panic, when we use DRAM to store log instead
> of persistant storage and during warm reset when we copy these
> data outside of ram. Missing check on prz->start(write position)
> can cause crash because it can be any value and can point outside
> the mapped region. So add the start check to avoid.
> 
> Signed-off-by: Huang Yiwei <hyiwei@codeaurora.org>
> Signed-off-by: Mukesh Ojha <mojha@codeaurora.org>
> ---
> change in v2:
>  - this is on top of first patchset.
> 
>  fs/pstore/ram_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
> index 0da012f..a15748a 100644
> --- a/fs/pstore/ram_core.c
> +++ b/fs/pstore/ram_core.c
> @@ -514,7 +514,7 @@ static int persistent_ram_post_init(struct persistent_ram_zone *prz, u32 sig,
>  	sig ^= PERSISTENT_RAM_SIG;
>  
>  	if (prz->buffer->sig == sig) {
> -		if (buffer_size(prz) == 0) {
> +		if (buffer_size(prz) == 0 && buffer_start(prz) == 0) {
>  			pr_debug("found existing empty buffer\n");
>  			return 0;

Were you seeing cases where the sig was correct, size was zero and start
was non-zero but still smaller than buffer_size(prz)?

That should only happen if a prz changed in size (but not location)
across boots, and I guess, yes, should be detected and zapped. But that
means you need a case for size=0 buffer_start!=0 for zapping.

But you talk about a crash without this test? "any value" isn't true,
the next tests make sure it's within buffer_size:

                if (buffer_size(prz) > prz->buffer_size ||
                    buffer_start(prz) > buffer_size(prz)) {
                        pr_info("found existing invalid buffer, size %zu, start %zu\n",
                                buffer_size(prz), buffer_start(prz));
                        zap = true;

can you tell me more about what you're seeing?

-Kees

-- 
Kees Cook

Re: [RESEND PATCH v2 1/2] pstore: Add mem_type property DT parsing support

[Kees Cook]

On Thu, Feb 25, 2021 at 09:30:16PM +0530, Mukesh Ojha wrote:
> There could be a sceanario where we define some region
> in normal memory and use them store to logs which is later
> retrieved by bootloader during warm reset.
> 
> In this scenario, we wanted to treat this memory as normal
> cacheable memory instead of default behaviour which
> is an overhead. Making it cacheable could improve
> performance.
> 
> This commit gives control to change mem_type from Device
> tree, and also documents the value for normal memory.
> 
> Signed-off-by: Mukesh Ojha <mojha@codeaurora.org>
> ---
> Changes in v2:
>  - if-else converted to switch case
>  - updated MODULE_PARM_DESC with new memory type.
>  - default setting is still intact.
> 
>  Documentation/admin-guide/ramoops.rst |  4 +++-
>  fs/pstore/ram.c                       |  3 ++-
>  fs/pstore/ram_core.c                  | 18 ++++++++++++++++--
>  3 files changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/admin-guide/ramoops.rst b/Documentation/admin-guide/ramoops.rst
> index b0a1ae7..8f107d8 100644
> --- a/Documentation/admin-guide/ramoops.rst
> +++ b/Documentation/admin-guide/ramoops.rst
> @@ -3,7 +3,7 @@ Ramoops oops/panic logger
>  
>  Sergiu Iordache <sergiu@chromium.org>
>  
> -Updated: 17 November 2011
> +Updated: 10 Feb 2021
>  
>  Introduction
>  ------------
> @@ -30,6 +30,8 @@ mapping to pgprot_writecombine. Setting ``mem_type=1`` attempts to use
>  depends on atomic operations. At least on ARM, pgprot_noncached causes the
>  memory to be mapped strongly ordered, and atomic operations on strongly ordered
>  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.
>  
>  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
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index ca6d8a8..af4ca6a4 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -56,7 +56,7 @@ MODULE_PARM_DESC(mem_size,
>  static unsigned int mem_type;
>  module_param(mem_type, uint, 0400);
>  MODULE_PARM_DESC(mem_type,
> -		"set to 1 to try to use unbuffered memory (default 0)");
> +		"set to 1 to use unbuffered memory, 2 for cached memory (default 0)");

I'd like to be as explicit as possible (0 wasn't listed), so about this:

"memory type: 0=write-combined (default), 1=unbuffered, 2=cached"

>  static int ramoops_max_reason = -1;
>  module_param_named(max_reason, ramoops_max_reason, int, 0400);
> @@ -666,6 +666,7 @@ static int ramoops_parse_dt(struct platform_device *pdev,
>  		field = value;						\
>  	}
>  
> +	parse_u32("mem-type", pdata->record_size, pdata->mem_type);

Please update the documentation at:
Documentation/devicetree/bindings/reserved-memory/ramoops.txt
(and please move and update the language about "unbuffered" being
deprecated like "no-dump-oops", so that it's clear what happens when
both are specified -- "mem-type" overrides "unbuffered".)

>  	parse_u32("record-size", pdata->record_size, 0);
>  	parse_u32("console-size", pdata->console_size, 0);
>  	parse_u32("ftrace-size", pdata->ftrace_size, 0);
> diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
> index aa8e0b6..0da012f 100644
> --- a/fs/pstore/ram_core.c
> +++ b/fs/pstore/ram_core.c
> @@ -396,6 +396,10 @@ void persistent_ram_zap(struct persistent_ram_zone *prz)
>  	persistent_ram_update_header_ecc(prz);
>  }
>  
> +#define MEM_TYPE_WCOMBINE	0
> +#define MEM_TYPE_NONCACHED	1
> +#define MEM_TYPE_NORMAL		2
> +
>  static void *persistent_ram_vmap(phys_addr_t start, size_t size,
>  		unsigned int memtype)
>  {
> @@ -409,10 +413,20 @@ static void *persistent_ram_vmap(phys_addr_t start, size_t size,
>  	page_start = start - offset_in_page(start);
>  	page_count = DIV_ROUND_UP(size + offset_in_page(start), PAGE_SIZE);
>  
> -	if (memtype)
> +	switch (memtype) {
> +	case MEM_TYPE_NORMAL:
> +		prot = PAGE_KERNEL;
> +		break;
> +	case MEM_TYPE_NONCACHED:
>  		prot = pgprot_noncached(PAGE_KERNEL);
> -	else
> +		break;
> +	case MEM_TYPE_WCOMBINE:
>  		prot = pgprot_writecombine(PAGE_KERNEL);
> +		break;
> +	default:
> +		pr_err("invalid memory type\n");

This should be more verbose:

		pr_err("invalid mem_type=%d\n", memtype);

> +		return NULL;
> +	}
>  
>  	pages = kmalloc_array(page_count, sizeof(struct page *), GFP_KERNEL);
>  	if (!pages) {

With those changes, it looks good to me. Thanks!

-- 
Kees Cook