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

[Simon Glass]

Hi Alex,

On 23 June 2018 at 01:31, Alexander Graf <agraf at suse.de> wrote:
>
>
> 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.

I'm not sure either, but it might be worth taking a look once the dust settles.

>
> 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.

Well I think the problem is the API using addresses in one method and
pointers in another. We can't fix that.

Maybe that would work. One of us should take a look down the track.

Regards,
Simon