diff mbox series

of/fdt: Make sure no-map does not remove already reserved regions

Message ID 20190703050827.173284-1-drinkcat@chromium.org
State Changes Requested, archived
Headers show
Series of/fdt: Make sure no-map does not remove already reserved regions | expand

Checks

Context Check Description
robh/checkpatch success

Commit Message

Nicolas Boichat July 3, 2019, 5:08 a.m. UTC 
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(-)

Comments

Stephen Boyd July 16, 2019, 10:35 p.m. UTC | #1 
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>
Florian Fainelli July 16, 2019, 10:46 p.m. UTC | #2 
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.
Rob Herring July 16, 2019, 11:12 p.m. UTC | #3 
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/
Florian Fainelli July 16, 2019, 11:17 p.m. UTC | #4 
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.
Nicolas Boichat July 22, 2019, 5:53 a.m. UTC | #5 
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
Jan Kiszka March 22, 2021, 7:58 a.m. UTC | #6 
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
Jan Kiszka March 22, 2021, 6:05 p.m. UTC | #7 
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
Jan Kiszka March 22, 2021, 8:15 p.m. UTC | #8 
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
diff mbox series

Patch

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);
 }