rproc_elf_load_segments: rproc_elf_load_segments elf loading problem

rproc_elf_load_segments: rproc_elf_load_segments elf loading problem

[Denis Ryndine]

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 ).

Regards

Denis Ryndine

---
dry@embedded-synergy.co.za | www.embedded-synergy.co.za

Re: rproc_elf_load_segments: rproc_elf_load_segments elf loading problem

[Suman Anna]

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
will be filesz, the rest is zero initialized. Both these are from the
program headers. Have you seen some issues around this?

regards
Suman

Re: rproc_elf_load_segments: rproc_elf_load_segments elf loading problem

[Denis Ryndine]

Hello Suman,

On Thu, Aug 23, 2018 at 6:50 AM, Suman Anna <s-anna@ti.com> 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.


> regards
> Suman

Regards,

Denis

Re: rproc_elf_load_segments: rproc_elf_load_segments elf loading problem

[Felix Siegel]

Hi Denis,

On Thu, 23 Aug 2018 11:28:08 +1000
Denis Ryndine <dry@embedded-synergy.co.za> wrote:

> Hello Suman,
> 
> On Thu, Aug 23, 2018 at 6:50 AM, Suman Anna <s-anna@ti.com> 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.

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.
> 
> 
> > regards
> > Suman
> 
> Regards,
> 
> Denis

Regards,

Felix

Re: rproc_elf_load_segments: rproc_elf_load_segments elf loading problem

[Denis Ryndine]

Hello Felix,

On Fri, Aug 24, 2018 at 12:12 AM, Felix Siegel <felix.siegel@posteo.de> wrote:
> Hi Denis,
>
> On Thu, 23 Aug 2018 11:28:08 +1000
> Denis Ryndine <dry@embedded-synergy.co.za> wrote:
>
>> Hello Suman,
>>
>> On Thu, Aug 23, 2018 at 6:50 AM, Suman Anna <s-anna@ti.com> 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.

I understand, changing the layout of sections in linker script will
make this go away. It may seem though it shouldn't hurt, as it's
filesz bytes that gets programmed.

> The NXP linker file stores the data segment directly behind the code segment in the code memory region

I read it (script) that they only put initialization and RO/read-only
data directly behind text, within text segment. Organization wise this
looks nice, put text & other RO data together (at least for those apps
which don't modify it runtime)

> (causing the difference between VirtAddr and PhysAddr) and the startup assembly then loads the data segment into the data memory region.
>
> 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.

Oops, didn't think about the unnecessary copying of data by on the app start up.
However, may be because on iMXx you can lock / secure the text & RO
data memory from further modifications after it's been programmed, so
that could be advantageous?

Apologies with this iMX stuff discussion here, on the question about
general elf proc load.

Going back to rproc_elf_load_segments however, it's been pointed out
to me on another forum that there will be another problem with my
strange linker script & the elf it produces:


* 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);

This ain't going to be harmless for my case, as it will go cross
segments and may go cross physical memories.

>>
>> > regards
>> > Suman
>>
>> Regards,
>>
>> Denis
>
> Regards,
>
> Felix
>

Re: rproc_elf_load_segments: rproc_elf_load_segments elf loading problem

[Stefan Agner]

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@embedded-synergy.co.za> wrote:
> 
>> Hello Suman,
>>
>> On Thu, Aug 23, 2018 at 6:50 AM, Suman Anna <s-anna@ti.com> 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

Re: rproc_elf_load_segments: rproc_elf_load_segments elf loading problem

[Denis Ryndine]

Hi Stefan,

If you thus also add this change :

--- a/drivers/remoteproc/remoteproc_elf_loader.c
+++ b/drivers/remoteproc/remoteproc_elf_loader.c
@@ -182,7 +182,7 @@ int 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);
                        ret = -EINVAL;


Then the loader should be more immune even when the elf was generated
with questionable configs.

Denis

On Thu, Aug 30, 2018 at 10:45 PM, Stefan Agner <stefan@agner.ch> 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@embedded-synergy.co.za> wrote:
>>
>>> Hello Suman,
>>>
>>> On Thu, Aug 23, 2018 at 6:50 AM, Suman Anna <s-anna@ti.com> 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

Re: rproc_elf_load_segments: rproc_elf_load_segments elf loading problem

[Denis Ryndine]

Hello there,

The issues (both) looks like were raised over a year ago by
henri.roosen@ginzinger.com.
See here: https://lists.gt.net/linux/kernel/2684252?search_string=rproc_elf_load_segments;#2684252

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@embedded-synergy.co.za | www.embedded-synergy.co.za


On Thu, Aug 30, 2018 at 10:45 PM, Stefan Agner <stefan@agner.ch> 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@embedded-synergy.co.za> wrote:
>>
>>> Hello Suman,
>>>
>>> On Thu, Aug 23, 2018 at 6:50 AM, Suman Anna <s-anna@ti.com> 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

Re: rproc_elf_load_segments: rproc_elf_load_segments elf loading problem

[Stefan Agner]

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@ginzinger.com.
> 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@ginzinger.com/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@embedded-synergy.co.za | www.embedded-synergy.co.za
> 
> 
> On Thu, Aug 30, 2018 at 10:45 PM, Stefan Agner <stefan@agner.ch> 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@embedded-synergy.co.za> wrote:
>>>
>>>> Hello Suman,
>>>>
>>>> On Thu, Aug 23, 2018 at 6:50 AM, Suman Anna <s-anna@ti.com> 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

Re: rproc_elf_load_segments: rproc_elf_load_segments elf loading problem

[Denis Ryndine]

Hi

On Thu, Sep 6, 2018 at 10:58 AM, Stefan Agner <stefan@agner.ch> 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@ginzinger.com.
>> 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@ginzinger.com/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@agner.ch> 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@embedded-synergy.co.za> wrote:
>>>>
>>>>> Hello Suman,
>>>>>
>>>>> On Thu, Aug 23, 2018 at 6:50 AM, Suman Anna <s-anna@ti.com> 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

Re: rproc_elf_load_segments: rproc_elf_load_segments elf loading problem

[Stefan Agner]

On 05.09.2018 18:23, Denis Ryndine wrote:
> Hi
> 
> On Thu, Sep 6, 2018 at 10:58 AM, Stefan Agner <stefan@agner.ch> 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@ginzinger.com.
>>> 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@ginzinger.com/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@agner.ch> 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@embedded-synergy.co.za> wrote:
>>>>>
>>>>>> Hello Suman,
>>>>>>
>>>>>> On Thu, Aug 23, 2018 at 6:50 AM, Suman Anna <s-anna@ti.com> 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

Re: rproc_elf_load_segments: rproc_elf_load_segments elf loading problem

[Denis Ryndine]

On Thu, Sep 6, 2018 at 11:43 AM, Stefan Agner <stefan@agner.ch> wrote:
> On 05.09.2018 18:23, Denis Ryndine wrote:
>> Hi
>>
>> On Thu, Sep 6, 2018 at 10:58 AM, Stefan Agner <stefan@agner.ch> 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@ginzinger.com.
>>>> 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@ginzinger.com/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@agner.ch> 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@embedded-synergy.co.za> wrote:
>>>>>>
>>>>>>> Hello Suman,
>>>>>>>
>>>>>>> On Thu, Aug 23, 2018 at 6:50 AM, Suman Anna <s-anna@ti.com> 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