[U-Boot] [PATCH v6 03/13] efi: sandbox: Adjust memory usage for sandbox
Simon Glass
sjg at chromium.org
Thu Jun 14 19:02:37 UTC 2018
Hi Alex,
On 14 June 2018 at 12:05, Alexander Graf <agraf at suse.de> wrote:
>
>
>
> On 14.06.18 19:15, Simon Glass wrote:
> > Hi Alex,
> >
> > On 14 June 2018 at 11:08, Alexander Graf <agraf at suse.de> wrote:
> >>
> >> On 06/14/2018 06:55 PM, Simon Glass wrote:
> >>>
> >>> Hi Alex,
> >>>
> >>> On 14 June 2018 at 10:42, Alexander Graf <agraf at suse.de> wrote:
> >>>>
> >>>> On 06/14/2018 06:33 PM, Simon Glass wrote:
> >>>>>
> >>>>> Hi Alex,
> >>>>>
> >>>>> On 14 June 2018 at 10:26, Alexander Graf <agraf at suse.de> wrote:
> >>>>>>
> >>>>>> On 06/14/2018 06:13 PM, Simon Glass wrote:
> >>>>>>>
> >>>>>>> Hi Alex,
> >>>>>>>
> >>>>>>> On 14 June 2018 at 10:07, Alexander Graf <agraf at suse.de> wrote:
> >>>>>>>>
> >>>>>>>> On 06/14/2018 05:53 PM, Simon Glass wrote:
> >>>>>>>>>
> >>>>>>>>> Hi Alex,
> >>>>>>>>>
> >>>>>>>>> On 14 June 2018 at 09:47, Alexander Graf <agraf at suse.de> wrote:
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>> Am 14.06.2018 um 17:43 schrieb Simon Glass <sjg at chromium.org>:
> >>>>>>>>>>>
> >>>>>>>>>>> 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?
> >>>>>>>>>>
> >>>>>>>>>> You allocate it, but payloads expect that the address you pass in as
> >>>>>>>>>> address you allocate at is the pointer/address that gets returned.
> >>>>>>>>>>
> >>>>>>>>> Can you please point me to that? Are you referring to this call?
> >>>>>>>>>
> >>>>>>>>> static efi_status_t EFIAPI efi_get_memory_map_ext(
> >>>>>>>>> efi_uintn_t *memory_map_size,
> >>>>>>>>> struct efi_mem_desc *memory_map,
> >>>>>>>>> efi_uintn_t *map_key,
> >>>>>>>>> efi_uintn_t *descriptor_size,
> >>>>>>>>> uint32_t *descriptor_version)
> >>>>>>>>>
> >>>>>>>>> If so, we can still support that.
> >>>>>>>>>
> >>>>>>>>>> In a nutshell, what you're proposing is that the efi quivalent of
> >>>>>>>>>> mmap(addr, MAP_FIXED) returns something != addr. That will break
> >>>>>>>>>> things.
> >>>>>>>>>
> >>>>>>>>> I see this function:
> >>>>>>>>>
> >>>>>>>>> efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void
> >>>>>>>>> **buffer)
> >>>>>>>>>
> >>>>>>>>> So there appears to be no way for people to specify the address they
> >>>>>>>>> want. Is there another call somewhere?
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> You can call efi_allocate_pages() with EFI_ALLOCATE_ADDRESS or even
> >>>>>>>> EFI_ALLOCATE_MAX_ADDRESS which you usually base on information you
> >>>>>>>> receive
> >>>>>>>> from efi_get_memory_map().
> >>>>>>>
> >>>>>>> That's fine though, I'm not worried about that. It will work correctly,
> >>>>>>> right?
> >>>>>>>
> >>>>>>>>> NOTE: I am not proposing that the address is made available to the EFI
> >>>>>>>>> application - that would always see the sandbox pointer.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> In that case it's *much* easier to just always store host pointers in
> >>>>>>>> the
> >>>>>>>> efi memory map. Everything else will be very intrusive.
> >>>>>>>
> >>>>>>> Yes there will be map_sysmem() and map_to_sysmem() calls at each
> >>>>>>> boundary. That 'intrusion' is what we have everywhere in sandbox and
> >>>>>>> it is how things work. But the nice thing is that the tables don't end
> >>>>>>> up with 'private' sandbox addresses in them. Also see me comments
> >>>>>>> above.
> >>>>>>
> >>>>>>
> >>>>>> I seem to be missing the point. efi_loader is just a tiny shim layer
> >>>>>> whose
> >>>>>> only purpose it is to interface between the binary efi interface and
> >>>>>> U-Boot
> >>>>>> internal code. So why shouldn't efi_loader treat addresses as pointers
> >>>>>> internally?
> >>>>>>
> >>>>>> So how about this: I'll send out a patch set that works the way I think
> >>>>>> is
> >>>>>> the right way forward. With this I can at least run binaries generated by
> >>>>>> gnu-efi. Then you can send a patch on top with a proposal how you think
> >>>>>> the
> >>>>>> mapping should happen. My guess is that your patch will just clutter the
> >>>>>> code, but I'll be happy to be persuaded otherwise.
> >>>>>
> >>>>> I'm just not keen on putting sandbox pointers in internal data
> >>>>> structures, sorry. It's not the way it is supposed to work and it
> >>>>> makes sandbox diffferent from all the other archs. Trying to minimise
> >>>>> the number of calls to map_sysmem() is not really the goal here.
> >>>>
> >>>>
> >>>> Well, you're not the one who needs to maintain efi_loader code after every
> >>>> 2nd line in it ends up being some random map/unmap call ;).
> >>>>
> >>>> But my proposal was serious. Putting virtual addresses in is trivial and
> >>>> gives us proper API interfaces. We can based on that still decide whether
> >>>> it's better to not have pointers in the map. I am not convinced yet, but
> >>>> open to the idea.
> >>>>
> >>>> And yes, code maintainability is my uttermost goal of this. Things have to
> >>>> be simple for sandbox, otherwise they will break.
> >>>
> >>> A virtual address has a particular meaning. This is not what is going
> >>> on with sandbox...
> >>>
> >>> Making sandbox store its internal addresses in an EFI table is not a
> >>> good idea. I've tried to explain why. It is not more maintainable - it
> >>> just adds confusion. At present all ulong addresses are normal U-Boot
> >>> addresses and pointers point into the emulated RAM. It's pretty clear
> >>> for people. Please don't change that for EFI.
> >>
> >>
> >> If that's your concern, let's have the efi memory map contain pointers; because that's what it really does.
> >>
> >>> Can we get my patches applied and then add yours on top? I will send
> >>> v7 which I think addresses all comments.
> >>
> >>
> >> The patch set I just sent contains all patches from your set that I think we need, no?
> >
> > I don't think so, you are missing several. I would like to resolve
> > comments and get those applied and then move forward with expanding
> > the feature set. At least that's how it normally works in U-Boot - we
> > don't normally have the maintainer modify the patch series to his
> > liking :-)
>
> True, usually I would just ignore patches after my comments weren't
> address 5 times in a row ;).
What are you talking about? I only missed one comment on v5, for one
patch, which was creating the function, now done in v7. Please
explain.
>
> In all seriousness, I want us to move this forward properly. So what
> patches from your set are missing?
>
>
> [U-Boot,v7,10/10] efi: Rename bootefi_test_finish() to bootefi_run_finish()
> [U-Boot,v7,09/10] efi: Create a function to set up for running EFI code
> [U-Boot,v7,08/10] efi: sandbox: Add a simple 'bootefi test' command
> [U-Boot,v7,07/10] efi: Split out test init/uninit into functions
>
> I don't want to introduce any new commands or do refactoring at this
> point. Let's make the selftest work first.
>
> [U-Boot,v7,06/10] efi: sandbox: Enable EFI loader build for sandbox
>
> Included
>
> [U-Boot,v7,05/10] efi: sandbox: Add relocation constants
>
> Included
>
> [U-Boot,v7,04/10] efi: sandbox: Add distroboot support
>
> Included
>
> [U-Boot,v7,03/10] sandbox: smbios: Update to support sandbox
>
> Dropped because it's no longer needed.
Actually this one should be applied. These are addresses so we should
not be passing sandbox pointers in here.
>
> [U-Boot,v7,02/10] efi: sandbox: Adjust memory usage for sandbox
>
> Dropped because it's no longer needed.
Same here
>
> [U-Boot,v7,01/10] efi: Don't allow CMD_BOOTEFI_SELFTEST on sandbox
>
> Replaced with Heinrich's other proposal
>
>
> So I think all patches that should be in are in?
I'd like to get 7-10 in as well. The 'bootefi test' thing is useful as
a sanity check. Or are you saying you don't want that simple test?
Otherwise you should not be changing the ordering as a maintainer.
The main issue is the sandbox memory mapping stuff, which we still disagree on.
Regards,
Simon
More information about the U-Boot
mailing list