Hi On Thu, Sep 6, 2018 at 10:58 AM, Stefan Agner <stefan@xxxxxxxx> wrote: > 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... Why not allow such elfs? With the 2 fixes, they should be loadable & runnable (but I haven't done tests on the NXP's sample elfs for M4 using the rpoc) . What gets broken by allowing it ? Denis > > -- > Stefan > >> >> But the suggested fixes for both - filesz and memset - were curbed, ref Bjorn. >> >> Is this unfortunate .. ? >> >> >> 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
