On 22.06.18 21:31, Simon Glass wrote: > Hi Alex, > > On 22 June 2018 at 06:26, Alexander Graf <[email protected]> wrote: >> On 06/21/2018 09:45 PM, Simon Glass wrote: >>> >>> Hi Alex, >>> >>> On 21 June 2018 at 04:13, Alexander Graf <[email protected]> wrote: >>>> >>>> On 06/21/2018 04:01 AM, Simon Glass wrote: >>>>> >>>>> Hi Alex, >>>>> >>>>> On 18 June 2018 at 09:00, Alexander Graf <[email protected]> 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 _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
