Hi Alex, On 14 June 2018 at 04:12, Alexander Graf <ag...@suse.de> wrote: > On 06/13/2018 04:37 AM, Simon Glass wrote: >> >> With sandbox the U-Boot code is not mapped into the sandbox memory range >> so does not need to be excluded when allocating EFI memory. Update the EFI >> memory init code to take account of that. >> >> Also use mapmem instead of a cast to convert a memory address to a >> pointer. >> >> Signed-off-by: Simon Glass <s...@chromium.org> >> --- >> >> Changes in v6: None >> Changes in v5: None >> Changes in v4: None >> Changes in v3: None >> Changes in v2: >> - Update to use mapmem instead of a cast >> >> lib/efi_loader/efi_memory.c | 31 ++++++++++++++++++------------- >> 1 file changed, 18 insertions(+), 13 deletions(-) >> >> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c >> index ec66af98ea..210e49ee8b 100644 >> --- a/lib/efi_loader/efi_memory.c >> +++ b/lib/efi_loader/efi_memory.c >> @@ -9,6 +9,7 @@ >> #include <efi_loader.h> >> #include <inttypes.h> >> #include <malloc.h> >> +#include <mapmem.h> >> #include <watchdog.h> >> #include <linux/list_sort.h> >> @@ -393,7 +394,7 @@ efi_status_t efi_allocate_pool(int pool_type, >> efi_uintn_t size, void **buffer) >> &t); >> if (r == EFI_SUCCESS) { >> - struct efi_pool_allocation *alloc = (void *)(uintptr_t)t; >> + struct efi_pool_allocation *alloc = map_sysmem(t, size); > > > This is still the wrong spot. We don't want the conversion to happen when > going from an EFI internal address to an allocation, but when determining > which addresses are usable in the first place.
There seem to be two ways to do this: 1. Record addresses (ulong) in the EFI tables and use map_sysmem() before returning an address in the allocator 2. Store pointers in the EFI tables using map_sysmem(), then do no mapping in the allocator I've gone with option 1 since: - the EFI addresses are not pointers - it makes sandbox consistent with other architectures which use an address rather than a pointer in EFI tables - we don't normally do pointer arithmetic on the results of map_sysmem() - we normally map the memory when it is used rather than when it is set up I think you are asking for option 2. I think that would get very confusing. The addresses where things actually end up in sandbox are best kept to sandbox. Overall I feel that you are either missing the point of sandbox addressing, or don't agree with how it is done. But it does work pretty well and we don't get a lot of confusion with sandbox pointers since we typically use the address until the last moment. > >> alloc->num_pages = num_pages; >> *buffer = alloc->data; >> } >> @@ -504,18 +505,22 @@ int efi_memory_init(void) >> efi_add_known_memory(); >> - /* Add U-Boot */ >> - uboot_start = (gd->start_addr_sp - uboot_stack_size) & >> ~EFI_PAGE_MASK; >> - uboot_pages = (gd->ram_top - uboot_start) >> EFI_PAGE_SHIFT; >> - efi_add_memory_map(uboot_start, uboot_pages, EFI_LOADER_DATA, >> false); >> - >> - /* Add Runtime Services */ >> - runtime_start = (ulong)&__efi_runtime_start & ~EFI_PAGE_MASK; >> - runtime_end = (ulong)&__efi_runtime_stop; >> - runtime_end = (runtime_end + EFI_PAGE_MASK) & ~EFI_PAGE_MASK; >> - runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT; >> - efi_add_memory_map(runtime_start, runtime_pages, >> - EFI_RUNTIME_SERVICES_CODE, false); >> + if (!IS_ENABLED(CONFIG_SANDBOX)) { >> + /* Add U-Boot */ >> + uboot_start = (gd->start_addr_sp - uboot_stack_size) & >> + ~EFI_PAGE_MASK; >> + uboot_pages = (gd->ram_top - uboot_start) >> >> EFI_PAGE_SHIFT; >> + efi_add_memory_map(uboot_start, uboot_pages, >> EFI_LOADER_DATA, >> + false); > > > Didn't we want to have these in a function? Yes but I forgot. Will do for v7. Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot