| Message ID | 20230116143645.888786209@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: retbleed=stuff fixes | expand |
* Peter Zijlstra <peterz@infradead.org> wrote: > + /* > + * Definitely wrong, but at this point we should have at least enough > + * to do CALL/RET (consider SKL callthunks) and this avoids having > + * to deal with the noinstr explosion for now :/ > + */ > + instrumentation_begin(); BTW., readability side note: instrumentation_begin()/end() are the misnomers of the century - they don't signal the start/end of instrumented code areas like the name falsely & naively suggests, but the exact opposite: start/end of *non-*instrumented code areas. As such they should probably be something like: noinstr_begin(); ... noinstr_end(); ... to reuse the nomenclature of the 'noinstr' attribute? Thanks, Ingo
On Tue, Jan 17, 2023 at 10:31:05AM +0100, Ingo Molnar wrote: > > * Peter Zijlstra <peterz@infradead.org> wrote: > > > + /* > > + * Definitely wrong, but at this point we should have at least enough > > + * to do CALL/RET (consider SKL callthunks) and this avoids having > > + * to deal with the noinstr explosion for now :/ > > + */ > > + instrumentation_begin(); > > BTW., readability side note: instrumentation_begin()/end() are the > misnomers of the century - they don't signal the start/end of instrumented > code areas like the name falsely & naively suggests, but the exact > opposite: start/end of *non-*instrumented code areas. Nope, they do as they say on the tin. noinstr void foo(void) { } declares the whole function as non-instrumented. Within such functions, we demark regions where instrumentation is allowed by: noinstr void foo(void) { instrumentation_begin(); /* code that calls non-noinstr functions goes here */ instrumentation_end(); } (note the double negative in the comment)
* Peter Zijlstra <peterz@infradead.org> wrote: > Nope, they do as they say on the tin. > > noinstr void foo(void) > { > } > > declares the whole function as non-instrumented. > > Within such functions, we demark regions where instrumentation is > allowed by: > > noinstr void foo(void) > { > instrumentation_begin(); > /* code that calls non-noinstr functions goes here */ > instrumentation_end(); Indeed, you are right - I should have gotten more of my morning tea ... :-/ Thanks, Ingo
--- a/arch/x86/power/cpu.c +++ b/arch/x86/power/cpu.c @@ -192,7 +192,7 @@ static void fix_processor_context(void) * The asm code that gets us here will have restored a usable GDT, although * it will be pointing to the wrong alias. */ -static void notrace __restore_processor_state(struct saved_context *ctxt) +static __always_inline void __restore_processor_state(struct saved_context *ctxt) { struct cpuinfo_x86 *c; @@ -235,6 +235,13 @@ static void notrace __restore_processor_ loadsegment(fs, __KERNEL_PERCPU); #endif + /* + * Definitely wrong, but at this point we should have at least enough + * to do CALL/RET (consider SKL callthunks) and this avoids having + * to deal with the noinstr explosion for now :/ + */ + instrumentation_begin(); + /* Restore the TSS, RO GDT, LDT, and usermode-relevant MSRs. */ fix_processor_context(); @@ -276,10 +283,12 @@ static void notrace __restore_processor_ * because some of the MSRs are "emulated" in microcode. */ msr_restore_context(ctxt); + + instrumentation_end(); } /* Needed by apm.c */ -void notrace restore_processor_state(void) +void noinstr restore_processor_state(void) { __restore_processor_state(&saved_context); }
Ensure no compiler instrumentation sneaks in while restoring the CPU state. Specifically we can't handle CALL/RET until GS is restored. Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> --- arch/x86/power/cpu.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)