[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 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.
>