Re: [PATCH 1/2] remoteproc: drop memset when loading elf segments

[Bjorn Andersson]

On Thu 09 Apr 18:29 PDT 2020, Peng Fan wrote:

> Hi Bjorn,
> 
> > Subject: Re: [PATCH 1/2] remoteproc: drop memset when loading elf
> > segments
> > 
> > On Thu 09 Apr 01:22 PDT 2020, Peng Fan wrote:
> > 
> > > To arm64, "dc      zva, dst" is used in memset.
> > > Per ARM DDI 0487A.j, chapter C5.3.8 DC ZVA, Data Cache Zero by VA,
> > >
> > > "If the memory region being zeroed is any type of Device memory, this
> > > instruction can give an alignment fault which is prioritized in the
> > > same way as other alignment faults that are determined by the memory
> > > type."
> > >
> > > On i.MX platforms, when elf is loaded to onchip TCM area, the region
> > > is ioremapped, so "dc zva, dst" will trigger abort.
> > >
> > > Since memset is not strictly required, let's drop it.
> > >
> > 
> > This would imply that we trust that the firmware doesn't expect remoteproc
> > to zero out the memory, which we've always done. So I don't think we can say
> > that it's not required.
> 
> Saying an image runs on a remote core needs Linux to help zero out BSS section,
> this not make sense to me.

Maybe not, but it has always done it, so if there's firmware out there
that depends on it such change would break them..

> My case is as following, I need to load section 7 data.
> I no need to let remoteproc to memset section 8/9/10/11/12, the firmware itself
> could handle that. Just because the memsz is larger than filesz, remoreproc must
> memset?

By having a PT_LOAD segment covering these I think it's reasonable to
assume that the ELF loader should be able to touch the associated
memory.

> Section Headers:
>   [Nr] Name              Type            Addr     Off    Size   ES Flg Lk Inf Al
>   [ 0]                   NULL            00000000 000000 000000 00      0   0  0
>   [ 1] .interrupts       PROGBITS        1ffe0000 010000 000240 00   A  0   0  4
>   [ 2] .resource_table   PROGBITS        1ffe0240 010240 000058 00   A  0   0  1
>   [ 3] .text             PROGBITS        1ffe02a0 0102a0 009ccc 00  AX  0   0 16
>   [ 4] .ARM              ARM_EXIDX       1ffe9f6c 019f6c 000008 00  AL  3   0  4
>   [ 5] .init_array       INIT_ARRAY      1ffe9f74 019f74 000004 04  WA  0   0  4
>   [ 6] .fini_array       FINI_ARRAY      1ffe9f78 019f78 000004 04  WA  0   0  4
>   [ 7] .data             PROGBITS        1fff9240 029240 000084 00  WA  0   0  4
>   [ 8] .ncache.init      PROGBITS        1fff92c4 0292c4 000000 00   W  0   0  1
>   [ 9] .ncache           NOBITS          1fff92c4 0292c4 000a80 00  WA  0   0  4
>   [10] .bss              NOBITS          1fff9d44 0292c4 01f5c0 00  WA  0   0  4
>   [11] .heap             NOBITS          20019304 0292c4 000404 00  WA  0   0  1
>   [12] .stack            NOBITS          20019708 0292c4 000400 00  WA  0   0  1
> 
> > 
> > > Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> > > ---
> > >  drivers/remoteproc/remoteproc_elf_loader.c | 7 ++-----
> > >  1 file changed, 2 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/remoteproc/remoteproc_elf_loader.c
> > > b/drivers/remoteproc/remoteproc_elf_loader.c
> > > index 16e2c496fd45..cc50fe70d50c 100644
> > > --- a/drivers/remoteproc/remoteproc_elf_loader.c
> > > +++ b/drivers/remoteproc/remoteproc_elf_loader.c
> > > @@ -238,14 +238,11 @@ int rproc_elf_load_segments(struct rproc *rproc,
> > const struct firmware *fw)
> > >  			memcpy(ptr, elf_data + offset, filesz);
> > >
> > >  		/*
> > > -		 * Zero out remaining memory for this segment.
> > > +		 * No need 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.
> > > +		 * did this for us.
> > 
> > In the case of recovery this comment is wrong, we do not
> > dma_alloc_coherent() the carveout during a recovery.
> 
> Isn't the it the firmware's job to memset the region?
> 

I'm not aware of this being a documented requirement, we've always done
it here for them, so removing this call would be a change in behavior.

> > 
> > And in your case you ioremapped existing TCM, so it's never true.
> > 
> > >  		 */
> > > -		if (memsz > filesz)
> > > -			memset(ptr + filesz, 0, memsz - filesz);
> > 
> > So I think you do want to zero out this region. Question is how we do it...
> 
> I have contacted our M4 owners, we no need clear it from Linux side.

And I think _most_ firmware out there, like yours, does clear BSS etc
during initialization.

> We also support booting m4 before booting Linux, at that case, Linux has
> noting to do with memset. It is just I try loading m4 image with Linux,
> and met the issue that memset trigger abort.
> 

Please see the proposal for attaching to already running remoteproc's
from Mathieu. I don't expect that you want to load your PROGBITS either
in this case?

Regards,
Bjorn