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

[Simon Glass]

Hi Alex,

On 14 June 2018 at 04:12, Alexander Graf <agraf at suse.de> wrote:
> On 06/13/2018 04:37 AM, 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 v6: None
>> 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);
>
>
> This is still the wrong spot. We don't want the conversion to happen when
> going from an EFI internal address to an allocation, but when determining
> which addresses are usable in the first place.

There seem to be two ways to do this:

1. Record addresses (ulong) in the EFI tables and use map_sysmem()
before returning an address in the allocator
2. Store pointers in the EFI tables using map_sysmem(), then do no
mapping in the allocator

I've gone with option 1 since:

- the EFI addresses are not pointers
- it makes sandbox consistent with other architectures which use an
address rather than a pointer in EFI tables
- we don't normally do pointer arithmetic on the results of map_sysmem()
- we normally map the memory when it is used rather than when it is set up

I think you are asking for option 2. I think that would get very
confusing. The addresses where things actually end up in sandbox are
best kept to sandbox.

Overall I feel that you are either missing the point of sandbox
addressing, or don't agree with how it is done. But it does work
pretty well and we don't get a lot of confusion with sandbox pointers
since we typically use the address until the last moment.

>
>>                 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);
>
>
> Didn't we want to have these in a function?

Yes but I forgot. Will do for v7.

Regards,
Simon