[PATCH 0/4] bootefi fixes for aarch64/layerscape

[PATCH 0/4] bootefi fixes for aarch64/layerscape

[Michael Walle]

I'm bringing up efiboot on a ARM64 board which runs without TF-a and PSCI,
therefore the secondary cores are brought up by spin-tables. I ran into
several problems. Here are the fixes.

Michael Walle (4):
  efi_loader: aarch64: align runtime section to 64kb
  efi_loader: check alignment in efi_add_memory_map()
  fsl-layerscape: align first parameter of efi_add_memory_map()
  efi_loader: call smp_kick_all_cpus()

 arch/arm/cpu/armv8/fsl-layerscape/fdt.c |  2 +-
 arch/arm/cpu/armv8/u-boot.lds           |  9 ++++++++-
 common/bootm.c                          |  9 +++++++++
 lib/efi_loader/efi_memory.c             | 18 +++++-------------
 lib/efi_loader/efi_setup.c              |  6 ++++++
 5 files changed, 29 insertions(+), 15 deletions(-)

-- 
2.20.1

[PATCH 1/4] efi_loader: aarch64: align runtime section to 64kb

[Michael Walle]

Commit 7a82c3051c8f ("efi_loader: Align runtime section to 64kb")
already aligned the memory region to 64kb, but it does not align the
actual efi runtime code. Thus it is likely, that efi_add_memory_map()
actually adds a larger memory region than the efi runtime code really
is, which is no error I guess. But what actually leads to an error is
that there might be other efi_add_memory_map() calls with regions
overlapping with the already registered efi runtime code section.

Align the actual runtime code to 64kb instead.

Fixes: 7a82c3051c8f ("efi_loader: Align runtime section to 64kb")
Signed-off-by: Michael Walle <michael@walle.cc>
---
 arch/arm/cpu/armv8/u-boot.lds |  9 ++++++++-
 lib/efi_loader/efi_memory.c   | 15 ++-------------
 2 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
index 2554980595..3bc4675586 100644
--- a/arch/arm/cpu/armv8/u-boot.lds
+++ b/arch/arm/cpu/armv8/u-boot.lds
@@ -27,7 +27,14 @@ SECTIONS
 		CPUDIR/start.o (.text*)
 	}
 
-	/* This needs to come before *(.text*) */
+	/*
+	 * Runtime Services must be 64KiB aligned according to the
+	 * "AArch64 Platforms" section in the UEFI spec (2.7+).
+	 *
+	 * This needs to come before *(.text*)
+	 */
+
+	. = ALIGN(65536);
 	.efi_runtime : {
                 __efi_runtime_start = .;
 		*(.text.efi_runtime*)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index 97d90f069a..fd79178da9 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -12,7 +12,6 @@
 #include <mapmem.h>
 #include <watchdog.h>
 #include <linux/list_sort.h>
-#include <linux/sizes.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -734,7 +733,6 @@ __weak void efi_add_known_memory(void)
 static void add_u_boot_and_runtime(void)
 {
 	unsigned long runtime_start, runtime_end, runtime_pages;
-	unsigned long runtime_mask = EFI_PAGE_MASK;
 	unsigned long uboot_start, uboot_pages;
 	unsigned long uboot_stack_size = 16 * 1024 * 1024;
 
@@ -745,22 +743,13 @@ static void add_u_boot_and_runtime(void)
 		       uboot_start + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
 	efi_add_memory_map(uboot_start, uboot_pages, EFI_LOADER_DATA, false);
 
-#if defined(__aarch64__)
-	/*
-	 * Runtime Services must be 64KiB aligned according to the
-	 * "AArch64 Platforms" section in the UEFI spec (2.7+).
-	 */
-
-	runtime_mask = SZ_64K - 1;
-#endif
-
 	/*
 	 * Add Runtime Services. We mark surrounding boottime code as runtime as
 	 * well to fulfill the runtime alignment constraints but avoid padding.
 	 */
-	runtime_start = (ulong)&__efi_runtime_start & ~runtime_mask;
+	runtime_start = (ulong)&__efi_runtime_start & ~EFI_PAGE_MASK;
 	runtime_end = (ulong)&__efi_runtime_stop;
-	runtime_end = (runtime_end + runtime_mask) & ~runtime_mask;
+	runtime_end = (runtime_end + EFI_PAGE_MASK) & ~EFI_PAGE_MASK;
 	runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT;
 	efi_add_memory_map(runtime_start, runtime_pages,
 			   EFI_RUNTIME_SERVICES_CODE, false);
-- 
2.20.1

[PATCH 1/4] efi_loader: aarch64: align runtime section to 64kb

[Heinrich Schuchardt]

On 5/14/20 2:38 PM, Michael Walle wrote:
> Commit 7a82c3051c8f ("efi_loader: Align runtime section to 64kb")
> already aligned the memory region to 64kb, but it does not align the
> actual efi runtime code. Thus it is likely, that efi_add_memory_map()
> actually adds a larger memory region than the efi runtime code really
> is, which is no error I guess. But what actually leads to an error is
> that there might be other efi_add_memory_map() calls with regions
> overlapping with the already registered efi runtime code section.

Do you relate to this sentence:

"If a 64KiB physical page contains any 4KiB page with any of the
following types listed below, then all 4KiB pages in the 64KiB page must
use identical ARM Memory Page Attributes"?


>
> Align the actual runtime code to 64kb instead.
>
> Fixes: 7a82c3051c8f ("efi_loader: Align runtime section to 64kb")
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  arch/arm/cpu/armv8/u-boot.lds |  9 ++++++++-
>  lib/efi_loader/efi_memory.c   | 15 ++-------------
>  2 files changed, 10 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
> index 2554980595..3bc4675586 100644
> --- a/arch/arm/cpu/armv8/u-boot.lds
> +++ b/arch/arm/cpu/armv8/u-boot.lds
> @@ -27,7 +27,14 @@ SECTIONS
>  		CPUDIR/start.o (.text*)
>  	}
>
> -	/* This needs to come before *(.text*) */
> +	/*
> +	 * Runtime Services must be 64KiB aligned according to the
> +	 * "AArch64 Platforms" section in the UEFI spec (2.7+).

This is not the exact requirement of the spec. Please, use a description
that matches the spec.

The requirement that 64KiB areas should have the same attributes was
already presen in UEFI spec 2.4. Please, drop the version reference.

> +	 *
> +	 * This needs to come before *(.text*)
> +	 */
> +
> +	. = ALIGN(65536);

Isn't this an alignment before relocation that is irrelevant with
regards to the UEFI spec?

>  	.efi_runtime : {
>                  __efi_runtime_start = .;
>  		*(.text.efi_runtime*)
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index 97d90f069a..fd79178da9 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -12,7 +12,6 @@
>  #include <mapmem.h>
>  #include <watchdog.h>
>  #include <linux/list_sort.h>
> -#include <linux/sizes.h>
>
>  DECLARE_GLOBAL_DATA_PTR;
>
> @@ -734,7 +733,6 @@ __weak void efi_add_known_memory(void)
>  static void add_u_boot_and_runtime(void)
>  {
>  	unsigned long runtime_start, runtime_end, runtime_pages;
> -	unsigned long runtime_mask = EFI_PAGE_MASK;
>  	unsigned long uboot_start, uboot_pages;
>  	unsigned long uboot_stack_size = 16 * 1024 * 1024;
>
> @@ -745,22 +743,13 @@ static void add_u_boot_and_runtime(void)
>  		       uboot_start + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
>  	efi_add_memory_map(uboot_start, uboot_pages, EFI_LOADER_DATA, false);
>
> -#if defined(__aarch64__)
> -	/*
> -	 * Runtime Services must be 64KiB aligned according to the
> -	 * "AArch64 Platforms" section in the UEFI spec (2.7+).> -	 */
> -
> -	runtime_mask = SZ_64K - 1;
> -#endif
> -
>  	/*
>  	 * Add Runtime Services. We mark surrounding boottime code as runtime as
>  	 * well to fulfill the runtime alignment constraints but avoid padding.
>  	 */
> -	runtime_start = (ulong)&__efi_runtime_start & ~runtime_mask;
> +	runtime_start = (ulong)&__efi_runtime_start & ~EFI_PAGE_MASK;
>  	runtime_end = (ulong)&__efi_runtime_stop;
> -	runtime_end = (runtime_end + runtime_mask) & ~runtime_mask;
> +	runtime_end = (runtime_end + EFI_PAGE_MASK) & ~EFI_PAGE_MASK;

I cannot see that after these changes you match the requirements of the
UEFI spec.

Best regards

Heinrich

>  	runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT;
>  	efi_add_memory_map(runtime_start, runtime_pages,
>  			   EFI_RUNTIME_SERVICES_CODE, false);
>

[PATCH 1/4] efi_loader: aarch64: align runtime section to 64kb

[Michael Walle]

Am 2020-05-14 20:27, schrieb Heinrich Schuchardt:
> On 5/14/20 2:38 PM, Michael Walle wrote:
>> Commit 7a82c3051c8f ("efi_loader: Align runtime section to 64kb")
>> already aligned the memory region to 64kb, but it does not align the
>> actual efi runtime code. Thus it is likely, that efi_add_memory_map()
>> actually adds a larger memory region than the efi runtime code really
>> is, which is no error I guess. But what actually leads to an error is
>> that there might be other efi_add_memory_map() calls with regions
>> overlapping with the already registered efi runtime code section.
> 
> Do you relate to this sentence:
> 
> "If a 64KiB physical page contains any 4KiB page with any of the
> following types listed below, then all 4KiB pages in the 64KiB page 
> must
> use identical ARM Memory Page Attributes"?

I don't think this is what I want to fix here.

Commit 7a82c3051c8f ("efi_loader: Align runtime section to 64kb")
reserves the memory region for runtime services and align the start
(and size) to 64kb boundaries. But the actual runtime services are
not 64kb aligned. And at least on my board, another memory region
right next to it is reserved as well. But of course as another type.

>> 
>> Align the actual runtime code to 64kb instead.
>> 
>> Fixes: 7a82c3051c8f ("efi_loader: Align runtime section to 64kb")
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>>  arch/arm/cpu/armv8/u-boot.lds |  9 ++++++++-
>>  lib/efi_loader/efi_memory.c   | 15 ++-------------
>>  2 files changed, 10 insertions(+), 14 deletions(-)
>> 
>> diff --git a/arch/arm/cpu/armv8/u-boot.lds 
>> b/arch/arm/cpu/armv8/u-boot.lds
>> index 2554980595..3bc4675586 100644
>> --- a/arch/arm/cpu/armv8/u-boot.lds
>> +++ b/arch/arm/cpu/armv8/u-boot.lds
>> @@ -27,7 +27,14 @@ SECTIONS
>>  		CPUDIR/start.o (.text*)
>>  	}
>> 
>> -	/* This needs to come before *(.text*) */
>> +	/*
>> +	 * Runtime Services must be 64KiB aligned according to the
>> +	 * "AArch64 Platforms" section in the UEFI spec (2.7+).
> 
> This is not the exact requirement of the spec. Please, use a 
> description
> that matches the spec.

well I just moved this exact sentence. I'm not familiar with the UEFI
spec.

> The requirement that 64KiB areas should have the same attributes was
> already presen in UEFI spec 2.4. Please, drop the version reference.

As mentioned above, its about the alignment of the runtime section.

-michael

> 
>> +	 *
>> +	 * This needs to come before *(.text*)
>> +	 */
>> +
>> +	. = ALIGN(65536);
> 
> Isn't this an alignment before relocation that is irrelevant with
> regards to the UEFI spec?
> 
>>  	.efi_runtime : {
>>                  __efi_runtime_start = .;
>>  		*(.text.efi_runtime*)
>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
>> index 97d90f069a..fd79178da9 100644
>> --- a/lib/efi_loader/efi_memory.c
>> +++ b/lib/efi_loader/efi_memory.c
>> @@ -12,7 +12,6 @@
>>  #include <mapmem.h>
>>  #include <watchdog.h>
>>  #include <linux/list_sort.h>
>> -#include <linux/sizes.h>
>> 
>>  DECLARE_GLOBAL_DATA_PTR;
>> 
>> @@ -734,7 +733,6 @@ __weak void efi_add_known_memory(void)
>>  static void add_u_boot_and_runtime(void)
>>  {
>>  	unsigned long runtime_start, runtime_end, runtime_pages;
>> -	unsigned long runtime_mask = EFI_PAGE_MASK;
>>  	unsigned long uboot_start, uboot_pages;
>>  	unsigned long uboot_stack_size = 16 * 1024 * 1024;
>> 
>> @@ -745,22 +743,13 @@ static void add_u_boot_and_runtime(void)
>>  		       uboot_start + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
>>  	efi_add_memory_map(uboot_start, uboot_pages, EFI_LOADER_DATA, 
>> false);
>> 
>> -#if defined(__aarch64__)
>> -	/*
>> -	 * Runtime Services must be 64KiB aligned according to the
>> -	 * "AArch64 Platforms" section in the UEFI spec (2.7+).> -	 */
>> -
>> -	runtime_mask = SZ_64K - 1;
>> -#endif
>> -
>>  	/*
>>  	 * Add Runtime Services. We mark surrounding boottime code as 
>> runtime as
>>  	 * well to fulfill the runtime alignment constraints but avoid 
>> padding.
>>  	 */
>> -	runtime_start = (ulong)&__efi_runtime_start & ~runtime_mask;
>> +	runtime_start = (ulong)&__efi_runtime_start & ~EFI_PAGE_MASK;
>>  	runtime_end = (ulong)&__efi_runtime_stop;
>> -	runtime_end = (runtime_end + runtime_mask) & ~runtime_mask;
>> +	runtime_end = (runtime_end + EFI_PAGE_MASK) & ~EFI_PAGE_MASK;
> 
> I cannot see that after these changes you match the requirements of the
> UEFI spec.
> 
> Best regards
> 
> Heinrich
> 
>>  	runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT;
>>  	efi_add_memory_map(runtime_start, runtime_pages,
>>  			   EFI_RUNTIME_SERVICES_CODE, false);
>> 

-- 
-michael

[PATCH 1/4] efi_loader: aarch64: align runtime section to 64kb

[Heinrich Schuchardt]

On 5/14/20 9:04 PM, Michael Walle wrote:
> Am 2020-05-14 20:27, schrieb Heinrich Schuchardt:
>> On 5/14/20 2:38 PM, Michael Walle wrote:
>>> Commit 7a82c3051c8f ("efi_loader: Align runtime section to 64kb")
>>> already aligned the memory region to 64kb, but it does not align the
>>> actual efi runtime code. Thus it is likely, that efi_add_memory_map()
>>> actually adds a larger memory region than the efi runtime code really
>>> is, which is no error I guess. But what actually leads to an error is
>>> that there might be other efi_add_memory_map() calls with regions
>>> overlapping with the already registered efi runtime code section.
>>
>> Do you relate to this sentence:
>>
>> "If a 64KiB physical page contains any 4KiB page with any of the
>> following types listed below, then all 4KiB pages in the 64KiB page must
>> use identical ARM Memory Page Attributes"?
>
> I don't think this is what I want to fix here.
>
> Commit 7a82c3051c8f ("efi_loader: Align runtime section to 64kb")
> reserves the memory region for runtime services and align the start
> (and size) to 64kb boundaries. But the actual runtime services are
> not 64kb aligned. And at least on my board, another memory region
> right next to it is reserved as well. But of course as another type.
>
>>>
>>> Align the actual runtime code to 64kb instead.
>>>
>>> Fixes: 7a82c3051c8f ("efi_loader: Align runtime section to 64kb")
>>> Signed-off-by: Michael Walle <michael@walle.cc>
>>> ---
>>> ?arch/arm/cpu/armv8/u-boot.lds |? 9 ++++++++-
>>> ?lib/efi_loader/efi_memory.c?? | 15 ++-------------
>>> ?2 files changed, 10 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/arch/arm/cpu/armv8/u-boot.lds
>>> b/arch/arm/cpu/armv8/u-boot.lds
>>> index 2554980595..3bc4675586 100644
>>> --- a/arch/arm/cpu/armv8/u-boot.lds
>>> +++ b/arch/arm/cpu/armv8/u-boot.lds
>>> @@ -27,7 +27,14 @@ SECTIONS
>>> ???????? CPUDIR/start.o (.text*)
>>> ???? }
>>>
>>> -??? /* This needs to come before *(.text*) */
>>> +??? /*
>>> +???? * Runtime Services must be 64KiB aligned according to the
>>> +???? * "AArch64 Platforms" section in the UEFI spec (2.7+).
>>
>> This is not the exact requirement of the spec. Please, use a description
>> that matches the spec.
>
> well I just moved this exact sentence. I'm not familiar with the UEFI
> spec.
>
>> The requirement that 64KiB areas should have the same attributes was
>> already presen in UEFI spec 2.4. Please, drop the version reference.
>
> As mentioned above, its about the alignment of the runtime section.

Please, indicate the exact requirement in the "UEFI 2.8 errata A"
specification you are refering to. Cf.
file:///home/zfsdt/Documents/UEFI/UEFI_Spec_2_8_A_Feb14.pdf

I only found the requirement at the bottom of page 35 of said PDF
dealing with 64KiB pages.

Please, further indicate in which respect the current code violates the
UEFI requirements.

Best regards

Heinrich

>
> -michael
>
>>
>>> +???? *
>>> +???? * This needs to come before *(.text*)
>>> +???? */
>>> +
>>> +??? . = ALIGN(65536);
>>
>> Isn't this an alignment before relocation that is irrelevant with
>> regards to the UEFI spec?
>>
>>> ???? .efi_runtime : {
>>> ???????????????? __efi_runtime_start = .;
>>> ???????? *(.text.efi_runtime*)
>>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
>>> index 97d90f069a..fd79178da9 100644
>>> --- a/lib/efi_loader/efi_memory.c
>>> +++ b/lib/efi_loader/efi_memory.c
>>> @@ -12,7 +12,6 @@
>>> ?#include <mapmem.h>
>>> ?#include <watchdog.h>
>>> ?#include <linux/list_sort.h>
>>> -#include <linux/sizes.h>
>>>
>>> ?DECLARE_GLOBAL_DATA_PTR;
>>>
>>> @@ -734,7 +733,6 @@ __weak void efi_add_known_memory(void)
>>> ?static void add_u_boot_and_runtime(void)
>>> ?{
>>> ???? unsigned long runtime_start, runtime_end, runtime_pages;
>>> -??? unsigned long runtime_mask = EFI_PAGE_MASK;
>>> ???? unsigned long uboot_start, uboot_pages;
>>> ???? unsigned long uboot_stack_size = 16 * 1024 * 1024;
>>>
>>> @@ -745,22 +743,13 @@ static void add_u_boot_and_runtime(void)
>>> ??????????????? uboot_start + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
>>> ???? efi_add_memory_map(uboot_start, uboot_pages, EFI_LOADER_DATA,
>>> false);
>>>
>>> -#if defined(__aarch64__)
>>> -??? /*
>>> -???? * Runtime Services must be 64KiB aligned according to the
>>> -???? * "AArch64 Platforms" section in the UEFI spec (2.7+).> -???? */
>>> -
>>> -??? runtime_mask = SZ_64K - 1;
>>> -#endif
>>> -
>>> ???? /*
>>> ????? * Add Runtime Services. We mark surrounding boottime code as
>>> runtime as
>>> ????? * well to fulfill the runtime alignment constraints but avoid
>>> padding.
>>> ????? */
>>> -??? runtime_start = (ulong)&__efi_runtime_start & ~runtime_mask;
>>> +??? runtime_start = (ulong)&__efi_runtime_start & ~EFI_PAGE_MASK;
>>> ???? runtime_end = (ulong)&__efi_runtime_stop;
>>> -??? runtime_end = (runtime_end + runtime_mask) & ~runtime_mask;
>>> +??? runtime_end = (runtime_end + EFI_PAGE_MASK) & ~EFI_PAGE_MASK;
>>
>> I cannot see that after these changes you match the requirements of the
>> UEFI spec.
>>
>> Best regards
>>
>> Heinrich
>>
>>> ???? runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT;
>>> ???? efi_add_memory_map(runtime_start, runtime_pages,
>>> ??????????????? EFI_RUNTIME_SERVICES_CODE, false);
>>>
>

[PATCH 1/4] efi_loader: aarch64: align runtime section to 64kb

[Michael Walle]

Am 2020-05-14 23:03, schrieb Heinrich Schuchardt:
> On 5/14/20 9:04 PM, Michael Walle wrote:
>> Am 2020-05-14 20:27, schrieb Heinrich Schuchardt:
>>> On 5/14/20 2:38 PM, Michael Walle wrote:
>>>> Commit 7a82c3051c8f ("efi_loader: Align runtime section to 64kb")
>>>> already aligned the memory region to 64kb, but it does not align the
>>>> actual efi runtime code. Thus it is likely, that 
>>>> efi_add_memory_map()
>>>> actually adds a larger memory region than the efi runtime code 
>>>> really
>>>> is, which is no error I guess. But what actually leads to an error 
>>>> is
>>>> that there might be other efi_add_memory_map() calls with regions
>>>> overlapping with the already registered efi runtime code section.
>>> 
>>> Do you relate to this sentence:
>>> 
>>> "If a 64KiB physical page contains any 4KiB page with any of the
>>> following types listed below, then all 4KiB pages in the 64KiB page 
>>> must
>>> use identical ARM Memory Page Attributes"?
>> 
>> I don't think this is what I want to fix here.
>> 
>> Commit 7a82c3051c8f ("efi_loader: Align runtime section to 64kb")
>> reserves the memory region for runtime services and align the start
>> (and size) to 64kb boundaries. But the actual runtime services are
>> not 64kb aligned. And at least on my board, another memory region
>> right next to it is reserved as well. But of course as another type.
>> 
>>>> 
>>>> Align the actual runtime code to 64kb instead.
>>>> 
>>>> Fixes: 7a82c3051c8f ("efi_loader: Align runtime section to 64kb")
>>>> Signed-off-by: Michael Walle <michael@walle.cc>
>>>> ---
>>>> ?arch/arm/cpu/armv8/u-boot.lds |? 9 ++++++++-
>>>> ?lib/efi_loader/efi_memory.c?? | 15 ++-------------
>>>> ?2 files changed, 10 insertions(+), 14 deletions(-)
>>>> 
>>>> diff --git a/arch/arm/cpu/armv8/u-boot.lds
>>>> b/arch/arm/cpu/armv8/u-boot.lds
>>>> index 2554980595..3bc4675586 100644
>>>> --- a/arch/arm/cpu/armv8/u-boot.lds
>>>> +++ b/arch/arm/cpu/armv8/u-boot.lds
>>>> @@ -27,7 +27,14 @@ SECTIONS
>>>> ???????? CPUDIR/start.o (.text*)
>>>> ???? }
>>>> 
>>>> -??? /* This needs to come before *(.text*) */
>>>> +??? /*
>>>> +???? * Runtime Services must be 64KiB aligned according to the
>>>> +???? * "AArch64 Platforms" section in the UEFI spec (2.7+).
>>> 
>>> This is not the exact requirement of the spec. Please, use a 
>>> description
>>> that matches the spec.
>> 
>> well I just moved this exact sentence. I'm not familiar with the UEFI
>> spec.
>> 
>>> The requirement that 64KiB areas should have the same attributes was
>>> already presen in UEFI spec 2.4. Please, drop the version reference.
>> 
>> As mentioned above, its about the alignment of the runtime section.
> 
> Please, indicate the exact requirement in the "UEFI 2.8 errata A"
> specification you are refering to. Cf.
> file:///home/zfsdt/Documents/UEFI/UEFI_Spec_2_8_A_Feb14.pdf
> 
> I only found the requirement at the bottom of page 35 of said PDF
> dealing with 64KiB pages.
> 
> Please, further indicate in which respect the current code violates the
> UEFI requirements.

I don't try to fix anything regarding the spec. As I said, I don't know
what specific section Alex was referring to in his original commit.

I guess it is better to give you an example. These are the relevant
outputs on my board using the original code:

[..]
__efi_runtime_start=fbd48210
__efi_runtime_end=fbd48b88
efi_add_memory_map: 0xfbd40000 0x10 5 no
[..]

Because of the 64k alignment, the whole region from 0xfbd40000 to
0xfbd50000 is added as EFI_RUNTIME_SERVICES_CODE.

Later, another region (that is the spin table) is added. But this
time as EFI_RESERVED_MEMORY_TYPE and the region overlaps the former.

[..]
efi_add_memory_map: 0xfbd49000 0x1 0 no
[..]

Which eventually leads to

[    0.067055] Remapping and enabling EFI services.
[    0.071719] UEFI virtual mapping missing or invalid -- runtime 
services will not be available

[on a side note, this is because the sort and merge of the memory
  region will split the EFI_RUNTIME_SERVICES_CODE into two regions,
  because there is now one EFI_RESERVED_MEMORY_TYPE region in between]


>>>> +???? *
>>>> +???? * This needs to come before *(.text*)
>>>> +???? */
>>>> +
>>>> +??? . = ALIGN(65536);
>>> 
>>> Isn't this an alignment before relocation that is irrelevant with
>>> regards to the UEFI spec?

Oh right. My intention was to align the relocated efi runtime section
to 64kb. So this doesn't work.

But to complete the example above, with my (broken) patch applied:

__efi_runtime_start=fbd48000
__efi_runtime_end=fbd48978
efi_add_memory_map: 0xfbd48000 0x1 5 no

Which works because it basically reverts the original commit
and just adds one 4k page to the memory map.

So if there is indeed no such requirement to align the runtime
services to 64kb reverting Alex commit works too.

-- 
Hope my problem is clearer now.
-michael

>>> 
>>>> ???? .efi_runtime : {
>>>> ???????????????? __efi_runtime_start = .;
>>>> ???????? *(.text.efi_runtime*)
>>>> diff --git a/lib/efi_loader/efi_memory.c 
>>>> b/lib/efi_loader/efi_memory.c
>>>> index 97d90f069a..fd79178da9 100644
>>>> --- a/lib/efi_loader/efi_memory.c
>>>> +++ b/lib/efi_loader/efi_memory.c
>>>> @@ -12,7 +12,6 @@
>>>> ?#include <mapmem.h>
>>>> ?#include <watchdog.h>
>>>> ?#include <linux/list_sort.h>
>>>> -#include <linux/sizes.h>
>>>> 
>>>> ?DECLARE_GLOBAL_DATA_PTR;
>>>> 
>>>> @@ -734,7 +733,6 @@ __weak void efi_add_known_memory(void)
>>>> ?static void add_u_boot_and_runtime(void)
>>>> ?{
>>>> ???? unsigned long runtime_start, runtime_end, runtime_pages;
>>>> -??? unsigned long runtime_mask = EFI_PAGE_MASK;
>>>> ???? unsigned long uboot_start, uboot_pages;
>>>> ???? unsigned long uboot_stack_size = 16 * 1024 * 1024;
>>>> 
>>>> @@ -745,22 +743,13 @@ static void add_u_boot_and_runtime(void)
>>>> ??????????????? uboot_start + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
>>>> ???? efi_add_memory_map(uboot_start, uboot_pages, EFI_LOADER_DATA,
>>>> false);
>>>> 
>>>> -#if defined(__aarch64__)
>>>> -??? /*
>>>> -???? * Runtime Services must be 64KiB aligned according to the
>>>> -???? * "AArch64 Platforms" section in the UEFI spec (2.7+).> -???? 
>>>> */
>>>> -
>>>> -??? runtime_mask = SZ_64K - 1;
>>>> -#endif
>>>> -
>>>> ???? /*
>>>> ????? * Add Runtime Services. We mark surrounding boottime code as
>>>> runtime as
>>>> ????? * well to fulfill the runtime alignment constraints but avoid
>>>> padding.
>>>> ????? */
>>>> -??? runtime_start = (ulong)&__efi_runtime_start & ~runtime_mask;
>>>> +??? runtime_start = (ulong)&__efi_runtime_start & ~EFI_PAGE_MASK;
>>>> ???? runtime_end = (ulong)&__efi_runtime_stop;
>>>> -??? runtime_end = (runtime_end + runtime_mask) & ~runtime_mask;
>>>> +??? runtime_end = (runtime_end + EFI_PAGE_MASK) & ~EFI_PAGE_MASK;
>>> 
>>> I cannot see that after these changes you match the requirements of 
>>> the
>>> UEFI spec.
>>> 
>>> Best regards
>>> 
>>> Heinrich
>>> 
>>>> ???? runtime_pages = (runtime_end - runtime_start) >> 
>>>> EFI_PAGE_SHIFT;
>>>> ???? efi_add_memory_map(runtime_start, runtime_pages,
>>>> ??????????????? EFI_RUNTIME_SERVICES_CODE, false);
>>>> 
>> 

[PATCH 1/4] efi_loader: aarch64: align runtime section to 64kb

[Heinrich Schuchardt]

On 5/15/20 12:02 AM, Michael Walle wrote:
> Am 2020-05-14 23:03, schrieb Heinrich Schuchardt:
>> On 5/14/20 9:04 PM, Michael Walle wrote:
>>> Am 2020-05-14 20:27, schrieb Heinrich Schuchardt:
>>>> On 5/14/20 2:38 PM, Michael Walle wrote:
>>>>> Commit 7a82c3051c8f ("efi_loader: Align runtime section to 64kb")
>>>>> already aligned the memory region to 64kb, but it does not align the
>>>>> actual efi runtime code. Thus it is likely, that efi_add_memory_map()
>>>>> actually adds a larger memory region than the efi runtime code really
>>>>> is, which is no error I guess. But what actually leads to an error is
>>>>> that there might be other efi_add_memory_map() calls with regions
>>>>> overlapping with the already registered efi runtime code section.
>>>>
>>>> Do you relate to this sentence:
>>>>
>>>> "If a 64KiB physical page contains any 4KiB page with any of the
>>>> following types listed below, then all 4KiB pages in the 64KiB page
>>>> must
>>>> use identical ARM Memory Page Attributes"?
>>>
>>> I don't think this is what I want to fix here.
>>>
>>> Commit 7a82c3051c8f ("efi_loader: Align runtime section to 64kb")
>>> reserves the memory region for runtime services and align the start
>>> (and size) to 64kb boundaries. But the actual runtime services are
>>> not 64kb aligned. And at least on my board, another memory region
>>> right next to it is reserved as well. But of course as another type.
>>>
>>>>>
>>>>> Align the actual runtime code to 64kb instead.
>>>>>
>>>>> Fixes: 7a82c3051c8f ("efi_loader: Align runtime section to 64kb")
>>>>> Signed-off-by: Michael Walle <michael@walle.cc>
>>>>> ---
>>>>> ?arch/arm/cpu/armv8/u-boot.lds |? 9 ++++++++-
>>>>> ?lib/efi_loader/efi_memory.c?? | 15 ++-------------
>>>>> ?2 files changed, 10 insertions(+), 14 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/cpu/armv8/u-boot.lds
>>>>> b/arch/arm/cpu/armv8/u-boot.lds
>>>>> index 2554980595..3bc4675586 100644
>>>>> --- a/arch/arm/cpu/armv8/u-boot.lds
>>>>> +++ b/arch/arm/cpu/armv8/u-boot.lds
>>>>> @@ -27,7 +27,14 @@ SECTIONS
>>>>> ???????? CPUDIR/start.o (.text*)
>>>>> ???? }
>>>>>
>>>>> -??? /* This needs to come before *(.text*) */
>>>>> +??? /*
>>>>> +???? * Runtime Services must be 64KiB aligned according to the
>>>>> +???? * "AArch64 Platforms" section in the UEFI spec (2.7+).
>>>>
>>>> This is not the exact requirement of the spec. Please, use a
>>>> description
>>>> that matches the spec.
>>>
>>> well I just moved this exact sentence. I'm not familiar with the UEFI
>>> spec.
>>>
>>>> The requirement that 64KiB areas should have the same attributes was
>>>> already presen in UEFI spec 2.4. Please, drop the version reference.
>>>
>>> As mentioned above, its about the alignment of the runtime section.
>>
>> Please, indicate the exact requirement in the "UEFI 2.8 errata A"
>> specification you are refering to. Cf.
>> file:///home/zfsdt/Documents/UEFI/UEFI_Spec_2_8_A_Feb14.pdf
>>
>> I only found the requirement at the bottom of page 35 of said PDF
>> dealing with 64KiB pages.
>>
>> Please, further indicate in which respect the current code violates the
>> UEFI requirements.
>
> I don't try to fix anything regarding the spec. As I said, I don't know
> what specific section Alex was referring to in his original commit.
>
> I guess it is better to give you an example. These are the relevant
> outputs on my board using the original code:
>
> [..]
> __efi_runtime_start=fbd48210
> __efi_runtime_end=fbd48b88
> efi_add_memory_map: 0xfbd40000 0x10 5 no
> [..]
>
> Because of the 64k alignment, the whole region from 0xfbd40000 to
> 0xfbd50000 is added as EFI_RUNTIME_SERVICES_CODE.
>
> Later, another region (that is the spin table) is added. But this
> time as EFI_RESERVED_MEMORY_TYPE and the region overlaps the former.

This sounds like a real bug.

Could you, please, indicate which function is adding that spin table and
how the address of the spin table is chosen.

Is this __spin_table in arch/arm/cpu/armv8/fsl-layerscape/lowlevel.S?

@Alex:
What is that efi_add_memory_map((uintptr_t)&secondary_boot_code,...)
call good for? Is that secondary boot code ever invoked *after*
ExitBootServices()?
See your patch 5a37a2f0140c ("armv8: ls2080a: Declare spin tables as
reserved for efi loader").

Best regards

Heinrich

>
> [..]
> efi_add_memory_map: 0xfbd49000 0x1 0 no
> [..]
>
> Which eventually leads to
>
> [??? 0.067055] Remapping and enabling EFI services.
> [??? 0.071719] UEFI virtual mapping missing or invalid -- runtime
> services will not be available
>
> [on a side note, this is because the sort and merge of the memory
> ?region will split the EFI_RUNTIME_SERVICES_CODE into two regions,
> ?because there is now one EFI_RESERVED_MEMORY_TYPE region in between]
>
>
>>>>> +???? *
>>>>> +???? * This needs to come before *(.text*)
>>>>> +???? */
>>>>> +
>>>>> +??? . = ALIGN(65536);
>>>>
>>>> Isn't this an alignment before relocation that is irrelevant with
>>>> regards to the UEFI spec?
>
> Oh right. My intention was to align the relocated efi runtime section
> to 64kb. So this doesn't work.
>
> But to complete the example above, with my (broken) patch applied:
>
> __efi_runtime_start=fbd48000
> __efi_runtime_end=fbd48978
> efi_add_memory_map: 0xfbd48000 0x1 5 no
>
> Which works because it basically reverts the original commit
> and just adds one 4k page to the memory map.
>
> So if there is indeed no such requirement to align the runtime
> services to 64kb reverting Alex commit works too.
>

[PATCH 1/4] efi_loader: aarch64: align runtime section to 64kb

[Heinrich Schuchardt]

On 5/15/20 12:27 AM, Heinrich Schuchardt wrote:
> On 5/15/20 12:02 AM, Michael Walle wrote:
>> Am 2020-05-14 23:03, schrieb Heinrich Schuchardt:
>>> On 5/14/20 9:04 PM, Michael Walle wrote:
>>>> Am 2020-05-14 20:27, schrieb Heinrich Schuchardt:
>>>>> On 5/14/20 2:38 PM, Michael Walle wrote:
>>>>>> Commit 7a82c3051c8f ("efi_loader: Align runtime section to 64kb")
>>>>>> already aligned the memory region to 64kb, but it does not align the
>>>>>> actual efi runtime code. Thus it is likely, that efi_add_memory_map()
>>>>>> actually adds a larger memory region than the efi runtime code really
>>>>>> is, which is no error I guess. But what actually leads to an error is
>>>>>> that there might be other efi_add_memory_map() calls with regions
>>>>>> overlapping with the already registered efi runtime code section.
>>>>>
>>>>> Do you relate to this sentence:
>>>>>
>>>>> "If a 64KiB physical page contains any 4KiB page with any of the
>>>>> following types listed below, then all 4KiB pages in the 64KiB page
>>>>> must
>>>>> use identical ARM Memory Page Attributes"?
>>>>
>>>> I don't think this is what I want to fix here.
>>>>
>>>> Commit 7a82c3051c8f ("efi_loader: Align runtime section to 64kb")
>>>> reserves the memory region for runtime services and align the start
>>>> (and size) to 64kb boundaries. But the actual runtime services are
>>>> not 64kb aligned. And at least on my board, another memory region
>>>> right next to it is reserved as well. But of course as another type.
>>>>
>>>>>>
>>>>>> Align the actual runtime code to 64kb instead.
>>>>>>
>>>>>> Fixes: 7a82c3051c8f ("efi_loader: Align runtime section to 64kb")
>>>>>> Signed-off-by: Michael Walle <michael@walle.cc>
>>>>>> ---
>>>>>> ?arch/arm/cpu/armv8/u-boot.lds |? 9 ++++++++-
>>>>>> ?lib/efi_loader/efi_memory.c?? | 15 ++-------------
>>>>>> ?2 files changed, 10 insertions(+), 14 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/arm/cpu/armv8/u-boot.lds
>>>>>> b/arch/arm/cpu/armv8/u-boot.lds
>>>>>> index 2554980595..3bc4675586 100644
>>>>>> --- a/arch/arm/cpu/armv8/u-boot.lds
>>>>>> +++ b/arch/arm/cpu/armv8/u-boot.lds
>>>>>> @@ -27,7 +27,14 @@ SECTIONS
>>>>>> ???????? CPUDIR/start.o (.text*)
>>>>>> ???? }
>>>>>>
>>>>>> -??? /* This needs to come before *(.text*) */
>>>>>> +??? /*
>>>>>> +???? * Runtime Services must be 64KiB aligned according to the
>>>>>> +???? * "AArch64 Platforms" section in the UEFI spec (2.7+).
>>>>>
>>>>> This is not the exact requirement of the spec. Please, use a
>>>>> description
>>>>> that matches the spec.
>>>>
>>>> well I just moved this exact sentence. I'm not familiar with the UEFI
>>>> spec.
>>>>
>>>>> The requirement that 64KiB areas should have the same attributes was
>>>>> already presen in UEFI spec 2.4. Please, drop the version reference.
>>>>
>>>> As mentioned above, its about the alignment of the runtime section.
>>>
>>> Please, indicate the exact requirement in the "UEFI 2.8 errata A"
>>> specification you are refering to. Cf.
>>> file:///home/zfsdt/Documents/UEFI/UEFI_Spec_2_8_A_Feb14.pdf
>>>
>>> I only found the requirement at the bottom of page 35 of said PDF
>>> dealing with 64KiB pages.
>>>
>>> Please, further indicate in which respect the current code violates the
>>> UEFI requirements.
>>
>> I don't try to fix anything regarding the spec. As I said, I don't know
>> what specific section Alex was referring to in his original commit.
>>
>> I guess it is better to give you an example. These are the relevant
>> outputs on my board using the original code:
>>
>> [..]
>> __efi_runtime_start=fbd48210
>> __efi_runtime_end=fbd48b88
>> efi_add_memory_map: 0xfbd40000 0x10 5 no
>> [..]
>>
>> Because of the 64k alignment, the whole region from 0xfbd40000 to
>> 0xfbd50000 is added as EFI_RUNTIME_SERVICES_CODE.
>>
>> Later, another region (that is the spin table) is added. But this
>> time as EFI_RESERVED_MEMORY_TYPE and the region overlaps the former.
>
> This sounds like a real bug.
>
> Could you, please, indicate which function is adding that spin table and
> how the address of the spin table is chosen.
>
> Is this __spin_table in arch/arm/cpu/armv8/fsl-layerscape/lowlevel.S?
>
> @Alex:
> What is that efi_add_memory_map((uintptr_t)&secondary_boot_code,...)
> call good for? Is that secondary boot code ever invoked *after*
> ExitBootServices()?
> See your patch 5a37a2f0140c ("armv8: ls2080a: Declare spin tables as
> reserved for efi loader").

In ft_fixup_cpu(), arch/arm/cpu/armv8/fsl-layerscape/fdt.c the address
of __spin_table is added to the device tree.

The use of spin tables is described in Documentation/arm64/booting.rst.

So what is required is that after relocation the runtime code is not in
the same 64KiB page as __spin_table.

arch/arm/cpu/armv8/fsl-layerscape/lowlevel.S seems to be inconsistent:

/* Using 64 bit alignment since the spin table is accessed as data */
????????.align 4
????????.global secondary_boot_code
????????/* Secondary Boot Code starts here */
secondary_boot_code:
????????.global __spin_table
__spin_table:
????????.space CONFIG_MAX_CPUS*SPIN_TABLE_ELEM_SIZE

????????.align 2
ENTRY(secondary_boot_func)

.align 4 does not sound like 64bit alignment.

Function secondary_boot_func() uses __spin_table. But does this memory
location really have to be the same as the reserved memory area that is
passed to the kernel in the device tree? Or could this be two different
memory areas?

Best regards

Heinrich
>>
>> [..]
>> efi_add_memory_map: 0xfbd49000 0x1 0 no
>> [..]
>>
>> Which eventually leads to
>>
>> [??? 0.067055] Remapping and enabling EFI services.
>> [??? 0.071719] UEFI virtual mapping missing or invalid -- runtime
>> services will not be available
>>
>> [on a side note, this is because the sort and merge of the memory
>> ?region will split the EFI_RUNTIME_SERVICES_CODE into two regions,
>> ?because there is now one EFI_RESERVED_MEMORY_TYPE region in between]
>>
>>
>>>>>> +???? *
>>>>>> +???? * This needs to come before *(.text*)
>>>>>> +???? */
>>>>>> +
>>>>>> +??? . = ALIGN(65536);
>>>>>
>>>>> Isn't this an alignment before relocation that is irrelevant with
>>>>> regards to the UEFI spec?
>>
>> Oh right. My intention was to align the relocated efi runtime section
>> to 64kb. So this doesn't work.
>>
>> But to complete the example above, with my (broken) patch applied:
>>
>> __efi_runtime_start=fbd48000
>> __efi_runtime_end=fbd48978
>> efi_add_memory_map: 0xfbd48000 0x1 5 no
>>
>> Which works because it basically reverts the original commit
>> and just adds one 4k page to the memory map.
>>
>> So if there is indeed no such requirement to align the runtime
>> services to 64kb reverting Alex commit works too.
>>
>

[PATCH 1/4] efi_loader: aarch64: align runtime section to 64kb

[Michael Walle]

Am 2020-05-15 01:04, schrieb Heinrich Schuchardt:
> On 5/15/20 12:27 AM, Heinrich Schuchardt wrote:
>> On 5/15/20 12:02 AM, Michael Walle wrote:
>>> Am 2020-05-14 23:03, schrieb Heinrich Schuchardt:
>>>> On 5/14/20 9:04 PM, Michael Walle wrote:
>>>>> Am 2020-05-14 20:27, schrieb Heinrich Schuchardt:
>>>>>> On 5/14/20 2:38 PM, Michael Walle wrote:
>>>>>>> Commit 7a82c3051c8f ("efi_loader: Align runtime section to 64kb")
>>>>>>> already aligned the memory region to 64kb, but it does not align 
>>>>>>> the
>>>>>>> actual efi runtime code. Thus it is likely, that 
>>>>>>> efi_add_memory_map()
>>>>>>> actually adds a larger memory region than the efi runtime code 
>>>>>>> really
>>>>>>> is, which is no error I guess. But what actually leads to an 
>>>>>>> error is
>>>>>>> that there might be other efi_add_memory_map() calls with regions
>>>>>>> overlapping with the already registered efi runtime code section.
>>>>>> 
>>>>>> Do you relate to this sentence:
>>>>>> 
>>>>>> "If a 64KiB physical page contains any 4KiB page with any of the
>>>>>> following types listed below, then all 4KiB pages in the 64KiB 
>>>>>> page
>>>>>> must
>>>>>> use identical ARM Memory Page Attributes"?
>>>>> 
>>>>> I don't think this is what I want to fix here.
>>>>> 
>>>>> Commit 7a82c3051c8f ("efi_loader: Align runtime section to 64kb")
>>>>> reserves the memory region for runtime services and align the start
>>>>> (and size) to 64kb boundaries. But the actual runtime services are
>>>>> not 64kb aligned. And at least on my board, another memory region
>>>>> right next to it is reserved as well. But of course as another 
>>>>> type.
>>>>> 
>>>>>>> 
>>>>>>> Align the actual runtime code to 64kb instead.
>>>>>>> 
>>>>>>> Fixes: 7a82c3051c8f ("efi_loader: Align runtime section to 64kb")
>>>>>>> Signed-off-by: Michael Walle <michael@walle.cc>
>>>>>>> ---
>>>>>>> ?arch/arm/cpu/armv8/u-boot.lds |? 9 ++++++++-
>>>>>>> ?lib/efi_loader/efi_memory.c?? | 15 ++-------------
>>>>>>> ?2 files changed, 10 insertions(+), 14 deletions(-)
>>>>>>> 
>>>>>>> diff --git a/arch/arm/cpu/armv8/u-boot.lds
>>>>>>> b/arch/arm/cpu/armv8/u-boot.lds
>>>>>>> index 2554980595..3bc4675586 100644
>>>>>>> --- a/arch/arm/cpu/armv8/u-boot.lds
>>>>>>> +++ b/arch/arm/cpu/armv8/u-boot.lds
>>>>>>> @@ -27,7 +27,14 @@ SECTIONS
>>>>>>> ???????? CPUDIR/start.o (.text*)
>>>>>>> ???? }
>>>>>>> 
>>>>>>> -??? /* This needs to come before *(.text*) */
>>>>>>> +??? /*
>>>>>>> +???? * Runtime Services must be 64KiB aligned according to the
>>>>>>> +???? * "AArch64 Platforms" section in the UEFI spec (2.7+).
>>>>>> 
>>>>>> This is not the exact requirement of the spec. Please, use a
>>>>>> description
>>>>>> that matches the spec.
>>>>> 
>>>>> well I just moved this exact sentence. I'm not familiar with the 
>>>>> UEFI
>>>>> spec.
>>>>> 
>>>>>> The requirement that 64KiB areas should have the same attributes 
>>>>>> was
>>>>>> already presen in UEFI spec 2.4. Please, drop the version 
>>>>>> reference.
>>>>> 
>>>>> As mentioned above, its about the alignment of the runtime section.
>>>> 
>>>> Please, indicate the exact requirement in the "UEFI 2.8 errata A"
>>>> specification you are refering to. Cf.
>>>> file:///home/zfsdt/Documents/UEFI/UEFI_Spec_2_8_A_Feb14.pdf
>>>> 
>>>> I only found the requirement at the bottom of page 35 of said PDF
>>>> dealing with 64KiB pages.
>>>> 
>>>> Please, further indicate in which respect the current code violates 
>>>> the
>>>> UEFI requirements.
>>> 
>>> I don't try to fix anything regarding the spec. As I said, I don't 
>>> know
>>> what specific section Alex was referring to in his original commit.
>>> 
>>> I guess it is better to give you an example. These are the relevant
>>> outputs on my board using the original code:
>>> 
>>> [..]
>>> __efi_runtime_start=fbd48210
>>> __efi_runtime_end=fbd48b88
>>> efi_add_memory_map: 0xfbd40000 0x10 5 no
>>> [..]
>>> 
>>> Because of the 64k alignment, the whole region from 0xfbd40000 to
>>> 0xfbd50000 is added as EFI_RUNTIME_SERVICES_CODE.
>>> 
>>> Later, another region (that is the spin table) is added. But this
>>> time as EFI_RESERVED_MEMORY_TYPE and the region overlaps the former.
>> 
>> This sounds like a real bug.
>> 
>> Could you, please, indicate which function is adding that spin table 
>> and
>> how the address of the spin table is chosen.
>> 
>> Is this __spin_table in arch/arm/cpu/armv8/fsl-layerscape/lowlevel.S?

yes

>> @Alex:
>> What is that efi_add_memory_map((uintptr_t)&secondary_boot_code,...)
>> call good for? Is that secondary boot code ever invoked *after*
>> ExitBootServices()?
>> See your patch 5a37a2f0140c ("armv8: ls2080a: Declare spin tables as
>> reserved for efi loader").
> 
> In ft_fixup_cpu(), arch/arm/cpu/armv8/fsl-layerscape/fdt.c the address
> of __spin_table is added to the device tree.
> 
> The use of spin tables is described in Documentation/arm64/booting.rst.
> 
> So what is required is that after relocation the runtime code is not in
> the same 64KiB page as __spin_table.

Yes, but I guess its __spin_table as well as secondary_boot_code. But I
still fail to understand how fixing that will eventually fixing the
underlying issue, that is: the reserved runtime code memory region will
be a 64kb page, but the actual runtime code in u-boot will (likely) fit
in one 4k page. Which opens the door for code in u-boot to do
efi_add_memory_map() on 4k pages within the range of the 64kb runtime
code range. I.e. what happened here.

> 
> arch/arm/cpu/armv8/fsl-layerscape/lowlevel.S seems to be inconsistent:
> 
> /* Using 64 bit alignment since the spin table is accessed as data */
> ????????.align 4
> ????????.global secondary_boot_code
> ????????/* Secondary Boot Code starts here */
> secondary_boot_code:
> ????????.global __spin_table
> __spin_table:
> ????????.space CONFIG_MAX_CPUS*SPIN_TABLE_ELEM_SIZE
> 
> ????????.align 2
> ENTRY(secondary_boot_func)
> 
> .align 4 does not sound like 64bit alignment.
> 
> Function secondary_boot_func() uses __spin_table. But does this memory
> location really have to be the same as the reserved memory area that is
> passed to the kernel in the device tree? Or could this be two different
> memory areas?

What would be different if they are in two different areas? The 
secondary
cores are spinning in secondary_boot_func and poll the __spin_table,
which means both need to be EFI_RESERVED_MEMORY_TYPE.

-michael

> 
> Best regards
> 
> Heinrich
>>> 
>>> [..]
>>> efi_add_memory_map: 0xfbd49000 0x1 0 no
>>> [..]
>>> 
>>> Which eventually leads to
>>> 
>>> [??? 0.067055] Remapping and enabling EFI services.
>>> [??? 0.071719] UEFI virtual mapping missing or invalid -- runtime
>>> services will not be available
>>> 
>>> [on a side note, this is because the sort and merge of the memory
>>> ?region will split the EFI_RUNTIME_SERVICES_CODE into two regions,
>>> ?because there is now one EFI_RESERVED_MEMORY_TYPE region in between]
>>> 
>>> 
>>>>>>> +???? *
>>>>>>> +???? * This needs to come before *(.text*)
>>>>>>> +???? */
>>>>>>> +
>>>>>>> +??? . = ALIGN(65536);
>>>>>> 
>>>>>> Isn't this an alignment before relocation that is irrelevant with
>>>>>> regards to the UEFI spec?
>>> 
>>> Oh right. My intention was to align the relocated efi runtime section
>>> to 64kb. So this doesn't work.
>>> 
>>> But to complete the example above, with my (broken) patch applied:
>>> 
>>> __efi_runtime_start=fbd48000
>>> __efi_runtime_end=fbd48978
>>> efi_add_memory_map: 0xfbd48000 0x1 5 no
>>> 
>>> Which works because it basically reverts the original commit
>>> and just adds one 4k page to the memory map.
>>> 
>>> So if there is indeed no such requirement to align the runtime
>>> services to 64kb reverting Alex commit works too.
>>> 
>> 

[PATCH 1/4] efi_loader: aarch64: align runtime section to 64kb

[Heinrich Schuchardt]

On 5/14/20 8:27 PM, Heinrich Schuchardt wrote:
> On 5/14/20 2:38 PM, Michael Walle wrote:
>> Commit 7a82c3051c8f ("efi_loader: Align runtime section to 64kb")
>> already aligned the memory region to 64kb, but it does not align the
>> actual efi runtime code. Thus it is likely, that efi_add_memory_map()
>> actually adds a larger memory region than the efi runtime code really
>> is, which is no error I guess. But what actually leads to an error is
>> that there might be other efi_add_memory_map() calls with regions
>> overlapping with the already registered efi runtime code section.
>
> Do you relate to this sentence:
>
> "If a 64KiB physical page contains any 4KiB page with any of the
> following types listed below, then all 4KiB pages in the 64KiB page must
> use identical ARM Memory Page Attributes"?
>
>
>>
>> Align the actual runtime code to 64kb instead.
>>
>> Fixes: 7a82c3051c8f ("efi_loader: Align runtime section to 64kb")
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>>  arch/arm/cpu/armv8/u-boot.lds |  9 ++++++++-
>>  lib/efi_loader/efi_memory.c   | 15 ++-------------
>>  2 files changed, 10 insertions(+), 14 deletions(-)
>>
>> diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
>> index 2554980595..3bc4675586 100644
>> --- a/arch/arm/cpu/armv8/u-boot.lds
>> +++ b/arch/arm/cpu/armv8/u-boot.lds
>> @@ -27,7 +27,14 @@ SECTIONS
>>  		CPUDIR/start.o (.text*)
>>  	}
>>
>> -	/* This needs to come before *(.text*) */
>> +	/*
>> +	 * Runtime Services must be 64KiB aligned according to the
>> +	 * "AArch64 Platforms" section in the UEFI spec (2.7+).
>
> This is not the exact requirement of the spec. Please, use a description
> that matches the spec.
>
> The requirement that 64KiB areas should have the same attributes was
> already presen in UEFI spec 2.4. Please, drop the version reference.
>
>> +	 *
>> +	 * This needs to come before *(.text*)
>> +	 */
>> +
>> +	. = ALIGN(65536);
>
> Isn't this an alignment before relocation that is irrelevant with
> regards to the UEFI spec?
>
>>  	.efi_runtime : {
>>                  __efi_runtime_start = .;
>>  		*(.text.efi_runtime*)
>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
>> index 97d90f069a..fd79178da9 100644
>> --- a/lib/efi_loader/efi_memory.c
>> +++ b/lib/efi_loader/efi_memory.c
>> @@ -12,7 +12,6 @@
>>  #include <mapmem.h>
>>  #include <watchdog.h>
>>  #include <linux/list_sort.h>
>> -#include <linux/sizes.h>
>>
>>  DECLARE_GLOBAL_DATA_PTR;
>>
>> @@ -734,7 +733,6 @@ __weak void efi_add_known_memory(void)
>>  static void add_u_boot_and_runtime(void)
>>  {
>>  	unsigned long runtime_start, runtime_end, runtime_pages;
>> -	unsigned long runtime_mask = EFI_PAGE_MASK;
>>  	unsigned long uboot_start, uboot_pages;
>>  	unsigned long uboot_stack_size = 16 * 1024 * 1024;
>>
>> @@ -745,22 +743,13 @@ static void add_u_boot_and_runtime(void)
>>  		       uboot_start + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
>>  	efi_add_memory_map(uboot_start, uboot_pages, EFI_LOADER_DATA, false);
>>
>> -#if defined(__aarch64__)
>> -	/*
>> -	 * Runtime Services must be 64KiB aligned according to the
>> -	 * "AArch64 Platforms" section in the UEFI spec (2.7+).> -	 */
>> -
>> -	runtime_mask = SZ_64K - 1;
>> -#endif
>> -
>>  	/*
>>  	 * Add Runtime Services. We mark surrounding boottime code as runtime as
>>  	 * well to fulfill the runtime alignment constraints but avoid padding.
>>  	 */
>> -	runtime_start = (ulong)&__efi_runtime_start & ~runtime_mask;
>> +	runtime_start = (ulong)&__efi_runtime_start & ~EFI_PAGE_MASK;
>>  	runtime_end = (ulong)&__efi_runtime_stop;
>> -	runtime_end = (runtime_end + runtime_mask) & ~runtime_mask;
>> +	runtime_end = (runtime_end + EFI_PAGE_MASK) & ~EFI_PAGE_MASK;
>
> I cannot see that after these changes you match the requirements of the
> UEFI spec.
>
> Best regards
>
> Heinrich
>
>>  	runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT;
>>  	efi_add_memory_map(runtime_start, runtime_pages,
>>  			   EFI_RUNTIME_SERVICES_CODE, false);
>>
>

Hello Michael,

your described that on ARMv8 the UEFI runtime must be in a 64k page that
is has to be separate from the 4k page reserved for spin-tables for
certain boards. A change that would achieve this is shown as diff below.
But it has a major impact on image size:

qemu_arm64_defconfig before:
6276656 May 28 18:53 u-boot
 673904 May 28 18:53 u-boot.bin

after:
6338240 May 28 18:51 u-boot
 735368 May 28 18:51 u-boot.bin

Image size is critical on many boards therefore this seems not to be a
good way to go forward.

U-Boot's PSCI has a similar problem to solve. Here the code has been put
into section _secure.text and the data in_secure.data. Function
relocate_secure_section() is used to move the code to the address
specified by CONFIG_ARMV8_SECURE_BASE. I think the same should work for
the spin-tables.

Could you, please, evaluate this idea.

Here is the diff for the solution I would not like to implement:

diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
index 2554980595..e1ba450937 100644
--- a/arch/arm/cpu/armv8/u-boot.lds
+++ b/arch/arm/cpu/armv8/u-boot.lds
@@ -15,6 +15,7 @@ OUTPUT_ARCH(aarch64)
 ENTRY(_start)
 SECTIONS
 {
+       . = ALIGN(65536);
 #ifdef CONFIG_ARMV8_SECURE_BASE
        /DISCARD/ : { *(.rela._secure*) }
 #endif
@@ -36,6 +37,7 @@ SECTIONS
                 __efi_runtime_stop = .;
        }

+       . = ALIGN(65536);
        .text_rest :
        {
                *(.text*)
diff --git a/common/board_f.c b/common/board_f.c
index 01194eaa0e..42d7fdff12 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -438,7 +438,8 @@ static int reserve_uboot(void)
                 */
                gd->relocaddr -= gd->mon_len;
                gd->relocaddr &= ~(4096 - 1);
-       #if defined(CONFIG_E500) || defined(CONFIG_MIPS)
+       #if defined(CONFIG_E500) || defined(CONFIG_MIPS) || \
+           (defined(CONFIG_ARM64) && defined(CONFIG_EFI_LOADER))
                /* round down to next 64 kB limit so that IVPR stays
aligned */
                gd->relocaddr &= ~(65536 - 1);
        #endif

Best regards

Heinrich

[PATCH 2/4] efi_loader: check alignment in efi_add_memory_map()

[Michael Walle]

The first argument has to be aligned with EFI_PAGE_SIZE. This alignment
is already checked for external callers but it is not checked for
internal callers. Unfortunately, most of the time the return value is
not checked, so scream loud and clear.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 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 fd79178da9..b56e19cb30 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -248,6 +248,9 @@ efi_status_t efi_add_memory_map(uint64_t start, uint64_t pages, int memory_type,
 	EFI_PRINT("%s: 0x%llx 0x%llx %d %s\n", __func__,
 		  start, pages, memory_type, overlap_only_ram ? "yes" : "no");
 
+	if (start & EFI_PAGE_MASK)
+		panic("%s: start not aligned\n", __func__);
+
 	if (memory_type >= EFI_MAX_MEMORY_TYPE)
 		return EFI_INVALID_PARAMETER;
 
-- 
2.20.1

[PATCH 2/4] efi_loader: check alignment in efi_add_memory_map()

[Heinrich Schuchardt]

On 5/14/20 2:38 PM, Michael Walle wrote:
> The first argument has to be aligned with EFI_PAGE_SIZE. This alignment
> is already checked for external callers but it is not checked for
> internal callers. Unfortunately, most of the time the return value is
> not checked, so scream loud and clear.

Why do you mention the return value here?

>
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  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 fd79178da9..b56e19cb30 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -248,6 +248,9 @@ efi_status_t efi_add_memory_map(uint64_t start, uint64_t pages, int memory_type,
>  	EFI_PRINT("%s: 0x%llx 0x%llx %d %s\n", __func__,
>  		  start, pages, memory_type, overlap_only_ram ? "yes" : "no");
>
> +	if (start & EFI_PAGE_MASK)
> +		panic("%s: start not aligned\n", __func__);
> +

Did you find any internal caller that has a problem?
We do not want to increase code size.

Best regards

Heinrich

>  	if (memory_type >= EFI_MAX_MEMORY_TYPE)
>  		return EFI_INVALID_PARAMETER;
>
>

[PATCH 2/4] efi_loader: check alignment in efi_add_memory_map()

[Michael Walle]

Am 2020-05-14 20:35, schrieb Heinrich Schuchardt:
> On 5/14/20 2:38 PM, Michael Walle wrote:
>> The first argument has to be aligned with EFI_PAGE_SIZE. This 
>> alignment
>> is already checked for external callers but it is not checked for
>> internal callers. Unfortunately, most of the time the return value is
>> not checked, so scream loud and clear.
> 
> Why do you mention the return value here?

most callers just ignore the return value. so if not aligned this will
silently fail.

>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>>  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 fd79178da9..b56e19cb30 100644
>> --- a/lib/efi_loader/efi_memory.c
>> +++ b/lib/efi_loader/efi_memory.c
>> @@ -248,6 +248,9 @@ efi_status_t efi_add_memory_map(uint64_t start, 
>> uint64_t pages, int memory_type,
>>  	EFI_PRINT("%s: 0x%llx 0x%llx %d %s\n", __func__,
>>  		  start, pages, memory_type, overlap_only_ram ? "yes" : "no");
>> 
>> +	if (start & EFI_PAGE_MASK)
>> +		panic("%s: start not aligned\n", __func__);
>> +
> 
> Did you find any internal caller that has a problem?
See next patch.

> We do not want to increase code size.
Mh, even within the efi_loader? Well I could do a

if (start & EFI_PAGE_MASK) {
     debug("%s: start not aligned\n", __func__);
     return EFI_INVALID_PARAMETER;
}

but as I said, nobody checks the return value.


-- 
-michael

[PATCH 2/4] efi_loader: check alignment in efi_add_memory_map()

[Heinrich Schuchardt]

On 5/14/20 8:50 PM, Michael Walle wrote:
> Am 2020-05-14 20:35, schrieb Heinrich Schuchardt:
>> On 5/14/20 2:38 PM, Michael Walle wrote:
>>> The first argument has to be aligned with EFI_PAGE_SIZE. This alignment
>>> is already checked for external callers but it is not checked for
>>> internal callers. Unfortunately, most of the time the return value is
>>> not checked, so scream loud and clear.
>>
>> Why do you mention the return value here?
>
> most callers just ignore the return value. so if not aligned this will
> silently fail.
>
>>>
>>> Signed-off-by: Michael Walle <michael@walle.cc>
>>> ---
>>> ?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 fd79178da9..b56e19cb30 100644
>>> --- a/lib/efi_loader/efi_memory.c
>>> +++ b/lib/efi_loader/efi_memory.c
>>> @@ -248,6 +248,9 @@ efi_status_t efi_add_memory_map(uint64_t start,
>>> uint64_t pages, int memory_type,
>>> ???? EFI_PRINT("%s: 0x%llx 0x%llx %d %s\n", __func__,
>>> ?????????? start, pages, memory_type, overlap_only_ram ? "yes" : "no");
>>>
>>> +??? if (start & EFI_PAGE_MASK)
>>> +??????? panic("%s: start not aligned\n", __func__);
>>> +
>>
>> Did you find any internal caller that has a problem?
> See next patch.
>
>> We do not want to increase code size.
> Mh, even within the efi_loader? Well I could do a
>
> if (start & EFI_PAGE_MASK) {
> ??? debug("%s: start not aligned\n", __func__);
> ??? return EFI_INVALID_PARAMETER;
> }
>
> but as I said, nobody checks the return value.

The Linux kernel has this nice code comment:
"Don't use BUG() or BUG_ON() unless there's really no way out;"

Looking at the different internal callers, some of these actively ensure
alignment others don't. Wouldn't it make more sense to move all rounding
into a common function taking a start address and a size in bytes as
arguments.

Best regards

Heinrich

>
>

[PATCH 3/4] fsl-layerscape: align first parameter of efi_add_memory_map()

[Michael Walle]

The start parameter must be aligned to EFI_PAGE_SIZE.

Fixes: 5a37a2f0140c ("armv8: ls2080a: Declare spin tables as reserved for efi loader")
Signed-off-by: Michael Walle <michael@walle.cc>
---
 arch/arm/cpu/armv8/fsl-layerscape/fdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/cpu/armv8/fsl-layerscape/fdt.c b/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
index 3bbad827cb..fc65ad6c1e 100644
--- a/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
+++ b/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
@@ -146,7 +146,7 @@ remove_psci_node:
 	fdt_add_mem_rsv(blob, (uintptr_t)&secondary_boot_code,
 			*boot_code_size);
 #if CONFIG_IS_ENABLED(EFI_LOADER)
-	efi_add_memory_map((uintptr_t)&secondary_boot_code,
+	efi_add_memory_map(ALIGN_DOWN((uintptr_t)&secondary_boot_code, EFI_PAGE_SIZE),
 			   ALIGN(*boot_code_size, EFI_PAGE_SIZE) >> EFI_PAGE_SHIFT,
 			   EFI_RESERVED_MEMORY_TYPE, false);
 #endif
-- 
2.20.1

[PATCH 3/4] fsl-layerscape: align first parameter of efi_add_memory_map()

[Heinrich Schuchardt]

On 5/14/20 2:38 PM, Michael Walle wrote:
> The start parameter must be aligned to EFI_PAGE_SIZE.
>
> Fixes: 5a37a2f0140c ("armv8: ls2080a: Declare spin tables as reserved for efi loader")
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  arch/arm/cpu/armv8/fsl-layerscape/fdt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/cpu/armv8/fsl-layerscape/fdt.c b/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
> index 3bbad827cb..fc65ad6c1e 100644
> --- a/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
> +++ b/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
> @@ -146,7 +146,7 @@ remove_psci_node:
>  	fdt_add_mem_rsv(blob, (uintptr_t)&secondary_boot_code,
>  			*boot_code_size);
>  #if CONFIG_IS_ENABLED(EFI_LOADER)
> -	efi_add_memory_map((uintptr_t)&secondary_boot_code,
> +	efi_add_memory_map(ALIGN_DOWN((uintptr_t)&secondary_boot_code, EFI_PAGE_SIZE),
>  			   ALIGN(*boot_code_size, EFI_PAGE_SIZE) >> EFI_PAGE_SHIFT,

If secondary_boot_code is not EFI_PAGE_SIZE aligned, your code gets the
number of pages wrong.

Your code exceeds 80 characters per line.

Best regards

Heinrich

>  			   EFI_RESERVED_MEMORY_TYPE, false);
>  #endif
>

[PATCH 4/4] efi_loader: call smp_kick_all_cpus()

[Michael Walle]

On some architectures, specifically the layerscape, the secondary cores
wait for an interrupt before entering the spin-tables. This applies only
to boards which doesn't have PSCI provided by TF-a and u-boot does the
secondary cores handling.
bootm/booti already call that function for ARM architecture; also add it
to bootelf before switching to EL2. Additionally, provide a weak noop
function so we don't have to have "#ifdef CONFIG_ARM64" guards.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 common/bootm.c             | 9 +++++++++
 lib/efi_loader/efi_setup.c | 6 ++++++
 2 files changed, 15 insertions(+)

diff --git a/common/bootm.c b/common/bootm.c
index db4362a643..65adf29329 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -816,6 +816,15 @@ void __weak switch_to_non_secure_mode(void)
 {
 }
 
+/**
+ * smp_kick_all_cpus() - kick all CPUs
+ *
+ * This routine is overridden by architectures requiring this feature.
+ */
+void __weak smp_kick_all_cpus(void)
+{
+}
+
 #else /* USE_HOSTCC */
 
 #if defined(CONFIG_FIT_SIGNATURE)
diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
index 26a7423203..7e5364adc5 100644
--- a/lib/efi_loader/efi_setup.c
+++ b/lib/efi_loader/efi_setup.c
@@ -132,6 +132,12 @@ efi_status_t efi_init_obj_list(void)
 	/* Allow unaligned memory access */
 	allow_unaligned();
 
+	/*
+	 * Some architectures need to kick secondary cores to enter their
+	 * spin table.
+	 */
+	smp_kick_all_cpus();
+
 	/* On ARM switch from EL3 or secure mode to EL2 or non-secure mode */
 	switch_to_non_secure_mode();
 
-- 
2.20.1

[PATCH 4/4] efi_loader: call smp_kick_all_cpus()

[Heinrich Schuchardt]

On 5/14/20 2:38 PM, Michael Walle wrote:
> On some architectures, specifically the layerscape, the secondary cores
> wait for an interrupt before entering the spin-tables. This applies only
> to boards which doesn't have PSCI provided by TF-a and u-boot does the

%s/TF-a/TF-A/, %s/u-boot/U-Boot/

> secondary cores handling.
> bootm/booti already call that function for ARM architecture; also add it
> to bootelf before switching to EL2. Additionally, provide a weak noop
> function so we don't have to have "#ifdef CONFIG_ARM64" guards.
>
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  common/bootm.c             | 9 +++++++++
>  lib/efi_loader/efi_setup.c | 6 ++++++
>  2 files changed, 15 insertions(+)
>
> diff --git a/common/bootm.c b/common/bootm.c
> index db4362a643..65adf29329 100644
> --- a/common/bootm.c
> +++ b/common/bootm.c
> @@ -816,6 +816,15 @@ void __weak switch_to_non_secure_mode(void)
>  {
>  }
>
> +/**
> + * smp_kick_all_cpus() - kick all CPUs
> + *
> + * This routine is overridden by architectures requiring this feature.
> + */
> +void __weak smp_kick_all_cpus(void)
> +{
> +}
> +
>  #else /* USE_HOSTCC */
>
>  #if defined(CONFIG_FIT_SIGNATURE)
> diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> index 26a7423203..7e5364adc5 100644
> --- a/lib/efi_loader/efi_setup.c
> +++ b/lib/efi_loader/efi_setup.c
> @@ -132,6 +132,12 @@ efi_status_t efi_init_obj_list(void)
>  	/* Allow unaligned memory access */
>  	allow_unaligned();
>
> +	/*
> +	 * Some architectures need to kick secondary cores to enter their
> +	 * spin table.
> +	 */
> +	smp_kick_all_cpus();

This will not compile with

CONFIG_CMD_BOOTI=n
CONFIG_CMD_BOOTM=n
CONFIG_CMD_BOOTZ=n

Best regards

Heinrich

> +
>  	/* On ARM switch from EL3 or secure mode to EL2 or non-secure mode */
>  	switch_to_non_secure_mode();
>
>

[PATCH 4/4] efi_loader: call smp_kick_all_cpus()

[Alexander Graf]

On 14.05.20 20:46, Heinrich Schuchardt wrote:
> On 5/14/20 2:38 PM, Michael Walle wrote:
>> On some architectures, specifically the layerscape, the secondary cores
>> wait for an interrupt before entering the spin-tables. This applies only
>> to boards which doesn't have PSCI provided by TF-a and u-boot does the
> %s/TF-a/TF-A/, %s/u-boot/U-Boot/
>
>> secondary cores handling.
>> bootm/booti already call that function for ARM architecture; also add it
>> to bootelf before switching to EL2. Additionally, provide a weak noop
>> function so we don't have to have "#ifdef CONFIG_ARM64" guards.
>>
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>>   common/bootm.c             | 9 +++++++++
>>   lib/efi_loader/efi_setup.c | 6 ++++++
>>   2 files changed, 15 insertions(+)
>>
>> diff --git a/common/bootm.c b/common/bootm.c
>> index db4362a643..65adf29329 100644
>> --- a/common/bootm.c
>> +++ b/common/bootm.c
>> @@ -816,6 +816,15 @@ void __weak switch_to_non_secure_mode(void)
>>   {
>>   }
>>
>> +/**
>> + * smp_kick_all_cpus() - kick all CPUs
>> + *
>> + * This routine is overridden by architectures requiring this feature.
>> + */
>> +void __weak smp_kick_all_cpus(void)
>> +{
>> +}
>> +
>>   #else /* USE_HOSTCC */
>>
>>   #if defined(CONFIG_FIT_SIGNATURE)
>> diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
>> index 26a7423203..7e5364adc5 100644
>> --- a/lib/efi_loader/efi_setup.c
>> +++ b/lib/efi_loader/efi_setup.c
>> @@ -132,6 +132,12 @@ efi_status_t efi_init_obj_list(void)
>>   	/* Allow unaligned memory access */
>>   	allow_unaligned();
>>
>> +	/*
>> +	 * Some architectures need to kick secondary cores to enter their
>> +	 * spin table.
>> +	 */
>> +	smp_kick_all_cpus();
> This will not compile with
>
> CONFIG_CMD_BOOTI=n
> CONFIG_CMD_BOOTM=n
> CONFIG_CMD_BOOTZ=n


Much worse is that in incurs needless overhead on PSCI capable 
platforms. Can we move the smp_kick_all_cpus() to the board or soc level 
of the few systems that use spin tables please? :)

Alex

[PATCH 4/4] efi_loader: call smp_kick_all_cpus()

[Michael Walle]

Am 2020-05-14 22:17, schrieb Alexander Graf:
> On 14.05.20 20:46, Heinrich Schuchardt wrote:
>> On 5/14/20 2:38 PM, Michael Walle wrote:
>>> On some architectures, specifically the layerscape, the secondary 
>>> cores
>>> wait for an interrupt before entering the spin-tables. This applies 
>>> only
>>> to boards which doesn't have PSCI provided by TF-a and u-boot does 
>>> the
>> %s/TF-a/TF-A/, %s/u-boot/U-Boot/
>> 
>>> secondary cores handling.
>>> bootm/booti already call that function for ARM architecture; also add 
>>> it
>>> to bootelf before switching to EL2. Additionally, provide a weak noop
>>> function so we don't have to have "#ifdef CONFIG_ARM64" guards.
>>> 
>>> Signed-off-by: Michael Walle <michael@walle.cc>
>>> ---
>>>   common/bootm.c             | 9 +++++++++
>>>   lib/efi_loader/efi_setup.c | 6 ++++++
>>>   2 files changed, 15 insertions(+)
>>> 
>>> diff --git a/common/bootm.c b/common/bootm.c
>>> index db4362a643..65adf29329 100644
>>> --- a/common/bootm.c
>>> +++ b/common/bootm.c
>>> @@ -816,6 +816,15 @@ void __weak switch_to_non_secure_mode(void)
>>>   {
>>>   }
>>> 
>>> +/**
>>> + * smp_kick_all_cpus() - kick all CPUs
>>> + *
>>> + * This routine is overridden by architectures requiring this 
>>> feature.
>>> + */
>>> +void __weak smp_kick_all_cpus(void)
>>> +{
>>> +}
>>> +
>>>   #else /* USE_HOSTCC */
>>> 
>>>   #if defined(CONFIG_FIT_SIGNATURE)
>>> diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
>>> index 26a7423203..7e5364adc5 100644
>>> --- a/lib/efi_loader/efi_setup.c
>>> +++ b/lib/efi_loader/efi_setup.c
>>> @@ -132,6 +132,12 @@ efi_status_t efi_init_obj_list(void)
>>>   	/* Allow unaligned memory access */
>>>   	allow_unaligned();
>>> 
>>> +	/*
>>> +	 * Some architectures need to kick secondary cores to enter their
>>> +	 * spin table.
>>> +	 */
>>> +	smp_kick_all_cpus();
>> This will not compile with
>> 
>> CONFIG_CMD_BOOTI=n
>> CONFIG_CMD_BOOTM=n
>> CONFIG_CMD_BOOTZ=n
> 
> 
> Much worse is that in incurs needless overhead on PSCI capable
> platforms. Can we move the smp_kick_all_cpus() to the board or soc
> level of the few systems that use spin tables please? :)

By having a weak function called here instead of smp_kick_all_cpus()?

-- 
-michael

[PATCH 4/4] efi_loader: call smp_kick_all_cpus()

[Michael Walle]

[Also adding Tom Rini as ARM maintainer]

Am 2020-05-14 22:17, schrieb Alexander Graf:
> On 14.05.20 20:46, Heinrich Schuchardt wrote:
>> On 5/14/20 2:38 PM, Michael Walle wrote:
>>> On some architectures, specifically the layerscape, the secondary 
>>> cores
>>> wait for an interrupt before entering the spin-tables. This applies 
>>> only
>>> to boards which doesn't have PSCI provided by TF-a and u-boot does 
>>> the
>> %s/TF-a/TF-A/, %s/u-boot/U-Boot/
>> 
>>> secondary cores handling.
>>> bootm/booti already call that function for ARM architecture; also add 
>>> it
>>> to bootelf before switching to EL2. Additionally, provide a weak noop
>>> function so we don't have to have "#ifdef CONFIG_ARM64" guards.
>>> 
>>> Signed-off-by: Michael Walle <michael@walle.cc>
>>> ---
>>>   common/bootm.c             | 9 +++++++++
>>>   lib/efi_loader/efi_setup.c | 6 ++++++
>>>   2 files changed, 15 insertions(+)
>>> 
>>> diff --git a/common/bootm.c b/common/bootm.c
>>> index db4362a643..65adf29329 100644
>>> --- a/common/bootm.c
>>> +++ b/common/bootm.c
>>> @@ -816,6 +816,15 @@ void __weak switch_to_non_secure_mode(void)
>>>   {
>>>   }
>>> 
>>> +/**
>>> + * smp_kick_all_cpus() - kick all CPUs
>>> + *
>>> + * This routine is overridden by architectures requiring this 
>>> feature.
>>> + */
>>> +void __weak smp_kick_all_cpus(void)
>>> +{
>>> +}
>>> +
>>>   #else /* USE_HOSTCC */
>>> 
>>>   #if defined(CONFIG_FIT_SIGNATURE)
>>> diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
>>> index 26a7423203..7e5364adc5 100644
>>> --- a/lib/efi_loader/efi_setup.c
>>> +++ b/lib/efi_loader/efi_setup.c
>>> @@ -132,6 +132,12 @@ efi_status_t efi_init_obj_list(void)
>>>   	/* Allow unaligned memory access */
>>>   	allow_unaligned();
>>> 
>>> +	/*
>>> +	 * Some architectures need to kick secondary cores to enter their
>>> +	 * spin table.
>>> +	 */
>>> +	smp_kick_all_cpus();
>> This will not compile with
>> 
>> CONFIG_CMD_BOOTI=n
>> CONFIG_CMD_BOOTM=n
>> CONFIG_CMD_BOOTZ=n
> 
> 
> Much worse is that in incurs needless overhead on PSCI capable
> platforms. Can we move the smp_kick_all_cpus() to the board or soc
> level of the few systems that use spin tables please? :)

Ok, I'm not sure where I should put the smp_kick_all_cpus(). In the
bootm/booti path this is done in boot_jump_linux() (unconditionally
in do_nonsec_virt_switch()). Therefore, shouldn't the kick be in
switch_to_non_secure_mode(), too?

Regarding the overhead, isn't it just called once before starting
the OS? Is that really worth adding yet another weak function or
ifdef guards? Isn't it better to behave like the bootm case?

-- 
-michael

[PATCH 4/4] efi_loader: call smp_kick_all_cpus()

[Tom Rini]

On Sat, May 16, 2020 at 05:54:43PM +0200, Michael Walle wrote:
> [Also adding Tom Rini as ARM maintainer]
> 
> Am 2020-05-14 22:17, schrieb Alexander Graf:
> > On 14.05.20 20:46, Heinrich Schuchardt wrote:
> > > On 5/14/20 2:38 PM, Michael Walle wrote:
> > > > On some architectures, specifically the layerscape, the
> > > > secondary cores
> > > > wait for an interrupt before entering the spin-tables. This
> > > > applies only
> > > > to boards which doesn't have PSCI provided by TF-a and u-boot
> > > > does the
> > > %s/TF-a/TF-A/, %s/u-boot/U-Boot/
> > > 
> > > > secondary cores handling.
> > > > bootm/booti already call that function for ARM architecture;
> > > > also add it
> > > > to bootelf before switching to EL2. Additionally, provide a weak noop
> > > > function so we don't have to have "#ifdef CONFIG_ARM64" guards.
> > > > 
> > > > Signed-off-by: Michael Walle <michael@walle.cc>
> > > > ---
> > > >   common/bootm.c             | 9 +++++++++
> > > >   lib/efi_loader/efi_setup.c | 6 ++++++
> > > >   2 files changed, 15 insertions(+)
> > > > 
> > > > diff --git a/common/bootm.c b/common/bootm.c
> > > > index db4362a643..65adf29329 100644
> > > > --- a/common/bootm.c
> > > > +++ b/common/bootm.c
> > > > @@ -816,6 +816,15 @@ void __weak switch_to_non_secure_mode(void)
> > > >   {
> > > >   }
> > > > 
> > > > +/**
> > > > + * smp_kick_all_cpus() - kick all CPUs
> > > > + *
> > > > + * This routine is overridden by architectures requiring this
> > > > feature.
> > > > + */
> > > > +void __weak smp_kick_all_cpus(void)
> > > > +{
> > > > +}
> > > > +
> > > >   #else /* USE_HOSTCC */
> > > > 
> > > >   #if defined(CONFIG_FIT_SIGNATURE)
> > > > diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> > > > index 26a7423203..7e5364adc5 100644
> > > > --- a/lib/efi_loader/efi_setup.c
> > > > +++ b/lib/efi_loader/efi_setup.c
> > > > @@ -132,6 +132,12 @@ efi_status_t efi_init_obj_list(void)
> > > >   	/* Allow unaligned memory access */
> > > >   	allow_unaligned();
> > > > 
> > > > +	/*
> > > > +	 * Some architectures need to kick secondary cores to enter their
> > > > +	 * spin table.
> > > > +	 */
> > > > +	smp_kick_all_cpus();
> > > This will not compile with
> > > 
> > > CONFIG_CMD_BOOTI=n
> > > CONFIG_CMD_BOOTM=n
> > > CONFIG_CMD_BOOTZ=n
> > 
> > 
> > Much worse is that in incurs needless overhead on PSCI capable
> > platforms. Can we move the smp_kick_all_cpus() to the board or soc
> > level of the few systems that use spin tables please? :)
> 
> Ok, I'm not sure where I should put the smp_kick_all_cpus(). In the
> bootm/booti path this is done in boot_jump_linux() (unconditionally
> in do_nonsec_virt_switch()). Therefore, shouldn't the kick be in
> switch_to_non_secure_mode(), too?
> 
> Regarding the overhead, isn't it just called once before starting
> the OS? Is that really worth adding yet another weak function or
> ifdef guards? Isn't it better to behave like the bootm case?

So, I think this shows that there's some consolidation needed as VxWorks
and ELF support have also had to solve this problem and are copying
around the same code that I believe Alex is saying we should in fact not
need on SoCs that handle this via PSCI instead of a spintable?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200518/8493d743/attachment.sig>

[PATCH 4/4] efi_loader: call smp_kick_all_cpus()

[Heinrich Schuchardt]

Am May 18, 2020 6:30:09 PM UTC schrieb Tom Rini <trini@konsulko.com>:
>On Sat, May 16, 2020 at 05:54:43PM +0200, Michael Walle wrote:
>> [Also adding Tom Rini as ARM maintainer]
>> 
>> Am 2020-05-14 22:17, schrieb Alexander Graf:
>> > On 14.05.20 20:46, Heinrich Schuchardt wrote:
>> > > On 5/14/20 2:38 PM, Michael Walle wrote:
>> > > > On some architectures, specifically the layerscape, the
>> > > > secondary cores
>> > > > wait for an interrupt before entering the spin-tables. This
>> > > > applies only
>> > > > to boards which doesn't have PSCI provided by TF-a and u-boot
>> > > > does the
>> > > %s/TF-a/TF-A/, %s/u-boot/U-Boot/
>> > > 
>> > > > secondary cores handling.
>> > > > bootm/booti already call that function for ARM architecture;
>> > > > also add it
>> > > > to bootelf before switching to EL2. Additionally, provide a
>weak noop
>> > > > function so we don't have to have "#ifdef CONFIG_ARM64" guards.
>> > > > 
>> > > > Signed-off-by: Michael Walle <michael@walle.cc>
>> > > > ---
>> > > >   common/bootm.c             | 9 +++++++++
>> > > >   lib/efi_loader/efi_setup.c | 6 ++++++
>> > > >   2 files changed, 15 insertions(+)
>> > > > 
>> > > > diff --git a/common/bootm.c b/common/bootm.c
>> > > > index db4362a643..65adf29329 100644
>> > > > --- a/common/bootm.c
>> > > > +++ b/common/bootm.c
>> > > > @@ -816,6 +816,15 @@ void __weak
>switch_to_non_secure_mode(void)
>> > > >   {
>> > > >   }
>> > > > 
>> > > > +/**
>> > > > + * smp_kick_all_cpus() - kick all CPUs
>> > > > + *
>> > > > + * This routine is overridden by architectures requiring this
>> > > > feature.
>> > > > + */
>> > > > +void __weak smp_kick_all_cpus(void)
>> > > > +{
>> > > > +}
>> > > > +
>> > > >   #else /* USE_HOSTCC */
>> > > > 
>> > > >   #if defined(CONFIG_FIT_SIGNATURE)
>> > > > diff --git a/lib/efi_loader/efi_setup.c
>b/lib/efi_loader/efi_setup.c
>> > > > index 26a7423203..7e5364adc5 100644
>> > > > --- a/lib/efi_loader/efi_setup.c
>> > > > +++ b/lib/efi_loader/efi_setup.c
>> > > > @@ -132,6 +132,12 @@ efi_status_t efi_init_obj_list(void)
>> > > >   	/* Allow unaligned memory access */
>> > > >   	allow_unaligned();
>> > > > 
>> > > > +	/*
>> > > > +	 * Some architectures need to kick secondary cores to enter
>their
>> > > > +	 * spin table.
>> > > > +	 */
>> > > > +	smp_kick_all_cpus();
>> > > This will not compile with
>> > > 
>> > > CONFIG_CMD_BOOTI=n
>> > > CONFIG_CMD_BOOTM=n
>> > > CONFIG_CMD_BOOTZ=n
>> > 
>> > 
>> > Much worse is that in incurs needless overhead on PSCI capable
>> > platforms. Can we move the smp_kick_all_cpus() to the board or soc
>> > level of the few systems that use spin tables please? :)
>> 
>> Ok, I'm not sure where I should put the smp_kick_all_cpus(). In the
>> bootm/booti path this is done in boot_jump_linux() (unconditionally
>> in do_nonsec_virt_switch()). Therefore, shouldn't the kick be in
>> switch_to_non_secure_mode(), too?
>> 
>> Regarding the overhead, isn't it just called once before starting
>> the OS? Is that really worth adding yet another weak function or
>> ifdef guards? Isn't it better to behave like the bootm case?
>
>So, I think this shows that there's some consolidation needed as
>VxWorks
>and ELF support have also had to solve this problem and are copying
>around the same code that I believe Alex is saying we should in fact
>not
>need on SoCs that handle this via PSCI instead of a spintable?

Michael wants to boot ARMv8 boards using spin-tables via EFI (FSL-Layerscape).

You might argue that the effort would be better invested in getting PSCI properly enabled for these boards.

Best regards

Heinrich

[PATCH 4/4] efi_loader: call smp_kick_all_cpus()

[Tom Rini]

On Mon, May 18, 2020 at 06:45:17PM +0000, Heinrich Schuchardt wrote:
> Am May 18, 2020 6:30:09 PM UTC schrieb Tom Rini <trini@konsulko.com>:
> >On Sat, May 16, 2020 at 05:54:43PM +0200, Michael Walle wrote:
> >> [Also adding Tom Rini as ARM maintainer]
> >> 
> >> Am 2020-05-14 22:17, schrieb Alexander Graf:
> >> > On 14.05.20 20:46, Heinrich Schuchardt wrote:
> >> > > On 5/14/20 2:38 PM, Michael Walle wrote:
> >> > > > On some architectures, specifically the layerscape, the
> >> > > > secondary cores
> >> > > > wait for an interrupt before entering the spin-tables. This
> >> > > > applies only
> >> > > > to boards which doesn't have PSCI provided by TF-a and u-boot
> >> > > > does the
> >> > > %s/TF-a/TF-A/, %s/u-boot/U-Boot/
> >> > > 
> >> > > > secondary cores handling.
> >> > > > bootm/booti already call that function for ARM architecture;
> >> > > > also add it
> >> > > > to bootelf before switching to EL2. Additionally, provide a
> >weak noop
> >> > > > function so we don't have to have "#ifdef CONFIG_ARM64" guards.
> >> > > > 
> >> > > > Signed-off-by: Michael Walle <michael@walle.cc>
> >> > > > ---
> >> > > >   common/bootm.c             | 9 +++++++++
> >> > > >   lib/efi_loader/efi_setup.c | 6 ++++++
> >> > > >   2 files changed, 15 insertions(+)
> >> > > > 
> >> > > > diff --git a/common/bootm.c b/common/bootm.c
> >> > > > index db4362a643..65adf29329 100644
> >> > > > --- a/common/bootm.c
> >> > > > +++ b/common/bootm.c
> >> > > > @@ -816,6 +816,15 @@ void __weak
> >switch_to_non_secure_mode(void)
> >> > > >   {
> >> > > >   }
> >> > > > 
> >> > > > +/**
> >> > > > + * smp_kick_all_cpus() - kick all CPUs
> >> > > > + *
> >> > > > + * This routine is overridden by architectures requiring this
> >> > > > feature.
> >> > > > + */
> >> > > > +void __weak smp_kick_all_cpus(void)
> >> > > > +{
> >> > > > +}
> >> > > > +
> >> > > >   #else /* USE_HOSTCC */
> >> > > > 
> >> > > >   #if defined(CONFIG_FIT_SIGNATURE)
> >> > > > diff --git a/lib/efi_loader/efi_setup.c
> >b/lib/efi_loader/efi_setup.c
> >> > > > index 26a7423203..7e5364adc5 100644
> >> > > > --- a/lib/efi_loader/efi_setup.c
> >> > > > +++ b/lib/efi_loader/efi_setup.c
> >> > > > @@ -132,6 +132,12 @@ efi_status_t efi_init_obj_list(void)
> >> > > >   	/* Allow unaligned memory access */
> >> > > >   	allow_unaligned();
> >> > > > 
> >> > > > +	/*
> >> > > > +	 * Some architectures need to kick secondary cores to enter
> >their
> >> > > > +	 * spin table.
> >> > > > +	 */
> >> > > > +	smp_kick_all_cpus();
> >> > > This will not compile with
> >> > > 
> >> > > CONFIG_CMD_BOOTI=n
> >> > > CONFIG_CMD_BOOTM=n
> >> > > CONFIG_CMD_BOOTZ=n
> >> > 
> >> > 
> >> > Much worse is that in incurs needless overhead on PSCI capable
> >> > platforms. Can we move the smp_kick_all_cpus() to the board or soc
> >> > level of the few systems that use spin tables please? :)
> >> 
> >> Ok, I'm not sure where I should put the smp_kick_all_cpus(). In the
> >> bootm/booti path this is done in boot_jump_linux() (unconditionally
> >> in do_nonsec_virt_switch()). Therefore, shouldn't the kick be in
> >> switch_to_non_secure_mode(), too?
> >> 
> >> Regarding the overhead, isn't it just called once before starting
> >> the OS? Is that really worth adding yet another weak function or
> >> ifdef guards? Isn't it better to behave like the bootm case?
> >
> >So, I think this shows that there's some consolidation needed as
> >VxWorks
> >and ELF support have also had to solve this problem and are copying
> >around the same code that I believe Alex is saying we should in fact
> >not
> >need on SoCs that handle this via PSCI instead of a spintable?
> 
> Michael wants to boot ARMv8 boards using spin-tables via EFI (FSL-Layerscape).
> 
> You might argue that the effort would be better invested in getting PSCI properly enabled for these boards.

Lets assume that it's somewhere on the TODO list for Layerscape but
since it's not done today it won't be done tomorrow and we should figure
out how to solve this, and if possible in a way that unifies the
approach that we have for Linux, VxWorks, ELF and binary applications
which I think only ZynqMP platforms handle today even.  The latter of
which implies that perhaps Zynq is in the same boat here?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200518/66a25373/attachment.sig>