[U-Boot] [PATCH v6 03/13] efi: sandbox: Adjust memory usage for sandbox
Simon Glass
sjg at chromium.org
Wed Jun 20 17:51:52 UTC 2018
Hi Alex,
On 14 June 2018 at 13:32, Alexander Graf <agraf at suse.de> wrote:
>
>
> On 14.06.18 21:02, Simon Glass wrote:
>> 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.
>
> I thought I had expressed my dislike for the current memory mapping
> model a few times by now. Maybe I wasn't explicit enough?
I was pretty clear that I don't want to change the sandbox memory
model, so that's why I didn't do anything on that point. I don't think
I could have been any clearer.
>
>>> 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.
>
> Well, maybe. But then we'd have to adjust for the API the other way
> around, as now pointers are what allocations return (because they're a
> payload visible API, so they have to).
Actually the function I am talking about is internal to U-Boot. The
_ext() function can do the mapping as needed, as per my latest series.
But I feel it can still be improved, since addresses and pointers are
still a little mixed together in the EFI code.
>
>>
>>>
>>> [U-Boot,v7,02/10] efi: sandbox: Adjust memory usage for sandbox
>>>
>>> Dropped because it's no longer needed.
>>
>> Same here
>
> Why does it hurt to expose the U-Boot and RTS memory addresses?
What is an RTS memory address?
>
>>
>>>
>>> [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?
>
> I want a simple test, but I think the simple test is bootefi selftest.
> You can limit its scope to only emit "hello world" by setting an
> environment variable if you want to :).
I've reviewed selftest as it was built up and again recently. It is
certainly not a simple sanity chec, which is what hello word is for.
>
>> 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.
>
> I guess we agree that we disagree :).
>
> Looking at the current code base, we pretty much only call the map/unmap
> dance every time we go from command line arguments (where you pass in
> u-boot addresses) to pointers. Anything internal is usually dealing with
> pointers.
That's just not the case, as I have explained. Images in U-Boot,
device tree, frame buffer, all peripheral's register locations, bootm,
FIT and many other things are dealt with with addresses. U-Boot uses
addresses for addresses, not pointers. The pointers are created from
the addresses, but are not the primary means for communicating
addresses.
>
> I think that's a reasonable thing to do. And I think we should follow
> the same logic in the efi_loader parts. Command line parameters can
> easily be handled via map/unmap, but anything internal - and the efi
> memory map is what I consider internal - should just contain pointers.
>
> The only reason they're not marked as pointers in the struct is that we
> want to copy the structs right back into the payload and there they are
> ABI defined as 64bit fields.
I still strongly disagree with this. I would like to get the sandbox
feature in without changing how sandbox works.
Regards,
Simon
More information about the U-Boot
mailing list