> Am 14.06.2018 um 17:43 schrieb Simon Glass <s...@chromium.org>:
>
> Hi Alex,
>
>> On 14 June 2018 at 08:22, Alexander Graf <ag...@suse.de> wrote:
>>
>>
>>
>>> Am 14.06.2018 um 16:12 schrieb Simon Glass <s...@chromium.org>:
>>>
>>> Hi Alex,
>>>
>>>>> On 14 June 2018 at 07:41, Alexander Graf <ag...@suse.de> wrote:
>>>>> On 06/14/2018 02:58 PM, Simon Glass wrote:
>>>>>
>>>>> Hi Alex,
>>>>>
>>>>>>> On 14 June 2018 at 04:12, Alexander Graf <ag...@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 <s...@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.
In a nutshell, what you're proposing is that the efi quivalent of mmap(addr,
MAP_FIXED) returns something != addr. That will break things.
Alex
>
> Regards,
> Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
