On 22.06.18 21:28, Simon Glass wrote:
Hi Alex,
On 22 June 2018 at 06:08, Alexander Graf agraf@suse.de wrote:
On 06/21/2018 09:45 PM, Simon Glass wrote:
Hi Alex,
On 21 June 2018 at 03:52, Alexander Graf agraf@suse.de wrote:
On 06/21/2018 04:02 AM, Simon Glass wrote:
Hi Alex,
On 20 June 2018 at 02:54, Alexander Graf agraf@suse.de 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 sjg@chromium.org >> --- >> >> 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.
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.
Alex