[U-Boot] [PATCH v5 03/13] efi: sandbox: Adjust memory usage for sandbox

[Alexander Graf]

On 12.06.18 15:48, Simon Glass wrote:
> Hi Alex,
> 
> On 12 June 2018 at 02:05, Alexander Graf <agraf at suse.de> wrote:
>>
>>
>> On 12.06.18 07:26, 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 <sjg at chromium.org>
>>> ---
>>>
>>> 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);
>>
>> We want to eventually be able to run efi binaries inside sandbox, so we
>> need to expose a 1:1 memory map inside the efi interfaces.
>>
>> That means the memory argument of efi_allocate_pages() already needs to
>> be set to the virtual address in real VA space. The easiest way to get
>> there is if you just put virtual addresses in the efi memory map.
> 
> Can you please explain that a bit more, or give a code example? I'm
> not quite sure what you mean.

In efi_add_known_memory() we populate the EFI memory table with
addresses that EFI allocations can return. Because we don't control the
payloads that call these functions, we can only return pointers. That
means efi_add_known_memory() should add map_sysmem()'ed values into its
own table.

Basically while we expose "virtual 0 offset" addresses to the command
line, anything internal still works on pointers. And everything EFI
internal needs to be considered a pointer, because we don't control the
code that deals with them.

So in a nutshell, I mean something like this (untested, probably
whitespace broken and line wrapped):

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index ec66af98ea..80211d8c95 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -491,6 +491,9 @@ __weak void efi_add_known_memory(void)
 		u64 start = (ram_start + EFI_PAGE_MASK) & ~EFI_PAGE_MASK;
 		u64 pages = (ram_size + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;

+		/* Sandbox needs virtual addresses here */
+		start = (uintptr_t)map_sysmem(start, pages * EFI_PAGE_SIZE);
+
 		efi_add_memory_map(start, pages, EFI_CONVENTIONAL_MEMORY,
 				   false);
 	}

> 
>>
>>>               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);
>>
>> I guess it's ok to hide U-Boot from the payload for sandbox, yes. Please
>> extract the code into a function though, so that we don't get into too
>> much indenting.
>>
> 
> OK
> 
>>> +
>>> +             /* 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);
>>> +     }
>>
>> I guess we do want to indicate RTS, no? But like everything else we want
>> to expose it with the real VA addresses.
> 
> I suspect it would never be used but also that we should indicate RTS
> is present so that things that check for it don't fail. What do you
> think we should do here?

I'm not 100% sure yet. It'll be very hard to support anything that
relies on exit_boot_services() from within user space. So yes, just
leave it out for sandbox for now.


Alex