On 16.10.2018 00:23, Russell King - ARM Linux wrote: > On Tue, Oct 16, 2018 at 12:16:29AM +0200, Stefan Agner wrote: >> When functions incoming parameters are not in input operands list gcc >> 4.5 does not load the parameters into registers before calling this >> function but the inline assembly assumes valid addresses inside this >> function. This breaks the code because r0 and r1 are invalid when >> execution enters v4wb_copy_user_page () > > NAK. Naked functions must never be inlined. Please add a "noinline" > attribute to the function rather than making things more complex. >
To be honest, I did not put much thought into this commit since it is just doing to copypage-fa.c what 9a40ac86152c ("ARM: 6164/1: Add kto and kfrom to input operands list.") has been done to the other copypage implementations... [adding Khem] > The GCC manual states: > > `naked' > Use this attribute on the ARM, AVR, MCORE, MSP430, NDS32, RL78, RX > and SPU ports to indicate that the specified function does not > need prologue/epilogue sequences generated by the compiler. It is > up to the programmer to provide these sequences. The only > ^^^^^^^^ > statements that can be safely included in naked functions are > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > `asm' statements that do not have operands. All other statements, > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > including declarations of local variables, `if' statements, and so > forth, should be avoided. Naked functions should be used to > implement the body of an assembly function, while allowing the > compiler to construct the requisite function declaration for the > assembler. > > The 'I' attribute is fine here because it is a constant that is not > allowed to be in a register (and hence has no code generation side > effects.) > > Adding operands for the input parameters, however, isn't going to > work around the fact that _this_ assembly is written to be out of > line and so it must never be inlined by the compiler. I briefly looked at a disassembled version after applying both patches, it indeed leads to inlining. However, the code seems to be working (thanks to asm volatile?)... Anyway, my goal is actually what patch 2 ("ARM: copypage: do not use naked functions") is doing: Make Clang happy. As a matter of fact, reverting 9a40ac86152c actually fixes compilation for Clang too, and seems to lead to a working Kernel (tested with versatile_defconfig in Qemu), so maybe that is what we should do here? -- Stefan > >> Also the constant needs to be used as third input operand so account >> for that as well. >> >> This fixes copypage-fa.c what has previously done before for the other >> copypage implementations in commit 9a40ac86152c ("ARM: 6164/1: Add kto >> and kfrom to input operands list."). >> >> Signed-off-by: Stefan Agner <ste...@agner.ch> >> --- >> arch/arm/mm/copypage-fa.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm/mm/copypage-fa.c b/arch/arm/mm/copypage-fa.c >> index d130a5ece5d5..ec6501308c60 100644 >> --- a/arch/arm/mm/copypage-fa.c >> +++ b/arch/arm/mm/copypage-fa.c >> @@ -22,7 +22,7 @@ fa_copy_user_page(void *kto, const void *kfrom) >> { >> asm("\ >> stmfd sp!, {r4, lr} @ 2\n\ >> - mov r2, %0 @ 1\n\ >> + mov r2, %2 @ 1\n\ >> 1: ldmia r1!, {r3, r4, ip, lr} @ 4\n\ >> stmia r0, {r3, r4, ip, lr} @ 4\n\ >> mcr p15, 0, r0, c7, c14, 1 @ 1 clean and invalidate D >> line\n\ >> @@ -36,7 +36,7 @@ fa_copy_user_page(void *kto, const void *kfrom) >> mcr p15, 0, r2, c7, c10, 4 @ 1 drain WB\n\ >> ldmfd sp!, {r4, pc} @ 3" >> : >> - : "I" (PAGE_SIZE / 32)); >> + : "r" (kto), "r" (kfrom), "I" (PAGE_SIZE / 32)); >> } >> >> void fa_copy_user_highpage(struct page *to, struct page *from, >> -- >> 2.19.1 >>