| Message ID | 20210908204541.3632269-3-morbo@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Prevent inlining for asm blocks with labels | expand |
On Wed, Sep 08, 2021, Bill Wendling wrote: > exec_in_big_real_mode() uses inline asm that has labels. Clang decides _global_ labels. Inlining functions with local labels, including asm goto labels, is not problematic, the issue is specific to labels that must be unique for a given compilation unit. > that it can inline this function, which causes the assembler to complain > about duplicate symbols. Mark the function as "noinline" to prevent > this. > > Signed-off-by: Bill Wendling <morbo@google.com> > --- > x86/realmode.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/x86/realmode.c b/x86/realmode.c > index b4fa603..07a477f 100644 > --- a/x86/realmode.c > +++ b/x86/realmode.c > @@ -178,7 +178,7 @@ static inline void init_inregs(struct regs *regs) > inregs.esp = (unsigned long)&tmp_stack.top; > } > > -static void exec_in_big_real_mode(struct insn_desc *insn) > +static __attribute__((noinline)) void exec_in_big_real_mode(struct insn_desc *insn) Forgot to use the new define in this patch :-) > { > unsigned long tmp; > static struct regs save; > -- > 2.33.0.309.g3052b89438-goog >
On Wed, Sep 8, 2021 at 2:49 PM Sean Christopherson <seanjc@google.com> wrote: > > On Wed, Sep 08, 2021, Bill Wendling wrote: > > exec_in_big_real_mode() uses inline asm that has labels. Clang decides > > _global_ labels. Inlining functions with local labels, including asm goto labels, > is not problematic, the issue is specific to labels that must be unique for a > given compilation unit. > > > that it can inline this function, which causes the assembler to complain > > about duplicate symbols. Mark the function as "noinline" to prevent > > this. > > > > Signed-off-by: Bill Wendling <morbo@google.com> > > --- > > x86/realmode.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/x86/realmode.c b/x86/realmode.c > > index b4fa603..07a477f 100644 > > --- a/x86/realmode.c > > +++ b/x86/realmode.c > > @@ -178,7 +178,7 @@ static inline void init_inregs(struct regs *regs) > > inregs.esp = (unsigned long)&tmp_stack.top; > > } > > > > -static void exec_in_big_real_mode(struct insn_desc *insn) > > +static __attribute__((noinline)) void exec_in_big_real_mode(struct insn_desc *insn) > > Forgot to use the new define in this patch :-) > This was intentional. realmode.c doesn't #include any header files, and adding '#include "libflat.h" causes a lot of warnings and errors. We could do that, but I feel it's beyond the scope of this series of patches. -bw
On Wed, Sep 08, 2021, Bill Wendling wrote: > On Wed, Sep 8, 2021 at 2:49 PM Sean Christopherson <seanjc@google.com> wrote: > > > > On Wed, Sep 08, 2021, Bill Wendling wrote: > > > exec_in_big_real_mode() uses inline asm that has labels. Clang decides > > > > _global_ labels. Inlining functions with local labels, including asm goto labels, > > is not problematic, the issue is specific to labels that must be unique for a > > given compilation unit. > > > > > that it can inline this function, which causes the assembler to complain > > > about duplicate symbols. Mark the function as "noinline" to prevent > > > this. > > > > > > Signed-off-by: Bill Wendling <morbo@google.com> > > > --- > > > x86/realmode.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/x86/realmode.c b/x86/realmode.c > > > index b4fa603..07a477f 100644 > > > --- a/x86/realmode.c > > > +++ b/x86/realmode.c > > > @@ -178,7 +178,7 @@ static inline void init_inregs(struct regs *regs) > > > inregs.esp = (unsigned long)&tmp_stack.top; > > > } > > > > > > -static void exec_in_big_real_mode(struct insn_desc *insn) > > > +static __attribute__((noinline)) void exec_in_big_real_mode(struct insn_desc *insn) > > > > Forgot to use the new define in this patch :-) > > > This was intentional. realmode.c doesn't #include any header files, > and adding '#include "libflat.h" causes a lot of warnings and errors. > We could do that, but I feel it's beyond the scope of this series of > patches. Ah, right, realmode is compiled for real mode and can't use any of the libcflat stuff. A better option would be to put the #define in linux/compiler.h and include that in libcflat.h and directly in realmode.h. It only requires a small prep patch to avoid a duplicate barrier() definition. From 6e6971ef22c335732a9597409a45fee8a3be6fb7 Mon Sep 17 00:00:00 2001 From: Sean Christopherson <seanjc@google.com> Date: Wed, 8 Sep 2021 15:41:12 -0700 Subject: [PATCH] lib: Drop x86/processor.h's barrier() in favor of compiler.h version Drop x86's duplicate version of barrier() in favor of the generic #define provided by linux/compiler.h. Include compiler.h in the all-encompassing libcflat.h to pick up barrier() and other future goodies, e.g. new attributes defines. Signed-off-by: Sean Christopherson <seanjc@google.com> --- lib/libcflat.h | 1 + lib/x86/processor.h | 5 ----- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/lib/libcflat.h b/lib/libcflat.h index 97db9e3..e619de1 100644 --- a/lib/libcflat.h +++ b/lib/libcflat.h @@ -22,6 +22,7 @@ #ifndef __ASSEMBLY__ +#include <linux/compiler.h> #include <stdarg.h> #include <stddef.h> #include <stdint.h> diff --git a/lib/x86/processor.h b/lib/x86/processor.h index f380321..eaf24d4 100644 --- a/lib/x86/processor.h +++ b/lib/x86/processor.h @@ -216,11 +216,6 @@ struct descriptor_table_ptr { ulong base; } __attribute__((packed)); -static inline void barrier(void) -{ - asm volatile ("" : : : "memory"); -} - static inline void clac(void) { asm volatile (".byte 0x0f, 0x01, 0xca" : : : "memory"); -- and then your patch 1 can become: diff --git a/lib/linux/compiler.h b/lib/linux/compiler.h index 5d9552a..5937b7b 100644 --- a/lib/linux/compiler.h +++ b/lib/linux/compiler.h @@ -46,6 +46,7 @@ #define barrier() asm volatile("" : : : "memory") #define __always_inline inline __attribute__((always_inline)) +#define noinline __attribute__((noinline))
On Wed, Sep 8, 2021 at 3:51 PM Sean Christopherson <seanjc@google.com> wrote: > > On Wed, Sep 08, 2021, Bill Wendling wrote: > > On Wed, Sep 8, 2021 at 2:49 PM Sean Christopherson <seanjc@google.com> wrote: > > > > > > On Wed, Sep 08, 2021, Bill Wendling wrote: > > > > exec_in_big_real_mode() uses inline asm that has labels. Clang decides > > > > > > _global_ labels. Inlining functions with local labels, including asm goto labels, > > > is not problematic, the issue is specific to labels that must be unique for a > > > given compilation unit. > > > > > > > that it can inline this function, which causes the assembler to complain > > > > about duplicate symbols. Mark the function as "noinline" to prevent > > > > this. > > > > > > > > Signed-off-by: Bill Wendling <morbo@google.com> > > > > --- > > > > x86/realmode.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/x86/realmode.c b/x86/realmode.c > > > > index b4fa603..07a477f 100644 > > > > --- a/x86/realmode.c > > > > +++ b/x86/realmode.c > > > > @@ -178,7 +178,7 @@ static inline void init_inregs(struct regs *regs) > > > > inregs.esp = (unsigned long)&tmp_stack.top; > > > > } > > > > > > > > -static void exec_in_big_real_mode(struct insn_desc *insn) > > > > +static __attribute__((noinline)) void exec_in_big_real_mode(struct insn_desc *insn) > > > > > > Forgot to use the new define in this patch :-) > > > > > This was intentional. realmode.c doesn't #include any header files, > > and adding '#include "libflat.h" causes a lot of warnings and errors. > > We could do that, but I feel it's beyond the scope of this series of > > patches. > > Ah, right, realmode is compiled for real mode and can't use any of the libcflat > stuff. > > A better option would be to put the #define in linux/compiler.h and include that > in libcflat.h and directly in realmode.h. It only requires a small prep patch to > avoid a duplicate barrier() definition. > Sounds good to me. Please go ahead and submit this for inclusion and I can revamp my stuff. -bw > From 6e6971ef22c335732a9597409a45fee8a3be6fb7 Mon Sep 17 00:00:00 2001 > From: Sean Christopherson <seanjc@google.com> > Date: Wed, 8 Sep 2021 15:41:12 -0700 > Subject: [PATCH] lib: Drop x86/processor.h's barrier() in favor of compiler.h > version > > Drop x86's duplicate version of barrier() in favor of the generic #define > provided by linux/compiler.h. Include compiler.h in the all-encompassing > libcflat.h to pick up barrier() and other future goodies, e.g. new > attributes defines. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > lib/libcflat.h | 1 + > lib/x86/processor.h | 5 ----- > 2 files changed, 1 insertion(+), 5 deletions(-) > > diff --git a/lib/libcflat.h b/lib/libcflat.h > index 97db9e3..e619de1 100644 > --- a/lib/libcflat.h > +++ b/lib/libcflat.h > @@ -22,6 +22,7 @@ > > #ifndef __ASSEMBLY__ > > +#include <linux/compiler.h> > #include <stdarg.h> > #include <stddef.h> > #include <stdint.h> > diff --git a/lib/x86/processor.h b/lib/x86/processor.h > index f380321..eaf24d4 100644 > --- a/lib/x86/processor.h > +++ b/lib/x86/processor.h > @@ -216,11 +216,6 @@ struct descriptor_table_ptr { > ulong base; > } __attribute__((packed)); > > -static inline void barrier(void) > -{ > - asm volatile ("" : : : "memory"); > -} > - > static inline void clac(void) > { > asm volatile (".byte 0x0f, 0x01, 0xca" : : : "memory"); > -- > > and then your patch 1 can become: > > diff --git a/lib/linux/compiler.h b/lib/linux/compiler.h > index 5d9552a..5937b7b 100644 > --- a/lib/linux/compiler.h > +++ b/lib/linux/compiler.h > @@ -46,6 +46,7 @@ > #define barrier() asm volatile("" : : : "memory") > > #define __always_inline inline __attribute__((always_inline)) > +#define noinline __attribute__((noinline)) >
diff --git a/x86/realmode.c b/x86/realmode.c index b4fa603..07a477f 100644 --- a/x86/realmode.c +++ b/x86/realmode.c @@ -178,7 +178,7 @@ static inline void init_inregs(struct regs *regs) inregs.esp = (unsigned long)&tmp_stack.top; } -static void exec_in_big_real_mode(struct insn_desc *insn) +static __attribute__((noinline)) void exec_in_big_real_mode(struct insn_desc *insn) { unsigned long tmp; static struct regs save;
exec_in_big_real_mode() uses inline asm that has labels. Clang decides that it can inline this function, which causes the assembler to complain about duplicate symbols. Mark the function as "noinline" to prevent this. Signed-off-by: Bill Wendling <morbo@google.com> --- x86/realmode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)