[PATCH] pstore/ram: Add support for dynamically allocated ramoops memory regions

[PATCH] pstore/ram: Add support for dynamically allocated ramoops memory regions

[Isaac J. Manjarres]

From: "Isaac J. Manjarres" <isaacm@codeaurora.org>

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 environments where it is preferred for the region to be dynamically
allocated early during boot (i.e. the memory region is defined with
the "alloc-ranges" property instead of the "reg" property).

If the location of the ramoops region is not fixed via the "reg"
devicetree property, the call to platform_get_resource() will fail
because resources of type IORESOURCE_MEM must be described with the
"reg" property.

Since ramoops regions are part of the reserved-memory devicetree
node, they exist in the reserved_mem array. This means that the
of_reserved_mem_lookup() function can be used to retrieve the
reserved_mem structure for the ramoops region, and that structure
contains the base and size of the region, even if it has been
dynamically allocated.

Thus invoke of_reserved_mem_lookup() in case the call to
platform_get_resource() fails in order to support dynamically
allocated ramoops memory regions.

Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>
Signed-off-by: Mukesh Ojha <mojha@codeaurora.org>
Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
Signed-off-by: Isaac J. Manjarres <isaacmanjarres@google.com>
---
 fs/pstore/ram.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index ade66dbe5f39..e4bbba187011 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,20 @@ 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) {
+			pdata->mem_size = rmem->size;
+			pdata->mem_address = rmem->base;
+		} else {
+			dev_err(&pdev->dev,
+				"failed to locate DT /reserved-memory resource\n");
+			return -EINVAL;
+		}
+	} 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.41.0.178.g377b9f9a00-goog

Re: [PATCH] pstore/ram: Add support for dynamically allocated ramoops memory regions

[John Stultz]

On Wed, Jun 21, 2023 at 5:52 PM 'Isaac J. Manjarres' via kernel-team
<kernel-team@android.com> wrote:
>
> From: "Isaac J. Manjarres" <isaacm@codeaurora.org>
>
> 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 environments where it is preferred for the region to be dynamically
> allocated early during boot (i.e. the memory region is defined with
> the "alloc-ranges" property instead of the "reg" property).
>

Thanks for sending this out, Isaac!

Apologies, I've forgotten much of the details around dt bindings here,
so forgive my questions:
If the memory is dynamically allocated from a specific range, is it
guaranteed to be consistently the same address boot to boot?

> Since ramoops regions are part of the reserved-memory devicetree
> node, they exist in the reserved_mem array. This means that the
> of_reserved_mem_lookup() function can be used to retrieve the
> reserved_mem structure for the ramoops region, and that structure
> contains the base and size of the region, even if it has been
> dynamically allocated.

I think this is answering my question above, but it's a little opaque,
so I'm not sure.

> Thus invoke of_reserved_mem_lookup() in case the call to
> platform_get_resource() fails in order to support dynamically
> allocated ramoops memory regions.
>
> Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>
> Signed-off-by: Mukesh Ojha <mojha@codeaurora.org>
> Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
> Signed-off-by: Isaac J. Manjarres <isaacmanjarres@google.com>
> ---
>  fs/pstore/ram.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index ade66dbe5f39..e4bbba187011 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,20 @@ 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);

Nit: you could keep rmem scoped locally here.

Otherwise the code looks sane, I just suspect the commit message could
be more clear in explaining the need/utility of the dts entry using
alloc-ranges.

thanks
-john

Re: [PATCH] pstore/ram: Add support for dynamically allocated ramoops memory regions

[Kees Cook]

On Wed, Jun 21, 2023 at 09:47:26PM -0700, John Stultz wrote:
> On Wed, Jun 21, 2023 at 5:52 PM 'Isaac J. Manjarres' via kernel-team
> <kernel-team@android.com> wrote:
> >
> > From: "Isaac J. Manjarres" <isaacm@codeaurora.org>
> >
> > 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 environments where it is preferred for the region to be dynamically
> > allocated early during boot (i.e. the memory region is defined with
> > the "alloc-ranges" property instead of the "reg" property).
> >
> 
> Thanks for sending this out, Isaac!
> 
> Apologies, I've forgotten much of the details around dt bindings here,
> so forgive my questions:
> If the memory is dynamically allocated from a specific range, is it
> guaranteed to be consistently the same address boot to boot?
> 
> > Since ramoops regions are part of the reserved-memory devicetree
> > node, they exist in the reserved_mem array. This means that the
> > of_reserved_mem_lookup() function can be used to retrieve the
> > reserved_mem structure for the ramoops region, and that structure
> > contains the base and size of the region, even if it has been
> > dynamically allocated.
> 
> I think this is answering my question above, but it's a little opaque,
> so I'm not sure.

Yeah, I had exactly the same question: will this be the same
boot-to-boot?

> 
> > Thus invoke of_reserved_mem_lookup() in case the call to
> > platform_get_resource() fails in order to support dynamically
> > allocated ramoops memory regions.
> >
> > Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>
> > Signed-off-by: Mukesh Ojha <mojha@codeaurora.org>
> > Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
> > Signed-off-by: Isaac J. Manjarres <isaacmanjarres@google.com>

I think this should have "Co-developed-by:"s for each person, since this
isn't explicitly a S-o-B chain...

> > ---
> >  fs/pstore/ram.c | 19 ++++++++++++++-----
> >  1 file changed, 14 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> > index ade66dbe5f39..e4bbba187011 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,20 @@ 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);
> 
> Nit: you could keep rmem scoped locally here.
> 
> Otherwise the code looks sane, I just suspect the commit message could
> be more clear in explaining the need/utility of the dts entry using
> alloc-ranges.

I haven't looked closely at the API here, but does this need a "put"
like the "get" stuff? (I assume not, given the "lookup" is on a node...)

-Kees

-- 
Kees Cook

Re: [PATCH] pstore/ram: Add support for dynamically allocated ramoops memory regions

[Isaac Manjarres]

On Wed, Jun 21, 2023 at 10:15:45PM -0700, Kees Cook wrote:
> On Wed, Jun 21, 2023 at 09:47:26PM -0700, John Stultz 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 environments where it is preferred for the region to be dynamically
> > > allocated early during boot (i.e. the memory region is defined with
> > > the "alloc-ranges" property instead of the "reg" property).
> > >
> > 
> > Thanks for sending this out, Isaac!
> > 
> > Apologies, I've forgotten much of the details around dt bindings here,
> > so forgive my questions:
> > If the memory is dynamically allocated from a specific range, is it
> > guaranteed to be consistently the same address boot to boot?
> > 
> > > Since ramoops regions are part of the reserved-memory devicetree
> > > node, they exist in the reserved_mem array. This means that the
> > > of_reserved_mem_lookup() function can be used to retrieve the
> > > reserved_mem structure for the ramoops region, and that structure
> > > contains the base and size of the region, even if it has been
> > > dynamically allocated.
> > 
> > I think this is answering my question above, but it's a little opaque,
> > so I'm not sure.
> 
> Yeah, I had exactly the same question: will this be the same
> boot-to-boot?

Hi Kees,

Thank you for taking a look at this patch and for your review! When the
alloc-ranges property is used to describe a memory region, the memory
region will always be allocated within that range, but it's not
guaranteed to be allocated at the same base address across reboots.

I had proposed re-wording the end of the commit message in my response
to John as follows:

"...and that structure contains the address of the base of the region
that was allocated at boot anywhere within the range specified by the
"alloc-ranges" devicetree property."

Does that clarify things better?

> > 
> > > Thus invoke of_reserved_mem_lookup() in case the call to
> > > platform_get_resource() fails in order to support dynamically
> > > allocated ramoops memory regions.
> > >
> > > Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>
> > > Signed-off-by: Mukesh Ojha <mojha@codeaurora.org>
> > > Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
> > > Signed-off-by: Isaac J. Manjarres <isaacmanjarres@google.com>
> 
> I think this should have "Co-developed-by:"s for each person, since this
> isn't explicitly a S-o-B chain...

Noted. I'll fix this up for v2 of the patch.

> > > @@ -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,20 @@ 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);
> > 
> > Nit: you could keep rmem scoped locally here.
> > 
> > Otherwise the code looks sane, I just suspect the commit message could
> > be more clear in explaining the need/utility of the dts entry using
> > alloc-ranges.
> 
> I haven't looked closely at the API here, but does this need a "put"
> like the "get" stuff? (I assume not, given the "lookup" is on a node...)

No, it doesn't need a put, since of_reserved_mem_lookup() doesn't
acquire a reference to anything.

Thanks,
Isaac

Re: [PATCH] pstore/ram: Add support for dynamically allocated ramoops memory regions

[Kees Cook]

On June 22, 2023 10:26:35 AM PDT, Isaac Manjarres <isaacmanjarres@google.com> wrote:
>On Wed, Jun 21, 2023 at 10:15:45PM -0700, Kees Cook wrote:
>> On Wed, Jun 21, 2023 at 09:47:26PM -0700, John Stultz 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 environments where it is preferred for the region to be dynamically
>> > > allocated early during boot (i.e. the memory region is defined with
>> > > the "alloc-ranges" property instead of the "reg" property).
>> > >
>> > 
>> > Thanks for sending this out, Isaac!
>> > 
>> > Apologies, I've forgotten much of the details around dt bindings here,
>> > so forgive my questions:
>> > If the memory is dynamically allocated from a specific range, is it
>> > guaranteed to be consistently the same address boot to boot?
>> > 
>> > > Since ramoops regions are part of the reserved-memory devicetree
>> > > node, they exist in the reserved_mem array. This means that the
>> > > of_reserved_mem_lookup() function can be used to retrieve the
>> > > reserved_mem structure for the ramoops region, and that structure
>> > > contains the base and size of the region, even if it has been
>> > > dynamically allocated.
>> > 
>> > I think this is answering my question above, but it's a little opaque,
>> > so I'm not sure.
>> 
>> Yeah, I had exactly the same question: will this be the same
>> boot-to-boot?
>
>Hi Kees,
>
>Thank you for taking a look at this patch and for your review! When the
>alloc-ranges property is used to describe a memory region, the memory
>region will always be allocated within that range, but it's not
>guaranteed to be allocated at the same base address across reboots.
>
>I had proposed re-wording the end of the commit message in my response
>to John as follows:
>
>"...and that structure contains the address of the base of the region
>that was allocated at boot anywhere within the range specified by the
>"alloc-ranges" devicetree property."
>
>Does that clarify things better?

I am probably misunderstanding something still, but it it varies from boot to boot, what utility is there for pstore if it changes? I.e. the things written during the last boot would then no longer accessible at the next boot? E.g.:

Boot 1.
Get address Foo.
Crash, write to Foo.
Boot 2.
Get address Bar, different from Foo.
Nothing found at Bar, so nothing populated in pstorefs; crash report from Boot 1 unavailable.

I feel like there is something I don't understand about the Foo/Bar addresses in my example.

-Kees



-- 
Kees Cook

Re: [PATCH] pstore/ram: Add support for dynamically allocated ramoops memory regions

[Elliot Berman]

On 6/22/2023 10:58 AM, Kees Cook wrote:
> On June 22, 2023 10:26:35 AM PDT, Isaac Manjarres <isaacmanjarres@google.com> wrote:
>> On Wed, Jun 21, 2023 at 10:15:45PM -0700, Kees Cook wrote:
>>> On Wed, Jun 21, 2023 at 09:47:26PM -0700, John Stultz 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 environments where it is preferred for the region to be dynamically
>>>>> allocated early during boot (i.e. the memory region is defined with
>>>>> the "alloc-ranges" property instead of the "reg" property).
>>>>>
>>>>
>>>> Thanks for sending this out, Isaac!
>>>>
>>>> Apologies, I've forgotten much of the details around dt bindings here,
>>>> so forgive my questions:
>>>> If the memory is dynamically allocated from a specific range, is it
>>>> guaranteed to be consistently the same address boot to boot?
>>>>
>>>>> Since ramoops regions are part of the reserved-memory devicetree
>>>>> node, they exist in the reserved_mem array. This means that the
>>>>> of_reserved_mem_lookup() function can be used to retrieve the
>>>>> reserved_mem structure for the ramoops region, and that structure
>>>>> contains the base and size of the region, even if it has been
>>>>> dynamically allocated.
>>>>
>>>> I think this is answering my question above, but it's a little opaque,
>>>> so I'm not sure.
>>>
>>> Yeah, I had exactly the same question: will this be the same
>>> boot-to-boot?
>>
>> Hi Kees,
>>
>> Thank you for taking a look at this patch and for your review! When the
>> alloc-ranges property is used to describe a memory region, the memory
>> region will always be allocated within that range, but it's not
>> guaranteed to be allocated at the same base address across reboots.
>>
>> I had proposed re-wording the end of the commit message in my response
>> to John as follows:
>>
>> "...and that structure contains the address of the base of the region
>> that was allocated at boot anywhere within the range specified by the
>> "alloc-ranges" devicetree property."
>>
>> Does that clarify things better?
> 
> I am probably misunderstanding something still, but it it varies from boot to boot, what utility is there for pstore if it changes? I.e. the things written during the last boot would then no longer accessible at the next boot? E.g.:
> 
> Boot 1.
> Get address Foo.
> Crash, write to Foo.
> Boot 2.
> Get address Bar, different from Foo.
> Nothing found at Bar, so nothing populated in pstorefs; crash report from Boot 1 unavailable.
> 
> I feel like there is something I don't understand about the Foo/Bar addresses in my example.
> 

I believe this is being added to support the QCOM SoC minidump feature. 
Mukesh has posted it on the mailing lists here:

https://lore.kernel.org/all/1683133352-10046-1-git-send-email-quic_mojha@quicinc.com/

https://lore.kernel.org/all/1683133352-10046-10-git-send-email-quic_mojha@quicinc.com/

Mukesh, could you comment whether this patch is wanted for us in the 
version you have posted? It looks like maybe not based on the commit 
text in patch #9.

  - Elliot

Re: [PATCH] pstore/ram: Add support for dynamically allocated ramoops memory regions

[Mukesh Ojha]

On 6/23/2023 1:21 AM, Elliot Berman wrote:
> 
> 
> On 6/22/2023 10:58 AM, Kees Cook wrote:
>> On June 22, 2023 10:26:35 AM PDT, Isaac Manjarres 
>> <isaacmanjarres@google.com> wrote:
>>> On Wed, Jun 21, 2023 at 10:15:45PM -0700, Kees Cook wrote:
>>>> On Wed, Jun 21, 2023 at 09:47:26PM -0700, John Stultz 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 environments where it is preferred for the region to be 
>>>>>> dynamically
>>>>>> allocated early during boot (i.e. the memory region is defined with
>>>>>> the "alloc-ranges" property instead of the "reg" property).
>>>>>>
>>>>>
>>>>> Thanks for sending this out, Isaac!
>>>>>
>>>>> Apologies, I've forgotten much of the details around dt bindings here,
>>>>> so forgive my questions:
>>>>> If the memory is dynamically allocated from a specific range, is it
>>>>> guaranteed to be consistently the same address boot to boot?
>>>>>
>>>>>> Since ramoops regions are part of the reserved-memory devicetree
>>>>>> node, they exist in the reserved_mem array. This means that the
>>>>>> of_reserved_mem_lookup() function can be used to retrieve the
>>>>>> reserved_mem structure for the ramoops region, and that structure
>>>>>> contains the base and size of the region, even if it has been
>>>>>> dynamically allocated.
>>>>>
>>>>> I think this is answering my question above, but it's a little opaque,
>>>>> so I'm not sure.
>>>>
>>>> Yeah, I had exactly the same question: will this be the same
>>>> boot-to-boot?
>>>
>>> Hi Kees,
>>>
>>> Thank you for taking a look at this patch and for your review! When the
>>> alloc-ranges property is used to describe a memory region, the memory
>>> region will always be allocated within that range, but it's not
>>> guaranteed to be allocated at the same base address across reboots.
>>>
>>> I had proposed re-wording the end of the commit message in my response
>>> to John as follows:
>>>
>>> "...and that structure contains the address of the base of the region
>>> that was allocated at boot anywhere within the range specified by the
>>> "alloc-ranges" devicetree property."
>>>
>>> Does that clarify things better?
>>
>> I am probably misunderstanding something still, but it it varies from 
>> boot to boot, what utility is there for pstore if it changes? I.e. the 
>> things written during the last boot would then no longer accessible at 
>> the next boot? E.g.:
>>
>> Boot 1.
>> Get address Foo.
>> Crash, write to Foo.
>> Boot 2.
>> Get address Bar, different from Foo.
>> Nothing found at Bar, so nothing populated in pstorefs; crash report 
>> from Boot 1 unavailable.
>>
>> I feel like there is something I don't understand about the Foo/Bar 
>> addresses in my example.
>>
> 
> I believe this is being added to support the QCOM SoC minidump feature. 
> Mukesh has posted it on the mailing lists here:
> 
> https://lore.kernel.org/all/1683133352-10046-1-git-send-email-quic_mojha@quicinc.com/
> 
> https://lore.kernel.org/all/1683133352-10046-10-git-send-email-quic_mojha@quicinc.com/
> 
> Mukesh, could you comment whether this patch is wanted for us in the 
> version you have posted? It looks like maybe not based on the commit 
> text in patch #9.

No, this is no needed after patch #9 .

I have tried multiple attempt already to get this patch in

https://lore.kernel.org/lkml/1675330081-15029-2-git-send-email-quic_mojha@quicinc.com/

later tried the approach of patch #9 along with minidump series..


- Mukesh

> 
>   - Elliot

Re: [PATCH] pstore/ram: Add support for dynamically allocated ramoops memory regions

[Krzysztof Kozlowski]

On 26/06/2023 19:34, Mukesh Ojha wrote:
> 
> 
> On 6/23/2023 1:21 AM, Elliot Berman wrote:
>>
>>
>> On 6/22/2023 10:58 AM, Kees Cook wrote:
>>> On June 22, 2023 10:26:35 AM PDT, Isaac Manjarres 
>>> <isaacmanjarres@google.com> wrote:
>>>> On Wed, Jun 21, 2023 at 10:15:45PM -0700, Kees Cook wrote:
>>>>> On Wed, Jun 21, 2023 at 09:47:26PM -0700, John Stultz 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 environments where it is preferred for the region to be 
>>>>>>> dynamically
>>>>>>> allocated early during boot (i.e. the memory region is defined with
>>>>>>> the "alloc-ranges" property instead of the "reg" property).
>>>>>>>
>>>>>>
>>>>>> Thanks for sending this out, Isaac!
>>>>>>
>>>>>> Apologies, I've forgotten much of the details around dt bindings here,
>>>>>> so forgive my questions:
>>>>>> If the memory is dynamically allocated from a specific range, is it
>>>>>> guaranteed to be consistently the same address boot to boot?
>>>>>>
>>>>>>> Since ramoops regions are part of the reserved-memory devicetree
>>>>>>> node, they exist in the reserved_mem array. This means that the
>>>>>>> of_reserved_mem_lookup() function can be used to retrieve the
>>>>>>> reserved_mem structure for the ramoops region, and that structure
>>>>>>> contains the base and size of the region, even if it has been
>>>>>>> dynamically allocated.
>>>>>>
>>>>>> I think this is answering my question above, but it's a little opaque,
>>>>>> so I'm not sure.
>>>>>
>>>>> Yeah, I had exactly the same question: will this be the same
>>>>> boot-to-boot?
>>>>
>>>> Hi Kees,
>>>>
>>>> Thank you for taking a look at this patch and for your review! When the
>>>> alloc-ranges property is used to describe a memory region, the memory
>>>> region will always be allocated within that range, but it's not
>>>> guaranteed to be allocated at the same base address across reboots.
>>>>
>>>> I had proposed re-wording the end of the commit message in my response
>>>> to John as follows:
>>>>
>>>> "...and that structure contains the address of the base of the region
>>>> that was allocated at boot anywhere within the range specified by the
>>>> "alloc-ranges" devicetree property."
>>>>
>>>> Does that clarify things better?
>>>
>>> I am probably misunderstanding something still, but it it varies from 
>>> boot to boot, what utility is there for pstore if it changes? I.e. the 
>>> things written during the last boot would then no longer accessible at 
>>> the next boot? E.g.:
>>>
>>> Boot 1.
>>> Get address Foo.
>>> Crash, write to Foo.
>>> Boot 2.
>>> Get address Bar, different from Foo.
>>> Nothing found at Bar, so nothing populated in pstorefs; crash report 
>>> from Boot 1 unavailable.
>>>
>>> I feel like there is something I don't understand about the Foo/Bar 
>>> addresses in my example.
>>>
>>
>> I believe this is being added to support the QCOM SoC minidump feature. 
>> Mukesh has posted it on the mailing lists here:
>>
>> https://lore.kernel.org/all/1683133352-10046-1-git-send-email-quic_mojha@quicinc.com/
>>
>> https://lore.kernel.org/all/1683133352-10046-10-git-send-email-quic_mojha@quicinc.com/
>>
>> Mukesh, could you comment whether this patch is wanted for us in the 
>> version you have posted? It looks like maybe not based on the commit 
>> text in patch #9.
> 
> No, this is no needed after patch #9 .
> 
> I have tried multiple attempt already to get this patch in
> 
> https://lore.kernel.org/lkml/1675330081-15029-2-git-send-email-quic_mojha@quicinc.com/
> 
> later tried the approach of patch #9 along with minidump series..

For all dynamic reserved-memory-ramoops thingy, I think Rob made clear
statement:

"I don't think dynamic ramoops location should be defined in DT."

https://lore.kernel.org/lkml/CAL_JsqKV6EEpsDNdj1BTN6nODb=hsHkzsdkCzzWwnTrygn0K-A@mail.gmail.com/

Please do not send three versions of the same patch hoping one will
sneak in.

Best regards,
Krzysztof

Re: [PATCH] pstore/ram: Add support for dynamically allocated ramoops memory regions

[Kees Cook]

On Tue, Jul 04, 2023 at 08:07:09AM +0200, Krzysztof Kozlowski wrote:
> On 26/06/2023 19:34, Mukesh Ojha wrote:
> > I have tried multiple attempt already to get this patch in
> > 
> > https://lore.kernel.org/lkml/1675330081-15029-2-git-send-email-quic_mojha@quicinc.com/
> > 
> > later tried the approach of patch #9 along with minidump series..
> 
> For all dynamic reserved-memory-ramoops thingy, I think Rob made clear
> statement:
> 
> "I don't think dynamic ramoops location should be defined in DT."
> 
> https://lore.kernel.org/lkml/CAL_JsqKV6EEpsDNdj1BTN6nODb=hsHkzsdkCzzWwnTrygn0K-A@mail.gmail.com/
> 
> Please do not send three versions of the same patch hoping one will
> sneak in.

If I understand correctly, the driving issue here is that minidump wants
to be able to find the crash report "externally". Perhaps pstore could
provide either a known symbol that contains the address that was used,
or could add something to the kernel crash image (like is done for
KASLR), so that an external consumer of the kernel crash image could
find it?

And then the RAM backend for pstore could gain an option for "just
allocate regular RAM" for holding the dump? In other words, the address
is chosen by pstore, but an external consumer could still locate it.

-Kees

-- 
Kees Cook

Re: [PATCH] pstore/ram: Add support for dynamically allocated ramoops memory regions

[Mukesh Ojha]

On 7/6/2023 3:51 AM, Kees Cook wrote:
> On Tue, Jul 04, 2023 at 08:07:09AM +0200, Krzysztof Kozlowski wrote:
>> On 26/06/2023 19:34, Mukesh Ojha wrote:
>>> I have tried multiple attempt already to get this patch in
>>>
>>> https://lore.kernel.org/lkml/1675330081-15029-2-git-send-email-quic_mojha@quicinc.com/
>>>
>>> later tried the approach of patch #9 along with minidump series..
>>
>> For all dynamic reserved-memory-ramoops thingy, I think Rob made clear
>> statement:
>>
>> "I don't think dynamic ramoops location should be defined in DT."
>>
>> https://lore.kernel.org/lkml/CAL_JsqKV6EEpsDNdj1BTN6nODb=hsHkzsdkCzzWwnTrygn0K-A@mail.gmail.com/
>>
>> Please do not send three versions of the same patch hoping one will
>> sneak in.
> 
> If I understand correctly, the driving issue here is that minidump wants
> to be able to find the crash report "externally". Perhaps pstore could
> provide either a known symbol that contains the address that was used,
> or could add something to the kernel crash image (like is done for
> KASLR), so that an external consumer of the kernel crash image could
> find it?
> 
> And then the RAM backend for pstore could gain an option for "just
> allocate regular RAM" for holding the dump? In other words, the address
> is chosen by pstore, but an external consumer could still locate it.

Yes, in-short, it wants RAM backend (fs/pstore/ram.c) to use regular ram
instead of fixed ram address range and at the same time it should be
able to query the address used.

So, registering with minidump is what telling upfront what address range
content would we be getting in final crash dump.

-Mukesh

> 
> -Kees
> 

Re: [PATCH] pstore/ram: Add support for dynamically allocated ramoops memory regions

[Krzysztof Kozlowski]

On 22/06/2023 21:51, Elliot Berman wrote:
>> Boot 1.
>> Get address Foo.
>> Crash, write to Foo.
>> Boot 2.
>> Get address Bar, different from Foo.
>> Nothing found at Bar, so nothing populated in pstorefs; crash report from Boot 1 unavailable.
>>
>> I feel like there is something I don't understand about the Foo/Bar addresses in my example.
>>
> 
> I believe this is being added to support the QCOM SoC minidump feature. 
> Mukesh has posted it on the mailing lists here:
> 
> https://lore.kernel.org/all/1683133352-10046-1-git-send-email-quic_mojha@quicinc.com/
> 
> https://lore.kernel.org/all/1683133352-10046-10-git-send-email-quic_mojha@quicinc.com/

And since this is coming bypassing DT maintainers and clearly against
Rob's feedback so far, that's a no.

> 
> Mukesh, could you comment whether this patch is wanted for us in the 
> version you have posted? It looks like maybe not based on the commit 
> text in patch #9.


Best regards,
Krzysztof

Re: [PATCH] pstore/ram: Add support for dynamically allocated ramoops memory regions

[Isaac Manjarres]

On Wed, Jun 21, 2023 at 09:47:26PM -0700, John Stultz 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 environments where it is preferred for the region to be dynamically
> > allocated early during boot (i.e. the memory region is defined with
> > the "alloc-ranges" property instead of the "reg" property).
> >
> 
> Thanks for sending this out, Isaac!
> 
> Apologies, I've forgotten much of the details around dt bindings here,
> so forgive my questions:
> If the memory is dynamically allocated from a specific range, is it
> guaranteed to be consistently the same address boot to boot?

Hi John,

Thanks for reviewing this patch! When you use the "alloc-ranges"
property, you specify a range for the memory region to reside in.
This means that the region will lie somewhere in between the range
you specified, but it is not guaranteed to be in the same location
across reboots.

> > Since ramoops regions are part of the reserved-memory devicetree
> > node, they exist in the reserved_mem array. This means that the
> > of_reserved_mem_lookup() function can be used to retrieve the
> > reserved_mem structure for the ramoops region, and that structure
> > contains the base and size of the region, even if it has been
> > dynamically allocated.
> 
> I think this is answering my question above, but it's a little opaque,
> so I'm not sure.

How about re-wording the end as such?

"...and that structure contains the address of the base of the region
that was allocated at boot anywhere within the range specified by the
"alloc-ranges" devicetree property."


> > @@ -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,20 @@ 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);
> 
> Nit: you could keep rmem scoped locally here.

Noted! Thanks for the feedback.

> Otherwise the code looks sane, I just suspect the commit message could
> be more clear in explaining the need/utility of the dts entry using
> alloc-ranges.

Got it; please let me know if the edit I suggested earlier clarifies
things.

Thanks,
Isaac

Re: [PATCH] pstore/ram: Add support for dynamically allocated ramoops memory regions

[Krzysztof Kozlowski]

On 22/06/2023 02:52, Isaac J. Manjarres wrote:
> From: "Isaac J. Manjarres" <isaacm@codeaurora.org>
> 
> 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 environments where it is preferred for the region to be dynamically
> allocated early during boot (i.e. the memory region is defined with
> the "alloc-ranges" property instead of the "reg" property).
> 
> If the location of the ramoops region is not fixed via the "reg"
> devicetree property, the call to platform_get_resource() will fail
> because resources of type IORESOURCE_MEM must be described with the
> "reg" property.
> 
> Since ramoops regions are part of the reserved-memory devicetree
> node, they exist in the reserved_mem array. This means that the
> of_reserved_mem_lookup() function can be used to retrieve the
> reserved_mem structure for the ramoops region, and that structure
> contains the base and size of the region, even if it has been
> dynamically allocated.
> 
> Thus invoke of_reserved_mem_lookup() in case the call to
> platform_get_resource() fails in order to support dynamically
> allocated ramoops memory regions.

You missed change to Devicetree binding, so the reg is still required
for all DT-based platforms. For such, this patch is just half-baked.

You also did not CC DT maintainers, which would be nice considering some
improper advises for minidump like here:
https://lore.kernel.org/lkml/e25723bf-be85-b458-a84c-1a45392683bb@gmail.com/

DT is not for some dynamically allocated properties or dynamic system
configuration. You don't need DT for that. Therefore if you remove the
reg, I question the entire point of this binding.

See also:
https://lore.kernel.org/lkml/CAL_JsqJ_TTnGjjB2d8_FKHpWBRG5GHLoWnabCKjsdeZ4QFdNEg@mail.gmail.com/

https://lore.kernel.org/lkml/20230202235916.GA2931100-robh@kernel.org/


Best regards,
Krzysztof