native_save_fl() causes a warning

native_save_fl() causes a warning

[Jean Delvare]

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.

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?

Thanks,
-- 
Jean Delvare
SUSE L3 Support

Re: native_save_fl() causes a warning

[Nick Desaulniers]

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?

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.
-- 
Thanks,
~Nick Desaulniers

[PATCH] x86/irqflags: provide a declaration for native_save_fl

[Nick Desaulniers]

Fixes commit d0a8d9378d16 ("x86/paravirt: Make native_save_fl() extern
inline").

It was reported that the above commit was causing users of gcc < 4.9 to
observe -Werror=missing-prototypes errors.

Indeed, it seems that:
extern inline unsigned long native_save_fl(void) { return 0; }

compiled with -Werror=missing-prototypes produces this warning in gcc <
4.9, but not gcc >= 4.9.

Cc: stable@vger.kernel.org # 4.17, 4.14, 4.9, 4.4
Reported-by: David Laight <david.laight@aculab.com>
Reported-by: Jean Delvare <jdelvare@suse.de>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
 arch/x86/include/asm/irqflags.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/include/asm/irqflags.h b/arch/x86/include/asm/irqflags.h
index c4fc17220df9..c14f2a74b2be 100644
--- a/arch/x86/include/asm/irqflags.h
+++ b/arch/x86/include/asm/irqflags.h
@@ -13,6 +13,8 @@
  * Interrupt control:
  */
 
+/* Declaration required for gcc < 4.9 to prevent -Werror=missing-prototypes */
+extern inline unsigned long native_save_fl(void);
 extern inline unsigned long native_save_fl(void)
 {
 	unsigned long flags;
-- 
2.18.0.597.ga71716f1ad-goog

Re: native_save_fl() causes a warning

[Alistair Strachan]

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

[tip:x86/urgent] x86/irqflags: Provide a declaration for native_save_fl

[tip-bot for Nick Desaulniers]

Commit-ID:  208cbb32558907f68b3b2a081ca2337ac3744794
Gitweb:     https://git.kernel.org/tip/208cbb32558907f68b3b2a081ca2337ac3744794
Author:     Nick Desaulniers <ndesaulniers@google.com>
AuthorDate: Fri, 3 Aug 2018 10:05:50 -0700
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Sun, 5 Aug 2018 22:30:37 +0200

x86/irqflags: Provide a declaration for native_save_fl

It was reported that the commit d0a8d9378d16 is causing users of gcc < 4.9
to observe -Werror=missing-prototypes errors.

Indeed, it seems that:
extern inline unsigned long native_save_fl(void) { return 0; }

compiled with -Werror=missing-prototypes produces this warning in gcc <
4.9, but not gcc >= 4.9.

Fixes: d0a8d9378d16 ("x86/paravirt: Make native_save_fl() extern inline").
Reported-by: David Laight <david.laight@aculab.com>
Reported-by: Jean Delvare <jdelvare@suse.de>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: hpa@zytor.com
Cc: jgross@suse.com
Cc: kstewart@linuxfoundation.org
Cc: gregkh@linuxfoundation.org
Cc: boris.ostrovsky@oracle.com
Cc: astrachan@google.com
Cc: mka@chromium.org
Cc: arnd@arndb.de
Cc: tstellar@redhat.com
Cc: sedat.dilek@gmail.com
Cc: David.Laight@aculab.com
Cc: stable@vger.kernel.org
Link: https://lkml.kernel.org/r/20180803170550.164688-1-ndesaulniers@google.com

---
 arch/x86/include/asm/irqflags.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/include/asm/irqflags.h b/arch/x86/include/asm/irqflags.h
index c4fc17220df9..c14f2a74b2be 100644
--- a/arch/x86/include/asm/irqflags.h
+++ b/arch/x86/include/asm/irqflags.h
@@ -13,6 +13,8 @@
  * Interrupt control:
  */
 
+/* Declaration required for gcc < 4.9 to prevent -Werror=missing-prototypes */
+extern inline unsigned long native_save_fl(void);
 extern inline unsigned long native_save_fl(void)
 {
 	unsigned long flags;

RE: [PATCH] x86/irqflags: provide a declaration for native_save_fl

[David Laight]

From: Nick Desaulniers
> Sent: 03 August 2018 18:06
> Fixes commit d0a8d9378d16 ("x86/paravirt: Make native_save_fl() extern
> inline").
> 
> It was reported that the above commit was causing users of gcc < 4.9 to
> observe -Werror=missing-prototypes errors.
> 
> Indeed, it seems that:
> extern inline unsigned long native_save_fl(void) { return 0; }
> 
> compiled with -Werror=missing-prototypes produces this warning in gcc <
> 4.9, but not gcc >= 4.9.
> 
> Cc: stable@vger.kernel.org # 4.17, 4.14, 4.9, 4.4
> Reported-by: David Laight <david.laight@aculab.com>
> Reported-by: Jean Delvare <jdelvare@suse.de>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>

You can add my:

Tested-by: David Laight <david.laight@acuab.com>

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] x86/irqflags: provide a declaration for native_save_fl

[Sedat Dilek]

On Mon, Aug 6, 2018 at 12:25 PM, David Laight <David.Laight@aculab.com> wrote:
> From: Nick Desaulniers
>> Sent: 03 August 2018 18:06
>> Fixes commit d0a8d9378d16 ("x86/paravirt: Make native_save_fl() extern
>> inline").
>>
>> It was reported that the above commit was causing users of gcc < 4.9 to
>> observe -Werror=missing-prototypes errors.
>>
>> Indeed, it seems that:
>> extern inline unsigned long native_save_fl(void) { return 0; }
>>
>> compiled with -Werror=missing-prototypes produces this warning in gcc <
>> 4.9, but not gcc >= 4.9.
>>
>> Cc: stable@vger.kernel.org # 4.17, 4.14, 4.9, 4.4
>> Reported-by: David Laight <david.laight@aculab.com>
>> Reported-by: Jean Delvare <jdelvare@suse.de>
>> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
>
> You can add my:
>
> Tested-by: David Laight <david.laight@acuab.com>
>

Tested-by: Sedat Dilek <sedat.dilek@gmail.com>

On top of Linux v4.18-rc8 with clang version 7.0.0-svn338205.

- sed@ -

Re: [tip:x86/urgent] x86/irqflags: Provide a declaration for native_save_fl

[Nick Desaulniers]

On Sun, Aug 5, 2018 at 1:33 PM tip-bot for Nick Desaulniers
<tipbot@zytor.com> wrote:
>
> Commit-ID:  208cbb32558907f68b3b2a081ca2337ac3744794
> Gitweb:     https://git.kernel.org/tip/208cbb32558907f68b3b2a081ca2337ac3744794
> Author:     Nick Desaulniers <ndesaulniers@google.com>
> AuthorDate: Fri, 3 Aug 2018 10:05:50 -0700
> Committer:  Thomas Gleixner <tglx@linutronix.de>
> CommitDate: Sun, 5 Aug 2018 22:30:37 +0200
>
> x86/irqflags: Provide a declaration for native_save_fl
>
> It was reported that the commit d0a8d9378d16 is causing users of gcc < 4.9
> to observe -Werror=missing-prototypes errors.
>
> Indeed, it seems that:
> extern inline unsigned long native_save_fl(void) { return 0; }
>
> compiled with -Werror=missing-prototypes produces this warning in gcc <
> 4.9, but not gcc >= 4.9.
>
> Fixes: d0a8d9378d16 ("x86/paravirt: Make native_save_fl() extern inline").
> Reported-by: David Laight <david.laight@aculab.com>
> Reported-by: Jean Delvare <jdelvare@suse.de>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: hpa@zytor.com
> Cc: jgross@suse.com
> Cc: kstewart@linuxfoundation.org
> Cc: gregkh@linuxfoundation.org
> Cc: boris.ostrovsky@oracle.com
> Cc: astrachan@google.com
> Cc: mka@chromium.org
> Cc: arnd@arndb.de
> Cc: tstellar@redhat.com
> Cc: sedat.dilek@gmail.com
> Cc: David.Laight@aculab.com
> Cc: stable@vger.kernel.org

Not sure if this was going to be cleaned up in an automated way, but
looks like this commit message drops the comment to stable as to how
far back it should go:

# 4.17, 4.14, 4.9, 4.4

also, there were tested by's reported:

Tested-by: David Laight <david.laight@acuab.com>
Tested-by: Sedat Dilek <sedat.dilek@gmail.com>

--
Thanks,
~Nick Desaulniers

Re: [tip:x86/urgent] x86/irqflags: Provide a declaration for native_save_fl

[Thomas Gleixner]

On Mon, 6 Aug 2018, Nick Desaulniers wrote:
> On Sun, Aug 5, 2018 at 1:33 PM tip-bot for Nick Desaulniers
> <tipbot@zytor.com> wrote:
> > Cc: stable@vger.kernel.org
> 
> Not sure if this was going to be cleaned up in an automated way, but
> looks like this commit message drops the comment to stable as to how
> far back it should go:
> 
> # 4.17, 4.14, 4.9, 4.4

The Fixes tag is enough. If the upstream commit was backported, then the
tools of the stable folks will find it and know exactly how far it needs to
go back.

> also, there were tested by's reported:
> 
> Tested-by: David Laight <david.laight@acuab.com>
> Tested-by: Sedat Dilek <sedat.dilek@gmail.com>

Which came in after I applied it....

Thanks,

	tglx

Re: [tip:x86/urgent] x86/irqflags: Provide a declaration for native_save_fl

[Nick Desaulniers]

On Tue, Aug 7, 2018 at 12:29 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Mon, 6 Aug 2018, Nick Desaulniers wrote:
> > On Sun, Aug 5, 2018 at 1:33 PM tip-bot for Nick Desaulniers
> > <tipbot@zytor.com> wrote:
> > > Cc: stable@vger.kernel.org
> >
> > Not sure if this was going to be cleaned up in an automated way, but
> > looks like this commit message drops the comment to stable as to how
> > far back it should go:
> >
> > # 4.17, 4.14, 4.9, 4.4
>
> The Fixes tag is enough. If the upstream commit was backported, then the
> tools of the stable folks will find it and know exactly how far it needs to
> go back.

Oh, cool.

> > also, there were tested by's reported:
> >
> > Tested-by: David Laight <david.laight@acuab.com>
> > Tested-by: Sedat Dilek <sedat.dilek@gmail.com>
>
> Which came in after I applied it....

I've seen other maintainers revise commit messages before sending pull
requests along, but I guess that's problematic as anyone else who has
pulled before the revision would then have a merge conflict.
-- 
Thanks,
~Nick Desaulniers

Re: [tip:x86/urgent] x86/irqflags: Provide a declaration for native_save_fl

[Thomas Gleixner]

On Tue, 7 Aug 2018, Nick Desaulniers wrote:
> On Tue, Aug 7, 2018 at 12:29 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Mon, 6 Aug 2018, Nick Desaulniers wrote:
> > > also, there were tested by's reported:
> > >
> > > Tested-by: David Laight <david.laight@acuab.com>
> > > Tested-by: Sedat Dilek <sedat.dilek@gmail.com>
> >
> > Which came in after I applied it....
> 
> I've seen other maintainers revise commit messages before sending pull
> requests along, but I guess that's problematic as anyone else who has
> pulled before the revision would then have a merge conflict.

In general I avoid redoing commits for that reason and others and one of
the reasons why we have the Link: tag in the commit message is to be able
to look up such additional information easily - assumed that the archives
have not vanished from the planet.

Thanks,

	tglx