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

[Simon Glass]

Hi Alex,

On 23 June 2018 at 01:24, Alexander Graf <agraf at suse.de> wrote:
>
>
> 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

OK thanks, I replied on that. I do think the address handling is
confusing, but we can worry about that in separate patches.

Regards,
Simon