[U-Boot] [PATCH v4 21/21] efi_loader: Expose U-Boot addresses in memory map for sandbox

[Simon Glass]

Hi Alex,

On 18 June 2018 at 09:23, Alexander Graf <agraf at suse.de> wrote:
>
> We currently expose host addresses in the EFI memory map. That can be
> bad if we ever want to use sandbox to boot strap a real kernel, because
> then the kernel would fetch its memory table from our host virtual address
> map. But to make that use case work, we would need to have full control
> over the address space the EFI application sees.
>
> So let's expose only U-Boot addresses to the guest until we get to the
> point of allocation. EFI's allocation functions are fun - they can take
> U-Boot addresses as input values for hints and return host addresses as
> allocation results through the same uint64_t * parameter. So we need to
> be extra careful on what to pass in when.
>
> With this patch I am successfully able to run the efi selftest suite as
> well as grub.efi on aarch64.
>
> Signed-off-by: Alexander Graf <agraf at suse.de>
> ---
>  arch/sandbox/cpu/cpu.c      | 19 -------------------
>  lib/efi_loader/efi_memory.c | 12 ++++++------
>  2 files changed, 6 insertions(+), 25 deletions(-)

This seems to compete with a patch from my series, but it's not clear
to be what the overlap is.

>
> diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c
> index 641b66a0a7..be88ab2f1c 100644
> --- a/arch/sandbox/cpu/cpu.c
> +++ b/arch/sandbox/cpu/cpu.c
> @@ -176,25 +176,6 @@ void longjmp(jmp_buf jmp, int ret)
>
>  #if CONFIG_IS_ENABLED(EFI_LOADER)
>
> -/*
> - * In sandbox, we don't have a 1:1 map, so we need to expose
> - * process addresses instead of U-Boot addresses
> - */
> -void efi_add_known_memory(void)
> -{
> -       u64 ram_start = (uintptr_t)map_sysmem(0, gd->ram_size);
> -       u64 ram_size = gd->ram_size;
> -       u64 start = (ram_start + EFI_PAGE_MASK) & ~EFI_PAGE_MASK;
> -       u64 pages = (ram_size + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
> -
> -       efi_add_memory_map(start, pages, EFI_CONVENTIONAL_MEMORY,
> -                          false);
> -}
> -
> -#endif
> -
> -#if CONFIG_IS_ENABLED(EFI_LOADER)
> -
>  void allow_unaligned(void)
>  {
>         int r;
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index 19492df518..64d2b8f7fa 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -326,7 +326,7 @@ efi_status_t efi_allocate_pages(int type, int memory_type,
>                 /* Reserve that map in our memory maps */
>                 ret = efi_add_memory_map(addr, pages, memory_type, true);
>                 if (ret == addr) {
> -                       *memory = addr;
> +                       *memory = (uintptr_t)map_sysmem(addr, pages * EFI_PAGE_SIZE);

This line does not seem to correspond to the code in your efi-sandbox-v5 tree.

Also if an address is passed in then presumably it needs
map_to_sysmem() before use (e.g. replace the *memory accesses in this
function).  I suspect those code paths have no tests which is why
things work.

Given that you have efi_allocate_pages() takes (and returns) a pointer
(but stored in a uin64_t) I think you are asking for confusion. At the
least this needs a comment to explain what is being returned.

Apart from that I think it looks right.

>                 } else {
>                         /* Map would overlap, bail out */
>                         r = EFI_OUT_OF_RESOURCES;
> @@ -360,11 +360,12 @@ void *efi_alloc(uint64_t len, int memory_type)
>  efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages)
>  {
>         uint64_t r = 0;
> +       uint64_t addr = map_to_sysmem((void*)(uintptr_t)memory);
>
> -       r = efi_add_memory_map(memory, pages, EFI_CONVENTIONAL_MEMORY, false);
> +       r = efi_add_memory_map(addr, pages, EFI_CONVENTIONAL_MEMORY, false);
>         /* Merging of adjacent free regions is missing */
>
> -       if (r == memory)
> +       if (r == addr)
>                 return EFI_SUCCESS;
>
>         return EFI_NOT_FOUND;
> @@ -381,9 +382,9 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages)
>  efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer)
>  {
>         efi_status_t r;
> -       efi_physical_addr_t t;
>         u64 num_pages = (size + sizeof(struct efi_pool_allocation) +
>                          EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
> +       struct efi_pool_allocation *alloc;
>
>         if (size == 0) {
>                 *buffer = NULL;
> @@ -391,10 +392,9 @@ efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer)
>         }
>
>         r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, pool_type, num_pages,
> -                              &t);
> +                              (uint64_t*)&alloc);
>
>         if (r == EFI_SUCCESS) {
> -               struct efi_pool_allocation *alloc = (void *)(uintptr_t)t;
>                 alloc->num_pages = num_pages;
>                 *buffer = alloc->data;
>         }
> --
> 2.12.3
>

Regards,
Simon