Hi Alex,
On 23 June 2018 at 01:24, Alexander Graf agraf@suse.de wrote:
On 22.06.18 21:31, Simon Glass wrote:
Hi Alex,
On 22 June 2018 at 06:26, Alexander Graf agraf@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@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@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 theaddress 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/6f2ac2061ea499c4d23f407798b96b868cb2c...
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