[1/1] efi_loader: create reservations after ft_board_setup

Some memory reservations are made in ft_board_setup(). Ensure that we
create reserved memory map entries after ft_board_setup().

The downside of this patch is that if bootefi is called multiple times with
an devicetree argument superfluous reservations for the old copies of the
device tree will exist. But that is still better than missing a reservation.

Deleting the superfluous reservations is not possible because reservations
in the memory map are rounded to page size and may be coallesced.

Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
---
 cmd/bootefi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

--
2.25.1

On Sat, Mar 14, 2020 at 3:12 AM Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> Some memory reservations are made in ft_board_setup(). Ensure that we
> create reserved memory map entries after ft_board_setup().
>
> The downside of this patch is that if bootefi is called multiple times with
> an devicetree argument superfluous reservations for the old copies of the
> device tree will exist. But that is still better than missing a reservation.
>
> Deleting the superfluous reservations is not possible because reservations
> in the memory map are rounded to page size and may be coallesced.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
> ---
>  cmd/bootefi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 485f4b408a..9990959fe7 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -293,9 +293,6 @@ efi_status_t efi_install_fdt(void *fdt)
>                 return EFI_LOAD_ERROR;
>         }
>
> -       /* Create memory reservations as indicated by the device tree */
> -       efi_carve_out_dt_rsv(fdt);
> -
>         /* Prepare device tree for payload */
>         ret = copy_fdt(&fdt);
>         if (ret) {
> @@ -303,6 +300,9 @@ efi_status_t efi_install_fdt(void *fdt)
>                 return EFI_OUT_OF_RESOURCES;
>         }
>
> +       /* Create memory reservations as indicated by the device tree */
> +       efi_carve_out_dt_rsv(fdt);
> +
>         /* Install device tree as UEFI table */
>         ret = efi_install_configuration_table(&efi_guid_fdt, fdt);
>         if (ret != EFI_SUCCESS) {
> --
> 2.25.1
>
As per my understanding, copy_fdt tries allocate efi memory below dram
start + 127MB
for device tree. If efi_carve_out_dt_rsv is called after copy_fdt, it
may overwrite
a reserved-memory region. No ?

On 3/18/20 8:26 PM, Atish Patra wrote:
> On Sat, Mar 14, 2020 at 3:12 AM Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>>
>> Some memory reservations are made in ft_board_setup(). Ensure that we
>> create reserved memory map entries after ft_board_setup().
>>
>> The downside of this patch is that if bootefi is called multiple times with
>> an devicetree argument superfluous reservations for the old copies of the
>> device tree will exist. But that is still better than missing a reservation.
>>
>> Deleting the superfluous reservations is not possible because reservations
>> in the memory map are rounded to page size and may be coallesced.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>> ---
>>   cmd/bootefi.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>> index 485f4b408a..9990959fe7 100644
>> --- a/cmd/bootefi.c
>> +++ b/cmd/bootefi.c
>> @@ -293,9 +293,6 @@ efi_status_t efi_install_fdt(void *fdt)
>>                  return EFI_LOAD_ERROR;
>>          }
>>
>> -       /* Create memory reservations as indicated by the device tree */
>> -       efi_carve_out_dt_rsv(fdt);
>> -
>>          /* Prepare device tree for payload */
>>          ret = copy_fdt(&fdt);
>>          if (ret) {
>> @@ -303,6 +300,9 @@ efi_status_t efi_install_fdt(void *fdt)
>>                  return EFI_OUT_OF_RESOURCES;
>>          }
>>
>> +       /* Create memory reservations as indicated by the device tree */
>> +       efi_carve_out_dt_rsv(fdt);
>> +
>>          /* Install device tree as UEFI table */
>>          ret = efi_install_configuration_table(&efi_guid_fdt, fdt);
>>          if (ret != EFI_SUCCESS) {
>> --
>> 2.25.1
>>
> As per my understanding, copy_fdt tries allocate efi memory below dram
> start + 127MB
> for device tree. If efi_carve_out_dt_rsv is called after copy_fdt, it
> may overwrite
> a reserved-memory region. No ?

Hello Alex,

in commit ad0c1a3d2cea03011091b07e9e066bf261d1556e you wrote:

"The uEFI spec doesn't dictate where the device tree should live at, but
legacy 32bit ARM grub2 has some assumptions that it may stay at its
place when it's already loaded by the firmware.

So let's put it somewhere where Linux that comes after would happily
find it - around the recommended 128MB line."

https://www.kernel.org/doc/Documentation/devicetree/booting-without-of.txt
says:
"Device tree can be located anywhere in system RAM, but it should be
aligned on a 64 bit boundary."

Do you remember from where you took the 128 MiB value?

Best regards

Heinrich