Hi Alex,
On 18 June 2018 at 09:23, Alexander Graf <ag...@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 <ag...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
