Messages in this thread | | | From | Alistair Strachan <> | Date | Fri, 3 Aug 2018 10:49:51 -0700 | Subject | Re: native_save_fl() causes a warning |
| |
On Fri, Aug 3, 2018 at 9:38 AM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Fri, Aug 3, 2018 at 6:10 AM Jean Delvare <jdelvare@suse.de> wrote: > > > > Hi Nick, > > > > It seems that this linux kernel commit of yours: > > > > commit d0a8d9378d16eb3c69bd8e6d23779fbdbee3a8c7 > > Author: Nick Desaulniers > > Date: Thu Jun 21 09:23:24 2018 -0700 > > > > x86/paravirt: Make native_save_fl() extern inline > > > > introduced a new warning (with W=1): > > > > ./arch/x86/include/asm/irqflags.h:16:29: warning: no previous prototype for ‘native_save_fl’ [-Wmissing-prototypes] > > extern inline unsigned long native_save_fl(void) > > ^ > > > > Please fix it. > > Hi Jean, thanks for the report. David Laight also reported this > warning; he tested a patch I sent him overnight. > > Let me guess, you're using a version of GCC < 4.9? It seems that GCC > < 4.9 will produce that warning for extern inline functions without > previous declarations. > > I'll add your Reported-By tag to the patch that I will send out in a > few minutes. > > > Secondly, I am quite curious why you changed only native_save_fl() from > > static inline to extern inline, when native_restore_fl(), > > native_irq_disable() and native_irq_enable() are equally referenced by > > address in arch/x86/kernel/paravirt.c and thus should suffer from the > > same problem. Can you explain? > > This is a good point. With native_save_fl, we were not able to boot > the kernel at all. Maybe this was called from the boot sequence > (maybe Juergen knows more)? It seems that the other functions aren't > preventing us from booting, but maybe exercising other paths in > paravirt would expose such an issue? Or maybe paravirt doesn't have > the same calling convention requirements for those functions?
The core issue these patches worked around was the automatic/heuristic generation of stack guard code by clang, which ended up clobbering %ecx/%rcx in a way not expected by the contract of the paravirt code. The only function affected by this problem was native_save_fl(), because only it has a C stack (and thus, has a stack that requires guarding).
The other functions could have been converted at the same time, and they will have to be converted if (down the line) somebody adds C stack variables to them. But, for now, the patch series seems to correctly work around this issue.
> Is there a standard testing procedure for paravirt? I'd be happy to > try it to see if we can expose more things that should have the same > cleanup.
This bug was so obvious you just enabled CONFIG_PARAVIRT, built with CC=clang, and booted the x86_64 bzImage on any qemu. The minute the paravirt alternatives were patched in, it exploded.
> -- > Thanks, > ~Nick Desaulniers
| |