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

[Simon Glass]

Hi Alex,

On 22 June 2018 at 06:08, Alexander Graf <agraf at suse.de> wrote:
> 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.

Well, we can move things around, but fundamentally I think the current
efi_allocate_pages() needs something that maps its memory. In my patch
I put this in the ..._ext() function. I feel that the uint64_t *
prarameter of efi_allocate_pages() is really confusing, since it
implies an address rather than a pointer.

My vote would be to rename the function entirely and change the API to
work with pointers (doing the mapping inside itself).