On 12.06.18 15:48, Simon Glass wrote:
> Hi Alex,
>
> On 12 June 2018 at 02:05, Alexander Graf <ag...@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 <s...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
