From: "Isaac J. Manjarres" <[email protected]>
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 <[email protected]>
Signed-off-by: Mukesh Ojha <[email protected]>
Signed-off-by: Prasad Sodagudi <[email protected]>
Signed-off-by: Isaac J. Manjarres <[email protected]>
---
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
On Wed, Jun 21, 2023 at 5:52 PM 'Isaac J. Manjarres' via kernel-team
<[email protected]> wrote:
>
> From: "Isaac J. Manjarres" <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Mukesh Ojha <[email protected]>
> Signed-off-by: Prasad Sodagudi <[email protected]>
> Signed-off-by: Isaac J. Manjarres <[email protected]>
> ---
> 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
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
> <[email protected]> wrote:
> >
> > From: "Isaac J. Manjarres" <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Mukesh Ojha <[email protected]>
> > Signed-off-by: Prasad Sodagudi <[email protected]>
> > Signed-off-by: Isaac J. Manjarres <[email protected]>
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
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 <[email protected]>
> > > Signed-off-by: Mukesh Ojha <[email protected]>
> > > Signed-off-by: Prasad Sodagudi <[email protected]>
> > > Signed-off-by: Isaac J. Manjarres <[email protected]>
>
> 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
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
On June 22, 2023 10:26:35 AM PDT, Isaac Manjarres <[email protected]> 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
On 6/22/2023 10:58 AM, Kees Cook wrote:
> On June 22, 2023 10:26:35 AM PDT, Isaac Manjarres <[email protected]> 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/[email protected]/
https://lore.kernel.org/all/[email protected]/
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
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
>> <[email protected]> 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/[email protected]/
>
> https://lore.kernel.org/all/[email protected]/
>
> 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/[email protected]/
later tried the approach of patch #9 along with minidump series..
- Mukesh
>
> - Elliot
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
>>> <[email protected]> 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/[email protected]/
>>
>> https://lore.kernel.org/all/[email protected]/
>>
>> 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/[email protected]/
>
> 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
On 22/06/2023 02:52, Isaac J. Manjarres wrote:
> From: "Isaac J. Manjarres" <[email protected]>
>
> 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/[email protected]/
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/[email protected]/
Best regards,
Krzysztof
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/[email protected]/
>
> https://lore.kernel.org/all/[email protected]/
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
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/[email protected]/
> >
> > 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
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/[email protected]/
>>>
>>> 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
>