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