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

Thu Jun 14 16:07:43 UTC 2018

On 06/14/2018 05:53 PM, Simon Glass wrote:
> Hi Alex,
>
> On 14 June 2018 at 09:47, Alexander Graf <agraf at suse.de> wrote:
>>
>>> Am 14.06.2018 um 17:43 schrieb Simon Glass <sjg at chromium.org>:
>>>
>>> Hi Alex,
>>>
>>>> On 14 June 2018 at 08:22, Alexander Graf <agraf at suse.de> wrote:
>>>>
>>>>
>>>>
>>>>> Am 14.06.2018 um 16:12 schrieb Simon Glass <sjg at chromium.org>:
>>>>>
>>>>> 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?
>>>> Option 2 is what you called it. It's the only option we have to make efi binaries work.
>>>>
>>>>> It looks like you've added one patch,
>>>>> so will you send that to the list?
>>>> It's more than 1 patch. And yes, I'll send them.
>>>>
>>>>> Anyway, I hope I can convince you of the above, the way sandbox memory works.
>>>> I still dislike option 1 :)
>>>>
>>>> The reason is simple: The efi memory map is available to efi payloads. It's perfectly legal for them to do a static allocation at a particular address. We also share a lot of (host) pointers for callbacks and structs already with efi applications, so there is no real point to have a split brain situation between u-boot and host pointers.
>>> OK so you mean they don't have to allocate memory before using it? How
>>> then do you make sure that there is no conflict? I thought EFI was an
>>> API?
>> You allocate it, but payloads expect that the address you pass in as address you allocate at is the pointer/address that gets returned.
>>
> Can you please point me to that? Are you referring to this call?
>
> static efi_status_t EFIAPI efi_get_memory_map_ext(
>          efi_uintn_t *memory_map_size,
>          struct efi_mem_desc *memory_map,
>          efi_uintn_t *map_key,
>          efi_uintn_t *descriptor_size,
>          uint32_t *descriptor_version)
>
> If so, we can still support that.
>
>> In a nutshell, what you're proposing is that the efi quivalent of mmap(addr, MAP_FIXED) returns something != addr. That will break things.
> I see this function:
>
> efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer)
>
> So there appears to be no way for people to specify the address they
> want. Is there another call somewhere?

You can call efi_allocate_pages() with EFI_ALLOCATE_ADDRESS or even 
EFI_ALLOCATE_MAX_ADDRESS which you usually base on information you 
receive from efi_get_memory_map().

> NOTE: I am not proposing that the address is made available to the EFI
> application - that would always see the sandbox pointer.

In that case it's *much* easier to just always store host pointers in 
the efi memory map. Everything else will be very intrusive.

Alex