[U-Boot] [PATCH v8 02/30] efi: sandbox: Adjust memory usage for sandbox

[Alexander Graf]

On 06/21/2018 09:45 PM, Simon Glass wrote:
> Hi Alex,
>
> On 21 June 2018 at 03:52, Alexander Graf <agraf at suse.de> wrote:
>> On 06/21/2018 04:02 AM, Simon Glass wrote:
>>> Hi Alex,
>>>
>>> On 20 June 2018 at 02:54, Alexander Graf <agraf at suse.de> wrote:
>>>> On 06/20/2018 08:10 AM, Heinrich Schuchardt wrote:
>>>>> On 06/18/2018 04:08 PM, 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.
>>>>> This is not reflected in the patch.
>>>>>
>>>>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>>>>> ---
>>>>>>
>>>>>> Changes in v8: None
>>>>>> Changes in v7:
>>>>>> - Move some of the code from efi_memory_init() into a separate function
>>>>>>
>>>>>> 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 | 16 ++++++++++++----
>>>>>>     1 file changed, 12 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
>>>>>> index ec66af98ea..c6410613c7 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>
>>>>> I cannot see any use of this include in the patch.
>>>>>
>>>>>>     #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 where mapmem.h gets used. And yes, it's the wrong place. So NAK
>>>> on
>>>> the patch as-is.
>>> What is wrong with it?
>>
>> Efi_allocate_pool calls efi_allocate_pages() which according to spec returns
>> a pointer. So efi_allocate_pool() should not have to map anything, because
>> it does not receive an addres.
> You are referring to efi_allocate_pages_ext() I suspect. In my series
> this does the mapping, so that efi_allocate_pages() uses addresses
> only. We could rename it if you like.

I don't think we should rename things there. I really think there is a 
fundamental misunderstanding here on what the APIs are supposed to do. 
Basically:

   efi_allocate_pages() is mmap() in Liunx
   efi_allocate_pool() is malloc() in Linux

plus a few extra features of course, but you can think of them at the 
same level.

When a payload calls efi_allocate_pool, that really calls into a more 
sophisticated memory allocator usually which can allocate small chunks 
of memory efficiently. Given the amount of RAM modern systems have, I 
opted for simplicity though so I just mapped it to basically allocate 
pages using efi_allocate_pages().

As for _ext functions, the *only* thing we want in an _ext function is 
to isolate the logic that EFI_CALL() would do otherwise - namely swap 
gd. No more.


Alex