[U-Boot] [PATCH] efi_loader: don't allocate unusable RAM

[U-Boot] [PATCH] efi_loader: don't allocate unusable RAM

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

Some boards define a maximum usable RAM top that's more restrictive than
the ranges defined by U-Boot's memory bank definitions[1]. In this case,
the unusable RAM isn't mapped in the page tables, and so the EFI code must
not attempt to allocate RAM from outside the usable regions. Fix
efi_find_free_memory() to detect when max_addr is unconstrained or out of
range, and substitue a valid value.

[1] For example, when some peripherals can't access RAM above 4GiB, it's
simplest to force U-Boot's ram_top to a smaller value to avoid dealing
with this issue more explicitly. However, the RAM bank definitions still
describe the unusable RAM so that the booted OS has access to it, since
the OS can typically deal with such restrictions in a more complex
manner.

Fixes: aa909462d018 "efi_loader: efi_allocate_pages is too restrictive"
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 lib/efi_loader/efi_memory.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index 967c3f733e4c..5064ff2ccbe8 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -251,6 +251,9 @@ static uint64_t efi_find_free_memory(uint64_t len, uint64_t max_addr)
 {
 	struct list_head *lhandle;
 
+	if ((max_addr == -1ULL) || (max_addr > gd->ram_top))
+		max_addr = gd->ram_top;
+
 	list_for_each(lhandle, &efi_mem) {
 		struct efi_mem_list *lmem = list_entry(lhandle,
 			struct efi_mem_list, link);
-- 
2.18.0

[U-Boot] [PATCH] efi_loader: don't allocate unusable RAM

[Alexander Graf]

> Am 31.07.2018 um 21:44 schrieb Stephen Warren <swarren@wwwdotorg.org>:
> 
> From: Stephen Warren <swarren@nvidia.com>
> 
> Some boards define a maximum usable RAM top that's more restrictive than
> the ranges defined by U-Boot's memory bank definitions[1]. In this case,
> the unusable RAM isn't mapped in the page tables, and so the EFI code must
> not attempt to allocate RAM from outside the usable regions. Fix
> efi_find_free_memory() to detect when max_addr is unconstrained or out of
> range, and substitue a valid value.
> 
> [1] For example, when some peripherals can't access RAM above 4GiB, it's
> simplest to force U-Boot's ram_top to a smaller value to avoid dealing
> with this issue more explicitly. However, the RAM bank definitions still
> describe the unusable RAM so that the booted OS has access to it, since
> the OS can typically deal with such restrictions in a more complex
> manner.

That's what we have the efi bounce buffers for. Can't we just enable those for tegra?

Alex

> 
> Fixes: aa909462d018 "efi_loader: efi_allocate_pages is too restrictive"
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
> lib/efi_loader/efi_memory.c | 3 +++
> 1 file changed, 3 insertions(+)
> 
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index 967c3f733e4c..5064ff2ccbe8 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -251,6 +251,9 @@ static uint64_t efi_find_free_memory(uint64_t len, uint64_t max_addr)
> {
>    struct list_head *lhandle;
> 
> +    if ((max_addr == -1ULL) || (max_addr > gd->ram_top))
> +        max_addr = gd->ram_top;
> +
>    list_for_each(lhandle, &efi_mem) {
>        struct efi_mem_list *lmem = list_entry(lhandle,
>            struct efi_mem_list, link);
> -- 
> 2.18.0
> 

[U-Boot] [PATCH] efi_loader: don't allocate unusable RAM

[Stephen Warren]

On 07/31/2018 02:03 PM, Alexander Graf wrote:
> 
> 
>> Am 31.07.2018 um 21:44 schrieb Stephen Warren <swarren@wwwdotorg.org>:
>>
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> Some boards define a maximum usable RAM top that's more restrictive than
>> the ranges defined by U-Boot's memory bank definitions[1]. In this case,
>> the unusable RAM isn't mapped in the page tables, and so the EFI code must
>> not attempt to allocate RAM from outside the usable regions. Fix
>> efi_find_free_memory() to detect when max_addr is unconstrained or out of
>> range, and substitue a valid value.
>>
>> [1] For example, when some peripherals can't access RAM above 4GiB, it's
>> simplest to force U-Boot's ram_top to a smaller value to avoid dealing
>> with this issue more explicitly. However, the RAM bank definitions still
>> describe the unusable RAM so that the booted OS has access to it, since
>> the OS can typically deal with such restrictions in a more complex
>> manner.
> 
> That's what we have the efi bounce buffers for. Can't we just enable those for tegra?

No, because that wouldn't help all the non-EFI code, or cause the RAM to 
be mapped in the page tables due to the non-EFI code not needing it.

[U-Boot] [PATCH] efi_loader: don't allocate unusable RAM

[Alexander Graf]

> Am 31.07.2018 um 22:04 schrieb Stephen Warren <swarren@wwwdotorg.org>:
> 
> On 07/31/2018 02:03 PM, Alexander Graf wrote:
>>> Am 31.07.2018 um 21:44 schrieb Stephen Warren <swarren@wwwdotorg.org>:
>>> 
>>> From: Stephen Warren <swarren@nvidia.com>
>>> 
>>> Some boards define a maximum usable RAM top that's more restrictive than
>>> the ranges defined by U-Boot's memory bank definitions[1]. In this case,
>>> the unusable RAM isn't mapped in the page tables, and so the EFI code must
>>> not attempt to allocate RAM from outside the usable regions. Fix
>>> efi_find_free_memory() to detect when max_addr is unconstrained or out of
>>> range, and substitue a valid value.
>>> 
>>> [1] For example, when some peripherals can't access RAM above 4GiB, it's
>>> simplest to force U-Boot's ram_top to a smaller value to avoid dealing
>>> with this issue more explicitly. However, the RAM bank definitions still
>>> describe the unusable RAM so that the booted OS has access to it, since
>>> the OS can typically deal with such restrictions in a more complex
>>> manner.
>> That's what we have the efi bounce buffers for. Can't we just enable those for tegra?
> 
> No, because that wouldn't help all the non-EFI code, or cause the RAM to be mapped in the page tables due to the non-EFI code not needing it.

The non-efi code can still rely on ram_top just fine, no? I was more thinking of enabling the bounce buffer config option as alternative to your patch.


Alex

[U-Boot] [PATCH] efi_loader: don't allocate unusable RAM

[Stephen Warren]

On 07/31/2018 02:15 PM, Alexander Graf wrote:
> 
> 
>> Am 31.07.2018 um 22:04 schrieb Stephen Warren <swarren@wwwdotorg.org>:
>>
>> On 07/31/2018 02:03 PM, Alexander Graf wrote:
>>>> Am 31.07.2018 um 21:44 schrieb Stephen Warren <swarren@wwwdotorg.org>:
>>>>
>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>
>>>> Some boards define a maximum usable RAM top that's more restrictive than
>>>> the ranges defined by U-Boot's memory bank definitions[1]. In this case,
>>>> the unusable RAM isn't mapped in the page tables, and so the EFI code must
>>>> not attempt to allocate RAM from outside the usable regions. Fix
>>>> efi_find_free_memory() to detect when max_addr is unconstrained or out of
>>>> range, and substitue a valid value.
>>>>
>>>> [1] For example, when some peripherals can't access RAM above 4GiB, it's
>>>> simplest to force U-Boot's ram_top to a smaller value to avoid dealing
>>>> with this issue more explicitly. However, the RAM bank definitions still
>>>> describe the unusable RAM so that the booted OS has access to it, since
>>>> the OS can typically deal with such restrictions in a more complex
>>>> manner.
>>> That's what we have the efi bounce buffers for. Can't we just enable those for tegra?
>>
>> No, because that wouldn't help all the non-EFI code, or cause the RAM to be mapped in the page tables due to the non-EFI code not needing it.
> 
> The non-efi code can still rely on ram_top just fine, no? I was more thinking of enabling the bounce buffer config option as alternative to your patch.

A lot more besides enabling that option would have to be done. The issue 
here is that EFI is attempting to use RAM that's not in the page tables 
and hence crashes. Enabling bounce buffers alone isn't going to help 
that; we'd also have to rejig the U-Boot MMU code to set up page table 
entries for the entire of the RAM banks rather than just the usable RAM, 
and then we've have to develop some new way of preventing U-Boot from 
mapping RAM that's part of the RAM banks but shouldn't be mapped because 
e.g. it's part of the secure carveout that can't be mapped in the MMU in 
order to ensure that the CPU never pre-fetches that RAM to avoid taking 
security faults on the fetch. The fact that ram_top is lower than the 
HW-defined RAM limit exists for reasons other than just preventing use 
of RAM that some peripheral HW can't access, so bounce buffers will only 
solve one of the causes, not everything.

[U-Boot] [PATCH] efi_loader: don't allocate unusable RAM

[Alexander Graf]

On 31.07.18 22:20, Stephen Warren wrote:
> On 07/31/2018 02:15 PM, Alexander Graf wrote:
>>
>>
>>> Am 31.07.2018 um 22:04 schrieb Stephen Warren <swarren@wwwdotorg.org>:
>>>
>>> On 07/31/2018 02:03 PM, Alexander Graf wrote:
>>>>> Am 31.07.2018 um 21:44 schrieb Stephen Warren <swarren@wwwdotorg.org>:
>>>>>
>>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>>
>>>>> Some boards define a maximum usable RAM top that's more restrictive
>>>>> than
>>>>> the ranges defined by U-Boot's memory bank definitions[1]. In this
>>>>> case,
>>>>> the unusable RAM isn't mapped in the page tables, and so the EFI
>>>>> code must
>>>>> not attempt to allocate RAM from outside the usable regions. Fix
>>>>> efi_find_free_memory() to detect when max_addr is unconstrained or
>>>>> out of
>>>>> range, and substitue a valid value.
>>>>>
>>>>> [1] For example, when some peripherals can't access RAM above 4GiB,
>>>>> it's
>>>>> simplest to force U-Boot's ram_top to a smaller value to avoid dealing
>>>>> with this issue more explicitly. However, the RAM bank definitions
>>>>> still
>>>>> describe the unusable RAM so that the booted OS has access to it,
>>>>> since
>>>>> the OS can typically deal with such restrictions in a more complex
>>>>> manner.
>>>> That's what we have the efi bounce buffers for. Can't we just enable
>>>> those for tegra?
>>>
>>> No, because that wouldn't help all the non-EFI code, or cause the RAM
>>> to be mapped in the page tables due to the non-EFI code not needing it.
>>
>> The non-efi code can still rely on ram_top just fine, no? I was more
>> thinking of enabling the bounce buffer config option as alternative to
>> your patch.
> 
> A lot more besides enabling that option would have to be done. The issue
> here is that EFI is attempting to use RAM that's not in the page tables
> and hence crashes. Enabling bounce buffers alone isn't going to help
> that; we'd also have to rejig the U-Boot MMU code to set up page table
> entries for the entire of the RAM banks rather than just the usable RAM,
> and then we've have to develop some new way of preventing U-Boot from
> mapping RAM that's part of the RAM banks but shouldn't be mapped because
> e.g. it's part of the secure carveout that can't be mapped in the MMU in
> order to ensure that the CPU never pre-fetches that RAM to avoid taking
> security faults on the fetch. The fact that ram_top is lower than the
> HW-defined RAM limit exists for reasons other than just preventing use
> of RAM that some peripheral HW can't access, so bounce buffers will only
> solve one of the causes, not everything.

I see. The main problem I just see is exactly the rationale you
described above: The EFI memory map will be used by the booted OS to
indicate which regions are usable. If they really aren't, then we need
to properly annotate that in the EFI memory map as well, because
otherwise Linux may end up writing to carved out secure regions of RAM.


Alex

[U-Boot] [PATCH] efi_loader: don't allocate unusable RAM

[Heinrich Schuchardt]

On 07/31/2018 09:44 PM, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> Some boards define a maximum usable RAM top that's more restrictive than
> the ranges defined by U-Boot's memory bank definitions[1]. In this case,
> the unusable RAM isn't mapped in the page tables, and so the EFI code must
> not attempt to allocate RAM from outside the usable regions. Fix
> efi_find_free_memory() to detect when max_addr is unconstrained or out of
> range, and substitue a valid value.
> 

In this case the board has to call efi_add_memory_map() to mark the
reserved region (cf. board/raspberrypi/rpi/rpi.c) or have to mark the
memory region as reserved in the device tree (see efi_carve_out_dt_rsv()).

Please, check if the tegra boards with which you had problems do so.

Best regards

Heinrich

> [1] For example, when some peripherals can't access RAM above 4GiB, it's
> simplest to force U-Boot's ram_top to a smaller value to avoid dealing
> with this issue more explicitly. However, the RAM bank definitions still
> describe the unusable RAM so that the booted OS has access to it, since
> the OS can typically deal with such restrictions in a more complex
> manner.
> 
> Fixes: aa909462d018 "efi_loader: efi_allocate_pages is too restrictive"
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  lib/efi_loader/efi_memory.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index 967c3f733e4c..5064ff2ccbe8 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -251,6 +251,9 @@ static uint64_t efi_find_free_memory(uint64_t len, uint64_t max_addr)
>  {
>  	struct list_head *lhandle;
>  
> +	if ((max_addr == -1ULL) || (max_addr > gd->ram_top))
> +		max_addr = gd->ram_top;
> +
>  	list_for_each(lhandle, &efi_mem) {
>  		struct efi_mem_list *lmem = list_entry(lhandle,
>  			struct efi_mem_list, link);
> 

[U-Boot] [PATCH] efi_loader: don't allocate unusable RAM

[Peter Robinson]

>>> On 07/31/2018 02:03 PM, Alexander Graf wrote:
>>>>>
>>>>> Am 31.07.2018 um 21:44 schrieb Stephen Warren <swarren@wwwdotorg.org>:
>>>>>
>>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>>
>>>>> Some boards define a maximum usable RAM top that's more restrictive
>>>>> than
>>>>> the ranges defined by U-Boot's memory bank definitions[1]. In this
>>>>> case,
>>>>> the unusable RAM isn't mapped in the page tables, and so the EFI code
>>>>> must
>>>>> not attempt to allocate RAM from outside the usable regions. Fix
>>>>> efi_find_free_memory() to detect when max_addr is unconstrained or out
>>>>> of
>>>>> range, and substitue a valid value.
>>>>>
>>>>> [1] For example, when some peripherals can't access RAM above 4GiB,
>>>>> it's
>>>>> simplest to force U-Boot's ram_top to a smaller value to avoid dealing
>>>>> with this issue more explicitly. However, the RAM bank definitions
>>>>> still
>>>>> describe the unusable RAM so that the booted OS has access to it, since
>>>>> the OS can typically deal with such restrictions in a more complex
>>>>> manner.
>>>>
>>>> That's what we have the efi bounce buffers for. Can't we just enable
>>>> those for tegra?
>>>
>>>
>>> No, because that wouldn't help all the non-EFI code, or cause the RAM to
>>> be mapped in the page tables due to the non-EFI code not needing it.
>>
>>
>> The non-efi code can still rely on ram_top just fine, no? I was more
>> thinking of enabling the bounce buffer config option as alternative to your
>> patch.
>
>
> A lot more besides enabling that option would have to be done. The issue
> here is that EFI is attempting to use RAM that's not in the page tables and
> hence crashes. Enabling bounce buffers alone isn't going to help that; we'd
> also have to rejig the U-Boot MMU code to set up page table entries for the
> entire of the RAM banks rather than just the usable RAM, and then we've have
> to develop some new way of preventing U-Boot from mapping RAM that's part of
> the RAM banks but shouldn't be mapped because e.g. it's part of the secure
> carveout that can't be mapped in the MMU in order to ensure that the CPU
> never pre-fetches that RAM to avoid taking security faults on the fetch. The
> fact that ram_top is lower than the HW-defined RAM limit exists for reasons
> other than just preventing use of RAM that some peripheral HW can't access,
> so bounce buffers will only solve one of the causes, not everything.

For reference on the TX1 this patch doesn't fix the problem I've seen
on TX series that I previously sent the patch enabling the bounce
buffer option on the TX2, if I enable bounce buffers it works as I
need with or without this patch. I didn't test TX2 but can if it's
useful (I had to power up the TX1 for other reasons this morning).

Peter

[U-Boot] [PATCH] efi_loader: don't allocate unusable RAM

[Stephen Warren]

On 08/01/2018 12:07 AM, Heinrich Schuchardt wrote:
> On 07/31/2018 09:44 PM, Stephen Warren wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> Some boards define a maximum usable RAM top that's more restrictive than
>> the ranges defined by U-Boot's memory bank definitions[1]. In this case,
>> the unusable RAM isn't mapped in the page tables, and so the EFI code must
>> not attempt to allocate RAM from outside the usable regions. Fix
>> efi_find_free_memory() to detect when max_addr is unconstrained or out of
>> range, and substitue a valid value.
>>
> 
> In this case the board has to call efi_add_memory_map() to mark the
> reserved region (cf. board/raspberrypi/rpi/rpi.c) or have to mark the
> memory region as reserved in the device tree (see efi_carve_out_dt_rsv()).
> 
> Please, check if the tegra boards with which you had problems do so.

I don't think this makes sense; memory should be managed the same way 
within U-Boot no matter which part of U-Boot is consuming that memory. 
Memory regions are currently represented by the content of the memory 
bank definitions and gd->ram_top. Having different parts of the code 
define legal RAM usage in different ways is horribly inconsistent.

At this point, I think the best thing is to revert aa909462d018 
"efi_loader: efi_allocate_pages is too restrictive" since it causes a 
regression on Jetson TX1, and we can work out the correct way to fix all 
this once the system is working again.

[U-Boot] [PATCH] efi_loader: don't allocate unusable RAM

[Heinrich Schuchardt]

On 08/01/2018 06:17 PM, Stephen Warren wrote:
> On 08/01/2018 12:07 AM, Heinrich Schuchardt wrote:
>> On 07/31/2018 09:44 PM, Stephen Warren wrote:
>>> From: Stephen Warren <swarren@nvidia.com>
>>>
>>> Some boards define a maximum usable RAM top that's more restrictive than
>>> the ranges defined by U-Boot's memory bank definitions[1]. In this case,
>>> the unusable RAM isn't mapped in the page tables, and so the EFI code
>>> must
>>> not attempt to allocate RAM from outside the usable regions. Fix
>>> efi_find_free_memory() to detect when max_addr is unconstrained or
>>> out of
>>> range, and substitue a valid value.
>>>
>>
>> In this case the board has to call efi_add_memory_map() to mark the
>> reserved region (cf. board/raspberrypi/rpi/rpi.c) or have to mark the
>> memory region as reserved in the device tree (see
>> efi_carve_out_dt_rsv()).
>>
>> Please, check if the tegra boards with which you had problems do so.
> 
> I don't think this makes sense; memory should be managed the same way
> within U-Boot no matter which part of U-Boot is consuming that memory.
> Memory regions are currently represented by the content of the memory
> bank definitions and gd->ram_top. Having different parts of the code
> define legal RAM usage in different ways is horribly inconsistent.

Memory banks and gd->top together cannot reflect unusable memory regions
in the middle of the RAM area.

To be consistent reflect all information in the device tree and
calculate gd->ram_top from the device tree information.

> 
> At this point, I think the best thing is to revert aa909462d018
> "efi_loader: efi_allocate_pages is too restrictive" since it causes a
> regression on Jetson TX1, and we can work out the correct way to fix all
> this once the system is working again.
> 

The situation before the patch was really buggy. It is sufficient if you
get the Jetson device tree right.

Best regards

Heinrcih

[U-Boot] [PATCH] efi_loader: don't allocate unusable RAM

[Simon Glass]

Hi,

On 1 August 2018 at 10:17, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 08/01/2018 12:07 AM, Heinrich Schuchardt wrote:
>>
>> On 07/31/2018 09:44 PM, Stephen Warren wrote:
>>>
>>> From: Stephen Warren <swarren@nvidia.com>
>>>
>>> Some boards define a maximum usable RAM top that's more restrictive than
>>> the ranges defined by U-Boot's memory bank definitions[1]. In this case,
>>> the unusable RAM isn't mapped in the page tables, and so the EFI code
>>> must
>>> not attempt to allocate RAM from outside the usable regions. Fix
>>> efi_find_free_memory() to detect when max_addr is unconstrained or out of
>>> range, and substitue a valid value.
>>>
>>
>> In this case the board has to call efi_add_memory_map() to mark the
>> reserved region (cf. board/raspberrypi/rpi/rpi.c) or have to mark the
>> memory region as reserved in the device tree (see efi_carve_out_dt_rsv()).
>>
>> Please, check if the tegra boards with which you had problems do so.
>
>
> I don't think this makes sense; memory should be managed the same way within
> U-Boot no matter which part of U-Boot is consuming that memory. Memory
> regions are currently represented by the content of the memory bank
> definitions and gd->ram_top. Having different parts of the code define legal
> RAM usage in different ways is horribly inconsistent.
>
> At this point, I think the best thing is to revert aa909462d018 "efi_loader:
> efi_allocate_pages is too restrictive" since it causes a regression on
> Jetson TX1, and we can work out the correct way to fix all this once the
> system is working again.

That seems OK to me, since I don't think that patch actually fixed anything...?

Regards,
Simon

[U-Boot] [PATCH] efi_loader: don't allocate unusable RAM

[Alexander Graf]

On 08/02/2018 02:15 PM, Simon Glass wrote:
> Hi,
>
> On 1 August 2018 at 10:17, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 08/01/2018 12:07 AM, Heinrich Schuchardt wrote:
>>> On 07/31/2018 09:44 PM, Stephen Warren wrote:
>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>
>>>> Some boards define a maximum usable RAM top that's more restrictive than
>>>> the ranges defined by U-Boot's memory bank definitions[1]. In this case,
>>>> the unusable RAM isn't mapped in the page tables, and so the EFI code
>>>> must
>>>> not attempt to allocate RAM from outside the usable regions. Fix
>>>> efi_find_free_memory() to detect when max_addr is unconstrained or out of
>>>> range, and substitue a valid value.
>>>>
>>> In this case the board has to call efi_add_memory_map() to mark the
>>> reserved region (cf. board/raspberrypi/rpi/rpi.c) or have to mark the
>>> memory region as reserved in the device tree (see efi_carve_out_dt_rsv()).
>>>
>>> Please, check if the tegra boards with which you had problems do so.
>>
>> I don't think this makes sense; memory should be managed the same way within
>> U-Boot no matter which part of U-Boot is consuming that memory. Memory
>> regions are currently represented by the content of the memory bank
>> definitions and gd->ram_top. Having different parts of the code define legal
>> RAM usage in different ways is horribly inconsistent.
>>
>> At this point, I think the best thing is to revert aa909462d018 "efi_loader:
>> efi_allocate_pages is too restrictive" since it causes a regression on
>> Jetson TX1, and we can work out the correct way to fix all this once the
>> system is working again.
> That seems OK to me, since I don't think that patch actually fixed anything...?

I think I ran into a problem with the limitation to start_addr_sp in 
some other case as well, but I can't remember where. So yes, reverting 
it works for me for now if that gets us forward on the path to make 
efi_loader work for real on TX1 ;)


Alex

[U-Boot] [PATCH] efi_loader: don't allocate unusable RAM

[Stephen Warren]

On 08/01/2018 11:57 PM, Heinrich Schuchardt wrote:
> On 08/01/2018 06:17 PM, Stephen Warren wrote:
>> On 08/01/2018 12:07 AM, Heinrich Schuchardt wrote:
>>> On 07/31/2018 09:44 PM, Stephen Warren wrote:
>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>
>>>> Some boards define a maximum usable RAM top that's more restrictive than
>>>> the ranges defined by U-Boot's memory bank definitions[1]. In this case,
>>>> the unusable RAM isn't mapped in the page tables, and so the EFI code
>>>> must
>>>> not attempt to allocate RAM from outside the usable regions. Fix
>>>> efi_find_free_memory() to detect when max_addr is unconstrained or
>>>> out of
>>>> range, and substitue a valid value.
>>>>
>>>
>>> In this case the board has to call efi_add_memory_map() to mark the
>>> reserved region (cf. board/raspberrypi/rpi/rpi.c) or have to mark the
>>> memory region as reserved in the device tree (see
>>> efi_carve_out_dt_rsv()).
>>>
>>> Please, check if the tegra boards with which you had problems do so.
>>
>> I don't think this makes sense; memory should be managed the same way
>> within U-Boot no matter which part of U-Boot is consuming that memory.
>> Memory regions are currently represented by the content of the memory
>> bank definitions and gd->ram_top. Having different parts of the code
>> define legal RAM usage in different ways is horribly inconsistent.
> 
> Memory banks and gd->top together cannot reflect unusable memory regions
> in the middle of the RAM area.

Sure this is possible. For example, a common set of memory banks for 
Tegra is:

bank 0:
start:  0x080000000
length: 0x070000000
limit:  0x0f0000000
bank 1:
start:  0x100000000
length: 0x080000000
limit:  0x180000000

gd->ram_top = 0x0f0000000
That is set so that U-Boot itself (not just the UEFI code) doesn't use 
RAM above 4GB due to HW peripherals that can't access such RAM. The RAM 
above 4GB is left in the DRAM banks so that the kernel can use it for 
non-DMA purposes.

That represents a hole at 0xf0000000..0x100000000.

> To be consistent reflect all information in the device tree and
> calculate gd->ram_top from the device tree information.

Nothing in U-Boot that I'm aware of gets RAM information from DT. It all 
comes from the banks in gd. That's set up by some early low level code 
in arch/arm/mach-tegra/board2.c, which at least upstream hard-codes the 
size of the RAM gap below 4G rather than getting information from either 
DT or the earlier FW loads. So, I'm not sure what DT has to do with this.

>> At this point, I think the best thing is to revert aa909462d018
>> "efi_loader: efi_allocate_pages is too restrictive" since it causes a
>> regression on Jetson TX1, and we can work out the correct way to fix all
>> this once the system is working again.
>>
> 
> The situation before the patch was really buggy. It is sufficient if you
> get the Jetson device tree right.

What bugs specifically? Perhaps there's a better way to fix them.

Anyway, there was agreement in other messages in the thread to revert 
the problematic patch to avoid breaking the code while we work on a more 
complete solution. That's about all I can do right now since I'm heading 
off on vacation for a couple of weeks and would rather we have a working 
U-Boot running in the test farm during that time so that any other bugs 
that are introduced get caught rather than hidden. I'll send the revert.

[U-Boot] [PATCH] efi_loader: don't allocate unusable RAM

[Heinrich Schuchardt]

On 08/02/2018 07:00 PM, Stephen Warren wrote:
> On 08/01/2018 11:57 PM, Heinrich Schuchardt wrote:
>> On 08/01/2018 06:17 PM, Stephen Warren wrote:
>>> On 08/01/2018 12:07 AM, Heinrich Schuchardt wrote:
>>>> On 07/31/2018 09:44 PM, Stephen Warren wrote:
>>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>>
>>>>> Some boards define a maximum usable RAM top that's more restrictive
>>>>> than
>>>>> the ranges defined by U-Boot's memory bank definitions[1]. In this
>>>>> case,
>>>>> the unusable RAM isn't mapped in the page tables, and so the EFI code
>>>>> must
>>>>> not attempt to allocate RAM from outside the usable regions. Fix
>>>>> efi_find_free_memory() to detect when max_addr is unconstrained or
>>>>> out of
>>>>> range, and substitue a valid value.
>>>>>
>>>>
>>>> In this case the board has to call efi_add_memory_map() to mark the
>>>> reserved region (cf. board/raspberrypi/rpi/rpi.c) or have to mark the
>>>> memory region as reserved in the device tree (see
>>>> efi_carve_out_dt_rsv()).
>>>>
>>>> Please, check if the tegra boards with which you had problems do so.
>>>
>>> I don't think this makes sense; memory should be managed the same way
>>> within U-Boot no matter which part of U-Boot is consuming that memory.
>>> Memory regions are currently represented by the content of the memory
>>> bank definitions and gd->ram_top. Having different parts of the code
>>> define legal RAM usage in different ways is horribly inconsistent.
>>
>> Memory banks and gd->top together cannot reflect unusable memory regions
>> in the middle of the RAM area.
> 
> Sure this is possible. For example, a common set of memory banks for
> Tegra is:
> 
> bank 0:
> start:  0x080000000
> length: 0x070000000
> limit:  0x0f0000000
> bank 1:
> start:  0x100000000
> length: 0x080000000
> limit:  0x180000000
> 
> gd->ram_top = 0x0f0000000
> That is set so that U-Boot itself (not just the UEFI code) doesn't use
> RAM above 4GB due to HW peripherals that can't access such RAM. The RAM
> above 4GB is left in the DRAM banks so that the kernel can use it for
> non-DMA purposes

Hello Stephen,

this explanation makes the problem much clearer. Memory usable by the
operating system but not by the firmware cannot be expressed in the
device tree.

If bank 1 cannot be used by U-Boot but can be used by the operating
system we should mark it as EFI_BOOT_SERVICES_DATA by calling
efi_add_memory_map() in arch/arm/mach-tegra/board2.c. This will indicate
to the operating system that it may use this memory after exiting boot
services and the U-Boot EFI subsystem will not use this region. This
call could be done in dram_init_banksize().

Could you give this a try, please.

Best regards

Heinrich


> 
> That represents a hole at 0xf0000000..0x100000000.
> 
>> To be consistent reflect all information in the device tree and
>> calculate gd->ram_top from the device tree information.
> 
> Nothing in U-Boot that I'm aware of gets RAM information from DT. It all
> comes from the banks in gd. That's set up by some early low level code
> in arch/arm/mach-tegra/board2.c, which at least upstream hard-codes the
> size of the RAM gap below 4G rather than getting information from either
> DT or the earlier FW loads. So, I'm not sure what DT has to do with this.
> 
>>> At this point, I think the best thing is to revert aa909462d018
>>> "efi_loader: efi_allocate_pages is too restrictive" since it causes a
>>> regression on Jetson TX1, and we can work out the correct way to fix all
>>> this once the system is working again.
>>>
>>
>> The situation before the patch was really buggy. It is sufficient if you
>> get the Jetson device tree right.
> 
> What bugs specifically? Perhaps there's a better way to fix them.
> 
> Anyway, there was agreement in other messages in the thread to revert
> the problematic patch to avoid breaking the code while we work on a more
> complete solution. That's about all I can do right now since I'm heading
> off on vacation for a couple of weeks and would rather we have a working
> U-Boot running in the test farm during that time so that any other bugs
> that are introduced get caught rather than hidden. I'll send the revert.
> 

[U-Boot] [PATCH] efi_loader: don't allocate unusable RAM

[Stephen Warren]

On 08/04/2018 10:56 AM, Heinrich Schuchardt wrote:
> On 08/02/2018 07:00 PM, Stephen Warren wrote:
>> On 08/01/2018 11:57 PM, Heinrich Schuchardt wrote:
>>> On 08/01/2018 06:17 PM, Stephen Warren wrote:
>>>> On 08/01/2018 12:07 AM, Heinrich Schuchardt wrote:
>>>>> On 07/31/2018 09:44 PM, Stephen Warren wrote:
>>>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>>>
>>>>>> Some boards define a maximum usable RAM top that's more restrictive
>>>>>> than
>>>>>> the ranges defined by U-Boot's memory bank definitions[1]. In this
>>>>>> case,
>>>>>> the unusable RAM isn't mapped in the page tables, and so the EFI code
>>>>>> must
>>>>>> not attempt to allocate RAM from outside the usable regions. Fix
>>>>>> efi_find_free_memory() to detect when max_addr is unconstrained or
>>>>>> out of
>>>>>> range, and substitue a valid value.
>>>>>>
>>>>>
>>>>> In this case the board has to call efi_add_memory_map() to mark the
>>>>> reserved region (cf. board/raspberrypi/rpi/rpi.c) or have to mark the
>>>>> memory region as reserved in the device tree (see
>>>>> efi_carve_out_dt_rsv()).
>>>>>
>>>>> Please, check if the tegra boards with which you had problems do so.
>>>>
>>>> I don't think this makes sense; memory should be managed the same way
>>>> within U-Boot no matter which part of U-Boot is consuming that memory.
>>>> Memory regions are currently represented by the content of the memory
>>>> bank definitions and gd->ram_top. Having different parts of the code
>>>> define legal RAM usage in different ways is horribly inconsistent.
>>>
>>> Memory banks and gd->top together cannot reflect unusable memory regions
>>> in the middle of the RAM area.
>>
>> Sure this is possible. For example, a common set of memory banks for
>> Tegra is:
>>
>> bank 0:
>> start:  0x080000000
>> length: 0x070000000
>> limit:  0x0f0000000
>> bank 1:
>> start:  0x100000000
>> length: 0x080000000
>> limit:  0x180000000
>>
>> gd->ram_top = 0x0f0000000
>> That is set so that U-Boot itself (not just the UEFI code) doesn't use
>> RAM above 4GB due to HW peripherals that can't access such RAM. The RAM
>> above 4GB is left in the DRAM banks so that the kernel can use it for
>> non-DMA purposes
> 
> Hello Stephen,
> 
> this explanation makes the problem much clearer. Memory usable by the
> operating system but not by the firmware cannot be expressed in the
> device tree.
> 
> If bank 1 cannot be used by U-Boot but can be used by the operating
> system we should mark it as EFI_BOOT_SERVICES_DATA by calling
> efi_add_memory_map() in arch/arm/mach-tegra/board2.c. This will indicate
> to the operating system that it may use this memory after exiting boot
> services and the U-Boot EFI subsystem will not use this region. This
> call could be done in dram_init_banksize().
> 
> Could you give this a try, please.

Please revert the patch first, then we can fix this later (I sent the
revert as a patch already). I'm leaving on vacation right now, so won't
be able to look at this for at least 2 weeks. We need to fix the
regression immediately. Equally, EFI shouldn't require boards to provide
memory information to U-Boot in a different way than the rest of U-Boot
as I mentioned, so even if this WAR works, it doesn't sound scalable
across all boards.