| Message ID | 20200514123831.30157-2-michael@walle.cc |
|---|---|
| State | New |
| Headers | show |
| Series | bootefi fixes for aarch64/layerscape | expand |
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 at 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); >
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 at 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); >>
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 at 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); >>> >
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 at 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.
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 at 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. >
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 at 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. >> >
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 at 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. >>> >>
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);
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 at walle.cc> --- arch/arm/cpu/armv8/u-boot.lds | 9 ++++++++- lib/efi_loader/efi_memory.c | 15 ++------------- 2 files changed, 10 insertions(+), 14 deletions(-)