| Message ID | 20180618140835.195901-26-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Alexander Graf |
| Headers | show |
| Series | efi: Enable sandbox support for EFI loader | expand |
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> Very useful indeed. Reviewed-by: Alexander Graf <agraf@suse.de> Alex
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: 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)
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: > > 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
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); return EFI_EXIT(r); } @@ -364,11 +368,25 @@ static efi_status_t EFIAPI efi_get_memory_map_ext( uint32_t *descriptor_version) { 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); + 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)
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(-)