| Message ID | 1493813529-19184-1-git-send-email-henri.roosen@ginzinger.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On Wed 03 May 05:12 PDT 2017, Henri Roosen wrote: > Consider a system with 2 memory regions: > 0x1fff8000 - 0x1fffffff: iram So I presume there's a hole here. > 0x21000000 - 0x21007fff: dram > > The .elf file for this system contains the following Program Headers: > Program Headers: > Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align > LOAD 0x000094 0x1fff8000 0x1fff8000 0x00240 0x00240 R 0x4 > LOAD 0x0002e0 0x1fff8240 0x1fff8240 0x03d1c 0x03d1c RWE 0x10 > LOAD 0x003ffc 0x21000000 0x1fffbf5c 0x001cc 0x048a0 RW 0x4 > Your ELF header states that there is a segment of 0x48a0 bytes starting at 0x1fffbf5c, but your iram ends after 0x40a3 bytes. I assume your MemSiz comes from some linker script, which would mean that your firmware expects to be able to use all 0x48a0 bytes, which should be invalid. > Section to Segment mapping: > Segment Sections... > 00 .interrupts > 01 .text .ARM .init_array .fini_array > 02 .data .bss .heap .stack > > The problem is with the 3rd segment: it has 0x1cc bytes of ROM .data > which need to be placed at PhysAddr 0x1fffbf5c. Using MemSiz as len > parameter for rproc_da_to_va is incorrect (goes beyond 0x1fffffff). The > correct len parameter to be used here is FileSiz. > The fact that MemSiz is larger than FileSiz makes sense because you have .bss, .heap and .stack there - which we conveniently clear for you. > The actual memcpy of the segment was already correctly using the FileSiz > for length, however the unnecessary "Zero out remaining memory" would > write beyond the 0x1fffffff end of the memory region! This patch removes > the harmful code. > Perhaps I'm missing something, but I think the problem is that your memory map is broken. We do want to clear out the remaining part of each segment. Note though that we don't yet have means to direct your carveout allocations to the two segments and get the firmware loaded at the physical addresses specified. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/11/2017 02:05 AM, Bjorn Andersson wrote: > On Wed 03 May 05:12 PDT 2017, Henri Roosen wrote: > >> Consider a system with 2 memory regions: >> 0x1fff8000 - 0x1fffffff: iram > > So I presume there's a hole here. > >> 0x21000000 - 0x21007fff: dram >> >> The .elf file for this system contains the following Program Headers: >> Program Headers: >> Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align >> LOAD 0x000094 0x1fff8000 0x1fff8000 0x00240 0x00240 R 0x4 >> LOAD 0x0002e0 0x1fff8240 0x1fff8240 0x03d1c 0x03d1c RWE 0x10 >> LOAD 0x003ffc 0x21000000 0x1fffbf5c 0x001cc 0x048a0 RW 0x4 >> > > Your ELF header states that there is a segment of 0x48a0 bytes starting > at 0x1fffbf5c, but your iram ends after 0x40a3 bytes. I assume your > MemSiz comes from some linker script, which would mean that your > firmware expects to be able to use all 0x48a0 bytes, which should be > invalid. I had a closer look at the linker script. The .data section uses the "AT"-keyword to place the initialized .data right after the .text section (0x1fffbf5c/PhysAddr). Some run-time startup-code is responsible for copying the initialized data to its runtime address (0x21000000/VirtAddr). The run-time startup-code is also responsible for zero-ing the .bss section. The size of the initialized data is 0x1cc (FileSiz). The size of the whole 3rd segment at run-time is 0x048a0 (MemSiz), starting from 0x21000000 (VirtAddr), which also includes the .bss .heap and .stack sections. > >> Section to Segment mapping: >> Segment Sections... >> 00 .interrupts >> 01 .text .ARM .init_array .fini_array >> 02 .data .bss .heap .stack >> >> The problem is with the 3rd segment: it has 0x1cc bytes of ROM .data >> which need to be placed at PhysAddr 0x1fffbf5c. Using MemSiz as len >> parameter for rproc_da_to_va is incorrect (goes beyond 0x1fffffff). The >> correct len parameter to be used here is FileSiz. >> > > The fact that MemSiz is larger than FileSiz makes sense because you have > .bss, .heap and .stack there - which we conveniently clear for you. I agree the value of MemSiz being larger than FileSiz is correct; this is the case when there are uninitialized data sections like .bss. In my opinion this reflects the run-time size of the segment, starting from address 0x21000000. However the remoteproc-elf-loader is clearing with the load-time-address (0x1fffbf5c) as base. Note that the linker-scripts which are not using the "AT"-keyword have the VirtAddr and PhysAddr to be the same, so would not cause trouble; the remoteproc-elf-loader seems to have a problem when VirtAddr and PhysAddr have different values. > >> The actual memcpy of the segment was already correctly using the FileSiz >> for length, however the unnecessary "Zero out remaining memory" would >> write beyond the 0x1fffffff end of the memory region! This patch removes >> the harmful code. >> > > Perhaps I'm missing something, but I think the problem is that your > memory map is broken. We do want to clear out the remaining part of each > segment. I'm not sure it's the memory map which is broken. The "AT"-keyword should be valid to use I guess. However, remoteproc might decide not to support such elf-files. A detection for equality of VirtAddr and PhysAddr could then be used for that. Please find below some dummy test code (which leaves out the initialization code for copying .data and clearing .bss) and a linker script to reproduce: test.ld ------------ SECTIONS { . = 0x1fff0000; .text : { *(.text); } __flash_sdata = .; . = 0x21000000; .data : AT (__flash_sdata) { *(.data); } .bss : { *(.bss) *(COMMON); } } test.s ------------ .bss uninit_data: .space 0x8000-8 .data data1: .4byte 1 data2: .4byte 2 .text _start: b _start reproduce ------------ arm-none-eabi-as -o test.o test.s arm-none-eabi-ld -T test.ld -o test.elf test.o arm-none-eabi-objdump -x test.elf > > > > Note though that we don't yet have means to direct your carveout > allocations to the two segments and get the firmware loaded at the > physical addresses specified. I'm currently using the .da_to_va callback and translate with offsets read by custom devicetree bindings. Regards, Henri > > Regards, > Bjorn > -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu 11 May 09:12 PDT 2017, Henri Roosen wrote: > On 05/11/2017 02:05 AM, Bjorn Andersson wrote: > > On Wed 03 May 05:12 PDT 2017, Henri Roosen wrote: > > > > > Consider a system with 2 memory regions: > > > 0x1fff8000 - 0x1fffffff: iram > > > > So I presume there's a hole here. > > > > > 0x21000000 - 0x21007fff: dram > > > > > > The .elf file for this system contains the following Program Headers: > > > Program Headers: > > > Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align > > > LOAD 0x000094 0x1fff8000 0x1fff8000 0x00240 0x00240 R 0x4 > > > LOAD 0x0002e0 0x1fff8240 0x1fff8240 0x03d1c 0x03d1c RWE 0x10 > > > LOAD 0x003ffc 0x21000000 0x1fffbf5c 0x001cc 0x048a0 RW 0x4 > > > > > > > Your ELF header states that there is a segment of 0x48a0 bytes starting > > at 0x1fffbf5c, but your iram ends after 0x40a3 bytes. I assume your > > MemSiz comes from some linker script, which would mean that your > > firmware expects to be able to use all 0x48a0 bytes, which should be > > invalid. > > I had a closer look at the linker script. The .data section uses the > "AT"-keyword to place the initialized .data right after the .text section > (0x1fffbf5c/PhysAddr). > > Some run-time startup-code is responsible for copying the initialized data > to its runtime address (0x21000000/VirtAddr). The run-time startup-code is > also responsible for zero-ing the .bss section. > > The size of the initialized data is 0x1cc (FileSiz). The size of the whole > 3rd segment at run-time is 0x048a0 (MemSiz), starting from 0x21000000 > (VirtAddr), which also includes the .bss .heap and .stack sections. > [1] specifies that p_memsz is the size of the memory segment and that the difference between p_filesz and p_memsz are defined to hold the value 0. So I believe that your segment list states that the physical range 0x1fffbf5c through 0x200007fc is valid and should be populated. [1] https://refspecs.linuxfoundation.org/elf/elf.pdf Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/14/2017 06:14 AM, Bjorn Andersson wrote: > On Thu 11 May 09:12 PDT 2017, Henri Roosen wrote: > >> On 05/11/2017 02:05 AM, Bjorn Andersson wrote: >>> On Wed 03 May 05:12 PDT 2017, Henri Roosen wrote: >>> >>>> Consider a system with 2 memory regions: >>>> 0x1fff8000 - 0x1fffffff: iram >>> >>> So I presume there's a hole here. >>> >>>> 0x21000000 - 0x21007fff: dram >>>> >>>> The .elf file for this system contains the following Program Headers: >>>> Program Headers: >>>> Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align >>>> LOAD 0x000094 0x1fff8000 0x1fff8000 0x00240 0x00240 R 0x4 >>>> LOAD 0x0002e0 0x1fff8240 0x1fff8240 0x03d1c 0x03d1c RWE 0x10 >>>> LOAD 0x003ffc 0x21000000 0x1fffbf5c 0x001cc 0x048a0 RW 0x4 >>>> >>> >>> Your ELF header states that there is a segment of 0x48a0 bytes starting >>> at 0x1fffbf5c, but your iram ends after 0x40a3 bytes. I assume your >>> MemSiz comes from some linker script, which would mean that your >>> firmware expects to be able to use all 0x48a0 bytes, which should be >>> invalid. >> >> I had a closer look at the linker script. The .data section uses the >> "AT"-keyword to place the initialized .data right after the .text section >> (0x1fffbf5c/PhysAddr). >> >> Some run-time startup-code is responsible for copying the initialized data >> to its runtime address (0x21000000/VirtAddr). The run-time startup-code is >> also responsible for zero-ing the .bss section. >> >> The size of the initialized data is 0x1cc (FileSiz). The size of the whole >> 3rd segment at run-time is 0x048a0 (MemSiz), starting from 0x21000000 >> (VirtAddr), which also includes the .bss .heap and .stack sections. >> > > [1] specifies that p_memsz is the size of the memory segment and > that the difference between p_filesz and p_memsz are defined to hold the > value 0. I think the main problem is that there is no specification how to deal with ELF files which are linked with an AT-attribute in a segment. In that case VirtAddr and PhysAddr differ. Then it's unclear whether to use VirtAddr or PhysAddr for zeroing (or even whether zeroing should be done at all). IMHO, the way I interpret the specification, zeroing (and loading in general) should be done using VirtAddr. However, the spec [1] is not very clear on this. Zeroing using PhysAddr might not cause problems on ELF files without AT-keyword using GNU-linkers which have PhysAddr and VirtAddr to be equal, but I'm not sure if all linkers generate PhysAddr and VirtAddr equal. My patch is clearly not the correct fix for "AT-linked-ELF's", so please ignore it. The AT-keyword is actually mainly used when linking for micro-controllers with FLASH/ROM and I can remove it from my linker-file for the remoteproc-firmware generation. Remoteproc might think of detecting and reject loading such ELF's. > > So I believe that your segment list states that the physical range > 0x1fffbf5c through 0x200007fc is valid and should be populated. As explained above, IMHO the segment list states that the range 0x21000000 through 0x210048a0 is valid. This range would also fit the actual memory region. (And for the AT-keyword to work, 0x1cc bytes should be populated at 0x1fffbf5c, but I don't know of any specification for this). Regards, Henri > > [1] https://refspecs.linuxfoundation.org/elf/elf.pdf > > Regards, > Bjorn > -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon 15 May 07:37 PDT 2017, Henri Roosen wrote: > On 05/14/2017 06:14 AM, Bjorn Andersson wrote: > > On Thu 11 May 09:12 PDT 2017, Henri Roosen wrote: > > > > > On 05/11/2017 02:05 AM, Bjorn Andersson wrote: > > > > On Wed 03 May 05:12 PDT 2017, Henri Roosen wrote: > > > > > > > > > Consider a system with 2 memory regions: > > > > > 0x1fff8000 - 0x1fffffff: iram > > > > > > > > So I presume there's a hole here. > > > > > > > > > 0x21000000 - 0x21007fff: dram > > > > > > > > > > The .elf file for this system contains the following Program Headers: > > > > > Program Headers: > > > > > Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align > > > > > LOAD 0x000094 0x1fff8000 0x1fff8000 0x00240 0x00240 R 0x4 > > > > > LOAD 0x0002e0 0x1fff8240 0x1fff8240 0x03d1c 0x03d1c RWE 0x10 > > > > > LOAD 0x003ffc 0x21000000 0x1fffbf5c 0x001cc 0x048a0 RW 0x4 > > > > > > > > > > > > > Your ELF header states that there is a segment of 0x48a0 bytes starting > > > > at 0x1fffbf5c, but your iram ends after 0x40a3 bytes. I assume your > > > > MemSiz comes from some linker script, which would mean that your > > > > firmware expects to be able to use all 0x48a0 bytes, which should be > > > > invalid. > > > > > > I had a closer look at the linker script. The .data section uses the > > > "AT"-keyword to place the initialized .data right after the .text section > > > (0x1fffbf5c/PhysAddr). > > > > > > Some run-time startup-code is responsible for copying the initialized data > > > to its runtime address (0x21000000/VirtAddr). The run-time startup-code is > > > also responsible for zero-ing the .bss section. > > > > > > The size of the initialized data is 0x1cc (FileSiz). The size of the whole > > > 3rd segment at run-time is 0x048a0 (MemSiz), starting from 0x21000000 > > > (VirtAddr), which also includes the .bss .heap and .stack sections. > > > > > > > [1] specifies that p_memsz is the size of the memory segment and > > that the difference between p_filesz and p_memsz are defined to hold the > > value 0. > > I think the main problem is that there is no specification how to deal with > ELF files which are linked with an AT-attribute in a segment. In that case > VirtAddr and PhysAddr differ. Then it's unclear whether to use VirtAddr or > PhysAddr for zeroing (or even whether zeroing should be done at all). > > IMHO, the way I interpret the specification, zeroing (and loading in > general) should be done using VirtAddr. However, the spec [1] is not very > clear on this. Zeroing using PhysAddr might not cause problems on ELF files > without AT-keyword using GNU-linkers which have PhysAddr and VirtAddr to be > equal, but I'm not sure if all linkers generate PhysAddr and VirtAddr equal. > I believe that the cases where VirtAddr != PhysAddr you're supposed to have some sort of 1:1 mapping between them - e.g. using an MMU. I would like to stick with this belief, but am not sure if this is the defined behavior... Sorry about that. > My patch is clearly not the correct fix for "AT-linked-ELF's", so please > ignore it. The AT-keyword is actually mainly used when linking for > micro-controllers with FLASH/ROM and I can remove it from my linker-file for > the remoteproc-firmware generation. Remoteproc might think of detecting and > reject loading such ELF's. > We load the ELF files in three steps, first we do a sanity check of the header, then we parse and act on the resource table and finally we iterate over the segments loading things into ram. The second step will currently "define" all the memory regions, so we can't verify their sanity in the first step. So what you're seeing is the ELF loader failing due to insufficient space. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c index c523983..3fa159a 100644 --- a/drivers/remoteproc/remoteproc_elf_loader.c +++ b/drivers/remoteproc/remoteproc_elf_loader.c @@ -183,9 +183,10 @@ rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw) } /* grab the kernel address for this device address */ - ptr = rproc_da_to_va(rproc, da, memsz); + ptr = rproc_da_to_va(rproc, da, filesz); if (!ptr) { - dev_err(dev, "bad phdr da 0x%x mem 0x%x\n", da, memsz); + dev_err(dev, "bad phdr da 0x%x filesz 0x%x\n", + da, filesz); ret = -EINVAL; break; } @@ -193,16 +194,6 @@ rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw) /* put the segment where the remote processor expects it */ if (phdr->p_filesz) memcpy(ptr, elf_data + phdr->p_offset, filesz); - - /* - * Zero out remaining memory for this segment. - * - * This isn't strictly required since dma_alloc_coherent already - * did this for us. albeit harmless, we may consider removing - * this. - */ - if (memsz > filesz) - memset(ptr + filesz, 0, memsz - filesz); } return ret;
Consider a system with 2 memory regions: 0x1fff8000 - 0x1fffffff: iram 0x21000000 - 0x21007fff: dram The .elf file for this system contains the following Program Headers: Program Headers: Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align LOAD 0x000094 0x1fff8000 0x1fff8000 0x00240 0x00240 R 0x4 LOAD 0x0002e0 0x1fff8240 0x1fff8240 0x03d1c 0x03d1c RWE 0x10 LOAD 0x003ffc 0x21000000 0x1fffbf5c 0x001cc 0x048a0 RW 0x4 Section to Segment mapping: Segment Sections... 00 .interrupts 01 .text .ARM .init_array .fini_array 02 .data .bss .heap .stack The problem is with the 3rd segment: it has 0x1cc bytes of ROM .data which need to be placed at PhysAddr 0x1fffbf5c. Using MemSiz as len parameter for rproc_da_to_va is incorrect (goes beyond 0x1fffffff). The correct len parameter to be used here is FileSiz. The actual memcpy of the segment was already correctly using the FileSiz for length, however the unnecessary "Zero out remaining memory" would write beyond the 0x1fffffff end of the memory region! This patch removes the harmful code. Signed-off-by: Henri Roosen <henri.roosen@ginzinger.com> --- drivers/remoteproc/remoteproc_elf_loader.c | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-)