Re: [PATCH v5] x86/msr: Add write msr notrace to avoid the debug codes splash
2016-10-26 19:50 GMT+08:00 Borislav Petkov : > On Wed, Oct 26, 2016 at 07:35:26PM +0800, Wanpeng Li wrote: >> From: Wanpeng Li >> >> As Peterz pointed out: >> >> | The thing is, many many smp_reschedule_interrupt() invocations don't >> | actually execute anything much at all and are only send to tickle the >> | return to user path (which does the actual preemption). >> >> This patch add write msr notrace to avoid the debug codes splash. >> >> Suggested-by: Peter Zijlstra >> Suggested-by: Paolo Bonzini >> Cc: Ingo Molnar >> Cc: Mike Galbraith >> Cc: Peter Zijlstra >> Cc: Thomas Gleixner >> Cc: Paolo Bonzini >> Signed-off-by: Wanpeng Li >> --- >> arch/x86/include/asm/apic.h | 3 ++- >> arch/x86/include/asm/msr.h | 15 +++ >> arch/x86/kernel/apic/apic.c | 1 + >> arch/x86/kernel/kvm.c | 4 ++-- >> arch/x86/kernel/smp.c | 2 -- >> 5 files changed, 20 insertions(+), 5 deletions(-) >> >> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h >> index f5aaf6c..a5a0bcf 100644 >> --- a/arch/x86/include/asm/apic.h >> +++ b/arch/x86/include/asm/apic.h >> @@ -196,7 +196,7 @@ static inline void native_apic_msr_write(u32 reg, u32 v) >> >> static inline void native_apic_msr_eoi_write(u32 reg, u32 v) >> { >> - wrmsr(APIC_BASE_MSR + (APIC_EOI >> 4), APIC_EOI_ACK, 0); >> + wrmsr_notrace(APIC_BASE_MSR + (APIC_EOI >> 4), APIC_EOI_ACK, 0); >> } >> >> static inline u32 native_apic_msr_read(u32 reg) >> @@ -332,6 +332,7 @@ struct apic { >>* on write for EOI. >>*/ >> void (*eoi_write)(u32 reg, u32 v); >> + void (*native_eoi_write)(u32 reg, u32 v); >> u64 (*icr_read)(void); >> void (*icr_write)(u32 low, u32 high); >> void (*wait_icr_idle)(void); >> diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h >> index b5fee97..afbb221 100644 >> --- a/arch/x86/include/asm/msr.h >> +++ b/arch/x86/include/asm/msr.h >> @@ -127,6 +127,21 @@ notrace static inline void native_write_msr(unsigned >> int msr, >> } >> >> /* Can be uninlined because referenced by paravirt */ >> +notrace static inline void native_write_msr_notrace(unsigned int msr, >> + unsigned low, unsigned high) >> +{ >> + asm volatile("1: wrmsr\n" >> + "2:\n" >> + _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_wrmsr_unsafe) >> + : : "c" (msr), "a"(low), "d" (high) : "memory"); >> +} > > Why is this duplicating the inline asm? > > This function should be called __native_write_msr_notrace() instead and > then native_write_msr() should do: > > notrace static inline void native_write_msr(unsigned int msr, > unsigned low, unsigned > high) > { > __native_write_msr_notrace(msr, low, high); > if (msr_tracepoint_active(__tracepoint_write_msr)) > do_trace_write_msr(msr, ((u64)high << 32 | low), 0); > } > > This way it is way more clear who calls what. > > And this thing should be two patches anyway: > > 1. change native_write_msr() > 2. Add the apic changes. Thanks for pointing out. Regards, Wanpeng Li
Re: [PATCH v5] x86/msr: Add write msr notrace to avoid the debug codes splash
On Wed, Oct 26, 2016 at 07:35:26PM +0800, Wanpeng Li wrote: > From: Wanpeng Li > > As Peterz pointed out: > > | The thing is, many many smp_reschedule_interrupt() invocations don't > | actually execute anything much at all and are only send to tickle the > | return to user path (which does the actual preemption). > > This patch add write msr notrace to avoid the debug codes splash. > > Suggested-by: Peter Zijlstra > Suggested-by: Paolo Bonzini > Cc: Ingo Molnar > Cc: Mike Galbraith > Cc: Peter Zijlstra > Cc: Thomas Gleixner > Cc: Paolo Bonzini > Signed-off-by: Wanpeng Li > --- > arch/x86/include/asm/apic.h | 3 ++- > arch/x86/include/asm/msr.h | 15 +++ > arch/x86/kernel/apic/apic.c | 1 + > arch/x86/kernel/kvm.c | 4 ++-- > arch/x86/kernel/smp.c | 2 -- > 5 files changed, 20 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h > index f5aaf6c..a5a0bcf 100644 > --- a/arch/x86/include/asm/apic.h > +++ b/arch/x86/include/asm/apic.h > @@ -196,7 +196,7 @@ static inline void native_apic_msr_write(u32 reg, u32 v) > > static inline void native_apic_msr_eoi_write(u32 reg, u32 v) > { > - wrmsr(APIC_BASE_MSR + (APIC_EOI >> 4), APIC_EOI_ACK, 0); > + wrmsr_notrace(APIC_BASE_MSR + (APIC_EOI >> 4), APIC_EOI_ACK, 0); > } > > static inline u32 native_apic_msr_read(u32 reg) > @@ -332,6 +332,7 @@ struct apic { >* on write for EOI. >*/ > void (*eoi_write)(u32 reg, u32 v); > + void (*native_eoi_write)(u32 reg, u32 v); > u64 (*icr_read)(void); > void (*icr_write)(u32 low, u32 high); > void (*wait_icr_idle)(void); > diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h > index b5fee97..afbb221 100644 > --- a/arch/x86/include/asm/msr.h > +++ b/arch/x86/include/asm/msr.h > @@ -127,6 +127,21 @@ notrace static inline void native_write_msr(unsigned int > msr, > } > > /* Can be uninlined because referenced by paravirt */ > +notrace static inline void native_write_msr_notrace(unsigned int msr, > + unsigned low, unsigned high) > +{ > + asm volatile("1: wrmsr\n" > + "2:\n" > + _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_wrmsr_unsafe) > + : : "c" (msr), "a"(low), "d" (high) : "memory"); > +} Why is this duplicating the inline asm? This function should be called __native_write_msr_notrace() instead and then native_write_msr() should do: notrace static inline void native_write_msr(unsigned int msr, unsigned low, unsigned high) { __native_write_msr_notrace(msr, low, high); if (msr_tracepoint_active(__tracepoint_write_msr)) do_trace_write_msr(msr, ((u64)high << 32 | low), 0); } This way it is way more clear who calls what. And this thing should be two patches anyway: 1. change native_write_msr() 2. Add the apic changes. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply.
Re: [PATCH v5] x86/msr: Add write msr notrace to avoid the debug codes splash
On 26/10/2016 13:35, Wanpeng Li wrote: > From: Wanpeng Li > > As Peterz pointed out: > > | The thing is, many many smp_reschedule_interrupt() invocations don't > | actually execute anything much at all and are only send to tickle the > | return to user path (which does the actual preemption). > > This patch add write msr notrace to avoid the debug codes splash. > > Suggested-by: Peter Zijlstra > Suggested-by: Paolo Bonzini > Cc: Ingo Molnar > Cc: Mike Galbraith > Cc: Peter Zijlstra > Cc: Thomas Gleixner > Cc: Paolo Bonzini > Signed-off-by: Wanpeng Li > --- > arch/x86/include/asm/apic.h | 3 ++- > arch/x86/include/asm/msr.h | 15 +++ > arch/x86/kernel/apic/apic.c | 1 + > arch/x86/kernel/kvm.c | 4 ++-- > arch/x86/kernel/smp.c | 2 -- > 5 files changed, 20 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h > index f5aaf6c..a5a0bcf 100644 > --- a/arch/x86/include/asm/apic.h > +++ b/arch/x86/include/asm/apic.h > @@ -196,7 +196,7 @@ static inline void native_apic_msr_write(u32 reg, u32 v) > > static inline void native_apic_msr_eoi_write(u32 reg, u32 v) > { > - wrmsr(APIC_BASE_MSR + (APIC_EOI >> 4), APIC_EOI_ACK, 0); > + wrmsr_notrace(APIC_BASE_MSR + (APIC_EOI >> 4), APIC_EOI_ACK, 0); > } > > static inline u32 native_apic_msr_read(u32 reg) > @@ -332,6 +332,7 @@ struct apic { >* on write for EOI. >*/ > void (*eoi_write)(u32 reg, u32 v); > + void (*native_eoi_write)(u32 reg, u32 v); > u64 (*icr_read)(void); > void (*icr_write)(u32 low, u32 high); > void (*wait_icr_idle)(void); > diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h > index b5fee97..afbb221 100644 > --- a/arch/x86/include/asm/msr.h > +++ b/arch/x86/include/asm/msr.h > @@ -127,6 +127,21 @@ notrace static inline void native_write_msr(unsigned int > msr, > } > > /* Can be uninlined because referenced by paravirt */ > +notrace static inline void native_write_msr_notrace(unsigned int msr, > + unsigned low, unsigned high) > +{ > + asm volatile("1: wrmsr\n" > + "2:\n" > + _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_wrmsr_unsafe) > + : : "c" (msr), "a"(low), "d" (high) : "memory"); > +} > + > +static inline void wrmsr_notrace(unsigned msr, unsigned low, unsigned high) > +{ > + native_write_msr_notrace(msr, low, high); > +} > + > +/* Can be uninlined because referenced by paravirt */ > notrace static inline int native_write_msr_safe(unsigned int msr, > unsigned low, unsigned high) > { > diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c > index 88c657b..2686894 100644 > --- a/arch/x86/kernel/apic/apic.c > +++ b/arch/x86/kernel/apic/apic.c > @@ -2263,6 +2263,7 @@ void __init apic_set_eoi_write(void (*eoi_write)(u32 > reg, u32 v)) > for (drv = __apicdrivers; drv < __apicdrivers_end; drv++) { > /* Should happen once for each apic */ > WARN_ON((*drv)->eoi_write == eoi_write); > + (*drv)->native_eoi_write = (*drv)->eoi_write; > (*drv)->eoi_write = eoi_write; > } > } > diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c > index edbbfc8..d230513 100644 > --- a/arch/x86/kernel/kvm.c > +++ b/arch/x86/kernel/kvm.c > @@ -308,7 +308,7 @@ static void kvm_register_steal_time(void) > > static DEFINE_PER_CPU(unsigned long, kvm_apic_eoi) = KVM_PV_EOI_DISABLED; > > -static void kvm_guest_apic_eoi_write(u32 reg, u32 val) > +notrace static void kvm_guest_apic_eoi_write(u32 reg, u32 val) > { > /** >* This relies on __test_and_clear_bit to modify the memory > @@ -319,7 +319,7 @@ static void kvm_guest_apic_eoi_write(u32 reg, u32 val) >*/ > if (__test_and_clear_bit(KVM_PV_EOI_BIT, this_cpu_ptr(&kvm_apic_eoi))) > return; > - apic_write(APIC_EOI, APIC_EOI_ACK); > + apic->native_eoi_write(APIC_EOI, APIC_EOI_ACK); > } > > static void kvm_guest_cpu_init(void) > diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c > index c00cb64..68f8cc2 100644 > --- a/arch/x86/kernel/smp.c > +++ b/arch/x86/kernel/smp.c > @@ -261,10 +261,8 @@ static inline void __smp_reschedule_interrupt(void) > > __visible void smp_reschedule_interrupt(struct pt_regs *regs) > { > - irq_enter(); > ack_APIC_irq(); > __smp_reschedule_interrupt(); > - irq_exit(); > /* >* KVM uses this interrupt to force a cpu out of guest mode >*/ > Acked-by: Paolo Bonzini
[PATCH v5] x86/msr: Add write msr notrace to avoid the debug codes splash
From: Wanpeng Li As Peterz pointed out: | The thing is, many many smp_reschedule_interrupt() invocations don't | actually execute anything much at all and are only send to tickle the | return to user path (which does the actual preemption). This patch add write msr notrace to avoid the debug codes splash. Suggested-by: Peter Zijlstra Suggested-by: Paolo Bonzini Cc: Ingo Molnar Cc: Mike Galbraith Cc: Peter Zijlstra Cc: Thomas Gleixner Cc: Paolo Bonzini Signed-off-by: Wanpeng Li --- arch/x86/include/asm/apic.h | 3 ++- arch/x86/include/asm/msr.h | 15 +++ arch/x86/kernel/apic/apic.c | 1 + arch/x86/kernel/kvm.c | 4 ++-- arch/x86/kernel/smp.c | 2 -- 5 files changed, 20 insertions(+), 5 deletions(-) diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h index f5aaf6c..a5a0bcf 100644 --- a/arch/x86/include/asm/apic.h +++ b/arch/x86/include/asm/apic.h @@ -196,7 +196,7 @@ static inline void native_apic_msr_write(u32 reg, u32 v) static inline void native_apic_msr_eoi_write(u32 reg, u32 v) { - wrmsr(APIC_BASE_MSR + (APIC_EOI >> 4), APIC_EOI_ACK, 0); + wrmsr_notrace(APIC_BASE_MSR + (APIC_EOI >> 4), APIC_EOI_ACK, 0); } static inline u32 native_apic_msr_read(u32 reg) @@ -332,6 +332,7 @@ struct apic { * on write for EOI. */ void (*eoi_write)(u32 reg, u32 v); + void (*native_eoi_write)(u32 reg, u32 v); u64 (*icr_read)(void); void (*icr_write)(u32 low, u32 high); void (*wait_icr_idle)(void); diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h index b5fee97..afbb221 100644 --- a/arch/x86/include/asm/msr.h +++ b/arch/x86/include/asm/msr.h @@ -127,6 +127,21 @@ notrace static inline void native_write_msr(unsigned int msr, } /* Can be uninlined because referenced by paravirt */ +notrace static inline void native_write_msr_notrace(unsigned int msr, + unsigned low, unsigned high) +{ + asm volatile("1: wrmsr\n" +"2:\n" +_ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_wrmsr_unsafe) +: : "c" (msr), "a"(low), "d" (high) : "memory"); +} + +static inline void wrmsr_notrace(unsigned msr, unsigned low, unsigned high) +{ + native_write_msr_notrace(msr, low, high); +} + +/* Can be uninlined because referenced by paravirt */ notrace static inline int native_write_msr_safe(unsigned int msr, unsigned low, unsigned high) { diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c index 88c657b..2686894 100644 --- a/arch/x86/kernel/apic/apic.c +++ b/arch/x86/kernel/apic/apic.c @@ -2263,6 +2263,7 @@ void __init apic_set_eoi_write(void (*eoi_write)(u32 reg, u32 v)) for (drv = __apicdrivers; drv < __apicdrivers_end; drv++) { /* Should happen once for each apic */ WARN_ON((*drv)->eoi_write == eoi_write); + (*drv)->native_eoi_write = (*drv)->eoi_write; (*drv)->eoi_write = eoi_write; } } diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c index edbbfc8..d230513 100644 --- a/arch/x86/kernel/kvm.c +++ b/arch/x86/kernel/kvm.c @@ -308,7 +308,7 @@ static void kvm_register_steal_time(void) static DEFINE_PER_CPU(unsigned long, kvm_apic_eoi) = KVM_PV_EOI_DISABLED; -static void kvm_guest_apic_eoi_write(u32 reg, u32 val) +notrace static void kvm_guest_apic_eoi_write(u32 reg, u32 val) { /** * This relies on __test_and_clear_bit to modify the memory @@ -319,7 +319,7 @@ static void kvm_guest_apic_eoi_write(u32 reg, u32 val) */ if (__test_and_clear_bit(KVM_PV_EOI_BIT, this_cpu_ptr(&kvm_apic_eoi))) return; - apic_write(APIC_EOI, APIC_EOI_ACK); + apic->native_eoi_write(APIC_EOI, APIC_EOI_ACK); } static void kvm_guest_cpu_init(void) diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c index c00cb64..68f8cc2 100644 --- a/arch/x86/kernel/smp.c +++ b/arch/x86/kernel/smp.c @@ -261,10 +261,8 @@ static inline void __smp_reschedule_interrupt(void) __visible void smp_reschedule_interrupt(struct pt_regs *regs) { - irq_enter(); ack_APIC_irq(); __smp_reschedule_interrupt(); - irq_exit(); /* * KVM uses this interrupt to force a cpu out of guest mode */ -- 1.9.1