Hi Alex,
On 21 June 2018 at 10:45, Alexander Graf agraf@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@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:
- 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.
- 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);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