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