Re: [PATCH 059/112] efi: payload: init: restrict barebox mem to first 1G only on x86

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

On 05.01.24 10:31, Michael Olbrich wrote:
> Hi,
> 
> On Fri, Jan 05, 2024 at 10:14:33AM +0100, Ahmad Fatoum wrote:
>> On 04.01.24 19:10, Michael Olbrich wrote:
>>> On Thu, Jan 04, 2024 at 12:17:12PM +0100, Ahmad Fatoum wrote:
>>>> On 03.01.24 19:58, Michael Olbrich wrote:
>>>>> On Wed, Jan 03, 2024 at 07:12:19PM +0100, Ahmad Fatoum wrote:
>>>>>> When run as EFI payload, barebox will not have full access to DRAM and will
>>>>>> have to ask the EFI firmware for memory. It does so once at the beginning
>>>>>> and since commit 356aaef5a37d ("efi: improve malloc pool allocation"),
>>>>>> the buffer was always placed beneath the 1G boundary.
>>>>>>
>>>>>> Aforementioned commit didn't elaborate why a maximum of 1G in particular
>>>>>> was chosen. Anyways, non-PC architectures have different memory maps, so
>>>>>> a 1G limit doesn't make sense there. Therefore restrict the limit to
>>>>>> ARCH_X86. On non-x86, we allow the memory pages to be located anywhere.
>>>>>
>>>>> The 1G is somewhat arbitrary but if you don't restrict the memory to 32bit,
>>>>> then loading the kernel may fail: The addresses in "struct
>>>>> linux_kernel_header" are only 32bit. So if you impose no restrictions at
>>>>> all, then this memory cannot be used for the kernel and initrd.
>>>>
>>>> The limit for x86 remains at 1G. It's only for other architectures, that we
>>>> remove the limit. I suspect that other architectures won't have this kind
>>>> of problems though , because they would boot the kernel image via EFI load
>>>> and start image boot services, where the EFI firmware takes care to layout
>>>> the boot artifacts.
>>>>
>>>> Once we have that in place, we could even use it on x86 instead of
>>>> using struct linux_kernel_header, but I haven't investigated this yet.
>>>
>>> How do you load the initrd via EFI? As far as I know, that only works as a
>>> filename in the EFI boot partition.
>>
>> This seems to be relaxed in newer kernels, see
>> https://lore.kernel.org/linux-efi/20200207202637.GA3464906@rani.riverdale.lan/T/
> 
> It's not quite clear to me how that works, but from what I can tell,
> requires support from the firmware and barebox needs to use it. So that
> does not help here.
> You need to either disable the legacy handler to avoid arbitrary failures
> or explicitly allocate the initrd memory from efi with the restriction to
> <4G.

The legacy handler wouldn't apply to other architectures:

  - The Kernel images for ARM64 and RISC-V have different more-specific filetypes
    than filetype_exe

  - The Linux kernel header check will probabaly fail for every format that's not
    x86

Cheers,
Ahmad 

> 
> Michael
> 
>>> That was the reason for implementing
>>> the "legacy" boot mode. And what about the device-tree?
>>
>> There is an EFI config table for FDT (DEVICE_TREE_GUID in Linux code). barebox
>> as EFI loader can use it to provide its own DT. I have to check what's the cleanest
>> way for barebox as EFI payload to provide a different DT, but I hope there is a way
>> without barebox as EFI payload having to manipulate the device tree itself.
>>
>> Cheers,
>> Ahmad
>>
>>>
>>> Michael
>>>
>>>>>> This can break PCI drivers on systems that so far assumed barebox memory
>>>>>> is located in the first 4G, but that's ok. That code will need to be
>>>>>> fixed eventually anyway.
>>>>>>
>>>>>> Cc: Michael Olbrich <m.olbrich@pengutronix.de>
>>>>>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>>>>>> ---
>>>>>>  efi/payload/init.c | 2 +-
>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/efi/payload/init.c b/efi/payload/init.c
>>>>>> index 4b612e61afb3..0c2f38e9c86a 100644
>>>>>> --- a/efi/payload/init.c
>>>>>> +++ b/efi/payload/init.c
>>>>>> @@ -292,7 +292,7 @@ void efi_main(efi_handle_t image, struct efi_system_table *sys_table)
>>>>>>  		BS->handle_protocol(efi_loaded_image->device_handle,
>>>>>>  				&efi_device_path_protocol_guid, (void **)&efi_device_path);
>>>>>>  
>>>>>> -	mem = 0x3fffffff;
>>>>>> +	mem = IS_ENABLED(CONFIG_X86) ? 0x3fffffff : ~0ULL;
>>>>>>  	for (memsize = SZ_256M; memsize >= SZ_8M; memsize /= 2) {
>>>>>>  		efiret  = BS->allocate_pages(EFI_ALLOCATE_MAX_ADDRESS,
>>>>>>  					     EFI_LOADER_DATA,
>>>>>> -- 
>>>>>> 2.39.2
>>>>>>
>>>>>>
>>>>>
>>>>
>>>> -- 
>>>> Pengutronix e.K.                           |                             |
>>>> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
>>>> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
>>>> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>>>>
>>>>
>>>
>>
>> -- 
>> Pengutronix e.K.                           |                             |
>> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
>> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
>> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>>
>>
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |