On 05.09.2018 16:57, Denis Ryndine wrote: > Hello there, > > The issues (both) looks like were raised over a year ago by > henri.roosen@xxxxxxxxxxxxx. > See here: > https://lists.gt.net/linux/kernel/2684252?search_string=rproc_elf_load_segments;#2684252 Hm, indeed, the very same issue. And it seems that at least Henri came to a similar conclusion than I did ("Remoteproc might think of detecting and reject loading such ELF's.")... The kernel.org link for the discussion: https://lore.kernel.org/lkml/1493813529-19184-1-git-send-email-henri.roosen@xxxxxxxxxxxxx/T/#u I think a warning along with not zeroing out would be ideal... -- Stefan > > But the suggested fixes for both - filesz and memset - were curbed, ref Bjorn. > > Is this unfortunate .. ? > > Denis Ryndine | Lead Software Engineer | Embedded Synergy (Pty) Ltd | > P.O. Box 55874 | Polokwane | Limpopo | 0700 | South Africa > T +27110837168 > dry@xxxxxxxxxxxxxxxxxxxxxx | www.embedded-synergy.co.za > > > On Thu, Aug 30, 2018 at 10:45 PM, Stefan Agner <stefan@xxxxxxxx> wrote: >> Hi there, >> >> On 23.08.2018 16:12, Felix Siegel wrote: >>> Hi Denis, >>> >>> On Thu, 23 Aug 2018 11:28:08 +1000 >>> Denis Ryndine <dry@xxxxxxxxxxxxxxxxxxxxxx> wrote: >>> >>>> Hello Suman, >>>> >>>> On Thu, Aug 23, 2018 at 6:50 AM, Suman Anna <s-anna@xxxxxx> wrote: >>>> > >>>> > Hi Denis, >>>> > >>>> > On 08/20/2018 12:40 AM, Denis Ryndine wrote: >>>> > > Hello, >>>> > > >>>> > > The following may look like an error, could someone review. >>>> > > >>>> > > In rproc_elf_load_segments: >>>> > > >>>> > > /* grab the kernel address for this device address */ >>>> > > ptr = rproc_da_to_va(rproc, da, memsz); >>>> > > >>>> > > The last parameter should be filesz. Otherwise this call may fail, as >>>> > > the case when da is an address within a segment / memory. >>>> > > (E.g. placing a RO data after the code within text segment /memory ). >>>> > >>>> > No, it's alright, memsz >= filesz usually. The actual loadable content >>>> >>>> No issue here. >>>> >>>> > will be filesz, the rest is zero initialized. Both these are from the >>>> > program headers. Have you seen some issues around this? >>>> >>>> If the da points to the beginning of the segment or the device's >>>> memory then it's all good. But da can point somewhere with-in or at >>>> the end of the previous memory segment, where there is enough room to >>>> fit filesz. >>>> The check above may fail using memsz (memsz >= filesz) if there isn't >>>> physical memory left to fit memsz, but for >= filesz. >>>> >>>> Consider an OCRAM linked firmware (for iMX), with elf : >>>> >>>> Program Headers: >>>> Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align >>>> LOAD 0x001000 0x00000000 0x00000000 0x00240 0x00240 R 0x1000 >>>> LOAD 0x002000 0x00910000 0x00910000 0x0c2e0 0x0c2e0 RWE 0x1000 >>>> LOAD 0x00f000 0x20220000 0x0091c2e0 0x00210 0x072e0 RW 0x1000 >>>> >>>> The grab kernel address fails trying grab too much: >>>> remoteproc remoteproc0: bad phdr da 0x91c2e0 mem 0x72e0 >>>> >>>> But it shouldn't, as there is enough space in that memory for the >>>> filesz, which is what to be programmed into it, not the memsz. >>>> >>>> For iMX, for example, the device specific rproc_da_to_va() would have >>>> resolved the needed kernel address, if filesz would have been passed. >>>> See imx_rproc.c - imx_rproc_da_to_va() -> imx_rproc_da_to_sys(), >>>> which would return the needed address: there is enough in that memory >>>> block for filesz, but not for memsz. >>> >>> I had a similar problem with the iMX7 working with TCM. >>> Your fix would probably work however this only occurs due to strange >>> behaviour in the NXP linker and startup files. >>> The NXP linker file stores the data segment directly behind the code >>> segment in the code memory region >>> (causing the difference between VirtAddr and PhysAddr) and the startup >>> assembly then loads the data segment into the data memory region. >> >> I also worked with the i.MX 7 TCM linker file, and I agree, in the >> context of remoteproc etc the linker file does really >> unnecessary/strange section placements. >> >> I guess this comes from the microcontroller world, there memory is >> volatile and the firmware initialization code loads the .data section >> into memory from ROM. >> >> That said, the difference in VirtAddr and PhysAddr is caused by the `AT` >> keyword in the linker file: >> https://sourceware.org/binutils/docs/ld/Output-Section-LMA.html >> >> From what I can tell, because remoteproc uses paddr as base and memsz as >> length, remoteproc makes the assumption that the virtual and physical >> addressing is fully aligned... For a lot of linker files this is >> probably a reasonable assumption since we do not *need* startup code >> which relocates sections... >> >> However, if we make this assumption, maybe we should check if paddr and >> vaddr are really aligned, e.g. by using: >> >> WARN_ON(phdr->p_paddr != phdr->p_vaddr) >> >> Or, we could not zero out in case paddr/vaddr are not aligned, just to >> be on the safe side e.g.: >> >> --- a/drivers/remoteproc/remoteproc_elf_loader.c >> +++ b/drivers/remoteproc/remoteproc_elf_loader.c >> @@ -200,7 +200,7 @@ int rproc_elf_load_segments(struct rproc *rproc, >> const struct firmware *fw) >> * did this for us. albeit harmless, we may consider >> removing >> * this. >> */ >> - if (memsz > filesz) >> + if (phdr->p_paddr == phdr->p_vaddr && memsz > filesz) >> memset(ptr + filesz, 0, memsz - filesz); >> } >> >> >>> >>> This would make sense for a normal microcontroller with persistent >>> memory to boot from, but atleast on the imx7 the M4 requires the A7 to >>> start it >>> and the supported memory regions are all volatile anyway. >>> >>> After I changed the linker script and the startup routine it worked >>> for me. It also avoids needlessly copying data around. >> >> True, and that is what we ended up doing to. >> >> Still, maybe the kernel could behave a bit smarter. >> >> -- >> Stefan >> >>>> >>>> >>>> > regards >>>> > Suman >>>> >>>> Regards, >>>> >>>> Denis >>> >>> Regards, >>> >>> Felix
