Hi Alex, On 22 June 2018 at 06:08, Alexander Graf <[email protected]> wrote: > On 06/21/2018 09:45 PM, Simon Glass wrote: >> >> Hi Alex, >> >> On 21 June 2018 at 03:52, Alexander Graf <[email protected]> wrote: >>> >>> On 06/21/2018 04:02 AM, Simon Glass wrote: >>>> >>>> Hi Alex, >>>> >>>> On 20 June 2018 at 02:54, Alexander Graf <[email protected]> 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 <[email protected]> >>>>>>> --- >>>>>>> >>>>>>> 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. My vote would be to rename the function entirely and change the API to work with pointers (doing the mapping inside itself). _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
