[U-Boot] [PATCH v6 03/13] efi: sandbox: Adjust memory usage for sandbox
Simon Glass
sjg at chromium.org
Thu Jun 14 17:15:29 UTC 2018
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 :-)
Regards,
Simon
More information about the U-Boot
mailing list