Hi Alex, On 23 June 2018 at 01:31, Alexander Graf <[email protected]> wrote: > > > On 22.06.18 21:28, Simon Glass wrote: >> 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. > > I agree that it's confusing, but it's probably the only thing that works > when running on a 32bit platform. > >> My vote would be to rename the function entirely and change the API to >> work with pointers (doing the mapping inside itself). > > The problem I see there is that we're trying to do a shim that is as > tiny as possible for real efi functionality. I don't know if splitting > the function is going to give us real improvements in readability or > maintainability, since there will be people who will come from an EFI > background and grasp what the EFI functions do, but not our internal layers.
I'm not sure either, but it might be worth taking a look once the dust settles. > > What we could do is something the other way around: An internal wrapper > on top of efi_allocate_pages. Something like efi_get_mem() which takes > an address in one parameter and returns a pointer in another. But that > function would just call efi_allocate_pages(). If that again is a static > inline function, code size shouldn't even change. Well I think the problem is the API using addresses in one method and pointers in another. We can't fix that. Maybe that would work. One of us should take a look down the track. Regards, Simon _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
