LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] of/fdt: Make sure no-map does not remove already reserved regions
@ 2019-07-03  5:08 Nicolas Boichat
  2019-07-16 22:35 ` Stephen Boyd
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Nicolas Boichat @ 2019-07-03  5:08 UTC (permalink / raw)
  To: Rob Herring
  Cc: Frank Rowand, devicetree, linux-kernel, Ian Campbell,
	Grant Likely, Stephen Boyd

If the device tree is incorrectly configured, and attempts to
define a "no-map" reserved memory that overlaps with the kernel
data/code, the kernel would crash quickly after boot, with no
obvious clue about the nature of the issue.

For example, this would happen if we have the kernel mapped at
these addresses (from /proc/iomem):
40000000-41ffffff : System RAM
  40080000-40dfffff : Kernel code
  40e00000-411fffff : reserved
  41200000-413e0fff : Kernel data

And we declare a no-map shared-dma-pool region at a fixed address
within that range:
mem_reserved: mem_region {
	compatible = "shared-dma-pool";
	reg = <0 0x40000000 0 0x01A00000>;
	no-map;
};

To fix this, when removing memory regions at early boot (which is
what "no-map" regions do), we need to make sure that the memory
is not already reserved. If we do, __reserved_mem_reserve_reg
will throw an error:
[    0.000000] OF: fdt: Reserved memory: failed to reserve memory
   for node 'mem_region': base 0x0000000040000000, size 26 MiB
and the code that will try to use the region should also fail,
later on.

We do not do anything for non-"no-map" regions, as memblock
explicitly allows reserved regions to overlap, and the commit
that this fixes removed the check for that precise reason.

Fixes: 094cb98179f19b7 ("of/fdt: memblock_reserve /memreserve/ regions in the case of partial overlap")
Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
---
 drivers/of/fdt.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index cd17dc62a71980a..a1ded43fc332d0c 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -1138,8 +1138,16 @@ int __init __weak early_init_dt_mark_hotplug_memory_arch(u64 base, u64 size)
 int __init __weak early_init_dt_reserve_memory_arch(phys_addr_t base,
 					phys_addr_t size, bool nomap)
 {
-	if (nomap)
+	if (nomap) {
+		/*
+		 * If the memory is already reserved (by another region), we
+		 * should not allow it to be removed altogether.
+		 */
+		if (memblock_is_region_reserved(base, size))
+			return -EBUSY;
+
 		return memblock_remove(base, size);
+	}
 	return memblock_reserve(base, size);
 }
 
-- 
2.22.0.410.gd8fdbe21b5-goog


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] of/fdt: Make sure no-map does not remove already reserved regions
  2019-07-03  5:08 [PATCH] of/fdt: Make sure no-map does not remove already reserved regions Nicolas Boichat
@ 2019-07-16 22:35 ` Stephen Boyd
  2019-07-16 22:46 ` Florian Fainelli
  2021-03-22  7:58 ` Jan Kiszka
  2 siblings, 0 replies; 9+ messages in thread
From: Stephen Boyd @ 2019-07-16 22:35 UTC (permalink / raw)
  To: Nicolas Boichat, Rob Herring
  Cc: Frank Rowand, devicetree, linux-kernel, Ian Campbell, Grant Likely

Quoting Nicolas Boichat (2019-07-02 22:08:27)
> If the device tree is incorrectly configured, and attempts to
> define a "no-map" reserved memory that overlaps with the kernel
> data/code, the kernel would crash quickly after boot, with no
> obvious clue about the nature of the issue.
> 
> For example, this would happen if we have the kernel mapped at
> these addresses (from /proc/iomem):
> 40000000-41ffffff : System RAM
>   40080000-40dfffff : Kernel code
>   40e00000-411fffff : reserved
>   41200000-413e0fff : Kernel data
> 
> And we declare a no-map shared-dma-pool region at a fixed address
> within that range:
> mem_reserved: mem_region {
>         compatible = "shared-dma-pool";
>         reg = <0 0x40000000 0 0x01A00000>;
>         no-map;
> };
> 
> To fix this, when removing memory regions at early boot (which is
> what "no-map" regions do), we need to make sure that the memory
> is not already reserved. If we do, __reserved_mem_reserve_reg
> will throw an error:
> [    0.000000] OF: fdt: Reserved memory: failed to reserve memory
>    for node 'mem_region': base 0x0000000040000000, size 26 MiB
> and the code that will try to use the region should also fail,
> later on.
> 
> We do not do anything for non-"no-map" regions, as memblock
> explicitly allows reserved regions to overlap, and the commit
> that this fixes removed the check for that precise reason.
> 
> Fixes: 094cb98179f19b7 ("of/fdt: memblock_reserve /memreserve/ regions in the case of partial overlap")
> Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] of/fdt: Make sure no-map does not remove already reserved regions
  2019-07-03  5:08 [PATCH] of/fdt: Make sure no-map does not remove already reserved regions Nicolas Boichat
  2019-07-16 22:35 ` Stephen Boyd
@ 2019-07-16 22:46 ` Florian Fainelli
  2019-07-16 23:12   ` Rob Herring
  2021-03-22  7:58 ` Jan Kiszka
  2 siblings, 1 reply; 9+ messages in thread
From: Florian Fainelli @ 2019-07-16 22:46 UTC (permalink / raw)
  To: Nicolas Boichat, Rob Herring
  Cc: Frank Rowand, devicetree, linux-kernel, Ian Campbell,
	Grant Likely, Stephen Boyd

On 7/2/19 10:08 PM, Nicolas Boichat wrote:
> If the device tree is incorrectly configured, and attempts to
> define a "no-map" reserved memory that overlaps with the kernel
> data/code, the kernel would crash quickly after boot, with no
> obvious clue about the nature of the issue.
> 
> For example, this would happen if we have the kernel mapped at
> these addresses (from /proc/iomem):
> 40000000-41ffffff : System RAM
>   40080000-40dfffff : Kernel code
>   40e00000-411fffff : reserved
>   41200000-413e0fff : Kernel data
> 
> And we declare a no-map shared-dma-pool region at a fixed address
> within that range:
> mem_reserved: mem_region {
> 	compatible = "shared-dma-pool";
> 	reg = <0 0x40000000 0 0x01A00000>;
> 	no-map;
> };
> 
> To fix this, when removing memory regions at early boot (which is
> what "no-map" regions do), we need to make sure that the memory
> is not already reserved. If we do, __reserved_mem_reserve_reg
> will throw an error:
> [    0.000000] OF: fdt: Reserved memory: failed to reserve memory
>    for node 'mem_region': base 0x0000000040000000, size 26 MiB
> and the code that will try to use the region should also fail,
> later on.
> 
> We do not do anything for non-"no-map" regions, as memblock
> explicitly allows reserved regions to overlap, and the commit
> that this fixes removed the check for that precise reason.
> 
> Fixes: 094cb98179f19b7 ("of/fdt: memblock_reserve /memreserve/ regions in the case of partial overlap")
> Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> ---
>  drivers/of/fdt.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index cd17dc62a71980a..a1ded43fc332d0c 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -1138,8 +1138,16 @@ int __init __weak early_init_dt_mark_hotplug_memory_arch(u64 base, u64 size)
>  int __init __weak early_init_dt_reserve_memory_arch(phys_addr_t base,
>  					phys_addr_t size, bool nomap)
>  {
> -	if (nomap)
> +	if (nomap) {
> +		/*
> +		 * If the memory is already reserved (by another region), we
> +		 * should not allow it to be removed altogether.
> +		 */
> +		if (memblock_is_region_reserved(base, size))
> +			return -EBUSY;
> +
>  		return memblock_remove(base, size);

While you are it, the nomap argument (introduced with
e8d9d1f5485b52ec3c4d7af839e6914438f6c285) predates the introduction of
memblock_is_nomap() (bf3d3cc580f9960883ebf9ea05868f336d9491c2), so
should just remove memblock_remove() and use memblock_mark_nomap()
instead here.
-- 
Florian

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] of/fdt: Make sure no-map does not remove already reserved regions
  2019-07-16 22:46 ` Florian Fainelli
@ 2019-07-16 23:12   ` Rob Herring
  2019-07-16 23:17     ` Florian Fainelli
  0 siblings, 1 reply; 9+ messages in thread
From: Rob Herring @ 2019-07-16 23:12 UTC (permalink / raw)
  To: Florian Fainelli, KarimAllah Ahmed, Nicolas Boichat
  Cc: Frank Rowand, devicetree, linux-kernel, Ian Campbell,
	Grant Likely, Stephen Boyd

On Tue, Jul 16, 2019 at 4:46 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> On 7/2/19 10:08 PM, Nicolas Boichat wrote:
> > If the device tree is incorrectly configured, and attempts to
> > define a "no-map" reserved memory that overlaps with the kernel
> > data/code, the kernel would crash quickly after boot, with no
> > obvious clue about the nature of the issue.
> >
> > For example, this would happen if we have the kernel mapped at
> > these addresses (from /proc/iomem):
> > 40000000-41ffffff : System RAM
> >   40080000-40dfffff : Kernel code
> >   40e00000-411fffff : reserved
> >   41200000-413e0fff : Kernel data
> >
> > And we declare a no-map shared-dma-pool region at a fixed address
> > within that range:
> > mem_reserved: mem_region {
> >       compatible = "shared-dma-pool";
> >       reg = <0 0x40000000 0 0x01A00000>;
> >       no-map;
> > };
> >
> > To fix this, when removing memory regions at early boot (which is
> > what "no-map" regions do), we need to make sure that the memory
> > is not already reserved. If we do, __reserved_mem_reserve_reg
> > will throw an error:
> > [    0.000000] OF: fdt: Reserved memory: failed to reserve memory
> >    for node 'mem_region': base 0x0000000040000000, size 26 MiB
> > and the code that will try to use the region should also fail,
> > later on.
> >
> > We do not do anything for non-"no-map" regions, as memblock
> > explicitly allows reserved regions to overlap, and the commit
> > that this fixes removed the check for that precise reason.
> >
> > Fixes: 094cb98179f19b7 ("of/fdt: memblock_reserve /memreserve/ regions in the case of partial overlap")
> > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> > ---
> >  drivers/of/fdt.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > index cd17dc62a71980a..a1ded43fc332d0c 100644
> > --- a/drivers/of/fdt.c
> > +++ b/drivers/of/fdt.c
> > @@ -1138,8 +1138,16 @@ int __init __weak early_init_dt_mark_hotplug_memory_arch(u64 base, u64 size)
> >  int __init __weak early_init_dt_reserve_memory_arch(phys_addr_t base,
> >                                       phys_addr_t size, bool nomap)
> >  {
> > -     if (nomap)
> > +     if (nomap) {
> > +             /*
> > +              * If the memory is already reserved (by another region), we
> > +              * should not allow it to be removed altogether.
> > +              */
> > +             if (memblock_is_region_reserved(base, size))
> > +                     return -EBUSY;
> > +
> >               return memblock_remove(base, size);
>
> While you are it, the nomap argument (introduced with
> e8d9d1f5485b52ec3c4d7af839e6914438f6c285) predates the introduction of
> memblock_is_nomap() (bf3d3cc580f9960883ebf9ea05868f336d9491c2), so
> should just remove memblock_remove() and use memblock_mark_nomap()
> instead here.

Perhaps like this patch[1]? Though the reasoning is different and the
commit message here is more thorough, so can I get a combined patch.
However, I don't under how handling a misconfigured DT and aligned
with EFI are the same patch. What's considered valid for EFI is not
for DT regions?

Rob

[1] https://patchwork.ozlabs.org/patch/1131232/

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] of/fdt: Make sure no-map does not remove already reserved regions
  2019-07-16 23:12   ` Rob Herring
@ 2019-07-16 23:17     ` Florian Fainelli
  2019-07-22  5:53       ` Nicolas Boichat
  0 siblings, 1 reply; 9+ messages in thread
From: Florian Fainelli @ 2019-07-16 23:17 UTC (permalink / raw)
  To: Rob Herring, KarimAllah Ahmed, Nicolas Boichat
  Cc: Frank Rowand, devicetree, linux-kernel, Ian Campbell,
	Grant Likely, Stephen Boyd

On 7/16/19 4:12 PM, Rob Herring wrote:
> On Tue, Jul 16, 2019 at 4:46 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>> On 7/2/19 10:08 PM, Nicolas Boichat wrote:
>>> If the device tree is incorrectly configured, and attempts to
>>> define a "no-map" reserved memory that overlaps with the kernel
>>> data/code, the kernel would crash quickly after boot, with no
>>> obvious clue about the nature of the issue.
>>>
>>> For example, this would happen if we have the kernel mapped at
>>> these addresses (from /proc/iomem):
>>> 40000000-41ffffff : System RAM
>>>   40080000-40dfffff : Kernel code
>>>   40e00000-411fffff : reserved
>>>   41200000-413e0fff : Kernel data
>>>
>>> And we declare a no-map shared-dma-pool region at a fixed address
>>> within that range:
>>> mem_reserved: mem_region {
>>>       compatible = "shared-dma-pool";
>>>       reg = <0 0x40000000 0 0x01A00000>;
>>>       no-map;
>>> };
>>>
>>> To fix this, when removing memory regions at early boot (which is
>>> what "no-map" regions do), we need to make sure that the memory
>>> is not already reserved. If we do, __reserved_mem_reserve_reg
>>> will throw an error:
>>> [    0.000000] OF: fdt: Reserved memory: failed to reserve memory
>>>    for node 'mem_region': base 0x0000000040000000, size 26 MiB
>>> and the code that will try to use the region should also fail,
>>> later on.
>>>
>>> We do not do anything for non-"no-map" regions, as memblock
>>> explicitly allows reserved regions to overlap, and the commit
>>> that this fixes removed the check for that precise reason.
>>>
>>> Fixes: 094cb98179f19b7 ("of/fdt: memblock_reserve /memreserve/ regions in the case of partial overlap")
>>> Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
>>> ---
>>>  drivers/of/fdt.c | 10 +++++++++-
>>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>>> index cd17dc62a71980a..a1ded43fc332d0c 100644
>>> --- a/drivers/of/fdt.c
>>> +++ b/drivers/of/fdt.c
>>> @@ -1138,8 +1138,16 @@ int __init __weak early_init_dt_mark_hotplug_memory_arch(u64 base, u64 size)
>>>  int __init __weak early_init_dt_reserve_memory_arch(phys_addr_t base,
>>>                                       phys_addr_t size, bool nomap)
>>>  {
>>> -     if (nomap)
>>> +     if (nomap) {
>>> +             /*
>>> +              * If the memory is already reserved (by another region), we
>>> +              * should not allow it to be removed altogether.
>>> +              */
>>> +             if (memblock_is_region_reserved(base, size))
>>> +                     return -EBUSY;
>>> +
>>>               return memblock_remove(base, size);
>>
>> While you are it, the nomap argument (introduced with
>> e8d9d1f5485b52ec3c4d7af839e6914438f6c285) predates the introduction of
>> memblock_is_nomap() (bf3d3cc580f9960883ebf9ea05868f336d9491c2), so
>> should just remove memblock_remove() and use memblock_mark_nomap()
>> instead here.
> 
> Perhaps like this patch[1]? Though the reasoning is different and the
> commit message here is more thorough, so can I get a combined patch.

From a quick reading it does look like memblock_isolate_range(), as
called by memblock_setclr_flag() should be able to detect this region
was already reserved, though I have not tried it.

> However, I don't under how handling a misconfigured DT and aligned
> with EFI are the same patch. What's considered valid for EFI is not
> for DT regions?

That I don't know how to answer.
-- 
Florian

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] of/fdt: Make sure no-map does not remove already reserved regions
  2019-07-16 23:17     ` Florian Fainelli
@ 2019-07-22  5:53       ` Nicolas Boichat
  0 siblings, 0 replies; 9+ messages in thread
From: Nicolas Boichat @ 2019-07-22  5:53 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Rob Herring, KarimAllah Ahmed, Frank Rowand, devicetree,
	linux-kernel, Ian Campbell, Grant Likely, Stephen Boyd

On Wed, Jul 17, 2019 at 7:17 AM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> On 7/16/19 4:12 PM, Rob Herring wrote:
> > On Tue, Jul 16, 2019 at 4:46 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> >>
> >> On 7/2/19 10:08 PM, Nicolas Boichat wrote:
> >>> If the device tree is incorrectly configured, and attempts to
> >>> define a "no-map" reserved memory that overlaps with the kernel
> >>> data/code, the kernel would crash quickly after boot, with no
> >>> obvious clue about the nature of the issue.
> >>>
> >>> For example, this would happen if we have the kernel mapped at
> >>> these addresses (from /proc/iomem):
> >>> 40000000-41ffffff : System RAM
> >>>   40080000-40dfffff : Kernel code
> >>>   40e00000-411fffff : reserved
> >>>   41200000-413e0fff : Kernel data
> >>>
> >>> And we declare a no-map shared-dma-pool region at a fixed address
> >>> within that range:
> >>> mem_reserved: mem_region {
> >>>       compatible = "shared-dma-pool";
> >>>       reg = <0 0x40000000 0 0x01A00000>;
> >>>       no-map;
> >>> };
> >>>
> >>> To fix this, when removing memory regions at early boot (which is
> >>> what "no-map" regions do), we need to make sure that the memory
> >>> is not already reserved. If we do, __reserved_mem_reserve_reg
> >>> will throw an error:
> >>> [    0.000000] OF: fdt: Reserved memory: failed to reserve memory
> >>>    for node 'mem_region': base 0x0000000040000000, size 26 MiB
> >>> and the code that will try to use the region should also fail,
> >>> later on.
> >>>
> >>> We do not do anything for non-"no-map" regions, as memblock
> >>> explicitly allows reserved regions to overlap, and the commit
> >>> that this fixes removed the check for that precise reason.
> >>>
> >>> Fixes: 094cb98179f19b7 ("of/fdt: memblock_reserve /memreserve/ regions in the case of partial overlap")
> >>> Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> >>> ---
> >>>  drivers/of/fdt.c | 10 +++++++++-
> >>>  1 file changed, 9 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> >>> index cd17dc62a71980a..a1ded43fc332d0c 100644
> >>> --- a/drivers/of/fdt.c
> >>> +++ b/drivers/of/fdt.c
> >>> @@ -1138,8 +1138,16 @@ int __init __weak early_init_dt_mark_hotplug_memory_arch(u64 base, u64 size)
> >>>  int __init __weak early_init_dt_reserve_memory_arch(phys_addr_t base,
> >>>                                       phys_addr_t size, bool nomap)
> >>>  {
> >>> -     if (nomap)
> >>> +     if (nomap) {
> >>> +             /*
> >>> +              * If the memory is already reserved (by another region), we
> >>> +              * should not allow it to be removed altogether.
> >>> +              */
> >>> +             if (memblock_is_region_reserved(base, size))
> >>> +                     return -EBUSY;
> >>> +
> >>>               return memblock_remove(base, size);
> >>
> >> While you are it, the nomap argument (introduced with
> >> e8d9d1f5485b52ec3c4d7af839e6914438f6c285) predates the introduction of
> >> memblock_is_nomap() (bf3d3cc580f9960883ebf9ea05868f336d9491c2), so
> >> should just remove memblock_remove() and use memblock_mark_nomap()
> >> instead here.
> >
> > Perhaps like this patch[1]? Though the reasoning is different and the
> > commit message here is more thorough, so can I get a combined patch.
>
> From a quick reading it does look like memblock_isolate_range(), as
> called by memblock_setclr_flag() should be able to detect this region
> was already reserved, though I have not tried it.

I quickly tested it, and just using memblock_mark_nomap does not seem
be be enough (the call does not fail, and the nomap memory is still
allocated).

> > However, I don't under how handling a misconfigured DT and aligned
> > with EFI are the same patch. What's considered valid for EFI is not
> > for DT regions?
>
> That I don't know how to answer.
> --
> Florian

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] of/fdt: Make sure no-map does not remove already reserved regions
  2019-07-03  5:08 [PATCH] of/fdt: Make sure no-map does not remove already reserved regions Nicolas Boichat
  2019-07-16 22:35 ` Stephen Boyd
  2019-07-16 22:46 ` Florian Fainelli
@ 2021-03-22  7:58 ` Jan Kiszka
  2021-03-22 18:05   ` Jan Kiszka
  2 siblings, 1 reply; 9+ messages in thread
From: Jan Kiszka @ 2021-03-22  7:58 UTC (permalink / raw)
  To: Nicolas Boichat, Rob Herring, Andre Przywara
  Cc: Frank Rowand, devicetree, linux-kernel, Ian Campbell,
	Grant Likely, Stephen Boyd

On 03.07.19 07:08, Nicolas Boichat wrote:
> If the device tree is incorrectly configured, and attempts to
> define a "no-map" reserved memory that overlaps with the kernel
> data/code, the kernel would crash quickly after boot, with no
> obvious clue about the nature of the issue.
> 
> For example, this would happen if we have the kernel mapped at
> these addresses (from /proc/iomem):
> 40000000-41ffffff : System RAM
>   40080000-40dfffff : Kernel code
>   40e00000-411fffff : reserved
>   41200000-413e0fff : Kernel data
> 
> And we declare a no-map shared-dma-pool region at a fixed address
> within that range:
> mem_reserved: mem_region {
> 	compatible = "shared-dma-pool";
> 	reg = <0 0x40000000 0 0x01A00000>;
> 	no-map;
> };
> 
> To fix this, when removing memory regions at early boot (which is
> what "no-map" regions do), we need to make sure that the memory
> is not already reserved. If we do, __reserved_mem_reserve_reg
> will throw an error:
> [    0.000000] OF: fdt: Reserved memory: failed to reserve memory
>    for node 'mem_region': base 0x0000000040000000, size 26 MiB
> and the code that will try to use the region should also fail,
> later on.
> 
> We do not do anything for non-"no-map" regions, as memblock
> explicitly allows reserved regions to overlap, and the commit
> that this fixes removed the check for that precise reason.
> 
> Fixes: 094cb98179f19b7 ("of/fdt: memblock_reserve /memreserve/ regions in the case of partial overlap")
> Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> ---
>  drivers/of/fdt.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index cd17dc62a71980a..a1ded43fc332d0c 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -1138,8 +1138,16 @@ int __init __weak early_init_dt_mark_hotplug_memory_arch(u64 base, u64 size)
>  int __init __weak early_init_dt_reserve_memory_arch(phys_addr_t base,
>  					phys_addr_t size, bool nomap)
>  {
> -	if (nomap)
> +	if (nomap) {
> +		/*
> +		 * If the memory is already reserved (by another region), we
> +		 * should not allow it to be removed altogether.
> +		 */
> +		if (memblock_is_region_reserved(base, size))
> +			return -EBUSY;
> +
>  		return memblock_remove(base, size);
> +	}
>  	return memblock_reserve(base, size);
>  }
>  
> 

Likely the wrong patch to blame but hopefully the right audience:

I'm trying to migrate my RPi4 setup to mainline, and this commit breaks 
booting with TF-A (current master) in the loop. Error:

[    0.000000] Booting Linux on physical CPU 0x0000000000 [0x410fd083]                                                                                                                                                                        
[    0.000000] Linux version 5.10.24+ (jan@md1f2u6c) (aarch64-linux-gnu-gcc (GNU Toolchain for the A-profile Architecture 9.2-2019.12 (arm-9.10)) 9.2.1 20191025, GNU ld (GNU Toolchain for the A-profile Architecture 9.2-2019.12 (arm-9.10)1
[    0.000000] Machine model: Raspberry Pi 4 Model B Rev 1.1                                                                                                                                                                                  
[    0.000000] efi: UEFI not found.                                                                                                                                                                                                           
[    0.000000] OF: fdt: Reserved memory: failed to reserve memory for node 'atf@0': base 0x0000000000000000, size 0 MiB                                                                                                                       

And then we hang later on when Linux does start to use that memory and 
seems to trigger an exception.

Is there a bug in the upstream RPi4 DT?

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] of/fdt: Make sure no-map does not remove already reserved regions
  2021-03-22  7:58 ` Jan Kiszka
@ 2021-03-22 18:05   ` Jan Kiszka
  2021-03-22 20:15     ` Jan Kiszka
  0 siblings, 1 reply; 9+ messages in thread
From: Jan Kiszka @ 2021-03-22 18:05 UTC (permalink / raw)
  To: Nicolas Boichat, Rob Herring, Andre Przywara, Phil Elwell
  Cc: Frank Rowand, devicetree, linux-kernel, Ian Campbell,
	Grant Likely, Stephen Boyd

On 22.03.21 08:58, Jan Kiszka wrote:
> On 03.07.19 07:08, Nicolas Boichat wrote:
>> If the device tree is incorrectly configured, and attempts to
>> define a "no-map" reserved memory that overlaps with the kernel
>> data/code, the kernel would crash quickly after boot, with no
>> obvious clue about the nature of the issue.
>>
>> For example, this would happen if we have the kernel mapped at
>> these addresses (from /proc/iomem):
>> 40000000-41ffffff : System RAM
>>   40080000-40dfffff : Kernel code
>>   40e00000-411fffff : reserved
>>   41200000-413e0fff : Kernel data
>>
>> And we declare a no-map shared-dma-pool region at a fixed address
>> within that range:
>> mem_reserved: mem_region {
>> 	compatible = "shared-dma-pool";
>> 	reg = <0 0x40000000 0 0x01A00000>;
>> 	no-map;
>> };
>>
>> To fix this, when removing memory regions at early boot (which is
>> what "no-map" regions do), we need to make sure that the memory
>> is not already reserved. If we do, __reserved_mem_reserve_reg
>> will throw an error:
>> [    0.000000] OF: fdt: Reserved memory: failed to reserve memory
>>    for node 'mem_region': base 0x0000000040000000, size 26 MiB
>> and the code that will try to use the region should also fail,
>> later on.
>>
>> We do not do anything for non-"no-map" regions, as memblock
>> explicitly allows reserved regions to overlap, and the commit
>> that this fixes removed the check for that precise reason.
>>
>> Fixes: 094cb98179f19b7 ("of/fdt: memblock_reserve /memreserve/ regions in the case of partial overlap")
>> Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
>> ---
>>  drivers/of/fdt.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>> index cd17dc62a71980a..a1ded43fc332d0c 100644
>> --- a/drivers/of/fdt.c
>> +++ b/drivers/of/fdt.c
>> @@ -1138,8 +1138,16 @@ int __init __weak early_init_dt_mark_hotplug_memory_arch(u64 base, u64 size)
>>  int __init __weak early_init_dt_reserve_memory_arch(phys_addr_t base,
>>  					phys_addr_t size, bool nomap)
>>  {
>> -	if (nomap)
>> +	if (nomap) {
>> +		/*
>> +		 * If the memory is already reserved (by another region), we
>> +		 * should not allow it to be removed altogether.
>> +		 */
>> +		if (memblock_is_region_reserved(base, size))
>> +			return -EBUSY;
>> +
>>  		return memblock_remove(base, size);
>> +	}
>>  	return memblock_reserve(base, size);
>>  }
>>  
>>
> 
> Likely the wrong patch to blame but hopefully the right audience:
> 
> I'm trying to migrate my RPi4 setup to mainline, and this commit breaks 
> booting with TF-A (current master) in the loop. Error:
> 
> [    0.000000] Booting Linux on physical CPU 0x0000000000 [0x410fd083]                                                                                                                                                                        
> [    0.000000] Linux version 5.10.24+ (jan@md1f2u6c) (aarch64-linux-gnu-gcc (GNU Toolchain for the A-profile Architecture 9.2-2019.12 (arm-9.10)) 9.2.1 20191025, GNU ld (GNU Toolchain for the A-profile Architecture 9.2-2019.12 (arm-9.10)1
> [    0.000000] Machine model: Raspberry Pi 4 Model B Rev 1.1                                                                                                                                                                                  
> [    0.000000] efi: UEFI not found.                                                                                                                                                                                                           
> [    0.000000] OF: fdt: Reserved memory: failed to reserve memory for node 'atf@0': base 0x0000000000000000, size 0 MiB                                                                                                                       
> 
> And then we hang later on when Linux does start to use that memory and 
> seems to trigger an exception.
> 
> Is there a bug in the upstream RPi4 DT?
> 

FWIW, this is triggering the conflict:

(arch/arm/boot/dts/bcm283x.dtsi)

/* firmware-provided startup stubs live here, where the secondary CPUs are
 * spinning.
 */
/memreserve/ 0x00000000 0x00001000;

I strongly suspect this is only needed in case of TF-A-free boot. With 
TF-A we have standard PCSI (my motivation to use TF-A in the first 
place) - and then this is in conflict with the firmware's reservation.

Do we need separate DTs for this use case? Or should TF-A account for 
this?

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] of/fdt: Make sure no-map does not remove already reserved regions
  2021-03-22 18:05   ` Jan Kiszka
@ 2021-03-22 20:15     ` Jan Kiszka
  0 siblings, 0 replies; 9+ messages in thread
From: Jan Kiszka @ 2021-03-22 20:15 UTC (permalink / raw)
  To: Nicolas Boichat, Rob Herring, Andre Przywara, Phil Elwell
  Cc: Frank Rowand, devicetree, linux-kernel, Ian Campbell,
	Grant Likely, Stephen Boyd

On 22.03.21 19:05, Jan Kiszka wrote:
> On 22.03.21 08:58, Jan Kiszka wrote:
>> On 03.07.19 07:08, Nicolas Boichat wrote:
>>> If the device tree is incorrectly configured, and attempts to
>>> define a "no-map" reserved memory that overlaps with the kernel
>>> data/code, the kernel would crash quickly after boot, with no
>>> obvious clue about the nature of the issue.
>>>
>>> For example, this would happen if we have the kernel mapped at
>>> these addresses (from /proc/iomem):
>>> 40000000-41ffffff : System RAM
>>>   40080000-40dfffff : Kernel code
>>>   40e00000-411fffff : reserved
>>>   41200000-413e0fff : Kernel data
>>>
>>> And we declare a no-map shared-dma-pool region at a fixed address
>>> within that range:
>>> mem_reserved: mem_region {
>>> 	compatible = "shared-dma-pool";
>>> 	reg = <0 0x40000000 0 0x01A00000>;
>>> 	no-map;
>>> };
>>>
>>> To fix this, when removing memory regions at early boot (which is
>>> what "no-map" regions do), we need to make sure that the memory
>>> is not already reserved. If we do, __reserved_mem_reserve_reg
>>> will throw an error:
>>> [    0.000000] OF: fdt: Reserved memory: failed to reserve memory
>>>    for node 'mem_region': base 0x0000000040000000, size 26 MiB
>>> and the code that will try to use the region should also fail,
>>> later on.
>>>
>>> We do not do anything for non-"no-map" regions, as memblock
>>> explicitly allows reserved regions to overlap, and the commit
>>> that this fixes removed the check for that precise reason.
>>>
>>> Fixes: 094cb98179f19b7 ("of/fdt: memblock_reserve /memreserve/ regions in the case of partial overlap")
>>> Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
>>> ---
>>>  drivers/of/fdt.c | 10 +++++++++-
>>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>>> index cd17dc62a71980a..a1ded43fc332d0c 100644
>>> --- a/drivers/of/fdt.c
>>> +++ b/drivers/of/fdt.c
>>> @@ -1138,8 +1138,16 @@ int __init __weak early_init_dt_mark_hotplug_memory_arch(u64 base, u64 size)
>>>  int __init __weak early_init_dt_reserve_memory_arch(phys_addr_t base,
>>>  					phys_addr_t size, bool nomap)
>>>  {
>>> -	if (nomap)
>>> +	if (nomap) {
>>> +		/*
>>> +		 * If the memory is already reserved (by another region), we
>>> +		 * should not allow it to be removed altogether.
>>> +		 */
>>> +		if (memblock_is_region_reserved(base, size))
>>> +			return -EBUSY;
>>> +
>>>  		return memblock_remove(base, size);
>>> +	}
>>>  	return memblock_reserve(base, size);
>>>  }
>>>  
>>>
>>
>> Likely the wrong patch to blame but hopefully the right audience:
>>
>> I'm trying to migrate my RPi4 setup to mainline, and this commit breaks 
>> booting with TF-A (current master) in the loop. Error:
>>
>> [    0.000000] Booting Linux on physical CPU 0x0000000000 [0x410fd083]                                                                                                                                                                        
>> [    0.000000] Linux version 5.10.24+ (jan@md1f2u6c) (aarch64-linux-gnu-gcc (GNU Toolchain for the A-profile Architecture 9.2-2019.12 (arm-9.10)) 9.2.1 20191025, GNU ld (GNU Toolchain for the A-profile Architecture 9.2-2019.12 (arm-9.10)1
>> [    0.000000] Machine model: Raspberry Pi 4 Model B Rev 1.1                                                                                                                                                                                  
>> [    0.000000] efi: UEFI not found.                                                                                                                                                                                                           
>> [    0.000000] OF: fdt: Reserved memory: failed to reserve memory for node 'atf@0': base 0x0000000000000000, size 0 MiB                                                                                                                       
>>
>> And then we hang later on when Linux does start to use that memory and 
>> seems to trigger an exception.
>>
>> Is there a bug in the upstream RPi4 DT?
>>
> 
> FWIW, this is triggering the conflict:
> 
> (arch/arm/boot/dts/bcm283x.dtsi)
> 
> /* firmware-provided startup stubs live here, where the secondary CPUs are
>  * spinning.
>  */
> /memreserve/ 0x00000000 0x00001000;
> 
> I strongly suspect this is only needed in case of TF-A-free boot. With 
> TF-A we have standard PCSI (my motivation to use TF-A in the first 
> place) - and then this is in conflict with the firmware's reservation.
> 
> Do we need separate DTs for this use case? Or should TF-A account for 
> this?
> 

Nah, TF-A issue:

https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/9316

With that applied, upstream kernel & DT work fine.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-03-22 20:21 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-03  5:08 [PATCH] of/fdt: Make sure no-map does not remove already reserved regions Nicolas Boichat
2019-07-16 22:35 ` Stephen Boyd
2019-07-16 22:46 ` Florian Fainelli
2019-07-16 23:12   ` Rob Herring
2019-07-16 23:17     ` Florian Fainelli
2019-07-22  5:53       ` Nicolas Boichat
2021-03-22  7:58 ` Jan Kiszka
2021-03-22 18:05   ` Jan Kiszka
2021-03-22 20:15     ` Jan Kiszka

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).