On 05.09.2018 18:23, Denis Ryndine wrote: > 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) . We treat firmware elfs differently depending on whether they use different/same Phys/Virt addresses... At least a information message might be appropriate? How about: /* * elf files linked with the AT attribute might have different phys/virt address * mapping. Typically this is used to store initialization data on ROM, and the * firmware relocates the data (filesz) into its final place (virt memory address). * In this case it is not safe to zero out the physical address space up to memsz. */ bool virt_differ = phdr->p_paddr != phdr->p_vaddr; if (virt_differ) pr_info("Not zeroing out remaining memory due to phys/virt memory difference\n"); ptr = rproc_da_to_va(rproc, da, virt_differ ? filesz : memsz); ... /* * 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 (!virt_differ && memsz > filesz) memset(ptr + filesz, 0, memsz - filesz); -- Stefan > > 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
