| Message ID | 20231204093731.574465649@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/kvm/emulate: Avoid RET for FASTOPs | expand |
On Mon, Dec 04, 2023, Peter Zijlstra wrote: > > --- a/arch/x86/include/asm/nospec-branch.h > +++ b/arch/x86/include/asm/nospec-branch.h > @@ -193,12 +193,7 @@ > * objtool the subsequent indirect jump/call is vouched safe for retpoline > * builds. > */ > -.macro ANNOTATE_RETPOLINE_SAFE > -.Lhere_\@: > - .pushsection .discard.retpoline_safe > - .long .Lhere_\@ > - .popsection > -.endm > +#define ANNOTATE_RETPOLINE_SAFE ANNOTATE type=ANNOTYPE_RETPOLINE_SAFE > > /* > * (ab)use RETPOLINE_SAFE on RET to annotate away 'bare' RET instructions > @@ -317,11 +312,7 @@ > > #else /* __ASSEMBLY__ */ > > -#define ANNOTATE_RETPOLINE_SAFE \ > - "999:\n\t" \ > - ".pushsection .discard.retpoline_safe\n\t" \ > - ".long 999b\n\t" \ > - ".popsection\n\t" > +#define ANNOTATE_RETPOLINE_SAFE ASM_ANNOTATE(ANNOTYPE_RETPOLINE_SAFE) This fails for some of my builds that end up with CONFIG_OBJTOOl=n. Adding a stub for ASM_ANNOTATE() gets me past that: @@ -156,6 +171,7 @@ #define STACK_FRAME_NON_STANDARD(func) #define STACK_FRAME_NON_STANDARD_FP(func) #define ANNOTATE_NOENDBR +#define ASM_ANNOTATE(x) #define ASM_REACHABLE #else #define ANNOTATE_INTRA_FUNCTION_CALL but then I run into other issues: arch/x86/kernel/relocate_kernel_32.S: Assembler messages: arch/x86/kernel/relocate_kernel_32.S:96: Error: Parameter named `type' does not exist for macro `annotate' arch/x86/kernel/relocate_kernel_32.S:166: Error: Parameter named `type' does not exist for macro `annotate' arch/x86/kernel/relocate_kernel_32.S:174: Error: Parameter named `type' does not exist for macro `annotate' arch/x86/kernel/relocate_kernel_32.S:200: Error: Parameter named `type' does not exist for macro `annotate' arch/x86/kernel/relocate_kernel_32.S:220: Error: Parameter named `type' does not exist for macro `annotate' arch/x86/kernel/relocate_kernel_32.S:285: Error: Parameter named `type' does not exist for macro `annotate'
Hi Peter, On Wed, 6 Dec 2023, Sean Christopherson wrote: > On Mon, Dec 04, 2023, Peter Zijlstra wrote: > > > > --- a/arch/x86/include/asm/nospec-branch.h > > +++ b/arch/x86/include/asm/nospec-branch.h > > @@ -193,12 +193,7 @@ > > * objtool the subsequent indirect jump/call is vouched safe for retpoline > > * builds. > > */ > > -.macro ANNOTATE_RETPOLINE_SAFE > > -.Lhere_\@: > > - .pushsection .discard.retpoline_safe > > - .long .Lhere_\@ > > - .popsection > > -.endm > > +#define ANNOTATE_RETPOLINE_SAFE ANNOTATE type=ANNOTYPE_RETPOLINE_SAFE > > > > /* > > * (ab)use RETPOLINE_SAFE on RET to annotate away 'bare' RET instructions > > @@ -317,11 +312,7 @@ > > > > #else /* __ASSEMBLY__ */ > > > > -#define ANNOTATE_RETPOLINE_SAFE \ > > - "999:\n\t" \ > > - ".pushsection .discard.retpoline_safe\n\t" \ > > - ".long 999b\n\t" \ > > - ".popsection\n\t" > > +#define ANNOTATE_RETPOLINE_SAFE ASM_ANNOTATE(ANNOTYPE_RETPOLINE_SAFE) > > This fails for some of my builds that end up with CONFIG_OBJTOOl=n. Adding a > stub for ASM_ANNOTATE() gets me past that: > > @@ -156,6 +171,7 @@ > #define STACK_FRAME_NON_STANDARD(func) > #define STACK_FRAME_NON_STANDARD_FP(func) > #define ANNOTATE_NOENDBR > +#define ASM_ANNOTATE(x) > #define ASM_REACHABLE > #else > #define ANNOTATE_INTRA_FUNCTION_CALL > > but then I run into other issues: > > arch/x86/kernel/relocate_kernel_32.S: Assembler messages: > arch/x86/kernel/relocate_kernel_32.S:96: Error: Parameter named `type' does not exist for macro `annotate' > arch/x86/kernel/relocate_kernel_32.S:166: Error: Parameter named `type' does not exist for macro `annotate' > arch/x86/kernel/relocate_kernel_32.S:174: Error: Parameter named `type' does not exist for macro `annotate' > arch/x86/kernel/relocate_kernel_32.S:200: Error: Parameter named `type' does not exist for macro `annotate' > arch/x86/kernel/relocate_kernel_32.S:220: Error: Parameter named `type' does not exist for macro `annotate' > arch/x86/kernel/relocate_kernel_32.S:285: Error: Parameter named `type' does not exist for macro `annotate' Sean pointed me at this series recently. It seems like these compile errors (and some others) go away with the following diff: diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h index 0bebdcad7ba1..036ab199859a 100644 --- a/arch/x86/include/asm/nospec-branch.h +++ b/arch/x86/include/asm/nospec-branch.h @@ -489,7 +489,7 @@ static inline void call_depth_return_thunk(void) {} " .align 16\n" \ "903: lea 4(%%esp), %%esp;\n" \ " pushl %[thunk_target];\n" \ - " ret;\n" \ + " ret;\n", \ X86_FEATURE_RETPOLINE, \ "lfence;\n" \ ANNOTATE_RETPOLINE_SAFE \ diff --git a/include/linux/objtool.h b/include/linux/objtool.h index f6f80bfefe3b..e811b2ff3a9c 100644 --- a/include/linux/objtool.h +++ b/include/linux/objtool.h @@ -159,6 +159,7 @@ #define STACK_FRAME_NON_STANDARD_FP(func) #define ANNOTATE_NOENDBR #define ASM_REACHABLE +#define ASM_ANNOTATE(x) #else #define ANNOTATE_INTRA_FUNCTION_CALL .macro UNWIND_HINT type:req sp_reg=0 sp_offset=0 signal=0 @@ -169,7 +170,7 @@ .endm .macro REACHABLE .endm -.macro ANNOTATE +.macro ANNOTATE type:req .endm #endif This series applies on top of the latest kvm-x86/next with only a few trivial conflicts, so I hope you are able to send a new version. I could send one for you, but I have no idea how to properly test it.
On Fri, Sep 13, 2024 at 1:47 PM James Houghton <jthoughton@google.com> wrote: > > On Wed, 6 Dec 2023, Sean Christopherson wrote: > > On Mon, Dec 04, 2023, Peter Zijlstra wrote: > > > > > > --- a/arch/x86/include/asm/nospec-branch.h > > > +++ b/arch/x86/include/asm/nospec-branch.h > > > @@ -193,12 +193,7 @@ > > > * objtool the subsequent indirect jump/call is vouched safe for retpoline > > > * builds. > > > */ > > > -.macro ANNOTATE_RETPOLINE_SAFE > > > -.Lhere_\@: > > > - .pushsection .discard.retpoline_safe > > > - .long .Lhere_\@ > > > - .popsection > > > -.endm > > > +#define ANNOTATE_RETPOLINE_SAFE ANNOTATE type=ANNOTYPE_RETPOLINE_SAFE > > > > > > /* > > > * (ab)use RETPOLINE_SAFE on RET to annotate away 'bare' RET instructions > > > @@ -317,11 +312,7 @@ > > > > > > #else /* __ASSEMBLY__ */ > > > > > > -#define ANNOTATE_RETPOLINE_SAFE \ > > > - "999:\n\t" \ > > > - ".pushsection .discard.retpoline_safe\n\t" \ > > > - ".long 999b\n\t" \ > > > - ".popsection\n\t" > > > +#define ANNOTATE_RETPOLINE_SAFE ASM_ANNOTATE(ANNOTYPE_RETPOLINE_SAFE) > > > > This fails for some of my builds that end up with CONFIG_OBJTOOl=n. Adding a > > stub for ASM_ANNOTATE() gets me past that: > > > > @@ -156,6 +171,7 @@ > > #define STACK_FRAME_NON_STANDARD(func) > > #define STACK_FRAME_NON_STANDARD_FP(func) > > #define ANNOTATE_NOENDBR > > +#define ASM_ANNOTATE(x) > > #define ASM_REACHABLE > > #else > > #define ANNOTATE_INTRA_FUNCTION_CALL > > > > but then I run into other issues: > > > > arch/x86/kernel/relocate_kernel_32.S: Assembler messages: > > arch/x86/kernel/relocate_kernel_32.S:96: Error: Parameter named `type' does not exist for macro `annotate' > > arch/x86/kernel/relocate_kernel_32.S:166: Error: Parameter named `type' does not exist for macro `annotate' > > arch/x86/kernel/relocate_kernel_32.S:174: Error: Parameter named `type' does not exist for macro `annotate' > > arch/x86/kernel/relocate_kernel_32.S:200: Error: Parameter named `type' does not exist for macro `annotate' > > arch/x86/kernel/relocate_kernel_32.S:220: Error: Parameter named `type' does not exist for macro `annotate' > > arch/x86/kernel/relocate_kernel_32.S:285: Error: Parameter named `type' does not exist for macro `annotate' > > Sean pointed me at this series recently. It seems like these compile errors > (and some others) go away with the following diff: > > diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h > index 0bebdcad7ba1..036ab199859a 100644 > --- a/arch/x86/include/asm/nospec-branch.h > +++ b/arch/x86/include/asm/nospec-branch.h > @@ -489,7 +489,7 @@ static inline void call_depth_return_thunk(void) {} > " .align 16\n" \ > "903: lea 4(%%esp), %%esp;\n" \ > " pushl %[thunk_target];\n" \ > - " ret;\n" \ > + " ret;\n", \ > X86_FEATURE_RETPOLINE, \ > "lfence;\n" \ > ANNOTATE_RETPOLINE_SAFE \ > diff --git a/include/linux/objtool.h b/include/linux/objtool.h > index f6f80bfefe3b..e811b2ff3a9c 100644 > --- a/include/linux/objtool.h > +++ b/include/linux/objtool.h > @@ -159,6 +159,7 @@ > #define STACK_FRAME_NON_STANDARD_FP(func) > #define ANNOTATE_NOENDBR > #define ASM_REACHABLE > +#define ASM_ANNOTATE(x) > #else > #define ANNOTATE_INTRA_FUNCTION_CALL > .macro UNWIND_HINT type:req sp_reg=0 sp_offset=0 signal=0 > @@ -169,7 +170,7 @@ > .endm > .macro REACHABLE > .endm > -.macro ANNOTATE > +.macro ANNOTATE type:req > .endm > #endif > > This series applies on top of the latest kvm-x86/next with only a few trivial > conflicts, so I hope you are able to send a new version. > > I could send one for you, but I have no idea how to properly test it. Hi Peter, I'll go ahead and repost this series soon unless you tell me otherwise. :)
On Wed, Oct 30, 2024 at 01:08:28PM -0700, James Houghton wrote: > Hi Peter, > > I'll go ahead and repost this series soon unless you tell me otherwise. :) Thanks for your persistent poking. I've refreshed the queue:x86/kvm branch, and provided the robots don't scream, I'll go and post it.
--- a/arch/x86/include/asm/nospec-branch.h +++ b/arch/x86/include/asm/nospec-branch.h @@ -193,12 +193,7 @@ * objtool the subsequent indirect jump/call is vouched safe for retpoline * builds. */ -.macro ANNOTATE_RETPOLINE_SAFE -.Lhere_\@: - .pushsection .discard.retpoline_safe - .long .Lhere_\@ - .popsection -.endm +#define ANNOTATE_RETPOLINE_SAFE ANNOTATE type=ANNOTYPE_RETPOLINE_SAFE /* * (ab)use RETPOLINE_SAFE on RET to annotate away 'bare' RET instructions @@ -317,11 +312,7 @@ #else /* __ASSEMBLY__ */ -#define ANNOTATE_RETPOLINE_SAFE \ - "999:\n\t" \ - ".pushsection .discard.retpoline_safe\n\t" \ - ".long 999b\n\t" \ - ".popsection\n\t" +#define ANNOTATE_RETPOLINE_SAFE ASM_ANNOTATE(ANNOTYPE_RETPOLINE_SAFE) typedef u8 retpoline_thunk_t[RETPOLINE_THUNK_SIZE]; extern retpoline_thunk_t __x86_indirect_thunk_array[]; --- a/include/linux/objtool_types.h +++ b/include/linux/objtool_types.h @@ -58,5 +58,6 @@ struct unwind_hint { * Annotate types */ #define ANNOTYPE_NOENDBR 1 +#define ANNOTYPE_RETPOLINE_SAFE 2 #endif /* _LINUX_OBJTOOL_TYPES_H */ --- a/tools/include/linux/objtool_types.h +++ b/tools/include/linux/objtool_types.h @@ -58,5 +58,6 @@ struct unwind_hint { * Annotate types */ #define ANNOTYPE_NOENDBR 1 +#define ANNOTYPE_RETPOLINE_SAFE 2 #endif /* _LINUX_OBJTOOL_TYPES_H */ --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -2308,12 +2308,12 @@ static int read_unwind_hints(struct objt return 0; } -static int read_annotate(struct objtool_file *file, void (*func)(int type, struct instruction *insn)) +static int read_annotate(struct objtool_file *file, int (*func)(int type, struct instruction *insn)) { struct section *rsec, *sec; struct instruction *insn; struct reloc *reloc; - int type; + int type, ret; rsec = find_section_by_name(file->elf, ".rela.discard.annotate"); if (!rsec) @@ -2333,53 +2333,37 @@ static int read_annotate(struct objtool_ type = *(u32 *)(sec->data->d_buf + (reloc_idx(reloc) * sec->sh.sh_entsize) + 4); - func(type, insn); + ret = func(type, insn); + if (ret < 0) + return ret; } return 0; } -static void __annotate_noendbr(int type, struct instruction *insn) +static int __annotate_noendbr(int type, struct instruction *insn) { if (type != ANNOTYPE_NOENDBR) - return; + return 0; insn->noendbr = 1; + return 0; } -static int read_retpoline_hints(struct objtool_file *file) +static int __annotate_retpoline_safe(int type, struct instruction *insn) { - struct section *rsec; - struct instruction *insn; - struct reloc *reloc; - - rsec = find_section_by_name(file->elf, ".rela.discard.retpoline_safe"); - if (!rsec) + if (type != ANNOTYPE_RETPOLINE_SAFE) return 0; - for_each_reloc(rsec, reloc) { - if (reloc->sym->type != STT_SECTION) { - WARN("unexpected relocation symbol type in %s", rsec->name); - return -1; - } - - insn = find_insn(file, reloc->sym->sec, reloc_addend(reloc)); - if (!insn) { - WARN("bad .discard.retpoline_safe entry"); - return -1; - } - - if (insn->type != INSN_JUMP_DYNAMIC && - insn->type != INSN_CALL_DYNAMIC && - insn->type != INSN_RETURN && - insn->type != INSN_NOP) { - WARN_INSN(insn, "retpoline_safe hint not an indirect jump/call/ret/nop"); - return -1; - } - - insn->retpoline_safe = true; + if (insn->type != INSN_JUMP_DYNAMIC && + insn->type != INSN_CALL_DYNAMIC && + insn->type != INSN_RETURN && + insn->type != INSN_NOP) { + WARN_INSN(insn, "retpoline_safe hint not an indirect jump/call/ret/nop"); + return -1; } + insn->retpoline_safe = true; return 0; } @@ -2666,7 +2650,7 @@ static int decode_sections(struct objtoo if (ret) return ret; - ret = read_retpoline_hints(file); + ret = read_annotate(file, __annotate_retpoline_safe); if (ret) return ret;
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> --- arch/x86/include/asm/nospec-branch.h | 13 +------- include/linux/objtool_types.h | 1 tools/include/linux/objtool_types.h | 1 tools/objtool/check.c | 52 ++++++++++++----------------------- 4 files changed, 22 insertions(+), 45 deletions(-)