efi: arm-stub: Correct FDT and initrd allocation rules for arm64

Message ID 1486635389-26974-1-git-send-email-ard.biesheuvel@linaro.org
State New
Headers show

Commit Message

Ard Biesheuvel Feb. 9, 2017, 10:16 a.m.
On arm64, we have made some changes over the past year to the way the
kernel itself is allocated and to how it deals with the initrd and FDT.
This patch brings the allocation logic in the EFI stub in line with that,
which is necessary because the introduction of KASLR has created the
possibility for the initrd to be allocated in a place where the kernel
may not be able to map it. (This is currently a theoretical scenario,
since it only affects systems where the size of RAM exceeds the size of
the linear mapping.)

So adhere to the arm64 boot protocol, and make sure that the initrd is
fully inside a 1GB aligned 32 GB window that covers the kernel as well.

The FDT may be anywhere in memory on arm64 now that we map it via the
fixmap, so we can lift the address restriction there completely.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 arch/arm/include/asm/efi.h              | 14 +++++++++++++-
 arch/arm64/include/asm/efi.h            | 19 ++++++++++++++++++-
 drivers/firmware/efi/libstub/arm-stub.c |  7 ++++---
 3 files changed, 35 insertions(+), 5 deletions(-)

-- 
2.7.4

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

Comments

Ard Biesheuvel Feb. 9, 2017, 10:20 a.m. | #1
On 9 February 2017 at 10:16, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On arm64, we have made some changes over the past year to the way the

> kernel itself is allocated and to how it deals with the initrd and FDT.

> This patch brings the allocation logic in the EFI stub in line with that,

> which is necessary because the introduction of KASLR has created the

> possibility for the initrd to be allocated in a place where the kernel

> may not be able to map it. (This is currently a theoretical scenario,

> since it only affects systems where the size of RAM exceeds the size of

> the linear mapping.)

>


Actually, this affects systems where the RAM /footprint/ exceeds the
size of the linear region, and it isn't that theoretical given that
some Freescale systems were already affected by this in some way. But
we switched to 48-bit VAs by default, and in that configuration, this
issue is unlikely to occur at present.


> So adhere to the arm64 boot protocol, and make sure that the initrd is

> fully inside a 1GB aligned 32 GB window that covers the kernel as well.

>

> The FDT may be anywhere in memory on arm64 now that we map it via the

> fixmap, so we can lift the address restriction there completely.

>

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  arch/arm/include/asm/efi.h              | 14 +++++++++++++-

>  arch/arm64/include/asm/efi.h            | 19 ++++++++++++++++++-

>  drivers/firmware/efi/libstub/arm-stub.c |  7 ++++---

>  3 files changed, 35 insertions(+), 5 deletions(-)

>

> diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h

> index 0b06f5341b45..62620451f60b 100644

> --- a/arch/arm/include/asm/efi.h

> +++ b/arch/arm/include/asm/efi.h

> @@ -84,6 +84,18 @@ static inline void efifb_setup_from_dmi(struct screen_info *si, const char *opt)

>   */

>  #define ZIMAGE_OFFSET_LIMIT    SZ_128M

>  #define MIN_ZIMAGE_OFFSET      MAX_UNCOMP_KERNEL_SIZE

> -#define MAX_FDT_OFFSET         ZIMAGE_OFFSET_LIMIT

> +

> +/* on ARM, the FDT should be located in the first 128 MB of RAM */

> +static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)

> +{

> +               return dram_base + ZIMAGE_OFFSET_LIMIT;

> +}

> +

> +/* on ARM, the initrd should be loaded in a lowmem region */

> +static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,

> +                                                   unsigned long image_addr)

> +{

> +       return dram_base + SZ_512M;

> +}

>

>  #endif /* _ASM_ARM_EFI_H */

> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h

> index 0b6b1633017f..6a6c8a0d1424 100644

> --- a/arch/arm64/include/asm/efi.h

> +++ b/arch/arm64/include/asm/efi.h

> @@ -46,7 +46,24 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);

>   * 2MiB so we know it won't cross a 2MiB boundary.

>   */

>  #define EFI_FDT_ALIGN  SZ_2M   /* used by allocate_new_fdt_and_exit_boot() */

> -#define MAX_FDT_OFFSET SZ_512M

> +

> +/* on arm64, the FDT may be located anywhere in system RAM */

> +static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)

> +{

> +       return ULONG_MAX;

> +}

> +

> +/*

> + * On arm64, the initrd must be completely inside a 1 GB aligned 32 GB window

> + * that covers Image as well. Since we allocate from the top down, set a max

> + * address that is virtually guaranteed to produce a suitable allocation even

> + * when the physical address of Image is randomized.

> + */

> +static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,

> +                                                   unsigned long image_addr)

> +{

> +       return ALIGN(image_addr, SZ_1G) + 31UL * SZ_1G;

> +}

>

>  #define efi_call_early(f, ...)         sys_table_arg->boottime->f(__VA_ARGS__)

>  #define __efi_call_early(f, ...)       f(__VA_ARGS__)

> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c

> index b4f7d78f9e8b..557281fe375f 100644

> --- a/drivers/firmware/efi/libstub/arm-stub.c

> +++ b/drivers/firmware/efi/libstub/arm-stub.c

> @@ -333,8 +333,9 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,

>         if (!fdt_addr)

>                 pr_efi(sys_table, "Generating empty DTB\n");

>

> -       status = handle_cmdline_files(sys_table, image, cmdline_ptr,

> -                                     "initrd=", dram_base + SZ_512M,

> +       status = handle_cmdline_files(sys_table, image, cmdline_ptr, "initrd=",

> +                                     efi_get_max_initrd_addr(dram_base,

> +                                                             *image_addr),

>                                       (unsigned long *)&initrd_addr,

>                                       (unsigned long *)&initrd_size);

>         if (status != EFI_SUCCESS)

> @@ -344,7 +345,7 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,

>

>         new_fdt_addr = fdt_addr;

>         status = allocate_new_fdt_and_exit_boot(sys_table, handle,

> -                               &new_fdt_addr, dram_base + MAX_FDT_OFFSET,

> +                               &new_fdt_addr, efi_get_max_fdt_addr(dram_base),

>                                 initrd_addr, initrd_size, cmdline_ptr,

>                                 fdt_addr, fdt_size);

>

> --

> 2.7.4

>

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeffrey Hugo Feb. 9, 2017, 5:06 p.m. | #2
On 2/9/2017 3:16 AM, Ard Biesheuvel wrote:
> On arm64, we have made some changes over the past year to the way the

> kernel itself is allocated and to how it deals with the initrd and FDT.

> This patch brings the allocation logic in the EFI stub in line with that,

> which is necessary because the introduction of KASLR has created the

> possibility for the initrd to be allocated in a place where the kernel

> may not be able to map it. (This is currently a theoretical scenario,

> since it only affects systems where the size of RAM exceeds the size of

> the linear mapping.)

>

> So adhere to the arm64 boot protocol, and make sure that the initrd is

> fully inside a 1GB aligned 32 GB window that covers the kernel as well.

>

> The FDT may be anywhere in memory on arm64 now that we map it via the

> fixmap, so we can lift the address restriction there completely.

>

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---


I'll give this a test on our platform that was running into the current 
limitation - probably this weekend.

I reviewed the code and its ok, but I do have one question.  Do we need 
to handle the case where initrd ends up below the kernel?

Lets assume KALSR puts the kernel somewhere up high in DDR, after the 
32GB mark in DDR.  Now lets assume the unlikely scenario that the initrd 
won't fit anywhere after 32GB, but will fit before 32GB.  Per my 
understanding of efi_high_alloc, it will put the initrd before the 32GB 
mark, which will be outside of the window where the kernel is.

Seems like there are 3 options -
1. We consider the scenario unlikely to the point that we don't care, 
and don't do anything
2. We check to see if initrd is allocated to be within the window, and 
if not print an error message
3. Change efi_high_alloc to take a min addess as well as a max, so that 
it will fail if the initrd can't fit in the window (which will result in 
an error message printed)

Thoughts?

>  arch/arm/include/asm/efi.h              | 14 +++++++++++++-

>  arch/arm64/include/asm/efi.h            | 19 ++++++++++++++++++-

>  drivers/firmware/efi/libstub/arm-stub.c |  7 ++++---

>  3 files changed, 35 insertions(+), 5 deletions(-)

>

> diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h

> index 0b06f5341b45..62620451f60b 100644

> --- a/arch/arm/include/asm/efi.h

> +++ b/arch/arm/include/asm/efi.h

> @@ -84,6 +84,18 @@ static inline void efifb_setup_from_dmi(struct screen_info *si, const char *opt)

>   */

>  #define ZIMAGE_OFFSET_LIMIT	SZ_128M

>  #define MIN_ZIMAGE_OFFSET	MAX_UNCOMP_KERNEL_SIZE

> -#define MAX_FDT_OFFSET		ZIMAGE_OFFSET_LIMIT

> +

> +/* on ARM, the FDT should be located in the first 128 MB of RAM */

> +static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)

> +{

> +		return dram_base + ZIMAGE_OFFSET_LIMIT;

> +}

> +

> +/* on ARM, the initrd should be loaded in a lowmem region */

> +static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,

> +						    unsigned long image_addr)

> +{

> +	return dram_base + SZ_512M;

> +}

>

>  #endif /* _ASM_ARM_EFI_H */

> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h

> index 0b6b1633017f..6a6c8a0d1424 100644

> --- a/arch/arm64/include/asm/efi.h

> +++ b/arch/arm64/include/asm/efi.h

> @@ -46,7 +46,24 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);

>   * 2MiB so we know it won't cross a 2MiB boundary.

>   */

>  #define EFI_FDT_ALIGN	SZ_2M   /* used by allocate_new_fdt_and_exit_boot() */

> -#define MAX_FDT_OFFSET	SZ_512M

> +

> +/* on arm64, the FDT may be located anywhere in system RAM */

> +static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)

> +{

> +	return ULONG_MAX;

> +}

> +

> +/*

> + * On arm64, the initrd must be completely inside a 1 GB aligned 32 GB window

> + * that covers Image as well. Since we allocate from the top down, set a max

> + * address that is virtually guaranteed to produce a suitable allocation even

> + * when the physical address of Image is randomized.

> + */

> +static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,

> +						    unsigned long image_addr)

> +{

> +	return ALIGN(image_addr, SZ_1G) + 31UL * SZ_1G;

> +}

>

>  #define efi_call_early(f, ...)		sys_table_arg->boottime->f(__VA_ARGS__)

>  #define __efi_call_early(f, ...)	f(__VA_ARGS__)

> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c

> index b4f7d78f9e8b..557281fe375f 100644

> --- a/drivers/firmware/efi/libstub/arm-stub.c

> +++ b/drivers/firmware/efi/libstub/arm-stub.c

> @@ -333,8 +333,9 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,

>  	if (!fdt_addr)

>  		pr_efi(sys_table, "Generating empty DTB\n");

>

> -	status = handle_cmdline_files(sys_table, image, cmdline_ptr,

> -				      "initrd=", dram_base + SZ_512M,

> +	status = handle_cmdline_files(sys_table, image, cmdline_ptr, "initrd=",

> +				      efi_get_max_initrd_addr(dram_base,

> +							      *image_addr),

>  				      (unsigned long *)&initrd_addr,

>  				      (unsigned long *)&initrd_size);

>  	if (status != EFI_SUCCESS)

> @@ -344,7 +345,7 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,

>

>  	new_fdt_addr = fdt_addr;

>  	status = allocate_new_fdt_and_exit_boot(sys_table, handle,

> -				&new_fdt_addr, dram_base + MAX_FDT_OFFSET,

> +				&new_fdt_addr, efi_get_max_fdt_addr(dram_base),

>  				initrd_addr, initrd_size, cmdline_ptr,

>  				fdt_addr, fdt_size);

>

>



-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ard Biesheuvel Feb. 9, 2017, 5:16 p.m. | #3
On 9 February 2017 at 17:06, Jeffrey Hugo <jhugo@codeaurora.org> wrote:
> On 2/9/2017 3:16 AM, Ard Biesheuvel wrote:

>>

>> On arm64, we have made some changes over the past year to the way the

>> kernel itself is allocated and to how it deals with the initrd and FDT.

>> This patch brings the allocation logic in the EFI stub in line with that,

>> which is necessary because the introduction of KASLR has created the

>> possibility for the initrd to be allocated in a place where the kernel

>> may not be able to map it. (This is currently a theoretical scenario,

>> since it only affects systems where the size of RAM exceeds the size of

>> the linear mapping.)

>>

>> So adhere to the arm64 boot protocol, and make sure that the initrd is

>> fully inside a 1GB aligned 32 GB window that covers the kernel as well.

>>

>> The FDT may be anywhere in memory on arm64 now that we map it via the

>> fixmap, so we can lift the address restriction there completely.

>>

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> ---

>

>

> I'll give this a test on our platform that was running into the current

> limitation - probably this weekend.

>

> I reviewed the code and its ok, but I do have one question.  Do we need to

> handle the case where initrd ends up below the kernel?

>

> Lets assume KALSR puts the kernel somewhere up high in DDR, after the 32GB

> mark in DDR.  Now lets assume the unlikely scenario that the initrd won't

> fit anywhere after 32GB, but will fit before 32GB.  Per my understanding of

> efi_high_alloc, it will put the initrd before the 32GB mark, which will be

> outside of the window where the kernel is.

>


The 32 GB does not have to be 32 GB aligned, only 1 GB aligned. So as
long as the follow expression holds, we should be fine


align(max(kernel_end, initrd_end), 1g) - align_down (min
(kernel_start, initrd_start), 1g) <= 32g


> Seems like there are 3 options -

> 1. We consider the scenario unlikely to the point that we don't care, and

> don't do anything

> 2. We check to see if initrd is allocated to be within the window, and if

> not print an error message

> 3. Change efi_high_alloc to take a min addess as well as a max, so that it

> will fail if the initrd can't fit in the window (which will result in an

> error message printed)

>

> Thoughts?

>

>

>>  arch/arm/include/asm/efi.h              | 14 +++++++++++++-

>>  arch/arm64/include/asm/efi.h            | 19 ++++++++++++++++++-

>>  drivers/firmware/efi/libstub/arm-stub.c |  7 ++++---

>>  3 files changed, 35 insertions(+), 5 deletions(-)

>>

>> diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h

>> index 0b06f5341b45..62620451f60b 100644

>> --- a/arch/arm/include/asm/efi.h

>> +++ b/arch/arm/include/asm/efi.h

>> @@ -84,6 +84,18 @@ static inline void efifb_setup_from_dmi(struct

>> screen_info *si, const char *opt)

>>   */

>>  #define ZIMAGE_OFFSET_LIMIT    SZ_128M

>>  #define MIN_ZIMAGE_OFFSET      MAX_UNCOMP_KERNEL_SIZE

>> -#define MAX_FDT_OFFSET         ZIMAGE_OFFSET_LIMIT

>> +

>> +/* on ARM, the FDT should be located in the first 128 MB of RAM */

>> +static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)

>> +{

>> +               return dram_base + ZIMAGE_OFFSET_LIMIT;

>> +}

>> +

>> +/* on ARM, the initrd should be loaded in a lowmem region */

>> +static inline unsigned long efi_get_max_initrd_addr(unsigned long

>> dram_base,

>> +                                                   unsigned long

>> image_addr)

>> +{

>> +       return dram_base + SZ_512M;

>> +}

>>

>>  #endif /* _ASM_ARM_EFI_H */

>> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h

>> index 0b6b1633017f..6a6c8a0d1424 100644

>> --- a/arch/arm64/include/asm/efi.h

>> +++ b/arch/arm64/include/asm/efi.h

>> @@ -46,7 +46,24 @@ int efi_set_mapping_permissions(struct mm_struct *mm,

>> efi_memory_desc_t *md);

>>   * 2MiB so we know it won't cross a 2MiB boundary.

>>   */

>>  #define EFI_FDT_ALIGN  SZ_2M   /* used by

>> allocate_new_fdt_and_exit_boot() */

>> -#define MAX_FDT_OFFSET SZ_512M

>> +

>> +/* on arm64, the FDT may be located anywhere in system RAM */

>> +static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)

>> +{

>> +       return ULONG_MAX;

>> +}

>> +

>> +/*

>> + * On arm64, the initrd must be completely inside a 1 GB aligned 32 GB

>> window

>> + * that covers Image as well. Since we allocate from the top down, set a

>> max

>> + * address that is virtually guaranteed to produce a suitable allocation

>> even

>> + * when the physical address of Image is randomized.

>> + */

>> +static inline unsigned long efi_get_max_initrd_addr(unsigned long

>> dram_base,

>> +                                                   unsigned long

>> image_addr)

>> +{

>> +       return ALIGN(image_addr, SZ_1G) + 31UL * SZ_1G;

>> +}

>>

>>  #define efi_call_early(f, ...)

>> sys_table_arg->boottime->f(__VA_ARGS__)

>>  #define __efi_call_early(f, ...)       f(__VA_ARGS__)

>> diff --git a/drivers/firmware/efi/libstub/arm-stub.c

>> b/drivers/firmware/efi/libstub/arm-stub.c

>> index b4f7d78f9e8b..557281fe375f 100644

>> --- a/drivers/firmware/efi/libstub/arm-stub.c

>> +++ b/drivers/firmware/efi/libstub/arm-stub.c

>> @@ -333,8 +333,9 @@ unsigned long efi_entry(void *handle,

>> efi_system_table_t *sys_table,

>>         if (!fdt_addr)

>>                 pr_efi(sys_table, "Generating empty DTB\n");

>>

>> -       status = handle_cmdline_files(sys_table, image, cmdline_ptr,

>> -                                     "initrd=", dram_base + SZ_512M,

>> +       status = handle_cmdline_files(sys_table, image, cmdline_ptr,

>> "initrd=",

>> +                                     efi_get_max_initrd_addr(dram_base,

>> +

>> *image_addr),

>>                                       (unsigned long *)&initrd_addr,

>>                                       (unsigned long *)&initrd_size);

>>         if (status != EFI_SUCCESS)

>> @@ -344,7 +345,7 @@ unsigned long efi_entry(void *handle,

>> efi_system_table_t *sys_table,

>>

>>         new_fdt_addr = fdt_addr;

>>         status = allocate_new_fdt_and_exit_boot(sys_table, handle,

>> -                               &new_fdt_addr, dram_base + MAX_FDT_OFFSET,

>> +                               &new_fdt_addr,

>> efi_get_max_fdt_addr(dram_base),

>>                                 initrd_addr, initrd_size, cmdline_ptr,

>>                                 fdt_addr, fdt_size);

>>

>>

>

>

> --

> Jeffrey Hugo

> Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies,

> Inc.

> Qualcomm Technologies, Inc. is a member of the

> Code Aurora Forum, a Linux Foundation Collaborative Project.

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeffrey Hugo Feb. 9, 2017, 5:41 p.m. | #4
On 2/9/2017 10:16 AM, Ard Biesheuvel wrote:
> On 9 February 2017 at 17:06, Jeffrey Hugo <jhugo@codeaurora.org> wrote:

>> On 2/9/2017 3:16 AM, Ard Biesheuvel wrote:

>>>

>>> On arm64, we have made some changes over the past year to the way the

>>> kernel itself is allocated and to how it deals with the initrd and FDT.

>>> This patch brings the allocation logic in the EFI stub in line with that,

>>> which is necessary because the introduction of KASLR has created the

>>> possibility for the initrd to be allocated in a place where the kernel

>>> may not be able to map it. (This is currently a theoretical scenario,

>>> since it only affects systems where the size of RAM exceeds the size of

>>> the linear mapping.)

>>>

>>> So adhere to the arm64 boot protocol, and make sure that the initrd is

>>> fully inside a 1GB aligned 32 GB window that covers the kernel as well.

>>>

>>> The FDT may be anywhere in memory on arm64 now that we map it via the

>>> fixmap, so we can lift the address restriction there completely.

>>>

>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>>> ---

>>

>>

>> I'll give this a test on our platform that was running into the current

>> limitation - probably this weekend.

>>

>> I reviewed the code and its ok, but I do have one question.  Do we need to

>> handle the case where initrd ends up below the kernel?

>>

>> Lets assume KALSR puts the kernel somewhere up high in DDR, after the 32GB

>> mark in DDR.  Now lets assume the unlikely scenario that the initrd won't

>> fit anywhere after 32GB, but will fit before 32GB.  Per my understanding of

>> efi_high_alloc, it will put the initrd before the 32GB mark, which will be

>> outside of the window where the kernel is.

>>

>

> The 32 GB does not have to be 32 GB aligned, only 1 GB aligned. So as

> long as the follow expression holds, we should be fine

>

>

> align(max(kernel_end, initrd_end), 1g) - align_down (min

> (kernel_start, initrd_start), 1g) <= 32g


Yes, and I argue there is a possibility (we'll call it extremely remote) 
where that may not hold.  My question is, do we care about that 
possibility, and if so, do we do anything about it?

>

>

>> Seems like there are 3 options -

>> 1. We consider the scenario unlikely to the point that we don't care, and

>> don't do anything

>> 2. We check to see if initrd is allocated to be within the window, and if

>> not print an error message

>> 3. Change efi_high_alloc to take a min addess as well as a max, so that it

>> will fail if the initrd can't fit in the window (which will result in an

>> error message printed)

>>

>> Thoughts?

>>

>>

>>>  arch/arm/include/asm/efi.h              | 14 +++++++++++++-

>>>  arch/arm64/include/asm/efi.h            | 19 ++++++++++++++++++-

>>>  drivers/firmware/efi/libstub/arm-stub.c |  7 ++++---

>>>  3 files changed, 35 insertions(+), 5 deletions(-)

>>>

>>> diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h

>>> index 0b06f5341b45..62620451f60b 100644

>>> --- a/arch/arm/include/asm/efi.h

>>> +++ b/arch/arm/include/asm/efi.h

>>> @@ -84,6 +84,18 @@ static inline void efifb_setup_from_dmi(struct

>>> screen_info *si, const char *opt)

>>>   */

>>>  #define ZIMAGE_OFFSET_LIMIT    SZ_128M

>>>  #define MIN_ZIMAGE_OFFSET      MAX_UNCOMP_KERNEL_SIZE

>>> -#define MAX_FDT_OFFSET         ZIMAGE_OFFSET_LIMIT

>>> +

>>> +/* on ARM, the FDT should be located in the first 128 MB of RAM */

>>> +static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)

>>> +{

>>> +               return dram_base + ZIMAGE_OFFSET_LIMIT;

>>> +}

>>> +

>>> +/* on ARM, the initrd should be loaded in a lowmem region */

>>> +static inline unsigned long efi_get_max_initrd_addr(unsigned long

>>> dram_base,

>>> +                                                   unsigned long

>>> image_addr)

>>> +{

>>> +       return dram_base + SZ_512M;

>>> +}

>>>

>>>  #endif /* _ASM_ARM_EFI_H */

>>> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h

>>> index 0b6b1633017f..6a6c8a0d1424 100644

>>> --- a/arch/arm64/include/asm/efi.h

>>> +++ b/arch/arm64/include/asm/efi.h

>>> @@ -46,7 +46,24 @@ int efi_set_mapping_permissions(struct mm_struct *mm,

>>> efi_memory_desc_t *md);

>>>   * 2MiB so we know it won't cross a 2MiB boundary.

>>>   */

>>>  #define EFI_FDT_ALIGN  SZ_2M   /* used by

>>> allocate_new_fdt_and_exit_boot() */

>>> -#define MAX_FDT_OFFSET SZ_512M

>>> +

>>> +/* on arm64, the FDT may be located anywhere in system RAM */

>>> +static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)

>>> +{

>>> +       return ULONG_MAX;

>>> +}

>>> +

>>> +/*

>>> + * On arm64, the initrd must be completely inside a 1 GB aligned 32 GB

>>> window

>>> + * that covers Image as well. Since we allocate from the top down, set a

>>> max

>>> + * address that is virtually guaranteed to produce a suitable allocation

>>> even

>>> + * when the physical address of Image is randomized.

>>> + */

>>> +static inline unsigned long efi_get_max_initrd_addr(unsigned long

>>> dram_base,

>>> +                                                   unsigned long

>>> image_addr)

>>> +{

>>> +       return ALIGN(image_addr, SZ_1G) + 31UL * SZ_1G;

>>> +}

>>>

>>>  #define efi_call_early(f, ...)

>>> sys_table_arg->boottime->f(__VA_ARGS__)

>>>  #define __efi_call_early(f, ...)       f(__VA_ARGS__)

>>> diff --git a/drivers/firmware/efi/libstub/arm-stub.c

>>> b/drivers/firmware/efi/libstub/arm-stub.c

>>> index b4f7d78f9e8b..557281fe375f 100644

>>> --- a/drivers/firmware/efi/libstub/arm-stub.c

>>> +++ b/drivers/firmware/efi/libstub/arm-stub.c

>>> @@ -333,8 +333,9 @@ unsigned long efi_entry(void *handle,

>>> efi_system_table_t *sys_table,

>>>         if (!fdt_addr)

>>>                 pr_efi(sys_table, "Generating empty DTB\n");

>>>

>>> -       status = handle_cmdline_files(sys_table, image, cmdline_ptr,

>>> -                                     "initrd=", dram_base + SZ_512M,

>>> +       status = handle_cmdline_files(sys_table, image, cmdline_ptr,

>>> "initrd=",

>>> +                                     efi_get_max_initrd_addr(dram_base,

>>> +

>>> *image_addr),

>>>                                       (unsigned long *)&initrd_addr,

>>>                                       (unsigned long *)&initrd_size);

>>>         if (status != EFI_SUCCESS)

>>> @@ -344,7 +345,7 @@ unsigned long efi_entry(void *handle,

>>> efi_system_table_t *sys_table,

>>>

>>>         new_fdt_addr = fdt_addr;

>>>         status = allocate_new_fdt_and_exit_boot(sys_table, handle,

>>> -                               &new_fdt_addr, dram_base + MAX_FDT_OFFSET,

>>> +                               &new_fdt_addr,

>>> efi_get_max_fdt_addr(dram_base),

>>>                                 initrd_addr, initrd_size, cmdline_ptr,

>>>                                 fdt_addr, fdt_size);

>>>

>>>

>>

>>

>> --

>> Jeffrey Hugo

>> Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies,

>> Inc.

>> Qualcomm Technologies, Inc. is a member of the

>> Code Aurora Forum, a Linux Foundation Collaborative Project.

>

> _______________________________________________

> linux-arm-kernel mailing list

> linux-arm-kernel@lists.infradead.org

> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

>



-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ard Biesheuvel Feb. 9, 2017, 5:45 p.m. | #5
On 9 February 2017 at 17:41, Jeffrey Hugo <jhugo@codeaurora.org> wrote:
> On 2/9/2017 10:16 AM, Ard Biesheuvel wrote:

>>

>> On 9 February 2017 at 17:06, Jeffrey Hugo <jhugo@codeaurora.org> wrote:

>>>

>>> On 2/9/2017 3:16 AM, Ard Biesheuvel wrote:

>>>>

>>>>

>>>> On arm64, we have made some changes over the past year to the way the

>>>> kernel itself is allocated and to how it deals with the initrd and FDT.

>>>> This patch brings the allocation logic in the EFI stub in line with

>>>> that,

>>>> which is necessary because the introduction of KASLR has created the

>>>> possibility for the initrd to be allocated in a place where the kernel

>>>> may not be able to map it. (This is currently a theoretical scenario,

>>>> since it only affects systems where the size of RAM exceeds the size of

>>>> the linear mapping.)

>>>>

>>>> So adhere to the arm64 boot protocol, and make sure that the initrd is

>>>> fully inside a 1GB aligned 32 GB window that covers the kernel as well.

>>>>

>>>> The FDT may be anywhere in memory on arm64 now that we map it via the

>>>> fixmap, so we can lift the address restriction there completely.

>>>>

>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>>>> ---

>>>

>>>

>>>

>>> I'll give this a test on our platform that was running into the current

>>> limitation - probably this weekend.

>>>

>>> I reviewed the code and its ok, but I do have one question.  Do we need

>>> to

>>> handle the case where initrd ends up below the kernel?

>>>

>>> Lets assume KALSR puts the kernel somewhere up high in DDR, after the

>>> 32GB

>>> mark in DDR.  Now lets assume the unlikely scenario that the initrd won't

>>> fit anywhere after 32GB, but will fit before 32GB.  Per my understanding

>>> of

>>> efi_high_alloc, it will put the initrd before the 32GB mark, which will

>>> be

>>> outside of the window where the kernel is.

>>>

>>

>> The 32 GB does not have to be 32 GB aligned, only 1 GB aligned. So as

>> long as the follow expression holds, we should be fine

>>

>>

>> align(max(kernel_end, initrd_end), 1g) - align_down (min

>> (kernel_start, initrd_start), 1g) <= 32g

>

>

> Yes, and I argue there is a possibility (we'll call it extremely remote)

> where that may not hold.  My question is, do we care about that possibility,

> and if so, do we do anything about it?

>


We allocate top down, so we start at align_down(base_of_image, 1g) +
32g, and go down until we hit a region that first our initrd. We will
disregard the region that the kernel occupies, but below that, we will
just proceed until we find a slot. This effectively means we have a 63
GB window, with the kernel in the middle, where we can load the initrd
and adhere to the boot protocol. I don't see how we could end up in
the situation where we load the kernel somewhere, and both the 31 GB
before *and* after are completely occupied.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeffrey Hugo Feb. 9, 2017, 6:01 p.m. | #6
On 2/9/2017 10:45 AM, Ard Biesheuvel wrote:
> On 9 February 2017 at 17:41, Jeffrey Hugo <jhugo@codeaurora.org> wrote:

>> On 2/9/2017 10:16 AM, Ard Biesheuvel wrote:

>>>

>>> On 9 February 2017 at 17:06, Jeffrey Hugo <jhugo@codeaurora.org> wrote:

>>>>

>>>> On 2/9/2017 3:16 AM, Ard Biesheuvel wrote:

>>>>>

>>>>>

>>>>> On arm64, we have made some changes over the past year to the way the

>>>>> kernel itself is allocated and to how it deals with the initrd and FDT.

>>>>> This patch brings the allocation logic in the EFI stub in line with

>>>>> that,

>>>>> which is necessary because the introduction of KASLR has created the

>>>>> possibility for the initrd to be allocated in a place where the kernel

>>>>> may not be able to map it. (This is currently a theoretical scenario,

>>>>> since it only affects systems where the size of RAM exceeds the size of

>>>>> the linear mapping.)

>>>>>

>>>>> So adhere to the arm64 boot protocol, and make sure that the initrd is

>>>>> fully inside a 1GB aligned 32 GB window that covers the kernel as well.

>>>>>

>>>>> The FDT may be anywhere in memory on arm64 now that we map it via the

>>>>> fixmap, so we can lift the address restriction there completely.

>>>>>

>>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>>>>> ---

>>>>

>>>>

>>>>

>>>> I'll give this a test on our platform that was running into the current

>>>> limitation - probably this weekend.

>>>>

>>>> I reviewed the code and its ok, but I do have one question.  Do we need

>>>> to

>>>> handle the case where initrd ends up below the kernel?

>>>>

>>>> Lets assume KALSR puts the kernel somewhere up high in DDR, after the

>>>> 32GB

>>>> mark in DDR.  Now lets assume the unlikely scenario that the initrd won't

>>>> fit anywhere after 32GB, but will fit before 32GB.  Per my understanding

>>>> of

>>>> efi_high_alloc, it will put the initrd before the 32GB mark, which will

>>>> be

>>>> outside of the window where the kernel is.

>>>>

>>>

>>> The 32 GB does not have to be 32 GB aligned, only 1 GB aligned. So as

>>> long as the follow expression holds, we should be fine

>>>

>>>

>>> align(max(kernel_end, initrd_end), 1g) - align_down (min

>>> (kernel_start, initrd_start), 1g) <= 32g

>>

>>

>> Yes, and I argue there is a possibility (we'll call it extremely remote)

>> where that may not hold.  My question is, do we care about that possibility,

>> and if so, do we do anything about it?

>>

>

> We allocate top down, so we start at align_down(base_of_image, 1g) +

> 32g, and go down until we hit a region that first our initrd. We will

> disregard the region that the kernel occupies, but below that, we will

> just proceed until we find a slot. This effectively means we have a 63

> GB window, with the kernel in the middle, where we can load the initrd

> and adhere to the boot protocol. I don't see how we could end up in

> the situation where we load the kernel somewhere, and both the 31 GB

> before *and* after are completely occupied.

>


No we don't.  We do not allocate top down.  Please look at efi_high_alloc.

Efi_high_alloc iterates though the memory map, low to high.  It looks to 
see if a slot can hold the allocation, and the slot does not exceed the 
specified max.  If so, efi_high_alloc retains a reference to the slot. 
Then efi_high_alloc continues iterating though the map, until the end. 
efi_high_alloc only stores a reference to the most recently valid slot, 
which would be the highest slot in the map.

My system can have 256GB (or more) of RAM.  It is possible, however 
remote, that the initrd and kernel can be more than 64GB away from each 
other.

Lets assume KASLR puts the kernel at 250GB.  Lets assume, for whatever 
reason, we can't fit the initrd above 150GB (there was just enough room 
to jam kernel there somwhow, but firmware is consuming the rest, maybe 
it put rootfs there via NFIT).  efi_high_alloc will put the initrd at 
some point just below 150GB, because it iterates low to high, and 150GB 
will be below the max of 250GB where the kernel is.  This will result in 
the initrd and kernel being ~100GB away in this example, which violates 
the requirements stated in Booting.txt

I see the situation is possible, but I admit it is remote.  If you want 
to ignore it, fine.  I would be happy with that so long as the 
assumption is documented so that if it is ever somehow violated in the 
real world, we know what broke.

-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ard Biesheuvel Feb. 9, 2017, 6:18 p.m. | #7
On 9 February 2017 at 18:01, Jeffrey Hugo <jhugo@codeaurora.org> wrote:
> On 2/9/2017 10:45 AM, Ard Biesheuvel wrote:

>>

>> On 9 February 2017 at 17:41, Jeffrey Hugo <jhugo@codeaurora.org> wrote:

>>>

>>> On 2/9/2017 10:16 AM, Ard Biesheuvel wrote:

>>>>

>>>>

>>>> On 9 February 2017 at 17:06, Jeffrey Hugo <jhugo@codeaurora.org> wrote:

>>>>>

>>>>>

>>>>> On 2/9/2017 3:16 AM, Ard Biesheuvel wrote:

>>>>>>

>>>>>>

>>>>>>

>>>>>> On arm64, we have made some changes over the past year to the way the

>>>>>> kernel itself is allocated and to how it deals with the initrd and

>>>>>> FDT.

>>>>>> This patch brings the allocation logic in the EFI stub in line with

>>>>>> that,

>>>>>> which is necessary because the introduction of KASLR has created the

>>>>>> possibility for the initrd to be allocated in a place where the kernel

>>>>>> may not be able to map it. (This is currently a theoretical scenario,

>>>>>> since it only affects systems where the size of RAM exceeds the size

>>>>>> of

>>>>>> the linear mapping.)

>>>>>>

>>>>>> So adhere to the arm64 boot protocol, and make sure that the initrd is

>>>>>> fully inside a 1GB aligned 32 GB window that covers the kernel as

>>>>>> well.

>>>>>>

>>>>>> The FDT may be anywhere in memory on arm64 now that we map it via the

>>>>>> fixmap, so we can lift the address restriction there completely.

>>>>>>

>>>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>>>>>> ---

>>>>>

>>>>>

>>>>>

>>>>>

>>>>> I'll give this a test on our platform that was running into the current

>>>>> limitation - probably this weekend.

>>>>>

>>>>> I reviewed the code and its ok, but I do have one question.  Do we need

>>>>> to

>>>>> handle the case where initrd ends up below the kernel?

>>>>>

>>>>> Lets assume KALSR puts the kernel somewhere up high in DDR, after the

>>>>> 32GB

>>>>> mark in DDR.  Now lets assume the unlikely scenario that the initrd

>>>>> won't

>>>>> fit anywhere after 32GB, but will fit before 32GB.  Per my

>>>>> understanding

>>>>> of

>>>>> efi_high_alloc, it will put the initrd before the 32GB mark, which will

>>>>> be

>>>>> outside of the window where the kernel is.

>>>>>

>>>>

>>>> The 32 GB does not have to be 32 GB aligned, only 1 GB aligned. So as

>>>> long as the follow expression holds, we should be fine

>>>>

>>>>

>>>> align(max(kernel_end, initrd_end), 1g) - align_down (min

>>>> (kernel_start, initrd_start), 1g) <= 32g

>>>

>>>

>>>

>>> Yes, and I argue there is a possibility (we'll call it extremely remote)

>>> where that may not hold.  My question is, do we care about that

>>> possibility,

>>> and if so, do we do anything about it?

>>>

>>

>> We allocate top down, so we start at align_down(base_of_image, 1g) +

>> 32g, and go down until we hit a region that first our initrd. We will

>> disregard the region that the kernel occupies, but below that, we will

>> just proceed until we find a slot. This effectively means we have a 63

>> GB window, with the kernel in the middle, where we can load the initrd

>> and adhere to the boot protocol. I don't see how we could end up in

>> the situation where we load the kernel somewhere, and both the 31 GB

>> before *and* after are completely occupied.

>>

>

> No we don't.  We do not allocate top down.  Please look at efi_high_alloc.

>

> Efi_high_alloc iterates though the memory map, low to high.  It looks to see

> if a slot can hold the allocation, and the slot does not exceed the

> specified max.  If so, efi_high_alloc retains a reference to the slot. Then

> efi_high_alloc continues iterating though the map, until the end.

> efi_high_alloc only stores a reference to the most recently valid slot,

> which would be the highest slot in the map.

>


It is documented as

/*
 * Allocate at the highest possible address that is not above 'max'.
 */

and what you describe is pretty much that, no?

> My system can have 256GB (or more) of RAM.  It is possible, however remote,

> that the initrd and kernel can be more than 64GB away from each other.

>

> Lets assume KASLR puts the kernel at 250GB.  Lets assume, for whatever

> reason, we can't fit the initrd above 150GB (there was just enough room to

> jam kernel there somwhow, but firmware is consuming the rest, maybe it put

> rootfs there via NFIT).


So before even booting the kernel, you already have 100 GB of memory
occupied? As I replied before, you are correct that in this case, you
will not be able to put the initrd within 32 GB of the kernel. But do
note that this 32 GB figure is derived from the linear region size of
a 16k pages kernel with 2 levels of translation, which is a niche
configuration by itself. On a system that has 256 GB of RAM, it is
highly unlikely that you will be using a kernel that can only map 32
GB of it.

The reason for choosing the 32 GB figure is that it relieves the boot
loader from having to go and figure out what kind of kernel is going
to be executed. Page size can be read from the Image header but the VA
size cannot. So 32 GB was a reasonable number imo.

> efi_high_alloc will put the initrd at some point

> just below 150GB, because it iterates low to high,


No, because everything above that is occupied. If efi_high_alloc()
does not do what it says on the tin, we should fix that.

> and 150GB will be below

> the max of 250GB where the kernel is.  This will result in the initrd and

> kernel being ~100GB away in this example, which violates the requirements

> stated in Booting.txt

>

> I see the situation is possible, but I admit it is remote.  If you want to

> ignore it, fine.  I would be happy with that so long as the assumption is

> documented so that if it is ever somehow violated in the real world, we know

> what broke.

>

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ard Biesheuvel Feb. 9, 2017, 6:26 p.m. | #8
On 9 February 2017 at 18:18, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 9 February 2017 at 18:01, Jeffrey Hugo <jhugo@codeaurora.org> wrote:

>> On 2/9/2017 10:45 AM, Ard Biesheuvel wrote:

>>>

>>> On 9 February 2017 at 17:41, Jeffrey Hugo <jhugo@codeaurora.org> wrote:

>>>>

>>>> On 2/9/2017 10:16 AM, Ard Biesheuvel wrote:

>>>>>

>>>>>

>>>>> On 9 February 2017 at 17:06, Jeffrey Hugo <jhugo@codeaurora.org> wrote:

>>>>>>

>>>>>>

>>>>>> On 2/9/2017 3:16 AM, Ard Biesheuvel wrote:

>>>>>>>

>>>>>>>

>>>>>>>

>>>>>>> On arm64, we have made some changes over the past year to the way the

>>>>>>> kernel itself is allocated and to how it deals with the initrd and

>>>>>>> FDT.

>>>>>>> This patch brings the allocation logic in the EFI stub in line with

>>>>>>> that,

>>>>>>> which is necessary because the introduction of KASLR has created the

>>>>>>> possibility for the initrd to be allocated in a place where the kernel

>>>>>>> may not be able to map it. (This is currently a theoretical scenario,

>>>>>>> since it only affects systems where the size of RAM exceeds the size

>>>>>>> of

>>>>>>> the linear mapping.)

>>>>>>>

>>>>>>> So adhere to the arm64 boot protocol, and make sure that the initrd is

>>>>>>> fully inside a 1GB aligned 32 GB window that covers the kernel as

>>>>>>> well.

>>>>>>>

>>>>>>> The FDT may be anywhere in memory on arm64 now that we map it via the

>>>>>>> fixmap, so we can lift the address restriction there completely.

>>>>>>>

>>>>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>>>>>>> ---

>>>>>>

>>>>>>

>>>>>>

>>>>>>

>>>>>> I'll give this a test on our platform that was running into the current

>>>>>> limitation - probably this weekend.

>>>>>>

>>>>>> I reviewed the code and its ok, but I do have one question.  Do we need

>>>>>> to

>>>>>> handle the case where initrd ends up below the kernel?

>>>>>>

>>>>>> Lets assume KALSR puts the kernel somewhere up high in DDR, after the

>>>>>> 32GB

>>>>>> mark in DDR.  Now lets assume the unlikely scenario that the initrd

>>>>>> won't

>>>>>> fit anywhere after 32GB, but will fit before 32GB.  Per my

>>>>>> understanding

>>>>>> of

>>>>>> efi_high_alloc, it will put the initrd before the 32GB mark, which will

>>>>>> be

>>>>>> outside of the window where the kernel is.

>>>>>>

>>>>>

>>>>> The 32 GB does not have to be 32 GB aligned, only 1 GB aligned. So as

>>>>> long as the follow expression holds, we should be fine

>>>>>

>>>>>

>>>>> align(max(kernel_end, initrd_end), 1g) - align_down (min

>>>>> (kernel_start, initrd_start), 1g) <= 32g

>>>>

>>>>

>>>>

>>>> Yes, and I argue there is a possibility (we'll call it extremely remote)

>>>> where that may not hold.  My question is, do we care about that

>>>> possibility,

>>>> and if so, do we do anything about it?

>>>>

>>>

>>> We allocate top down, so we start at align_down(base_of_image, 1g) +

>>> 32g, and go down until we hit a region that first our initrd. We will

>>> disregard the region that the kernel occupies, but below that, we will

>>> just proceed until we find a slot. This effectively means we have a 63

>>> GB window, with the kernel in the middle, where we can load the initrd

>>> and adhere to the boot protocol. I don't see how we could end up in

>>> the situation where we load the kernel somewhere, and both the 31 GB

>>> before *and* after are completely occupied.

>>>

>>

>> No we don't.  We do not allocate top down.  Please look at efi_high_alloc.

>>

>> Efi_high_alloc iterates though the memory map, low to high.


BTW the memory map isn't necessarily sorted per the UEFI spec, so it
iterates in what is essentially random order, not low to high.

>> It looks to see

>> if a slot can hold the allocation, and the slot does not exceed the

>> specified max.  If so, efi_high_alloc retains a reference to the slot. Then

>> efi_high_alloc continues iterating though the map, until the end.

>> efi_high_alloc only stores a reference to the most recently valid slot,

>> which would be the highest slot in the map.

>>

>

> It is documented as

>

> /*

>  * Allocate at the highest possible address that is not above 'max'.

>  */

>

> and what you describe is pretty much that, no?

>

>> My system can have 256GB (or more) of RAM.  It is possible, however remote,

>> that the initrd and kernel can be more than 64GB away from each other.

>>

>> Lets assume KASLR puts the kernel at 250GB.  Lets assume, for whatever

>> reason, we can't fit the initrd above 150GB (there was just enough room to

>> jam kernel there somwhow, but firmware is consuming the rest, maybe it put

>> rootfs there via NFIT).

>

> So before even booting the kernel, you already have 100 GB of memory

> occupied? As I replied before, you are correct that in this case, you

> will not be able to put the initrd within 32 GB of the kernel. But do

> note that this 32 GB figure is derived from the linear region size of

> a 16k pages kernel with 2 levels of translation, which is a niche

> configuration by itself. On a system that has 256 GB of RAM, it is

> highly unlikely that you will be using a kernel that can only map 32

> GB of it.

>

> The reason for choosing the 32 GB figure is that it relieves the boot

> loader from having to go and figure out what kind of kernel is going

> to be executed. Page size can be read from the Image header but the VA

> size cannot. So 32 GB was a reasonable number imo.

>

>> efi_high_alloc will put the initrd at some point

>> just below 150GB, because it iterates low to high,

>

> No, because everything above that is occupied. If efi_high_alloc()

> does not do what it says on the tin, we should fix that.

>

>> and 150GB will be below

>> the max of 250GB where the kernel is.  This will result in the initrd and

>> kernel being ~100GB away in this example, which violates the requirements

>> stated in Booting.txt

>>

>> I see the situation is possible, but I admit it is remote.  If you want to

>> ignore it, fine.  I would be happy with that so long as the assumption is

>> documented so that if it is ever somehow violated in the real world, we know

>> what broke.

>>

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeffrey Hugo Feb. 9, 2017, 7:04 p.m. | #9
On 2/9/2017 11:26 AM, Ard Biesheuvel wrote:
> On 9 February 2017 at 18:18, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

>> On 9 February 2017 at 18:01, Jeffrey Hugo <jhugo@codeaurora.org> wrote:

>>> On 2/9/2017 10:45 AM, Ard Biesheuvel wrote:

>>>>

>>>> On 9 February 2017 at 17:41, Jeffrey Hugo <jhugo@codeaurora.org> wrote:

>>>>>

>>>>> On 2/9/2017 10:16 AM, Ard Biesheuvel wrote:

>>>>>>

>>>>>>

>>>>>> On 9 February 2017 at 17:06, Jeffrey Hugo <jhugo@codeaurora.org> wrote:

>>>>>>>

>>>>>>>

>>>>>>> On 2/9/2017 3:16 AM, Ard Biesheuvel wrote:

>>>>>>>>

>>>>>>>>

>>>>>>>>

>>>>>>>> On arm64, we have made some changes over the past year to the way the

>>>>>>>> kernel itself is allocated and to how it deals with the initrd and

>>>>>>>> FDT.

>>>>>>>> This patch brings the allocation logic in the EFI stub in line with

>>>>>>>> that,

>>>>>>>> which is necessary because the introduction of KASLR has created the

>>>>>>>> possibility for the initrd to be allocated in a place where the kernel

>>>>>>>> may not be able to map it. (This is currently a theoretical scenario,

>>>>>>>> since it only affects systems where the size of RAM exceeds the size

>>>>>>>> of

>>>>>>>> the linear mapping.)

>>>>>>>>

>>>>>>>> So adhere to the arm64 boot protocol, and make sure that the initrd is

>>>>>>>> fully inside a 1GB aligned 32 GB window that covers the kernel as

>>>>>>>> well.

>>>>>>>>

>>>>>>>> The FDT may be anywhere in memory on arm64 now that we map it via the

>>>>>>>> fixmap, so we can lift the address restriction there completely.

>>>>>>>>

>>>>>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>>>>>>>> ---

>>>>>>>

>>>>>>>

>>>>>>>

>>>>>>>

>>>>>>> I'll give this a test on our platform that was running into the current

>>>>>>> limitation - probably this weekend.

>>>>>>>

>>>>>>> I reviewed the code and its ok, but I do have one question.  Do we need

>>>>>>> to

>>>>>>> handle the case where initrd ends up below the kernel?

>>>>>>>

>>>>>>> Lets assume KALSR puts the kernel somewhere up high in DDR, after the

>>>>>>> 32GB

>>>>>>> mark in DDR.  Now lets assume the unlikely scenario that the initrd

>>>>>>> won't

>>>>>>> fit anywhere after 32GB, but will fit before 32GB.  Per my

>>>>>>> understanding

>>>>>>> of

>>>>>>> efi_high_alloc, it will put the initrd before the 32GB mark, which will

>>>>>>> be

>>>>>>> outside of the window where the kernel is.

>>>>>>>

>>>>>>

>>>>>> The 32 GB does not have to be 32 GB aligned, only 1 GB aligned. So as

>>>>>> long as the follow expression holds, we should be fine

>>>>>>

>>>>>>

>>>>>> align(max(kernel_end, initrd_end), 1g) - align_down (min

>>>>>> (kernel_start, initrd_start), 1g) <= 32g

>>>>>

>>>>>

>>>>>

>>>>> Yes, and I argue there is a possibility (we'll call it extremely remote)

>>>>> where that may not hold.  My question is, do we care about that

>>>>> possibility,

>>>>> and if so, do we do anything about it?

>>>>>

>>>>

>>>> We allocate top down, so we start at align_down(base_of_image, 1g) +

>>>> 32g, and go down until we hit a region that first our initrd. We will

>>>> disregard the region that the kernel occupies, but below that, we will

>>>> just proceed until we find a slot. This effectively means we have a 63

>>>> GB window, with the kernel in the middle, where we can load the initrd

>>>> and adhere to the boot protocol. I don't see how we could end up in

>>>> the situation where we load the kernel somewhere, and both the 31 GB

>>>> before *and* after are completely occupied.

>>>>

>>>

>>> No we don't.  We do not allocate top down.  Please look at efi_high_alloc.

>>>

>>> Efi_high_alloc iterates though the memory map, low to high.

>

> BTW the memory map isn't necessarily sorted per the UEFI spec, so it

> iterates in what is essentially random order, not low to high.


True, I'm used to EDK2, which from what I've seen, keeps it ordered. 
However that's somewhat immaterial to my point that its possible for 
initrd to be far enough from kernel to break booting.txt

>

>>> It looks to see

>>> if a slot can hold the allocation, and the slot does not exceed the

>>> specified max.  If so, efi_high_alloc retains a reference to the slot. Then

>>> efi_high_alloc continues iterating though the map, until the end.

>>> efi_high_alloc only stores a reference to the most recently valid slot,

>>> which would be the highest slot in the map.

>>>

>>

>> It is documented as

>>

>> /*

>>  * Allocate at the highest possible address that is not above 'max'.

>>  */

>>

>> and what you describe is pretty much that, no?

>>

>>> My system can have 256GB (or more) of RAM.  It is possible, however remote,

>>> that the initrd and kernel can be more than 64GB away from each other.

>>>

>>> Lets assume KASLR puts the kernel at 250GB.  Lets assume, for whatever

>>> reason, we can't fit the initrd above 150GB (there was just enough room to

>>> jam kernel there somwhow, but firmware is consuming the rest, maybe it put

>>> rootfs there via NFIT).

>>

>> So before even booting the kernel, you already have 100 GB of memory

>> occupied?


That is possible, yes.  Likely?  Probably not.  Would our system fail if 
initrd and kernel are father than the prescribed restriction?  No, since 
the system can address all of RAM, we'd probably be fine.

>> As I replied before, you are correct that in this case, you

>> will not be able to put the initrd within 32 GB of the kernel. But do

>> note that this 32 GB figure is derived from the linear region size of

>> a 16k pages kernel with 2 levels of translation, which is a niche

>> configuration by itself. On a system that has 256 GB of RAM, it is

>> highly unlikely that you will be using a kernel that can only map 32

>> GB of it.

>>

>> The reason for choosing the 32 GB figure is that it relieves the boot

>> loader from having to go and figure out what kind of kernel is going

>> to be executed. Page size can be read from the Image header but the VA

>> size cannot. So 32 GB was a reasonable number imo.


Ok, so the restriction is completely arbitrary and has no real purpose. 
Ie nothing in the kernel will break, so long as you assume the system is 
not configured with more RAM than can be addressed, which doesn't feel 
reasonable to do.

I realize I'm being nitpicky, from my perspective, any issues related to 
efistub are particularly difficult to debug, so if this scenario we've 
been going around about ever popped up, it wouldn't even give you a 
print that happened when you back trace the output trying to figure out 
why the boot failed.

However, it really looks like even if the scenario occurred, there is 
zero realistic expectation anything would break, and its just a 
violation of some document that makes assumptions and should be treated 
more as guidance to try to follow, rather than hard rules.

I guess I'm satisfied, and don't see any need to continue the 
discussion.  Thanks for entertaining me.

>>

>>> efi_high_alloc will put the initrd at some point

>>> just below 150GB, because it iterates low to high,

>>

>> No, because everything above that is occupied. If efi_high_alloc()

>> does not do what it says on the tin, we should fix that.


I will agree, efi_high_alloc() does what it says on the tin (my 
interpretation of what you were saying what not what you intended, sorry 
about that), but relying on that is not sufficient to implicitly assume 
that we are holding to the restrictions in booting.txt in all scenarios.

>>

>>> and 150GB will be below

>>> the max of 250GB where the kernel is.  This will result in the initrd and

>>> kernel being ~100GB away in this example, which violates the requirements

>>> stated in Booting.txt

>>>

>>> I see the situation is possible, but I admit it is remote.  If you want to

>>> ignore it, fine.  I would be happy with that so long as the assumption is

>>> documented so that if it is ever somehow violated in the real world, we know

>>> what broke.

>>>

> --

> To unsubscribe from this list: send the line "unsubscribe linux-efi" in

> the body of a message to majordomo@vger.kernel.org

> More majordomo info at  http://vger.kernel.org/majordomo-info.html

>



-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ard Biesheuvel Feb. 9, 2017, 7:33 p.m. | #10
On 9 February 2017 at 19:04, Jeffrey Hugo <jhugo@codeaurora.org> wrote:
> On 2/9/2017 11:26 AM, Ard Biesheuvel wrote:

>>

>> BTW the memory map isn't necessarily sorted per the UEFI spec, so it

>> iterates in what is essentially random order, not low to high.

>

>

> True, I'm used to EDK2, which from what I've seen, keeps it ordered. However

> that's somewhat immaterial to my point that its possible for initrd to be

> far enough from kernel to break booting.txt

>

>>

>>>> It looks to see

>>>> if a slot can hold the allocation, and the slot does not exceed the

>>>> specified max.  If so, efi_high_alloc retains a reference to the slot.

>>>> Then

>>>> efi_high_alloc continues iterating though the map, until the end.

>>>> efi_high_alloc only stores a reference to the most recently valid slot,

>>>> which would be the highest slot in the map.

>>>>

>>>

>>> It is documented as

>>>

>>> /*

>>>  * Allocate at the highest possible address that is not above 'max'.

>>>  */

>>>

>>> and what you describe is pretty much that, no?

>>>

>>>> My system can have 256GB (or more) of RAM.  It is possible, however

>>>> remote,

>>>> that the initrd and kernel can be more than 64GB away from each other.

>>>>

>>>> Lets assume KASLR puts the kernel at 250GB.  Lets assume, for whatever

>>>> reason, we can't fit the initrd above 150GB (there was just enough room

>>>> to

>>>> jam kernel there somwhow, but firmware is consuming the rest, maybe it

>>>> put

>>>> rootfs there via NFIT).

>>>

>>>

>>> So before even booting the kernel, you already have 100 GB of memory

>>> occupied?

>

>

> That is possible, yes.  Likely?  Probably not.  Would our system fail if

> initrd and kernel are father than the prescribed restriction?  No, since the

> system can address all of RAM, we'd probably be fine.

>

>>> As I replied before, you are correct that in this case, you

>>> will not be able to put the initrd within 32 GB of the kernel. But do

>>> note that this 32 GB figure is derived from the linear region size of

>>> a 16k pages kernel with 2 levels of translation, which is a niche

>>> configuration by itself. On a system that has 256 GB of RAM, it is

>>> highly unlikely that you will be using a kernel that can only map 32

>>> GB of it.

>>>

>>> The reason for choosing the 32 GB figure is that it relieves the boot

>>> loader from having to go and figure out what kind of kernel is going

>>> to be executed. Page size can be read from the Image header but the VA

>>> size cannot. So 32 GB was a reasonable number imo.

>

>

> Ok, so the restriction is completely arbitrary and has no real purpose. Ie

> nothing in the kernel will break, so long as you assume the system is not

> configured with more RAM than can be addressed, which doesn't feel

> reasonable to do.

>

> I realize I'm being nitpicky, from my perspective, any issues related to

> efistub are particularly difficult to debug, so if this scenario we've been

> going around about ever popped up, it wouldn't even give you a print that

> happened when you back trace the output trying to figure out why the boot

> failed.

>

> However, it really looks like even if the scenario occurred, there is zero

> realistic expectation anything would break, and its just a violation of some

> document that makes assumptions and should be treated more as guidance to

> try to follow, rather than hard rules.

>


Actually, there is no reason for the stub to adhere 100% to the boot
protocol, and we already violate it deliberately by randomizing the
physical offset with a 64 KB granularity under KASLR, whereas the boot
protocol stipulates that it should be a 2 MB aligned base +
TEXT_OFFSET.

So in this case, I think it is reasonable to take VA_BITS into account
rather than use a hardcoded 32 GB. That will eliminate the problem
completely when you use a 48-bit VA kernel, and will make it highly
unlikely to ever occur on more common 39 and 42 bit VA configurations.

-- 
Ard.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ruigrok, Richard Feb. 10, 2017, 12:28 a.m. | #11
On 2/9/2017 3:16 AM, Ard Biesheuvel wrote:
> On arm64, we have made some changes over the past year to the way the

> kernel itself is allocated and to how it deals with the initrd and FDT.

> This patch brings the allocation logic in the EFI stub in line with that,

> which is necessary because the introduction of KASLR has created the

> possibility for the initrd to be allocated in a place where the kernel

> may not be able to map it. (This is currently a theoretical scenario,

> since it only affects systems where the size of RAM exceeds the size of

> the linear mapping.)

>

> So adhere to the arm64 boot protocol, and make sure that the initrd is

> fully inside a 1GB aligned 32 GB window that covers the kernel as well.

>

> The FDT may be anywhere in memory on arm64 now that we map it via the

> fixmap, so we can lift the address restriction there completely.

>

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  arch/arm/include/asm/efi.h              | 14 +++++++++++++-

>  arch/arm64/include/asm/efi.h            | 19 ++++++++++++++++++-

>  drivers/firmware/efi/libstub/arm-stub.c |  7 ++++---

>  3 files changed, 35 insertions(+), 5 deletions(-)

>

> diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h

> index 0b06f5341b45..62620451f60b 100644

> --- a/arch/arm/include/asm/efi.h

> +++ b/arch/arm/include/asm/efi.h

> @@ -84,6 +84,18 @@ static inline void efifb_setup_from_dmi(struct screen_info *si, const char *opt)

>   */

>  #define ZIMAGE_OFFSET_LIMIT	SZ_128M

>  #define MIN_ZIMAGE_OFFSET	MAX_UNCOMP_KERNEL_SIZE

> -#define MAX_FDT_OFFSET		ZIMAGE_OFFSET_LIMIT

> +

> +/* on ARM, the FDT should be located in the first 128 MB of RAM */

> +static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)

> +{

> +		return dram_base + ZIMAGE_OFFSET_LIMIT;

> +}

> +

> +/* on ARM, the initrd should be loaded in a lowmem region */

> +static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,

> +						    unsigned long image_addr)

> +{

> +	return dram_base + SZ_512M;

> +}

>  

>  #endif /* _ASM_ARM_EFI_H */

> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h

> index 0b6b1633017f..6a6c8a0d1424 100644

> --- a/arch/arm64/include/asm/efi.h

> +++ b/arch/arm64/include/asm/efi.h

> @@ -46,7 +46,24 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);

>   * 2MiB so we know it won't cross a 2MiB boundary.

>   */

>  #define EFI_FDT_ALIGN	SZ_2M   /* used by allocate_new_fdt_and_exit_boot() */

> -#define MAX_FDT_OFFSET	SZ_512M

> +

> +/* on arm64, the FDT may be located anywhere in system RAM */

> +static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)

> +{

> +	return ULONG_MAX;

> +}

> +

> +/*

> + * On arm64, the initrd must be completely inside a 1 GB aligned 32 GB window

> + * that covers Image as well. Since we allocate from the top down, set a max

> + * address that is virtually guaranteed to produce a suitable allocation even

> + * when the physical address of Image is randomized.

> + */

> +static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,

> +						    unsigned long image_addr)

> +{

> +	return ALIGN(image_addr, SZ_1G) + 31UL * SZ_1G;

> +}

>  

>  #define efi_call_early(f, ...)		sys_table_arg->boottime->f(__VA_ARGS__)

>  #define __efi_call_early(f, ...)	f(__VA_ARGS__)

> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c

> index b4f7d78f9e8b..557281fe375f 100644

> --- a/drivers/firmware/efi/libstub/arm-stub.c

> +++ b/drivers/firmware/efi/libstub/arm-stub.c

> @@ -333,8 +333,9 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,

>  	if (!fdt_addr)

>  		pr_efi(sys_table, "Generating empty DTB\n");

>  

> -	status = handle_cmdline_files(sys_table, image, cmdline_ptr,

> -				      "initrd=", dram_base + SZ_512M,

> +	status = handle_cmdline_files(sys_table, image, cmdline_ptr, "initrd=",

> +				      efi_get_max_initrd_addr(dram_base,

> +							      *image_addr),

>  				      (unsigned long *)&initrd_addr,

>  				      (unsigned long *)&initrd_size);

>  	if (status != EFI_SUCCESS)

> @@ -344,7 +345,7 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,

>  

>  	new_fdt_addr = fdt_addr;

>  	status = allocate_new_fdt_and_exit_boot(sys_table, handle,

> -				&new_fdt_addr, dram_base + MAX_FDT_OFFSET,

> +				&new_fdt_addr, efi_get_max_fdt_addr(dram_base),

>  				initrd_addr, initrd_size, cmdline_ptr,

>  				fdt_addr, fdt_size);

>  


This was tested successfully on our QDT2400 system on which we found this failure.

Tested-by: Richard Ruigrok <rruigrok@codeaurora.org>


-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

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

Patch hide | download patch | download mbox

diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
index 0b06f5341b45..62620451f60b 100644
--- a/arch/arm/include/asm/efi.h
+++ b/arch/arm/include/asm/efi.h
@@ -84,6 +84,18 @@  static inline void efifb_setup_from_dmi(struct screen_info *si, const char *opt)
  */
 #define ZIMAGE_OFFSET_LIMIT	SZ_128M
 #define MIN_ZIMAGE_OFFSET	MAX_UNCOMP_KERNEL_SIZE
-#define MAX_FDT_OFFSET		ZIMAGE_OFFSET_LIMIT
+
+/* on ARM, the FDT should be located in the first 128 MB of RAM */
+static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)
+{
+		return dram_base + ZIMAGE_OFFSET_LIMIT;
+}
+
+/* on ARM, the initrd should be loaded in a lowmem region */
+static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,
+						    unsigned long image_addr)
+{
+	return dram_base + SZ_512M;
+}
 
 #endif /* _ASM_ARM_EFI_H */
diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index 0b6b1633017f..6a6c8a0d1424 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -46,7 +46,24 @@  int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);
  * 2MiB so we know it won't cross a 2MiB boundary.
  */
 #define EFI_FDT_ALIGN	SZ_2M   /* used by allocate_new_fdt_and_exit_boot() */
-#define MAX_FDT_OFFSET	SZ_512M
+
+/* on arm64, the FDT may be located anywhere in system RAM */
+static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)
+{
+	return ULONG_MAX;
+}
+
+/*
+ * On arm64, the initrd must be completely inside a 1 GB aligned 32 GB window
+ * that covers Image as well. Since we allocate from the top down, set a max
+ * address that is virtually guaranteed to produce a suitable allocation even
+ * when the physical address of Image is randomized.
+ */
+static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,
+						    unsigned long image_addr)
+{
+	return ALIGN(image_addr, SZ_1G) + 31UL * SZ_1G;
+}
 
 #define efi_call_early(f, ...)		sys_table_arg->boottime->f(__VA_ARGS__)
 #define __efi_call_early(f, ...)	f(__VA_ARGS__)
diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
index b4f7d78f9e8b..557281fe375f 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -333,8 +333,9 @@  unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
 	if (!fdt_addr)
 		pr_efi(sys_table, "Generating empty DTB\n");
 
-	status = handle_cmdline_files(sys_table, image, cmdline_ptr,
-				      "initrd=", dram_base + SZ_512M,
+	status = handle_cmdline_files(sys_table, image, cmdline_ptr, "initrd=",
+				      efi_get_max_initrd_addr(dram_base,
+							      *image_addr),
 				      (unsigned long *)&initrd_addr,
 				      (unsigned long *)&initrd_size);
 	if (status != EFI_SUCCESS)
@@ -344,7 +345,7 @@  unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
 
 	new_fdt_addr = fdt_addr;
 	status = allocate_new_fdt_and_exit_boot(sys_table, handle,
-				&new_fdt_addr, dram_base + MAX_FDT_OFFSET,
+				&new_fdt_addr, efi_get_max_fdt_addr(dram_base),
 				initrd_addr, initrd_size, cmdline_ptr,
 				fdt_addr, fdt_size);