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: Huang Yiwei <[email protected]>
Signed-off-by: Mukesh Ojha <[email protected]>
---
Documentation/admin-guide/ramoops.rst | 4 +++-
fs/pstore/ram.c | 1 +
fs/pstore/ram_core.c | 10 ++++++++--
3 files changed, 12 insertions(+), 3 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 <[email protected]>
-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..b262c57 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -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..83cd612 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,9 +413,11 @@ 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)
+ if (memtype == MEM_TYPE_NORMAL)
+ prot = PAGE_KERNEL;
+ else if (memtype == MEM_TYPE_NONCACHED)
prot = pgprot_noncached(PAGE_KERNEL);
- else
+ else if (memtype == MEM_TYPE_WCOMBINE)
prot = pgprot_writecombine(PAGE_KERNEL);
pages = kmalloc_array(page_count, sizeof(struct page *), GFP_KERNEL);
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Hi--
On 2/10/21 6:52 AM, Mukesh Ojha wrote:
> There could be a sceanario where we define some region
scenario
> 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: Huang Yiwei <[email protected]>
> Signed-off-by: Mukesh Ojha <[email protected]>
> ---
> Documentation/admin-guide/ramoops.rst | 4 +++-
> fs/pstore/ram.c | 1 +
> fs/pstore/ram_core.c | 10 ++++++++--
> 3 files changed, 12 insertions(+), 3 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 <[email protected]>
>
> -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,
Does "mem_type=" work? or does it need to be "mem-type=", as below?
I.e., do both of them work?
> +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..b262c57 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -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);
here^^^^^^^^^^
thanks.
--
~Randy
On 2/10/21 11:40 AM, Randy Dunlap wrote:
> Hi--
>
> On 2/10/21 6:52 AM, Mukesh Ojha wrote:
>> There could be a sceanario where we define some region
>
> scenario
>
>> 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: Huang Yiwei <[email protected]>
>> Signed-off-by: Mukesh Ojha <[email protected]>
>> ---
>> Documentation/admin-guide/ramoops.rst | 4 +++-
>> fs/pstore/ram.c | 1 +
>> fs/pstore/ram_core.c | 10 ++++++++--
>> 3 files changed, 12 insertions(+), 3 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 <[email protected]>
>>
>> -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,
>
> Does "mem_type=" work? or does it need to be "mem-type=", as below?
> I.e., do both of them work?
>
Ah yes, as documented:
"Hyphens (dashes) and underscores are equivalent in parameter names,"
thanks.
>> +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..b262c57 100644
>> --- a/fs/pstore/ram.c
>> +++ b/fs/pstore/ram.c
>> @@ -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);
>
> here^^^^^^^^^^
--
~Randy
On Wed, Feb 10, 2021 at 08:22:21PM +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.
Cool; yeah. I like this idea.
>
> This commit gives control to change mem_type from Device
> tree, and also documents the value for normal memory.
What's the safest default setting?
>
> Signed-off-by: Huang Yiwei <[email protected]>
> Signed-off-by: Mukesh Ojha <[email protected]>
> ---
> Documentation/admin-guide/ramoops.rst | 4 +++-
> fs/pstore/ram.c | 1 +
> fs/pstore/ram_core.c | 10 ++++++++--
> 3 files changed, 12 insertions(+), 3 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 <[email protected]>
>
> -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..b262c57 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -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);
This was handled by "unbuffered" above. Can you find a way to resolve
potential conflicts between these?
> 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..83cd612 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
It might be nice for this to have a human-readable name too, but let's
start with numeric. :)
Please update the mem_type MODULE_PARM_DESC to detail the new values too.
> +
> static void *persistent_ram_vmap(phys_addr_t start, size_t size,
> unsigned int memtype)
> {
> @@ -409,9 +413,11 @@ 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)
> + if (memtype == MEM_TYPE_NORMAL)
> + prot = PAGE_KERNEL;
> + else if (memtype == MEM_TYPE_NONCACHED)
> prot = pgprot_noncached(PAGE_KERNEL);
> - else
> + else if (memtype == MEM_TYPE_WCOMBINE)
> prot = pgprot_writecombine(PAGE_KERNEL);
Let's make this a switch statement.
>
> pages = kmalloc_array(page_count, sizeof(struct page *), GFP_KERNEL);
> --
> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
> Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
>
--
Kees Cook
On 2/11/2021 1:47 AM, Kees Cook wrote:
> On Wed, Feb 10, 2021 at 08:22:21PM +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.
> Cool; yeah. I like this idea.
Thanks.
>
>> This commit gives control to change mem_type from Device
>> tree, and also documents the value for normal memory.
> What's the safest default setting?
We could keep it default, like it already exist to have unbuffered or
not to have it in Device tree.
and could use mem-type to cover normal memory(DDR) as cached one and
it will also cover buffered and unbufferd mapping
by mentioning it in device tree as numeric value.
>
>> Signed-off-by: Huang Yiwei <[email protected]>
>> Signed-off-by: Mukesh Ojha <[email protected]>
>> ---
>> Documentation/admin-guide/ramoops.rst | 4 +++-
>> fs/pstore/ram.c | 1 +
>> fs/pstore/ram_core.c | 10 ++++++++--
>> 3 files changed, 12 insertions(+), 3 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 <[email protected]>
>>
>> -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..b262c57 100644
>> --- a/fs/pstore/ram.c
>> +++ b/fs/pstore/ram.c
>> @@ -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);
> This was handled by "unbuffered" above. Can you find a way to resolve
> potential conflicts between these?
Currently there is no conflict code-wise, even after this change, as it
overrides if mem-type is there
otherwise everything is intact.
However, if we want to use only mem-type property then i can remove
unbuffered property.
not having unbuffered property i.e default=> mem-type = 0
having unbuffered property => mem-type = 1
mem-type = 2 for cached normal memory
>
>> 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..83cd612 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
> It might be nice for this to have a human-readable name too, but let's
> start with numeric. :)
>
> Please update the mem_type MODULE_PARM_DESC to detail the new values too.
Ok, will do in next patch.
>
>> +
>> static void *persistent_ram_vmap(phys_addr_t start, size_t size,
>> unsigned int memtype)
>> {
>> @@ -409,9 +413,11 @@ 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)
>> + if (memtype == MEM_TYPE_NORMAL)
>> + prot = PAGE_KERNEL;
>> + else if (memtype == MEM_TYPE_NONCACHED)
>> prot = pgprot_noncached(PAGE_KERNEL);
>> - else
>> + else if (memtype == MEM_TYPE_WCOMBINE)
>> prot = pgprot_writecombine(PAGE_KERNEL);
> Let's make this a switch statement.
Ok, will do in next patch.
Thanks
Mukesh
>
>>
>> pages = kmalloc_array(page_count, sizeof(struct page *), GFP_KERNEL);
>> --
>> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
>> Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
>>