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

[Alexander Graf]

On 22.06.18 21:28, Simon Glass wrote:
> 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.

I agree that it's confusing, but it's probably the only thing that works
when running on a 32bit platform.

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

The problem I see there is that we're trying to do a shim that is as
tiny as possible for real efi functionality. I don't know if splitting
the function is going to give us real improvements in readability or
maintainability, since there will be people who will come from an EFI
background and grasp what the EFI functions do, but not our internal layers.

What we could do is something the other way around: An internal wrapper
on top of efi_allocate_pages. Something like efi_get_mem() which takes
an address in one parameter and returns a pointer in another. But that
function would just call efi_allocate_pages(). If that again is a static
inline function, code size shouldn't even change.


Alex