| Message ID | 20180618140835.195901-23-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Alexander Graf |
| Headers | show |
| Series | efi: Enable sandbox support for EFI loader | expand |
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. So yes, we have to cast. There is no other way around it without completely creating a trainwreck of the API. Alex [1] http://www.uefi.org/sites/default/files/resources/UEFI%20Spec%202_7_A%20Sept%206.pdf
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. > > 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(). Of course this is future possible work, we don't need to do it now. > > > Alex > > [1] > http://www.uefi.org/sites/default/files/resources/UEFI%20Spec%202_7_A%20Sept%206.pdf > Regards, Simon
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. Alex
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 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? Regards, Simon
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 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. Either way, I've applied the patch that calls map_sysmem() inside efi_allocate_pages() to efi-next. Alex
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 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 *. 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. > > 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? Regards, Simon
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 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
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 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
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index b62635f448..5ef2d8499c 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -119,17 +119,30 @@ static void set_load_options(struct efi_loaded_image *loaded_image_info, loaded_image_info->load_options_size = size * 2; } -static void *copy_fdt(void *fdt) +/** + * copy_fdt() - Copy the device tree to a new location available to EFI + * + * The FDT is relocated into a suitable location within the EFI memory map. + * An additional 12KB is added to the space in case the device tree needs to be + * expanded later with fdt_open_into(). + * + * @fdt_addr: On entry, address of start of FDT. On exit, address of relocated + * FDT start + * @fdt_sizep: Returns new size of FDT, including + * @return new relocated address of FDT + */ +static efi_status_t copy_fdt(ulong *fdt_addrp, ulong *fdt_sizep) { - u64 fdt_size = fdt_totalsize(fdt); unsigned long fdt_ram_start = -1L, fdt_pages; + efi_status_t ret = 0; + void *fdt, *new_fdt; u64 new_fdt_addr; - void *new_fdt; + uint fdt_size; int i; - for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) { - u64 ram_start = gd->bd->bi_dram[i].start; - u64 ram_size = gd->bd->bi_dram[i].size; + for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) { + u64 ram_start = gd->bd->bi_dram[i].start; + u64 ram_size = gd->bd->bi_dram[i].size; if (!ram_size) continue; @@ -142,30 +155,37 @@ static void *copy_fdt(void *fdt) * Give us at least 4KB of breathing room in case the device tree needs * to be expanded later. Round up to the nearest EFI page boundary. */ - fdt_size += 4096; + fdt = map_sysmem(*fdt_addrp, 0); + fdt_size = fdt_totalsize(fdt); + fdt_size += 4096 * 3; fdt_size = ALIGN(fdt_size + EFI_PAGE_SIZE - 1, EFI_PAGE_SIZE); fdt_pages = fdt_size >> EFI_PAGE_SHIFT; /* Safe fdt location is at 127MB */ new_fdt_addr = fdt_ram_start + (127 * 1024 * 1024) + fdt_size; - if (efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, - EFI_RUNTIME_SERVICES_DATA, fdt_pages, - &new_fdt_addr) != EFI_SUCCESS) { + ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, + EFI_RUNTIME_SERVICES_DATA, fdt_pages, + &new_fdt_addr); + if (ret != EFI_SUCCESS) { /* If we can't put it there, put it somewhere */ new_fdt_addr = (ulong)memalign(EFI_PAGE_SIZE, fdt_size); - if (efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, - EFI_RUNTIME_SERVICES_DATA, fdt_pages, - &new_fdt_addr) != EFI_SUCCESS) { + ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, + EFI_RUNTIME_SERVICES_DATA, fdt_pages, + &new_fdt_addr); + if (ret != EFI_SUCCESS) { printf("ERROR: Failed to reserve space for FDT\n"); - return NULL; + goto done; } } - new_fdt = (void*)(ulong)new_fdt_addr; + new_fdt = map_sysmem(new_fdt_addr, fdt_size); memcpy(new_fdt, fdt, fdt_totalsize(fdt)); fdt_set_totalsize(new_fdt, fdt_size); - return new_fdt; + *fdt_addrp = new_fdt_addr; + *fdt_sizep = fdt_size; +done: + return ret; } static efi_status_t efi_do_enter( @@ -215,22 +235,27 @@ static efi_status_t efi_carve_out_dt_rsv(void *fdt) return EFI_SUCCESS; } -static efi_status_t efi_install_fdt(void *fdt) +static efi_status_t efi_install_fdt(ulong fdt_addr) { bootm_headers_t img = { 0 }; ulong fdt_pages, fdt_size, fdt_start, fdt_end; efi_status_t ret; + void *fdt; + fdt = map_sysmem(fdt_addr, 0); if (fdt_check_header(fdt)) { printf("ERROR: invalid device tree\n"); return EFI_INVALID_PARAMETER; } /* Prepare fdt for payload */ - fdt = copy_fdt(fdt); - if (!fdt) - return EFI_OUT_OF_RESOURCES; + ret = copy_fdt(&fdt_addr, &fdt_size); + if (ret) + return ret; + unmap_sysmem(fdt); + fdt = map_sysmem(fdt_addr, 0); + fdt_size = fdt_totalsize(fdt); if (image_setup_libfdt(&img, fdt, 0, NULL)) { printf("ERROR: failed to process device tree\n"); return EFI_LOAD_ERROR; @@ -247,14 +272,13 @@ static efi_status_t efi_install_fdt(void *fdt) return EFI_OUT_OF_RESOURCES; /* And reserve the space in the memory map */ - fdt_start = ((ulong)fdt) & ~EFI_PAGE_MASK; - fdt_end = ((ulong)fdt) + fdt_totalsize(fdt); - fdt_size = (fdt_end - fdt_start) + EFI_PAGE_MASK; + fdt_start = fdt_addr; + fdt_end = fdt_addr + fdt_size; fdt_pages = fdt_size >> EFI_PAGE_SHIFT; - /* Give a bootloader the chance to modify the device tree */ - fdt_pages += 2; + ret = efi_add_memory_map(fdt_start, fdt_pages, EFI_BOOT_SERVICES_DATA, true); + return ret; } @@ -454,7 +478,6 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) unsigned long addr; efi_status_t r; unsigned long fdt_addr; - void *fdt; /* Allow unaligned memory access */ allow_unaligned(); @@ -475,8 +498,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) if (!fdt_addr && *argv[2] != '0') return CMD_RET_USAGE; /* Install device tree */ - fdt = map_sysmem(fdt_addr, 0); - r = efi_install_fdt(fdt); + r = efi_install_fdt(fdt_addr); if (r != EFI_SUCCESS) { printf("ERROR: failed to install device tree\n"); return CMD_RET_FAILURE;
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. Also the function calculates the FDT size rounded up to the neared EFI page size, then its caller recalculates the size and adds a bit more to it. This function is much better written as something that works with addresses only, and returns both the address and the size of the relocated FDT. Also, copy_fdt() returns NULL on error, but really should propagate the error from efi_allocate_pages(). To do this it needs to return an efi_status_t, not a void *. Update the code in this way, so that it is easier to follow, and also supports sandbox. Signed-off-by: Simon Glass <sjg@chromium.org> --- Changes in v8: None Changes in v7: None Changes in v6: None Changes in v5: None Changes in v4: None Changes in v3: None Changes in v2: None cmd/bootefi.c | 78 +++++++++++++++++++++++++++++++++------------------ 1 file changed, 50 insertions(+), 28 deletions(-)