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
