Hi Rob,
On 18 September 2017 at 11:03, Rob Clark <robdcl...@gmail.com> wrote:
> On Mon, Sep 18, 2017 at 11:07 AM, Rob Clark <robdcl...@gmail.com> wrote:
>> On Mon, Sep 18, 2017 at 10:30 AM, Rob Clark <robdcl...@gmail.com> wrote:
>>> On Mon, Sep 18, 2017 at 9:31 AM, Rob Clark <robdcl...@gmail.com> wrote:
>>>> On Mon, Sep 18, 2017 at 9:18 AM, Rob Clark <robdcl...@gmail.com> wrote:
>>>>> On Sun, Sep 17, 2017 at 11:48 PM, Heinrich Schuchardt
>>>>> <xypron.g...@gmx.de> wrote:
>>>>>> On 09/18/2017 12:59 AM, Simon Glass wrote:
>>>>>>> A limitation of the EFI loader at present is that it does not build with
>>>>>>> sandbox. This makes it hard to write tests, since sandbox is used for
>>>>>>> most
>>>>>>> testing in U-Boot.
>>>>>>>
>>>>>>> This series enables the EFI loader feature. It allows sandbox to build
>>>>>>> and
>>>>>>> run a trivial function which calls the EFI API to output a message.
>>>>>>>
>>>>>>> Much work remains but this should serve as a basis for adding tests more
>>>>>>> easily for EFI loader.
>>>>>>>
>>>>>>> This series sits on top of Heinrich's recent EFI test series. It is
>>>>>>> available at u-boot-dm/efi-working
>>>>>>>
>>>>>>>
>>>>>>> Simon Glass (16):
>>>>>>> efi: Update efi_smbios_register() to return error code
>>>>>>> efi: Move the init check inside efi_init_obj_list()
>>>>>>> efi: Add error checking for efi_init_obj_list()
>>>>>>> efi: Add a TODO to efi_init_obj_list()
>>>>>>> efi: Correct header order in efi_memory
>>>>>>> efi: sandbox: Adjust memory setup for sandbox
>>>>>>> sandbox: smbios: Update to support sandbox
>>>>>>> sandbox: Add a setjmp() implementation
>>>>>>> efi: sandbox: Add required linker sections
>>>>>>> efi: sandbox: Add distroboot support
>>>>>>> Define board_quiesce_devices() in a shared location
>>>>>>> Add a comment for board_quiesce_devices()
>>>>>>> efi: sandbox: Add relocation constants
>>>>>>> efi: Add a comment about duplicated ELF constants
>>>>>>> efi: sandbox: Enable EFI loader builder for sandbox
>>>>>>> efi: sandbox: Add a simple 'bootefi test' command
>>>>>>>
>>>>>>> arch/arm/include/asm/u-boot-arm.h | 1 -
>>>>>>> arch/sandbox/cpu/cpu.c | 13 ++++++++++
>>>>>>> arch/sandbox/cpu/os.c | 17 ++++++++++++
>>>>>>> arch/sandbox/cpu/u-boot.lds | 29 +++++++++++++++++++++
>>>>>>> arch/sandbox/include/asm/setjmp.h | 21 +++++++++++++++
>>>>>>> arch/sandbox/lib/Makefile | 2 +-
>>>>>>> arch/sandbox/lib/sections.c | 12 +++++++++
>>>>>>> arch/x86/include/asm/u-boot-x86.h | 1 -
>>>>>>> arch/x86/lib/bootm.c | 4 ---
>>>>>>> cmd/bootefi.c | 54
>>>>>>> ++++++++++++++++++++++++++++++++++-----
>>>>>>> common/bootm.c | 4 +++
>>>>>>> configs/sandbox_defconfig | 1 +
>>>>>>> include/bootm.h | 8 ++++++
>>>>>>> include/config_distro_bootcmd.h | 2 +-
>>>>>>> include/efi_loader.h | 13 ++++++++--
>>>>>>> include/os.h | 21 +++++++++++++++
>>>>>>> lib/efi_loader/Kconfig | 12 ++++++++-
>>>>>>> lib/efi_loader/Makefile | 1 +
>>>>>>> lib/efi_loader/efi_boottime.c | 4 +++
>>>>>>> lib/efi_loader/efi_memory.c | 33 +++++++++++++-----------
>>>>>>> lib/efi_loader/efi_runtime.c | 7 +++++
>>>>>>> lib/efi_loader/efi_smbios.c | 6 +++--
>>>>>>> lib/efi_loader/efi_test.c | 17 ++++++++++++
>>>>>>> lib/smbios.c | 38 ++++++++++++++++++++-------
>>>>>>> 24 files changed, 277 insertions(+), 44 deletions(-)
>>>>>>> create mode 100644 arch/sandbox/include/asm/setjmp.h
>>>>>>> create mode 100644 arch/sandbox/lib/sections.c
>>>>>>> create mode 100644 lib/efi_loader/efi_test.c
>>>>>>>
>>>>>> Thanks for enabling efi_loader on sandbox. That will make many things
>>>>>> easier.
>>>>>>
>>>>>> Unfortunately
>>>>>> efi_status_t EFIAPI efi_selftest(efi_handle_t image_handle,
>>>>>> struct efi_system_table *systab)
>>>>>> {
>>>>>> ...
>>>>>> boottime = systable->boottime;
>>>>>> ...
>>>>>> ret = boottime->allocate_pool(EFI_BOOT_SERVICES_DATA, map_size,
>>>>>> (void **)&memory_map);
>>>>>> leads to a segmentation fault:
>>>>>
>>>>> I'm seeing something similar, because:
>>>>>
>>>>> (gdb) print gd->bd->bi_dram[0]
>>>>> $2 = {start = 0, size = 134217728}
>>>>>
>>>>> u-boot expects 1:1 phys:virt mapping, so that probably won't work.
>>>>
>>>> The following quick hack works.. something similar could probably be
>>>> smashed in to ""
>>>>
>>>> --------
>>>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
>>>> index cddafe2d43..da2079a4b1 100644
>>>> --- a/lib/efi_loader/efi_memory.c
>>>> +++ b/lib/efi_loader/efi_memory.c
>>>> @@ -459,9 +459,10 @@ int efi_memory_init(void)
>>>> unsigned long uboot_start, uboot_pages;
>>>> unsigned long uboot_stack_size = 16 * 1024 * 1024;
>>>>
>>>> - efi_add_known_memory();
>>>>
>>>> if (!IS_ENABLED(CONFIG_SANDBOX)) {
>>>> + efi_add_known_memory();
>>>> +
>>>> /* Add U-Boot */
>>>> uboot_start = (gd->start_addr_sp - uboot_stack_size) &
>>>> ~EFI_PAGE_MASK;
>>>> @@ -476,6 +477,12 @@ int efi_memory_init(void)
>>>> runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT;
>>>> efi_add_memory_map(runtime_start, runtime_pages,
>>>> EFI_RUNTIME_SERVICES_CODE, false);
>>>> + } else {
>>>> +#define SZ_256M 0x10000000
>>>> + size_t sz = SZ_256M;
>>>> + void *ram = os_malloc(sz);
>>>> + efi_add_memory_map((uintptr_t)ram, sz >> EFI_PAGE_SHIFT,
>>>> + EFI_CONVENTIONAL_MEMORY, false);
>>>> }
>>>>
>>>> #ifdef CONFIG_EFI_LOADER_BOUNCE_BUFFER
>>>> --------
>>>>
>>>> With that I'm at least getting further.. efi_allocate_pool()
>>>> eventually fails, possibly making every small memory allocation page
>>>> aligned means that 256m isn't enough..
>>>
>>> Ok, still just as hacky, but works a bit better:
>>>
>>> ---------
>>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
>>> index cddafe2d43..b546b5e35d 100644
>>> --- a/lib/efi_loader/efi_memory.c
>>> +++ b/lib/efi_loader/efi_memory.c
>>> @@ -14,6 +14,7 @@
>>> #include <linux/list_sort.h>
>>> #include <inttypes.h>
>>> #include <watchdog.h>
>>> +#include <os.h>
>>>
>>> DECLARE_GLOBAL_DATA_PTR;
>>>
>>> @@ -459,9 +460,9 @@ int efi_memory_init(void)
>>> unsigned long uboot_start, uboot_pages;
>>> unsigned long uboot_stack_size = 16 * 1024 * 1024;
>>>
>>> - efi_add_known_memory();
>>> -
>>> if (!IS_ENABLED(CONFIG_SANDBOX)) {
>>> + efi_add_known_memory();
>>> +
>>> /* Add U-Boot */
>>> uboot_start = (gd->start_addr_sp - uboot_stack_size) &
>>> ~EFI_PAGE_MASK;
>>> @@ -476,6 +477,14 @@ int efi_memory_init(void)
>>> runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT;
>>> efi_add_memory_map(runtime_start, runtime_pages,
>>> EFI_RUNTIME_SERVICES_CODE, false);
>>> + } else {
>>> +#define SZ_4K 0x00001000
>>> +#define SZ_256M 0x10000000
>>> + size_t sz = SZ_256M;
>>> + uintptr_t ram = (uintptr_t)os_malloc(sz + SZ_4K) + SZ_4K;
>>> + efi_add_memory_map(ram & ~EFI_PAGE_MASK, sz >> EFI_PAGE_SHIFT,
>>> + EFI_CONVENTIONAL_MEMORY, false);
>>> + gd->start_addr_sp = ~0;
>>> }
>>>
>>> #ifdef CONFIG_EFI_LOADER_BOUNCE_BUFFER
>>> ---------
>>>
>>> At this point it crashes in efi_load_pe() when it first tries to
>>> dereference the address of the image passed in, ie. I'm running:
>>>
>>> host bind 0 x86_64-sct.img
>>> load host 0:1 0x01000000 /efi/boot/shell.efi
>>> bootefi 0x01000000
>>>
>>> Not sure if there is a better way to pick an address to load into. Or
>>> maybe just assuming that PA==VA isn't a good idea in sandbox?
>>>
>>
>> Ok, I realized there is map_sysmem().. which gets me further..
>> efi_loader really expects identity mapping (PA==VA), and iirc this is
>> what UEFI spec expects too so I wouldn't necessarily call it a bug in
>> efi_loader.
>>
>
> So, I don't know x86(_64) asm or calling conventions as well as arm..
> but I wonder if we are screwing up something long those lines:
>
>
> 0000000000000280 <.text>:
> 280: 48 89 5c 24 08 mov %rbx,0x8(%rsp)
> 285: 57 push %rdi
> 286: 48 83 ec 20 sub $0x20,%rsp
> 28a: 48 8b f9 mov %rcx,%rdi
>>> 28d: e8 1e 00 00 00 callq 0x2b0
> this jump is taken to 0x2b0
>
> 292: e8 2d 06 00 00 callq 0x8c4
> 297: 48 8b cf mov %rdi,%rcx
> 29a: 48 8b d8 mov %rax,%rbx
> 29d: e8 ea 01 00 00 callq 0x48c
> 2a2: 48 8b c3 mov %rbx,%rax
> 2a5: 48 8b 5c 24 30 mov 0x30(%rsp),%rbx
> 2aa: 48 83 c4 20 add $0x20,%rsp
> 2ae: 5f pop %rdi
> 2af: c3 retq
>>> 2b0: 40 53 rex push %rbx
> 2b2: 48 83 ec 20 sub $0x20,%rsp
> 2b6: 48 89 0d e3 b9 05 00 mov %rcx,0x5b9e3(%rip)
> # 0x5bca0
> 2bd: 4c 8d 05 f4 b9 05 00 lea 0x5b9f4(%rip),%r8
> # 0x5bcb8
>>> 2c4: 48 8b 42 60 mov 0x60(%rdx),%rax
>
> and at 0x2c4 %rdx is 0x2.. I always thought x86 asm syntax strange,
> but I assume that is trying to write to value of %rdx + offset of
> 0x60?? But this is a register never written, so I assume it is
> expected to be passed from efi_loader?
>
> From https://en.wikipedia.org/wiki/X86_calling_conventions it seems
> that MS calling convention expects 2nd arg in %rdx, but linux/gcc
> calling convention expects 3rd arg in %rdx (there is no 3rd arg)..
I don't know it well either. But so long as functions are properly
declared in header files I don't really understand how we can have a
mismatch.
Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
