[U-Boot] [PATCH v6 03/13] efi: sandbox: Adjust memory usage for sandbox

[Simon Glass]

Hi Alex,

On 14 June 2018 at 08:22, Alexander Graf <agraf at suse.de> wrote:
>
>
>
> > Am 14.06.2018 um 16:12 schrieb Simon Glass <sjg at chromium.org>:
> >
> > Hi Alex,
> >
> >> On 14 June 2018 at 07:41, Alexander Graf <agraf at suse.de> wrote:
> >>> On 06/14/2018 02:58 PM, Simon Glass wrote:
> >>>
> >>> Hi Alex,
> >>>
> >>>> On 14 June 2018 at 04:12, Alexander Graf <agraf at suse.de> wrote:
> >>>>
> >>>>> On 06/13/2018 04:37 AM, 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.
> >>>>>
> >>>>> Signed-off-by: Simon Glass <sjg at chromium.org>
> >>>>> ---
> >>>>>
> >>>>> 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 | 31 ++++++++++++++++++-------------
> >>>>>   1 file changed, 18 insertions(+), 13 deletions(-)
> >>>>>
> >>>>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> >>>>> index ec66af98ea..210e49ee8b 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);
> >>>>
> >>>>
> >>>> This is still the wrong spot. We don't want the conversion to happen when
> >>>> going from an EFI internal address to an allocation, but when determining
> >>>> which addresses are usable in the first place.
> >>>
> >>> There seem to be two ways to do this:
> >>>
> >>> 1. Record addresses (ulong) in the EFI tables and use map_sysmem()
> >>> before returning an address in the allocator
> >>> 2. Store pointers in the EFI tables using map_sysmem(), then do no
> >>> mapping in the allocator
> >>>
> >>> I've gone with option 1 since:
> >>>
> >>> - the EFI addresses are not pointers
> >>> - it makes sandbox consistent with other architectures which use an
> >>> address rather than a pointer in EFI tables
> >>> - we don't normally do pointer arithmetic on the results of map_sysmem()
> >>> - we normally map the memory when it is used rather than when it is set up
> >>>
> >>> I think you are asking for option 2. I think that would get very
> >>> confusing. The addresses where things actually end up in sandbox are
> >>> best kept to sandbox.
> >>>
> >>> Overall I feel that you are either missing the point of sandbox
> >>> addressing, or don't agree with how it is done. But it does work
> >>> pretty well and we don't get a lot of confusion with sandbox pointers
> >>> since we typically use the address until the last moment.
> >>
> >>
> >> I've assembled a quick tree for version 2. With that I'm able to run a
> >> simple hello world efi application. Grub refuses to start because it wants
> >> memory in the low 32bit and also emits random PIO accessing functions, which
> >> obviously don't work work from user space.
> >>
> >> But overall, I think this is the right path to tackle this:
> >>
> >>  https://github.com/agraf/u-boot/tree/efi-sandbox
> >
> > What do you mean by version 2?
>
> Option 2 is what you called it. It's the only option we have to make efi binaries work.
>
> > It looks like you've added one patch,
> > so will you send that to the list?
>
> It's more than 1 patch. And yes, I'll send them.
>
> >
> > Anyway, I hope I can convince you of the above, the way sandbox memory works.
>
> I still dislike option 1 :)
>
> The reason is simple: The efi memory map is available to efi payloads. It's perfectly legal for them to do a static allocation at a particular address. We also share a lot of (host) pointers for callbacks and structs already with efi applications, so there is no real point to have a split brain situation between u-boot and host pointers.

OK so you mean they don't have to allocate memory before using it? How
then do you make sure that there is no conflict? I thought EFI was an
API?

Regards,
Simon