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.
My vote would be to rename the function entirely and change the API to work with pointers (doing the mapping inside itself).