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

[Simon Glass]

Hi Alex,

On 14 June 2018 at 07:41, Alexander Graf <agraf at suse.de> wrote:
> On 06/14/2018 02:58 PM, Simon Glass wrote:
>>
>> 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.
>
>
> I've assembled a quick tree for version 2. With that I'm able to run a
> simple hello world efi application. Grub refuses to start because it wants
> memory in the low 32bit and also emits random PIO accessing functions, which
> obviously don't work work from user space.
>
> But overall, I think this is the right path to tackle this:
>
>   https://github.com/agraf/u-boot/tree/efi-sandbox

What do you mean by version 2? It looks like you've added one patch,
so will you send that to the list?

Anyway, I hope I can convince you of the above, the way sandbox memory works.

Regards,
Simon