* [PATCH v4 2/2] pstore/ram: Rework logic for detecting ramoops
2023-01-27 16:00 [PATCH v4 1/2] dt-bindings: ramoops: Inherit reserve memory property Mukesh Ojha
@ 2023-01-27 16:00 ` Mukesh Ojha
2023-01-27 19:18 ` [PATCH v4 1/2] dt-bindings: ramoops: Inherit reserve memory property Kees Cook
` (3 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Mukesh Ojha @ 2023-01-27 16:00 UTC (permalink / raw)
To: linux-hardening, linux-doc, linux-kernel, devicetree
Cc: Mukesh Ojha, Kees Cook, Tony Luck, Guilherme G. Piccoli, Jonathan Corbet
The reserved memory region for ramoops is assumed to be at a fixed
and known location when read from the devicetree. This is not desirable
in an environment where it is preferred the region to be dynamically
allocated at runtime, as opposed to being fixed at compile time.
Also, some of the platforms might be still expecting dedicated
memory region for ramoops node where the region is known beforehand
and platform_get_resource() is used in that case.
So, add logic to detect the start and size of the ramoops memory
region by looking up reserved memory region with of_reserved_mem_lookup()
api when platform_get_resource() fails also update the ramoops
documentation,
Cc: Kees Cook <keescook@chromium.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: "Guilherme G. Piccoli" <gpiccoli@igalia.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
Changes in v4:
- Updated the minor change in documentation.
Changes in v3:
- Merged 2/3 and 3/3 into one.
https://lore.kernel.org/lkml/1673611126-13803-2-git-send-email-quic_mojha@quicinc.com/
https://lore.kernel.org/lkml/1673611126-13803-3-git-send-email-quic_mojha@quicinc.com/
Changes in v2:
- Addressed the comments made by kees and Guilherme in v1.
Documentation/admin-guide/ramoops.rst | 25 ++++++++++++++++++++++---
fs/pstore/ram.c | 18 +++++++++++++-----
2 files changed, 35 insertions(+), 8 deletions(-)
diff --git a/Documentation/admin-guide/ramoops.rst b/Documentation/admin-guide/ramoops.rst
index e9f8514..f701fab 100644
--- a/Documentation/admin-guide/ramoops.rst
+++ b/Documentation/admin-guide/ramoops.rst
@@ -16,8 +16,9 @@ survive after a restart.
Ramoops concepts
----------------
-Ramoops uses a predefined memory area to store the dump. The start and size
-and type of the memory area are set using three variables:
+Ramoops uses both predefined and dynamically memory area to store the dump.
+The start and size and type of the memory area are set using three
+variables:
* ``mem_address`` for the start
* ``mem_size`` for the size. The memory size will be rounded down to a
@@ -70,7 +71,8 @@ Setting the ramoops parameters can be done in several different manners:
B. Use Device Tree bindings, as described in
``Documentation/devicetree/bindings/reserved-memory/ramoops.yaml``.
- For example::
+
+ Example of statically reserved ramoops region::
reserved-memory {
#address-cells = <2>;
@@ -85,6 +87,23 @@ Setting the ramoops parameters can be done in several different manners:
};
};
+ Example of dynamically reserved ramoops region::
+
+ reserved-memory {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+
+ ramoops_region: ramoops {
+ compatible = "ramoops";
+ alloc-ranges = <0x00000000 0xffffffff>;
+ size = <0 0x100000>;
+ record-size = <0x4000>;
+ console-size = <0x4000>;
+ };
+ };
+
+
C. Use a platform device and set the platform data. The parameters can then
be set through that platform data. An example of doing that is:
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index ade66db..17c9f46 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/of_reserved_mem.h>
#include "internal.h"
#include "ram_internal.h"
@@ -643,6 +644,7 @@ static int ramoops_parse_dt(struct platform_device *pdev,
{
struct device_node *of_node = pdev->dev.of_node;
struct device_node *parent_node;
+ struct reserved_mem *rmem;
struct resource *res;
u32 value;
int ret;
@@ -651,13 +653,19 @@ static int ramoops_parse_dt(struct platform_device *pdev,
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!res) {
- dev_err(&pdev->dev,
- "failed to locate DT /reserved-memory resource\n");
- return -EINVAL;
+ rmem = of_reserved_mem_lookup(of_node);
+ if (!rmem) {
+ dev_err(&pdev->dev,
+ "failed to locate DT /reserved-memory resource\n");
+ return -EINVAL;
+ }
+ pdata->mem_size = rmem->size;
+ pdata->mem_address = rmem->base;
+ } else {
+ pdata->mem_size = resource_size(res);
+ pdata->mem_address = res->start;
}
- pdata->mem_size = resource_size(res);
- pdata->mem_address = res->start;
/*
* Setting "unbuffered" is deprecated and will be ignored if
* "mem_type" is also specified.
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v4 1/2] dt-bindings: ramoops: Inherit reserve memory property
2023-01-27 16:00 [PATCH v4 1/2] dt-bindings: ramoops: Inherit reserve memory property Mukesh Ojha
2023-01-27 16:00 ` [PATCH v4 2/2] pstore/ram: Rework logic for detecting ramoops Mukesh Ojha
@ 2023-01-27 19:18 ` Kees Cook
2023-01-27 20:03 ` Krzysztof Kozlowski
` (2 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Kees Cook @ 2023-01-27 19:18 UTC (permalink / raw)
To: Mukesh Ojha
Cc: linux-hardening, linux-doc, linux-kernel, devicetree, Tony Luck,
Guilherme G . Piccoli, Rob Herring, Krzysztof Kozlowski
On Fri, Jan 27, 2023 at 09:30:51PM +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
> is not desirable in an environment where it is preferred the
> region to be dynamically allocated at runtime, as opposed to
> being fixed at compile time.
>
> So, update the ramoops binding by inheriting some reserve memory
> property to allocate the ramoops region dynamically.
>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Guilherme G. Piccoli <gpiccoli@igalia.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
> Changes in v4:
> - Addressed comment made by Krzysztof on ramoops node name.
Thanks! Krzysztof, if this looks good to you, I can carry it in the
pstore tree.
-Kees
>
> Changes in v3:
> - Fixed yaml error and updated commit text as per comment.
>
> Change in v2:
> - Added this patch as per changes going to be done in patch 3/3
>
> .../bindings/reserved-memory/ramoops.yaml | 34 ++++++++++++++++++++--
> 1 file changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml b/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml
> index 0391871..8741626 100644
> --- a/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml
> +++ b/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml
> @@ -10,7 +10,8 @@ description: |
> ramoops provides persistent RAM storage for oops and panics, so they can be
> recovered after a reboot. This is a child-node of "/reserved-memory", and
> is named "ramoops" after the backend, rather than "pstore" which is the
> - subsystem.
> + subsystem. This region can be reserved both statically or dynamically by
> + using appropriate property in device tree.
>
> Parts of this storage may be set aside for other persistent log buffers, such
> as kernel log messages, or for optional ECC error-correction data. The total
> @@ -112,7 +113,13 @@ unevaluatedProperties: false
>
> required:
> - compatible
> - - reg
> +
> +oneOf:
> + - required:
> + - reg
> +
> + - required:
> + - size
>
> anyOf:
> - required: [record-size]
> @@ -142,3 +149,26 @@ examples:
> };
> };
> };
> +
> + - |
> + / {
> + compatible = "foo";
> + model = "foo";
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + reserved-memory {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> +
> + ramoops_region: ramoops {
> + compatible = "ramoops";
> + alloc-ranges = <0x00000000 0xffffffff>;
> + size = <0x0 0x10000>; /* 64kB */
> + console-size = <0x8000>; /* 32kB */
> + record-size = <0x400>; /* 1kB */
> + ecc-size = <16>;
> + };
> + };
> + };
> --
> 2.7.4
>
--
Kees Cook
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v4 1/2] dt-bindings: ramoops: Inherit reserve memory property
2023-01-27 16:00 [PATCH v4 1/2] dt-bindings: ramoops: Inherit reserve memory property Mukesh Ojha
2023-01-27 16:00 ` [PATCH v4 2/2] pstore/ram: Rework logic for detecting ramoops Mukesh Ojha
2023-01-27 19:18 ` [PATCH v4 1/2] dt-bindings: ramoops: Inherit reserve memory property Kees Cook
@ 2023-01-27 20:03 ` Krzysztof Kozlowski
2023-01-30 8:13 ` Mukesh Ojha
2023-01-27 20:04 ` Krzysztof Kozlowski
2023-01-27 20:05 ` Rob Herring
4 siblings, 1 reply; 9+ messages in thread
From: Krzysztof Kozlowski @ 2023-01-27 20:03 UTC (permalink / raw)
To: Mukesh Ojha, linux-hardening, linux-doc, linux-kernel, devicetree
Cc: Kees Cook, Tony Luck, Guilherme G . Piccoli, Rob Herring,
Krzysztof Kozlowski
On 27/01/2023 17:00, 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
> is not desirable in an environment where it is preferred the
> region to be dynamically allocated at runtime, as opposed to
> being fixed at compile time.
>
> So, update the ramoops binding by inheriting some reserve memory
> property to allocate the ramoops region dynamically.
Where is the update which adds "inheriting"?
>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Guilherme G. Piccoli <gpiccoli@igalia.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
> Changes in v4:
> - Addressed comment made by Krzysztof on ramoops node name.
>
> Changes in v3:
> - Fixed yaml error and updated commit text as per comment.
>
> Change in v2:
> - Added this patch as per changes going to be done in patch 3/3
>
> .../bindings/reserved-memory/ramoops.yaml | 34 ++++++++++++++++++++--
> 1 file changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml b/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml
> index 0391871..8741626 100644
> --- a/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml
> +++ b/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml
> @@ -10,7 +10,8 @@ description: |
> ramoops provides persistent RAM storage for oops and panics, so they can be
> recovered after a reboot. This is a child-node of "/reserved-memory", and
> is named "ramoops" after the backend, rather than "pstore" which is the
> - subsystem.
> + subsystem. This region can be reserved both statically or dynamically by
> + using appropriate property in device tree.
>
> Parts of this storage may be set aside for other persistent log buffers, such
> as kernel log messages, or for optional ECC error-correction data. The total
> @@ -112,7 +113,13 @@ unevaluatedProperties: false
>
> required:
> - compatible
> - - reg
This is okay, but:
> +
> +oneOf:
> + - required:
> + - reg
> +
> + - required:
> + - size
I now keep wondering - why do you need this?
>
> anyOf:
> - required: [record-size]
> @@ -142,3 +149,26 @@ examples:
> };
> };
> };
> +
> + - |
> + / {
> + compatible = "foo";
> + model = "foo";
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + reserved-memory {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> +
> + ramoops_region: ramoops {
> + compatible = "ramoops";
> + alloc-ranges = <0x00000000 0xffffffff>;
> + size = <0x0 0x10000>; /* 64kB */
> + console-size = <0x8000>; /* 32kB */
> + record-size = <0x400>; /* 1kB */
> + ecc-size = <16>;
> + };
> + };
> + };
This example does not bring anything new for the ramoops. It's an
example for reserved-memory to show usage with alloc-ranges. There is
nothing useful here in terms of ramoops, so I think it should be dropped.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v4 1/2] dt-bindings: ramoops: Inherit reserve memory property
2023-01-27 20:03 ` Krzysztof Kozlowski
@ 2023-01-30 8:13 ` Mukesh Ojha
0 siblings, 0 replies; 9+ messages in thread
From: Mukesh Ojha @ 2023-01-30 8:13 UTC (permalink / raw)
To: Krzysztof Kozlowski, linux-hardening, linux-doc, linux-kernel,
devicetree
Cc: Kees Cook, Tony Luck, Guilherme G . Piccoli, Rob Herring,
Krzysztof Kozlowski
On 1/28/2023 1:33 AM, Krzysztof Kozlowski wrote:
> On 27/01/2023 17:00, 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
>> is not desirable in an environment where it is preferred the
>> region to be dynamically allocated at runtime, as opposed to
>> being fixed at compile time.
>>
>> So, update the ramoops binding by inheriting some reserve memory
>> property to allocate the ramoops region dynamically.
>
> Where is the update which adds "inheriting"?
By inheriting, i meant using reserve memory properties..
Probably rephrase above as.
"dt-bindings: ramoops: Support dynamic ramoops region allocation
The reserved memory region for ramoops is assumed to be at a
fixed and known location when read from the devicetree. This
is not desirable in an environment where it is preferred the
region to be dynamically allocated at runtime, as opposed to
being fixed at compile time. This can be done with minor update
in ramoops binding as it inherit reserve memory property
(.yaml) in the binding.
Dynamic region could be used by providing size(region size) and
alloc-ranges(allowed ddr region to allocate the size from) instead
of mentioning regs"
Does it sound reasonable ?
-Mukesh
>
>>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Tony Luck <tony.luck@intel.com>
>> Cc: Guilherme G. Piccoli <gpiccoli@igalia.com>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>> ---
>> Changes in v4:
>> - Addressed comment made by Krzysztof on ramoops node name.
>>
>> Changes in v3:
>> - Fixed yaml error and updated commit text as per comment.
>>
>> Change in v2:
>> - Added this patch as per changes going to be done in patch 3/3
>>
>> .../bindings/reserved-memory/ramoops.yaml | 34 ++++++++++++++++++++--
>> 1 file changed, 32 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml b/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml
>> index 0391871..8741626 100644
>> --- a/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml
>> +++ b/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml
>> @@ -10,7 +10,8 @@ description: |
>> ramoops provides persistent RAM storage for oops and panics, so they can be
>> recovered after a reboot. This is a child-node of "/reserved-memory", and
>> is named "ramoops" after the backend, rather than "pstore" which is the
>> - subsystem.
>> + subsystem. This region can be reserved both statically or dynamically by
>> + using appropriate property in device tree.
>>
>> Parts of this storage may be set aside for other persistent log buffers, such
>> as kernel log messages, or for optional ECC error-correction data. The total
>> @@ -112,7 +113,13 @@ unevaluatedProperties: false
>>
>> required:
>> - compatible
>> - - reg
>
> This is okay, but:
>
>> +
>> +oneOf:
>> + - required:
>> + - reg
>> +
>> + - required:
>> + - size
>
> I now keep wondering - why do you need this?
This should be same as..
Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml
>
>>
>> anyOf:
>> - required: [record-size]
>> @@ -142,3 +149,26 @@ examples:
>> };
>> };
>> };
>> +
>> + - |
>> + / {
>> + compatible = "foo";
>> + model = "foo";
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> +
>> + reserved-memory {
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + ranges;
>> +
>> + ramoops_region: ramoops {
>> + compatible = "ramoops";
>> + alloc-ranges = <0x00000000 0xffffffff>;
>> + size = <0x0 0x10000>; /* 64kB */
>> + console-size = <0x8000>; /* 32kB */
>> + record-size = <0x400>; /* 1kB */
>> + ecc-size = <16>;
>> + };
>> + };
>> + };
>
> This example does not bring anything new for the ramoops. It's an
> example for reserved-memory to show usage with alloc-ranges. There is
> nothing useful here in terms of ramoops, so I think it should be dropped.
>
will drop this..
> Best regards,
> Krzysztof
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: ramoops: Inherit reserve memory property
2023-01-27 16:00 [PATCH v4 1/2] dt-bindings: ramoops: Inherit reserve memory property Mukesh Ojha
` (2 preceding siblings ...)
2023-01-27 20:03 ` Krzysztof Kozlowski
@ 2023-01-27 20:04 ` Krzysztof Kozlowski
2023-01-30 5:14 ` Mukesh Ojha
2023-01-27 20:05 ` Rob Herring
4 siblings, 1 reply; 9+ messages in thread
From: Krzysztof Kozlowski @ 2023-01-27 20:04 UTC (permalink / raw)
To: Mukesh Ojha, linux-hardening, linux-doc, linux-kernel, devicetree
Cc: Kees Cook, Tony Luck, Guilherme G . Piccoli, Rob Herring,
Krzysztof Kozlowski
On 27/01/2023 17:00, 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
> is not desirable in an environment where it is preferred the
> region to be dynamically allocated at runtime, as opposed to
> being fixed at compile time.
>
> So, update the ramoops binding by inheriting some reserve memory
> property to allocate the ramoops region dynamically.
>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Guilherme G. Piccoli <gpiccoli@igalia.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
Drop. There is no single need to store automated output of
get_maintainers.pl in the git log forever. It can be easily re-created
at any given time.
If you need it for your own patch management purposes, keep it under ---.
Additionally:
where is patch number 2?
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v4 1/2] dt-bindings: ramoops: Inherit reserve memory property
2023-01-27 20:04 ` Krzysztof Kozlowski
@ 2023-01-30 5:14 ` Mukesh Ojha
0 siblings, 0 replies; 9+ messages in thread
From: Mukesh Ojha @ 2023-01-30 5:14 UTC (permalink / raw)
To: Krzysztof Kozlowski, linux-hardening, linux-doc, linux-kernel,
devicetree
Cc: Kees Cook, Tony Luck, Guilherme G . Piccoli, Rob Herring,
Krzysztof Kozlowski
On 1/28/2023 1:34 AM, Krzysztof Kozlowski wrote:
> On 27/01/2023 17:00, 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
>> is not desirable in an environment where it is preferred the
>> region to be dynamically allocated at runtime, as opposed to
>> being fixed at compile time.
>>
>> So, update the ramoops binding by inheriting some reserve memory
>> property to allocate the ramoops region dynamically.
>>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Tony Luck <tony.luck@intel.com>
>> Cc: Guilherme G. Piccoli <gpiccoli@igalia.com>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
>
> Drop. There is no single need to store automated output of
> get_maintainers.pl in the git log forever. It can be easily re-created
> at any given time.
>
> If you need it for your own patch management purposes, keep it under ---.
>
Will drop, thanks.
> Additionally:
> where is patch number 2?
It was merged with 2/2 as it was not making sense to keep it as separate
patch
-Mukesh
>
> Best regards,
> Krzysztof
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: ramoops: Inherit reserve memory property
2023-01-27 16:00 [PATCH v4 1/2] dt-bindings: ramoops: Inherit reserve memory property Mukesh Ojha
` (3 preceding siblings ...)
2023-01-27 20:04 ` Krzysztof Kozlowski
@ 2023-01-27 20:05 ` Rob Herring
2023-01-30 8:32 ` Mukesh Ojha
4 siblings, 1 reply; 9+ messages in thread
From: Rob Herring @ 2023-01-27 20:05 UTC (permalink / raw)
To: Mukesh Ojha
Cc: linux-hardening, linux-doc, linux-kernel, devicetree, Kees Cook,
Tony Luck, Guilherme G . Piccoli, Krzysztof Kozlowski
On Fri, Jan 27, 2023 at 10:01 AM Mukesh Ojha <quic_mojha@quicinc.com> wrote:
>
> The reserved memory region for ramoops is assumed to be at a
> fixed and known location when read from the devicetree. This
> is not desirable in an environment where it is preferred the
> region to be dynamically allocated at runtime, as opposed to
> being fixed at compile time.
I don't see how this can work. How do you find the region after
rebooting? Or this is only used for the current boot? If so, provide a
userspace interface to configure it (or the existing module params).
The addition of 'size' type regions was primarily for large carveouts
that needed to be allocated before anything else. ramoops is not that.
It's 10s or 100s of KB at most.
> So, update the ramoops binding by inheriting some reserve memory
> property to allocate the ramoops region dynamically.
>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Guilherme G. Piccoli <gpiccoli@igalia.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
> Changes in v4:
> - Addressed comment made by Krzysztof on ramoops node name.
>
> Changes in v3:
> - Fixed yaml error and updated commit text as per comment.
>
> Change in v2:
> - Added this patch as per changes going to be done in patch 3/3
>
> .../bindings/reserved-memory/ramoops.yaml | 34 ++++++++++++++++++++--
> 1 file changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml b/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml
> index 0391871..8741626 100644
> --- a/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml
> +++ b/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml
> @@ -10,7 +10,8 @@ description: |
> ramoops provides persistent RAM storage for oops and panics, so they can be
> recovered after a reboot. This is a child-node of "/reserved-memory", and
> is named "ramoops" after the backend, rather than "pstore" which is the
> - subsystem.
> + subsystem. This region can be reserved both statically or dynamically by
> + using appropriate property in device tree.
>
> Parts of this storage may be set aside for other persistent log buffers, such
> as kernel log messages, or for optional ECC error-correction data. The total
> @@ -112,7 +113,13 @@ unevaluatedProperties: false
>
> required:
> - compatible
> - - reg
> +
> +oneOf:
> + - required:
> + - reg
> +
> + - required:
> + - size
>
> anyOf:
> - required: [record-size]
> @@ -142,3 +149,26 @@ examples:
> };
> };
> };
> +
> + - |
> + / {
You can't have multiple root node examples. Check the example dts, the
examples will be merged together.
> + compatible = "foo";
> + model = "foo";
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + reserved-memory {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> +
> + ramoops_region: ramoops {
> + compatible = "ramoops";
> + alloc-ranges = <0x00000000 0xffffffff>;
> + size = <0x0 0x10000>; /* 64kB */
> + console-size = <0x8000>; /* 32kB */
> + record-size = <0x400>; /* 1kB */
> + ecc-size = <16>;
> + };
> + };
> + };
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v4 1/2] dt-bindings: ramoops: Inherit reserve memory property
2023-01-27 20:05 ` Rob Herring
@ 2023-01-30 8:32 ` Mukesh Ojha
0 siblings, 0 replies; 9+ messages in thread
From: Mukesh Ojha @ 2023-01-30 8:32 UTC (permalink / raw)
To: Rob Herring
Cc: linux-hardening, linux-doc, linux-kernel, devicetree, Kees Cook,
Tony Luck, Guilherme G . Piccoli, Krzysztof Kozlowski
On 1/28/2023 1:35 AM, Rob Herring wrote:
> On Fri, Jan 27, 2023 at 10:01 AM Mukesh Ojha <quic_mojha@quicinc.com> wrote:
>>
>> The reserved memory region for ramoops is assumed to be at a
>> fixed and known location when read from the devicetree. This
>> is not desirable in an environment where it is preferred the
>> region to be dynamically allocated at runtime, as opposed to
>> being fixed at compile time.
>
> I don't see how this can work. How do you find the region after
> rebooting? Or this is only used for the current boot? If so, provide a
> userspace interface to configure it (or the existing module params).
>
Client can do of_reserved_mem_lookup() on this ramoops node to get
region addresses and it is for regular boots and it is not for
current boot.
Basically, here we are trying to support system which does not
completely support warm reset(content could be inconsistent across
boots) and has their own mechanism to dump ramoops region on crash.
Also, If it is required, to override the existing ramoops region of
current boot(from static to dynamic), would provide user space interface
as well.
> The addition of 'size' type regions was primarily for large carveouts
> that needed to be allocated before anything else. ramoops is not that.
> It's 10s or 100s of KB at most.
I agree..
Is this the only issue ?
I thought if it is inheriting reserve-memory binding, it can be used if
it is not breaking anything what ramoops already supports..
you can have a look at 2/2 patch.
-Mukesh
>
>
>> So, update the ramoops binding by inheriting some reserve memory
>> property to allocate the ramoops region dynamically.
>>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Tony Luck <tony.luck@intel.com>
>> Cc: Guilherme G. Piccoli <gpiccoli@igalia.com>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>> ---
>> Changes in v4:
>> - Addressed comment made by Krzysztof on ramoops node name.
>>
>> Changes in v3:
>> - Fixed yaml error and updated commit text as per comment.
>>
>> Change in v2:
>> - Added this patch as per changes going to be done in patch 3/3
>>
>> .../bindings/reserved-memory/ramoops.yaml | 34 ++++++++++++++++++++--
>> 1 file changed, 32 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml b/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml
>> index 0391871..8741626 100644
>> --- a/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml
>> +++ b/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml
>> @@ -10,7 +10,8 @@ description: |
>> ramoops provides persistent RAM storage for oops and panics, so they can be
>> recovered after a reboot. This is a child-node of "/reserved-memory", and
>> is named "ramoops" after the backend, rather than "pstore" which is the
>> - subsystem.
>> + subsystem. This region can be reserved both statically or dynamically by
>> + using appropriate property in device tree.
>>
>> Parts of this storage may be set aside for other persistent log buffers, such
>> as kernel log messages, or for optional ECC error-correction data. The total
>> @@ -112,7 +113,13 @@ unevaluatedProperties: false
>>
>> required:
>> - compatible
>> - - reg
>> +
>> +oneOf:
>> + - required:
>> + - reg
>> +
>> + - required:
>> + - size
>>
>> anyOf:
>> - required: [record-size]
>> @@ -142,3 +149,26 @@ examples:
>> };
>> };
>> };
>> +
>> + - |
>> + / {
>
> You can't have multiple root node examples. Check the example dts, the
> examples will be merged together.
Sure, will remove this..
>
>> + compatible = "foo";
>> + model = "foo";
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> +
>> + reserved-memory {
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + ranges;
>> +
>> + ramoops_region: ramoops {
>> + compatible = "ramoops";
>> + alloc-ranges = <0x00000000 0xffffffff>;
>> + size = <0x0 0x10000>; /* 64kB */
>> + console-size = <0x8000>; /* 32kB */
>> + record-size = <0x400>; /* 1kB */
>> + ecc-size = <16>;
>> + };
>> + };
>> + };
>> --
>> 2.7.4
>>
^ permalink raw reply [flat|nested] 9+ messages in thread