[PATCH] x86/smp: Add irq_enter/exit() in smp_reschedule_interrupt()

[PATCH] x86/smp: Add irq_enter/exit() in smp_reschedule_interrupt()

[Wanpeng Li]

 ===============================
 [ INFO: suspicious RCU usage. ]
 4.8.0+ #24 Not tainted
 -------------------------------
 ./arch/x86/include/asm/msr-trace.h:47 suspicious rcu_dereference_check() usage!
 
 other info that might help us debug this:
 
 
 RCU used illegally from idle CPU!
 rcu_scheduler_active = 1, debug_locks = 0
 RCU used illegally from extended quiescent state!
 no locks held by swapper/1/0.
 
 stack backtrace:
 CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.8.0+ #24
 Hardware name: Dell Inc. OptiPlex 7020/0F5C5X, BIOS A03 01/08/2015
  ffff90285de03f58 ffffffff9d44a0c9 ffff90285ca5d100 0000000000000001
  ffff90285de03f88 ffffffff9d0ebd67 ffff902845165410 000000000000080b
  0000000000000000 0000000000000000 ffff90285de03fb8 ffffffff9d492b95
 Call Trace:
  <IRQ>  [<ffffffff9d44a0c9>] dump_stack+0x99/0xd0
  [<ffffffff9d0ebd67>] lockdep_rcu_suspicious+0xe7/0x120
  [<ffffffff9d492b95>] do_trace_write_msr+0x135/0x140
  [<ffffffff9d06f860>] native_write_msr+0x20/0x30
  [<ffffffff9d065fad>] native_apic_msr_eoi_write+0x1d/0x30
  [<ffffffff9d05bd1d>] smp_reschedule_interrupt+0x1d/0x30
  [<ffffffff9d8daec6>] reschedule_interrupt+0x96/0xa0
  <EOI>  [<ffffffff9d732634>] ? cpuidle_enter_state+0xe4/0x360
  [<ffffffff9d73261f>] ? cpuidle_enter_state+0xcf/0x360
  [<ffffffff9d7328e7>] cpuidle_enter+0x17/0x20
  [<ffffffff9d0e1a73>] call_cpuidle+0x23/0x50
  [<ffffffff9d0e1d0c>] cpu_startup_entry+0x15c/0x280
  [<ffffffff9d05ce64>] start_secondary+0x154/0x180

Reschedule interrupt may be called in cpu idle state. This causes lockdep 
check warning above. 

Add irq_enter/exit() in smp_reschedule_interrupt(), irq_enter() tells the RCU 
subsystems to end the extended quiescent state, so the following trace call in 
ack_APIC_irq() works correctly.

Cc: Ingo Molnar <mingo@kernel.org>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
 arch/x86/kernel/smp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
index 658777c..ac2ee87 100644
--- a/arch/x86/kernel/smp.c
+++ b/arch/x86/kernel/smp.c
@@ -259,8 +259,10 @@ 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

[tip:x86/urgent] x86/smp: Add irq_enter/exit() in smp_reschedule_interrupt()

[tip-bot for Wanpeng Li]

Commit-ID:  1ec6ec14a2943f6f611fc1d5fb2d4eaa85bd9d72
Gitweb:     http://git.kernel.org/tip/1ec6ec14a2943f6f611fc1d5fb2d4eaa85bd9d72
Author:     Wanpeng Li <kernellwp@gmail.com>
AuthorDate: Fri, 14 Oct 2016 09:48:53 +0800
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Fri, 14 Oct 2016 14:14:20 +0200

x86/smp: Add irq_enter/exit() in smp_reschedule_interrupt()

 ===============================
 [ INFO: suspicious RCU usage. ]
 4.8.0+ #24 Not tainted
 -------------------------------
 ./arch/x86/include/asm/msr-trace.h:47 suspicious rcu_dereference_check() usage!
 
 other info that might help us debug this:
 
 RCU used illegally from idle CPU!
 rcu_scheduler_active = 1, debug_locks = 0
 RCU used illegally from extended quiescent state!
 no locks held by swapper/1/0.
 
  [<ffffffff9d492b95>] do_trace_write_msr+0x135/0x140
  [<ffffffff9d06f860>] native_write_msr+0x20/0x30
  [<ffffffff9d065fad>] native_apic_msr_eoi_write+0x1d/0x30
  [<ffffffff9d05bd1d>] smp_reschedule_interrupt+0x1d/0x30
  [<ffffffff9d8daec6>] reschedule_interrupt+0x96/0xa0

Reschedule interrupt may be called in cpu idle state. This causes lockdep 
check warning above. 

Add irq_enter/exit() in smp_reschedule_interrupt(), irq_enter() tells the RCU 
subsystems to end the extended quiescent state, so the following trace call in 
ack_APIC_irq() works correctly.

Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Mike Galbraith <efault@gmx.de>
Link: http://lkml.kernel.org/r/1476409733-5133-1-git-send-email-wanpeng.li@hotmail.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 arch/x86/kernel/smp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
index 658777c..ac2ee87 100644
--- a/arch/x86/kernel/smp.c
+++ b/arch/x86/kernel/smp.c
@@ -259,8 +259,10 @@ 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
 	 */

Re: [PATCH] x86/smp: Add irq_enter/exit() in smp_reschedule_interrupt()

[Peter Zijlstra]

On Fri, Oct 14, 2016 at 09:48:53AM +0800, Wanpeng Li wrote:
>  ===============================
>  [ INFO: suspicious RCU usage. ]
>  4.8.0+ #24 Not tainted
>  -------------------------------
>  ./arch/x86/include/asm/msr-trace.h:47 suspicious rcu_dereference_check() usage!
>  
>  other info that might help us debug this:
>  
>  
>  RCU used illegally from idle CPU!
>  rcu_scheduler_active = 1, debug_locks = 0
>  RCU used illegally from extended quiescent state!
>  no locks held by swapper/1/0.
>  
>  stack backtrace:
>  CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.8.0+ #24
>  Hardware name: Dell Inc. OptiPlex 7020/0F5C5X, BIOS A03 01/08/2015
>   ffff90285de03f58 ffffffff9d44a0c9 ffff90285ca5d100 0000000000000001
>   ffff90285de03f88 ffffffff9d0ebd67 ffff902845165410 000000000000080b
>   0000000000000000 0000000000000000 ffff90285de03fb8 ffffffff9d492b95
>  Call Trace:
>   <IRQ>  [<ffffffff9d44a0c9>] dump_stack+0x99/0xd0
>   [<ffffffff9d0ebd67>] lockdep_rcu_suspicious+0xe7/0x120
>   [<ffffffff9d492b95>] do_trace_write_msr+0x135/0x140
>   [<ffffffff9d06f860>] native_write_msr+0x20/0x30
>   [<ffffffff9d065fad>] native_apic_msr_eoi_write+0x1d/0x30
>   [<ffffffff9d05bd1d>] smp_reschedule_interrupt+0x1d/0x30
>   [<ffffffff9d8daec6>] reschedule_interrupt+0x96/0xa0
>   <EOI>  [<ffffffff9d732634>] ? cpuidle_enter_state+0xe4/0x360
>   [<ffffffff9d73261f>] ? cpuidle_enter_state+0xcf/0x360
>   [<ffffffff9d7328e7>] cpuidle_enter+0x17/0x20
>   [<ffffffff9d0e1a73>] call_cpuidle+0x23/0x50
>   [<ffffffff9d0e1d0c>] cpu_startup_entry+0x15c/0x280
>   [<ffffffff9d05ce64>] start_secondary+0x154/0x180
> 
> Reschedule interrupt may be called in cpu idle state. This causes lockdep 
> check warning above. 
> 
> Add irq_enter/exit() in smp_reschedule_interrupt(), irq_enter() tells the RCU 
> subsystems to end the extended quiescent state, so the following trace call in 
> ack_APIC_irq() works correctly.
> 
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Mike Galbraith <efault@gmx.de>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> ---
>  arch/x86/kernel/smp.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
> index 658777c..ac2ee87 100644
> --- a/arch/x86/kernel/smp.c
> +++ b/arch/x86/kernel/smp.c
> @@ -259,8 +259,10 @@ 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();

Urgh, I really hate this...

So now we're making a very frequent interrupt slower because of debug
code :/

Re: [PATCH] x86/smp: Add irq_enter/exit() in smp_reschedule_interrupt()

[Wanpeng Li]

2016-10-16 21:39 GMT+08:00 Peter Zijlstra <peterz@infradead.org>:
> On Fri, Oct 14, 2016 at 09:48:53AM +0800, Wanpeng Li wrote:
>>  ===============================
>>  [ INFO: suspicious RCU usage. ]
>>  4.8.0+ #24 Not tainted
>>  -------------------------------
>>  ./arch/x86/include/asm/msr-trace.h:47 suspicious rcu_dereference_check() usage!
>>
>>  other info that might help us debug this:
>>
>>
>>  RCU used illegally from idle CPU!
>>  rcu_scheduler_active = 1, debug_locks = 0
>>  RCU used illegally from extended quiescent state!
>>  no locks held by swapper/1/0.
>>
>>  stack backtrace:
>>  CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.8.0+ #24
>>  Hardware name: Dell Inc. OptiPlex 7020/0F5C5X, BIOS A03 01/08/2015
>>   ffff90285de03f58 ffffffff9d44a0c9 ffff90285ca5d100 0000000000000001
>>   ffff90285de03f88 ffffffff9d0ebd67 ffff902845165410 000000000000080b
>>   0000000000000000 0000000000000000 ffff90285de03fb8 ffffffff9d492b95
>>  Call Trace:
>>   <IRQ>  [<ffffffff9d44a0c9>] dump_stack+0x99/0xd0
>>   [<ffffffff9d0ebd67>] lockdep_rcu_suspicious+0xe7/0x120
>>   [<ffffffff9d492b95>] do_trace_write_msr+0x135/0x140
>>   [<ffffffff9d06f860>] native_write_msr+0x20/0x30
>>   [<ffffffff9d065fad>] native_apic_msr_eoi_write+0x1d/0x30
>>   [<ffffffff9d05bd1d>] smp_reschedule_interrupt+0x1d/0x30
>>   [<ffffffff9d8daec6>] reschedule_interrupt+0x96/0xa0
>>   <EOI>  [<ffffffff9d732634>] ? cpuidle_enter_state+0xe4/0x360
>>   [<ffffffff9d73261f>] ? cpuidle_enter_state+0xcf/0x360
>>   [<ffffffff9d7328e7>] cpuidle_enter+0x17/0x20
>>   [<ffffffff9d0e1a73>] call_cpuidle+0x23/0x50
>>   [<ffffffff9d0e1d0c>] cpu_startup_entry+0x15c/0x280
>>   [<ffffffff9d05ce64>] start_secondary+0x154/0x180
>>
>> Reschedule interrupt may be called in cpu idle state. This causes lockdep
>> check warning above.
>>
>> Add irq_enter/exit() in smp_reschedule_interrupt(), irq_enter() tells the RCU
>> subsystems to end the extended quiescent state, so the following trace call in
>> ack_APIC_irq() works correctly.
>>
>> Cc: Ingo Molnar <mingo@kernel.org>
>> Cc: Mike Galbraith <efault@gmx.de>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
>> ---
>>  arch/x86/kernel/smp.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
>> index 658777c..ac2ee87 100644
>> --- a/arch/x86/kernel/smp.c
>> +++ b/arch/x86/kernel/smp.c
>> @@ -259,8 +259,10 @@ 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();
>
> Urgh, I really hate this...
>
> So now we're making a very frequent interrupt slower because of debug
> code :/

Do you have a better idea? :)

Regards,
Wanpeng Li

Re: [PATCH] x86/smp: Add irq_enter/exit() in smp_reschedule_interrupt()

[Peter Zijlstra]

On Mon, Oct 17, 2016 at 12:19:43PM +0800, Wanpeng Li wrote:
> 2016-10-16 21:39 GMT+08:00 Peter Zijlstra <peterz@infradead.org>:

> >>   [<ffffffff9d492b95>] do_trace_write_msr+0x135/0x140
> >>   [<ffffffff9d06f860>] native_write_msr+0x20/0x30
> >>   [<ffffffff9d065fad>] native_apic_msr_eoi_write+0x1d/0x30
> >>   [<ffffffff9d05bd1d>] smp_reschedule_interrupt+0x1d/0x30
> >>   [<ffffffff9d8daec6>] reschedule_interrupt+0x96/0xa0

> >>  __visible void smp_reschedule_interrupt(struct pt_regs *regs)
> >>  {
> >> +     irq_enter();
> >>       ack_APIC_irq();
> >>       __smp_reschedule_interrupt();
> >> +     irq_exit();
> >
> > Urgh, I really hate this...
> >
> > So now we're making a very frequent interrupt slower because of debug
> > code :/
> 
> Do you have a better idea? :)

Something like the below avoids all that. Paravirt will still need fixing.

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).

Having to do the whole irq_enter/irq_exit dance just for this unlikely
debug case totally blows.

---
 arch/x86/include/asm/apic.h |  2 +-
 arch/x86/include/asm/msr.h  | 15 +++++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index f5aaf6c83222..b97bfeed6456 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)
diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
index b5fee97813cd..45c080449d5b 100644
--- a/arch/x86/include/asm/msr.h
+++ b/arch/x86/include/asm/msr.h
@@ -127,6 +127,16 @@ 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");
+}
+
+/* Can be uninlined because referenced by paravirt */
 notrace static inline int native_write_msr_safe(unsigned int msr,
 					unsigned low, unsigned high)
 {
@@ -228,6 +238,11 @@ static inline void wrmsr(unsigned msr, unsigned low, unsigned high)
 	native_write_msr(msr, low, high);
 }
 
+static inline void wrmsr_notrace(unsigned msr, unsigned low, unsigned high)
+{
+	native_write_msr_notrace(msr, low, high);
+}
+
 #define rdmsrl(msr, val)			\
 	((val) = native_read_msr((msr)))
 

Re: [PATCH] x86/smp: Add irq_enter/exit() in smp_reschedule_interrupt()

[Wanpeng Li]

Cc Paolo,
2016-10-17 16:22 GMT+08:00 Peter Zijlstra <peterz@infradead.org>:
> On Mon, Oct 17, 2016 at 12:19:43PM +0800, Wanpeng Li wrote:
>> 2016-10-16 21:39 GMT+08:00 Peter Zijlstra <peterz@infradead.org>:
>
>> >>   [<ffffffff9d492b95>] do_trace_write_msr+0x135/0x140
>> >>   [<ffffffff9d06f860>] native_write_msr+0x20/0x30
>> >>   [<ffffffff9d065fad>] native_apic_msr_eoi_write+0x1d/0x30
>> >>   [<ffffffff9d05bd1d>] smp_reschedule_interrupt+0x1d/0x30
>> >>   [<ffffffff9d8daec6>] reschedule_interrupt+0x96/0xa0
>
>> >>  __visible void smp_reschedule_interrupt(struct pt_regs *regs)
>> >>  {
>> >> +     irq_enter();
>> >>       ack_APIC_irq();
>> >>       __smp_reschedule_interrupt();
>> >> +     irq_exit();
>> >
>> > Urgh, I really hate this...
>> >
>> > So now we're making a very frequent interrupt slower because of debug
>> > code :/
>>
>> Do you have a better idea? :)
>
> Something like the below avoids all that. Paravirt will still need fixing.

kvm_guest_apic_eoi_write
 -> native_apic_msr_write

I think you can replace the wrmsr in native_apic_msr_write() by your
wrmsr_notrace().

Regards,
Wanpeng Li

>
> 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).
>
> Having to do the whole irq_enter/irq_exit dance just for this unlikely
> debug case totally blows.
>
> ---
>  arch/x86/include/asm/apic.h |  2 +-
>  arch/x86/include/asm/msr.h  | 15 +++++++++++++++
>  2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
> index f5aaf6c83222..b97bfeed6456 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)
> diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
> index b5fee97813cd..45c080449d5b 100644
> --- a/arch/x86/include/asm/msr.h
> +++ b/arch/x86/include/asm/msr.h
> @@ -127,6 +127,16 @@ 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");
> +}
> +
> +/* Can be uninlined because referenced by paravirt */
>  notrace static inline int native_write_msr_safe(unsigned int msr,
>                                         unsigned low, unsigned high)
>  {
> @@ -228,6 +238,11 @@ static inline void wrmsr(unsigned msr, unsigned low, unsigned high)
>         native_write_msr(msr, low, high);
>  }
>
> +static inline void wrmsr_notrace(unsigned msr, unsigned low, unsigned high)
> +{
> +       native_write_msr_notrace(msr, low, high);
> +}
> +
>  #define rdmsrl(msr, val)                       \
>         ((val) = native_read_msr((msr)))
>

Re: [PATCH] x86/smp: Add irq_enter/exit() in smp_reschedule_interrupt()

[Paolo Bonzini]

----- Original Message -----
> From: "Wanpeng Li" <kernellwp@gmail.com>
> To: "Peter Zijlstra" <peterz@infradead.org>
> Cc: linux-kernel@vger.kernel.org, "Wanpeng Li" <wanpeng.li@hotmail.com>, "Ingo Molnar" <mingo@kernel.org>, "Mike
> Galbraith" <efault@gmx.de>, "Thomas Gleixner" <tglx@linutronix.de>, "Paolo Bonzini" <pbonzini@redhat.com>
> Sent: Monday, October 17, 2016 11:45:32 AM
> Subject: Re: [PATCH] x86/smp: Add irq_enter/exit() in smp_reschedule_interrupt()
> 
> Cc Paolo,
> 2016-10-17 16:22 GMT+08:00 Peter Zijlstra <peterz@infradead.org>:
> > On Mon, Oct 17, 2016 at 12:19:43PM +0800, Wanpeng Li wrote:
> >> 2016-10-16 21:39 GMT+08:00 Peter Zijlstra <peterz@infradead.org>:
> >
> >> >>   [<ffffffff9d492b95>] do_trace_write_msr+0x135/0x140
> >> >>   [<ffffffff9d06f860>] native_write_msr+0x20/0x30
> >> >>   [<ffffffff9d065fad>] native_apic_msr_eoi_write+0x1d/0x30
> >> >>   [<ffffffff9d05bd1d>] smp_reschedule_interrupt+0x1d/0x30
> >> >>   [<ffffffff9d8daec6>] reschedule_interrupt+0x96/0xa0
> >
> >> >>  __visible void smp_reschedule_interrupt(struct pt_regs *regs)
> >> >>  {
> >> >> +     irq_enter();
> >> >>       ack_APIC_irq();
> >> >>       __smp_reschedule_interrupt();
> >> >> +     irq_exit();
> >> >
> >> > Urgh, I really hate this...
> >> >
> >> > So now we're making a very frequent interrupt slower because of debug
> >> > code :/
> >>
> >> Do you have a better idea? :)
> >
> > Something like the below avoids all that. Paravirt will still need fixing.
> 
> kvm_guest_apic_eoi_write
>  -> native_apic_msr_write

kvm_guest_apic_eoi_write can use native_apic_msr_eoi_write too:

diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index f5aaf6c83222..9769d76a62c4 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -174,7 +174,7 @@ static inline void disable_local_APIC(void) { }
 static inline void lapic_update_tsc_freq(void) { }
 #endif /* !CONFIG_X86_LOCAL_APIC */
 
-#ifdef CONFIG_X86_X2APIC
+#if defined CONFIG_X86_X2APIC || defined CONFIG_KVM_GUEST
 /*
  * Make previous memory operations globally visible before
  * sending the IPI through x2apic wrmsr. We need a serializing instruction or
diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index edbbfc854e39..61cc6a5e3f44 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -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);
+	native_apic_msr_eoi_write(APIC_EOI, APIC_EOI_ACK);
 }
 
 static void kvm_guest_cpu_init(void)


Thanks,

Paolo

> I think you can replace the wrmsr in native_apic_msr_write() by your
> wrmsr_notrace().
> 
> Regards,
> Wanpeng Li
> 
> >
> > 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).
> >
> > Having to do the whole irq_enter/irq_exit dance just for this unlikely
> > debug case totally blows.
> >
> > ---
> >  arch/x86/include/asm/apic.h |  2 +-
> >  arch/x86/include/asm/msr.h  | 15 +++++++++++++++
> >  2 files changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
> > index f5aaf6c83222..b97bfeed6456 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)
> > diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
> > index b5fee97813cd..45c080449d5b 100644
> > --- a/arch/x86/include/asm/msr.h
> > +++ b/arch/x86/include/asm/msr.h
> > @@ -127,6 +127,16 @@ 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");
> > +}
> > +
> > +/* Can be uninlined because referenced by paravirt */
> >  notrace static inline int native_write_msr_safe(unsigned int msr,
> >                                         unsigned low, unsigned high)
> >  {
> > @@ -228,6 +238,11 @@ static inline void wrmsr(unsigned msr, unsigned low,
> > unsigned high)
> >         native_write_msr(msr, low, high);
> >  }
> >
> > +static inline void wrmsr_notrace(unsigned msr, unsigned low, unsigned
> > high)
> > +{
> > +       native_write_msr_notrace(msr, low, high);
> > +}
> > +
> >  #define rdmsrl(msr, val)                       \
> >         ((val) = native_read_msr((msr)))
> >
> 

Re: [PATCH] x86/smp: Add irq_enter/exit() in smp_reschedule_interrupt()

[Wanpeng Li]

2016-10-17 18:08 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>
>
> ----- Original Message -----
>> From: "Wanpeng Li" <kernellwp@gmail.com>
>> To: "Peter Zijlstra" <peterz@infradead.org>
>> Cc: linux-kernel@vger.kernel.org, "Wanpeng Li" <wanpeng.li@hotmail.com>, "Ingo Molnar" <mingo@kernel.org>, "Mike
>> Galbraith" <efault@gmx.de>, "Thomas Gleixner" <tglx@linutronix.de>, "Paolo Bonzini" <pbonzini@redhat.com>
>> Sent: Monday, October 17, 2016 11:45:32 AM
>> Subject: Re: [PATCH] x86/smp: Add irq_enter/exit() in smp_reschedule_interrupt()
>>
>> Cc Paolo,
>> 2016-10-17 16:22 GMT+08:00 Peter Zijlstra <peterz@infradead.org>:
>> > On Mon, Oct 17, 2016 at 12:19:43PM +0800, Wanpeng Li wrote:
>> >> 2016-10-16 21:39 GMT+08:00 Peter Zijlstra <peterz@infradead.org>:
>> >
>> >> >>   [<ffffffff9d492b95>] do_trace_write_msr+0x135/0x140
>> >> >>   [<ffffffff9d06f860>] native_write_msr+0x20/0x30
>> >> >>   [<ffffffff9d065fad>] native_apic_msr_eoi_write+0x1d/0x30
>> >> >>   [<ffffffff9d05bd1d>] smp_reschedule_interrupt+0x1d/0x30
>> >> >>   [<ffffffff9d8daec6>] reschedule_interrupt+0x96/0xa0
>> >
>> >> >>  __visible void smp_reschedule_interrupt(struct pt_regs *regs)
>> >> >>  {
>> >> >> +     irq_enter();
>> >> >>       ack_APIC_irq();
>> >> >>       __smp_reschedule_interrupt();
>> >> >> +     irq_exit();
>> >> >
>> >> > Urgh, I really hate this...
>> >> >
>> >> > So now we're making a very frequent interrupt slower because of debug
>> >> > code :/
>> >>
>> >> Do you have a better idea? :)
>> >
>> > Something like the below avoids all that. Paravirt will still need fixing.
>>
>> kvm_guest_apic_eoi_write
>>  -> native_apic_msr_write
>
> kvm_guest_apic_eoi_write can use native_apic_msr_eoi_write too:
>
> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
> index f5aaf6c83222..9769d76a62c4 100644
> --- a/arch/x86/include/asm/apic.h
> +++ b/arch/x86/include/asm/apic.h
> @@ -174,7 +174,7 @@ static inline void disable_local_APIC(void) { }
>  static inline void lapic_update_tsc_freq(void) { }
>  #endif /* !CONFIG_X86_LOCAL_APIC */
>
> -#ifdef CONFIG_X86_X2APIC
> +#if defined CONFIG_X86_X2APIC || defined CONFIG_KVM_GUEST
>  /*
>   * Make previous memory operations globally visible before
>   * sending the IPI through x2apic wrmsr. We need a serializing instruction or
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index edbbfc854e39..61cc6a5e3f44 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -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);
> +       native_apic_msr_eoi_write(APIC_EOI, APIC_EOI_ACK);
>  }
>
>  static void kvm_guest_cpu_init(void)

I see, thanks Paolo and Peterz. :)

Regards,
Wanpeng Li

>
>
> Thanks,
>
> Paolo
>
>> I think you can replace the wrmsr in native_apic_msr_write() by your
>> wrmsr_notrace().
>>
>> Regards,
>> Wanpeng Li
>>
>> >
>> > 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).
>> >
>> > Having to do the whole irq_enter/irq_exit dance just for this unlikely
>> > debug case totally blows.
>> >
>> > ---
>> >  arch/x86/include/asm/apic.h |  2 +-
>> >  arch/x86/include/asm/msr.h  | 15 +++++++++++++++
>> >  2 files changed, 16 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
>> > index f5aaf6c83222..b97bfeed6456 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)
>> > diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
>> > index b5fee97813cd..45c080449d5b 100644
>> > --- a/arch/x86/include/asm/msr.h
>> > +++ b/arch/x86/include/asm/msr.h
>> > @@ -127,6 +127,16 @@ 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");
>> > +}
>> > +
>> > +/* Can be uninlined because referenced by paravirt */
>> >  notrace static inline int native_write_msr_safe(unsigned int msr,
>> >                                         unsigned low, unsigned high)
>> >  {
>> > @@ -228,6 +238,11 @@ static inline void wrmsr(unsigned msr, unsigned low,
>> > unsigned high)
>> >         native_write_msr(msr, low, high);
>> >  }
>> >
>> > +static inline void wrmsr_notrace(unsigned msr, unsigned low, unsigned
>> > high)
>> > +{
>> > +       native_write_msr_notrace(msr, low, high);
>> > +}
>> > +
>> >  #define rdmsrl(msr, val)                       \
>> >         ((val) = native_read_msr((msr)))
>> >
>>

Re: [PATCH] x86/smp: Add irq_enter/exit() in smp_reschedule_interrupt()

[Wanpeng Li]

2016-10-17 16:22 GMT+08:00 Peter Zijlstra <peterz@infradead.org>:
> On Mon, Oct 17, 2016 at 12:19:43PM +0800, Wanpeng Li wrote:
>> 2016-10-16 21:39 GMT+08:00 Peter Zijlstra <peterz@infradead.org>:
>
>> >>   [<ffffffff9d492b95>] do_trace_write_msr+0x135/0x140
>> >>   [<ffffffff9d06f860>] native_write_msr+0x20/0x30
>> >>   [<ffffffff9d065fad>] native_apic_msr_eoi_write+0x1d/0x30
>> >>   [<ffffffff9d05bd1d>] smp_reschedule_interrupt+0x1d/0x30
>> >>   [<ffffffff9d8daec6>] reschedule_interrupt+0x96/0xa0
>
>> >>  __visible void smp_reschedule_interrupt(struct pt_regs *regs)
>> >>  {
>> >> +     irq_enter();
>> >>       ack_APIC_irq();
>> >>       __smp_reschedule_interrupt();
>> >> +     irq_exit();
>> >
>> > Urgh, I really hate this...
>> >
>> > So now we're making a very frequent interrupt slower because of debug
>> > code :/
>>
>> Do you have a better idea? :)
>
> Something like the below avoids all that. Paravirt will still need fixing.
>
> 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).
>
> Having to do the whole irq_enter/irq_exit dance just for this unlikely
> debug case totally blows.
>
> ---
>  arch/x86/include/asm/apic.h |  2 +-
>  arch/x86/include/asm/msr.h  | 15 +++++++++++++++
>  2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
> index f5aaf6c83222..b97bfeed6456 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)
> diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
> index b5fee97813cd..45c080449d5b 100644
> --- a/arch/x86/include/asm/msr.h
> +++ b/arch/x86/include/asm/msr.h
> @@ -127,6 +127,16 @@ 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");
> +}
> +
> +/* Can be uninlined because referenced by paravirt */
>  notrace static inline int native_write_msr_safe(unsigned int msr,
>                                         unsigned low, unsigned high)
>  {
> @@ -228,6 +238,11 @@ static inline void wrmsr(unsigned msr, unsigned low, unsigned high)
>         native_write_msr(msr, low, high);
>  }
>
> +static inline void wrmsr_notrace(unsigned msr, unsigned low, unsigned high)
> +{
> +       native_write_msr_notrace(msr, low, high);
> +}

I move this out of !PARAVIRT since PARAVIRT also will use it.

Regards,
Wanpeng Li

> +
>  #define rdmsrl(msr, val)                       \
>         ((val) = native_read_msr((msr)))
>

Re: [PATCH] x86/smp: Add irq_enter/exit() in smp_reschedule_interrupt()

[Wanpeng Li]

2016-10-17 18:08 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
[...]
>> > Something like the below avoids all that. Paravirt will still need fixing.
>>
>> kvm_guest_apic_eoi_write
>>  -> native_apic_msr_write
>
> kvm_guest_apic_eoi_write can use native_apic_msr_eoi_write too:
>
> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
> index f5aaf6c83222..9769d76a62c4 100644
> --- a/arch/x86/include/asm/apic.h
> +++ b/arch/x86/include/asm/apic.h
> @@ -174,7 +174,7 @@ static inline void disable_local_APIC(void) { }
>  static inline void lapic_update_tsc_freq(void) { }
>  #endif /* !CONFIG_X86_LOCAL_APIC */
>
> -#ifdef CONFIG_X86_X2APIC
> +#if defined CONFIG_X86_X2APIC || defined CONFIG_KVM_GUEST

If CONFIG_X86_X2APIC is undefined and CONFIG_KVM_GUEST is defined,
there are undefined reference warning for x2apic_mode and check_x2apic
etc since they are extern variables and just define under
CONFIG_X86_X2APIC.

>  /*
>   * Make previous memory operations globally visible before
>   * sending the IPI through x2apic wrmsr. We need a serializing instruction or
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index edbbfc854e39..61cc6a5e3f44 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -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);
> +       native_apic_msr_eoi_write(APIC_EOI, APIC_EOI_ACK);
>  }

If -cpu host,-x2apic we should go through mmio when writes xapic EOI.

Regards,
Wanpeng Li

Re: [PATCH] x86/smp: Add irq_enter/exit() in smp_reschedule_interrupt()

[Wanpeng Li]

2016-10-18 8:01 GMT+08:00 Wanpeng Li <kernellwp@gmail.com>:
> 2016-10-17 18:08 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
> [...]
>>> > Something like the below avoids all that. Paravirt will still need fixing.
>>>
>>> kvm_guest_apic_eoi_write
>>>  -> native_apic_msr_write
>>
>> kvm_guest_apic_eoi_write can use native_apic_msr_eoi_write too:
>>
>> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
>> index f5aaf6c83222..9769d76a62c4 100644
>> --- a/arch/x86/include/asm/apic.h
>> +++ b/arch/x86/include/asm/apic.h
>> @@ -174,7 +174,7 @@ static inline void disable_local_APIC(void) { }
>>  static inline void lapic_update_tsc_freq(void) { }
>>  #endif /* !CONFIG_X86_LOCAL_APIC */
>>
>> -#ifdef CONFIG_X86_X2APIC
>> +#if defined CONFIG_X86_X2APIC || defined CONFIG_KVM_GUEST
>
> If CONFIG_X86_X2APIC is undefined and CONFIG_KVM_GUEST is defined,
> there are undefined reference warning for x2apic_mode and check_x2apic
> etc since they are extern variables and just define under
> CONFIG_X86_X2APIC.

[...]

>
> If -cpu host,-x2apic we should go through mmio when writes xapic EOI.

Any ideas? :)

Regards,
Wanpeng Li

Re: [PATCH] x86/smp: Add irq_enter/exit() in smp_reschedule_interrupt()

[Paolo Bonzini]

On 19/10/2016 16:01, Wanpeng Li wrote:
> 2016-10-18 8:01 GMT+08:00 Wanpeng Li <kernellwp@gmail.com>:
>> 2016-10-17 18:08 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>> [...]
>>>>> Something like the below avoids all that. Paravirt will still need fixing.
>>>>
>>>> kvm_guest_apic_eoi_write
>>>>  -> native_apic_msr_write
>>>
>>> kvm_guest_apic_eoi_write can use native_apic_msr_eoi_write too:
>>>
>>> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
>>> index f5aaf6c83222..9769d76a62c4 100644
>>> --- a/arch/x86/include/asm/apic.h
>>> +++ b/arch/x86/include/asm/apic.h
>>> @@ -174,7 +174,7 @@ static inline void disable_local_APIC(void) { }
>>>  static inline void lapic_update_tsc_freq(void) { }
>>>  #endif /* !CONFIG_X86_LOCAL_APIC */
>>>
>>> -#ifdef CONFIG_X86_X2APIC
>>> +#if defined CONFIG_X86_X2APIC || defined CONFIG_KVM_GUEST
>>
>> If CONFIG_X86_X2APIC is undefined and CONFIG_KVM_GUEST is defined,
>> there are undefined reference warning for x2apic_mode and check_x2apic
>> etc since they are extern variables and just define under
>> CONFIG_X86_X2APIC.
> 
> [...]
> 
>>
>> If -cpu host,-x2apic we should go through mmio when writes xapic EOI.
> 
> Any ideas? :)

Make apic_set_eoi_write return the old pointer?

Paolo

Re: [PATCH] x86/smp: Add irq_enter/exit() in smp_reschedule_interrupt()

[Wanpeng Li]

Cc Radim,
2016-10-19 22:10 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>
>
> On 19/10/2016 16:01, Wanpeng Li wrote:
>> 2016-10-18 8:01 GMT+08:00 Wanpeng Li <kernellwp@gmail.com>:
>>> 2016-10-17 18:08 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>>> [...]
>>>>>> Something like the below avoids all that. Paravirt will still need fixing.
>>>>>
>>>>> kvm_guest_apic_eoi_write
>>>>>  -> native_apic_msr_write
>>>>
>>>> kvm_guest_apic_eoi_write can use native_apic_msr_eoi_write too:
>>>>
>>>> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
>>>> index f5aaf6c83222..9769d76a62c4 100644
>>>> --- a/arch/x86/include/asm/apic.h
>>>> +++ b/arch/x86/include/asm/apic.h
>>>> @@ -174,7 +174,7 @@ static inline void disable_local_APIC(void) { }
>>>>  static inline void lapic_update_tsc_freq(void) { }
>>>>  #endif /* !CONFIG_X86_LOCAL_APIC */
>>>>
>>>> -#ifdef CONFIG_X86_X2APIC
>>>> +#if defined CONFIG_X86_X2APIC || defined CONFIG_KVM_GUEST
>>>
>>> If CONFIG_X86_X2APIC is undefined and CONFIG_KVM_GUEST is defined,
>>> there are undefined reference warning for x2apic_mode and check_x2apic
>>> etc since they are extern variables and just define under
>>> CONFIG_X86_X2APIC.
>>

This results in undefined reference warning.

>>
>>>
>>> If -cpu host,-x2apic we should go through mmio when writes xapic EOI.
>>
>> Any ideas? :)
>
> Make apic_set_eoi_write return the old pointer?

Could you elaborate? :)

Regards,
Wanpeng Li

Re: [PATCH] x86/smp: Add irq_enter/exit() in smp_reschedule_interrupt()

[Paolo Bonzini]

On 19/10/2016 16:30, Wanpeng Li wrote:
> Cc Radim,
> 2016-10-19 22:10 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>>
>>
>> On 19/10/2016 16:01, Wanpeng Li wrote:
>>> 2016-10-18 8:01 GMT+08:00 Wanpeng Li <kernellwp@gmail.com>:
>>>> 2016-10-17 18:08 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>>>> [...]
>>>>>>> Something like the below avoids all that. Paravirt will still need fixing.
>>>>>>
>>>>>> kvm_guest_apic_eoi_write
>>>>>>  -> native_apic_msr_write
>>>>>
>>>>> kvm_guest_apic_eoi_write can use native_apic_msr_eoi_write too:
>>>>>
>>>>> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
>>>>> index f5aaf6c83222..9769d76a62c4 100644
>>>>> --- a/arch/x86/include/asm/apic.h
>>>>> +++ b/arch/x86/include/asm/apic.h
>>>>> @@ -174,7 +174,7 @@ static inline void disable_local_APIC(void) { }
>>>>>  static inline void lapic_update_tsc_freq(void) { }
>>>>>  #endif /* !CONFIG_X86_LOCAL_APIC */
>>>>>
>>>>> -#ifdef CONFIG_X86_X2APIC
>>>>> +#if defined CONFIG_X86_X2APIC || defined CONFIG_KVM_GUEST
>>>>
>>>> If CONFIG_X86_X2APIC is undefined and CONFIG_KVM_GUEST is defined,
>>>> there are undefined reference warning for x2apic_mode and check_x2apic
>>>> etc since they are extern variables and just define under
>>>> CONFIG_X86_X2APIC.
>>>
> 
> This results in undefined reference warning.
> 
>>>
>>>>
>>>> If -cpu host,-x2apic we should go through mmio when writes xapic EOI.
>>>
>>> Any ideas? :)
>>
>> Make apic_set_eoi_write return the old pointer?
> 
> Could you elaborate? :)

No, that won't work due to how struct apic works and how
apic_set_eoi_write patches it.  One possibility then is to add a

        void (*native_eoi_write)(u32 reg, u32 v);

field to struct apic, and make apic_set_eoi_write set

	(*drv)->native_eoi_write = (*drv)->eoi_write;
	(*drv)->eoi_write = eoi_write;

Then kvm.c can call

	apic->native_eoi_write(reg, v);

Thanks,

Paolo

Re: [PATCH] x86/smp: Add irq_enter/exit() in smp_reschedule_interrupt()

[Wanpeng Li]

2016-10-24 22:32 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>
>
> On 19/10/2016 16:30, Wanpeng Li wrote:
>> Cc Radim,
>> 2016-10-19 22:10 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>>>
>>>
>>> On 19/10/2016 16:01, Wanpeng Li wrote:
>>>> 2016-10-18 8:01 GMT+08:00 Wanpeng Li <kernellwp@gmail.com>:
>>>>> 2016-10-17 18:08 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>>>>> [...]
>>>>>>>> Something like the below avoids all that. Paravirt will still need fixing.
>>>>>>>
>>>>>>> kvm_guest_apic_eoi_write
>>>>>>>  -> native_apic_msr_write
>>>>>>
>>>>>> kvm_guest_apic_eoi_write can use native_apic_msr_eoi_write too:
>>>>>>
>>>>>> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
>>>>>> index f5aaf6c83222..9769d76a62c4 100644
>>>>>> --- a/arch/x86/include/asm/apic.h
>>>>>> +++ b/arch/x86/include/asm/apic.h
>>>>>> @@ -174,7 +174,7 @@ static inline void disable_local_APIC(void) { }
>>>>>>  static inline void lapic_update_tsc_freq(void) { }
>>>>>>  #endif /* !CONFIG_X86_LOCAL_APIC */
>>>>>>
>>>>>> -#ifdef CONFIG_X86_X2APIC
>>>>>> +#if defined CONFIG_X86_X2APIC || defined CONFIG_KVM_GUEST
>>>>>
>>>>> If CONFIG_X86_X2APIC is undefined and CONFIG_KVM_GUEST is defined,
>>>>> there are undefined reference warning for x2apic_mode and check_x2apic
>>>>> etc since they are extern variables and just define under
>>>>> CONFIG_X86_X2APIC.
>>>>
>>
>> This results in undefined reference warning.
>>
>>>>
>>>>>
>>>>> If -cpu host,-x2apic we should go through mmio when writes xapic EOI.
>>>>
>>>> Any ideas? :)
>>>
>>> Make apic_set_eoi_write return the old pointer?
>>
>> Could you elaborate? :)
>
> No, that won't work due to how struct apic works and how
> apic_set_eoi_write patches it.  One possibility then is to add a
>
>         void (*native_eoi_write)(u32 reg, u32 v);
>
> field to struct apic, and make apic_set_eoi_write set
>
>         (*drv)->native_eoi_write = (*drv)->eoi_write;
>         (*drv)->eoi_write = eoi_write;
>
> Then kvm.c can call
>
>         apic->native_eoi_write(reg, v);

It works. Thanks Paolo. :)

Regards,
Wanpeng Li