Re: native_save_fl() causes a warning

[Alistair Strachan]

On Fri, Aug 3, 2018 at 9:38 AM Nick Desaulniers <ndesaulniers@xxxxxxxxxx> wrote:
>
> On Fri, Aug 3, 2018 at 6:10 AM Jean Delvare <jdelvare@xxxxxxx> 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