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

[Alexander Graf]

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.

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.

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

>   {
>   	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?


Alex

> +		struct efi_mem_desc *desc = &lmem->desc;
> +
> +		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);
> +	}
> +	debug("\n");
> +}
> +
>   uint64_t efi_add_memory_map(uint64_t start, uint64_t pages, int memory_type,
>   			    bool overlap_only_ram)
>   {
> @@ -237,6 +255,7 @@ uint64_t efi_add_memory_map(uint64_t start, uint64_t pages, int memory_type,
>   
>   	/* And make sure memory is listed in descending order */
>   	efi_mem_sort();
> +	efi_mem_print(__func__);
>   
>   	return start;
>   }
> @@ -453,7 +472,8 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
>   		map_entries++;
>   
>   	map_size = map_entries * sizeof(struct efi_mem_desc);
> -
> +	debug("%s: map_size %lx, provided_map_size %lx\n", __func__,
> +	      (ulong)map_size, (ulong)provided_map_size);
>   	*memory_map_size = map_size;
>   
>   	if (provided_map_size < map_size)