[PATCH v2] of: Check for overlap in reserved memory regions

[PATCH v2] of: Check for overlap in reserved memory regions

[Mitchel Humpherys]

Any overlap in the reserved memory regions (those specified in the
reserved-memory DT node) is a bug.  These bugs might go undetected as
long as the contested region isn't used simultaneously by multiple
software agents, which makes such bugs hard to debug.  Fix this by
printing a scary warning during boot if overlap is detected.

Signed-off-by: Mitchel Humpherys <mitchelh@codeaurora.org>
---
v1..v2:
  - Suggestions from Rob Herring (remove superfluous array and
    print statement)
---
 drivers/of/of_reserved_mem.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 726ebe792813..62f467b8ccae 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -1,7 +1,7 @@
 /*
  * Device tree based initialization code for reserved memory.
  *
- * Copyright (c) 2013, The Linux Foundation. All Rights Reserved.
+ * Copyright (c) 2013, 2015 The Linux Foundation. All Rights Reserved.
  * Copyright (c) 2013,2014 Samsung Electronics Co., Ltd.
  *		http://www.samsung.com
  * Author: Marek Szyprowski <m.szyprowski@samsung.com>
@@ -20,6 +20,7 @@
 #include <linux/mm.h>
 #include <linux/sizes.h>
 #include <linux/of_reserved_mem.h>
+#include <linux/sort.h>
 
 #define MAX_RESERVED_REGIONS	16
 static struct reserved_mem reserved_mem[MAX_RESERVED_REGIONS];
@@ -197,12 +198,52 @@ static int __init __reserved_mem_init_node(struct reserved_mem *rmem)
 	return -ENOENT;
 }
 
+static int __init __rmem_cmp(const void *a, const void *b)
+{
+	const struct reserved_mem *ra = a, *rb = b;
+
+	return ra->base - rb->base;
+}
+
+static void __init __rmem_check_for_overlap(void)
+{
+	int i;
+
+	if (reserved_mem_count < 2)
+		return;
+
+	sort(reserved_mem, reserved_mem_count, sizeof(reserved_mem[0]),
+	     __rmem_cmp, NULL);
+	for (i = 0; i < reserved_mem_count - 1; i++) {
+		struct reserved_mem *this, *next;
+
+		this = &reserved_mem[i];
+		next = &reserved_mem[i + 1];
+		if (!(this->base && next->base))
+			continue;
+		if (this->base + this->size > next->base) {
+			phys_addr_t this_end, next_end;
+
+			this_end = this->base + this->size;
+			next_end = next->base + next->size;
+			WARN(1,
+			     "Reserved memory: OVERLAP DETECTED!\n%s (%pa--%pa) overlaps with %s (%pa--%pa)\n",
+			     this->name, &this->base, &this_end,
+			     next->name, &next->base, &next_end);
+		}
+	}
+}
+
 /**
  * fdt_init_reserved_mem - allocate and init all saved reserved memory regions
  */
 void __init fdt_init_reserved_mem(void)
 {
 	int i;
+
+	/* check for overlapping reserved regions */
+	__rmem_check_for_overlap();
+
 	for (i = 0; i < reserved_mem_count; i++) {
 		struct reserved_mem *rmem = &reserved_mem[i];
 		unsigned long node = rmem->fdt_node;
-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2] of: Check for overlap in reserved memory regions

[Mitchel Humpherys]

Any overlap in the reserved memory regions (those specified in the
reserved-memory DT node) is a bug.  These bugs might go undetected as
long as the contested region isn't used simultaneously by multiple
software agents, which makes such bugs hard to debug.  Fix this by
printing a scary warning during boot if overlap is detected.

Signed-off-by: Mitchel Humpherys <mitchelh-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
---
v1..v2:
  - Suggestions from Rob Herring (remove superfluous array and
    print statement)
---
 drivers/of/of_reserved_mem.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 726ebe792813..62f467b8ccae 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -1,7 +1,7 @@
 /*
  * Device tree based initialization code for reserved memory.
  *
- * Copyright (c) 2013, The Linux Foundation. All Rights Reserved.
+ * Copyright (c) 2013, 2015 The Linux Foundation. All Rights Reserved.
  * Copyright (c) 2013,2014 Samsung Electronics Co., Ltd.
  *		http://www.samsung.com
  * Author: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
@@ -20,6 +20,7 @@
 #include <linux/mm.h>
 #include <linux/sizes.h>
 #include <linux/of_reserved_mem.h>
+#include <linux/sort.h>
 
 #define MAX_RESERVED_REGIONS	16
 static struct reserved_mem reserved_mem[MAX_RESERVED_REGIONS];
@@ -197,12 +198,52 @@ static int __init __reserved_mem_init_node(struct reserved_mem *rmem)
 	return -ENOENT;
 }
 
+static int __init __rmem_cmp(const void *a, const void *b)
+{
+	const struct reserved_mem *ra = a, *rb = b;
+
+	return ra->base - rb->base;
+}
+
+static void __init __rmem_check_for_overlap(void)
+{
+	int i;
+
+	if (reserved_mem_count < 2)
+		return;
+
+	sort(reserved_mem, reserved_mem_count, sizeof(reserved_mem[0]),
+	     __rmem_cmp, NULL);
+	for (i = 0; i < reserved_mem_count - 1; i++) {
+		struct reserved_mem *this, *next;
+
+		this = &reserved_mem[i];
+		next = &reserved_mem[i + 1];
+		if (!(this->base && next->base))
+			continue;
+		if (this->base + this->size > next->base) {
+			phys_addr_t this_end, next_end;
+
+			this_end = this->base + this->size;
+			next_end = next->base + next->size;
+			WARN(1,
+			     "Reserved memory: OVERLAP DETECTED!\n%s (%pa--%pa) overlaps with %s (%pa--%pa)\n",
+			     this->name, &this->base, &this_end,
+			     next->name, &next->base, &next_end);
+		}
+	}
+}
+
 /**
  * fdt_init_reserved_mem - allocate and init all saved reserved memory regions
  */
 void __init fdt_init_reserved_mem(void)
 {
 	int i;
+
+	/* check for overlapping reserved regions */
+	__rmem_check_for_overlap();
+
 	for (i = 0; i < reserved_mem_count; i++) {
 		struct reserved_mem *rmem = &reserved_mem[i];
 		unsigned long node = rmem->fdt_node;
-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] of: Check for overlap in reserved memory regions

[Rob Herring]

On Tue, Sep 15, 2015 at 8:30 PM, Mitchel Humpherys
<mitchelh@codeaurora.org> wrote:
> Any overlap in the reserved memory regions (those specified in the
> reserved-memory DT node) is a bug.  These bugs might go undetected as
> long as the contested region isn't used simultaneously by multiple
> software agents, which makes such bugs hard to debug.  Fix this by
> printing a scary warning during boot if overlap is detected.
>
> Signed-off-by: Mitchel Humpherys <mitchelh@codeaurora.org>

Applied this a while ago, but didn't reply here.

Rob

> ---
> v1..v2:
>   - Suggestions from Rob Herring (remove superfluous array and
>     print statement)
> ---
>  drivers/of/of_reserved_mem.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 726ebe792813..62f467b8ccae 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -1,7 +1,7 @@
>  /*
>   * Device tree based initialization code for reserved memory.
>   *
> - * Copyright (c) 2013, The Linux Foundation. All Rights Reserved.
> + * Copyright (c) 2013, 2015 The Linux Foundation. All Rights Reserved.
>   * Copyright (c) 2013,2014 Samsung Electronics Co., Ltd.
>   *             http://www.samsung.com
>   * Author: Marek Szyprowski <m.szyprowski@samsung.com>
> @@ -20,6 +20,7 @@
>  #include <linux/mm.h>
>  #include <linux/sizes.h>
>  #include <linux/of_reserved_mem.h>
> +#include <linux/sort.h>
>
>  #define MAX_RESERVED_REGIONS   16
>  static struct reserved_mem reserved_mem[MAX_RESERVED_REGIONS];
> @@ -197,12 +198,52 @@ static int __init __reserved_mem_init_node(struct reserved_mem *rmem)
>         return -ENOENT;
>  }
>
> +static int __init __rmem_cmp(const void *a, const void *b)
> +{
> +       const struct reserved_mem *ra = a, *rb = b;
> +
> +       return ra->base - rb->base;
> +}
> +
> +static void __init __rmem_check_for_overlap(void)
> +{
> +       int i;
> +
> +       if (reserved_mem_count < 2)
> +               return;
> +
> +       sort(reserved_mem, reserved_mem_count, sizeof(reserved_mem[0]),
> +            __rmem_cmp, NULL);
> +       for (i = 0; i < reserved_mem_count - 1; i++) {
> +               struct reserved_mem *this, *next;
> +
> +               this = &reserved_mem[i];
> +               next = &reserved_mem[i + 1];
> +               if (!(this->base && next->base))
> +                       continue;
> +               if (this->base + this->size > next->base) {
> +                       phys_addr_t this_end, next_end;
> +
> +                       this_end = this->base + this->size;
> +                       next_end = next->base + next->size;
> +                       WARN(1,
> +                            "Reserved memory: OVERLAP DETECTED!\n%s (%pa--%pa) overlaps with %s (%pa--%pa)\n",
> +                            this->name, &this->base, &this_end,
> +                            next->name, &next->base, &next_end);
> +               }
> +       }
> +}
> +
>  /**
>   * fdt_init_reserved_mem - allocate and init all saved reserved memory regions
>   */
>  void __init fdt_init_reserved_mem(void)
>  {
>         int i;
> +
> +       /* check for overlapping reserved regions */
> +       __rmem_check_for_overlap();
> +
>         for (i = 0; i < reserved_mem_count; i++) {
>                 struct reserved_mem *rmem = &reserved_mem[i];
>                 unsigned long node = rmem->fdt_node;
> --
> Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>

Re: [PATCH v2] of: Check for overlap in reserved memory regions

[Michael Ellerman]

On Tue, 2015-09-15 at 18:30 -0700, Mitchel Humpherys wrote:

> Any overlap in the reserved memory regions (those specified in the
> reserved-memory DT node) is a bug.  These bugs might go undetected as
> long as the contested region isn't used simultaneously by multiple
> software agents, which makes such bugs hard to debug.  Fix this by
> printing a scary warning during boot if overlap is detected.
> 
> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 726ebe792813..62f467b8ccae 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -197,12 +198,52 @@ static int __init __reserved_mem_init_node(struct reserved_mem *rmem)
>  	return -ENOENT;
>  }
>  
> +static int __init __rmem_cmp(const void *a, const void *b)
> +{
> +	const struct reserved_mem *ra = a, *rb = b;
> +
> +	return ra->base - rb->base;
> +}
> +
> +static void __init __rmem_check_for_overlap(void)
> +{
> +	int i;
> +
> +	if (reserved_mem_count < 2)
> +		return;
> +
> +	sort(reserved_mem, reserved_mem_count, sizeof(reserved_mem[0]),
> +	     __rmem_cmp, NULL);
> +	for (i = 0; i < reserved_mem_count - 1; i++) {
> +		struct reserved_mem *this, *next;
> +
> +		this = &reserved_mem[i];
> +		next = &reserved_mem[i + 1];
> +		if (!(this->base && next->base))
> +			continue;
> +		if (this->base + this->size > next->base) {
> +			phys_addr_t this_end, next_end;
> +
> +			this_end = this->base + this->size;
> +			next_end = next->base + next->size;
> +			WARN(1,
> +			     "Reserved memory: OVERLAP DETECTED!\n%s (%pa--%pa) overlaps with %s (%pa--%pa)\n",
> +			     this->name, &this->base, &this_end,
> +			     next->name, &next->base, &next_end);

This is blowing up on some powerpc machines.

It's too early in boot to call WARN() on these systems.

Can we turn it into a pr_err() for now?

I'll send a patch?

cheers

Re: [PATCH v2] of: Check for overlap in reserved memory regions

[Michael Ellerman]

On Tue, 2015-09-15 at 18:30 -0700, Mitchel Humpherys wrote:

> Any overlap in the reserved memory regions (those specified in the
> reserved-memory DT node) is a bug.  These bugs might go undetected as
> long as the contested region isn't used simultaneously by multiple
> software agents, which makes such bugs hard to debug.  Fix this by
> printing a scary warning during boot if overlap is detected.
> 
> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 726ebe792813..62f467b8ccae 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -197,12 +198,52 @@ static int __init __reserved_mem_init_node(struct reserved_mem *rmem)
>  	return -ENOENT;
>  }
>  
> +static int __init __rmem_cmp(const void *a, const void *b)
> +{
> +	const struct reserved_mem *ra = a, *rb = b;
> +
> +	return ra->base - rb->base;
> +}
> +
> +static void __init __rmem_check_for_overlap(void)
> +{
> +	int i;
> +
> +	if (reserved_mem_count < 2)
> +		return;
> +
> +	sort(reserved_mem, reserved_mem_count, sizeof(reserved_mem[0]),
> +	     __rmem_cmp, NULL);
> +	for (i = 0; i < reserved_mem_count - 1; i++) {
> +		struct reserved_mem *this, *next;
> +
> +		this = &reserved_mem[i];
> +		next = &reserved_mem[i + 1];
> +		if (!(this->base && next->base))
> +			continue;
> +		if (this->base + this->size > next->base) {
> +			phys_addr_t this_end, next_end;
> +
> +			this_end = this->base + this->size;
> +			next_end = next->base + next->size;
> +			WARN(1,
> +			     "Reserved memory: OVERLAP DETECTED!\n%s (%pa--%pa) overlaps with %s (%pa--%pa)\n",
> +			     this->name, &this->base, &this_end,
> +			     next->name, &next->base, &next_end);

This is blowing up on some powerpc machines.

It's too early in boot to call WARN() on these systems.

Can we turn it into a pr_err() for now?

I'll send a patch?

cheers

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] of: Check for overlap in reserved memory regions

[Rob Herring]

On Mon, Nov 9, 2015 at 10:29 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> On Tue, 2015-09-15 at 18:30 -0700, Mitchel Humpherys wrote:
>
>> Any overlap in the reserved memory regions (those specified in the
>> reserved-memory DT node) is a bug.  These bugs might go undetected as
>> long as the contested region isn't used simultaneously by multiple
>> software agents, which makes such bugs hard to debug.  Fix this by
>> printing a scary warning during boot if overlap is detected.
>>
>> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
>> index 726ebe792813..62f467b8ccae 100644
>> --- a/drivers/of/of_reserved_mem.c
>> +++ b/drivers/of/of_reserved_mem.c
>> @@ -197,12 +198,52 @@ static int __init __reserved_mem_init_node(struct reserved_mem *rmem)
>>       return -ENOENT;
>>  }
>>
>> +static int __init __rmem_cmp(const void *a, const void *b)
>> +{
>> +     const struct reserved_mem *ra = a, *rb = b;
>> +
>> +     return ra->base - rb->base;
>> +}
>> +
>> +static void __init __rmem_check_for_overlap(void)
>> +{
>> +     int i;
>> +
>> +     if (reserved_mem_count < 2)
>> +             return;
>> +
>> +     sort(reserved_mem, reserved_mem_count, sizeof(reserved_mem[0]),
>> +          __rmem_cmp, NULL);
>> +     for (i = 0; i < reserved_mem_count - 1; i++) {
>> +             struct reserved_mem *this, *next;
>> +
>> +             this = &reserved_mem[i];
>> +             next = &reserved_mem[i + 1];
>> +             if (!(this->base && next->base))
>> +                     continue;
>> +             if (this->base + this->size > next->base) {
>> +                     phys_addr_t this_end, next_end;
>> +
>> +                     this_end = this->base + this->size;
>> +                     next_end = next->base + next->size;
>> +                     WARN(1,
>> +                          "Reserved memory: OVERLAP DETECTED!\n%s (%pa--%pa) overlaps with %s (%pa--%pa)\n",
>> +                          this->name, &this->base, &this_end,
>> +                          next->name, &next->base, &next_end);
>
> This is blowing up on some powerpc machines.
>
> It's too early in boot to call WARN() on these systems.

I didn't realize WARN could not be used early. Good to know.

> Can we turn it into a pr_err() for now?

Sounds fine.

> I'll send a patch?

Great.

Rob

Re: [PATCH v2] of: Check for overlap in reserved memory regions

[Rob Herring]

On Mon, Nov 9, 2015 at 10:29 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> On Tue, 2015-09-15 at 18:30 -0700, Mitchel Humpherys wrote:
>
>> Any overlap in the reserved memory regions (those specified in the
>> reserved-memory DT node) is a bug.  These bugs might go undetected as
>> long as the contested region isn't used simultaneously by multiple
>> software agents, which makes such bugs hard to debug.  Fix this by
>> printing a scary warning during boot if overlap is detected.
>>
>> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
>> index 726ebe792813..62f467b8ccae 100644
>> --- a/drivers/of/of_reserved_mem.c
>> +++ b/drivers/of/of_reserved_mem.c
>> @@ -197,12 +198,52 @@ static int __init __reserved_mem_init_node(struct reserved_mem *rmem)
>>       return -ENOENT;
>>  }
>>
>> +static int __init __rmem_cmp(const void *a, const void *b)
>> +{
>> +     const struct reserved_mem *ra = a, *rb = b;
>> +
>> +     return ra->base - rb->base;
>> +}
>> +
>> +static void __init __rmem_check_for_overlap(void)
>> +{
>> +     int i;
>> +
>> +     if (reserved_mem_count < 2)
>> +             return;
>> +
>> +     sort(reserved_mem, reserved_mem_count, sizeof(reserved_mem[0]),
>> +          __rmem_cmp, NULL);
>> +     for (i = 0; i < reserved_mem_count - 1; i++) {
>> +             struct reserved_mem *this, *next;
>> +
>> +             this = &reserved_mem[i];
>> +             next = &reserved_mem[i + 1];
>> +             if (!(this->base && next->base))
>> +                     continue;
>> +             if (this->base + this->size > next->base) {
>> +                     phys_addr_t this_end, next_end;
>> +
>> +                     this_end = this->base + this->size;
>> +                     next_end = next->base + next->size;
>> +                     WARN(1,
>> +                          "Reserved memory: OVERLAP DETECTED!\n%s (%pa--%pa) overlaps with %s (%pa--%pa)\n",
>> +                          this->name, &this->base, &this_end,
>> +                          next->name, &next->base, &next_end);
>
> This is blowing up on some powerpc machines.
>
> It's too early in boot to call WARN() on these systems.

I didn't realize WARN could not be used early. Good to know.

> Can we turn it into a pr_err() for now?

Sounds fine.

> I'll send a patch?

Great.

Rob
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2] of: Check for overlap in reserved memory regions

[Michael Ellerman]

On Mon, 2015-11-09 at 22:41 -0600, Rob Herring wrote:
> On Mon, Nov 9, 2015 at 10:29 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> > On Tue, 2015-09-15 at 18:30 -0700, Mitchel Humpherys wrote:
> > > Any overlap in the reserved memory regions (those specified in the
> > > reserved-memory DT node) is a bug.  These bugs might go undetected as
> > > long as the contested region isn't used simultaneously by multiple
> > > software agents, which makes such bugs hard to debug.  Fix this by
> > > printing a scary warning during boot if overlap is detected.
> > > 
> > > diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> > > index 726ebe792813..62f467b8ccae 100644
> > > --- a/drivers/of/of_reserved_mem.c
> > > +++ b/drivers/of/of_reserved_mem.c
> > > @@ -197,12 +198,52 @@ static int __init __reserved_mem_init_node(struct reserved_mem *rmem)
> > >       return -ENOENT;
> > >  }
> > > 
> > > +static int __init __rmem_cmp(const void *a, const void *b)
> > > +{
> > > +     const struct reserved_mem *ra = a, *rb = b;
> > > +
> > > +     return ra->base - rb->base;
> > > +}
> > > +
> > > +static void __init __rmem_check_for_overlap(void)
> > > +{
> > > +     int i;
> > > +
> > > +     if (reserved_mem_count < 2)
> > > +             return;
> > > +
> > > +     sort(reserved_mem, reserved_mem_count, sizeof(reserved_mem[0]),
> > > +          __rmem_cmp, NULL);
> > > +     for (i = 0; i < reserved_mem_count - 1; i++) {
> > > +             struct reserved_mem *this, *next;
> > > +
> > > +             this = &reserved_mem[i];
> > > +             next = &reserved_mem[i + 1];
> > > +             if (!(this->base && next->base))
> > > +                     continue;
> > > +             if (this->base + this->size > next->base) {
> > > +                     phys_addr_t this_end, next_end;
> > > +
> > > +                     this_end = this->base + this->size;
> > > +                     next_end = next->base + next->size;
> > > +                     WARN(1,
> > > +                          "Reserved memory: OVERLAP DETECTED!\n%s (%pa--%pa) overlaps with %s (%pa--%pa)\n",
> > > +                          this->name, &this->base, &this_end,
> > > +                          next->name, &next->base, &next_end);
> > 
> > This is blowing up on some powerpc machines.
> > 
> > It's too early in boot to call WARN() on these systems.
> 
> I didn't realize WARN could not be used early. Good to know.

Yeah, it's a bit horrible.

It used to be even worse, we'd take a recursive trap and you'd get nothing
useful at all. Ben fixed that, which makes BUG work but not WARN, because WARN
requires you to take the trap _and return_. We should be able to fix it in the
medium term, but not immediately.

> > Can we turn it into a pr_err() for now?
> 
> Sounds fine.

> > I'll send a patch?
> 
> Great.

Thanks. I'll just test the final version and then send.

cheers

Re: [PATCH v2] of: Check for overlap in reserved memory regions

[Michael Ellerman]

On Mon, 2015-11-09 at 22:41 -0600, Rob Herring wrote:
> On Mon, Nov 9, 2015 at 10:29 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> > On Tue, 2015-09-15 at 18:30 -0700, Mitchel Humpherys wrote:
> > > Any overlap in the reserved memory regions (those specified in the
> > > reserved-memory DT node) is a bug.  These bugs might go undetected as
> > > long as the contested region isn't used simultaneously by multiple
> > > software agents, which makes such bugs hard to debug.  Fix this by
> > > printing a scary warning during boot if overlap is detected.
> > > 
> > > diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> > > index 726ebe792813..62f467b8ccae 100644
> > > --- a/drivers/of/of_reserved_mem.c
> > > +++ b/drivers/of/of_reserved_mem.c
> > > @@ -197,12 +198,52 @@ static int __init __reserved_mem_init_node(struct reserved_mem *rmem)
> > >       return -ENOENT;
> > >  }
> > > 
> > > +static int __init __rmem_cmp(const void *a, const void *b)
> > > +{
> > > +     const struct reserved_mem *ra = a, *rb = b;
> > > +
> > > +     return ra->base - rb->base;
> > > +}
> > > +
> > > +static void __init __rmem_check_for_overlap(void)
> > > +{
> > > +     int i;
> > > +
> > > +     if (reserved_mem_count < 2)
> > > +             return;
> > > +
> > > +     sort(reserved_mem, reserved_mem_count, sizeof(reserved_mem[0]),
> > > +          __rmem_cmp, NULL);
> > > +     for (i = 0; i < reserved_mem_count - 1; i++) {
> > > +             struct reserved_mem *this, *next;
> > > +
> > > +             this = &reserved_mem[i];
> > > +             next = &reserved_mem[i + 1];
> > > +             if (!(this->base && next->base))
> > > +                     continue;
> > > +             if (this->base + this->size > next->base) {
> > > +                     phys_addr_t this_end, next_end;
> > > +
> > > +                     this_end = this->base + this->size;
> > > +                     next_end = next->base + next->size;
> > > +                     WARN(1,
> > > +                          "Reserved memory: OVERLAP DETECTED!\n%s (%pa--%pa) overlaps with %s (%pa--%pa)\n",
> > > +                          this->name, &this->base, &this_end,
> > > +                          next->name, &next->base, &next_end);
> > 
> > This is blowing up on some powerpc machines.
> > 
> > It's too early in boot to call WARN() on these systems.
> 
> I didn't realize WARN could not be used early. Good to know.

Yeah, it's a bit horrible.

It used to be even worse, we'd take a recursive trap and you'd get nothing
useful at all. Ben fixed that, which makes BUG work but not WARN, because WARN
requires you to take the trap _and return_. We should be able to fix it in the
medium term, but not immediately.

> > Can we turn it into a pr_err() for now?
> 
> Sounds fine.

> > I'll send a patch?
> 
> Great.

Thanks. I'll just test the final version and then send.

cheers

Re: [PATCH v2] of: Check for overlap in reserved memory regions

[Michael Ellerman]

On Tue, 2015-09-15 at 18:30 -0700, Mitchel Humpherys wrote:

> Any overlap in the reserved memory regions (those specified in the
> reserved-memory DT node) is a bug.

Can you expand a bit on why you think it's a bug? I assume it was discussed at
some point on the list but I didn't see it sorry.

There's nothing I can see in the binding document[1] about whether regions can
overlap, or what it would mean if they did.

If we want to declare that overlapping regions are always a bug then there
should be some text in the binding explaining that. There's also the
possibility that we have existing device trees in the wild that contain
overlapping regions, and whether we think it's OK to retrospectively declare
that they're incorrect.

cheers

[1]: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt

Re: [PATCH v2] of: Check for overlap in reserved memory regions

[Michael Ellerman]

On Tue, 2015-09-15 at 18:30 -0700, Mitchel Humpherys wrote:

> Any overlap in the reserved memory regions (those specified in the
> reserved-memory DT node) is a bug.  These bugs might go undetected as
> long as the contested region isn't used simultaneously by multiple
> software agents, which makes such bugs hard to debug.  Fix this by
> printing a scary warning during boot if overlap is detected.
> 
> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 726ebe792813..62f467b8ccae 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -197,12 +198,52 @@ static int __init __reserved_mem_init_node(struct reserved_mem *rmem)
>  	return -ENOENT;
>  }
>  
> +static int __init __rmem_cmp(const void *a, const void *b)
> +{
> +	const struct reserved_mem *ra = a, *rb = b;
> +
> +	return ra->base - rb->base;
> +}

So on further inspection it seems this is really our problem.

The reserved_mem->base values are u64, so when the unsigned result of the
substraction is converted to int we may or may not get the right value. That
means the sort fails and the reserved mem check gives false positives.

eg:

  Reserved memory: OVERLAP DETECTED!
  ibm,slw-image@1ffe600000 (0x0000001ffe600000--0x0000001ffe700000) overlaps with ibm,firmware-allocs-memory@1000000000 (0x0000001000000000--0x0000001000dc0200)

Note there is no overlap:

  0x0000001ffe600000--0x0000001ffe700000
  vs
  0x0000001000000000--0x0000001000dc0200

cheers

Re: [PATCH v2] of: Check for overlap in reserved memory regions

[Michael Ellerman]

On Tue, 2015-09-15 at 18:30 -0700, Mitchel Humpherys wrote:

> Any overlap in the reserved memory regions (those specified in the
> reserved-memory DT node) is a bug.  These bugs might go undetected as
> long as the contested region isn't used simultaneously by multiple
> software agents, which makes such bugs hard to debug.  Fix this by
> printing a scary warning during boot if overlap is detected.
> 
> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 726ebe792813..62f467b8ccae 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -197,12 +198,52 @@ static int __init __reserved_mem_init_node(struct reserved_mem *rmem)
>  	return -ENOENT;
>  }
>  
> +static int __init __rmem_cmp(const void *a, const void *b)
> +{
> +	const struct reserved_mem *ra = a, *rb = b;
> +
> +	return ra->base - rb->base;
> +}

So on further inspection it seems this is really our problem.

The reserved_mem->base values are u64, so when the unsigned result of the
substraction is converted to int we may or may not get the right value. That
means the sort fails and the reserved mem check gives false positives.

eg:

  Reserved memory: OVERLAP DETECTED!
  ibm,slw-image@1ffe600000 (0x0000001ffe600000--0x0000001ffe700000) overlaps with ibm,firmware-allocs-memory@1000000000 (0x0000001000000000--0x0000001000dc0200)

Note there is no overlap:

  0x0000001ffe600000--0x0000001ffe700000
  vs
  0x0000001000000000--0x0000001000dc0200

cheers

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] of: Check for overlap in reserved memory regions

[Mitchel Humpherys]

On Thu, Nov 12 2015 at 01:19:59 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> On Tue, 2015-09-15 at 18:30 -0700, Mitchel Humpherys wrote:
>
>> Any overlap in the reserved memory regions (those specified in the
>> reserved-memory DT node) is a bug.
>
> Can you expand a bit on why you think it's a bug? I assume it was discussed at
> some point on the list but I didn't see it sorry.

The reason I think it's a bug is because the overlapping memory could be
handed out to multiple firmwares, which generally ends in "random"
firmware crashes.  We've found by sad experience that root-causing such
a crash can be quite difficult.

Is there a valid use case for overlapping regions?  I can't think of
one...

> There's nothing I can see in the binding document[1] about whether regions can
> overlap, or what it would mean if they did.

You're right, the bindings document doesn't say anything about
overlapping memory regions.  I can submit something unless someone comes
up with a reason why we should allow overlapping memory regions.

> If we want to declare that overlapping regions are always a bug then there
> should be some text in the binding explaining that. There's also the
> possibility that we have existing device trees in the wild that contain
> overlapping regions, and whether we think it's OK to retrospectively declare
> that they're incorrect.

I did a quick survey of in-tree users of reserved-memory and couldn't
find any overlapping regions.


-Mitch

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project