[PATCH] efi_loader : Suppress error print message

Sun Jan 28 23:38:31 CET 2024

On 1/28/24 18:38, Bhumkar, Tejas Arvind wrote:
> [AMD Official Use Only - General]
>
> Hi Heinrich,
>
>> -----Original Message-----
>> From: Heinrich Schuchardt <xypron.glpk at gmx.de>
>> Sent: Sunday, January 28, 2024 2:51 PM
>> To: Bhumkar, Tejas Arvind <tejas.arvind.bhumkar at amd.com>
>> Cc: u-boot at lists.denx.de; trini at konsulko.com; sjg at chromium.org; Simek, Michal
>> <michal.simek at amd.com>; Abbarapu, Venkatesh
>> <venkatesh.abbarapu at amd.com>; git at xilinx.com; Ilias Apalodimas
>> <ilias.apalodimas at linaro.org>
>> Subject: Re: [PATCH] efi_loader : Suppress error print message
>>
>> Caution: This message originated from an External Source. Use proper caution
>> when opening attachments, clicking links, or responding.
>>
>>
>> On 1/28/24 06:20, Bhumkar, Tejas Arvind wrote:
>>> [AMD Official Use Only - General]
>>>
>>> Hi Heinrich,
>>>
>>>> -----Original Message-----
>>>> From: Heinrich Schuchardt <xypron.glpk at gmx.de>
>>>> Sent: Wednesday, January 24, 2024 2:09 AM
>>>> To: Bhumkar, Tejas Arvind <tejas.arvind.bhumkar at amd.com>
>>>> Cc: u-boot at lists.denx.de; trini at konsulko.com; sjg at chromium.org;
>>>> Simek, Michal <michal.simek at amd.com>; Abbarapu, Venkatesh
>>>> <venkatesh.abbarapu at amd.com>; git at xilinx.com; Ilias Apalodimas
>>>> <ilias.apalodimas at linaro.org>
>>>> Subject: Re: [PATCH] efi_loader : Suppress error print message
>>>>
>>>> Caution: This message originated from an External Source. Use proper
>>>> caution when opening attachments, clicking links, or responding.
>>>>
>>>>
>>>> On 1/23/24 19:53, Bhumkar, Tejas Arvind wrote:
>>>>> [AMD Official Use Only - General]
>>>>>
>>>>> Hi Ilias & Heinrich,
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Heinrich Schuchardt <xypron.glpk at gmx.de>
>>>>>> Sent: Tuesday, January 23, 2024 3:30 PM
>>>>>> To: Ilias Apalodimas <ilias.apalodimas at linaro.org>; Bhumkar, Tejas
>>>>>> Arvind <tejas.arvind.bhumkar at amd.com>
>>>>>> Cc: u-boot at lists.denx.de; xypron.glpk at gmx.de; trini at konsulko.com;
>>>>>> sjg at chromium.org; Simek, Michal <michal.simek at amd.com>; Abbarapu,
>>>>>> Venkatesh <venkatesh.abbarapu at amd.com>; git at xilinx.com
>>>>>> Subject: Re: [PATCH] efi_loader : Suppress error print message
>>>>>>
>>>>>> Caution: This message originated from an External Source. Use
>>>>>> proper caution when opening attachments, clicking links, or responding.
>>>>>>
>>>>>>
>>>>>> On 1/23/24 09:33, Ilias Apalodimas wrote:
>>>>>>> Hi Tejas,
>>>>>>>
>>>>>>> On Mon, 22 Jan 2024 at 21:12, Tejas Bhumkar
>>>>>>> <tejas.arvind.bhumkar at amd.com> wrote:
>>>>>>>>
>>>>>>>> Currently, on certain Xilinx platforms, an issue has been
>>>>>>>> identified, manifesting as follows:
>>>>>>
>>>>>> Hello Tejas,
>>>>>>
>>>>>> thank you for bringing forward this issue.
>>>>>>
>>>>>> Which defconfig are you relating to?
>>>>>
>>>>> [Tejas]:
>>>>> Checking with xilinx_zynqmp_virt_defconfig
>>>>>
>>>>>>
>>>>>>>>
>>>>>>>> Starting kernel ...
>>>>>>>>
>>>>>>>> efi_free_pool: illegal free 0x0000000077830040
>>>>>>>> efi_free_pool: illegal free 0x000000007782d040
>>>>>>>> efi_free_pool: illegal free 0x000000007782c040
>>>>>>>>
>>>>>>>> The issue arises when the ramdisk image is relocated, placing it
>>>>>>>> within the previously allocated EFI memory region( as EFI is
>>>>>>>> established quite early in U-Boot).
>>>>>>
>>>>>> Which version of U-Boot are you on? Commit 06d514d77c37 ("lmb:
>>>>>> consider EFI memory map") was meant to avoid such a situation.
>>>>>
>>>>> [Tejas] :
>>>>> I'm verifying against the latest changes in the master branch. The
>>>>> introduction of commit 06d514d77c37 ("lmb: consider EFI memory map")
>>>>> has resolved the occurrence of the "efi_free_pool: illegal free"
>>>>> error. but, it leads to a new error, as detailed in the following
>>>>> patch:
>>>>> https://lore.kernel.org/all/20230212150706.2967007-1-sjoerd@collabora.
>>>>> com/
>>>>
>>>> Could you, please, try to reproduce your issues with origin/master as
>>>> of today and provide the full boot log. Please, also provide the
>>>> output of the bdinfo and the printenv command as well as the sizes of the
>> kernel and the RAM disk.
>>>
>>> [Tejas]: I've provided two logs: one obtained from the latest u-boot
>> origin/master, resulting in the "ERROR: reserving fdt memory region failed" error,
>> and the other from reverting commit 06d514d77c37 ("lmb: consider EFI memory
>> map"), which leads to the "efi_free_pool: illegal free" error.
>>>
>>> Thank You,
>>> Tejas.
>>
>> Where can I find these logs?
> [Tejas ] : I've sent the attachment in a previous email. I'm not sure why you didn't receive it.
> I've attached it again now. Please confirm if you still haven't received it.
>
> Thank You,
> Tejas.
>>

If I understand your FDT_memory_region_failed.log correctly:

Except for some added debug messages this log matches booting with an
unmodified upstream U-Boot as of Jan 22nd.

Your memory is split into two regions:

0x0 - 0x7ffffff and
0x800000000 - 0x87ffffff.

Your device-tree has a reserved region 0x7ff00000-0x7fffffff.

U-Boot relocates itself into the lower memory block due to current design.

efi_lmb_reserve() marks memory up to 0x7fffffff and
0x800000000-0x87fffffff as reserved.

boot_fdt_reserve_region() writes an error when it tries to reserve the
overlapping region 0x7ff00000-0x7fffffff because LMB recognizes this
region is already reserved.

This should not interfere with booting as boot_fdt_reserve_region() does
not have a return value but is not very clean and should be fixed.

Best regards

Heinrich
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>> I don't mind suppressing the print for some time, but out of
>>>>>>> curiosity, how is the ramdisk relocated? LMB should be aware of
>>>>>>> the EFI regions by then, so I assume the relocation code doesn't
>>>>>>> check those?
>>>>>
>>>>>
>>>>> [Tejas] :  I observe that the relocation of the RAM disk is taking
>>>>> place in the line
>>>> below.
>>>>> https://github.com/u-boot/u-boot/blob/master/lib/lmb.c#L480-L491
>>>>> Yes, the relocated code specifically examines the LMB region and
>>>>> does not
>>>> consider the EFI region.
>>>>>
>>>>>>
>>>>>> The indicated situation is serious. If the EFI sub-system is using
>>>>>> the same memory area as the ramdisk, this may lead to corruption of
>>>>>> the ramdisk. Suppressing the messages is by no means adequate to
>>>>>> solve the
>>>> problem.
>>>>>
>>>>> [Tejas] :
>>>>> The challenge arises when relocating the ramdisk image, inserting it
>>>>> into the previously assigned EFI memory region, established early in
>>>>> U-Boot. Consequently, when attempting to release memory in the EFI
>>>>> region
>>>> during the handover process to the kernel in the efi_free_pool
>>>> function, we first verify if the memory was allocated by efi_allocate_pool().
>>>>> The issue originates from a checksum mismatch because, during the
>>>>> ramdisk
>>>> relocation, we overwrite memory allocated by EFI.
>>>>> This leads to the appearance of the error message: efi_free_pool:
>>>>> illegal free
>>>> 0x0000000077830040.
>>>>> Crucially, there is no corruption of the ramdisk seen since we do
>>>>> not actually
>>>> releasing memory due to the checksum mismatch.
>>>>> In our testing, this issue was observed only when the ramdisk size
>>>>> exceeded
>>>> approximately 24 MB.
>>>>>
>>>>>>
>>>>>> Best regards
>>>>>>
>>>>>> Heinrich
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> Thanks
>>>>>>> /Ilias
>>>>>>>
>>>>>>>> Consequently, when
>>>>>>>> attempting to release memory in the EFI memory region during the
>>>>>>>> handover process to the kernel,we encounter memory violations.
>>>>>>>>
>>>>>>>> Highlighting that EFI remains active primarily during the booting
>>>>>>>> of an EFI application, and the lmb persists while configuring
>>>>>>>> images for the boot process. Since we aren't utilizing the EFI
>>>>>>>> memory region during the boot process, there is no adverse impact
>>>>>>>> even in the event of a violation.
>>>>>>>>
>>>>>>>> Currently, there is an ongoing discussion regarding the handling
>>>>>>>> strategies of three memory allocators: malloc, lmb, and EFI. This
>>>>>>>> discussion is documented in the email chain titled "Proposal:
>>>>>>>> U-Boot memory management."
>>>>>>>>
>>>>>>>> Therefore, it is advisable to suppress the print message during
>>>>>>>> the boot process for now.
>>>>>>>>
>>>>>>>> Signed-off-by: Tejas Bhumkar <tejas.arvind.bhumkar at amd.com>
>>>>>>>> ---
>>>>>>>>      lib/efi_loader/efi_memory.c | 2 +-
>>>>>>>>      1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/lib/efi_loader/efi_memory.c
>>>>>>>> b/lib/efi_loader/efi_memory.c index edfad2d95a..821fe7616e 100644
>>>>>>>> --- a/lib/efi_loader/efi_memory.c
>>>>>>>> +++ b/lib/efi_loader/efi_memory.c
>>>>>>>> @@ -713,7 +713,7 @@ efi_status_t efi_free_pool(void *buffer)
>>>>>>>>             /* Check that this memory was allocated by efi_allocate_pool() */
>>>>>>>>             if (((uintptr_t)alloc & EFI_PAGE_MASK) ||
>>>>>>>>                 alloc->checksum != checksum(alloc)) {
>>>>>>>> -               printf("%s: illegal free 0x%p\n", __func__, buffer);
>>>>>>>> +               debug("%s: illegal free 0x%p\n", __func__,
>>>>>>>> + buffer);
>>>>>>>>                     return EFI_INVALID_PARAMETER;
>>>>>>>>             }
>>>>>>>>             /* Avoid double free */
>>>>>>>> --
>>>>>>>> 2.27.0
>>>>>>>>
>>>>>
>>>
>