On May 25, 2018 9:27:40 AM PDT, Nick Desaulniers <ndesaulni...@google.com> wrote: >On Thu, May 24, 2018 at 3:43 PM <h...@zytor.com> wrote: >> On May 24, 2018 3:31:05 PM PDT, Nick Desaulniers ><ndesaulni...@google.com> >wrote: >> >On Thu, May 24, 2018 at 3:05 PM H. Peter Anvin <h...@zytor.com> >wrote: >> >> COMPILER AR: "=rm" should NEVER generate worse code than "=r". >That >> >is >> >> unequivocally a compiler bug. >> > >> >Filed: https://bugs.llvm.org/show_bug.cgi?id=37583 >> > >> >> >> You are claiming it doesn't buy us anything, but you are only >> >looking >> >at >> >> > the paravirt case which is kind of "special" (in the short bus >kind >> >of >> >way), >> >> > >> >> > That's fair. Is another possible solution to have paravirt >maybe >> >not >> >use >> >> > native_save_fl() then, but its own >> >non-static-inline-without-m-constraint >> >> > implementation? >> > >> >> KERNEL AR: change native_save_fl() to an extern inline with an >> >assembly >> >> out-of-line implementation, to satisfy the paravirt requirement >that >> >no >> >> GPRs other than %rax are clobbered. >> > >> >i'm happy to add that, do you have a recommendation if it should go >in >> >an >> >existing .S file or a new one (and if so where/what shall I call >it?). > >> How about irqflags.c since that is what the .h file is called. > >> It should simply be: > >> push %rdi >> popf >> ret > >> pushf >> pop %rax >> ret > >> ... but with all the regular assembly decorations, of course. > >Something like the following? > > >diff --git a/arch/x86/kernel/irqflags.c b/arch/x86/kernel/irqflags.c >new file mode 100644 >index 000000000000..59dc21bd3327 >--- /dev/null >+++ b/arch/x86/kernel/irqflags.c >@@ -0,0 +1,24 @@ >+#include <asm/asm.h> >+ >+extern unsigned long native_save_fl(void); >+extern void native_restore_fl(unsigned long flags); >+ >+asm( >+".pushsection .text;" >+".global native_save_fl;" >+".type native_save_fl, @function;" >+"native_save_fl:" >+"pushf;" >+"pop %" _ASM_AX ";" >+"ret;" >+".popsection"); >+ >+asm( >+".pushsection .text;" >+".global native_restore_fl;" >+".type native_restore_fl, @function;" >+"native_restore_fl:" >+"push %" _ASM_DI ";" >+"popf;" >+"ret;" >+".popsection"); > >And change the declaration in arch/x86/include/asm/irqflags.h to: >+extern inline unsigned long native_save_fl(void); >+extern inline void native_restore_fl(unsigned long flags); > >This seems to work, but >1. arch/x86/boot/compressed/misc.o warns that native_save_fl() is never >defined (arch_local_save_flags() uses it). Does that mean >arch_local_save_flags(), and friends would also have to move to the >newly >created .c file as well? >2. `extern inline` doesn't inline any instances (from what I can tell >from >disassembling vmlinux). I think this is strictly worse. Don't we only >want >pv_irq_ops.save_fl to be non-inlined in a way that no stack protector >can >be added? If that's the case, should my assembly based implementation >have >a different identifier (`native_save_fl_paravirt` or something). That >would >also fix point #1 above. But now the paravirt code has its own copy of >the >function.
Sorry, I meant irqflags.S. It still should be available as as inline, however, but now "extern inline". -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
