Messages in this thread | | | Subject | Re: [PATCH 1/2] remoteproc: drop memset when loading elf segments | From | Suman Anna <> | Date | Fri, 17 Apr 2020 11:43:56 -0500 |
| |
On 4/13/20 4:05 AM, Peng Fan wrote: > >> Subject: Re: [PATCH 1/2] remoteproc: drop memset when loading elf >> segments >> >> 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.. > > Got it. > >> >>> 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@nxp.com> >>>>> --- >>>>> 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? > > No need to load for early boot case. It is just userspace load trigger > kernel panic, because memset arm64 could not work for ioremapped memory. > > How about adding ops hooks for memset and memcpy to let driver > have their own implementation?
Hi Peng,
The trick is to use the ioremap_wc() variant instead of ioremap() in your platform driver while mapping the TCMs. I know multiple folks have run into this issue. This is what most of the remoteproc drivers use, and mmio-sram driver also uses the same.
regards Suman
| |