[U-Boot] [PATCH v8 22/30] efi: sandbox: Tidy up copy_fdt() to work with sandbox

[Alexander Graf]

On 22.06.18 21:31, Simon Glass wrote:
> Hi Alex,
> 
> On 22 June 2018 at 06:26, Alexander Graf <agraf at suse.de> wrote:
>> On 06/21/2018 09:45 PM, Simon Glass wrote:
>>>
>>> Hi Alex,
>>>
>>> On 21 June 2018 at 04:13, Alexander Graf <agraf at suse.de> wrote:
>>>>
>>>> On 06/21/2018 04:01 AM, Simon Glass wrote:
>>>>>
>>>>> Hi Alex,
>>>>>
>>>>> On 18 June 2018 at 09:00, Alexander Graf <agraf at suse.de> wrote:
>>>>>>
>>>>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>>>>
>>>>>>> At present this function takes a pointer as its argument, then passes
>>>>>>> this
>>>>>>> to efi_allocate_pages(), which actually takes an address. It uses
>>>>>>> casts,
>>>>>>> which are not supported on sandbox.
>>>>>>
>>>>>>
>>>>>> I think this is the big API misunderstanding that caused our
>>>>>> disagreements:
>>>>>> efi_allocate_pages(uint64_t *memory) gets a uint64_t *address as input
>>>>>> parameter, but uses the same field to actually return a void * pointer.
>>>>>> This
>>>>>> is the function that really converts between virtual and physical
>>>>>> address
>>>>>> space.
>>>>>>
>>>>>> This is the explicit wording of the spec[1] page 168:
>>>>>>
>>>>>>     The AllocatePages() function allocates the requested number of
>>>>>> pages
>>>>>> and
>>>>>> returns a pointer to the base address of the page range in the location
>>>>>> referenced by Memory.
>>>>>
>>>>> The code at present uses *Memory as the address on both input and
>>>>> output. The spec is confusing, but I suspect that is what it meant.
>>>>
>>>>
>>>> The spec means *Memory on IN is an address, on OUT is a pointer. It's
>>>> quite
>>>> clear on that. Since the functions is available to payloads, we should
>>>> follow that semantic.
>>>>
>>>>>> So yes, we have to cast. There is no other way around it without
>>>>>> completely
>>>>>> creating a trainwreck of the API.
>>>>>
>>>>> efi_allocate_pages_ext() can do this though. We don't need to copy the
>>>>> API to efi_allocate_pages().
>>>>
>>>>
>>>> Yikes. There's no way we'll create a frankenstein not-really-almost EFI
>>>> API
>>>> inside U-Boot. Either we stick to the standard or we don't.
>>>
>>> The API is the _ext() function, right? We can rename the internal
>>> function if you like.
>>>
>>> In any case I think you have this confused. From the spec:
>>>
>>>       "Pointer to a physical address. On input, the way in which the
>>> address is used depends on the value of Type. See “Description” for
>>> more information. On output the address is set to the base of the page
>>> range that was allocated. See “Related Definitions.”"
>>>
>>> The parameter does not turn into a pointer on exit. It is an address,
>>> just as it is on input. What am I missing?
>>
>>
>> Just keep reading. A few lines further down:
>>
>>   The AllocatePages() function allocates the requested number of pages and
>> returns a pointer to the base address of the page range in the location
>> referenced by Memory.
>>
>> the spec explicitly says the function returns *a pointer to the base
>> address*. It doesn't return an address. It returns a pointer.
> 
> I think we must be talking at cross-purposes. Perhaps the spec is
> ambiguous since I read it one way and you read it another. From my
> side, it 'returns a pointer to the base address' says that the base
> address is written to the pointer, but perhaps they mean what you
> think they mean? But if so, it should be void **, not uint64_t *.

The problem is that void ** is wrong for the IN path, so I assume they
had to decide on one and went with uint64_t * because that's always
bigger. Sizeof(void*) might be smaller than uint64_t on 32bit platforms.

> In any case it doesn't matter. It returns a 64-bit value which is both
> a pointer and an address. There is no distinction from the EFI side.
> From the U-Boot sandbox side, we must provide a pointer (both on input
> and output) since EFI does not understand our internal RAM buffer
> offsets.

Yes, I guess we agree by now :).

>> Either way, I've applied the patch that calls map_sysmem() inside
>> efi_allocate_pages() to efi-next.
> 
> Which patch is that? Have I reviewed it?

It's this beauty:


https://github.com/agraf/u-boot/commit/6f2ac2061ea499c4d23f407798b96b868cb2c3b4


Alex