Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20180803151035.238a19d2@endymion> <CAKwvOdmBtzkca-C1YpB_OYVyqFFi3P_Q8DJPaKr96DFZF0e6Hg@mail.gmail.com>
In-Reply-To: <CAKwvOdmBtzkca-C1YpB_OYVyqFFi3P_Q8DJPaKr96DFZF0e6Hg@mail.gmail.com>
From:   Alistair Strachan <astrachan@google.com>
Date:   Fri, 3 Aug 2018 10:49:51 -0700
Message-ID: <CANDihLGJ48iQTak-pFH4OYPqKtvFwHtgr9MBrpm7RmLX1Emo=Q@mail.gmail.com>
Subject: Re: native_save_fl() causes a warning
To:     Nick Desaulniers <ndesaulniers@google.com>
Cc:     jdelvare@suse.de, linux-kernel@vger.kernel.org, mka@chromium.org,
        Arnd Bergmann <arnd@arndb.de>, hpa@zytor.com,
        tstellar@redhat.com, sedat.dilek@gmail.com, jgross@suse.com,
        mingo@kernel.org, David.Laight@aculab.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Aug 3, 2018 at 9:38 AM Nick Desaulniers <ndesaulniers@google.com> w=
rote:
>
> 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=3D1):
> >
> > ./arch/x86/include/asm/irqflags.h:16:29: warning: no previous prototype=
 for =E2=80=98native_save_fl=E2=80=99 [-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=3Dclang, and booted the x86_64 bzImage on any qemu. The minute the
paravirt alternatives were patched in, it exploded.

> --
> Thanks,
> ~Nick Desaulniers