| Message ID | 20180618140835.195901-3-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Alexander Graf |
| Headers | show |
| Series | efi: Enable sandbox support for EFI loader | expand |
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); > alloc->num_pages = num_pages; > *buffer = alloc->data; > } > @@ -496,14 +497,13 @@ __weak void efi_add_known_memory(void) > } > } > > -int efi_memory_init(void) > +/* Add memory regions for U-Boot's memory and for the runtime services code */ > +static void add_u_boot_and_runtime(void) > { > unsigned long runtime_start, runtime_end, runtime_pages; > unsigned long uboot_start, uboot_pages; > unsigned long uboot_stack_size = 16 * 1024 * 1024; > > - efi_add_known_memory(); > - > /* Add U-Boot */ > uboot_start = (gd->start_addr_sp - uboot_stack_size) & ~EFI_PAGE_MASK; > uboot_pages = (gd->ram_top - uboot_start) >> EFI_PAGE_SHIFT; > @@ -516,6 +516,14 @@ int efi_memory_init(void) > runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT; > efi_add_memory_map(runtime_start, runtime_pages, > EFI_RUNTIME_SERVICES_CODE, false); > +} > + > +int efi_memory_init(void) > +{ > + efi_add_known_memory(); > + > + if (!IS_ENABLED(CONFIG_SANDBOX)) > + add_u_boot_and_runtime(); Is the sandbox not using relocation? A comment is missing in the code here to explain why add_u_boot_and_runtime() should not be called for the sandbox. Best regards Heinrich > > #ifdef CONFIG_EFI_LOADER_BOUNCE_BUFFER > /* Request a 32bit 64MB bounce buffer region */ >
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. Alex
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? Regards, Simon
Hi Heinrich, On 20 June 2018 at 00:10, Heinrich Schuchardt <xypron.glpk@gmx.de> 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. Please see ^^^^^^^ below, for example. > >> #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); ^^^^^^^^^^^^ >> alloc->num_pages = num_pages; >> *buffer = alloc->data; >> } >> @@ -496,14 +497,13 @@ __weak void efi_add_known_memory(void) >> } >> } >> >> -int efi_memory_init(void) >> +/* Add memory regions for U-Boot's memory and for the runtime services code */ >> +static void add_u_boot_and_runtime(void) >> { >> unsigned long runtime_start, runtime_end, runtime_pages; >> unsigned long uboot_start, uboot_pages; >> unsigned long uboot_stack_size = 16 * 1024 * 1024; >> >> - efi_add_known_memory(); >> - >> /* Add U-Boot */ >> uboot_start = (gd->start_addr_sp - uboot_stack_size) & ~EFI_PAGE_MASK; >> uboot_pages = (gd->ram_top - uboot_start) >> EFI_PAGE_SHIFT; >> @@ -516,6 +516,14 @@ int efi_memory_init(void) >> runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT; >> efi_add_memory_map(runtime_start, runtime_pages, >> EFI_RUNTIME_SERVICES_CODE, false); >> +} >> + >> +int efi_memory_init(void) >> +{ >> + efi_add_known_memory(); >> + >> + if (!IS_ENABLED(CONFIG_SANDBOX)) >> + add_u_boot_and_runtime(); > > Is the sandbox not using relocation? A comment is missing in the code > here to explain why add_u_boot_and_runtime() should not be called for > the sandbox. That's right, sandbox does not use relocation. But the real point is that its code is not within the sandbox memory map. Sandbox uses a emulated RAM buffer, separate from the U-Boot code and stack. Regards, Simon
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. Alex
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. Regards, Simon
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. Alex
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).
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
Hi Alex, On 23 June 2018 at 01:31, Alexander Graf <agraf@suse.de> wrote: > > > 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. 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
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> #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); alloc->num_pages = num_pages; *buffer = alloc->data; } @@ -496,14 +497,13 @@ __weak void efi_add_known_memory(void) } } -int efi_memory_init(void) +/* Add memory regions for U-Boot's memory and for the runtime services code */ +static void add_u_boot_and_runtime(void) { unsigned long runtime_start, runtime_end, runtime_pages; unsigned long uboot_start, uboot_pages; unsigned long uboot_stack_size = 16 * 1024 * 1024; - efi_add_known_memory(); - /* Add U-Boot */ uboot_start = (gd->start_addr_sp - uboot_stack_size) & ~EFI_PAGE_MASK; uboot_pages = (gd->ram_top - uboot_start) >> EFI_PAGE_SHIFT; @@ -516,6 +516,14 @@ int efi_memory_init(void) runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT; efi_add_memory_map(runtime_start, runtime_pages, EFI_RUNTIME_SERVICES_CODE, false); +} + +int efi_memory_init(void) +{ + efi_add_known_memory(); + + if (!IS_ENABLED(CONFIG_SANDBOX)) + add_u_boot_and_runtime(); #ifdef CONFIG_EFI_LOADER_BOUNCE_BUFFER /* Request a 32bit 64MB bounce buffer region */
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 <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(-)