On Thu, Sep 6, 2018 at 11:43 AM, Stefan Agner <stefan@xxxxxxxx> wrote: > 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... Ok, because of (optional anyway?) clearing memory in most cases and not in this one. > At least a information message might be appropriate? If nothing else allowed :). > > 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); > ... > Yes, prefer to try to load it and not just warn and exit. Denis > /* > * 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
