[U-Boot] [PATCH v8 25/30] efi: Add more debugging for memory allocations

[Simon Glass]

Hi Alex,

On 21 June 2018 at 10:45, Alexander Graf <agraf at suse.de> wrote:
> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>
>> Add some more verbose debugging when doing memory allocations. This might
>> help to find bugs later.
>>
>> Signed-off-by: Simon Glass <sjg at chromium.org>
>> ---
>>
>> Changes in v8: None
>> Changes in v7: None
>> Changes in v6: None
>> Changes in v5: None
>> Changes in v4: None
>> Changes in v3: None
>> Changes in v2: None
>>
>>   lib/efi_loader/efi_boottime.c | 18 ++++++++++++++++++
>>   lib/efi_loader/efi_memory.c   | 22 +++++++++++++++++++++-
>>   2 files changed, 39 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>> index aefafc3fba..2a41eb13aa 100644
>> --- a/lib/efi_loader/efi_boottime.c
>> +++ b/lib/efi_loader/efi_boottime.c
>> @@ -315,8 +315,12 @@ static efi_status_t EFIAPI efi_allocate_pages_ext(int
>> type, int memory_type,
>>                         map_to_sysmem((void *)(uintptr_t)*memory);
>>         else
>>                 addr = 0;
>> +       debug("   input ptr %lx, addr %lx\n", (unsigned long)*memory,
>> +             (unsigned long)addr);
>>         r = efi_allocate_pages(type, memory_type, pages, &addr);
>>         *memory = (uintptr_t)map_sysmem(addr, pages << EFI_PAGE_SHIFT);
>> +       debug("*  output addr %lx, ptr %lx\n", (unsigned long)addr,
>> +             (unsigned long)*memory);
>
>
> 2 nits:
>
> 1) On input, *memory is addr, on output *memory is ptr. I don't quite
> understand what the "addr" part above is supposed to do, but I'm fairly sure
> it's just remainders of some previous (incorrect) patch.

I don't want to raise this misconception in another thread.

>
> 2) Please don't put any debugging into _ext functions. I introduced them
> before we had EFI_CALL() which is a much better API for calling EFI
> functions. So sooner or later we'll probably get rid of _ext functions
> altogether and instead just call the EFI functions using EFI_CALL(). We'd
> lose all debugging output then.

OK, let's worry about this patch later, if we can get things agreed and landed.

>
>>         return EFI_EXIT(r);
>>   }
>>   @@ -364,11 +368,25 @@ static efi_status_t EFIAPI efi_get_memory_map_ext(
>>                                         uint32_t *descriptor_version)
>
>
> Same comment about _ext functions here.

OK.

>
>
>>   {
>>         efi_status_t r;
>> +       int i, entries;
>>         EFI_ENTRY("%p, %p, %p, %p, %p", memory_map_size, memory_map,
>>                   map_key, descriptor_size, descriptor_version);
>>         r = efi_get_memory_map(memory_map_size, memory_map, map_key,
>>                                descriptor_size, descriptor_version);
>> +       entries = *memory_map_size / sizeof(struct efi_mem_desc);
>> +       debug("   memory_map_size=%zx (%lx entries)\n", *memory_map_size,
>> +             (ulong)(*memory_map_size / sizeof(struct efi_mem_desc)));
>> +       if (memory_map) {
>> +               for (i = 0; i < entries; i++) {
>> +                       struct efi_mem_desc *desc = &memory_map[i];
>> +
>> +                       debug("   type %d, phys %lx, virt %lx, num_pages
>> %lx, attribute %lx\n",
>> +                             desc->type, (ulong)desc->physical_start,
>> +                             (ulong)desc->virtual_start,
>> +                             (ulong)desc->num_pages,
>> (ulong)desc->attribute);
>> +               }
>> +       }
>>         return EFI_EXIT(r);
>>   }
>>   diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
>> index ad61b723e6..856caa4a40 100644
>> --- a/lib/efi_loader/efi_memory.c
>> +++ b/lib/efi_loader/efi_memory.c
>> @@ -149,6 +149,24 @@ static s64 efi_mem_carve_out(struct efi_mem_list
>> *map,
>>         return EFI_CARVE_LOOP_AGAIN;
>>   }
>>   +static void efi_mem_print(const char *name)
>> +{
>> +       struct list_head *lhandle;
>> +
>> +       debug("   %s: memory map\n", name);
>> +       list_for_each(lhandle, &efi_mem) {
>> +               struct efi_mem_list *lmem = list_entry(lhandle,
>> +                       struct efi_mem_list, link);
>
>
> Are you sure the compiler is smart enough to optimize out the list walking
> in the debug case?

Yes it does for me, but I suppose it is not guaranteed that all
compilers would. In any case, I don't think it matters if an old
compiler is a bit crap and makes things a little slower.

[...]

Regards,
Simon