Hi Alex,
On 14 June 2018 at 09:47, Alexander Graf <ag...@suse.de> wrote:
>
>
>> 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.
>
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?
NOTE: I am not proposing that the address is made available to the EFI
application - that would always see the sandbox pointer.
Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
