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?
Alex