[U-Boot] [PATCH v6 03/13] efi: sandbox: Adjust memory usage for sandbox
Alexander Graf
agraf at suse.de
Thu Jun 14 18:05:35 UTC 2018
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 ;).
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.
[U-Boot,v7,02/10] efi: sandbox: Adjust memory usage for sandbox
Dropped because it's no longer needed.
[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?
Alex
More information about the U-Boot
mailing list